[PATCH v41 0/3] can: add mcp25xxfd support

[PATCH v41 0/3] can: add mcp25xxfd support

[Marc Kleine-Budde]

Hello,

this series adds support for the Microchip MCP25xxFD SPI CAN controller family.

The series is available at:

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-41

regards,
Marc

[PATCH v41 1/3] can: rx-offload: can_rx_offload_add_manual(): add new initialization function

[Marc Kleine-Budde]

This patch adds a new initialization function:
can_rx_offload_add_manual()

It should be used to add support rx-offload to a driver, if the callback
mechanism should not be used. Use e.g. can_rx_offload_queue_sorted() to queue
skbs into rx-offload.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/rx-offload.c   | 11 +++++++++++
 include/linux/can/rx-offload.h |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/can/rx-offload.c b/drivers/net/can/rx-offload.c
index e8328910a234..3b180269a92d 100644
--- a/drivers/net/can/rx-offload.c
+++ b/drivers/net/can/rx-offload.c
@@ -351,6 +351,17 @@ int can_rx_offload_add_fifo(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_add_fifo);
 
+int can_rx_offload_add_manual(struct net_device *dev,
+			      struct can_rx_offload *offload,
+			      unsigned int weight)
+{
+	if (offload->mailbox_read)
+		return -EINVAL;
+
+	return can_rx_offload_init_queue(dev, offload, weight);
+}
+EXPORT_SYMBOL_GPL(can_rx_offload_add_manual);
+
 void can_rx_offload_enable(struct can_rx_offload *offload)
 {
 	napi_enable(&offload->napi);
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index 1b78a0cfb615..f1b38088b765 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -35,6 +35,9 @@ int can_rx_offload_add_timestamp(struct net_device *dev,
 int can_rx_offload_add_fifo(struct net_device *dev,
 			    struct can_rx_offload *offload,
 			    unsigned int weight);
+int can_rx_offload_add_manual(struct net_device *dev,
+			      struct can_rx_offload *offload,
+			      unsigned int weight);
 int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload,
 					 u64 reg);
 int can_rx_offload_irq_offload_fifo(struct can_rx_offload *offload);
-- 
2.27.0

[PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Marc Kleine-Budde]

From: Oleksij Rempel <o.rempel@pengutronix.de>

This patch adds the device-tree binding documentation for the Microchip
MCP25xxFD SPI CAN controller family.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml

diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
new file mode 100644
index 000000000000..4993dd49181c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
+
+maintainers:
+  - Marc Kleine-Budde <mkl@pengutronix.de>
+
+properties:
+  compatible:
+    oneOf:
+      - const: microchip,mcp2517fd
+        description: for MCP2517FD
+      - const: microchip,mcp2518fd
+        description: for MCP2518FD
+      - const: microchip,mcp25xxfd
+        description: to autodetect chip variant
+
+  reg:
+    maxItems: 1
+
+  interrupts-extended:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that powers the CAN controller.
+    maxItems: 1
+
+  xceiver-supply:
+    description: Regulator that powers the CAN transceiver.
+    maxItems: 1
+
+  rx-int-gpios:
+    description:
+      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
+      signals a pending RX interrupt.
+    maxItems: 1
+
+  spi-max-frequency:
+    description:
+      Must be half or less of "clocks" frequency.
+    maximum: 20000000
+
+required:
+  - compatible
+  - reg
+  - interrupts-extended
+  - clocks
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        can@0 {
+            compatible = "microchip,mcp25xxfd";
+            reg = <0>;
+            clocks = <&can0_osc>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&can0_pins>;
+            spi-max-frequency = <20000000>;
+            interrupts-extended = <&gpio 13 IRQ_TYPE_LEVEL_LOW>;
+            rx-int-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&reg5v0>;
+            xceiver-supply = <&reg5v0>;
+        };
+    };
-- 
2.27.0

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Manivannan Sadhasivam]

Hi,

On Mon, Jun 22, 2020 at 01:46:02PM +0200, Marc Kleine-Budde wrote:
> From: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> This patch adds the device-tree binding documentation for the Microchip
> MCP25xxFD SPI CAN controller family.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

You need to CC Rob and devicetree list to get a review for this patch.

> ---
>  .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> new file mode 100644
> index 000000000000..4993dd49181c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
> +

MCP251{7/8}FD?

> +maintainers:
> +  - Marc Kleine-Budde <mkl@pengutronix.de>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: microchip,mcp2517fd
> +        description: for MCP2517FD
> +      - const: microchip,mcp2518fd
> +        description: for MCP2518FD
> +      - const: microchip,mcp25xxfd
> +        description: to autodetect chip variant
> +

Actually what benefit this generic compatible provides? User who is integrating
this driver should know the exact controller instance he is playing with, isn't
it?

> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    maxItems: 1
> +

Document this just above 'interrupts' property.

> +  clocks:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Regulator that powers the CAN controller.
> +    maxItems: 1
> +
> +  xceiver-supply:
> +    description: Regulator that powers the CAN transceiver.
> +    maxItems: 1
> +
> +  rx-int-gpios:
> +    description:
> +      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
> +      signals a pending RX interrupt.
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      Must be half or less of "clocks" frequency.
> +    maximum: 20000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts-extended
> +  - clocks
> +

The controller is capable of generating clocks and also able to control few
GPIOs. So eventually you need to document those properties in bindings even
your driver is not supporting all of them atm.

If you want you can take a look at the bindings patch I posted earlier:

dt-bindings: can: Document devicetree bindings for MCP25XXFD

Thanks,
Mani

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        can@0 {
> +            compatible = "microchip,mcp25xxfd";
> +            reg = <0>;
> +            clocks = <&can0_osc>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&can0_pins>;
> +            spi-max-frequency = <20000000>;
> +            interrupts-extended = <&gpio 13 IRQ_TYPE_LEVEL_LOW>;
> +            rx-int-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&reg5v0>;
> +            xceiver-supply = <&reg5v0>;
> +        };
> +    };
> -- 
> 2.27.0
> 

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Marc Kleine-Budde]

On 6/22/20 6:53 PM, Manivannan Sadhasivam wrote:
> Hi,
> 
> On Mon, Jun 22, 2020 at 01:46:02PM +0200, Marc Kleine-Budde wrote:
>> From: Oleksij Rempel <o.rempel@pengutronix.de>
>>
>> This patch adds the device-tree binding documentation for the Microchip
>> MCP25xxFD SPI CAN controller family.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> You need to CC Rob and devicetree list to get a review for this patch.

Will do for next round.

> 
>> ---
>>  .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
>> new file mode 100644
>> index 000000000000..4993dd49181c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
>> +
> 
> MCP251{7/8}FD?

Which expansion rules should be use for the title? In sh-like shells it would be
MCP251{7,8}FD.

> 
>> +maintainers:
>> +  - Marc Kleine-Budde <mkl@pengutronix.de>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - const: microchip,mcp2517fd
>> +        description: for MCP2517FD
>> +      - const: microchip,mcp2518fd
>> +        description: for MCP2518FD
>> +      - const: microchip,mcp25xxfd
>> +        description: to autodetect chip variant
>> +
> 
> Actually what benefit this generic compatible provides? User who is integrating
> this driver should know the exact controller instance he is playing with, isn't
> it?

As the chip variant can be autodetected why not do it? It makes device tree
overlays (e.g. for the rpi much simpler), as you don't have to specify the exact
chip variant.

Testing is much easier, as I don't have to change the overlays if swapping the
CAN hat.

> 
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts-extended:
>> +    maxItems: 1
>> +
> 
> Document this just above 'interrupts' property.

Do you mean I should change the order?
- reg:
- clocks:
- interrupts-extended:

> 
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  vdd-supply:
>> +    description: Regulator that powers the CAN controller.
>> +    maxItems: 1
>> +
>> +  xceiver-supply:
>> +    description: Regulator that powers the CAN transceiver.
>> +    maxItems: 1
>> +
>> +  rx-int-gpios:
>> +    description:
>> +      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
>> +      signals a pending RX interrupt.
>> +    maxItems: 1
>> +
>> +  spi-max-frequency:
>> +    description:
>> +      Must be half or less of "clocks" frequency.
>> +    maximum: 20000000
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts-extended
>> +  - clocks
>> +
> 
> The controller is capable of generating clocks and also able to control few
> GPIOs. So eventually you need to document those properties in bindings even
> your driver is not supporting all of them atm.

I'd like to add support for clocks and GPIOs as soon as someone needs them. DT
bindings will go along with that. So far I have no customer that needs support
for that, do you?

> If you want you can take a look at the bindings patch I posted earlier:
> 
> dt-bindings: can: Document devicetree bindings for MCP25XXFD

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Manivannan Sadhasivam]

On Mon, Jun 22, 2020 at 08:12:58PM +0200, Marc Kleine-Budde wrote:
> On 6/22/20 6:53 PM, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > On Mon, Jun 22, 2020 at 01:46:02PM +0200, Marc Kleine-Budde wrote:
> >> From: Oleksij Rempel <o.rempel@pengutronix.de>
> >>
> >> This patch adds the device-tree binding documentation for the Microchip
> >> MCP25xxFD SPI CAN controller family.
> >>
> >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > 
> > You need to CC Rob and devicetree list to get a review for this patch.
> 
> Will do for next round.
> 
> > 
> >> ---
> >>  .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
> >>  1 file changed, 77 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> >> new file mode 100644
> >> index 000000000000..4993dd49181c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> >> @@ -0,0 +1,77 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
> >> +
> > 
> > MCP251{7/8}FD?
> 
> Which expansion rules should be use for the title? In sh-like shells it would be
> MCP251{7,8}FD.
> 

Either one. I was just concerned about the original one which might create
ambiguity.

> > 
> >> +maintainers:
> >> +  - Marc Kleine-Budde <mkl@pengutronix.de>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - const: microchip,mcp2517fd
> >> +        description: for MCP2517FD
> >> +      - const: microchip,mcp2518fd
> >> +        description: for MCP2518FD
> >> +      - const: microchip,mcp25xxfd
> >> +        description: to autodetect chip variant
> >> +
> > 
> > Actually what benefit this generic compatible provides? User who is integrating
> > this driver should know the exact controller instance he is playing with, isn't
> > it?
> 
> As the chip variant can be autodetected why not do it? It makes device tree
> overlays (e.g. for the rpi much simpler), as you don't have to specify the exact
> chip variant.
> 
> Testing is much easier, as I don't have to change the overlays if swapping the
> CAN hat.
> 

Okay.

> > 
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts-extended:
> >> +    maxItems: 1
> >> +
> > 
> > Document this just above 'interrupts' property.
> 
> Do you mean I should change the order?
> - reg:
> - clocks:
> - interrupts-extended:
> 

Sorry, please ignore this comment. You can keep the order as it is.

> > 
> >> +  clocks:
> >> +    maxItems: 1
> >> +
> >> +  vdd-supply:
> >> +    description: Regulator that powers the CAN controller.
> >> +    maxItems: 1
> >> +
> >> +  xceiver-supply:
> >> +    description: Regulator that powers the CAN transceiver.
> >> +    maxItems: 1
> >> +
> >> +  rx-int-gpios:

This doesn't look like a standard property. So I think you need to add
'microchip' prefix to make it as vendor specific.

> >> +    description:
> >> +      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
> >> +      signals a pending RX interrupt.
> >> +    maxItems: 1
> >> +
> >> +  spi-max-frequency:
> >> +    description:
> >> +      Must be half or less of "clocks" frequency.
> >> +    maximum: 20000000
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts-extended
> >> +  - clocks
> >> +
> > 
> > The controller is capable of generating clocks and also able to control few
> > GPIOs. So eventually you need to document those properties in bindings even
> > your driver is not supporting all of them atm.
> 
> I'd like to add support for clocks and GPIOs as soon as someone needs them. DT
> bindings will go along with that. So far I have no customer that needs support
> for that, do you?
> 

DT binding should describe what the controller is capable of and not the
capability of the driver. You can always add functionality to driver but binding
should stay as it is (although there are exceptions...).

Thanks,
Mani

> > If you want you can take a look at the bindings patch I posted earlier:
> > 
> > dt-bindings: can: Document devicetree bindings for MCP25XXFD
> 
> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Oleksij Rempel]

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

On Tue, Jun 23, 2020 at 08:56:35AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 22, 2020 at 08:12:58PM +0200, Marc Kleine-Budde wrote:
> > On 6/22/20 6:53 PM, Manivannan Sadhasivam wrote:
> > > Hi,
> > > 
> > > On Mon, Jun 22, 2020 at 01:46:02PM +0200, Marc Kleine-Budde wrote:
> > >> From: Oleksij Rempel <o.rempel@pengutronix.de>
> > >>
> > >> This patch adds the device-tree binding documentation for the Microchip
> > >> MCP25xxFD SPI CAN controller family.
> > >>
> > >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > 
> > > You need to CC Rob and devicetree list to get a review for this patch.
> > 
> > Will do for next round.
> > 
> > > 
> > >> ---
> > >>  .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
> > >>  1 file changed, 77 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > >> new file mode 100644
> > >> index 000000000000..4993dd49181c
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > >> @@ -0,0 +1,77 @@
> > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
> > >> +
> > > 
> > > MCP251{7/8}FD?
> > 
> > Which expansion rules should be use for the title? In sh-like shells it would be
> > MCP251{7,8}FD.
> > 
> 
> Either one. I was just concerned about the original one which might create
> ambiguity.
> 
> > > 
> > >> +maintainers:
> > >> +  - Marc Kleine-Budde <mkl@pengutronix.de>
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    oneOf:
> > >> +      - const: microchip,mcp2517fd
> > >> +        description: for MCP2517FD
> > >> +      - const: microchip,mcp2518fd
> > >> +        description: for MCP2518FD
> > >> +      - const: microchip,mcp25xxfd
> > >> +        description: to autodetect chip variant
> > >> +
> > > 
> > > Actually what benefit this generic compatible provides? User who is integrating
> > > this driver should know the exact controller instance he is playing with, isn't
> > > it?
> > 
> > As the chip variant can be autodetected why not do it? It makes device tree
> > overlays (e.g. for the rpi much simpler), as you don't have to specify the exact
> > chip variant.
> > 
> > Testing is much easier, as I don't have to change the overlays if swapping the
> > CAN hat.
> > 
> 
> Okay.
> 
> > > 
> > >> +  reg:
> > >> +    maxItems: 1
> > >> +
> > >> +  interrupts-extended:
> > >> +    maxItems: 1
> > >> +
> > > 
> > > Document this just above 'interrupts' property.
> > 
> > Do you mean I should change the order?
> > - reg:
> > - clocks:
> > - interrupts-extended:
> > 
> 
> Sorry, please ignore this comment. You can keep the order as it is.
> 
> > > 
> > >> +  clocks:
> > >> +    maxItems: 1
> > >> +
> > >> +  vdd-supply:
> > >> +    description: Regulator that powers the CAN controller.
> > >> +    maxItems: 1
> > >> +
> > >> +  xceiver-supply:
> > >> +    description: Regulator that powers the CAN transceiver.
> > >> +    maxItems: 1
> > >> +
> > >> +  rx-int-gpios:
> 
> This doesn't look like a standard property. So I think you need to add
> 'microchip' prefix to make it as vendor specific.
> 
> > >> +    description:
> > >> +      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
> > >> +      signals a pending RX interrupt.
> > >> +    maxItems: 1
> > >> +
> > >> +  spi-max-frequency:
> > >> +    description:
> > >> +      Must be half or less of "clocks" frequency.
> > >> +    maximum: 20000000
> > >> +
> > >> +required:
> > >> +  - compatible
> > >> +  - reg
> > >> +  - interrupts-extended
> > >> +  - clocks
> > >> +
> > > 
> > > The controller is capable of generating clocks and also able to control few
> > > GPIOs. So eventually you need to document those properties in bindings even
> > > your driver is not supporting all of them atm.
> > 
> > I'd like to add support for clocks and GPIOs as soon as someone needs them. DT
> > bindings will go along with that. So far I have no customer that needs support
> > for that, do you?
> > 
> 
> DT binding should describe what the controller is capable of and not the
> capability of the driver. You can always add functionality to driver but binding
> should stay as it is (although there are exceptions...).

Adding binding for not implemented functionality adds more confusion:
- without implementing it, you do not know, how exactly should it be
  done. Should we request gpio as gpio or as irq? This will affect
  actual bindings.
- we need to care about backwards compatibility, implementing binding
  before knowing what they will do, will make driver development even harder.
  You need to care about quirks for bogus binding which was actually
  never used.
- Extending a binding can be always done if needed. Making things "just
  in case" will increase development overhead by reducing quality.

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Manivannan Sadhasivam]

On Tue, Jun 23, 2020 at 08:02:01AM +0200, Oleksij Rempel wrote:
> On Tue, Jun 23, 2020 at 08:56:35AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 22, 2020 at 08:12:58PM +0200, Marc Kleine-Budde wrote:
> > > On 6/22/20 6:53 PM, Manivannan Sadhasivam wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Jun 22, 2020 at 01:46:02PM +0200, Marc Kleine-Budde wrote:
> > > >> From: Oleksij Rempel <o.rempel@pengutronix.de>
> > > >>
> > > >> This patch adds the device-tree binding documentation for the Microchip
> > > >> MCP25xxFD SPI CAN controller family.
> > > >>
> > > >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > > 
> > > > You need to CC Rob and devicetree list to get a review for this patch.
> > > 
> > > Will do for next round.
> > > 
> > > > 
> > > >> ---
> > > >>  .../bindings/net/can/microchip,mcp25xxfd.yaml | 77 +++++++++++++++++++
> > > >>  1 file changed, 77 insertions(+)
> > > >>  create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > > >> new file mode 100644
> > > >> index 000000000000..4993dd49181c
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp25xxfd.yaml
> > > >> @@ -0,0 +1,77 @@
> > > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > >> +%YAML 1.2
> > > >> +---
> > > >> +$id: http://devicetree.org/schemas/net/can/microchip,mcp25xxfd.yaml#
> > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > >> +
> > > >> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
> > > >> +

[...]

> > > >> +required:
> > > >> +  - compatible
> > > >> +  - reg
> > > >> +  - interrupts-extended
> > > >> +  - clocks
> > > >> +
> > > > 
> > > > The controller is capable of generating clocks and also able to control few
> > > > GPIOs. So eventually you need to document those properties in bindings even
> > > > your driver is not supporting all of them atm.
> > > 
> > > I'd like to add support for clocks and GPIOs as soon as someone needs them. DT
> > > bindings will go along with that. So far I have no customer that needs support
> > > for that, do you?
> > > 
> > 
> > DT binding should describe what the controller is capable of and not the
> > capability of the driver. You can always add functionality to driver but binding
> > should stay as it is (although there are exceptions...).
> 
> Adding binding for not implemented functionality adds more confusion:
> - without implementing it, you do not know, how exactly should it be
>   done. Should we request gpio as gpio or as irq? This will affect
>   actual bindings.
> - we need to care about backwards compatibility, implementing binding
>   before knowing what they will do, will make driver development even harder.
>   You need to care about quirks for bogus binding which was actually
>   never used.
> - Extending a binding can be always done if needed. Making things "just
>   in case" will increase development overhead by reducing quality.
> 

This is not my call. Rob asked me to document all device specific properties
in binding before for some other controller. Let's see his response in next
iteration (we need to ask this question though).

Thanks,
Mani

> Regards,
> Oleksij
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 2/3] dt-binding: can: mcp25xxfd: document device tree bindings

[Marc Kleine-Budde]

On 6/23/20 5:26 AM, Manivannan Sadhasivam wrote:
>>>> +title: Microchip MCP2517/18FD stand-alone CAN controller device tree bindings
>>>> +
>>>
>>> MCP251{7/8}FD?
>>
>> Which expansion rules should be use for the title? In sh-like shells it would be
>> MCP251{7,8}FD.
>>
> 
> Either one. I was just concerned about the original one which might create
> ambiguity.

I've changed this to:

> Microchip MCP2517FD and MCP2518FD stand-alone CAN controller device tree
> bindings

[...]

>>>> +  rx-int-gpios:
> 
> This doesn't look like a standard property. So I think you need to add
> 'microchip' prefix to make it as vendor specific.

makes sense.

>>>> +    description:
>>>> +      GPIO phandle of GPIO connected to to INT1 pin of the MCP25XXFD, which
>>>> +      signals a pending RX interrupt.
>>>> +    maxItems: 1
>>>> +
>>>> +  spi-max-frequency:
>>>> +    description:
>>>> +      Must be half or less of "clocks" frequency.
>>>> +    maximum: 20000000
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts-extended
>>>> +  - clocks
>>>> +
>>>
>>> The controller is capable of generating clocks and also able to control few
>>> GPIOs. So eventually you need to document those properties in bindings even
>>> your driver is not supporting all of them atm.
>>
>> I'd like to add support for clocks and GPIOs as soon as someone needs them. DT
>> bindings will go along with that. So far I have no customer that needs support
>> for that, do you?>
> DT binding should describe what the controller is capable of and not the
> capability of the driver. You can always add functionality to driver but binding
> should stay as it is (although there are exceptions...).
As Oleksij pointed out, this make driver development very complicated. I'm not
even sure how to properly abstract the GPIO support. For open drain pinctrl
would be best, haveing one microchip,open-drain property in the DT would effect
all GPIOs.

Further this means I have to look into the driver if a documented property is
implemented and does what it is advertised to do? That sounds very counter
intuitive for me.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 0/3] can: add mcp25xxfd support

[Manivannan Sadhasivam]

On Mon, Jun 22, 2020 at 01:46:00PM +0200, Marc Kleine-Budde wrote:
> Hello,
> 
> this series adds support for the Microchip MCP25xxFD SPI CAN controller family.
> 
> The series is available at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-41
> 

Tested this series on an SM8250 based board connected to MCP2518FD over SPI.
Hence,

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> regards,
> Marc
> 
> 

Re: [PATCH v41 0/3] can: add mcp25xxfd support

[Marc Kleine-Budde]

On 6/23/20 2:46 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 22, 2020 at 01:46:00PM +0200, Marc Kleine-Budde wrote:
>> Hello,
>>
>> this series adds support for the Microchip MCP25xxFD SPI CAN controller family.
>>
>> The series is available at:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-41
>>
> 
> Tested this series on an SM8250 based board connected to MCP2518FD over SPI.
> Hence,
> 
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

tnx.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 3/3] can: mcp25xxfd: initial commit

[Marc Kleine-Budde]

On 6/26/20 3:32 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 22, 2020 at 01:46:03PM +0200, Marc Kleine-Budde wrote:
>> This patch add support for the Microchip MCP25xxFD SPI CAN controller family.
>>
>> Pending-Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
>> Pending-Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Could you please split this patch into multiple ones? Having ~4k lines for a
> patch makes it difficult to review. I know that some parts are difficult to
> split (happened with my series as well) but anything below 1k should be fine.

For now I split the regmap and crc16 into one patch, the core file into a
separate patch.

See:
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-45

If you want to have it in smaller pieces I'll need more time to figure out a
sensible way to split the driver.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 3/3] can: mcp25xxfd: initial commit

[Manivannan Sadhasivam]

On Fri, Jun 26, 2020 at 04:41:53PM +0200, Marc Kleine-Budde wrote:
> On 6/26/20 3:32 PM, Manivannan Sadhasivam wrote:
> > On Mon, Jun 22, 2020 at 01:46:03PM +0200, Marc Kleine-Budde wrote:
> >> This patch add support for the Microchip MCP25xxFD SPI CAN controller family.
> >>
> >> Pending-Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> >> Pending-Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > 
> > Could you please split this patch into multiple ones? Having ~4k lines for a
> > patch makes it difficult to review. I know that some parts are difficult to
> > split (happened with my series as well) but anything below 1k should be fine.
> 
> For now I split the regmap and crc16 into one patch, the core file into a
> separate patch.
> 
> See:
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-45
> 
> If you want to have it in smaller pieces I'll need more time to figure out a
> sensible way to split the driver.
> 

Can you please post the split version here so that I can do a formal review?

Thanks,
Mani

> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 3/3] can: mcp25xxfd: initial commit

[Manivannan Sadhasivam]

On Thu, Jul 16, 2020 at 06:58:00PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jun 26, 2020 at 04:41:53PM +0200, Marc Kleine-Budde wrote:
> > On 6/26/20 3:32 PM, Manivannan Sadhasivam wrote:
> > > On Mon, Jun 22, 2020 at 01:46:03PM +0200, Marc Kleine-Budde wrote:
> > >> This patch add support for the Microchip MCP25xxFD SPI CAN controller family.
> > >>
> > >> Pending-Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > >> Pending-Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > 
> > > Could you please split this patch into multiple ones? Having ~4k lines for a
> > > patch makes it difficult to review. I know that some parts are difficult to
> > > split (happened with my series as well) but anything below 1k should be fine.
> > 
> > For now I split the regmap and crc16 into one patch, the core file into a
> > separate patch.
> > 
> > See:
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-45
> > 
> > If you want to have it in smaller pieces I'll need more time to figure out a
> > sensible way to split the driver.
> > 
> 
> Can you please post the split version here so that I can do a formal review?
> 

Is there any update on the next iteration? I'd like to see this series moving
forward :)

Thanks,
Mani

> Thanks,
> Mani
> 
> > regards,
> > Marc
> > 
> > -- 
> > Pengutronix e.K.                 | Marc Kleine-Budde           |
> > Embedded Linux                   | https://www.pengutronix.de  |
> > Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> > Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v41 3/3] can: mcp25xxfd: initial commit

[Kurt Van Dijck]

On vr, 26 jun 2020 19:02:43 +0530, Manivannan Sadhasivam wrote:
> Hi,
> 
> On Mon, Jun 22, 2020 at 01:46:03PM +0200, Marc Kleine-Budde wrote:
> > This patch add support for the Microchip MCP25xxFD SPI CAN controller family.
> > 
> > Pending-Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > Pending-Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Could you please split this patch into multiple ones? Having ~4k lines for a
> patch makes it difficult to review. I know that some parts are difficult to
> split (happened with my series as well) but anything below 1k should be fine.

IMHO, there is a huge difference between a complete new driver of 4k
lines and a change of 100 lines.
It's useless to add a non-functional driver, only to add functionality
later on.

You're right concerning real changes to a driver.

Just my opinion ...

From what I see, Marc did a good job, providing minimal functionality in
the first series. now, the driver can evolve (like adding listen-only :-) )

Kurt

> 
> Thanks,
> Mani
> 
> > ---
> >  drivers/net/can/spi/Kconfig                   |    2 +
> >  drivers/net/can/spi/Makefile                  |    1 +
> >  drivers/net/can/spi/mcp25xxfd/Kconfig         |   17 +
> >  drivers/net/can/spi/mcp25xxfd/Makefile        |    8 +
> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-core.c    | 2890 +++++++++++++++++
> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-crc16.c   |   89 +
> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-regmap.c  |  554 ++++
> >  drivers/net/can/spi/mcp25xxfd/mcp25xxfd.h     |  828 +++++
> >  8 files changed, 4389 insertions(+)
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/Kconfig
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/Makefile
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd-crc16.c
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd-regmap.c
> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd.h
> > 

Re: [PATCH v41 3/3] can: mcp25xxfd: initial commit

[Manivannan Sadhasivam]

On 28 June 2020 1:26:05 AM IST, Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
>On vr, 26 jun 2020 19:02:43 +0530, Manivannan Sadhasivam wrote:
>> Hi,
>> 
>> On Mon, Jun 22, 2020 at 01:46:03PM +0200, Marc Kleine-Budde wrote:
>> > This patch add support for the Microchip MCP25xxFD SPI CAN
>controller family.
>> > 
>> > Pending-Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
>> > Pending-Tested-by: Manivannan Sadhasivam
><manivannan.sadhasivam@linaro.org>
>> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> 
>> Could you please split this patch into multiple ones? Having ~4k
>lines for a
>> patch makes it difficult to review. I know that some parts are
>difficult to
>> split (happened with my series as well) but anything below 1k should
>be fine.
>
>IMHO, there is a huge difference between a complete new driver of 4k
>lines and a change of 100 lines.
>It's useless to add a non-functional driver, only to add functionality
>later on.
>

I just asked to split commits in this series logically. Because, its hard for the reviewers to do the review of a bulk patch. This is an unwritten rule of the kernel community... 

>You're right concerning real changes to a driver.
>
>Just my opinion ...
>
From what I see, Marc did a good job, providing minimal functionality
>in
>the first series. now, the driver can evolve (like adding listen-only
>:-) )

Again, I have no concern over the patch series. I'm also planning to submit incremental patches on top of this. 

Thanks, 
Mani 
>
>Kurt
>
>> 
>> Thanks,
>> Mani
>> 
>> > ---
>> >  drivers/net/can/spi/Kconfig                   |    2 +
>> >  drivers/net/can/spi/Makefile                  |    1 +
>> >  drivers/net/can/spi/mcp25xxfd/Kconfig         |   17 +
>> >  drivers/net/can/spi/mcp25xxfd/Makefile        |    8 +
>> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-core.c    | 2890
>+++++++++++++++++
>> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-crc16.c   |   89 +
>> >  .../net/can/spi/mcp25xxfd/mcp25xxfd-regmap.c  |  554 ++++
>> >  drivers/net/can/spi/mcp25xxfd/mcp25xxfd.h     |  828 +++++
>> >  8 files changed, 4389 insertions(+)
>> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/Kconfig
>> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/Makefile
>> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c
>> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd-crc16.c
>> >  create mode 100644
>drivers/net/can/spi/mcp25xxfd/mcp25xxfd-regmap.c
>> >  create mode 100644 drivers/net/can/spi/mcp25xxfd/mcp25xxfd.h
>> > 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v41 0/3] can: add mcp25xxfd support

[Manivannan Sadhasivam]

On 0622, Marc Kleine-Budde wrote:
> Hello,
> 
> this series adds support for the Microchip MCP25xxFD SPI CAN controller family.
> 
> The series is available at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=mcp25xxfd-41
> 

Ping! Can you please post the splitted version?

Thanks,
Mani

> regards,
> Marc
> 
>