Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH 1/3] Input: add STMicroelectronics FingerTip touchscreen driver
From: Dmitry Torokhov @ 2017-01-17 20:47 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Andi Shyti, Rob Herring, Chanwoo Choi, Javier Martinez Canillas,
	linux-input, devicetree, linux-kernel, linux-samsung-soc,
	Andi Shyti
In-Reply-To: <20170117184332.6ne4ercemll6qrbm@kozik-lap>

On Tue, Jan 17, 2017 at 08:43:32PM +0200, Krzysztof Kozlowski wrote:
> On Tue, Jan 17, 2017 at 10:54:39PM +0900, Andi Shyti wrote:
> > Add binding for the STMicroelectronics FingerTip (stmfts)
> > touchscreen driver.
> > 
> > Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> > ---
> >  .../bindings/input/touchscreen/st,stmfts.txt       | 43 ++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt b/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> > new file mode 100644
> > index 000000000000..788f4ba744db
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> > @@ -0,0 +1,43 @@
> > +* ST-Microelectronics FingerTip touchscreen controller
> > +
> > +The ST-Microelectronics FingerTip device provides a basic touchscreen
> > +functionality. Along with it the user can enable the touchkey which can work as
> > +a basic HOME and BACK key for phones.
> > +
> > +The driver supports also hovering as an absolute single touch event with x, y, z
> > +coordinates.
> > +
> > +Required properties:
> > +- compatible		: must be "st,stmfts"
> > +- reg			: I2C slave address, (e.g. 0x49)
> > +- interrupt-parent	: the phandle to the interrupt controller which provides
> > +			  the interrupt
> > +- interrupts		: interrupt specification
> > +- avdd-supply		: analogic power supply
> > +- vdd-supply		: power supply
> > +- touchscreen-size-x	: see touchscreen.txt
> > +- touchscreen-size-y	: see touchscreen.txt
> > +
> > +Optional properties:
> > +- touch-key-connected	: specifies whether the touchkey feature is connected
> 
> You are making it a generic property but it is specific to this device,
> so:
> 	st,touch-key-connected
> ?
> 
> > +- ledvdd-supply		: power supply to the touch key leds
> 
> Is this really optional? If yes... how it gets the power when not
> provided?

Also, is this really a regulator? Given you are testing whether it is on
or off to report LED state it feels to me it might be a GPIO pin, not
regulator...

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH v2 2/3] mmc: sh_mobile_sdhi: explain clock bindings
From: Wolfram Sang @ 2017-01-17 20:33 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven, devicetree, linux-mmc,
	linux-renesas-soc
In-Reply-To: <20170117195940.25092-3-chris.brandt@renesas.com>

On Tue, Jan 17, 2017 at 02:59:39PM -0500, Chris Brandt wrote:
> In the case of a single clock source, you don't need names. However,
> if the controller has 2 clock sources, you need to name them correctly
> so the driver can find the 2nd one.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> index a1650ed..258b98c 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> @@ -25,8 +25,29 @@ Required properties:
>  		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
>  		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
>  
> +- clocks: Most controllers only have 1 clock source per cahnnel. However, some

channel.

> +	  have 2. If 2 clocks are specified, you must name them as "core" and

Maybe "However, some have a second clock dedicated to card detection."?

^ permalink raw reply

* Re: [PATCH v2 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks
From: Wolfram Sang @ 2017-01-17 20:30 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven, devicetree, linux-mmc,
	linux-renesas-soc
In-Reply-To: <20170117195940.25092-2-chris.brandt@renesas.com>

Hi Chris,

> +	struct clk *clk2;

I'd vote for 'clk_cd' since we are explicitly requesting that clock for
it.

> +	ret = clk_prepare_enable(priv->clk2);
> +	if (ret < 0)
> +		return ret;

You need to disable clk when failing.

Regards,

   Wolfram


^ permalink raw reply

* Re: [PATCH 1/4] phy: sun4i-usb: support PHY0 on H3 in MUSB mode
From: Chen-Yu Tsai @ 2017-01-17 20:09 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Icenowy Zheng, Rob Herring, Chen-Yu Tsai, Kishon Vijay Abraham I,
	Greg Kroah-Hartman, Bin Liu,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
In-Reply-To: <20170117200658.gcrcxeanthdtwg26@lukather>

Hi,

On Wed, Jan 18, 2017 at 4:06 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Wed, Jan 18, 2017 at 12:57:08AM +0800, Icenowy Zheng wrote:
>>
>>
>> 17.01.2017, 16:06, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>> > On Tue, Jan 17, 2017 at 03:14:46AM +0800, Icenowy Zheng wrote:
>> >>  The PHY0 on H3 can be wired either to MUSB controller or OHCI/EHCI
>> >>  controller.
>> >>
>> >>  The original driver wired it to OHCI/EHCI controller; however, as the
>> >>  code to use PHY0 as OHCI/EHCI is missing, it makes the PHY fully
>> >>  unusable.
>> >>
>> >>  Rename the register (according to its function and the name in BSP
>> >>  driver), and remove the code which wires the PHY0 to OHCI/EHCI, as MUSB
>> >>  can support both peripheral and host mode (although the host mode of
>> >>  MUSB is buggy).
>> >
>> > Can you elaborate on that? What's wrong with it?
>>
>> The configuration is at bit 0 of register 0x20 in PHY.
>>
>> When the PHY is reseted, it defaults as MUSB mode.
>>
>> However, the original author of the H3 PHY code seems to be lack of
>> this knowledge (He named it PHY_UNK_H3), and changed the PHY to HCI
>> mode.
>>
>> I just removed the code that wires it to HCI mode, thus it will work
>> in MUSB mode, with my sun8i-h3-musb patch.
>
> I have no idea what you mean by MUSB mode.
>
> Do you mean that the previous code was only working in host mode, and
> now it only works in peripheral?

>From what I understand, with the H3, Allwinner has put a mux
in front of the MUSB controller. The mux can send the USB data
to/from the MUSB controller, or a standard EHCI/OHCI pair.
This register controls said mux.

This means we can use a proper USB host for host mode,
instead of the limited support in MUSB.

ChenYu

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

^ permalink raw reply

* Re: [PATCH 1/4] phy: sun4i-usb: support PHY0 on H3 in MUSB mode
From: Maxime Ripard @ 2017-01-17 20:06 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Rob Herring, Chen-Yu Tsai, Kishon Vijay Abraham I,
	Greg Kroah-Hartman, Bin Liu,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
In-Reply-To: <3866431484672228-4uohKiiZEDlxpj1cXAZ9Bg@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

On Wed, Jan 18, 2017 at 12:57:08AM +0800, Icenowy Zheng wrote:
> 
> 
> 17.01.2017, 16:06, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Tue, Jan 17, 2017 at 03:14:46AM +0800, Icenowy Zheng wrote:
> >>  The PHY0 on H3 can be wired either to MUSB controller or OHCI/EHCI
> >>  controller.
> >>
> >>  The original driver wired it to OHCI/EHCI controller; however, as the
> >>  code to use PHY0 as OHCI/EHCI is missing, it makes the PHY fully
> >>  unusable.
> >>
> >>  Rename the register (according to its function and the name in BSP
> >>  driver), and remove the code which wires the PHY0 to OHCI/EHCI, as MUSB
> >>  can support both peripheral and host mode (although the host mode of
> >>  MUSB is buggy).
> >
> > Can you elaborate on that? What's wrong with it?
> 
> The configuration is at bit 0 of register 0x20 in PHY.
> 
> When the PHY is reseted, it defaults as MUSB mode.
> 
> However, the original author of the H3 PHY code seems to be lack of
> this knowledge (He named it PHY_UNK_H3), and changed the PHY to HCI
> mode.
> 
> I just removed the code that wires it to HCI mode, thus it will work
> in MUSB mode, with my sun8i-h3-musb patch.

I have no idea what you mean by MUSB mode.

Do you mean that the previous code was only working in host mode, and
now it only works in peripheral?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* [PATCH v2 3/3] ARM: dts: r7s72100: update sdhi clock bindings
From: Chris Brandt @ 2017-01-17 19:59 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Chris Brandt
In-Reply-To: <20170117195940.25092-1-chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

The SDHI controller in the RZ/A1 has 2 clock sources per channel and both
need to be enabled/disabled for proper operation. This fixes the fact that
the define for R7S72100_CLK_SDHI1 was not correct to begin with (typo), and
that all 4 clock sources need to be defined an used.

Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v2:
* add missing clock sources instead of just fixing typo
---
 arch/arm/boot/dts/r7s72100.dtsi            | 17 ++++++++++++-----
 include/dt-bindings/clock/r7s72100-clock.h |  6 ++++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 3dd427d..8e7e516 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -153,9 +153,12 @@
 			#clock-cells = <1>;
 			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
 			reg = <0xfcfe0444 4>;
-			clocks = <&p1_clk>, <&p1_clk>;
-			clock-indices = <R7S72100_CLK_SDHI1 R7S72100_CLK_SDHI0>;
-			clock-output-names = "sdhi1", "sdhi0";
+			clocks = <&p1_clk>, <&p1_clk>, <&p1_clk>, <&p1_clk>;
+			clock-indices = <
+				R7S72100_CLK_SDHI00 R7S72100_CLK_SDHI01
+				R7S72100_CLK_SDHI10 R7S72100_CLK_SDHI11
+			>;
+			clock-output-names = "sdhi00", "sdhi01", "sdhi10", "sdhi11";
 		};
 	};
 
@@ -478,7 +481,9 @@
 			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
 			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 
-		clocks = <&mstp12_clks R7S72100_CLK_SDHI0>;
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
+			 <&mstp12_clks R7S72100_CLK_SDHI01>;
+		clock-names = "core", "carddetect";
 		cap-sd-highspeed;
 		cap-sdio-irq;
 		status = "disabled";
@@ -491,7 +496,9 @@
 			      GIC_SPI 274 IRQ_TYPE_LEVEL_HIGH
 			      GIC_SPI 275 IRQ_TYPE_LEVEL_HIGH>;
 
-		clocks = <&mstp12_clks R7S72100_CLK_SDHI1>;
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI10>,
+			 <&mstp12_clks R7S72100_CLK_SDHI11>;
+		clock-names = "core", "carddetect";
 		cap-sd-highspeed;
 		cap-sdio-irq;
 		status = "disabled";
diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h
index 29e01ed..f2d8428 100644
--- a/include/dt-bindings/clock/r7s72100-clock.h
+++ b/include/dt-bindings/clock/r7s72100-clock.h
@@ -45,7 +45,9 @@
 #define R7S72100_CLK_SPI4	3
 
 /* MSTP12 */
-#define R7S72100_CLK_SDHI0	3
-#define R7S72100_CLK_SDHI1	2
+#define R7S72100_CLK_SDHI00	3
+#define R7S72100_CLK_SDHI01	2
+#define R7S72100_CLK_SDHI10	1
+#define R7S72100_CLK_SDHI11	0
 
 #endif /* __DT_BINDINGS_CLOCK_R7S72100_H__ */
-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 2/3] mmc: sh_mobile_sdhi: explain clock bindings
From: Chris Brandt @ 2017-01-17 19:59 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven
  Cc: devicetree, linux-mmc, linux-renesas-soc, Chris Brandt
In-Reply-To: <20170117195940.25092-1-chris.brandt@renesas.com>

In the case of a single clock source, you don't need names. However,
if the controller has 2 clock sources, you need to name them correctly
so the driver can find the 2nd one.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index a1650ed..258b98c 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -25,8 +25,29 @@ Required properties:
 		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
 		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
 
+- clocks: Most controllers only have 1 clock source per cahnnel. However, some
+	  have 2. If 2 clocks are specified, you must name them as "core" and
+	  "carddetect". If the controller only has 1 clock, naming is not
+	  required.
+
 Optional properties:
 - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
 - pinctrl-names: should be "default", "state_uhs"
 - pinctrl-0: should contain default/high speed pin ctrl
 - pinctrl-1: should contain uhs mode pin ctrl
+
+Example showing 2 clocks:
+	sdhi0: sd@e804e000 {
+		compatible = "renesas,sdhi-r7s72100";
+		reg = <0xe804e000 0x100>;
+		interrupts = <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
+
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
+			 <&mstp12_clks R7S72100_CLK_SDHI01>;
+		clock-names = "core", "carddetect";
+		cap-sd-highspeed;
+		cap-sdio-irq;
+		status = "disabled";
+	};
-- 
2.10.1

^ permalink raw reply related

* [PATCH v2 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks
From: Chris Brandt @ 2017-01-17 19:59 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Chris Brandt
In-Reply-To: <20170117195940.25092-1-chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

Some controllers have 2 clock sources instead of 1, so they both need
to be turned on/off.

Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 59db14b..5ad5744 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -143,6 +143,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
 
 struct sh_mobile_sdhi {
 	struct clk *clk;
+	struct clk *clk2;
 	struct tmio_mmc_data mmc_data;
 	struct tmio_mmc_dma dma_priv;
 	struct pinctrl *pinctrl;
@@ -190,6 +191,10 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(priv->clk2);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * The clock driver may not know what maximum frequency
 	 * actually works, so it should be set with the max-frequency
@@ -255,6 +260,8 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 
 	clk_disable_unprepare(priv->clk);
+	if (priv->clk2)
+		clk_disable_unprepare(priv->clk2);
 }
 
 static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc)
@@ -572,6 +579,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		goto eprobe;
 	}
 
+	priv->clk2 = devm_clk_get(&pdev->dev, "carddetect");
+	if (IS_ERR(priv->clk2))
+		priv->clk2 = NULL;
+
 	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (!IS_ERR(priv->pinctrl)) {
 		priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 0/3] mmc: sh_mobile_sdhi: fix missing r7s72100 clocks
From: Chris Brandt @ 2017-01-17 19:59 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Mark Rutland, Simon Horman,
	Wolfram Sang, Geert Uytterhoeven
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Chris Brandt

At first this started out as a simple typo fix, until I realized
that the SDHI in the RZ/A1 has 2 clocks per channel and both need
to be turned on/off.

This patch series adds the ability to specify 2 clocks instead of
just 1, and does so for the RZ/A1 r7s72100.

This patch has been tested on an RZ/A1 RSK board. Card detect works
fine as well as bind/unbind.



Chris Brandt (3):
  mmc: sh_mobile_sdhi: add support for 2 clocks
  mmc: sh_mobile_sdhi: explain clock bindings
  ARM: dts: r7s72100: update sdhi clock bindings

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi                    | 17 ++++++++++++-----
 drivers/mmc/host/sh_mobile_sdhi.c                  | 11 +++++++++++
 include/dt-bindings/clock/r7s72100-clock.h         |  6 ++++--
 4 files changed, 48 insertions(+), 7 deletions(-)

-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v9 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-17 19:37 UTC (permalink / raw)
  To: M'boumba Cedric Madianga
  Cc: wsa, robh+dt, mcoquelin.stm32, alexandre.torgue, linus.walleij,
	patrice.chotard, linux, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel
In-Reply-To: <1484666821-20551-3-git-send-email-cedric.madianga@gmail.com>

Hello,

On Tue, Jan 17, 2017 at 04:26:58PM +0100, M'boumba Cedric Madianga wrote:
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 freq = DIV_ROUND_UP(i2c_dev->parent_rate, HZ_TO_MHZ);
> +	u32 trise;
> +
> +	/*
> +	 * These bits must be programmed with the maximum SCL rise time given in
> +	 * the I2C bus specification, incremented by 1.
> +	 *
> +	 * In standard mode, the maximum allowed SCL rise time is 1000 ns.
> +	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> +	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> +	 * programmed with 09h.(1000 ns / 125 ns = 8 + 1)

	* programmed with 0x9. (1000 ns / 125 ns = 8)

> +	 * So, for I2C standard mode TRISE = FREQ[5:0] + 1
> +	 *
> +	 * In fast mode, the maximum allowed SCL rise time is 300 ns.
> +	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> +	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> +	 * programmed with 03h.(300 ns / 125 ns = 2 + 1)

as above s/03h/0x3/; s/.(/. (/; s/+ 1//;

> +	 * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
> +	 */
> +	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> +		trise = freq + 1;
> +	else
> +		trise = freq * 300 / 1000 + 1;

I'd use

	* 3 / 10

without downside and lesser chance to overflow.

> +
> +	writel_relaxed(STM32F4_I2C_TRISE_VALUE(trise),
> +		       i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 val;
> +	u32 ccr = 0;
> +
> +	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
> +		/*
> +		 * In standard mode:
> +		 * t_scl_high = t_scl_low = CCR * I2C parent clk period
> +		 * So to reach 100 kHz, we have:
> +		 * CCR = I2C parent rate / 100 kHz >> 1
> +		 *
> +		 * For example with parent rate = 2 MHz:
> +		 * CCR = 2000000 / (100000 << 1) = 10
> +		 * t_scl_high = t_scl_low = 10 * (1 / 2000000) = 5000 ns
> +		 * t_scl_high + t_scl_low = 10000 ns so 100 kHz is reached
> +		 */
> +		val = i2c_dev->parent_rate / (100000 << 1);
> +	} else {
> +		/*
> +		 * In fast mode, we compute CCR with duty = 0 as with low
> +		 * frequencies we are not able to reach 400 kHz.
> +		 * In that case:
> +		 * t_scl_high = CCR * I2C parent clk period
> +		 * t_scl_low = 2 * CCR * I2C parent clk period
> +		 * So, CCR = I2C parent rate / (400 kHz * 3)
> +		 *
> +		 * For example with parent rate = 6 MHz:
> +		 * CCR = 6000000 / (400000 * 3) = 5
> +		 * t_scl_high = 5 * (1 / 6000000) = 833 ns > 600 ns
> +		 * t_scl_low = 2 * 5 * (1 / 6000000) = 1667 ns > 1300 ns
> +		 * t_scl_high + t_scl_low = 2500 ns so 400 kHz is reached
> +		 */

Huh, that's surprising. So you don't use DUTY any more. I found two
hints in the manual that contradict here:

	f_{PCLK1} must be at least 2 MHz to achieve Sm mode I2C
	frequencies. It must be at least 4 MHz to achieve Fm mode I2C
	frequencies. It must be a multiple of 10MHz to reach the
	400 kHz maximum I2C Fm mode clock.

and

	[...]
	If DUTY = 1: (to reach 400 kHz)

Strange.

> +		val = DIV_ROUND_UP(i2c_dev->parent_rate, 400000 * 3);

the manual reads:

	The minimum allowed value is 0x04, except in FAST DUTY mode
	where the minimum allowed value is 0x01

You don't check for that, right?
CCR is 11 bits wide. A comment confirming that this cannot overflow
would be nice.

+		/* select Fast Mode */
> +		ccr |= STM32F4_I2C_CCR_FS;

I didn't check the rest of the code, so let's assume it's good :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH] ARM: dts: r7s72100: fix sdhi clock define
From: Wolfram Sang @ 2017-01-17 19:23 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Geert Uytterhoeven, Simon Horman, Magnus Damm, Rob Herring,
	Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Linux-Renesas, Wolfram Sang
In-Reply-To: <SG2PR06MB11653D8853CB565A629B782E8A7C0-ESzmfEwOt/xoAsOJh7vwSm0DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 141 bytes --]


> So, should sh_mobile_sdhi_remove() be changed to call sh_mobile_sdhi_clk_disable()?

Give me a minute, I already did a patch for this :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* Re: [PATCH 0/5] meson-gx: reset RGMII PHYs and configure TX delay
From: Jerome Brunet @ 2017-01-17 19:23 UTC (permalink / raw)
  To: Martin Blumenstingl,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	khilman-rdvid1DuHRBWk0Htik3J/w, carlo-KA+7E9HrN00dnm+yROfE0A
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8,
	catalin.marinas-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

On Sat, 2016-12-03 at 00:47 +0100, Martin Blumenstingl wrote:
> This partially fixes the 1000Mbit/s ethernet TX throughput issues (on
> networks which are not affected by the EEE problem, as reported here:
> [1]).
> The actual problem for the TX throughput issues was that the TX delay
> was applied twice:
> - once "accidentally" by the PHY (this was fixed with [2])
> - once by the MAC because there was a hardcoded TX delay (of 2ns),
>   this will be configurable with the changes from [0]
> 
> These are the dts changes which belong to my other series (in v2
> these patches were part of the other series, upon request of the
> net maintainers I have split the .dts changes into their own series
> so
> we are able to take both through different trees):
> "[PATCH net-next v3 0/2] stmmac: dwmac-meson8b: configurable
> RGMII TX delay": [0].
> Thus this series depends on the ACK for the binding changes in the
> other series!
> 
> I based these changes on my other series "[PATCH v2 0/2] GXL and GXM
> SCPI improvements": [3]
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2016-December/
> 001834.html
> [1] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/
> 001607.html
> [2] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/
> 001707.html
> [3] http://lists.infradead.org/pipermail/linux-amlogic/2016-December/
> 001831.html
> 
> Martin Blumenstingl (5):
>   ARM64: dts: meson-gx: move the MDIO node to meson-gx

Sorry for the late reply, I've only been able to test this yesterday.

With "snps,dwmac-mdio" provided in meson-gx.dtsi, the mdio_node is
defined in stmmac_mdio_register and auto-detection of the PHY is
disabled for all meson-gx boards.

I wonder if this is desirable ? or maybe this something we could fix in
stmmac ? (perform auto-detect the mdio bus is provided without a PHY)

Also, I think bisect is broken between patch 1 and patch 4: The PHY of
some boards won't be detected between these patches. Should we squash
them ? 

>   ARM64: dts: meson-gxbb-odroidc2: add reset for the ethernet PHY
>   ARM64: dts: meson-gxbb-p20x: add reset for the ethernet PHY
>   ARM64: dts: meson-gxbb-vega-s95: add reset for the ethernet PHY
>   ARM64: dts: amlogic: add the ethernet TX delay configuration
> 

Last remark, about the use of ethernet-phy-idXXXX.XXXX in the odroid
and the vega: Isn't it better to let phylib do the autodetection of the
phy id ?

If we want to specify the id in DT, we should probably add it for the
Micrel PHY of the p200 as well, for consistency.

>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi            |  6 ++++++
>  arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts  | 17
> +++++++++++++++++
>  arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi     | 17
> +++++++++++++++++
>  arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 17
> +++++++++++++++++
>  arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts |  2 ++
>  arch/arm64/boot/dts/amlogic/meson-gxl.dtsi           |  6 ------
>  arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts  |  2 ++
>  arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts  |  2 ++
>  8 files changed, 63 insertions(+), 6 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 3/5] ARM64: dts: meson-gxbb-p20x: add reset for the ethernet PHY
From: Jerome Brunet @ 2017-01-17 19:22 UTC (permalink / raw)
  To: Martin Blumenstingl,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	khilman-rdvid1DuHRBWk0Htik3J/w, carlo-KA+7E9HrN00dnm+yROfE0A
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8,
	catalin.marinas-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161202234739.22929-4-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

On Sat, 2016-12-03 at 00:47 +0100, Martin Blumenstingl wrote:
> This resets the ethernet PHY during boot to get the PHY into a
> "clean"
> state. While here also specify the phy-handle of the ethmac node to
> make the PHY configuration similar to the one we have on GXL devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23myhRSP0FMvGiw@public.gmane.org
> m>
> Tested-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 15
> +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> index 203be28..2abc553 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> @@ -134,10 +134,25 @@
>  	pinctrl-names = "default";
>  };
>  
> +&mdio0 {
> +	ethernet_phy0: ethernet-phy@0 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0>;

Sorry for the late reply.
I just tried on the p200 and this patch (serie) breaks the network on
it. The PHY is not detected anymore.

>From the KSZ9031 Datasheet : "PHY Address 0h is supported as the unique
PHY address only; it is not supported as the broadcast PHY address
[...]"

So we can't just use the broadcast address here:
reg should be <3>.

> +	};
> +};
> +
>  &ethmac {
>  	status = "okay";
>  	pinctrl-0 = <&eth_rgmii_pins>;
>  	pinctrl-names = "default";
> +
> +	phy-handle = <&ethernet_phy0>;
> +
> +	snps,reset-gpio = <&gpio GPIOZ_14 0>;
> +	snps,reset-delays-us = <0 10000 1000000>;
> +	snps,reset-active-low;
> +
> +	phy-mode = "rgmii";

We can't define this in p20x. actually the p201 uses an rmii.
I have not idea about gpio reset, or the phy address for the p201.

I suppose it would be better to move this to meson-gxbb_p200.dts

I don't know if anybody has a p201, but until we can confirm a working
Ethernet configuration, we should probably drop it for the p201

Of course the problem was already there before this patch ...

>  };
>  
>  &ir {
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
From: Akshay Bhat @ 2017-01-17 19:22 UTC (permalink / raw)
  To: wg, mkl
  Cc: linux-can, netdev, devicetree, linux-kernel, Akshay Bhat,
	Akshay Bhat
In-Reply-To: <1484680922-25813-1-git-send-email-akshay.bhat@timesys.com>

This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.

Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do

Signed-off-by: Akshay Bhat <nodeax@gmail.com>
---

v1 -> v2:
Address comments from Marc Kleine-Budde:
- use u8 instead of uint8_t
- alphabetically sort Makefile and Kconfig
- copy over copyright information from mcp251x
- use single space after each marco
- add missing HI3110_ namespace in defines
- remove magic number for IDE & SRR bits
- simplify logic to populate extended CAN ID
- remove unused parameters in hi3110_setup function
- remove redundant frame->can_dlc length check
- simplify error handling in hi3110_open function
Address comments from Julia Lawall:
- remove unnecessary semicolon after while loop in hi3110_can_ist

 drivers/net/can/spi/Kconfig  |    6 +
 drivers/net/can/spi/Makefile |    1 +
 drivers/net/can/spi/hi311x.c | 1069 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1076 insertions(+)
 create mode 100644 drivers/net/can/spi/hi311x.c

diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
index 148cae5..8f2e0dd 100644
--- a/drivers/net/can/spi/Kconfig
+++ b/drivers/net/can/spi/Kconfig
@@ -1,6 +1,12 @@
 menu "CAN SPI interfaces"
 	depends on SPI
 
+config CAN_HI311X
+	tristate "Holt HI311x SPI CAN controllers"
+	depends on CAN_DEV && SPI && HAS_DMA
+	---help---
+	  Driver for the Holt HI311x SPI CAN controllers.
+
 config CAN_MCP251X
 	tristate "Microchip MCP251x SPI CAN controllers"
 	depends on HAS_DMA
diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
index 0e86040..f59fa37 100644
--- a/drivers/net/can/spi/Makefile
+++ b/drivers/net/can/spi/Makefile
@@ -3,4 +3,5 @@
 #
 
 
+obj-$(CONFIG_CAN_HI311X)	+= hi311x.o
 obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
new file mode 100644
index 0000000..cccfe2d
--- /dev/null
+++ b/drivers/net/can/spi/hi311x.c
@@ -0,0 +1,1069 @@
+/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
+ *
+ * Copyright(C) Timesys Corporation 2016
+ *
+ * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
+ * Copyright 2009 Christian Pellegrin EVOL S.r.l.
+ * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved.
+ * Copyright 2006 Arcom Control Systems Ltd.
+ *
+ * Based on CAN bus driver for the CCAN controller written by
+ * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
+ * - Simon Kallweit, intefo AG
+ * Copyright 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/can/core.h>
+#include <linux/can/dev.h>
+#include <linux/can/led.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/freezer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/uaccess.h>
+
+#define HI3110_MASTER_RESET 0x56
+#define HI3110_READ_CTRL0 0xD2
+#define HI3110_READ_CTRL1 0xD4
+#define HI3110_READ_STATF 0xE2
+#define HI3110_WRITE_CTRL0 0x14
+#define HI3110_WRITE_CTRL1 0x16
+#define HI3110_WRITE_INTE 0x1C
+#define HI3110_WRITE_BTR0 0x18
+#define HI3110_WRITE_BTR1 0x1A
+#define HI3110_READ_BTR0 0xD6
+#define HI3110_READ_BTR1 0xD8
+#define HI3110_READ_INTF 0xDE
+#define HI3110_READ_ERR 0xDC
+#define HI3110_READ_FIFO_WOTIME 0x48
+#define HI3110_WRITE_FIFO 0x12
+#define HI3110_READ_MESSTAT 0xDA
+#define HI3110_READ_TEC 0xEC
+
+#define HI3110_CTRL0_MODE_MASK (7 << 5)
+#define HI3110_CTRL0_NORMAL_MODE (0 << 5)
+#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5)
+#define HI3110_CTRL0_MONITOR_MODE (2 << 5)
+#define HI3110_CTRL0_SLEEP_MODE (3 << 5)
+#define HI3110_CTRL0_INIT_MODE (4 << 5)
+
+#define HI3110_CTRL1_TXEN BIT(7)
+
+#define HI3110_INT_RXTMP BIT(7)
+#define HI3110_INT_RXFIFO BIT(6)
+#define HI3110_INT_TXCPLT BIT(5)
+#define HI3110_INT_BUSERR BIT(4)
+#define HI3110_INT_MCHG BIT(3)
+#define HI3110_INT_WAKEUP BIT(2)
+#define HI3110_INT_F1MESS BIT(1)
+#define HI3110_INT_F0MESS BIT(0)
+
+#define HI3110_ERR_BUSOFF BIT(7)
+#define HI3110_ERR_TXERRP BIT(6)
+#define HI3110_ERR_RXERRP BIT(5)
+#define HI3110_ERR_BITERR BIT(4)
+#define HI3110_ERR_FRMERR BIT(3)
+#define HI3110_ERR_CRCERR BIT(2)
+#define HI3110_ERR_ACKERR BIT(1)
+#define HI3110_ERR_STUFERR BIT(0)
+#define HI3110_ERR_PROTOCOL_MASK (0x1F)
+
+#define HI3110_STAT_RXFMTY BIT(1)
+
+#define HI3110_BTR0_SJW_SHIFT 6
+#define HI3110_BTR0_BRP_SHIFT 0
+
+#define HI3110_BTR1_SAMP_3PERBIT (1 << 7)
+#define HI3110_BTR1_SAMP_1PERBIT (0 << 7)
+#define HI3110_BTR1_TSEG2_SHIFT 4
+#define HI3110_BTR1_TSEG1_SHIFT 0
+
+#define HI3110_FIFO_WOTIME_TAG_OFF 0
+#define HI3110_FIFO_WOTIME_ID_OFF 1
+#define HI3110_FIFO_WOTIME_DLC_OFF 5
+#define HI3110_FIFO_WOTIME_DAT_OFF 6
+
+#define HI3110_FIFO_WOTIME_TAG_IDE BIT(7)
+#define HI3110_FIFO_WOTIME_ID_RTR BIT(0)
+
+#define HI3110_FIFO_TAG_OFF 0
+#define HI3110_FIFO_ID_OFF 1
+#define HI3110_FIFO_STD_DLC_OFF 3
+#define HI3110_FIFO_STD_DATA_OFF 4
+#define HI3110_FIFO_EXT_DLC_OFF 5
+#define HI3110_FIFO_EXT_DATA_OFF 6
+
+#define HI3110_CAN_FRAME_MAX_DATA_LEN 8
+#define HI3110_RX_BUF_LEN 15
+#define HI3110_TX_STD_BUF_LEN 12
+#define HI3110_TX_EXT_BUF_LEN 14
+#define HI3110_CAN_FRAME_MAX_BITS 128
+#define HI3110_EFF_FLAGS 0x18 /* IDE + SRR */
+
+#define HI3110_TX_ECHO_SKB_MAX 1
+
+#define HI3110_OST_DELAY_MS (10)
+
+#define DEVICE_NAME "hi3110"
+
+static int hi3110_enable_dma = 1; /* Enable SPI DMA. Default: 1 (On) */
+module_param(hi3110_enable_dma, int, 0444);
+MODULE_PARM_DESC(hi3110_enable_dma, "Enable SPI DMA. Default: 1 (On)");
+
+static const struct can_bittiming_const hi3110_bittiming_const = {
+	.name = DEVICE_NAME,
+	.tseg1_min = 2,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 64,
+	.brp_inc = 1,
+};
+
+enum hi3110_model {
+	CAN_HI3110_HI3110 = 0x3110,
+};
+
+struct hi3110_priv {
+	struct can_priv can;
+	struct net_device *net;
+	struct spi_device *spi;
+	enum hi3110_model model;
+
+	struct mutex hi3110_lock; /* SPI device lock */
+
+	u8 *spi_tx_buf;
+	u8 *spi_rx_buf;
+	dma_addr_t spi_tx_dma;
+	dma_addr_t spi_rx_dma;
+
+	struct sk_buff *tx_skb;
+	int tx_len;
+
+	struct workqueue_struct *wq;
+	struct work_struct tx_work;
+	struct work_struct restart_work;
+
+	int force_quit;
+	int after_suspend;
+#define HI3110_AFTER_SUSPEND_UP 1
+#define HI3110_AFTER_SUSPEND_DOWN 2
+#define HI3110_AFTER_SUSPEND_POWER 4
+#define HI3110_AFTER_SUSPEND_RESTART 8
+	int restart_tx;
+	struct regulator *power;
+	struct regulator *transceiver;
+	struct clk *clk;
+};
+
+static void hi3110_clean(struct net_device *net)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+
+	if (priv->tx_skb || priv->tx_len)
+		net->stats.tx_errors++;
+	if (priv->tx_skb)
+		dev_kfree_skb(priv->tx_skb);
+	if (priv->tx_len)
+		can_free_echo_skb(priv->net, 0);
+	priv->tx_skb = NULL;
+	priv->tx_len = 0;
+}
+
+/* Note about handling of error return of hi3110_spi_trans: accessing
+ * registers via SPI is not really different conceptually than using
+ * normal I/O assembler instructions, although it's much more
+ * complicated from a practical POV. So it's not advisable to always
+ * check the return value of this function. Imagine that every
+ * read{b,l}, write{b,l} and friends would be bracketed in "if ( < 0)
+ * error();", it would be a great mess (well there are some situation
+ * when exception handling C++ like could be useful after all). So we
+ * just check that transfers are OK at the beginning of our
+ * conversation with the chip and to avoid doing really nasty things
+ * (like injecting bogus packets in the network stack).
+ */
+static int hi3110_spi_trans(struct spi_device *spi, int len)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	struct spi_transfer t = {
+		.tx_buf = priv->spi_tx_buf,
+		.rx_buf = priv->spi_rx_buf,
+		.len = len,
+		.cs_change = 0,
+	};
+	struct spi_message m;
+	int ret;
+
+	spi_message_init(&m);
+
+	if (hi3110_enable_dma) {
+		t.tx_dma = priv->spi_tx_dma;
+		t.rx_dma = priv->spi_rx_dma;
+		m.is_dma_mapped = 1;
+	}
+
+	spi_message_add_tail(&t, &m);
+
+	ret = spi_sync(spi, &m);
+
+	if (ret)
+		dev_err(&spi->dev, "spi transfer failed: ret = %d\n", ret);
+	return ret;
+}
+
+static u8 hi3110_cmd(struct spi_device *spi, u8 command)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+	priv->spi_tx_buf[0] = command;
+	dev_dbg(&spi->dev, "hi3110_cmd: %02X\n", command);
+
+	return hi3110_spi_trans(spi, 1);
+}
+
+static u8 hi3110_read(struct spi_device *spi, u8 command)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	u8 val = 0;
+
+	priv->spi_tx_buf[0] = command;
+	hi3110_spi_trans(spi, 2);
+	val = priv->spi_rx_buf[1];
+	dev_dbg(&spi->dev, "hi3110_read: %02X, %02X\n", command, val);
+
+	return val;
+}
+
+static void hi3110_write(struct spi_device *spi, u8 reg, u8 val)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+	priv->spi_tx_buf[0] = reg;
+	priv->spi_tx_buf[1] = val;
+	dev_dbg(&spi->dev, "hi3110_write: %02X, %02X\n", reg, val);
+
+	hi3110_spi_trans(spi, 2);
+}
+
+static void hi3110_hw_tx_frame(struct spi_device *spi, u8 *buf, int len)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+	priv->spi_tx_buf[0] = HI3110_WRITE_FIFO;
+	memcpy(priv->spi_tx_buf + 1, buf, len);
+	hi3110_spi_trans(spi, len + 1);
+}
+
+static void hi3110_hw_tx(struct spi_device *spi, struct can_frame *frame)
+{
+	u8 buf[HI3110_TX_EXT_BUF_LEN];
+
+	buf[HI3110_FIFO_TAG_OFF] = 0;
+
+	if (frame->can_id & CAN_EFF_FLAG) {
+		/* Extended frame */
+		buf[HI3110_FIFO_ID_OFF] = (frame->can_id & CAN_EFF_MASK) >> 21;
+		buf[HI3110_FIFO_ID_OFF + 1] =
+			(((frame->can_id & CAN_EFF_MASK) >> 13) & 0xe0) |
+			HI3110_EFF_FLAGS |
+			(((frame->can_id & CAN_EFF_MASK) >> 15) & 0x07);
+		buf[HI3110_FIFO_ID_OFF + 2] =
+			(frame->can_id & CAN_EFF_MASK) >> 7;
+		buf[HI3110_FIFO_ID_OFF + 3] =
+			((frame->can_id & CAN_EFF_MASK) << 1) |
+			((frame->can_id & CAN_RTR_FLAG) ? 1 : 0);
+
+		buf[HI3110_FIFO_EXT_DLC_OFF] = frame->can_dlc;
+
+		memcpy(buf + HI3110_FIFO_EXT_DATA_OFF,
+		       frame->data, frame->can_dlc);
+
+		hi3110_hw_tx_frame(spi, buf, HI3110_TX_EXT_BUF_LEN -
+				   (HI3110_CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
+	} else {
+		/* Standard frame */
+		buf[HI3110_FIFO_ID_OFF] =   (frame->can_id & CAN_SFF_MASK) >> 3;
+		buf[HI3110_FIFO_ID_OFF + 1] =
+			((frame->can_id & CAN_SFF_MASK) << 5) |
+			((frame->can_id & CAN_RTR_FLAG) ? (1 << 4) : 0);
+
+		buf[HI3110_FIFO_STD_DLC_OFF] = frame->can_dlc;
+
+		memcpy(buf + HI3110_FIFO_STD_DATA_OFF,
+		       frame->data, frame->can_dlc);
+
+		hi3110_hw_tx_frame(spi, buf, HI3110_TX_STD_BUF_LEN -
+				   (HI3110_CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
+	}
+}
+
+static void hi3110_hw_rx_frame(struct spi_device *spi, u8 *buf)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+	priv->spi_tx_buf[0] = HI3110_READ_FIFO_WOTIME;
+	hi3110_spi_trans(spi, HI3110_RX_BUF_LEN);
+	memcpy(buf, priv->spi_rx_buf + 1, HI3110_RX_BUF_LEN - 1);
+}
+
+static void hi3110_hw_rx(struct spi_device *spi)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	struct sk_buff *skb;
+	struct can_frame *frame;
+	u8 buf[HI3110_RX_BUF_LEN - 1];
+
+	skb = alloc_can_skb(priv->net, &frame);
+	if (!skb) {
+		dev_err(&spi->dev, "cannot allocate RX skb\n");
+		priv->net->stats.rx_dropped++;
+		return;
+	}
+
+	hi3110_hw_rx_frame(spi, buf);
+	if (buf[HI3110_FIFO_WOTIME_TAG_OFF] & HI3110_FIFO_WOTIME_TAG_IDE) {
+		/* IDE is recessive (1), indicating extended 29-bit frame */
+		frame->can_id = CAN_EFF_FLAG;
+		frame->can_id |=
+		 (buf[HI3110_FIFO_WOTIME_ID_OFF] << 21) |
+		 (((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5) << 18) |
+		 ((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0x07) << 15) |
+		 (buf[HI3110_FIFO_WOTIME_ID_OFF + 2] << 7) |
+		 (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] >> 1);
+	} else {
+		/* IDE is dominant (0), frame indicating standard 11-bit */
+		frame->can_id =
+			(buf[HI3110_FIFO_WOTIME_ID_OFF] << 3) |
+			((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5);
+	}
+
+	if (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] & HI3110_FIFO_WOTIME_ID_RTR) {
+		/* RTR is recessive (1), indicating remote request frame */
+		frame->can_id |= CAN_RTR_FLAG;
+	}
+
+	/* Data length */
+	frame->can_dlc = get_can_dlc(buf[HI3110_FIFO_WOTIME_DLC_OFF] & 0x0F);
+	memcpy(frame->data, buf + HI3110_FIFO_WOTIME_DAT_OFF, frame->can_dlc);
+
+	priv->net->stats.rx_packets++;
+	priv->net->stats.rx_bytes += frame->can_dlc;
+
+	can_led_event(priv->net, CAN_LED_EVENT_RX);
+
+	netif_rx_ni(skb);
+}
+
+static void hi3110_hw_sleep(struct spi_device *spi)
+{
+	hi3110_write(spi, HI3110_WRITE_CTRL0, HI3110_CTRL0_SLEEP_MODE);
+}
+
+static netdev_tx_t hi3110_hard_start_xmit(struct sk_buff *skb,
+					  struct net_device *net)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+	struct spi_device *spi = priv->spi;
+
+	if (priv->tx_skb || priv->tx_len) {
+		dev_warn(&spi->dev, "hard_xmit called while tx busy\n");
+		return NETDEV_TX_BUSY;
+	}
+
+	if (can_dropped_invalid_skb(net, skb))
+		return NETDEV_TX_OK;
+
+	netif_stop_queue(net);
+	priv->tx_skb = skb;
+	queue_work(priv->wq, &priv->tx_work);
+
+	return NETDEV_TX_OK;
+}
+
+static int hi3110_do_set_mode(struct net_device *net, enum can_mode mode)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+
+	switch (mode) {
+	case CAN_MODE_START:
+		hi3110_clean(net);
+		/* We have to delay work since SPI I/O may sleep */
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+		priv->restart_tx = 1;
+		if (priv->can.restart_ms == 0)
+			priv->after_suspend = HI3110_AFTER_SUSPEND_RESTART;
+		queue_work(priv->wq, &priv->restart_work);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int hi3110_set_normal_mode(struct spi_device *spi)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	u8 reg;
+
+	hi3110_write(spi, HI3110_WRITE_INTE, HI3110_INT_BUSERR |
+		     HI3110_INT_RXFIFO | HI3110_INT_TXCPLT);
+
+	/* Enable TX */
+	hi3110_write(spi, HI3110_WRITE_CTRL1, HI3110_CTRL1_TXEN);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
+		/* Put device into loopback mode */
+		hi3110_write(spi, HI3110_WRITE_CTRL0,
+			     HI3110_CTRL0_LOOPBACK_MODE);
+	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+		/* Put device into listen-only mode */
+		hi3110_write(spi, HI3110_WRITE_CTRL0,
+			     HI3110_CTRL0_MONITOR_MODE);
+	} else {
+		/* Put device into normal mode */
+		hi3110_write(spi, HI3110_WRITE_CTRL0,
+			     HI3110_CTRL0_NORMAL_MODE);
+
+		/* Wait for the device to enter normal mode */
+		mdelay(HI3110_OST_DELAY_MS);
+		reg = hi3110_read(spi, HI3110_READ_CTRL0);
+		if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_NORMAL_MODE)
+			return -EBUSY;
+	}
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	return 0;
+}
+
+static int hi3110_do_set_bittiming(struct net_device *net)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	struct spi_device *spi = priv->spi;
+
+	hi3110_write(spi, HI3110_WRITE_BTR0,
+		     ((bt->sjw - 1) << HI3110_BTR0_SJW_SHIFT) |
+		     ((bt->brp - 1) << HI3110_BTR0_BRP_SHIFT));
+
+	hi3110_write(spi, HI3110_WRITE_BTR1,
+		     (priv->can.ctrlmode &
+		     CAN_CTRLMODE_3_SAMPLES ?
+		     HI3110_BTR1_SAMP_3PERBIT : HI3110_BTR1_SAMP_1PERBIT) |
+		     ((bt->phase_seg1 + bt->prop_seg - 1)
+		     << HI3110_BTR1_TSEG1_SHIFT) |
+		     ((bt->phase_seg2 - 1) << HI3110_BTR1_TSEG2_SHIFT));
+
+	dev_dbg(&spi->dev, "BT: 0x%02x 0x%02x\n",
+		hi3110_read(spi, HI3110_READ_BTR0),
+		hi3110_read(spi, HI3110_READ_BTR1));
+
+	return 0;
+}
+
+static int hi3110_setup(struct net_device *net)
+{
+	hi3110_do_set_bittiming(net);
+	return 0;
+}
+
+static int hi3110_hw_reset(struct spi_device *spi)
+{
+	u8 reg;
+	int ret;
+
+	/* Wait for oscillator startup timer after power up */
+	mdelay(HI3110_OST_DELAY_MS);
+
+	ret = hi3110_cmd(spi, HI3110_MASTER_RESET);
+	if (ret)
+		return ret;
+
+	/* Wait for oscillator startup timer after reset */
+	mdelay(HI3110_OST_DELAY_MS);
+
+	reg = hi3110_read(spi, HI3110_READ_CTRL0);
+	if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_INIT_MODE)
+		return -ENODEV;
+
+	/* As per the datasheet it appears the error flags are
+	 * not cleared on reset. Explicitly clear them by performing a read
+	 */
+	hi3110_read(spi, HI3110_READ_ERR);
+
+	return 0;
+}
+
+static int hi3110_hw_probe(struct spi_device *spi)
+{
+	u8 statf;
+
+	hi3110_hw_reset(spi);
+
+	/* Confirm correct operation by checking against reset values
+	 * in datasheet
+	 */
+	statf = hi3110_read(spi, HI3110_READ_STATF);
+
+	dev_dbg(&spi->dev, "statf: %02X\n", statf);
+
+	if (statf != 0x82)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int hi3110_power_enable(struct regulator *reg, int enable)
+{
+	if (IS_ERR_OR_NULL(reg))
+		return 0;
+
+	if (enable)
+		return regulator_enable(reg);
+	else
+		return regulator_disable(reg);
+}
+
+static int hi3110_stop(struct net_device *net)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+	struct spi_device *spi = priv->spi;
+
+	close_candev(net);
+
+	priv->force_quit = 1;
+	free_irq(spi->irq, priv);
+	destroy_workqueue(priv->wq);
+	priv->wq = NULL;
+
+	mutex_lock(&priv->hi3110_lock);
+
+	/* Disable transmit, interrupts and clear flags */
+	hi3110_write(spi, HI3110_WRITE_CTRL1, 0x0);
+	hi3110_write(spi, HI3110_WRITE_INTE, 0x0);
+	hi3110_read(spi, HI3110_READ_INTF);
+
+	hi3110_clean(net);
+
+	hi3110_hw_sleep(spi);
+
+	hi3110_power_enable(priv->transceiver, 0);
+
+	priv->can.state = CAN_STATE_STOPPED;
+
+	mutex_unlock(&priv->hi3110_lock);
+
+	can_led_event(net, CAN_LED_EVENT_STOP);
+
+	return 0;
+}
+
+static void hi3110_error_skb(struct net_device *net, int can_id,
+			     int data1, int data2)
+{
+	struct sk_buff *skb;
+	struct can_frame *frame;
+
+	skb = alloc_can_err_skb(net, &frame);
+	if (skb) {
+		frame->can_id |= can_id;
+		frame->data[1] = data1;
+		frame->data[2] = data2;
+		netif_rx_ni(skb);
+	} else {
+		netdev_err(net, "cannot allocate error skb\n");
+	}
+}
+
+static void hi3110_tx_work_handler(struct work_struct *ws)
+{
+	struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
+						 tx_work);
+	struct spi_device *spi = priv->spi;
+	struct net_device *net = priv->net;
+	struct can_frame *frame;
+
+	mutex_lock(&priv->hi3110_lock);
+	if (priv->tx_skb) {
+		if (priv->can.state == CAN_STATE_BUS_OFF) {
+			hi3110_clean(net);
+		} else {
+			frame = (struct can_frame *)priv->tx_skb->data;
+			hi3110_hw_tx(spi, frame);
+			priv->tx_len = 1 + frame->can_dlc;
+			can_put_echo_skb(priv->tx_skb, net, 0);
+			priv->tx_skb = NULL;
+		}
+	}
+	mutex_unlock(&priv->hi3110_lock);
+}
+
+static void hi3110_restart_work_handler(struct work_struct *ws)
+{
+	struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
+						 restart_work);
+	struct spi_device *spi = priv->spi;
+	struct net_device *net = priv->net;
+
+	mutex_lock(&priv->hi3110_lock);
+	if (priv->after_suspend) {
+		hi3110_hw_reset(spi);
+		hi3110_setup(net);
+		if (priv->after_suspend & HI3110_AFTER_SUSPEND_RESTART) {
+			hi3110_set_normal_mode(spi);
+		} else if (priv->after_suspend & HI3110_AFTER_SUSPEND_UP) {
+			netif_device_attach(net);
+			hi3110_clean(net);
+			hi3110_set_normal_mode(spi);
+			netif_wake_queue(net);
+		} else {
+			hi3110_hw_sleep(spi);
+		}
+		priv->after_suspend = 0;
+		priv->force_quit = 0;
+	}
+
+	if (priv->restart_tx) {
+		priv->restart_tx = 0;
+		hi3110_clean(net);
+		netif_wake_queue(net);
+		hi3110_error_skb(net, CAN_ERR_RESTARTED, 0, 0);
+	}
+	mutex_unlock(&priv->hi3110_lock);
+}
+
+static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
+{
+	struct hi3110_priv *priv = dev_id;
+	struct spi_device *spi = priv->spi;
+	struct net_device *net = priv->net;
+
+	mutex_lock(&priv->hi3110_lock);
+
+	while (!priv->force_quit) {
+		enum can_state new_state;
+		u8 intf;
+		u8 eflag;
+		int can_id = 0, data1 = 0, data2 = 0;
+
+		while (!(HI3110_STAT_RXFMTY &
+			hi3110_read(spi, HI3110_READ_STATF))) {
+			hi3110_hw_rx(spi);
+		}
+
+		intf = hi3110_read(spi, HI3110_READ_INTF);
+		eflag = hi3110_read(spi, HI3110_READ_ERR);
+		/* Update can state */
+		if (eflag & HI3110_ERR_BUSOFF) {
+			new_state = CAN_STATE_BUS_OFF;
+			can_id |= CAN_ERR_BUSOFF;
+		} else if (eflag & HI3110_ERR_TXERRP) {
+			new_state = CAN_STATE_ERROR_PASSIVE;
+			can_id |= CAN_ERR_CRTL;
+			data1 |= CAN_ERR_CRTL_TX_PASSIVE;
+		} else if (eflag & HI3110_ERR_RXERRP) {
+			new_state = CAN_STATE_ERROR_PASSIVE;
+			can_id |= CAN_ERR_CRTL;
+			data1 |= CAN_ERR_CRTL_RX_PASSIVE;
+		} else {
+			new_state = CAN_STATE_ERROR_ACTIVE;
+		}
+
+		/* Check for protocol errors */
+		if (eflag & HI3110_ERR_PROTOCOL_MASK) {
+			can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+			priv->can.can_stats.bus_error++;
+			priv->net->stats.rx_errors++;
+			if (eflag & HI3110_ERR_BITERR)
+				data2 |= CAN_ERR_PROT_BIT;
+			else if (eflag & HI3110_ERR_FRMERR)
+				data2 |= CAN_ERR_PROT_FORM;
+			else if (eflag & HI3110_ERR_STUFERR)
+				data2 |= CAN_ERR_PROT_STUFF;
+			else
+				data2 |= CAN_ERR_PROT_UNSPEC;
+		}
+
+		/* Update can state statistics */
+		switch (priv->can.state) {
+		case CAN_STATE_ERROR_ACTIVE:
+			if (new_state >= CAN_STATE_ERROR_WARNING &&
+			    new_state <= CAN_STATE_BUS_OFF)
+				priv->can.can_stats.error_warning++;
+		/* fallthrough */
+		case CAN_STATE_ERROR_WARNING:
+			if (new_state >= CAN_STATE_ERROR_PASSIVE &&
+			    new_state <= CAN_STATE_BUS_OFF)
+				priv->can.can_stats.error_passive++;
+			break;
+		default:
+			break;
+		}
+		priv->can.state = new_state;
+
+		if (intf & HI3110_INT_BUSERR) {
+			/* Note: HI3110 Does report overflow errors */
+			hi3110_error_skb(net, can_id, data1, data2);
+		}
+
+		if (priv->can.state == CAN_STATE_BUS_OFF) {
+			if (priv->can.restart_ms == 0) {
+				priv->force_quit = 1;
+				priv->can.can_stats.bus_off++;
+				can_bus_off(net);
+				hi3110_hw_sleep(spi);
+				break;
+			}
+		}
+
+		if (intf == 0)
+			break;
+
+		if (intf & HI3110_INT_TXCPLT) {
+			net->stats.tx_packets++;
+			net->stats.tx_bytes += priv->tx_len - 1;
+			can_led_event(net, CAN_LED_EVENT_TX);
+			if (priv->tx_len) {
+				can_get_echo_skb(net, 0);
+				priv->tx_len = 0;
+			}
+			netif_wake_queue(net);
+		}
+	}
+	mutex_unlock(&priv->hi3110_lock);
+	return IRQ_HANDLED;
+}
+
+static int hi3110_open(struct net_device *net)
+{
+	struct hi3110_priv *priv = netdev_priv(net);
+	struct spi_device *spi = priv->spi;
+	unsigned long flags = IRQF_ONESHOT | IRQF_TRIGGER_RISING;
+	int ret;
+
+	ret = open_candev(net);
+	if (ret) {
+		dev_err(&spi->dev, "unable to set initial baudrate!\n");
+		return ret;
+	}
+
+	mutex_lock(&priv->hi3110_lock);
+	hi3110_power_enable(priv->transceiver, 1);
+
+	priv->force_quit = 0;
+	priv->tx_skb = NULL;
+	priv->tx_len = 0;
+
+	ret = request_threaded_irq(spi->irq, NULL, hi3110_can_ist,
+				   flags, DEVICE_NAME, priv);
+	if (ret) {
+		dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
+		goto out_close;
+	}
+
+	priv->wq = alloc_workqueue("hi3110_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
+			   0);
+	INIT_WORK(&priv->tx_work, hi3110_tx_work_handler);
+	INIT_WORK(&priv->restart_work, hi3110_restart_work_handler);
+
+	ret = hi3110_hw_reset(spi);
+	if (ret)
+		goto out_free_irq;
+
+	ret = hi3110_setup(net);
+	if (ret)
+		goto out_free_irq;
+
+	ret = hi3110_set_normal_mode(spi);
+	if (ret)
+		goto out_free_irq;
+
+	can_led_event(net, CAN_LED_EVENT_OPEN);
+	netif_wake_queue(net);
+	mutex_unlock(&priv->hi3110_lock);
+
+	return 0;
+
+out_free_irq:
+	free_irq(spi->irq, priv);
+	hi3110_hw_sleep(spi);
+
+out_close:
+	hi3110_power_enable(priv->transceiver, 0);
+	close_candev(net);
+	mutex_unlock(&priv->hi3110_lock);
+	return ret;
+}
+
+static const struct net_device_ops hi3110_netdev_ops = {
+	.ndo_open = hi3110_open,
+	.ndo_stop = hi3110_stop,
+	.ndo_start_xmit = hi3110_hard_start_xmit,
+};
+
+static const struct of_device_id hi3110_of_match[] = {
+	{
+		.compatible	= "holt,hi3110",
+		.data		= (void *)CAN_HI3110_HI3110,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hi3110_of_match);
+
+static const struct spi_device_id hi3110_id_table[] = {
+	{
+		.name		= "hi3110",
+		.driver_data	= (kernel_ulong_t)CAN_HI3110_HI3110,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, hi3110_id_table);
+
+static int hi3110_can_probe(struct spi_device *spi)
+{
+	const struct of_device_id *of_id = of_match_device(hi3110_of_match,
+							   &spi->dev);
+	struct net_device *net;
+	struct hi3110_priv *priv;
+	struct clk *clk;
+	int freq, ret;
+
+	clk = devm_clk_get(&spi->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(&spi->dev, "no CAN clock source defined\n");
+		return PTR_ERR(clk);
+	}
+	freq = clk_get_rate(clk);
+
+	/* Sanity check */
+	if (freq > 40000000)
+		return -ERANGE;
+
+	/* Allocate can/net device */
+	net = alloc_candev(sizeof(struct hi3110_priv), HI3110_TX_ECHO_SKB_MAX);
+	if (!net)
+		return -ENOMEM;
+
+	if (!IS_ERR(clk)) {
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			goto out_free;
+	}
+
+	net->netdev_ops = &hi3110_netdev_ops;
+	net->flags |= IFF_ECHO;
+
+	priv = netdev_priv(net);
+	priv->can.bittiming_const = &hi3110_bittiming_const;
+	priv->can.do_set_mode = hi3110_do_set_mode;
+	priv->can.clock.freq = freq / 2;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
+		CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY;
+	if (of_id)
+		priv->model = (enum hi3110_model)of_id->data;
+	else
+		priv->model = spi_get_device_id(spi)->driver_data;
+	priv->net = net;
+	priv->clk = clk;
+
+	spi_set_drvdata(spi, priv);
+
+	/* Configure the SPI bus */
+	spi->bits_per_word = 8;
+	ret = spi_setup(spi);
+	if (ret)
+		goto out_clk;
+
+	priv->power = devm_regulator_get_optional(&spi->dev, "vdd");
+	priv->transceiver = devm_regulator_get_optional(&spi->dev, "xceiver");
+	if ((PTR_ERR(priv->power) == -EPROBE_DEFER) ||
+	    (PTR_ERR(priv->transceiver) == -EPROBE_DEFER)) {
+		ret = -EPROBE_DEFER;
+		goto out_clk;
+	}
+
+	ret = hi3110_power_enable(priv->power, 1);
+	if (ret)
+		goto out_clk;
+
+	priv->spi = spi;
+	mutex_init(&priv->hi3110_lock);
+
+	/* If requested, allocate DMA buffers */
+	if (hi3110_enable_dma) {
+		spi->dev.coherent_dma_mask = ~0;
+
+		/* Minimum coherent DMA allocation is PAGE_SIZE, so allocate
+		 * that much and share it between Tx and Rx DMA buffers.
+		 */
+		priv->spi_tx_buf = dmam_alloc_coherent(&spi->dev,
+						      PAGE_SIZE,
+						      &priv->spi_tx_dma,
+						      GFP_DMA);
+
+		if (priv->spi_tx_buf) {
+			priv->spi_rx_buf = (priv->spi_tx_buf + (PAGE_SIZE / 2));
+			priv->spi_rx_dma = (dma_addr_t)(priv->spi_tx_dma +
+							(PAGE_SIZE / 2));
+		} else {
+			/* Fall back to non-DMA */
+			hi3110_enable_dma = 0;
+		}
+	}
+
+	/* Allocate non-DMA buffers */
+	if (!hi3110_enable_dma) {
+		priv->spi_tx_buf = devm_kzalloc(&spi->dev, HI3110_RX_BUF_LEN,
+				GFP_KERNEL);
+		if (!priv->spi_tx_buf) {
+			ret = -ENOMEM;
+			goto error_probe;
+		}
+		priv->spi_rx_buf = devm_kzalloc(&spi->dev, HI3110_RX_BUF_LEN,
+				GFP_KERNEL);
+
+		if (!priv->spi_rx_buf) {
+			ret = -ENOMEM;
+			goto error_probe;
+		}
+	}
+
+	SET_NETDEV_DEV(net, &spi->dev);
+
+	ret = hi3110_hw_probe(spi);
+	if (ret) {
+		if (ret == -ENODEV)
+			dev_err(&spi->dev, "Cannot initialize %x. Wrong wiring?\n",
+				priv->model);
+		goto error_probe;
+	}
+	hi3110_hw_sleep(spi);
+
+	ret = register_candev(net);
+	if (ret)
+		goto error_probe;
+
+	devm_can_led_init(net);
+	netdev_info(net, "%x successfully initialized.\n", priv->model);
+
+	return 0;
+
+error_probe:
+	hi3110_power_enable(priv->power, 0);
+
+out_clk:
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
+
+out_free:
+	free_candev(net);
+
+	dev_err(&spi->dev, "Probe failed, err=%d\n", -ret);
+	return ret;
+}
+
+static int hi3110_can_remove(struct spi_device *spi)
+{
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	struct net_device *net = priv->net;
+
+	unregister_candev(net);
+
+	hi3110_power_enable(priv->power, 0);
+
+	if (!IS_ERR(priv->clk))
+		clk_disable_unprepare(priv->clk);
+
+	free_candev(net);
+
+	return 0;
+}
+
+static int __maybe_unused hi3110_can_suspend(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+	struct net_device *net = priv->net;
+
+	priv->force_quit = 1;
+	disable_irq(spi->irq);
+
+	/* Note: at this point neither IST nor workqueues are running.
+	 * open/stop cannot be called anyway so locking is not needed
+	 */
+	if (netif_running(net)) {
+		netif_device_detach(net);
+
+		hi3110_hw_sleep(spi);
+		hi3110_power_enable(priv->transceiver, 0);
+		priv->after_suspend = HI3110_AFTER_SUSPEND_UP;
+	} else {
+		priv->after_suspend = HI3110_AFTER_SUSPEND_DOWN;
+	}
+
+	if (!IS_ERR_OR_NULL(priv->power)) {
+		regulator_disable(priv->power);
+		priv->after_suspend |= HI3110_AFTER_SUSPEND_POWER;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused hi3110_can_resume(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+	if (priv->after_suspend & HI3110_AFTER_SUSPEND_POWER)
+		hi3110_power_enable(priv->power, 1);
+
+	if (priv->after_suspend & HI3110_AFTER_SUSPEND_UP) {
+		hi3110_power_enable(priv->transceiver, 1);
+		queue_work(priv->wq, &priv->restart_work);
+	} else {
+		priv->after_suspend = 0;
+	}
+
+	priv->force_quit = 0;
+	enable_irq(spi->irq);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(hi3110_can_pm_ops, hi3110_can_suspend,
+	hi3110_can_resume);
+
+static struct spi_driver hi3110_can_driver = {
+	.driver = {
+		.name = DEVICE_NAME,
+		.of_match_table = hi3110_of_match,
+		.pm = &hi3110_can_pm_ops,
+	},
+	.id_table = hi3110_id_table,
+	.probe = hi3110_can_probe,
+	.remove = hi3110_can_remove,
+};
+
+module_spi_driver(hi3110_can_driver);
+
+MODULE_AUTHOR("Akshay Bhat <akshay.bhat@timesys.com>");
+MODULE_AUTHOR("Casey Fitzpatrick <casey.fitzpatrick@timesys.com>");
+MODULE_DESCRIPTION("Holt HI-3110 CAN driver");
+MODULE_LICENSE("GPL v2");
-- 
2.8.1

^ permalink raw reply related

* [PATCH v2 1/2] can: holt_hi311x: document device tree bindings
From: Akshay Bhat @ 2017-01-17 19:22 UTC (permalink / raw)
  To: wg, mkl
  Cc: linux-can, netdev, devicetree, linux-kernel, Akshay Bhat,
	Akshay Bhat

Document the HOLT HI-311x CAN device tree bindings.

Signed-off-by: Akshay Bhat <nodeax@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---

v1 -> v2:
- No changes

 .../devicetree/bindings/net/can/holt_hi311x.txt    | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/holt_hi311x.txt

diff --git a/Documentation/devicetree/bindings/net/can/holt_hi311x.txt b/Documentation/devicetree/bindings/net/can/holt_hi311x.txt
new file mode 100644
index 0000000..23aa94e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/holt_hi311x.txt
@@ -0,0 +1,24 @@
+* Holt HI-311X stand-alone CAN controller device tree bindings
+
+Required properties:
+ - compatible: Should be one of the following:
+   - "holt,hi3110" for HI-3110
+ - reg: SPI chip select.
+ - clocks: The clock feeding the CAN controller.
+ - interrupt-parent: The parent interrupt controller.
+ - interrupts: Should contain IRQ line for the CAN controller.
+
+Optional properties:
+ - vdd-supply: Regulator that powers the CAN controller.
+ - xceiver-supply: Regulator that powers the CAN transceiver.
+
+Example:
+	can0: can@1 {
+		compatible = "holt,hi3110";
+		reg = <1>;
+		clocks = <&clk32m>;
+		interrupt-parent = <&gpio4>;
+		interrupts = <13 IRQ_TYPE_EDGE_RISING>;
+		vdd-supply = <&reg5v0>;
+		xceiver-supply = <&reg5v0>;
+	};
-- 
2.8.1


^ permalink raw reply related

* Re: [PATCH 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
From: Akshay Bhat @ 2017-01-17 19:11 UTC (permalink / raw)
  To: Marc Kleine-Budde, wg, robh+dt
  Cc: mark.rutland, linux-can, netdev, devicetree, linux-kernel,
	Akshay Bhat
In-Reply-To: <7ad30b57-b97f-466e-3792-a433e4a2a2ca@pengutronix.de>


[-- Attachment #1.1: Type: text/plain, Size: 36043 bytes --]

Marc,

On 01/03/2017 10:31 AM, Marc Kleine-Budde wrote:
> On 11/14/2016 06:55 PM, Akshay Bhat wrote:
>> This patch adds support for the Holt HI-311x CAN controller. The HI311x
>> CAN controller is capable of transmitting and receiving standard data
>> frames, extended data frames and remote frames. The HI311x interfaces
>> with the host over SPI.
> 
> Don't use uint8_t and similar in the kernel, please use u8 instead.
> 

Will fix in V2

>>
>> Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do
>>
>> Signed-off-by: Akshay Bhat <nodeax@gmail.com>
>> ---
>>  drivers/net/can/spi/Kconfig  |    6 +
>>  drivers/net/can/spi/Makefile |    1 +
>>  drivers/net/can/spi/hi311x.c | 1071 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 1078 insertions(+)
>>  create mode 100644 drivers/net/can/spi/hi311x.c
>>
>> diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
>> index 148cae5..9eb1bb1 100644
>> --- a/drivers/net/can/spi/Kconfig
>> +++ b/drivers/net/can/spi/Kconfig
>> @@ -7,4 +7,10 @@ config CAN_MCP251X
>>  	---help---
>>  	  Driver for the Microchip MCP251x SPI CAN controllers.
>>  
>> +config CAN_HI311X
>> +	tristate "Holt HI311x SPI CAN controllers"
>> +	depends on CAN_DEV && SPI && HAS_DMA
>> +	---help---
>> +	  Driver for the Holt HI311x SPI CAN controllers.
>> +
>>  endmenu
>> diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
>> index 0e86040..eac7c3a 100644
>> --- a/drivers/net/can/spi/Makefile
>> +++ b/drivers/net/can/spi/Makefile
>> @@ -4,3 +4,4 @@
>>  
>>  
>>  obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
>> +obj-$(CONFIG_CAN_HI311X)	+= hi311x.o
> 
> Please keep sorted alphabetically. Same for the Kconfig.
> 

Will fix in V2

>> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
>> new file mode 100644
>> index 0000000..1020166
>> --- /dev/null
>> +++ b/drivers/net/can/spi/hi311x.c
>> @@ -0,0 +1,1071 @@
>> +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
>> + *
>> + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
> 
> You might want to add the copyright of the mcp authors.
> 

Will add in V2

>> + *
>> + * Copyright(C) Timesys Corporation 2016
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/can/core.h>
>> +#include <linux/can/dev.h>
>> +#include <linux/can/led.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/freezer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/uaccess.h>
>> +
> 
> Please use just a single space after each macro.
> 
>                               VVVVVVVV

Will fix in V2.

>> +#define HI3110_MASTER_RESET        0x56
>> +#define HI3110_READ_CTRL0          0xD2
>> +#define HI3110_READ_CTRL1          0xD4
>> +#define HI3110_READ_STATF          0xE2
>> +#define HI3110_WRITE_CTRL0         0x14
>> +#define HI3110_WRITE_CTRL1         0x16
>> +#define HI3110_WRITE_INTE          0x1C
>> +#define HI3110_WRITE_BTR0          0x18
>> +#define HI3110_WRITE_BTR1          0x1A
>> +#define HI3110_READ_BTR0           0xD6
>> +#define HI3110_READ_BTR1           0xD8
>> +#define HI3110_READ_INTF           0xDE
>> +#define HI3110_READ_ERR            0xDC
>> +#define HI3110_READ_FIFO_WOTIME    0x48
>> +#define HI3110_WRITE_FIFO          0x12
>> +#define HI3110_READ_MESSTAT        0xDA
>> +#define HI3110_READ_TEC            0xEC
>> +
>> +#define HI3110_CTRL0_MODE_MASK     (7 << 5)
>> +#define HI3110_CTRL0_NORMAL_MODE   (0 << 5)
>> +#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5)
>> +#define HI3110_CTRL0_MONITOR_MODE  (2 << 5)
>> +#define HI3110_CTRL0_SLEEP_MODE    (3 << 5)
>> +#define HI3110_CTRL0_INIT_MODE     (4 << 5)
>> +
>> +#define HI3110_CTRL1_TXEN          BIT(7)
>> +
>> +#define HI3110_INT_RXTMP           BIT(7)
>> +#define HI3110_INT_RXFIFO          BIT(6)
>> +#define HI3110_INT_TXCPLT          BIT(5)
>> +#define HI3110_INT_BUSERR          BIT(4)
>> +#define HI3110_INT_MCHG            BIT(3)
>> +#define HI3110_INT_WAKEUP          BIT(2)
>> +#define HI3110_INT_F1MESS          BIT(1)
>> +#define HI3110_INT_F0MESS          BIT(0)
>> +
>> +#define HI3110_ERR_BUSOFF          BIT(7)
>> +#define HI3110_ERR_TXERRP          BIT(6)
>> +#define HI3110_ERR_RXERRP          BIT(5)
>> +#define HI3110_ERR_BITERR          BIT(4)
>> +#define HI3110_ERR_FRMERR          BIT(3)
>> +#define HI3110_ERR_CRCERR          BIT(2)
>> +#define HI3110_ERR_ACKERR          BIT(1)
>> +#define HI3110_ERR_STUFERR         BIT(0)
>> +#define HI3110_ERR_PROTOCOL_MASK   (0x1F)
>> +
>> +#define HI3110_STAT_RXFMTY         BIT(1)
>> +
>> +#define HI3110_BTR0_SJW_SHIFT      6
>> +#define HI3110_BTR0_BRP_SHIFT      0
>> +
>> +#define HI3110_BTR1_SAMP_3PERBIT   (1 << 7)
>> +#define HI3110_BTR1_SAMP_1PERBIT   (0 << 7)
>> +#define HI3110_BTR1_TSEG2_SHIFT    4
>> +#define HI3110_BTR1_TSEG1_SHIFT    0
>> +
>> +#define HI3110_FIFO_WOTIME_TAG_OFF 0
>> +#define HI3110_FIFO_WOTIME_ID_OFF  1
>> +#define HI3110_FIFO_WOTIME_DLC_OFF 5
>> +#define HI3110_FIFO_WOTIME_DAT_OFF 6
>> +
>> +#define HI3110_FIFO_WOTIME_TAG_IDE BIT(7)
>> +#define HI3110_FIFO_WOTIME_ID_RTR  BIT(0)
>> +
>> +#define HI3110_FIFO_TAG_OFF        0
>> +#define HI3110_FIFO_ID_OFF         1
>> +#define HI3110_FIFO_STD_DLC_OFF    3
>> +#define HI3110_FIFO_STD_DATA_OFF   4
>> +#define HI3110_FIFO_EXT_DLC_OFF    5
>> +#define HI3110_FIFO_EXT_DATA_OFF   6
>> +
> 
> Please add the already used HI3110_ namespace to these defines, too.
> 

Will fix in V2.

>> +#define CAN_FRAME_MAX_DATA_LEN 8
>> +#define RX_BUF_LEN             15
>> +#define TX_STD_BUF_LEN         12
>> +#define TX_EXT_BUF_LEN         14
>> +#define CAN_FRAME_MAX_BITS     128
>> +
>> +#define TX_ECHO_SKB_MAX	1
>> +
>> +#define HI3110_OST_DELAY_MS (10)
>> +
>> +#define DEVICE_NAME "hi3110"
>> +
>> +static int hi3110_enable_dma = 1; /* Enable SPI DMA. Default: 1 (On) */
>> +module_param(hi3110_enable_dma, int, 0444);
>> +MODULE_PARM_DESC(hi3110_enable_dma, "Enable SPI DMA. Default: 1 (On)");
>> +
>> +static const struct can_bittiming_const hi3110_bittiming_const = {
>> +	.name = DEVICE_NAME,
>> +	.tseg1_min = 2,
>> +	.tseg1_max = 16,
>> +	.tseg2_min = 2,
>> +	.tseg2_max = 8,
>> +	.sjw_max = 4,
>> +	.brp_min = 1,
>> +	.brp_max = 64,
>> +	.brp_inc = 1,
>> +};
>> +
>> +enum hi3110_model {
>> +	CAN_HI3110_HI3110	= 0x3110,
>                          ^^^^^^^
> single space here, too

Will fix in V2

>> +};
>> +
>> +struct hi3110_priv {
>> +	struct can_priv	   can;
>                        ^^^^
> here too

Will fix in V2

>> +	struct net_device *net;
>> +	struct spi_device *spi;
>> +	enum hi3110_model model;
>> +
>> +	struct mutex hi3110_lock; /* SPI device lock */
>> +
>> +	u8 *spi_tx_buf;
>> +	u8 *spi_rx_buf;
>> +	dma_addr_t spi_tx_dma;
>> +	dma_addr_t spi_rx_dma;
>> +
>> +	struct sk_buff *tx_skb;
>> +	int tx_len;
>> +
>> +	struct workqueue_struct *wq;
>> +	struct work_struct tx_work;
>> +	struct work_struct restart_work;
>> +
>> +	int force_quit;
>> +	int after_suspend;
> 
> Please add the already used HI3110_ namespace to these defines, too.
> 

Will fix in V2

>> +#define AFTER_SUSPEND_UP 1
>> +#define AFTER_SUSPEND_DOWN 2
>> +#define AFTER_SUSPEND_POWER 4
>> +#define AFTER_SUSPEND_RESTART 8
> 
>> +	int restart_tx;
>> +	struct regulator *power;
>> +	struct regulator *transceiver;
>> +	struct clk *clk;
>> +};
>> +
>> +static void hi3110_clean(struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +
>> +	if (priv->tx_skb || priv->tx_len)
>> +		net->stats.tx_errors++;
>> +	if (priv->tx_skb)
>> +		dev_kfree_skb(priv->tx_skb);
>> +	if (priv->tx_len)
>> +		can_free_echo_skb(priv->net, 0);
>> +	priv->tx_skb = NULL;
>> +	priv->tx_len = 0;
>> +}
>> +
>> +/* Note about handling of error return of hi3110_spi_trans: accessing
>> + * registers via SPI is not really different conceptually than using
>> + * normal I/O assembler instructions, although it's much more
>> + * complicated from a practical POV. So it's not advisable to always
>> + * check the return value of this function. Imagine that every
>> + * read{b,l}, write{b,l} and friends would be bracketed in "if ( < 0)
>> + * error();", it would be a great mess (well there are some situation
>> + * when exception handling C++ like could be useful after all). So we
>> + * just check that transfers are OK at the beginning of our
>> + * conversation with the chip and to avoid doing really nasty things
>> + * (like injecting bogus packets in the network stack).
>> + */
>> +static int hi3110_spi_trans(struct spi_device *spi, int len)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	struct spi_transfer t = {
>> +		.tx_buf = priv->spi_tx_buf,
>> +		.rx_buf = priv->spi_rx_buf,
>> +		.len = len,
>> +		.cs_change = 0,
>> +	};
>> +	struct spi_message m;
>> +	int ret;
>> +
>> +	spi_message_init(&m);
>> +
>> +	if (hi3110_enable_dma) {
>> +		t.tx_dma = priv->spi_tx_dma;
>> +		t.rx_dma = priv->spi_rx_dma;
>> +		m.is_dma_mapped = 1;
>> +	}
>> +
>> +	spi_message_add_tail(&t, &m);
>> +
>> +	ret = spi_sync(spi, &m);
>> +
>> +	if (ret)
>> +		dev_err(&spi->dev, "spi transfer failed: ret = %d\n", ret);
>> +	return ret;
>> +}
>> +
>> +static u8 hi3110_cmd(struct spi_device *spi, uint8_t command)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +
>> +	priv->spi_tx_buf[0] = command;
>> +	dev_dbg(&spi->dev, "hi3110_cmd: %02X\n", command);
>> +
>> +	return hi3110_spi_trans(spi, 1);
>> +}
>> +
>> +static u8 hi3110_read(struct spi_device *spi, uint8_t command)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	u8 val = 0;
>> +
>> +	priv->spi_tx_buf[0] = command;
>> +	hi3110_spi_trans(spi, 2);
>> +	val = priv->spi_rx_buf[1];
>> +	dev_dbg(&spi->dev, "hi3110_read: %02X, %02X\n", command, val);
>> +
>> +	return val;
>> +}
>> +
>> +static void hi3110_write(struct spi_device *spi, u8 reg, uint8_t val)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +
>> +	priv->spi_tx_buf[0] = reg;
>> +	priv->spi_tx_buf[1] = val;
>> +	dev_dbg(&spi->dev, "hi3110_write: %02X, %02X\n", reg, val);
>> +
>> +	hi3110_spi_trans(spi, 2);
>> +}
>> +
>> +static void hi3110_hw_tx_frame(struct spi_device *spi, u8 *buf, int len)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +
>> +	priv->spi_tx_buf[0] = HI3110_WRITE_FIFO;
>> +	memcpy(priv->spi_tx_buf + 1, buf, len);
>> +	hi3110_spi_trans(spi, len + 1);
>> +}
>> +
>> +static void hi3110_hw_tx(struct spi_device *spi, struct can_frame *frame)
>> +{
>> +	u8 buf[TX_EXT_BUF_LEN];
>> +
>> +	buf[HI3110_FIFO_TAG_OFF] = 0;
>> +
>> +	if (frame->can_id & CAN_EFF_FLAG) {
>> +		/* Extended frame */
>> +		buf[HI3110_FIFO_ID_OFF] = (frame->can_id & CAN_EFF_MASK) >> 21;
>> +		buf[HI3110_FIFO_ID_OFF + 1] =
>> +			((((frame->can_id & CAN_EFF_MASK) >> 18) & 0x07) << 5) |
> 
> Why do you first shift down then up?
> 

Simplified the logic in V2, shift down then up not needed

>> +			0x18 | /* Recessive SRR and IDE */
> 
> Can you add a define for the 0x18?
> 

Will add a define in V2

>> +			(((frame->can_id & CAN_EFF_MASK) >> 15) & 0x07);
>> +		buf[HI3110_FIFO_ID_OFF + 2] =
>> +			(frame->can_id & CAN_EFF_MASK) >> 7;
>> +		buf[HI3110_FIFO_ID_OFF + 3] =
>> +			((frame->can_id & CAN_EFF_MASK) << 1) |
>> +			((frame->can_id & CAN_RTR_FLAG) ? 1 : 0);
>> +
>> +		buf[HI3110_FIFO_EXT_DLC_OFF] = frame->can_dlc;
>> +
>> +		memcpy(buf + HI3110_FIFO_EXT_DATA_OFF,
>> +		       frame->data, frame->can_dlc);
>> +
>> +		hi3110_hw_tx_frame(spi, buf, TX_EXT_BUF_LEN -
>> +				   (CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
>> +	} else {
>> +		/* Standard frame */
>> +		buf[HI3110_FIFO_ID_OFF] =   (frame->can_id & CAN_SFF_MASK) >> 3;
>> +		buf[HI3110_FIFO_ID_OFF + 1] =
>> +			((frame->can_id & CAN_SFF_MASK) << 5) |
>> +			((frame->can_id & CAN_RTR_FLAG) ? (1 << 4) : 0);
>> +
>> +		buf[HI3110_FIFO_STD_DLC_OFF] = frame->can_dlc;
>> +
>> +		memcpy(buf + HI3110_FIFO_STD_DATA_OFF,
>> +		       frame->data, frame->can_dlc);
>> +
>> +		hi3110_hw_tx_frame(spi, buf, TX_STD_BUF_LEN -
>> +				   (CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
>> +	}
>> +}
>> +
>> +static void hi3110_hw_rx_frame(struct spi_device *spi, u8 *buf)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +
>> +	priv->spi_tx_buf[0] = HI3110_READ_FIFO_WOTIME;
>> +	hi3110_spi_trans(spi, RX_BUF_LEN);
>> +	memcpy(buf, priv->spi_rx_buf + 1, RX_BUF_LEN - 1);
>> +}
>> +
>> +static void hi3110_hw_rx(struct spi_device *spi)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	struct sk_buff *skb;
>> +	struct can_frame *frame;
>> +	u8 buf[RX_BUF_LEN - 1];
>> +
>> +	skb = alloc_can_skb(priv->net, &frame);
>> +	if (!skb) {
>> +		dev_err(&spi->dev, "cannot allocate RX skb\n");
>> +		priv->net->stats.rx_dropped++;
>> +		return;
>> +	}
>> +
>> +	hi3110_hw_rx_frame(spi, buf);
>> +	if (buf[HI3110_FIFO_WOTIME_TAG_OFF] & HI3110_FIFO_WOTIME_TAG_IDE) {
>> +		/* IDE is recessive (1), indicating extended 29-bit frame */
>> +		frame->can_id = CAN_EFF_FLAG;
>> +		frame->can_id |=
>> +		 (buf[HI3110_FIFO_WOTIME_ID_OFF] << 21) |
>> +		 (((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5) << 18) |
>> +		 ((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0x07) << 15) |
>> +		 (buf[HI3110_FIFO_WOTIME_ID_OFF + 2] << 7) |
>> +		 (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] >> 1);
>> +	} else {
>> +		/* IDE is dominant (0), frame indicating standard 11-bit */
>> +		frame->can_id =
>> +			(buf[HI3110_FIFO_WOTIME_ID_OFF] << 3) |
>> +			((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5);
>> +	}
>> +
>> +	if (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] & HI3110_FIFO_WOTIME_ID_RTR) {
>> +		/* RTR is recessive (1), indicating remote request frame */
>> +		frame->can_id |= CAN_RTR_FLAG;
>> +	}
>> +
>> +	/* Data length */
>> +	frame->can_dlc = get_can_dlc(buf[HI3110_FIFO_WOTIME_DLC_OFF] & 0x0F);
>> +	memcpy(frame->data, buf + HI3110_FIFO_WOTIME_DAT_OFF, frame->can_dlc);
>> +
>> +	priv->net->stats.rx_packets++;
>> +	priv->net->stats.rx_bytes += frame->can_dlc;
>> +
>> +	can_led_event(priv->net, CAN_LED_EVENT_RX);
>> +
>> +	netif_rx_ni(skb);
>> +}
>> +
>> +static void hi3110_hw_sleep(struct spi_device *spi)
>> +{
>> +	hi3110_write(spi, HI3110_WRITE_CTRL0, HI3110_CTRL0_SLEEP_MODE);
>> +}
>> +
>> +static netdev_tx_t hi3110_hard_start_xmit(struct sk_buff *skb,
>> +					  struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +	struct spi_device *spi = priv->spi;
>> +
>> +	if (priv->tx_skb || priv->tx_len) {
>> +		dev_warn(&spi->dev, "hard_xmit called while tx busy\n");
>> +		return NETDEV_TX_BUSY;
>> +	}
>> +
>> +	if (can_dropped_invalid_skb(net, skb))
>> +		return NETDEV_TX_OK;
>> +
>> +	netif_stop_queue(net);
>> +	priv->tx_skb = skb;
>> +	queue_work(priv->wq, &priv->tx_work);
>> +
>> +	return NETDEV_TX_OK;
>> +}
>> +
>> +static int hi3110_do_set_mode(struct net_device *net, enum can_mode mode)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +
>> +	switch (mode) {
>> +	case CAN_MODE_START:
>> +		hi3110_clean(net);
>> +		/* We have to delay work since SPI I/O may sleep */
>> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +		priv->restart_tx = 1;
>> +		if (priv->can.restart_ms == 0)
>> +			priv->after_suspend = AFTER_SUSPEND_RESTART;
>> +		queue_work(priv->wq, &priv->restart_work);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int hi3110_set_normal_mode(struct spi_device *spi)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	u8 reg;
>> +
>> +	hi3110_write(spi, HI3110_WRITE_INTE, HI3110_INT_BUSERR |
>> +		     HI3110_INT_RXFIFO | HI3110_INT_TXCPLT);
>> +
>> +	/* Enable TX */
>> +	hi3110_write(spi, HI3110_WRITE_CTRL1, HI3110_CTRL1_TXEN);
>> +
>> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>> +		/* Put device into loopback mode */
>> +		hi3110_write(spi, HI3110_WRITE_CTRL0,
>> +			     HI3110_CTRL0_LOOPBACK_MODE);
>> +	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>> +		/* Put device into listen-only mode */
>> +		hi3110_write(spi, HI3110_WRITE_CTRL0,
>> +			     HI3110_CTRL0_MONITOR_MODE);
>> +	} else {
>> +		/* Put device into normal mode */
>> +		hi3110_write(spi, HI3110_WRITE_CTRL0,
>> +			     HI3110_CTRL0_NORMAL_MODE);
>> +
>> +		/* Wait for the device to enter normal mode */
>> +		mdelay(HI3110_OST_DELAY_MS);
>> +		reg = hi3110_read(spi, HI3110_READ_CTRL0);
>> +		if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_NORMAL_MODE)
>> +			return -EBUSY;
>> +	}
>> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +	return 0;
>> +}
>> +
>> +static int hi3110_do_set_bittiming(struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +	struct can_bittiming *bt = &priv->can.bittiming;
>> +	struct spi_device *spi = priv->spi;
>> +
>> +	hi3110_write(spi, HI3110_WRITE_BTR0,
>> +		     ((bt->sjw - 1) << HI3110_BTR0_SJW_SHIFT) |
>> +		     ((bt->brp - 1) << HI3110_BTR0_BRP_SHIFT));
>> +
>> +	hi3110_write(spi, HI3110_WRITE_BTR1,
>> +		     (priv->can.ctrlmode &
>> +		     CAN_CTRLMODE_3_SAMPLES ?
>> +		     HI3110_BTR1_SAMP_3PERBIT : HI3110_BTR1_SAMP_1PERBIT) |
>> +		     ((bt->phase_seg1 + bt->prop_seg - 1)
>> +		     << HI3110_BTR1_TSEG1_SHIFT) |
>> +		     ((bt->phase_seg2 - 1) << HI3110_BTR1_TSEG2_SHIFT));
>> +
>> +	dev_dbg(&spi->dev, "BT: 0x%02x 0x%02x\n",
>> +		hi3110_read(spi, HI3110_READ_BTR0),
>> +		hi3110_read(spi, HI3110_READ_BTR1));
>> +
>> +	return 0;
>> +}
>> +
>> +static int hi3110_setup(struct net_device *net, struct hi3110_priv *priv,
>> +			struct spi_device *spi)
> 
> onlt the first parameter is used.
> 

Will remove the unused parameters in V2

>> +{
>> +	hi3110_do_set_bittiming(net);
>> +	return 0;
>> +}
>> +
>> +static int hi3110_hw_reset(struct spi_device *spi)
>> +{
>> +	u8 reg;
>> +	int ret;
>> +
>> +	/* Wait for oscillator startup timer after power up */
>> +	mdelay(HI3110_OST_DELAY_MS);
>> +
>> +	ret = hi3110_cmd(spi, HI3110_MASTER_RESET);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for oscillator startup timer after reset */
>> +	mdelay(HI3110_OST_DELAY_MS);
>> +
>> +	reg = hi3110_read(spi, HI3110_READ_CTRL0);
>> +	if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_INIT_MODE)
>> +		return -ENODEV;
>> +
>> +	/* As per the datasheet it appears the error flags are
>> +	 * not cleared on reset. Explicitly clear them by performing a read
>> +	 */
>> +	hi3110_read(spi, HI3110_READ_ERR);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hi3110_hw_probe(struct spi_device *spi)
>> +{
>> +	u8 statf;
>> +
>> +	hi3110_hw_reset(spi);
>> +
>> +	/* Confirm correct operation by checking against reset values
>> +	 * in datasheet
>> +	 */
>> +	statf = hi3110_read(spi, HI3110_READ_STATF);
>> +
>> +	dev_dbg(&spi->dev, "statf: %02X\n", statf);
>> +
>> +	if (statf != 0x82)
>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>> +static int hi3110_power_enable(struct regulator *reg, int enable)
>> +{
>> +	if (IS_ERR_OR_NULL(reg))
>> +		return 0;
>> +
>> +	if (enable)
>> +		return regulator_enable(reg);
>> +	else
>> +		return regulator_disable(reg);
>> +}
>> +
>> +static void hi3110_open_clean(struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +	struct spi_device *spi = priv->spi;
>> +
>> +	free_irq(spi->irq, priv);
>> +	hi3110_hw_sleep(spi);
>> +	hi3110_power_enable(priv->transceiver, 0);
>> +	close_candev(net);
>> +}
>> +
>> +static int hi3110_stop(struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +	struct spi_device *spi = priv->spi;
>> +
>> +	close_candev(net);
>> +
>> +	priv->force_quit = 1;
>> +	free_irq(spi->irq, priv);
>> +	destroy_workqueue(priv->wq);
>> +	priv->wq = NULL;
>> +
>> +	mutex_lock(&priv->hi3110_lock);
>> +
>> +	/* Disable transmit, interrupts and clear flags */
>> +	hi3110_write(spi, HI3110_WRITE_CTRL1, 0x0);
>> +	hi3110_write(spi, HI3110_WRITE_INTE, 0x0);
>> +	hi3110_read(spi, HI3110_READ_INTF);
>> +
>> +	hi3110_clean(net);
>> +
>> +	hi3110_hw_sleep(spi);
>> +
>> +	hi3110_power_enable(priv->transceiver, 0);
>> +
>> +	priv->can.state = CAN_STATE_STOPPED;
>> +
>> +	mutex_unlock(&priv->hi3110_lock);
>> +
>> +	can_led_event(net, CAN_LED_EVENT_STOP);
>> +
>> +	return 0;
>> +}
>> +
>> +static void hi3110_error_skb(struct net_device *net, int can_id,
>> +			     int data1, int data2)
>> +{
>> +	struct sk_buff *skb;
>> +	struct can_frame *frame;
>> +
>> +	skb = alloc_can_err_skb(net, &frame);
>> +	if (skb) {
>> +		frame->can_id |= can_id;
>> +		frame->data[1] = data1;
>> +		frame->data[2] = data2;
>> +		netif_rx_ni(skb);
>> +	} else {
>> +		netdev_err(net, "cannot allocate error skb\n");
>> +	}
>> +}
>> +
>> +static void hi3110_tx_work_handler(struct work_struct *ws)
>> +{
>> +	struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
>> +						 tx_work);
>> +	struct spi_device *spi = priv->spi;
>> +	struct net_device *net = priv->net;
>> +	struct can_frame *frame;
>> +
>> +	mutex_lock(&priv->hi3110_lock);
>> +	if (priv->tx_skb) {
>> +		if (priv->can.state == CAN_STATE_BUS_OFF) {
>> +			hi3110_clean(net);
>> +		} else {
>> +			frame = (struct can_frame *)priv->tx_skb->data;
>> +
>> +			if (frame->can_dlc > CAN_FRAME_MAX_DATA_LEN)
>> +				frame->can_dlc = CAN_FRAME_MAX_DATA_LEN;
> 
> this has already been checked
> 

Will remove the check in V2

>> +			hi3110_hw_tx(spi, frame);
>> +			priv->tx_len = 1 + frame->can_dlc;
>> +			can_put_echo_skb(priv->tx_skb, net, 0);
>> +			priv->tx_skb = NULL;
>> +		}
>> +	}
>> +	mutex_unlock(&priv->hi3110_lock);
>> +}
>> +
>> +static void hi3110_restart_work_handler(struct work_struct *ws)
>> +{
>> +	struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
>> +						 restart_work);
>> +	struct spi_device *spi = priv->spi;
>> +	struct net_device *net = priv->net;
>> +
>> +	mutex_lock(&priv->hi3110_lock);
>> +	if (priv->after_suspend) {
>> +		hi3110_hw_reset(spi);
>> +		hi3110_setup(net, priv, spi);
>> +		if (priv->after_suspend & AFTER_SUSPEND_RESTART) {
>> +			hi3110_set_normal_mode(spi);
>> +		} else if (priv->after_suspend & AFTER_SUSPEND_UP) {
>> +			netif_device_attach(net);
>> +			hi3110_clean(net);
>> +			hi3110_set_normal_mode(spi);
>> +			netif_wake_queue(net);
>> +		} else {
>> +			hi3110_hw_sleep(spi);
>> +		}
>> +		priv->after_suspend = 0;
>> +		priv->force_quit = 0;
>> +	}
>> +
>> +	if (priv->restart_tx) {
>> +		priv->restart_tx = 0;
>> +		hi3110_clean(net);
>> +		netif_wake_queue(net);
>> +		hi3110_error_skb(net, CAN_ERR_RESTARTED, 0, 0);
>> +	}
>> +	mutex_unlock(&priv->hi3110_lock);
>> +}
>> +
>> +static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
>> +{
>> +	struct hi3110_priv *priv = dev_id;
>> +	struct spi_device *spi = priv->spi;
>> +	struct net_device *net = priv->net;
>> +
>> +	mutex_lock(&priv->hi3110_lock);
>> +
>> +	while (!priv->force_quit) {
>> +		enum can_state new_state;
>> +		u8 intf;
>> +		u8 eflag;
>> +		int can_id = 0, data1 = 0, data2 = 0;
>> +
>> +		while (!(HI3110_STAT_RXFMTY &
>> +			hi3110_read(spi, HI3110_READ_STATF))) {
>> +			hi3110_hw_rx(spi);
>> +		};
>> +
>> +		intf = hi3110_read(spi, HI3110_READ_INTF);
>> +		eflag = hi3110_read(spi, HI3110_READ_ERR);
> 
> does the hardware supports multiple reads with a single transfer? If so
> make use of it, for performance reasons.
> 

Looking at the datasheet it does not seem possible, would be nice if it
was supported.

>> +		/* Update can state */
>> +		if (eflag & HI3110_ERR_BUSOFF) {
>> +			new_state = CAN_STATE_BUS_OFF;
>> +			can_id |= CAN_ERR_BUSOFF;
>> +		} else if (eflag & HI3110_ERR_TXERRP) {
>> +			new_state = CAN_STATE_ERROR_PASSIVE;
>> +			can_id |= CAN_ERR_CRTL;
>> +			data1 |= CAN_ERR_CRTL_TX_PASSIVE;
>> +		} else if (eflag & HI3110_ERR_RXERRP) {
>> +			new_state = CAN_STATE_ERROR_PASSIVE;
>> +			can_id |= CAN_ERR_CRTL;
>> +			data1 |= CAN_ERR_CRTL_RX_PASSIVE;
>> +		} else {
>> +			new_state = CAN_STATE_ERROR_ACTIVE;
>> +		}
>> +
>> +		/* Check for protocol errors */
>> +		if (eflag & HI3110_ERR_PROTOCOL_MASK) {
>> +			can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>> +			priv->can.can_stats.bus_error++;
>> +			priv->net->stats.rx_errors++;
>> +			if (eflag & HI3110_ERR_BITERR)
>> +				data2 |= CAN_ERR_PROT_BIT;
>> +			else if (eflag & HI3110_ERR_FRMERR)
>> +				data2 |= CAN_ERR_PROT_FORM;
>> +			else if (eflag & HI3110_ERR_STUFERR)
>> +				data2 |= CAN_ERR_PROT_STUFF;
>> +			else
>> +				data2 |= CAN_ERR_PROT_UNSPEC;
>> +		}
>> +
>> +		/* Update can state statistics */
>> +		switch (priv->can.state) {
>> +		case CAN_STATE_ERROR_ACTIVE:
>> +			if (new_state >= CAN_STATE_ERROR_WARNING &&
>> +			    new_state <= CAN_STATE_BUS_OFF)
>> +				priv->can.can_stats.error_warning++;
>> +		/* fallthrough */
>> +		case CAN_STATE_ERROR_WARNING:
>> +			if (new_state >= CAN_STATE_ERROR_PASSIVE &&
>> +			    new_state <= CAN_STATE_BUS_OFF)
>> +				priv->can.can_stats.error_passive++;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +		priv->can.state = new_state;
>> +
>> +		if (intf & HI3110_INT_BUSERR) {
>> +			/* Note: HI3110 Does report overflow errors */
>> +			hi3110_error_skb(net, can_id, data1, data2);
>> +		}
>> +
>> +		if (priv->can.state == CAN_STATE_BUS_OFF) {
>> +			if (priv->can.restart_ms == 0) {
>> +				priv->force_quit = 1;
>> +				priv->can.can_stats.bus_off++;
>> +				can_bus_off(net);
>> +				hi3110_hw_sleep(spi);
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (intf == 0)
>> +			break;
>> +
>> +		if (intf & HI3110_INT_TXCPLT) {
>> +			net->stats.tx_packets++;
>> +			net->stats.tx_bytes += priv->tx_len - 1;
>> +			can_led_event(net, CAN_LED_EVENT_TX);
>> +			if (priv->tx_len) {
>> +				can_get_echo_skb(net, 0);
>> +				priv->tx_len = 0;
>> +			}
>> +			netif_wake_queue(net);
>> +		}
>> +	}
>> +	mutex_unlock(&priv->hi3110_lock);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hi3110_open(struct net_device *net)
>> +{
>> +	struct hi3110_priv *priv = netdev_priv(net);
>> +	struct spi_device *spi = priv->spi;
>> +	unsigned long flags = IRQF_ONESHOT | IRQF_TRIGGER_RISING;
>> +	int ret;
>> +
>> +	ret = open_candev(net);
>> +	if (ret) {
>> +		dev_err(&spi->dev, "unable to set initial baudrate!\n");
>> +		return ret;
>> +	}
>> +
>> +	mutex_lock(&priv->hi3110_lock);
>> +	hi3110_power_enable(priv->transceiver, 1);
>> +
>> +	priv->force_quit = 0;
>> +	priv->tx_skb = NULL;
>> +	priv->tx_len = 0;
>> +
>> +	ret = request_threaded_irq(spi->irq, NULL, hi3110_can_ist,
>> +				   flags, DEVICE_NAME, priv);
>> +	if (ret) {
>> +		dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
> 
> add propoer goto targets at the and of this function, for easier error
> handling cleanup. This mean basically get rid of hi3110_open_clean(net).
> 

Will fix in V2

>> +		hi3110_power_enable(priv->transceiver, 0);
>> +		close_candev(net);
>> +		goto open_unlock;
>> +	}
>> +
>> +	priv->wq = alloc_workqueue("hi3110_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
>> +			   0);
>> +	INIT_WORK(&priv->tx_work, hi3110_tx_work_handler);
>> +	INIT_WORK(&priv->restart_work, hi3110_restart_work_handler);
>> +
>> +	ret = hi3110_hw_reset(spi);
>> +	if (ret) {
>> +		hi3110_open_clean(net);
>> +		goto open_unlock;
>> +	}
>> +	ret = hi3110_setup(net, priv, spi);
>> +	if (ret) {
>> +		hi3110_open_clean(net);
>> +		goto open_unlock;
>> +	}
>> +	ret = hi3110_set_normal_mode(spi);
>> +	if (ret) {
>> +		hi3110_open_clean(net);
>> +		goto open_unlock;
>> +	}
>> +	can_led_event(net, CAN_LED_EVENT_OPEN);
>> +	netif_wake_queue(net);
>> +
>> +open_unlock:
>> +	mutex_unlock(&priv->hi3110_lock);
>> +	return ret;
>> +}
>> +
>> +static const struct net_device_ops hi3110_netdev_ops = {
>> +	.ndo_open = hi3110_open,
>> +	.ndo_stop = hi3110_stop,
>> +	.ndo_start_xmit = hi3110_hard_start_xmit,
>> +};
>> +
>> +static const struct of_device_id hi3110_of_match[] = {
>> +	{
>> +		.compatible	= "holt,hi3110",
>> +		.data		= (void *)CAN_HI3110_HI3110,
>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, hi3110_of_match);
>> +
>> +static const struct spi_device_id hi3110_id_table[] = {
>> +	{
>> +		.name		= "hi3110",
>> +		.driver_data	= (kernel_ulong_t)CAN_HI3110_HI3110,
>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, hi3110_id_table);
>> +
>> +static int hi3110_can_probe(struct spi_device *spi)
>> +{
>> +	const struct of_device_id *of_id = of_match_device(hi3110_of_match,
>> +							   &spi->dev);
>> +	struct net_device *net;
>> +	struct hi3110_priv *priv;
>> +	struct clk *clk;
>> +	int freq, ret;
>> +
>> +	clk = devm_clk_get(&spi->dev, NULL);
>> +	if (IS_ERR(clk)) {
>> +		dev_err(&spi->dev, "no CAN clock source defined\n");
>> +		return PTR_ERR(clk);
>> +	}
>> +	freq = clk_get_rate(clk);
>> +
>> +	/* Sanity check */
>> +	if (freq > 40000000)
>> +		return -ERANGE;
>> +
>> +	/* Allocate can/net device */
>> +	net = alloc_candev(sizeof(struct hi3110_priv), TX_ECHO_SKB_MAX);
>> +	if (!net)
>> +		return -ENOMEM;
>> +
>> +	if (!IS_ERR(clk)) {
>> +		ret = clk_prepare_enable(clk);
>> +		if (ret)
>> +			goto out_free;
>> +	}
>> +
>> +	net->netdev_ops = &hi3110_netdev_ops;
>> +	net->flags |= IFF_ECHO;
>> +
>> +	priv = netdev_priv(net);
>> +	priv->can.bittiming_const = &hi3110_bittiming_const;
>> +	priv->can.do_set_mode = hi3110_do_set_mode;
>> +	priv->can.clock.freq = freq / 2;
>> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>> +		CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY;
>> +	if (of_id)
>> +		priv->model = (enum hi3110_model)of_id->data;
>> +	else
>> +		priv->model = spi_get_device_id(spi)->driver_data;
>> +	priv->net = net;
>> +	priv->clk = clk;
>> +
>> +	spi_set_drvdata(spi, priv);
>> +
>> +	/* Configure the SPI bus */
>> +	spi->bits_per_word = 8;
>> +	ret = spi_setup(spi);
>> +	if (ret)
>> +		goto out_clk;
>> +
>> +	priv->power = devm_regulator_get_optional(&spi->dev, "vdd");
>> +	priv->transceiver = devm_regulator_get_optional(&spi->dev, "xceiver");
>> +	if ((PTR_ERR(priv->power) == -EPROBE_DEFER) ||
>> +	    (PTR_ERR(priv->transceiver) == -EPROBE_DEFER)) {
>> +		ret = -EPROBE_DEFER;
>> +		goto out_clk;
>> +	}
>> +
>> +	ret = hi3110_power_enable(priv->power, 1);
>> +	if (ret)
>> +		goto out_clk;
>> +
>> +	priv->spi = spi;
>> +	mutex_init(&priv->hi3110_lock);
>> +
>> +	/* If requested, allocate DMA buffers */
>> +	if (hi3110_enable_dma) {
>> +		spi->dev.coherent_dma_mask = ~0;
>> +
>> +		/* Minimum coherent DMA allocation is PAGE_SIZE, so allocate
>> +		 * that much and share it between Tx and Rx DMA buffers.
>> +		 */
>> +		priv->spi_tx_buf = dmam_alloc_coherent(&spi->dev,
>> +						      PAGE_SIZE,
>> +						      &priv->spi_tx_dma,
>> +						      GFP_DMA);
>> +
>> +		if (priv->spi_tx_buf) {
>> +			priv->spi_rx_buf = (priv->spi_tx_buf + (PAGE_SIZE / 2));
>> +			priv->spi_rx_dma = (dma_addr_t)(priv->spi_tx_dma +
>> +							(PAGE_SIZE / 2));
>> +		} else {
>> +			/* Fall back to non-DMA */
>> +			hi3110_enable_dma = 0;
>> +		}
>> +	}
>> +
>> +	/* Allocate non-DMA buffers */
>> +	if (!hi3110_enable_dma) {
>> +		priv->spi_tx_buf = devm_kzalloc(&spi->dev, RX_BUF_LEN,
>> +				GFP_KERNEL);
>> +		if (!priv->spi_tx_buf) {
>> +			ret = -ENOMEM;
>> +			goto error_probe;
>> +		}
>> +		priv->spi_rx_buf = devm_kzalloc(&spi->dev, RX_BUF_LEN,
>> +				GFP_KERNEL);
>> +
>> +		if (!priv->spi_rx_buf) {
>> +			ret = -ENOMEM;
>> +			goto error_probe;
>> +		}
>> +	}
>> +
>> +	SET_NETDEV_DEV(net, &spi->dev);
>> +
>> +	ret = hi3110_hw_probe(spi);
>> +	if (ret) {
>> +		if (ret == -ENODEV)
>> +			dev_err(&spi->dev, "Cannot initialize %x. Wrong wiring?\n",
>> +				priv->model);
>> +		goto error_probe;
>> +	}
>> +	hi3110_hw_sleep(spi);
>> +
>> +	ret = register_candev(net);
>> +	if (ret)
>> +		goto error_probe;
>> +
>> +	devm_can_led_init(net);
>> +	netdev_info(net, "%x successfully initialized.\n", priv->model);
>> +
>> +	return 0;
>> +
>> +error_probe:
>> +	hi3110_power_enable(priv->power, 0);
>> +
>> +out_clk:
>> +	if (!IS_ERR(clk))
>> +		clk_disable_unprepare(clk);
>> +
>> +out_free:
>> +	free_candev(net);
>> +
>> +	dev_err(&spi->dev, "Probe failed, err=%d\n", -ret);
>> +	return ret;
>> +}
>> +
>> +static int hi3110_can_remove(struct spi_device *spi)
>> +{
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	struct net_device *net = priv->net;
>> +
>> +	unregister_candev(net);
>> +
>> +	hi3110_power_enable(priv->power, 0);
>> +
>> +	if (!IS_ERR(priv->clk))
>> +		clk_disable_unprepare(priv->clk);
>> +
>> +	free_candev(net);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused hi3110_can_suspend(struct device *dev)
>> +{
>> +	struct spi_device *spi = to_spi_device(dev);
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +	struct net_device *net = priv->net;
>> +
>> +	priv->force_quit = 1;
>> +	disable_irq(spi->irq);
>> +
>> +	/* Note: at this point neither IST nor workqueues are running.
>> +	 * open/stop cannot be called anyway so locking is not needed
>> +	 */
>> +	if (netif_running(net)) {
>> +		netif_device_detach(net);
>> +
>> +		hi3110_hw_sleep(spi);
>> +		hi3110_power_enable(priv->transceiver, 0);
>> +		priv->after_suspend = AFTER_SUSPEND_UP;
>> +	} else {
>> +		priv->after_suspend = AFTER_SUSPEND_DOWN;
>> +	}
>> +
>> +	if (!IS_ERR_OR_NULL(priv->power)) {
>> +		regulator_disable(priv->power);
>> +		priv->after_suspend |= AFTER_SUSPEND_POWER;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused hi3110_can_resume(struct device *dev)
>> +{
>> +	struct spi_device *spi = to_spi_device(dev);
>> +	struct hi3110_priv *priv = spi_get_drvdata(spi);
>> +
>> +	if (priv->after_suspend & AFTER_SUSPEND_POWER)
>> +		hi3110_power_enable(priv->power, 1);
>> +
>> +	if (priv->after_suspend & AFTER_SUSPEND_UP) {
>> +		hi3110_power_enable(priv->transceiver, 1);
>> +		queue_work(priv->wq, &priv->restart_work);
>> +	} else {
>> +		priv->after_suspend = 0;
>> +	}
>> +
>> +	priv->force_quit = 0;
>> +	enable_irq(spi->irq);
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(hi3110_can_pm_ops, hi3110_can_suspend,
>> +	hi3110_can_resume);
>> +
>> +static struct spi_driver hi3110_can_driver = {
>> +	.driver = {
>> +		.name = DEVICE_NAME,
>> +		.of_match_table = hi3110_of_match,
>> +		.pm = &hi3110_can_pm_ops,
>> +	},
>> +	.id_table = hi3110_id_table,
>> +	.probe = hi3110_can_probe,
>> +	.remove = hi3110_can_remove,
>> +};
>> +
>> +module_spi_driver(hi3110_can_driver);
>> +
>> +MODULE_AUTHOR("Akshay Bhat <akshay.bhat@timesys.com>");
>> +MODULE_AUTHOR("Casey Fitzpatrick <casey.fitzpatrick@timesys.com>");
>> +MODULE_DESCRIPTION("Holt HI-3110 CAN driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> Marc
> 

Thanks for all the feedback.
Akshay


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* Re: [PATCH v2 14/14] ARM: dts: da850-lcdk: enable the SATA node
From: David Lechner @ 2017-01-17 18:53 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484655976-25382-15-git-send-email-bgolaszewski@baylibre.com>

On 01/17/2017 06:26 AM, Bartosz Golaszewski wrote:
> Enable the SATA node for da850-lcdk. We omit the pinctrl property on
> purpose - the muxed SATA pins are not hooked up to anything
> SATA-related on the lcdk.
>
> The REFCLKN/P rate on the board is 100MHz, so we need a multiplier of
> 15 for 1.5GHz PLL rate.

The multiplier is no longer in device tree, so this comment is not 
necessary.

>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850-lcdk.dts | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
> index afcb482..fbeee3c 100644
> --- a/arch/arm/boot/dts/da850-lcdk.dts
> +++ b/arch/arm/boot/dts/da850-lcdk.dts
> @@ -105,6 +105,10 @@
>  	status = "okay";
>  };
>
> +&sata {
> +	status = "okay";
> +};
> +
>  &mdio {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&mdio_pins>;
>

^ permalink raw reply

* Re: [PATCH v2 11/14] sata: ahci-da850: un-hardcode the MPY bits
From: David Lechner @ 2017-01-17 18:51 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484655976-25382-12-git-send-email-bgolaszewski@baylibre.com>

On 01/17/2017 06:26 AM, Bartosz Golaszewski wrote:
> In order to make the MPY bits configurable, try to obtain the refclk
> and calculate the required multiplier from its rate.
>
> If we fail to get the clock, fall back to the default value which
> keeps backwards compatibility.

It seems like it would be wiser to make is so that if we fail to get the 
clock, it is an error. This way, if someone makes a new board and 
forgets to configure a clock, they will get an error instead of 
wondering why things are not working because it is using the wrong 
multiplier.

>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/ata/ahci_da850.c | 88 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> index a7a7161..f48b7d0 100644
> --- a/drivers/ata/ahci_da850.c
> +++ b/drivers/ata/ahci_da850.c
> @@ -14,6 +14,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/libata.h>
>  #include <linux/ahci_platform.h>
> +#include <asm/div64.h>
>  #include "ahci.h"
>
>  #define DRV_NAME		"ahci_da850"
> @@ -30,16 +31,14 @@
>  #define SATA_PHY_ENPLL(x)	((x) << 31)
>
>  /*
> - * The multiplier needed for 1.5GHz PLL output.
> - *
> - * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
> - * frequency (which is used by DA850 EVM board) and may need to be changed
> - * if you would like to use this driver on some other board.
> + * This is the default multiplier value used if the refclk could not be
> + * obtained. It corresponds with a crystal rate of 100MHz for 1.5GHz PLL
> + * output.
>   */
> -#define DA850_SATA_CLK_MULTIPLIER	7
> +#define DA850_SATA_MPY_DEFAULT	0x8
>
>  static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> -			    void __iomem *ahci_base)
> +			    void __iomem *ahci_base, u32 mpy)
>  {
>  	unsigned int val;
>
> @@ -48,13 +47,56 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
>  	val &= ~BIT(0);
>  	writel(val, pwrdn_reg);
>
> -	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
> -	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
> -	      SATA_PHY_ENPLL(1);
> +	val = SATA_PHY_MPY(mpy) | SATA_PHY_LOS(1) | SATA_PHY_RXCDR(4) |
> +	      SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) | SATA_PHY_ENPLL(1);
>
>  	writel(val, ahci_base + SATA_P0PHYCR_REG);
>  }
>
> +static u32 ahci_da850_calculate_mpy(unsigned long refclk_rate)
> +{
> +	u64 pll_output = 1500000000;
> +	u32 needed;
> +
> +	/*
> +	 * We need to determine the value of the multiplier (MPY) bits.
> +	 *
> +	 * In order to include the 12.5 multiplier we need to first multiply
> +	 * the desired rate of 1.5GHz by 10 before division.
> +	 */
> +	pll_output *= 10;
> +	needed = __div64_32(&pll_output, refclk_rate);

What if this does not divide evenly and there is a remainder. Shouldn't 
there be an error in that case?

...

^ permalink raw reply

* Applied "ASoC: sun4i-i2s: Increase DMA max burst to 8" to the asoc tree
From: Mark Brown @ 2017-01-17 18:44 UTC (permalink / raw)
  Cc: mark.rutland, thomas.petazzoni, alsa-devel, mylene.josserand,
	devicetree, linux-kernel, mturquette, linux-sunxi, sboyd, tiwai,
	robh+dt, lgirdwood, wens, broonie, alexandre.belloni,
	maxime.ripard, perex, linux-clk, linux-arm-kernel
In-Reply-To: <20170117140230.23142-2-mylene.josserand@free-electrons.com>

[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]

The patch

   ASoC: sun4i-i2s: Increase DMA max burst to 8

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From ebad64d19377957976963f99ce1fcf2f09796357 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Myl=C3=A8ne=20Josserand?=
 <mylene.josserand@free-electrons.com>
Date: Tue, 17 Jan 2017 15:02:21 +0100
Subject: [PATCH] ASoC: sun4i-i2s: Increase DMA max burst to 8
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As done previously for sun4i-codec, the DMA maxburst of 4
is not supported by every SoCs so the DMA controller engine
returns "unsupported value".

As a maxburst of 8 is supported by all variants, this patch
increases it to 8.

For more details, see commit from Chen-Yu Tsai:
commit 730e2dd0cbc7 ("ASoC: sun4i-codec: Increase DMA max burst to 8")

Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/sunxi/sun4i-i2s.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index f24d19526603..4237323ef594 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -694,10 +694,10 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 	}
 	
 	i2s->playback_dma_data.addr = res->start + SUN4I_I2S_FIFO_TX_REG;
-	i2s->playback_dma_data.maxburst = 4;
+	i2s->playback_dma_data.maxburst = 8;
 
 	i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
-	i2s->capture_dma_data.maxburst = 4;
+	i2s->capture_dma_data.maxburst = 8;
 
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
-- 
2.11.0



[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH 1/3] Input: add STMicroelectronics FingerTip touchscreen driver
From: Krzysztof Kozlowski @ 2017-01-17 18:43 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Dmitry Torokhov, Krzysztof Kozlowski, Rob Herring, Chanwoo Choi,
	Javier Martinez Canillas, linux-input, devicetree, linux-kernel,
	linux-samsung-soc, Andi Shyti
In-Reply-To: <20170117135441.31450-2-andi.shyti@samsung.com>

On Tue, Jan 17, 2017 at 10:54:39PM +0900, Andi Shyti wrote:
> Add binding for the STMicroelectronics FingerTip (stmfts)
> touchscreen driver.
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  .../bindings/input/touchscreen/st,stmfts.txt       | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt b/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> new file mode 100644
> index 000000000000..788f4ba744db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/st,stmfts.txt
> @@ -0,0 +1,43 @@
> +* ST-Microelectronics FingerTip touchscreen controller
> +
> +The ST-Microelectronics FingerTip device provides a basic touchscreen
> +functionality. Along with it the user can enable the touchkey which can work as
> +a basic HOME and BACK key for phones.
> +
> +The driver supports also hovering as an absolute single touch event with x, y, z
> +coordinates.
> +
> +Required properties:
> +- compatible		: must be "st,stmfts"
> +- reg			: I2C slave address, (e.g. 0x49)
> +- interrupt-parent	: the phandle to the interrupt controller which provides
> +			  the interrupt
> +- interrupts		: interrupt specification
> +- avdd-supply		: analogic power supply
> +- vdd-supply		: power supply
> +- touchscreen-size-x	: see touchscreen.txt
> +- touchscreen-size-y	: see touchscreen.txt
> +
> +Optional properties:
> +- touch-key-connected	: specifies whether the touchkey feature is connected

You are making it a generic property but it is specific to this device,
so:
	st,touch-key-connected
?

> +- ledvdd-supply		: power supply to the touch key leds

Is this really optional? If yes... how it gets the power when not
provided?

Best regards,
Krzysztof

> +
> +Example:
> +
> +i2c@00000000 {
> +
> +	/* ... */
> +
> +	stmfts@49 {
> +		compatible = "st,stmfts";
> +		reg = <0x49>;
> +		interrupt-parent = <&gpa1>;
> +		interrupts = <1 IRQ_TYPE_NONE>;
> +		touchscreen-size-x = <1599>;
> +		touchscreen-size-y = <2559>;
> +		touch-key-connected;
> +		avdd-supply = <&ldo30_reg>;
> +		vdd-supply = <&ldo31_reg>;
> +		ledvdd-supply = <&ldo33_reg>;
> +	};
> +};
> -- 
> 2.11.0
> 

^ permalink raw reply

* RE: [PATCH] ARM: dts: r7s72100: fix sdhi clock define
From: Chris Brandt @ 2017-01-17 18:42 UTC (permalink / raw)
  To: Wolfram Sang, Geert Uytterhoeven
  Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
	devicetree@vger.kernel.org, Linux-Renesas, Wolfram Sang
In-Reply-To: <20170117094529.GC1487@katana>

Hi Wolfram,

On Tuesday, January 17, 2017, Wolfram Sang wrote:
> > I have no idea if the SDHI driver disables the module clock when it is
> > idle, but shouldn't the card detect clock be running all the time when
> > the driver is bound to the device?
> 
> Yes, it should. And for all instances with just one clock, this means this
> main clock must be running. So, en-/disable functions are all about
> suspend/resume and bind/unbind. (Huh, looks like the unbind part is
> missing, though. Need to look closer).

I just did an unbind on my system:

$ echo e804e800.sd > /sys/bus/platform/drivers/sh_mobile_sdhi/unbind


Looks like sh_mobile_sdhi_remove() is called, but sh_mobile_sdhi_clk_disable()
is not.

So, should sh_mobile_sdhi_remove() be changed to call sh_mobile_sdhi_clk_disable()?


Chris

^ permalink raw reply

* Re: [PATCH v2 06/14] ARM: davinci: da850: model the SATA refclk
From: David Lechner @ 2017-01-17 18:40 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide, linux-kernel, linux-arm-kernel, devicetree
In-Reply-To: <1484655976-25382-7-git-send-email-bgolaszewski@baylibre.com>

On 01/17/2017 06:26 AM, Bartosz Golaszewski wrote:
> Register a dummy clock modelling the external SATA oscillator for
> da850 DT mode. For non-DT boot we don't register the clock - instead
> we rely on the default MPY value defined in the da850 ahci driver (as
> is done currently).

Why not register a clock for non-DT boot as well?

^ permalink raw reply

* Re: [PATCH 3/3] arm64: dts: exynos: Add stmfts touchscreen node for TM2 and TM2E
From: Krzysztof Kozlowski @ 2017-01-17 18:39 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Dmitry Torokhov, Krzysztof Kozlowski, Rob Herring, Chanwoo Choi,
	Javier Martinez Canillas, linux-input, devicetree, linux-kernel,
	linux-samsung-soc, Andi Shyti
In-Reply-To: <20170117135441.31450-4-andi.shyti@samsung.com>

On Tue, Jan 17, 2017 at 10:54:41PM +0900, Andi Shyti wrote:
> TM2 and TM2E devices are provided with a ST-Microelectronics
> Finger Tip S device with small differences:
> 
>  - screen size
>  - TM2E uses the stmfts also as a touchkey for "back" and "menu"
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 13 +++++++++++++
>  arch/arm64/boot/dts/exynos/exynos5433-tm2.dts         |  5 +++++
>  arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts        |  7 +++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> index a5c866901e93..689d5c124a49 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> @@ -745,6 +745,19 @@
>  	};
>  };
>  
> +&hsi2c_5 {
> +	status = "okay";
> +
> +	touchscreen: stmfts@49 {
> +		compatible = "st,stmfts";
> +		reg = <0x49>;
> +		interrupt-parent = <&gpa1>;
> +		interrupts = <1 IRQ_TYPE_NONE>;

Looks fine to me, just wondering:
1. Are you sure this is TYPE_NONE interrupt? Sometimes we put it
   everywhere instead of checking real value.
2. Don't you have to configure the gpa1-1 to EINT? Any pull up/down?

Best regards,
Krzysztof

> +		avdd-supply = <&ldo30_reg>;
> +		vdd-supply = <&ldo31_reg>;
> +	};
> +};
> +
>  &hsi2c_7 {
>  	status = "okay";
>  
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> index 2449266b268f..d0311d34c6bd 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> @@ -29,3 +29,8 @@
>  	regulator-min-microvolt = <3000000>;
>  	regulator-max-microvolt = <3000000>;
>  };
> +
> +&touchscreen {
> +	touchscreen-size-x = <1439>;
> +	touchscreen-size-y = <2559>;
> +};
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> index 2fbf3a860316..388880dcb5ae 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> @@ -29,3 +29,10 @@
>  	regulator-min-microvolt = <3300000>;
>  	regulator-max-microvolt = <3300000>;
>  };
> +
> +&touchscreen {
> +	touchscreen-size-x = <1599>;
> +	touchscreen-size-y = <2559>;
> +	touch-key-connected;
> +	ledvdd-supply = <&ldo33_reg>;
> +};
> -- 
> 2.11.0
> 

^ permalink raw reply

* Re: [PATCH v2 01/14] devicetree: bindings: add bindings for ahci-da850
From: David Lechner @ 2017-01-17 18:35 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484655976-25382-2-git-send-email-bgolaszewski@baylibre.com>

On 01/17/2017 06:26 AM, Bartosz Golaszewski wrote:
> Add DT bindings for the TI DA850 AHCI SATA controller.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  Documentation/devicetree/bindings/ata/ahci-da850.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> new file mode 100644
> index 0000000..e7111b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> @@ -0,0 +1,18 @@
> +Device tree binding for the TI DA850 AHCI SATA Controller
> +---------------------------------------------------------
> +
> +Required properties:
> +  - compatible: must be "ti,da850-ahci"
> +  - reg: physical base addresses and sizes of the controller's register areas
> +  - interrupts: interrupt specifier (refer to the interrupt binding)
> +
> +Optional properties:
> +  - clocks: clock specifier (refer to the common clock binding)

Won't you also need a clock-names property since there are two clocks?

> +
> +Example:
> +
> +	sata: ahci@218000 {
> +		compatible = "ti,da850-ahci";
> +		reg = <0x218000 0x2000>, <0x22c018 0x4>;
> +		interrupts = <67>;
> +	};
>

It would be nice to have clocks and clock-names in the example as well.

^ permalink raw reply

* Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline
From: Christopher Spinrath @ 2017-01-17 18:35 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	grinberg-UTxiZqZC01RS1MOuV/RT9w, fabio.estevam-3arQi8VN3Tc,
	christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg
In-Reply-To: <88bdfd5321484efc82af4132ac2f0d7e-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>

Hi Philipp,

thanks for the review!

On 01/17/2017 09:57 AM, Philipp Zabel wrote:
> [...]
>> +
>> +	parallel-display {
>> +		compatible = "fsl,imx-parallel-display";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ipu1>;
>> +
>> +		interface-pix-fmt = "rgb24";
>
> This is not necessary if the connector created by the tpf410 has the
> correct media bus format set in its display_info structure. This can be
> done in tfp410_attach, before calling drm_mode_connector_attach_encoder:
>
>         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>
> 	drm_display_info_set_bus_formats(&dvi->connector.display_info,
> 					 &bus_format, 1);
>
> After this is done, the above line should be removed in a follow-up
> patch.

Ok, I will send a mini follow-up series doing that with your 
Suggested-by (unless you object) in the next few days.

Cheers,
Christopher

>> +		port@0 {
>> +			reg = <0>;
>> +
>> +			parallel_display_in: endpoint {
>> +				remote-endpoint = <&ipu1_di0_disp0>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			parallel_display_out: endpoint {
>> +				remote-endpoint = <&tfp410_in>;
>> +			};
>> +		};
>> +	};
>>  };
>>  [...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox