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

[Philippe Schenker]

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@xxxxxxxxxxx>
> > 
> > Add mcp2515 spi-to-can to &lpspi2.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@xxxxxxxxxxx>
> > ---
> > 
> >  .../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
>