diff mbox series

[v1,17/25] arm64: dts: colibri-imx8x: eval: Add spi-to-can

Message ID 20230308125300.58244-18-dev@pschenker.ch
State New
Headers
Series Update Colibri iMX8X Devicetrees |

Commit Message

Philippe Schenker March 8, 2023, 12:52 p.m. UTC 
  From: Philippe Schenker <philippe.schenker@toradex.com>

Add mcp2515 spi-to-can to &lpspi2.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 .../dts/freescale/imx8x-colibri-eval-v3.dtsi  | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Philippe Schenker March 8, 2023, 1:43 p.m. UTC | #1 
On Wed, 2023-03-08 at 14:00 +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 13:52, Philippe Schenker wrote:
> > From: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > Add mcp2515 spi-to-can to &lpspi2.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> > 
> >  .../dts/freescale/imx8x-colibri-eval-v3.dtsi  | 19
> > +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-
> > v3.dtsi b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> > index 625d2caaf5d1..e7e3cf462408 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> 
> There is no such file.

Thanks for your feedback!

This file is being created int he first patch in the series.

> 
> > @@ -11,6 +11,13 @@ aliases {
> >                 rtc1 = &rtc;
> >         };
> >  
> > +       /* fixed crystal dedicated to mcp25xx */
> > +       clk16m: clock-16mhz-fixed {
> 
> Drop "fixed".

I'll prepare a v2 and plan to send it out beginning of next week.

> 
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <16000000>;
> > +       };
> > +
> >         gpio-keys {
> >                 compatible = "gpio-keys";
> >                 pinctrl-names = "default";
> > @@ -44,6 +51,18 @@ rtc_i2c: rtc@68 {
> >  /* Colibri SPI */
> >  &lpspi2 {
> >         status = "okay";
> > +
> > +       mcp2515: can@0 {
> > +               compatible = "microchip,mcp2515";
> > +               reg = <0>;
> > +               interrupt-parent = <&lsio_gpio3>;
> > +               interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
> > +               pinctrl-0 = <&pinctrl_can_int>;
> > +               pinctrl-names = "default";
> > +               clocks = <&clk16m>;
> 
> You just sorted all nodes in previous patches and add something
> unsorted? What is then the style of order? Random name?

My logic behind this one is

1. compatible property
2. reg property
3. standard properties 
   - first interrupt
   - then pinctrl
4. specific properties
   - again alphabetically: clocks, spi-max-frequency
5. status

How would you do it?

> 
> > +               spi-max-frequency = <10000000>;
> > +               status = "okay";
> 
> Why do you need it?

This chip is placed on our eval-board and we usually try to enable it so
someone can right away use CAN.

> 
> > +       };
> >  };
> >  
> >  /* Colibri UART_B */
> 
> Best regards,
> Krzysztof
>
Philippe Schenker March 8, 2023, 2:12 p.m. UTC | #2 
On Wed, 2023-03-08 at 14:00 +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 13:52, Philippe Schenker wrote:
> > From: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > Add mcp2515 spi-to-can to &lpspi2.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> > 
> >  .../dts/freescale/imx8x-colibri-eval-v3.dtsi  | 19
> > +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-
> > v3.dtsi b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> > index 625d2caaf5d1..e7e3cf462408 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
> 
> There is no such file.
> 
> > @@ -11,6 +11,13 @@ aliases {
> >                 rtc1 = &rtc;
> >         };
> >  
> > +       /* fixed crystal dedicated to mcp25xx */
> > +       clk16m: clock-16mhz-fixed {
> 
> Drop "fixed".
> 
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <16000000>;
> > +       };
> > +
> >         gpio-keys {
> >                 compatible = "gpio-keys";
> >                 pinctrl-names = "default";
> > @@ -44,6 +51,18 @@ rtc_i2c: rtc@68 {
> >  /* Colibri SPI */
> >  &lpspi2 {
> >         status = "okay";
> > +
> > +       mcp2515: can@0 {
> > +               compatible = "microchip,mcp2515";
> > +               reg = <0>;
> > +               interrupt-parent = <&lsio_gpio3>;
> > +               interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
> > +               pinctrl-0 = <&pinctrl_can_int>;
> > +               pinctrl-names = "default";
> > +               clocks = <&clk16m>;
> 
> You just sorted all nodes in previous patches and add something
> unsorted? What is then the style of order? Random name?
> 
> > +               spi-max-frequency = <10000000>;
> > +               status = "okay";
> 
> Why do you need it?

Ok, now I know what you mean. Will remove it in v2.

> 
> > +       };
> >  };
> >  
> >  /* Colibri UART_B */
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 2:34 p.m. UTC | #3 
On 08/03/2023 14:43, Philippe Schenker wrote:
>>> +       mcp2515: can@0 {
>>> +               compatible = "microchip,mcp2515";
>>> +               reg = <0>;
>>> +               interrupt-parent = <&lsio_gpio3>;
>>> +               interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
>>> +               pinctrl-0 = <&pinctrl_can_int>;
>>> +               pinctrl-names = "default";
>>> +               clocks = <&clk16m>;
>>
>> You just sorted all nodes in previous patches and add something
>> unsorted? What is then the style of order? Random name?
> 
> My logic behind this one is
> 
> 1. compatible property
> 2. reg property
> 3. standard properties 
>    - first interrupt
>    - then pinctrl
> 4. specific properties
>    - again alphabetically: clocks, spi-max-frequency

clocks and spi-max-frequency are standard properties.

BTW, what is a specific property?

Best regards,
Krzysztof
Philippe Schenker March 8, 2023, 2:46 p.m. UTC | #4 
On Wed, 2023-03-08 at 15:34 +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 14:43, Philippe Schenker wrote:
> > > > +       mcp2515: can@0 {
> > > > +               compatible = "microchip,mcp2515";
> > > > +               reg = <0>;
> > > > +               interrupt-parent = <&lsio_gpio3>;
> > > > +               interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
> > > > +               pinctrl-0 = <&pinctrl_can_int>;
> > > > +               pinctrl-names = "default";
> > > > +               clocks = <&clk16m>;
> > > 
> > > You just sorted all nodes in previous patches and add something
> > > unsorted? What is then the style of order? Random name?
> > 
> > My logic behind this one is
> > 
> > 1. compatible property
> > 2. reg property
> > 3. standard properties 
> >    - first interrupt
> >    - then pinctrl
> > 4. specific properties
> >    - again alphabetically: clocks, spi-max-frequency
> 
> clocks and spi-max-frequency are standard properties.
> 
> BTW, what is a specific property?

I mean specific to this driver. With standard properties I mean those
you can add everywhere like pinctrl, interrupts etc. But then yes I
agree you can pretty quickly start to argue...

Would in this particular case something like


	mcp2515: can@0 {
		compatible = "microchip,mcp2515";
		reg = <0>;
		pinctrl-0 = <&pinctrl_can_int>;
		pinctrl-names = "default";
		clocks = <&clk16m>;
		interrupt-parent = <&lsio_gpio3>;
		interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
		spi-max-frequency = <10000000>;
	};

be better?


> 
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
index 625d2caaf5d1..e7e3cf462408 100644
--- a/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8x-colibri-eval-v3.dtsi
@@ -11,6 +11,13 @@  aliases {
 		rtc1 = &rtc;
 	};
 
+	/* fixed crystal dedicated to mcp25xx */
+	clk16m: clock-16mhz-fixed {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <16000000>;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		pinctrl-names = "default";
@@ -44,6 +51,18 @@  rtc_i2c: rtc@68 {
 /* Colibri SPI */
 &lpspi2 {
 	status = "okay";
+
+	mcp2515: can@0 {
+		compatible = "microchip,mcp2515";
+		reg = <0>;
+		interrupt-parent = <&lsio_gpio3>;
+		interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+		pinctrl-0 = <&pinctrl_can_int>;
+		pinctrl-names = "default";
+		clocks = <&clk16m>;
+		spi-max-frequency = <10000000>;
+		status = "okay";
+	};
 };
 
 /* Colibri UART_B */