[PATCH v2 0/3] Add support for lan966x flexcom chip-select configuration

[PATCH v2 0/3] Add support for lan966x flexcom chip-select configuration

[Kavyasree Kotagiri]

This patch series converts atmel-flexcom bindings into json-schema format.
Adds support for lan966x flexcom chip-select configurations and its
DT bindings.

v1 -> v2:
 - minor fix in title of dt-bindings.
 - Modified new dt properties usage in atmel,flexcom.yaml.
 - Used GENMASK and macros for maximum allowed values.
 - Use u32 values for flexcom chipselects instead of strings.
 - disable clock in case of errors.

Kavyasree Kotagiri (3):
  dt-bindings: mfd: atmel,flexcom: Convert to json-schema
  dt-bindings: mfd: atmel,flexcom: Add new compatible string for lan966x
  mfd: atmel-flexcom: Add support for lan966x flexcom chip-select
    configuration

 .../bindings/mfd/atmel,flexcom.yaml           | 134 ++++++++++++++++++
 .../devicetree/bindings/mfd/atmel-flexcom.txt |  63 --------
 drivers/mfd/atmel-flexcom.c                   |  93 +++++++++++-
 3 files changed, 226 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
 delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

-- 
2.17.1

[PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Kavyasree Kotagiri]

Convert the Atmel flexcom device tree bindings to json schema.

Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
---
v1 -> v2:
 - Fix title.

 .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
 .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
 2 files changed, 97 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
 delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
new file mode 100644
index 000000000000..05cb6ebb4b2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Atmel Flexcom (Flexible Serial Communication Unit)
+
+maintainers:
+  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
+
+description:
+  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
+  an I2C controller and an USART. Only one function can be used at a
+  time and is chosen at boot time according to the device tree.
+
+properties:
+  compatible:
+    const: atmel,sama5d2-flexcom
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  ranges:
+    description:
+      One range for the full I/O register region. (including USART,
+      TWI and SPI registers).
+    items:
+      maxItems: 3
+
+  atmel,flexcom-mode:
+    description: |
+      Specifies the flexcom mode as follows:
+      1: USART
+      2: SPI
+      3: I2C.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 3]
+
+patternProperties:
+  "^serial@[0-9a-f]+$":
+    description: See atmel-usart.txt for details of USART bindings.
+    type: object
+
+  "^spi@[0-9a-f]+$":
+    description: See ../spi/spi_atmel.txt for details of SPI bindings.
+    type: object
+
+  "^i2c@[0-9a-f]+$":
+    description: See ../i2c/i2c-at91.txt for details of I2C bindings.
+    type: object
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - atmel,flexcom-mode
+
+additionalProperties: false
+
+examples:
+  - |
+    flx0: flexcom@f8034000 {
+          compatible = "atmel,sama5d2-flexcom";
+          reg = <0xf8034000 0x200>;
+          clocks = <&flx0_clk>;
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges = <0x0 0xf8034000 0x800>;
+          atmel,flexcom-mode = <2>;
+
+          spi0: spi@400 {
+                compatible = "atmel,at91rm9200-spi";
+                reg = <0x400 0x200>;
+                interrupts = <19 4 7>;
+                pinctrl-names = "default";
+                pinctrl-0 = <&pinctrl_flx0_default>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+                clocks = <&flx0_clk>;
+                clock-names = "spi_clk";
+                atmel,fifo-size = <32>;
+          };
+    };
+...
diff --git a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
deleted file mode 100644
index 9d837535637b..000000000000
--- a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
+++ /dev/null
@@ -1,63 +0,0 @@
-* Device tree bindings for Atmel Flexcom (Flexible Serial Communication Unit)
-
-The Atmel Flexcom is just a wrapper which embeds a SPI controller, an I2C
-controller and an USART. Only one function can be used at a time and is chosen
-at boot time according to the device tree.
-
-Required properties:
-- compatible:		Should be "atmel,sama5d2-flexcom"
-- reg:			Should be the offset/length value for Flexcom dedicated
-			I/O registers (without USART, TWI or SPI registers).
-- clocks:		Should be the Flexcom peripheral clock from PMC.
-- #address-cells:	Should be <1>
-- #size-cells:		Should be <1>
-- ranges:		Should be one range for the full I/O register region
-			(including USART, TWI and SPI registers).
-- atmel,flexcom-mode:	Should be one of the following values:
-			- <1> for USART
-			- <2> for SPI
-			- <3> for I2C
-
-Required child:
-A single available child device of type matching the "atmel,flexcom-mode"
-property.
-
-The phandle provided by the clocks property of the child is the same as one for
-the Flexcom parent.
-
-For other properties, please refer to the documentations of the respective
-device:
-- ../serial/atmel-usart.txt
-- ../spi/spi_atmel.txt
-- ../i2c/i2c-at91.txt
-
-Example:
-
-flexcom@f8034000 {
-	compatible = "atmel,sama5d2-flexcom";
-	reg = <0xf8034000 0x200>;
-	clocks = <&flx0_clk>;
-	#address-cells = <1>;
-	#size-cells = <1>;
-	ranges = <0x0 0xf8034000 0x800>;
-	atmel,flexcom-mode = <2>;
-
-	spi@400 {
-		compatible = "atmel,at91rm9200-spi";
-		reg = <0x400 0x200>;
-		interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_flx0_default>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-		clocks = <&flx0_clk>;
-		clock-names = "spi_clk";
-		atmel,fifo-size = <32>;
-
-		flash@0 {
-			compatible = "atmel,at25f512b";
-			reg = <0>;
-			spi-max-frequency = <20000000>;
-		};
-	};
-};
-- 
2.17.1

Re: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Claudiu.Beznea]

On 07.06.2022 17:47, Kavyasree Kotagiri wrote:
> Convert the Atmel flexcom device tree bindings to json schema.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> ---
> v1 -> v2:
>  - Fix title.
> 
>  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
>  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
>  2 files changed, 97 insertions(+), 63 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> new file mode 100644
> index 000000000000..05cb6ebb4b2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel Flexcom (Flexible Serial Communication Unit)
> +
> +maintainers:
> +  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> +
> +description:
> +  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
> +  an I2C controller and an USART. Only one function can be used at a
> +  time and is chosen at boot time according to the device tree.
> +
> +properties:
> +  compatible:
> +    const: atmel,sama5d2-flexcom
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges:
> +    description:
> +      One range for the full I/O register region. (including USART,
> +      TWI and SPI registers).
> +    items:
> +      maxItems: 3
> +
> +  atmel,flexcom-mode:
> +    description: |
> +      Specifies the flexcom mode as follows:
> +      1: USART
> +      2: SPI
> +      3: I2C.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]
> +
> +patternProperties:
> +  "^serial@[0-9a-f]+$":
> +    description: See atmel-usart.txt for details of USART bindings.
> +    type: object
> +
> +  "^spi@[0-9a-f]+$":
> +    description: See ../spi/spi_atmel.txt for details of SPI bindings.
> +    type: object
> +
> +  "^i2c@[0-9a-f]+$":
> +    description: See ../i2c/i2c-at91.txt for details of I2C bindings.
> +    type: object
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +  - atmel,flexcom-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    flx0: flexcom@f8034000 {
> +          compatible = "atmel,sama5d2-flexcom";
> +          reg = <0xf8034000 0x200>;
> +          clocks = <&flx0_clk>;
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges = <0x0 0xf8034000 0x800>;
> +          atmel,flexcom-mode = <2>;
> +
> +          spi0: spi@400 {
> +                compatible = "atmel,at91rm9200-spi";
> +                reg = <0x400 0x200>;
> +                interrupts = <19 4 7>;

You can still use IRQ_TYPE_LEVEL_HIGH instead of 4 as it was in previous
atmel-flexcom.txt.

> +                pinctrl-names = "default";
> +                pinctrl-0 = <&pinctrl_flx0_default>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                clocks = <&flx0_clk>;
> +                clock-names = "spi_clk";
> +                atmel,fifo-size = <32>;
> +          };
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> deleted file mode 100644
> index 9d837535637b..000000000000
> --- a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> +++ /dev/null
> @@ -1,63 +0,0 @@
> -* Device tree bindings for Atmel Flexcom (Flexible Serial Communication Unit)
> -
> -The Atmel Flexcom is just a wrapper which embeds a SPI controller, an I2C
> -controller and an USART. Only one function can be used at a time and is chosen
> -at boot time according to the device tree.
> -
> -Required properties:
> -- compatible:		Should be "atmel,sama5d2-flexcom"
> -- reg:			Should be the offset/length value for Flexcom dedicated
> -			I/O registers (without USART, TWI or SPI registers).
> -- clocks:		Should be the Flexcom peripheral clock from PMC.
> -- #address-cells:	Should be <1>
> -- #size-cells:		Should be <1>
> -- ranges:		Should be one range for the full I/O register region
> -			(including USART, TWI and SPI registers).
> -- atmel,flexcom-mode:	Should be one of the following values:
> -			- <1> for USART
> -			- <2> for SPI
> -			- <3> for I2C
> -
> -Required child:
> -A single available child device of type matching the "atmel,flexcom-mode"
> -property.
> -
> -The phandle provided by the clocks property of the child is the same as one for
> -the Flexcom parent.
> -
> -For other properties, please refer to the documentations of the respective
> -device:
> -- ../serial/atmel-usart.txt
> -- ../spi/spi_atmel.txt
> -- ../i2c/i2c-at91.txt
> -
> -Example:
> -
> -flexcom@f8034000 {
> -	compatible = "atmel,sama5d2-flexcom";
> -	reg = <0xf8034000 0x200>;
> -	clocks = <&flx0_clk>;
> -	#address-cells = <1>;
> -	#size-cells = <1>;
> -	ranges = <0x0 0xf8034000 0x800>;
> -	atmel,flexcom-mode = <2>;
> -
> -	spi@400 {
> -		compatible = "atmel,at91rm9200-spi";
> -		reg = <0x400 0x200>;
> -		interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&pinctrl_flx0_default>;
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -		clocks = <&flx0_clk>;
> -		clock-names = "spi_clk";
> -		atmel,fifo-size = <32>;
> -
> -		flash@0 {
> -			compatible = "atmel,at25f512b";
> -			reg = <0>;
> -			spi-max-frequency = <20000000>;
> -		};
> -	};
> -};

Re: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Krzysztof Kozlowski]

On 07/06/2022 16:47, Kavyasree Kotagiri wrote:
> Convert the Atmel flexcom device tree bindings to json schema.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> ---
> v1 -> v2:
>  - Fix title.
> 
>  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
>  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
>  2 files changed, 97 insertions(+), 63 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> new file mode 100644
> index 000000000000..05cb6ebb4b2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel Flexcom (Flexible Serial Communication Unit)
> +
> +maintainers:
> +  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> +
> +description:
> +  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
> +  an I2C controller and an USART. Only one function can be used at a
> +  time and is chosen at boot time according to the device tree.
> +
> +properties:
> +  compatible:
> +    const: atmel,sama5d2-flexcom

Same comment applies as before... Your previous set was better here and
for some reason you decided to change it. This should be enum so you
avoid useless change next patch.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges:
> +    description:
> +      One range for the full I/O register region. (including USART,
> +      TWI and SPI registers).
> +    items:
> +      maxItems: 3
> +
> +  atmel,flexcom-mode:
> +    description: |
> +      Specifies the flexcom mode as follows:
> +      1: USART
> +      2: SPI
> +      3: I2C.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]
> +
> +patternProperties:
> +  "^serial@[0-9a-f]+$":
> +    description: See atmel-usart.txt for details of USART bindings.
> +    type: object
> +
> +  "^spi@[0-9a-f]+$":
> +    description: See ../spi/spi_atmel.txt for details of SPI bindings.
> +    type: object
> +
> +  "^i2c@[0-9a-f]+$":
> +    description: See ../i2c/i2c-at91.txt for details of I2C bindings.
> +    type: object
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +  - atmel,flexcom-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    flx0: flexcom@f8034000 {
> +          compatible = "atmel,sama5d2-flexcom";
> +          reg = <0xf8034000 0x200>;
> +          clocks = <&flx0_clk>;
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges = <0x0 0xf8034000 0x800>;
> +          atmel,flexcom-mode = <2>;
> +
> +          spi0: spi@400 {
> +                compatible = "atmel,at91rm9200-spi";
> +                reg = <0x400 0x200>;
> +                interrupts = <19 4 7>;

as pointed - looks like a IRQ flag

> +                pinctrl-names = "default";
> +                pinctrl-0 = <&pinctrl_flx0_default>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                clocks = <&flx0_clk>;
> +                clock-names = "spi_clk";
> +                atmel,fifo-size = <32>;
> +          };
> +    };



Best regards,
Krzysztof

RE: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Kavyasree.Kotagiri]

> > Convert the Atmel flexcom device tree bindings to json schema.
> >
> > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> > ---
> > v1 -> v2:
> >  - Fix title.
> >
> >  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
> >  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
> >  2 files changed, 97 insertions(+), 63 deletions(-)
> >  create mode 100644
> Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-
> flexcom.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> > new file mode 100644
> > index 000000000000..05cb6ebb4b2a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> > @@ -0,0 +1,97 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Atmel Flexcom (Flexible Serial Communication Unit)
> > +
> > +maintainers:
> > +  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> > +
> > +description:
> > +  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
> > +  an I2C controller and an USART. Only one function can be used at a
> > +  time and is chosen at boot time according to the device tree.
> > +
> > +properties:
> > +  compatible:
> > +    const: atmel,sama5d2-flexcom
> 
> Same comment applies as before... Your previous set was better here and
> for some reason you decided to change it. This should be enum so you
> avoid useless change next patch.
> 
Thanks for your comments.
Do you mean use "enum" instead of "const" in current patch itself and add new compatible in 2/3 patch?

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 1
> > +
> > +  ranges:
> > +    description:
> > +      One range for the full I/O register region. (including USART,
> > +      TWI and SPI registers).
> > +    items:
> > +      maxItems: 3
> > +
> > +  atmel,flexcom-mode:
> > +    description: |
> > +      Specifies the flexcom mode as follows:
> > +      1: USART
> > +      2: SPI
> > +      3: I2C.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 3]
> > +
> > +patternProperties:
> > +  "^serial@[0-9a-f]+$":
> > +    description: See atmel-usart.txt for details of USART bindings.
> > +    type: object
> > +
> > +  "^spi@[0-9a-f]+$":
> > +    description: See ../spi/spi_atmel.txt for details of SPI bindings.
> > +    type: object
> > +
> > +  "^i2c@[0-9a-f]+$":
> > +    description: See ../i2c/i2c-at91.txt for details of I2C bindings.
> > +    type: object
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> > +  - atmel,flexcom-mode
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    flx0: flexcom@f8034000 {
> > +          compatible = "atmel,sama5d2-flexcom";
> > +          reg = <0xf8034000 0x200>;
> > +          clocks = <&flx0_clk>;
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +          ranges = <0x0 0xf8034000 0x800>;
> > +          atmel,flexcom-mode = <2>;
> > +
> > +          spi0: spi@400 {
> > +                compatible = "atmel,at91rm9200-spi";
> > +                reg = <0x400 0x200>;
> > +                interrupts = <19 4 7>;
> 
> as pointed - looks like a IRQ flag
> 
Ok. I will change it.

> > +                pinctrl-names = "default";
> > +                pinctrl-0 = <&pinctrl_flx0_default>;
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +                clocks = <&flx0_clk>;
> > +                clock-names = "spi_clk";
> > +                atmel,fifo-size = <32>;
> > +          };
> > +    };
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Krzysztof Kozlowski]

On 08/06/2022 11:31, Kavyasree.Kotagiri@microchip.com wrote:
>>> Convert the Atmel flexcom device tree bindings to json schema.
>>>
>>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
>>> ---
>>> v1 -> v2:
>>>  - Fix title.
>>>
>>>  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
>>>  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
>>>  2 files changed, 97 insertions(+), 63 deletions(-)
>>>  create mode 100644
>> Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>>>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-
>> flexcom.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>> b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>>> new file mode 100644
>>> index 000000000000..05cb6ebb4b2a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>>> @@ -0,0 +1,97 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Atmel Flexcom (Flexible Serial Communication Unit)
>>> +
>>> +maintainers:
>>> +  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
>>> +
>>> +description:
>>> +  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
>>> +  an I2C controller and an USART. Only one function can be used at a
>>> +  time and is chosen at boot time according to the device tree.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: atmel,sama5d2-flexcom
>>
>> Same comment applies as before... Your previous set was better here and
>> for some reason you decided to change it. This should be enum so you
>> avoid useless change next patch.
>>
> Thanks for your comments.
> Do you mean use "enum" instead of "const" in current patch itself and add new compatible in 2/3 patch?

Yes. This is how you did it in previous patchsets.

Best regards,
Krzysztof

RE: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Kavyasree.Kotagiri]

> >>> Convert the Atmel flexcom device tree bindings to json schema.
> >>>
> >>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> >>> ---
> >>> v1 -> v2:
> >>>  - Fix title.
> >>>
> >>>  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
> >>>  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
> >>>  2 files changed, 97 insertions(+), 63 deletions(-)
> >>>  create mode 100644
> >> Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> >>>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-
> >> flexcom.txt
> >>>
> >>> diff --git
> a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> >> b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> >>> new file mode 100644
> >>> index 000000000000..05cb6ebb4b2a
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
> >>> @@ -0,0 +1,97 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Atmel Flexcom (Flexible Serial Communication Unit)
> >>> +
> >>> +maintainers:
> >>> +  - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> >>> +
> >>> +description:
> >>> +  The Atmel Flexcom is just a wrapper which embeds a SPI controller,
> >>> +  an I2C controller and an USART. Only one function can be used at a
> >>> +  time and is chosen at boot time according to the device tree.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: atmel,sama5d2-flexcom
> >>
> >> Same comment applies as before... Your previous set was better here
> and
> >> for some reason you decided to change it. This should be enum so you
> >> avoid useless change next patch.
> >>
> > Thanks for your comments.
> > Do you mean use "enum" instead of "const" in current patch itself and
> add new compatible in 2/3 patch?
> 
> Yes. This is how you did it in previous patchsets.
> 
I did so in v3 series, but below errors are reported on 1/3 patch:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml: properties:compatible:enum: 'atmel,sama5d2-flexcom' is not of type 'array'
        from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml: properties:compatible:enum: 'atmel,sama5d2-flexcom' is not of type 'array'
        from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml: properties:compatible:enum: 'atmel,sama5d2-flexcom' is not of type 'array'
        from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#

> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Krzysztof Kozlowski]

On 16/06/2022 02:20, Kavyasree.Kotagiri@microchip.com wrote:
>>
>> Yes. This is how you did it in previous patchsets.
>>
> I did so in v3 series, but below errors are reported on 1/3 patch:
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml: properties:compatible:enum: 'atmel,sama5d2-flexcom' is not of type 'array'

I don't remember it but it's a simple fix of syntax.
Documentation/devicetree/bindings/arm/arm,cci-400.yaml


Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: mfd: atmel,flexcom: Convert to json-schema

[Rob Herring]

On Tue, 07 Jun 2022 20:17:38 +0530, Kavyasree Kotagiri wrote:
> Convert the Atmel flexcom device tree bindings to json schema.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> ---
> v1 -> v2:
>  - Fix title.
> 
>  .../bindings/mfd/atmel,flexcom.yaml           | 97 +++++++++++++++++++
>  .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------
>  2 files changed, 97 insertions(+), 63 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/mfd/atmel,flexcom.example.dtb:0:0: /example-0/flexcom@f8034000/spi@400: failed to match any schema with compatible: ['atmel,at91rm9200-spi']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v2 2/3] dt-bindings: mfd: atmel,flexcom: Add new compatible string for lan966x

[Kavyasree Kotagiri]

LAN966x SoC flexcoms has two optional I/O lines. Namely, CS0 and CS1
in flexcom SPI mode. CTS and RTS in flexcom USART mode. These pins
can be mapped to lan966x FLEXCOM_SHARED[0-20] pins and usage depends on
functions being configured.

Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
---
v1 -> v2:
 - Use allOf:if:then for lan966x dt properties

 .../bindings/mfd/atmel,flexcom.yaml           | 39 ++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
index 05cb6ebb4b2a..2d357217fe22 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml
@@ -16,7 +16,9 @@ description:
 
 properties:
   compatible:
-    const: atmel,sama5d2-flexcom
+    enum:
+      - atmel,sama5d2-flexcom
+      - microchip,lan966x-flexcom
 
   reg:
     maxItems: 1
@@ -46,6 +48,27 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 3]
 
+  microchip,flx-shrd-pins:
+    description: Specify the Flexcom shared pins to be used for flexcom
+      chip-selects.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    items:
+      minimum: 0
+      maximum: 20
+
+  microchip,flx-cs:
+    description: Flexcom chip selects. Here, value of '0' represents "cts" line
+      of flexcom USART or "cs0" line of flexcom SPI and value of '1' represents
+      "rts" line of flexcom USART or "cs1" line of flexcom SPI.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    items:
+      minimum: 0
+      maximum: 1
+
 patternProperties:
   "^serial@[0-9a-f]+$":
     description: See atmel-usart.txt for details of USART bindings.
@@ -68,6 +91,18 @@ required:
   - ranges
   - atmel,flexcom-mode
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: microchip,lan966x-flexcom
+
+    then:
+      required:
+        - microchip,flx-shrd-pins
+        - microchip,flx-cs
+
 additionalProperties: false
 
 examples:
@@ -80,6 +115,8 @@ examples:
           #size-cells = <1>;
           ranges = <0x0 0xf8034000 0x800>;
           atmel,flexcom-mode = <2>;
+          microchip,flx-shrd-pins = <9>;
+          microchip,flx-cs = <0>;
 
           spi0: spi@400 {
                 compatible = "atmel,at91rm9200-spi";
-- 
2.17.1

[PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Kavyasree Kotagiri]

LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
For each chip select of each flexcom there is a configuration
register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
configuration register is 21 because there are 21 shared pins
on each of which the chip select can be mapped. Each bit of the
register represents a different FLEXCOM_SHARED pin.

Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
---
v1 -> v2:
 - use GENMASK for mask, macros for maximum allowed values.
 - use u32 values for flexcom chipselects instead of strings.
 - disable clock in case of errors.

 drivers/mfd/atmel-flexcom.c | 93 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 33caa4fba6af..ac700a85b46f 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c
@@ -28,15 +28,68 @@
 #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
 				 FLEX_MR_OPMODE_MASK)
 
+/* LAN966x flexcom shared register offsets */
+#define FLEX_SHRD_SS_MASK_0	0x0
+#define FLEX_SHRD_SS_MASK_1	0x4
+#define FLEX_SHRD_PIN_MAX	20
+#define FLEX_CS_MAX		1
+#define FLEX_SHRD_MASK		GENMASK(20, 0)
+
+struct atmel_flex_caps {
+	bool has_flx_cs;
+};
+
 struct atmel_flexcom {
 	void __iomem *base;
+	void __iomem *flexcom_shared_base;
 	u32 opmode;
 	struct clk *clk;
 };
 
+static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev)
+{
+	struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+	u32 flx_shrd_pins[2], flx_cs[2], val;
+	int err, i, count;
+
+	count = of_property_count_u32_elems(np, "microchip,flx-shrd-pins");
+	if (count <= 0 || count > 2) {
+		dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-pins",
+				count);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", flx_shrd_pins, count);
+	if (err)
+		return err;
+
+	err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, count);
+	if (err)
+		return err;
+
+	for (i = 0; i < count; i++) {
+		if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
+			return -EINVAL;
+
+		if (flx_cs[i] > FLEX_CS_MAX)
+			return -EINVAL;
+
+		val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
+
+		if (flx_cs[i] == 0)
+			writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_0);
+		else
+			writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_1);
+	}
+
+	return 0;
+}
+
 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct atmel_flex_caps *caps;
 	struct resource *res;
 	struct atmel_flexcom *ddata;
 	int err;
@@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
 	 */
 	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
 
+	caps = of_device_get_match_data(&pdev->dev);
+	if (!caps) {
+		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
+		clk_disable_unprepare(ddata->clk);
+		return -EINVAL;
+	}
+
+	if (caps->has_flx_cs) {
+		ddata->flexcom_shared_base = devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
+		if (IS_ERR(ddata->flexcom_shared_base)) {
+			clk_disable_unprepare(ddata->clk);
+			return dev_err_probe(&pdev->dev,
+					PTR_ERR(ddata->flexcom_shared_base),
+					"failed to get flexcom shared base address\n");
+		}
+
+		err = atmel_flexcom_lan966x_cs_config(pdev);
+		if (err) {
+			clk_disable_unprepare(ddata->clk);
+			return err;
+		}
+	}
+
 	clk_disable_unprepare(ddata->clk);
 
 	return devm_of_platform_populate(&pdev->dev);
 }
 
+static const struct atmel_flex_caps atmel_flexcom_caps = {};
+
+static const struct atmel_flex_caps lan966x_flexcom_caps = {
+	.has_flx_cs = true,
+};
+
 static const struct of_device_id atmel_flexcom_of_match[] = {
-	{ .compatible = "atmel,sama5d2-flexcom" },
+	{
+		.compatible = "atmel,sama5d2-flexcom",
+		.data = &atmel_flexcom_caps,
+	},
+
+	{
+		.compatible = "microchip,lan966x-flexcom",
+		.data = &lan966x_flexcom_caps,
+	},
+
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
-- 
2.17.1

Re: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Claudiu.Beznea]

On 07.06.2022 17:47, Kavyasree Kotagiri wrote:
> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> ---
> v1 -> v2:
>  - use GENMASK for mask, macros for maximum allowed values.
>  - use u32 values for flexcom chipselects instead of strings.
>  - disable clock in case of errors.
> 
>  drivers/mfd/atmel-flexcom.c | 93 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 33caa4fba6af..ac700a85b46f 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -28,15 +28,68 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +/* LAN966x flexcom shared register offsets */
> +#define FLEX_SHRD_SS_MASK_0	0x0
> +#define FLEX_SHRD_SS_MASK_1	0x4
> +#define FLEX_SHRD_PIN_MAX	20
> +#define FLEX_CS_MAX		1
> +#define FLEX_SHRD_MASK		GENMASK(20, 0)
> +
> +struct atmel_flex_caps {
> +	bool has_flx_cs;
> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
> +	void __iomem *flexcom_shared_base;
>  	u32 opmode;
>  	struct clk *clk;
>  };
>  
> +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev)
> +{
> +	struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
> +	u32 flx_shrd_pins[2], flx_cs[2], val;
> +	int err, i, count;
> +
> +	count = of_property_count_u32_elems(np, "microchip,flx-shrd-pins");
> +	if (count <= 0 || count > 2) {
> +		dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-pins",
> +				count);
> +		return -EINVAL;
> +	}
> +
> +	err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", flx_shrd_pins, count);
> +	if (err)
> +		return err;
> +
> +	err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, count);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < count; i++) {
> +		if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
> +			return -EINVAL;
> +
> +		if (flx_cs[i] > FLEX_CS_MAX)
> +			return -EINVAL;
> +
> +		val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
> +
> +		if (flx_cs[i] == 0)
> +			writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_0);
> +		else
> +			writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_1);

There is still an open question on this topic from previous version.

> +	}
> +
> +	return 0;
> +}
> +
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> +		clk_disable_unprepare(ddata->clk);

Could you keep a common path to disable the clock? A goto label something
like this:
		ret = -EINVAL;
		got clk_disable_unprepare;

> +		return -EINVAL;
> +	}
> +
> +	if (caps->has_flx_cs) {
> +		ddata->flexcom_shared_base = devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
> +		if (IS_ERR(ddata->flexcom_shared_base)) {
> +			clk_disable_unprepare(ddata->clk);
> +			return dev_err_probe(&pdev->dev,
> +					PTR_ERR(ddata->flexcom_shared_base),
> +					"failed to get flexcom shared base address\n");
			ret = dev_err_probe(...);
			goto clk_disable_unprepare;
> +		}
> +
> +		err = atmel_flexcom_lan966x_cs_config(pdev);
> +		if (err) {
> +			clk_disable_unprepare(ddata->clk);
> +			return err;
			goto clk_disable_unprepare;
> +		}
> +	}
> +

clk_unprepare:
>  	clk_disable_unprepare(ddata->clk);
	if (ret)
		return ret;
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> +
> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_cs = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,
> +	},
> +
> +	{
> +		.compatible = "microchip,lan966x-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},
> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);

RE: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Kavyasree.Kotagiri]

> > LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> > For each chip select of each flexcom there is a configuration
> > register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> > configuration register is 21 because there are 21 shared pins
> > on each of which the chip select can be mapped. Each bit of the
> > register represents a different FLEXCOM_SHARED pin.
> >
> > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> > ---
> > v1 -> v2:
> >  - use GENMASK for mask, macros for maximum allowed values.
> >  - use u32 values for flexcom chipselects instead of strings.
> >  - disable clock in case of errors.
> >
> >  drivers/mfd/atmel-flexcom.c | 93
> ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> > index 33caa4fba6af..ac700a85b46f 100644
> > --- a/drivers/mfd/atmel-flexcom.c
> > +++ b/drivers/mfd/atmel-flexcom.c
> > @@ -28,15 +28,68 @@
> >  #define FLEX_MR_OPMODE(opmode)	(((opmode) <<
> FLEX_MR_OPMODE_OFFSET) &	\
> >  				 FLEX_MR_OPMODE_MASK)
> >
> > +/* LAN966x flexcom shared register offsets */
> > +#define FLEX_SHRD_SS_MASK_0	0x0
> > +#define FLEX_SHRD_SS_MASK_1	0x4
> > +#define FLEX_SHRD_PIN_MAX	20
> > +#define FLEX_CS_MAX		1
> > +#define FLEX_SHRD_MASK		GENMASK(20, 0)
> > +
> > +struct atmel_flex_caps {
> > +	bool has_flx_cs;
> > +};
> > +
> >  struct atmel_flexcom {
> >  	void __iomem *base;
> > +	void __iomem *flexcom_shared_base;
> >  	u32 opmode;
> >  	struct clk *clk;
> >  };
> >
> > +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev)
> > +{
> > +	struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	u32 flx_shrd_pins[2], flx_cs[2], val;
> > +	int err, i, count;
> > +
> > +	count = of_property_count_u32_elems(np, "microchip,flx-shrd-
> pins");
> > +	if (count <= 0 || count > 2) {
> > +		dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-
> pins",
> > +				count);
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = of_property_read_u32_array(np, "microchip,flx-shrd-pins",
> flx_shrd_pins, count);
> > +	if (err)
> > +		return err;
> > +
> > +	err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs,
> count);
> > +	if (err)
> > +		return err;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
> > +			return -EINVAL;
> > +
> > +		if (flx_cs[i] > FLEX_CS_MAX)
> > +			return -EINVAL;
> > +
> > +		val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
> > +
> > +		if (flx_cs[i] == 0)
> > +			writel(val, ddata->flexcom_shared_base +
> FLEX_SHRD_SS_MASK_0);
> > +		else
> > +			writel(val, ddata->flexcom_shared_base +
> FLEX_SHRD_SS_MASK_1);
> 
> There is still an open question on this topic from previous version.
> 
https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/
As part of comments from Peter Rosin - Instead of using mux driver, This patch is introducing 
new dt-properties in atmel-flexom driver itlself to configure Flexcom shared registers. 
Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write to the
respective register. 
If you still have any questions, please comment.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int atmel_flexcom_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *np = pdev->dev.of_node;
> > +	const struct atmel_flex_caps *caps;
> >  	struct resource *res;
> >  	struct atmel_flexcom *ddata;
> >  	int err;
> > @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct
> platform_device *pdev)
> >  	 */
> >  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
> FLEX_MR);
> >
> > +	caps = of_device_get_match_data(&pdev->dev);
> > +	if (!caps) {
> > +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> > +		clk_disable_unprepare(ddata->clk);
> 
> Could you keep a common path to disable the clock? A goto label something
> like this:
> 		ret = -EINVAL;
> 		got clk_disable_unprepare;
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (caps->has_flx_cs) {
> > +		ddata->flexcom_shared_base =
> devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
> > +		if (IS_ERR(ddata->flexcom_shared_base)) {
> > +			clk_disable_unprepare(ddata->clk);
> > +			return dev_err_probe(&pdev->dev,
> > +					PTR_ERR(ddata-
> >flexcom_shared_base),
> > +					"failed to get flexcom shared base
> address\n");
> 			ret = dev_err_probe(...);
> 			goto clk_disable_unprepare;
> > +		}
> > +
> > +		err = atmel_flexcom_lan966x_cs_config(pdev);
> > +		if (err) {
> > +			clk_disable_unprepare(ddata->clk);
> > +			return err;
> 			goto clk_disable_unprepare;
> > +		}
> > +	}
> > +
> 
> clk_unprepare:
> >  	clk_disable_unprepare(ddata->clk);
> 	if (ret)
> 		return ret;
> >
> >  	return devm_of_platform_populate(&pdev->dev);
> >  }
> >
> > +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> > +
> > +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> > +	.has_flx_cs = true,
> > +};
> > +
> >  static const struct of_device_id atmel_flexcom_of_match[] = {
> > -	{ .compatible = "atmel,sama5d2-flexcom" },
> > +	{
> > +		.compatible = "atmel,sama5d2-flexcom",
> > +		.data = &atmel_flexcom_caps,
> > +	},
> > +
> > +	{
> > +		.compatible = "microchip,lan966x-flexcom",
> > +		.data = &lan966x_flexcom_caps,
> > +	},
> > +
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);

Re: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Claudiu.Beznea]

On 08.06.2022 11:20, Kavyasree Kotagiri - I30978 wrote:
>>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
>>> For each chip select of each flexcom there is a configuration
>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
>>> configuration register is 21 because there are 21 shared pins
>>> on each of which the chip select can be mapped. Each bit of the
>>> register represents a different FLEXCOM_SHARED pin.
>>>
>>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
>>> ---
>>> v1 -> v2:
>>>  - use GENMASK for mask, macros for maximum allowed values.
>>>  - use u32 values for flexcom chipselects instead of strings.
>>>  - disable clock in case of errors.
>>>
>>>  drivers/mfd/atmel-flexcom.c | 93
>> ++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
>>> index 33caa4fba6af..ac700a85b46f 100644
>>> --- a/drivers/mfd/atmel-flexcom.c
>>> +++ b/drivers/mfd/atmel-flexcom.c
>>> @@ -28,15 +28,68 @@
>>>  #define FLEX_MR_OPMODE(opmode)	(((opmode) <<
>> FLEX_MR_OPMODE_OFFSET) &	\
>>>  				 FLEX_MR_OPMODE_MASK)
>>>
>>> +/* LAN966x flexcom shared register offsets */
>>> +#define FLEX_SHRD_SS_MASK_0	0x0
>>> +#define FLEX_SHRD_SS_MASK_1	0x4
>>> +#define FLEX_SHRD_PIN_MAX	20
>>> +#define FLEX_CS_MAX		1
>>> +#define FLEX_SHRD_MASK		GENMASK(20, 0)
>>> +
>>> +struct atmel_flex_caps {
>>> +	bool has_flx_cs;
>>> +};
>>> +
>>>  struct atmel_flexcom {
>>>  	void __iomem *base;
>>> +	void __iomem *flexcom_shared_base;
>>>  	u32 opmode;
>>>  	struct clk *clk;
>>>  };
>>>
>>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev)
>>> +{
>>> +	struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
>>> +	struct device_node *np = pdev->dev.of_node;
>>> +	u32 flx_shrd_pins[2], flx_cs[2], val;
>>> +	int err, i, count;
>>> +
>>> +	count = of_property_count_u32_elems(np, "microchip,flx-shrd-
>> pins");
>>> +	if (count <= 0 || count > 2) {
>>> +		dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-
>> pins",
>>> +				count);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	err = of_property_read_u32_array(np, "microchip,flx-shrd-pins",
>> flx_shrd_pins, count);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs,
>> count);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	for (i = 0; i < count; i++) {
>>> +		if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
>>> +			return -EINVAL;
>>> +
>>> +		if (flx_cs[i] > FLEX_CS_MAX)
>>> +			return -EINVAL;
>>> +
>>> +		val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
>>> +
>>> +		if (flx_cs[i] == 0)
>>> +			writel(val, ddata->flexcom_shared_base +
>> FLEX_SHRD_SS_MASK_0);
>>> +		else
>>> +			writel(val, ddata->flexcom_shared_base +
>> FLEX_SHRD_SS_MASK_1);
>>
>> There is still an open question on this topic from previous version.
>>
> https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/

"previous version" meant for me this the one at [1]... Another point that
the versioning of this series is bad.

The question was the following:

"I may miss something but I don't see here the approach you introduced in [1]:

+			err = mux_control_select(flx_mux, args.args[0]);
+			if (!err) {
+				mux_control_deselect(flx_mux);
"

As I had in mind that you said you need mux_control_deselect() because your
serial remain blocked otherwise (but I don't find that in the comments of
[1]). And I don't see something similar to mux_control_deselect() being
called in this patch.

[1]
https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f-cb73f60154ff@microchip.com/

> As part of comments from Peter Rosin - Instead of using mux driver, This patch is introducing 
> new dt-properties in atmel-flexom driver itlself to configure Flexcom shared registers. 
> Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write to the
> respective register. 
> If you still have any questions, please comment.
> 
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int atmel_flexcom_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device_node *np = pdev->dev.of_node;
>>> +	const struct atmel_flex_caps *caps;
>>>  	struct resource *res;
>>>  	struct atmel_flexcom *ddata;
>>>  	int err;
>>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct
>> platform_device *pdev)
>>>  	 */
>>>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
>> FLEX_MR);
>>>
>>> +	caps = of_device_get_match_data(&pdev->dev);
>>> +	if (!caps) {
>>> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
>>> +		clk_disable_unprepare(ddata->clk);
>>
>> Could you keep a common path to disable the clock? A goto label something
>> like this:
>> 		ret = -EINVAL;
>> 		got clk_disable_unprepare;
>>
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (caps->has_flx_cs) {
>>> +		ddata->flexcom_shared_base =
>> devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
>>> +		if (IS_ERR(ddata->flexcom_shared_base)) {
>>> +			clk_disable_unprepare(ddata->clk);
>>> +			return dev_err_probe(&pdev->dev,
>>> +					PTR_ERR(ddata-
>>> flexcom_shared_base),
>>> +					"failed to get flexcom shared base
>> address\n");
>> 			ret = dev_err_probe(...);
>> 			goto clk_disable_unprepare;
>>> +		}
>>> +
>>> +		err = atmel_flexcom_lan966x_cs_config(pdev);
>>> +		if (err) {
>>> +			clk_disable_unprepare(ddata->clk);
>>> +			return err;
>> 			goto clk_disable_unprepare;
>>> +		}
>>> +	}
>>> +
>>
>> clk_unprepare:
>>>  	clk_disable_unprepare(ddata->clk);
>> 	if (ret)
>> 		return ret;
>>>
>>>  	return devm_of_platform_populate(&pdev->dev);
>>>  }
>>>
>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
>>> +
>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
>>> +	.has_flx_cs = true,
>>> +};
>>> +
>>>  static const struct of_device_id atmel_flexcom_of_match[] = {
>>> -	{ .compatible = "atmel,sama5d2-flexcom" },
>>> +	{
>>> +		.compatible = "atmel,sama5d2-flexcom",
>>> +		.data = &atmel_flexcom_caps,
>>> +	},
>>> +
>>> +	{
>>> +		.compatible = "microchip,lan966x-flexcom",
>>> +		.data = &lan966x_flexcom_caps,
>>> +	},
>>> +
>>>  	{ /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> 

RE: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Kavyasree.Kotagiri]

> >>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> >>> For each chip select of each flexcom there is a configuration
> >>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> >>> configuration register is 21 because there are 21 shared pins
> >>> on each of which the chip select can be mapped. Each bit of the
> >>> register represents a different FLEXCOM_SHARED pin.
> >>>
> >>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> >>> ---
> >>> v1 -> v2:
> >>>  - use GENMASK for mask, macros for maximum allowed values.
> >>>  - use u32 values for flexcom chipselects instead of strings.
> >>>  - disable clock in case of errors.
> >>>
> >>>  drivers/mfd/atmel-flexcom.c | 93
> >> ++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 92 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> >>> index 33caa4fba6af..ac700a85b46f 100644
> >>> --- a/drivers/mfd/atmel-flexcom.c
> >>> +++ b/drivers/mfd/atmel-flexcom.c
> >>> @@ -28,15 +28,68 @@
> >>>  #define FLEX_MR_OPMODE(opmode)	(((opmode) <<
> >> FLEX_MR_OPMODE_OFFSET) &	\
> >>>  				 FLEX_MR_OPMODE_MASK)
> >>>
> >>> +/* LAN966x flexcom shared register offsets */
> >>> +#define FLEX_SHRD_SS_MASK_0	0x0
> >>> +#define FLEX_SHRD_SS_MASK_1	0x4
> >>> +#define FLEX_SHRD_PIN_MAX	20
> >>> +#define FLEX_CS_MAX		1
> >>> +#define FLEX_SHRD_MASK		GENMASK(20, 0)
> >>> +
> >>> +struct atmel_flex_caps {
> >>> +	bool has_flx_cs;
> >>> +};
> >>> +
> >>>  struct atmel_flexcom {
> >>>  	void __iomem *base;
> >>> +	void __iomem *flexcom_shared_base;
> >>>  	u32 opmode;
> >>>  	struct clk *clk;
> >>>  };
> >>>
> >>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device
> *pdev)
> >>> +{
> >>> +	struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
> >>> +	struct device_node *np = pdev->dev.of_node;
> >>> +	u32 flx_shrd_pins[2], flx_cs[2], val;
> >>> +	int err, i, count;
> >>> +
> >>> +	count = of_property_count_u32_elems(np, "microchip,flx-shrd-
> >> pins");
> >>> +	if (count <= 0 || count > 2) {
> >>> +		dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-
> >> pins",
> >>> +				count);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	err = of_property_read_u32_array(np, "microchip,flx-shrd-pins",
> >> flx_shrd_pins, count);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs,
> >> count);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	for (i = 0; i < count; i++) {
> >>> +		if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
> >>> +			return -EINVAL;
> >>> +
> >>> +		if (flx_cs[i] > FLEX_CS_MAX)
> >>> +			return -EINVAL;
> >>> +
> >>> +		val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
> >>> +
> >>> +		if (flx_cs[i] == 0)
> >>> +			writel(val, ddata->flexcom_shared_base +
> >> FLEX_SHRD_SS_MASK_0);
> >>> +		else
> >>> +			writel(val, ddata->flexcom_shared_base +
> >> FLEX_SHRD_SS_MASK_1);
> >>
> >> There is still an open question on this topic from previous version.
> >>
> > https://lore.kernel.org/linux-arm-
> kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.n
> amprd11.prod.outlook.com/
> 
> "previous version" meant for me this the one at [1]... Another point that
> the versioning of this series is bad.
> 
> The question was the following:
> 
> "I may miss something but I don't see here the approach you introduced in
> [1]:
> 
> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);
> "
> 
> As I had in mind that you said you need mux_control_deselect() because
> your
> serial remain blocked otherwise (but I don't find that in the comments of
> [1]). And I don't see something similar to mux_control_deselect() being
> called in this patch.
> 
> [1]
> https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f-
> cb73f60154ff@microchip.com/
> 
> > As part of comments from Peter Rosin - Instead of using mux driver, This
> patch is introducing
> > new dt-properties in atmel-flexom driver itlself to configure Flexcom
> shared registers.
> > Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write
> to the
> > respective register.
> > If you still have any questions, please comment.
> >
https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/
To avoid confusion, I stopped continuing with above patch versioning(mux driver approach).
I started new patch series in which I am configuring FLEXCOM_SHARED[0-4]:SS_MASK[0-1]
registers in atmel-flexcom.c driver using new DT-properties, mux driver approach is no more followed
as suggested by Peter Rosin:
"
> If you are content with just programming a fixed set of values to
> a couple of registers depending on how the board is wired, some
> extra DT property on some node related to the flexcom seems like
> a better fit than a mux driver.
Based on your inputs, I planned to send a new patch with new DT properties
introduced in atmel-flexcom.c driver rather than mux driver.

Thanks,
Kavya
"

Thanks,
Kavya

> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int atmel_flexcom_probe(struct platform_device *pdev)
> >>>  {
> >>>  	struct device_node *np = pdev->dev.of_node;
> >>> +	const struct atmel_flex_caps *caps;
> >>>  	struct resource *res;
> >>>  	struct atmel_flexcom *ddata;
> >>>  	int err;
> >>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct
> >> platform_device *pdev)
> >>>  	 */
> >>>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
> >> FLEX_MR);
> >>>
> >>> +	caps = of_device_get_match_data(&pdev->dev);
> >>> +	if (!caps) {
> >>> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> >>> +		clk_disable_unprepare(ddata->clk);
> >>
> >> Could you keep a common path to disable the clock? A goto label
> something
> >> like this:
> >> 		ret = -EINVAL;
> >> 		got clk_disable_unprepare;
> >>
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	if (caps->has_flx_cs) {
> >>> +		ddata->flexcom_shared_base =
> >> devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
> >>> +		if (IS_ERR(ddata->flexcom_shared_base)) {
> >>> +			clk_disable_unprepare(ddata->clk);
> >>> +			return dev_err_probe(&pdev->dev,
> >>> +					PTR_ERR(ddata-
> >>> flexcom_shared_base),
> >>> +					"failed to get flexcom shared base
> >> address\n");
> >> 			ret = dev_err_probe(...);
> >> 			goto clk_disable_unprepare;
> >>> +		}
> >>> +
> >>> +		err = atmel_flexcom_lan966x_cs_config(pdev);
> >>> +		if (err) {
> >>> +			clk_disable_unprepare(ddata->clk);
> >>> +			return err;
> >> 			goto clk_disable_unprepare;
> >>> +		}
> >>> +	}
> >>> +
> >>
> >> clk_unprepare:
> >>>  	clk_disable_unprepare(ddata->clk);
> >> 	if (ret)
> >> 		return ret;
> >>>
> >>>  	return devm_of_platform_populate(&pdev->dev);
> >>>  }
> >>>
> >>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> >>> +
> >>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> >>> +	.has_flx_cs = true,
> >>> +};
> >>> +
> >>>  static const struct of_device_id atmel_flexcom_of_match[] = {
> >>> -	{ .compatible = "atmel,sama5d2-flexcom" },
> >>> +	{
> >>> +		.compatible = "atmel,sama5d2-flexcom",
> >>> +		.data = &atmel_flexcom_caps,
> >>> +	},
> >>> +
> >>> +	{
> >>> +		.compatible = "microchip,lan966x-flexcom",
> >>> +		.data = &lan966x_flexcom_caps,
> >>> +	},
> >>> +
> >>>  	{ /* sentinel */ }
> >>>  };
> >>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> >

Re: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Kavyasree.Kotagiri]

>>>>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
>>>>> For each chip select of each flexcom there is a configuration
>>>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
>>>>> configuration register is 21 because there are 21 shared pins
>>>>> on each of which the chip select can be mapped. Each bit of the
>>>>> register represents a different FLEXCOM_SHARED pin.
>>>>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
>>>>> ---
>>>>> v1 -> v2:
>>>>> - use GENMASK for mask, macros for maximum allowed values.
>>>>> - use u32 values for flexcom chipselects instead of strings.
>>>>> - disable clock in case of errors.
>>>>> drivers/mfd/atmel-flexcom.c | 93
>>>> ++++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 92 insertions(+), 1 deletion(-)
>>>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
>>>>> index 33caa4fba6af..ac700a85b46f 100644
>>>>> --- a/drivers/mfd/atmel-flexcom.c
>>>>> +++ b/drivers/mfd/atmel-flexcom.c
>>>>> @@ -28,15 +28,68 @@
>>>>> #define FLEX_MR_OPMODE(opmode)    (((opmode) <<
>>>> FLEX_MR_OPMODE_OFFSET) &    \
>>>>>                FLEX_MR_OPMODE_MASK)
>>>>> +/* LAN966x flexcom shared register offsets */
>>>>> +#define FLEX_SHRD_SS_MASK_0    0x0
>>>>> +#define FLEX_SHRD_SS_MASK_1    0x4
>>>>> +#define FLEX_SHRD_PIN_MAX    20
>>>>> +#define FLEX_CS_MAX        1
>>>>> +#define FLEX_SHRD_MASK        GENMASK(20, 0)
>>>>> +
>>>>> +struct atmel_flex_caps {
>>>>> +    bool has_flx_cs;
>>>>> +};
>>>>> +
>>>>> struct atmel_flexcom {
>>>>>   void __iomem *base;
>>>>> +    void __iomem *flexcom_shared_base;
>>>>>   u32 opmode;
>>>>>   struct clk *clk;
>>>>> };
>>>>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device
>> *pdev)
>>>>> +{
>>>>> +    struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
>>>>> +    struct device_node *np = pdev->dev.of_node;
>>>>> +    u32 flx_shrd_pins[2], flx_cs[2], val;
>>>>> +    int err, i, count;
>>>>> +
>>>>> +    count = of_property_count_u32_elems(np, "microchip,flx-shrd-
>>>> pins");
>>>>> +    if (count <= 0 || count > 2) {
>>>>> +        dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-
>>>> pins",
>>>>> +                count);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    err = of_property_read_u32_array(np, "microchip,flx-shrd-pins",
>>>> flx_shrd_pins, count);
>>>>> +    if (err)
>>>>> +        return err;
>>>>> +
>>>>> +    err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs,
>>>> count);
>>>>> +    if (err)
>>>>> +        return err;
>>>>> +
>>>>> +    for (i = 0; i < count; i++) {
>>>>> +        if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        if (flx_cs[i] > FLEX_CS_MAX)
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
>>>>> +
>>>>> +        if (flx_cs[i] == 0)
>>>>> +            writel(val, ddata->flexcom_shared_base +
>>>> FLEX_SHRD_SS_MASK_0);
>>>>> +        else
>>>>> +            writel(val, ddata->flexcom_shared_base +
>>>> FLEX_SHRD_SS_MASK_1);
>>>> There is still an open question on this topic from previous version.
>>> https://lore.kernel.org/linux-arm-
>> kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.n
>> amprd11.prod.outlook.com/
>> "previous version" meant for me this the one at [1]... Another point that
>> the versioning of this series is bad.
>> The question was the following:
>> "I may miss something but I don't see here the approach you introduced in
>> [1]:
>> +            err = mux_control_select(flx_mux, args.args[0]);
>> +            if (!err) {
>> +                mux_control_deselect(flx_mux);
>> "
>> As I had in mind that you said you need mux_control_deselect() because
>> your
>> serial remain blocked otherwise (but I don't find that in the comments of
>> [1]). And I don't see something similar to mux_control_deselect() being
>> called in this patch.
>> [1]
>> https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f-
>> cb73f60154ff@microchip.com/
>>> As part of comments from Peter Rosin - Instead of using mux driver, This
>> patch is introducing
>>> new dt-properties in atmel-flexom driver itlself to configure Flexcom
>> shared registers.
>>> Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write
>> to the
>>> respective register.
>>> If you still have any questions, please comment.
> https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/
> To avoid confusion, I stopped continuing with above patch versioning(mux driver approach).
> I started new patch series in which I am configuring FLEXCOM_SHARED[0-4]:SS_MASK[0-1]
> registers in atmel-flexcom.c driver using new DT-properties, mux driver approach is no more followed
> as suggested by Peter Rosin:
> "
>> If you are content with just programming a fixed set of values to
>> a couple of registers depending on how the board is wired, some
>> extra DT property on some node related to the flexcom seems like
>> a better fit than a mux driver.
> Based on your inputs, I planned to send a new patch with new DT properties
> introduced in atmel-flexcom.c driver rather than mux driver.
> 
> Thanks,
> Kavya
> "
> 
> Thanks,
> Kavya

Hi Claudiu,

Please let me know if you still have any comments. If not, I will send my v3 with clk_disable_unprepare moved to goto and some minor fixes(irq flags) in dt-bindings.

>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> static int atmel_flexcom_probe(struct platform_device *pdev)
>>>>> {
>>>>>   struct device_node *np = pdev->dev.of_node;
>>>>> +    const struct atmel_flex_caps *caps;
>>>>>   struct resource *res;
>>>>>   struct atmel_flexcom *ddata;
>>>>>   int err;
>>>>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct
>>>> platform_device *pdev)
>>>>>    */
>>>>>   writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
>>>> FLEX_MR);
>>>>> +    caps = of_device_get_match_data(&pdev->dev);
>>>>> +    if (!caps) {
>>>>> +        dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
>>>>> +        clk_disable_unprepare(ddata->clk);
>>>> Could you keep a common path to disable the clock? A goto label
>> something
>>>> like this:
>>>>       ret = -EINVAL;
>>>>       got clk_disable_unprepare;
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if (caps->has_flx_cs) {
>>>>> +        ddata->flexcom_shared_base =
>>>> devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
>>>>> +        if (IS_ERR(ddata->flexcom_shared_base)) {
>>>>> +            clk_disable_unprepare(ddata->clk);
>>>>> +            return dev_err_probe(&pdev->dev,
>>>>> +                    PTR_ERR(ddata-
>>>>> flexcom_shared_base),
>>>>> +                    "failed to get flexcom shared base
>>>> address\n");
>>>>           ret = dev_err_probe(...);
>>>>           goto clk_disable_unprepare;
>>>>> +        }
>>>>> +
>>>>> +        err = atmel_flexcom_lan966x_cs_config(pdev);
>>>>> +        if (err) {
>>>>> +            clk_disable_unprepare(ddata->clk);
>>>>> +            return err;
>>>>           goto clk_disable_unprepare;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>> clk_unprepare:
>>>>>   clk_disable_unprepare(ddata->clk);
>>>>   if (ret)
>>>>       return ret;
>>>>>   return devm_of_platform_populate(&pdev->dev);
>>>>> }
>>>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
>>>>> +
>>>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
>>>>> +    .has_flx_cs = true,
>>>>> +};
>>>>> +
>>>>> static const struct of_device_id atmel_flexcom_of_match[] = {
>>>>> -    { .compatible = "atmel,sama5d2-flexcom" },
>>>>> +    {
>>>>> +        .compatible = "atmel,sama5d2-flexcom",
>>>>> +        .data = &atmel_flexcom_caps,
>>>>> +    },
>>>>> +
>>>>> +    {
>>>>> +        .compatible = "microchip,lan966x-flexcom",
>>>>> +        .data = &lan966x_flexcom_caps,
>>>>> +    },
>>>>> +
>>>>>   { /* sentinel */ }
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);

Re: [PATCH v2 3/3] mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration

[Claudiu.Beznea]

On 09.06.2022 16:34, Kavyasree Kotagiri - I30978 wrote:
> 
>>>>>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
>>>>>> For each chip select of each flexcom there is a configuration
>>>>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
>>>>>> configuration register is 21 because there are 21 shared pins
>>>>>> on each of which the chip select can be mapped. Each bit of the
>>>>>> register represents a different FLEXCOM_SHARED pin.
>>>>>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
>>>>>> ---
>>>>>> v1 -> v2:
>>>>>> - use GENMASK for mask, macros for maximum allowed values.
>>>>>> - use u32 values for flexcom chipselects instead of strings.
>>>>>> - disable clock in case of errors.
>>>>>> drivers/mfd/atmel-flexcom.c | 93
>>>>> ++++++++++++++++++++++++++++++++++++-
>>>>>> 1 file changed, 92 insertions(+), 1 deletion(-)
>>>>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
>>>>>> index 33caa4fba6af..ac700a85b46f 100644
>>>>>> --- a/drivers/mfd/atmel-flexcom.c
>>>>>> +++ b/drivers/mfd/atmel-flexcom.c
>>>>>> @@ -28,15 +28,68 @@
>>>>>> #define FLEX_MR_OPMODE(opmode)    (((opmode) <<
>>>>> FLEX_MR_OPMODE_OFFSET) &    \
>>>>>>                FLEX_MR_OPMODE_MASK)
>>>>>> +/* LAN966x flexcom shared register offsets */
>>>>>> +#define FLEX_SHRD_SS_MASK_0    0x0
>>>>>> +#define FLEX_SHRD_SS_MASK_1    0x4
>>>>>> +#define FLEX_SHRD_PIN_MAX    20
>>>>>> +#define FLEX_CS_MAX        1
>>>>>> +#define FLEX_SHRD_MASK        GENMASK(20, 0)
>>>>>> +
>>>>>> +struct atmel_flex_caps {
>>>>>> +    bool has_flx_cs;
>>>>>> +};
>>>>>> +
>>>>>> struct atmel_flexcom {
>>>>>>   void __iomem *base;
>>>>>> +    void __iomem *flexcom_shared_base;
>>>>>>   u32 opmode;
>>>>>>   struct clk *clk;
>>>>>> };
>>>>>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device
>>> *pdev)
>>>>>> +{
>>>>>> +    struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev);
>>>>>> +    struct device_node *np = pdev->dev.of_node;
>>>>>> +    u32 flx_shrd_pins[2], flx_cs[2], val;
>>>>>> +    int err, i, count;
>>>>>> +
>>>>>> +    count = of_property_count_u32_elems(np, "microchip,flx-shrd-
>>>>> pins");
>>>>>> +    if (count <= 0 || count > 2) {
>>>>>> +        dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-
>>>>> pins",
>>>>>> +                count);
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    err = of_property_read_u32_array(np, "microchip,flx-shrd-pins",
>>>>> flx_shrd_pins, count);
>>>>>> +    if (err)
>>>>>> +        return err;
>>>>>> +
>>>>>> +    err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs,
>>>>> count);
>>>>>> +    if (err)
>>>>>> +        return err;
>>>>>> +
>>>>>> +    for (i = 0; i < count; i++) {
>>>>>> +        if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX)
>>>>>> +            return -EINVAL;
>>>>>> +
>>>>>> +        if (flx_cs[i] > FLEX_CS_MAX)
>>>>>> +            return -EINVAL;
>>>>>> +
>>>>>> +        val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK;
>>>>>> +
>>>>>> +        if (flx_cs[i] == 0)
>>>>>> +            writel(val, ddata->flexcom_shared_base +
>>>>> FLEX_SHRD_SS_MASK_0);
>>>>>> +        else
>>>>>> +            writel(val, ddata->flexcom_shared_base +
>>>>> FLEX_SHRD_SS_MASK_1);
>>>>> There is still an open question on this topic from previous version.
>>>> https://lore.kernel.org/linux-arm-
>>> kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.n
>>> amprd11.prod.outlook.com/
>>> "previous version" meant for me this the one at [1]... Another point that
>>> the versioning of this series is bad.
>>> The question was the following:
>>> "I may miss something but I don't see here the approach you introduced in
>>> [1]:
>>> +            err = mux_control_select(flx_mux, args.args[0]);
>>> +            if (!err) {
>>> +                mux_control_deselect(flx_mux);
>>> "
>>> As I had in mind that you said you need mux_control_deselect() because
>>> your
>>> serial remain blocked otherwise (but I don't find that in the comments of
>>> [1]). And I don't see something similar to mux_control_deselect() being
>>> called in this patch.
>>> [1]
>>> https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f-
>>> cb73f60154ff@microchip.com/
>>>> As part of comments from Peter Rosin - Instead of using mux driver, This
>>> patch is introducing
>>>> new dt-properties in atmel-flexom driver itlself to configure Flexcom
>>> shared registers.
>>>> Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write
>>> to the
>>>> respective register.
>>>> If you still have any questions, please comment.
>> https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/
>> To avoid confusion, I stopped continuing with above patch versioning(mux driver approach).
>> I started new patch series in which I am configuring FLEXCOM_SHARED[0-4]:SS_MASK[0-1]
>> registers in atmel-flexcom.c driver using new DT-properties, mux driver approach is no more followed
>> as suggested by Peter Rosin:
>> "
>>> If you are content with just programming a fixed set of values to
>>> a couple of registers depending on how the board is wired, some
>>> extra DT property on some node related to the flexcom seems like
>>> a better fit than a mux driver.
>> Based on your inputs, I planned to send a new patch with new DT properties
>> introduced in atmel-flexcom.c driver rather than mux driver.
>>
>> Thanks,
>> Kavya
>> "
>>
>> Thanks,
>> Kavya
> 
> Hi Claudiu,
> 
> Please let me know if you still have any comments. If not, I will send my v3 with clk_disable_unprepare moved to goto and some minor fixes(irq flags) in dt-bindings.

I got it now after the talk we had on internal chat. Please go with v3.

Thank you,
Claudiu Beznea

> 
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> static int atmel_flexcom_probe(struct platform_device *pdev)
>>>>>> {
>>>>>>   struct device_node *np = pdev->dev.of_node;
>>>>>> +    const struct atmel_flex_caps *caps;
>>>>>>   struct resource *res;
>>>>>>   struct atmel_flexcom *ddata;
>>>>>>   int err;
>>>>>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct
>>>>> platform_device *pdev)
>>>>>>    */
>>>>>>   writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
>>>>> FLEX_MR);
>>>>>> +    caps = of_device_get_match_data(&pdev->dev);
>>>>>> +    if (!caps) {
>>>>>> +        dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
>>>>>> +        clk_disable_unprepare(ddata->clk);
>>>>> Could you keep a common path to disable the clock? A goto label
>>> something
>>>>> like this:
>>>>>       ret = -EINVAL;
>>>>>       got clk_disable_unprepare;
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (caps->has_flx_cs) {
>>>>>> +        ddata->flexcom_shared_base =
>>>>> devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
>>>>>> +        if (IS_ERR(ddata->flexcom_shared_base)) {
>>>>>> +            clk_disable_unprepare(ddata->clk);
>>>>>> +            return dev_err_probe(&pdev->dev,
>>>>>> +                    PTR_ERR(ddata-
>>>>>> flexcom_shared_base),
>>>>>> +                    "failed to get flexcom shared base
>>>>> address\n");
>>>>>           ret = dev_err_probe(...);
>>>>>           goto clk_disable_unprepare;
>>>>>> +        }
>>>>>> +
>>>>>> +        err = atmel_flexcom_lan966x_cs_config(pdev);
>>>>>> +        if (err) {
>>>>>> +            clk_disable_unprepare(ddata->clk);
>>>>>> +            return err;
>>>>>           goto clk_disable_unprepare;
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>> clk_unprepare:
>>>>>>   clk_disable_unprepare(ddata->clk);
>>>>>   if (ret)
>>>>>       return ret;
>>>>>>   return devm_of_platform_populate(&pdev->dev);
>>>>>> }
>>>>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
>>>>>> +
>>>>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
>>>>>> +    .has_flx_cs = true,
>>>>>> +};
>>>>>> +
>>>>>> static const struct of_device_id atmel_flexcom_of_match[] = {
>>>>>> -    { .compatible = "atmel,sama5d2-flexcom" },
>>>>>> +    {
>>>>>> +        .compatible = "atmel,sama5d2-flexcom",
>>>>>> +        .data = &atmel_flexcom_caps,
>>>>>> +    },
>>>>>> +
>>>>>> +    {
>>>>>> +        .compatible = "microchip,lan966x-flexcom",
>>>>>> +        .data = &lan966x_flexcom_caps,
>>>>>> +    },
>>>>>> +
>>>>>>   { /* sentinel */ }
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);