[PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Ivan Vecera]

Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
provides DPLL and PTP functionality. This series bring first part
that adds the common MFD driver that provides an access to the bus
that can be either I2C or SPI.

The next series will bring the DPLL driver that will covers DPLL
functionality. And another ones will bring PTP driver and flashing
capability via devlink.

Testing was done by myself and by Prathosh Satish on Microchip EDS2
development board with ZL30732 DPLL chip connected over I2C bus.

Patch breakdown
===============
Patch 1 - Common DT schema for DPLL device and pin
Patch 3 - Basic support for I2C, SPI and regmap
Patch 4 - Devlink registration
Patches 5-6 - Helpers for accessing device registers
Patches 7-8 - Component versions reporting via devlink dev info
Patches 9-10 - Helpers for accessing register mailboxes
Patch 11 - Clock ID generation for DPLL driver
Patch 12 - Export strnchrnul function for modules
           (used by next patch)
Patch 13 - Support for MFG config initialization file
Patch 14 - Fetch invariant register values used by DPLL and later by
           PTP driver

Ivan Vecera (14):
  dt-bindings: dpll: Add device tree bindings for DPLL device and pin
  dt-bindings: dpll: Add support for Microchip Azurite chip family
  mfd: Add Microchip ZL3073x support
  mfd: zl3073x: Register itself as devlink device
  mfd: zl3073x: Add register access helpers
  mfd: zl3073x: Add macros for device registers access
  mfd: zl3073x: Add components versions register defs
  mfd: zl3073x: Implement devlink device info
  mfd: zl3073x: Add macro to wait for register value bits to be cleared
  mfd: zl3073x: Add functions to work with register mailboxes
  mfd: zl3073x: Add clock_id field
  lib: Allow modules to use strnchrnul
  mfd: zl3073x: Load mfg file into HW if it is present
  mfd: zl3073x: Fetch invariants during probe

 .../devicetree/bindings/dpll/dpll-device.yaml |  76 ++
 .../devicetree/bindings/dpll/dpll-pin.yaml    |  44 +
 .../bindings/dpll/microchip,zl3073x-i2c.yaml  |  74 ++
 .../bindings/dpll/microchip,zl3073x-spi.yaml  |  77 ++
 MAINTAINERS                                   |  11 +
 drivers/mfd/Kconfig                           |  32 +
 drivers/mfd/Makefile                          |   5 +
 drivers/mfd/zl3073x-core.c                    | 883 ++++++++++++++++++
 drivers/mfd/zl3073x-i2c.c                     |  59 ++
 drivers/mfd/zl3073x-spi.c                     |  59 ++
 drivers/mfd/zl3073x.h                         |  14 +
 include/linux/mfd/zl3073x.h                   | 363 +++++++
 lib/string.c                                  |   1 +
 13 files changed, 1698 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
 create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml
 create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
 create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
 create mode 100644 drivers/mfd/zl3073x-core.c
 create mode 100644 drivers/mfd/zl3073x-i2c.c
 create mode 100644 drivers/mfd/zl3073x-spi.c
 create mode 100644 drivers/mfd/zl3073x.h
 create mode 100644 include/linux/mfd/zl3073x.h

-- 
2.48.1

[PATCH v2 01/14] dt-bindings: dpll: Add device tree bindings for DPLL device and pin

[Ivan Vecera]

Add a common DT schema for DPLL device and associated pin.
The DPLL (device phase-locked loop) is a device used for precise clock
synchronization in networking and telecom hardware.

The device itself is equipped with one or more DPLLs (channels) and
one or more physical input and output pins.

Each DPLL channel is used either to provide pulse-per-clock signal or
to drive ethernet equipment clock.

The input and output pins have a label (specifies board label),
type (specifies its usage depending on wiring), list of supported
or allowed frequencies (depending on how the pin is connected and
where) and can support embedded sync capability.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v1->v2:
* rewritten description for both device and pin
* dropped num-dplls property
* supported-frequencies property renamed to supported-frequencies-hz
---
 .../devicetree/bindings/dpll/dpll-device.yaml | 76 +++++++++++++++++++
 .../devicetree/bindings/dpll/dpll-pin.yaml    | 44 +++++++++++
 MAINTAINERS                                   |  2 +
 3 files changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
 create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml

diff --git a/Documentation/devicetree/bindings/dpll/dpll-device.yaml b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
new file mode 100644
index 0000000000000..11a02b74e28b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dpll/dpll-device.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Digital Phase-Locked Loop (DPLL) Device
+
+maintainers:
+  - Ivan Vecera <ivecera@redhat.com>
+
+description:
+  Digital Phase-Locked Loop (DPLL) device is used for precise clock
+  synchronization in networking and telecom hardware. The device can
+  have one or more channels (DPLLs) and one or more physical input and
+  output pins. Each DPLL channel can either produce pulse-per-clock signal
+  or drive ethernet equipment clock. The type of each channel can be
+  indicated by dpll-types property.
+
+properties:
+  $nodename:
+    pattern: "^dpll(@.*)?$"
+
+  "#address-cells":
+    const: 0
+
+  "#size-cells":
+    const: 0
+
+  dpll-types:
+    description: List of DPLL channel types, one per DPLL instance.
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    items:
+      enum: [pps, eec]
+
+  input-pins:
+    type: object
+    description: DPLL input pins
+    unevaluatedProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      "^pin@[0-9]+$":
+        $ref: /schemas/dpll/dpll-pin.yaml
+        unevaluatedProperties: false
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+  output-pins:
+    type: object
+    description: DPLL output pins
+    unevaluatedProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      "^pin@[0-9]+$":
+        $ref: /schemas/dpll/dpll-pin.yaml
+        unevaluatedProperties: false
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/dpll/dpll-pin.yaml b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
new file mode 100644
index 0000000000000..44af3a4398a5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dpll/dpll-pin.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DPLL Pin
+
+maintainers:
+  - Ivan Vecera <ivecera@redhat.com>
+
+description: |
+  The DPLL pin is either a physical input or output pin that is provided
+  by a DPLL( Digital Phase-Locked Loop) device. The pin is identified by
+  its physical order number that is stored in reg property and can have
+  an additional set of properties like supported (allowed) frequencies,
+  label, type and may support embedded sync.
+  Note that the pin in this context has nothing to do with pinctrl.
+
+properties:
+  reg:
+    description: Hardware index of the DPLL pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  esync-control:
+    description: Indicates whether the pin supports embedded sync functionality.
+    type: boolean
+
+  label:
+    description: String exposed as the pin board label
+    $ref: /schemas/types.yaml#/definitions/string
+
+  supported-frequencies-hz:
+    description: List of supported frequencies for this pin, expressed in Hz.
+
+  type:
+    description: Type of the pin
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ext, gnss, int, mux, synce]
+
+required:
+  - reg
+
+additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 4c5c2e2c12787..0742a10e87c88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7194,6 +7194,8 @@ M:	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
 M:	Jiri Pirko <jiri@resnulli.us>
 L:	netdev@vger.kernel.org
 S:	Supported
+F:	Documentation/devicetree/bindings/dpll/dpll-device.yaml
+F:	Documentation/devicetree/bindings/dpll/dpll-pin.yaml
 F:	Documentation/driver-api/dpll.rst
 F:	drivers/dpll/*
 F:	include/linux/dpll.h
-- 
2.48.1

[PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

Add DT bindings for Microchip Azurite DPLL chip family. These chips
provides 2 independent DPLL channels, up to 10 differential or
single-ended inputs and up to 20 differential or 20 single-ended outputs.
It can be connected via I2C or SPI busses.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
 .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 +++++++++++++++++++
 2 files changed, 151 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
 create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml

diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
new file mode 100644
index 0000000000000..d9280988f9eb7
--- /dev/null
+++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C-attached Microchip Azurite DPLL device
+
+maintainers:
+  - Ivan Vecera <ivecera@redhat.com>
+
+description:
+  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
+  provides 2 independent DPLL channels, up to 10 differential or
+  single-ended inputs and up to 20 differential or 20 single-ended outputs.
+  It can be connected via multiple busses, one of them being I2C.
+
+properties:
+  compatible:
+    enum:
+      - microchip,zl3073x-i2c
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/dpll/dpll-device.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      dpll@70 {
+        compatible = "microchip,zl3073x-i2c";
+        reg = <0x70>;
+        #address-cells = <0>;
+        #size-cells = <0>;
+        dpll-types = "pps", "eec";
+
+        input-pins {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          pin@0 { /* REF0P */
+            reg = <0>;
+            label = "Input 0";
+            supported-frequencies = /bits/ 64 <1 1000>;
+            type = "ext";
+          };
+        };
+
+        output-pins {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          pin@3 { /* OUT1N */
+            reg = <3>;
+            esync-control;
+            label = "Output 1";
+            supported-frequencies = /bits/ 64 <1 10000>;
+            type = "gnss";
+          };
+        };
+      };
+    };
+...
diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
new file mode 100644
index 0000000000000..7bd6e5099e1ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SPI-attached Microchip Azurite DPLL device
+
+maintainers:
+  - Ivan Vecera <ivecera@redhat.com>
+
+description:
+  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
+  provides 2 independent DPLL channels, up to 10 differential or
+  single-ended inputs and up to 20 differential or 20 single-ended outputs.
+  It can be connected via multiple busses, one of them being I2C.
+
+properties:
+  compatible:
+    enum:
+      - microchip,zl3073x-spi
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/dpll/dpll-device.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      dpll@70 {
+        compatible = "microchip,zl3073x-spi";
+        reg = <0x70>;
+        #address-cells = <0>;
+        #size-cells = <0>;
+        spi-max-frequency = <12500000>;
+
+        dpll-types = "pps", "eec";
+
+        input-pins {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          pin@0 { /* REF0P */
+            reg = <0>;
+            label = "Input 0";
+            supported-frequencies = /bits/ 64 <1 1000>;
+            type = "ext";
+          };
+        };
+
+        output-pins {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          pin@3 { /* OUT1N */
+            reg = <3>;
+            esync-control;
+            label = "Output 1";
+            supported-frequencies = /bits/ 64 <1 10000>;
+            type = "gnss";
+          };
+        };
+      };
+    };
+...
-- 
2.48.1

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Krzysztof Kozlowski]

On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
> Add DT bindings for Microchip Azurite DPLL chip family. These chips
> provides 2 independent DPLL channels, up to 10 differential or
> single-ended inputs and up to 20 differential or 20 single-ended outputs.
> It can be connected via I2C or SPI busses.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>  .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 +++++++++++++++++++

No, you do not get two files. No such bindings were accepted since some
years.

>  2 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
> new file mode 100644
> index 0000000000000..d9280988f9eb7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C-attached Microchip Azurite DPLL device
> +
> +maintainers:
> +  - Ivan Vecera <ivecera@redhat.com>
> +
> +description:
> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
> +  provides 2 independent DPLL channels, up to 10 differential or
> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
> +  It can be connected via multiple busses, one of them being I2C.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,zl3073x-i2c

I already said: you have one compatible, not two. One.

Also, still wildcard, so still a no.


> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: /schemas/dpll/dpll-device.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      dpll@70 {
> +        compatible = "microchip,zl3073x-i2c";
> +        reg = <0x70>;
> +        #address-cells = <0>;
> +        #size-cells = <0>;

Again, not used. Drop.


> +        dpll-types = "pps", "eec";
> +
> +        input-pins {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          pin@0 { /* REF0P */
> +            reg = <0>;
> +            label = "Input 0";
> +            supported-frequencies = /bits/ 64 <1 1000>;
> +            type = "ext";
> +          };
> +        };
> +
> +        output-pins {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          pin@3 { /* OUT1N */
> +            reg = <3>;
> +            esync-control;
> +            label = "Output 1";
> +            supported-frequencies = /bits/ 64 <1 10000>;
> +            type = "gnss";
> +          };
> +        };
> +      };
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
> new file mode 100644
> index 0000000000000..7bd6e5099e1ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml

No, you do not get two files. Neither two compatibles, nor two files.
Please look at existing bindings how they do it for such devices.

Best regards,
Krzysztof

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
> On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
>> Add DT bindings for Microchip Azurite DPLL chip family. These chips
>> provides 2 independent DPLL channels, up to 10 differential or
>> single-ended inputs and up to 20 differential or 20 single-ended outputs.
>> It can be connected via I2C or SPI busses.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>>   .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>>   .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 +++++++++++++++++++
> 
> No, you do not get two files. No such bindings were accepted since some
> years.
> 
>>   2 files changed, 151 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>> new file mode 100644
>> index 0000000000000..d9280988f9eb7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C-attached Microchip Azurite DPLL device
>> +
>> +maintainers:
>> +  - Ivan Vecera <ivecera@redhat.com>
>> +
>> +description:
>> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
>> +  provides 2 independent DPLL channels, up to 10 differential or
>> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
>> +  It can be connected via multiple busses, one of them being I2C.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,zl3073x-i2c
> 
> I already said: you have one compatible, not two. One.

Ah, you mean something like:
iio/accel/adi,adxl313.yaml

Do you?

> Also, still wildcard, so still a no.

This is not wildcard, Microchip uses this to designate DPLL devices with 
the same characteristics.

But I can use microchip,azurite, is it more appropriate?

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +allOf:
>> +  - $ref: /schemas/dpll/dpll-device.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      dpll@70 {
>> +        compatible = "microchip,zl3073x-i2c";
>> +        reg = <0x70>;
>> +        #address-cells = <0>;
>> +        #size-cells = <0>;
> 
> Again, not used. Drop.

Sorry, will do.

>> +        dpll-types = "pps", "eec";
>> +
>> +        input-pins {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +
>> +          pin@0 { /* REF0P */
>> +            reg = <0>;
>> +            label = "Input 0";
>> +            supported-frequencies = /bits/ 64 <1 1000>;
>> +            type = "ext";
>> +          };
>> +        };
>> +
>> +        output-pins {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +
>> +          pin@3 { /* OUT1N */
>> +            reg = <3>;
>> +            esync-control;
>> +            label = "Output 1";
>> +            supported-frequencies = /bits/ 64 <1 10000>;
>> +            type = "gnss";
>> +          };
>> +        };
>> +      };
>> +    };
>> +...
>> diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>> new file mode 100644
>> index 0000000000000..7bd6e5099e1ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
> 
> No, you do not get two files. Neither two compatibles, nor two files.
> Please look at existing bindings how they do it for such devices.

Thanks Krzysztof for comments.

Ivan

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Conor Dooley]

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

On Thu, Apr 10, 2025 at 09:45:47AM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
> > On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
> > > Add DT bindings for Microchip Azurite DPLL chip family. These chips
> > > provides 2 independent DPLL channels, up to 10 differential or
> > > single-ended inputs and up to 20 differential or 20 single-ended outputs.
> > > It can be connected via I2C or SPI busses.
> > > 
> > > Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> > > ---
> > >   .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
> > >   .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 +++++++++++++++++++
> > 
> > No, you do not get two files. No such bindings were accepted since some
> > years.
> > 
> > >   2 files changed, 151 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
> > >   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
> > > new file mode 100644
> > > index 0000000000000..d9280988f9eb7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
> > > @@ -0,0 +1,74 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: I2C-attached Microchip Azurite DPLL device
> > > +
> > > +maintainers:
> > > +  - Ivan Vecera <ivecera@redhat.com>
> > > +
> > > +description:
> > > +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
> > > +  provides 2 independent DPLL channels, up to 10 differential or
> > > +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
> > > +  It can be connected via multiple busses, one of them being I2C.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - microchip,zl3073x-i2c
> > 
> > I already said: you have one compatible, not two. One.
> 
> Ah, you mean something like:
> iio/accel/adi,adxl313.yaml
> 
> Do you?
> 
> > Also, still wildcard, so still a no.
> 
> This is not wildcard, Microchip uses this to designate DPLL devices with the
> same characteristics.

That's the very definition of a wildcard, no? The x is matching against
several different devices. There's like 14 different parts matching
zl3073x, with varying numbers of outputs and channels. One compatible
for all of that hardly seems suitable.

> 
> But I can use microchip,azurite, is it more appropriate?

No, I think that is worse actually.

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

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

On 10. 04. 25 3:18 odp., Conor Dooley wrote:
> On Thu, Apr 10, 2025 at 09:45:47AM +0200, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
>>> On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
>>>> Add DT bindings for Microchip Azurite DPLL chip family. These chips
>>>> provides 2 independent DPLL channels, up to 10 differential or
>>>> single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>> It can be connected via I2C or SPI busses.
>>>>
>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>> ---
>>>>    .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>>>>    .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 +++++++++++++++++++
>>>
>>> No, you do not get two files. No such bindings were accepted since some
>>> years.
>>>
>>>>    2 files changed, 151 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>    create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>> new file mode 100644
>>>> index 0000000000000..d9280988f9eb7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>> @@ -0,0 +1,74 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: I2C-attached Microchip Azurite DPLL device
>>>> +
>>>> +maintainers:
>>>> +  - Ivan Vecera <ivecera@redhat.com>
>>>> +
>>>> +description:
>>>> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices that
>>>> +  provides 2 independent DPLL channels, up to 10 differential or
>>>> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>> +  It can be connected via multiple busses, one of them being I2C.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - microchip,zl3073x-i2c
>>>
>>> I already said: you have one compatible, not two. One.
>>
>> Ah, you mean something like:
>> iio/accel/adi,adxl313.yaml
>>
>> Do you?
>>
>>> Also, still wildcard, so still a no.
>>
>> This is not wildcard, Microchip uses this to designate DPLL devices with the
>> same characteristics.
> 
> That's the very definition of a wildcard, no? The x is matching against
> several different devices. There's like 14 different parts matching
> zl3073x, with varying numbers of outputs and channels. One compatible
> for all of that hardly seems suitable.

Prathosh, could you please bring more light on this?

Thanks.
> 
>>
>> But I can use microchip,azurite, is it more appropriate?
> 
> No, I think that is worse actually.

RE: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Prathosh.Satish]

-----Original Message-----
From: Ivan Vecera <ivecera@redhat.com> 
Sent: Thursday 10 April 2025 14:36
To: Conor Dooley <conor@kernel.org>; Prathosh Satish - M66066 <Prathosh.Satish@microchip.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>; netdev@vger.kernel.org; Vadim Fedorenko <vadim.fedorenko@linux.dev>; Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>; Jiri Pirko <jiri@resnulli.us>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Prathosh Satish - M66066 <Prathosh.Satish@microchip.com>; Lee Jones <lee@kernel.org>; Kees Cook <kees@kernel.org>; Andy Shevchenko <andy@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; Michal Schmidt <mschmidt@redhat.com>; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 10. 04. 25 3:18 odp., Conor Dooley wrote:
> On Thu, Apr 10, 2025 at 09:45:47AM +0200, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
>>> On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
>>>> Add DT bindings for Microchip Azurite DPLL chip family. These chips 
>>>> provides 2 independent DPLL channels, up to 10 differential or 
>>>> single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>> It can be connected via I2C or SPI busses.
>>>>
>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>> ---
>>>>    .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>>>>    .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77 
>>>> +++++++++++++++++++
>>>
>>> No, you do not get two files. No such bindings were accepted since 
>>> some years.
>>>
>>>>    2 files changed, 151 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>    create mode 100644 
>>>> Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml 
>>>> b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>> new file mode 100644
>>>> index 0000000000000..d9280988f9eb7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.
>>>> +++ yaml
>>>> @@ -0,0 +1,74 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
>>>> +---
>>>> +$id: 
>>>> +http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: I2C-attached Microchip Azurite DPLL device
>>>> +
>>>> +maintainers:
>>>> +  - Ivan Vecera <ivecera@redhat.com>
>>>> +
>>>> +description:
>>>> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices 
>>>> +that
>>>> +  provides 2 independent DPLL channels, up to 10 differential or
>>>> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>> +  It can be connected via multiple busses, one of them being I2C.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - microchip,zl3073x-i2c
>>>
>>> I already said: you have one compatible, not two. One.
>>
>> Ah, you mean something like:
>> iio/accel/adi,adxl313.yaml
>>
>> Do you?
>>
>>> Also, still wildcard, so still a no.
>>
>> This is not wildcard, Microchip uses this to designate DPLL devices 
>> with the same characteristics.
>
> That's the very definition of a wildcard, no? The x is matching 
> against several different devices. There's like 14 different parts 
> matching zl3073x, with varying numbers of outputs and channels. One 
> compatible for all of that hardly seems suitable.

Prathosh, could you please bring more light on this?

> Just to clarify, the original driver was written specifically with 2-channel 
> chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> However, the final version of the driver will support the entire ZL3073x family 
> ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc 
> ensuring compatibility across all variants.


Thanks.
>
>>
>> But I can use microchip,azurite, is it more appropriate?
>
> No, I think that is worse actually.

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

On 10. 04. 25 7:07 odp., Prathosh.Satish@microchip.com wrote:
> -----Original Message-----
> From: Ivan Vecera <ivecera@redhat.com>
> Sent: Thursday 10 April 2025 14:36
> To: Conor Dooley <conor@kernel.org>; Prathosh Satish - M66066 <Prathosh.Satish@microchip.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>; netdev@vger.kernel.org; Vadim Fedorenko <vadim.fedorenko@linux.dev>; Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>; Jiri Pirko <jiri@resnulli.us>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Prathosh Satish - M66066 <Prathosh.Satish@microchip.com>; Lee Jones <lee@kernel.org>; Kees Cook <kees@kernel.org>; Andy Shevchenko <andy@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; Michal Schmidt <mschmidt@redhat.com>; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-hardening@vger.kernel.org
> Subject: Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10. 04. 25 3:18 odp., Conor Dooley wrote:
>> On Thu, Apr 10, 2025 at 09:45:47AM +0200, Ivan Vecera wrote:
>>>
>>>
>>> On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
>>>> On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
>>>>> Add DT bindings for Microchip Azurite DPLL chip family. These chips
>>>>> provides 2 independent DPLL channels, up to 10 differential or
>>>>> single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>>> It can be connected via I2C or SPI busses.
>>>>>
>>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>>> ---
>>>>>     .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>>>>>     .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77
>>>>> +++++++++++++++++++
>>>>
>>>> No, you do not get two files. No such bindings were accepted since
>>>> some years.
>>>>
>>>>>     2 files changed, 151 insertions(+)
>>>>>     create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>>     create mode 100644
>>>>> Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>> b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>> new file mode 100644
>>>>> index 0000000000000..d9280988f9eb7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.
>>>>> +++ yaml
>>>>> @@ -0,0 +1,74 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
>>>>> +---
>>>>> +$id:
>>>>> +http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: I2C-attached Microchip Azurite DPLL device
>>>>> +
>>>>> +maintainers:
>>>>> +  - Ivan Vecera <ivecera@redhat.com>
>>>>> +
>>>>> +description:
>>>>> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices
>>>>> +that
>>>>> +  provides 2 independent DPLL channels, up to 10 differential or
>>>>> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>>> +  It can be connected via multiple busses, one of them being I2C.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - microchip,zl3073x-i2c
>>>>
>>>> I already said: you have one compatible, not two. One.
>>>
>>> Ah, you mean something like:
>>> iio/accel/adi,adxl313.yaml
>>>
>>> Do you?
>>>
>>>> Also, still wildcard, so still a no.
>>>
>>> This is not wildcard, Microchip uses this to designate DPLL devices
>>> with the same characteristics.
>>
>> That's the very definition of a wildcard, no? The x is matching
>> against several different devices. There's like 14 different parts
>> matching zl3073x, with varying numbers of outputs and channels. One
>> compatible for all of that hardly seems suitable.
> 
> Prathosh, could you please bring more light on this?
> 
> Just to clarify, the original driver was written specifically with 2-channel
> chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> However, the final version of the driver will support the entire ZL3073x family
> ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc
> ensuring compatibility across all variants.

Huh, then ok... We should specify zl30731-5 compatibles and they differs 
only by number of channels (1-5) ?

The number of input and output pins are the same (10 and 20), right?

If so, I have to update the whole driver to accommodate dynamic number 
of channels according chip type.

Btw. Conor, Krzystof if we use microchip,zl30731, ..., 
microchip,zl30735... What should be the filename for the yaml file?

Thanks,
Ivan

> Thanks.
>>
>>>
>>> But I can use microchip,azurite, is it more appropriate?
>>
>> No, I think that is worse actually.
> 

RE: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Prathosh.Satish]

Hi Ivan,

yes you are right, the only difference is the number of channels.

Prathosh

-----Original Message-----
From: Ivan Vecera <ivecera@redhat.com> 
Sent: Thursday 10 April 2025 18:36
To: Prathosh Satish - M66066 <Prathosh.Satish@microchip.com>; conor@kernel.org
Cc: krzk@kernel.org; netdev@vger.kernel.org; vadim.fedorenko@linux.dev; arkadiusz.kubalewski@intel.com; jiri@resnulli.us; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; lee@kernel.org; kees@kernel.org; andy@kernel.org; akpm@linux-foundation.org; mschmidt@redhat.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 10. 04. 25 7:07 odp., Prathosh.Satish@microchip.com wrote:
> -----Original Message-----
> From: Ivan Vecera <ivecera@redhat.com>
> Sent: Thursday 10 April 2025 14:36
> To: Conor Dooley <conor@kernel.org>; Prathosh Satish - M66066 
> <Prathosh.Satish@microchip.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>; netdev@vger.kernel.org; 
> Vadim Fedorenko <vadim.fedorenko@linux.dev>; Arkadiusz Kubalewski 
> <arkadiusz.kubalewski@intel.com>; Jiri Pirko <jiri@resnulli.us>; Rob 
> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; 
> Conor Dooley <conor+dt@kernel.org>; Prathosh Satish - M66066 
> <Prathosh.Satish@microchip.com>; Lee Jones <lee@kernel.org>; Kees Cook 
> <kees@kernel.org>; Andy Shevchenko <andy@kernel.org>; Andrew Morton 
> <akpm@linux-foundation.org>; Michal Schmidt <mschmidt@redhat.com>; 
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; 
> linux-hardening@vger.kernel.org
> Subject: Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for 
> Microchip Azurite chip family
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
>
> On 10. 04. 25 3:18 odp., Conor Dooley wrote:
>> On Thu, Apr 10, 2025 at 09:45:47AM +0200, Ivan Vecera wrote:
>>>
>>>
>>> On 10. 04. 25 9:06 dop., Krzysztof Kozlowski wrote:
>>>> On Wed, Apr 09, 2025 at 04:42:38PM GMT, Ivan Vecera wrote:
>>>>> Add DT bindings for Microchip Azurite DPLL chip family. These 
>>>>> chips provides 2 independent DPLL channels, up to 10 differential 
>>>>> or single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>>> It can be connected via I2C or SPI busses.
>>>>>
>>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>>> ---
>>>>>     .../bindings/dpll/microchip,zl3073x-i2c.yaml  | 74 ++++++++++++++++++
>>>>>     .../bindings/dpll/microchip,zl3073x-spi.yaml  | 77
>>>>> +++++++++++++++++++
>>>>
>>>> No, you do not get two files. No such bindings were accepted since 
>>>> some years.
>>>>
>>>>>     2 files changed, 151 insertions(+)
>>>>>     create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>>>>     create mode 100644
>>>>> Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yam
>>>>> l 
>>>>> b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yam
>>>>> l
>>>>> new file mode 100644
>>>>> index 0000000000000..d9280988f9eb7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.
>>>>> +++ yaml
>>>>> @@ -0,0 +1,74 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
>>>>> +---
>>>>> +$id:
>>>>> +http://devicetree.org/schemas/dpll/microchip,zl3073x-i2c.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: I2C-attached Microchip Azurite DPLL device
>>>>> +
>>>>> +maintainers:
>>>>> +  - Ivan Vecera <ivecera@redhat.com>
>>>>> +
>>>>> +description:
>>>>> +  Microchip Azurite DPLL (ZL3073x) is a family of DPLL devices 
>>>>> +that
>>>>> +  provides 2 independent DPLL channels, up to 10 differential or
>>>>> +  single-ended inputs and up to 20 differential or 20 single-ended outputs.
>>>>> +  It can be connected via multiple busses, one of them being I2C.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - microchip,zl3073x-i2c
>>>>
>>>> I already said: you have one compatible, not two. One.
>>>
>>> Ah, you mean something like:
>>> iio/accel/adi,adxl313.yaml
>>>
>>> Do you?
>>>
>>>> Also, still wildcard, so still a no.
>>>
>>> This is not wildcard, Microchip uses this to designate DPLL devices 
>>> with the same characteristics.
>>
>> That's the very definition of a wildcard, no? The x is matching 
>> against several different devices. There's like 14 different parts 
>> matching zl3073x, with varying numbers of outputs and channels. One 
>> compatible for all of that hardly seems suitable.
>
> Prathosh, could you please bring more light on this?
>
> Just to clarify, the original driver was written specifically with 
> 2-channel chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> However, the final version of the driver will support the entire 
> ZL3073x family
> ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc 
> ensuring compatibility across all variants.

Huh, then ok... We should specify zl30731-5 compatibles and they differs only by number of channels (1-5) ?

The number of input and output pins are the same (10 and 20), right?

If so, I have to update the whole driver to accommodate dynamic number of channels according chip type.

Btw. Conor, Krzystof if we use microchip,zl30731, ..., microchip,zl30735... What should be the filename for the yaml file?

Thanks,
Ivan

> Thanks.
>>
>>>
>>> But I can use microchip,azurite, is it more appropriate?
>>
>> No, I think that is worse actually.
>

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Andrew Lunn]

> Prathosh, could you please bring more light on this?
> 
> > Just to clarify, the original driver was written specifically with 2-channel 
> > chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> > However, the final version of the driver will support the entire ZL3073x family 
> > ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc 
> > ensuring compatibility across all variants.

Hi Prathosh

Your email quoting is very odd, i nearly missed this reply.

Does the device itself have an ID register? If you know you have
something in the range ZL30731 to ZL30735, you can ask the hardware
what it is, and the driver then does not need any additional
information from DT, it can hard code it all based on the ID in the
register?

	Andrew

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

On 10. 04. 25 7:36 odp., Andrew Lunn wrote:
>> Prathosh, could you please bring more light on this?
>>
>>> Just to clarify, the original driver was written specifically with 2-channel
>>> chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
>>> However, the final version of the driver will support the entire ZL3073x family
>>> ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc
>>> ensuring compatibility across all variants.
> 
> Hi Prathosh
> 
> Your email quoting is very odd, i nearly missed this reply.
> 
> Does the device itself have an ID register? If you know you have
> something in the range ZL30731 to ZL30735, you can ask the hardware
> what it is, and the driver then does not need any additional
> information from DT, it can hard code it all based on the ID in the
> register?
> 
> 	Andrew
> 
Hi Andrew,
yes there is ID register that identifies the ID. But what compatible 
should be used?

microchip,zl3073x was rejected as wildcard and we should use all 
compatibles.

Thanks,
Ivan

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Andrew Lunn]

On Thu, Apr 10, 2025 at 08:33:31PM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 7:36 odp., Andrew Lunn wrote:
> > > Prathosh, could you please bring more light on this?
> > > 
> > > > Just to clarify, the original driver was written specifically with 2-channel
> > > > chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> > > > However, the final version of the driver will support the entire ZL3073x family
> > > > ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc
> > > > ensuring compatibility across all variants.
> > 
> > Hi Prathosh
> > 
> > Your email quoting is very odd, i nearly missed this reply.
> > 
> > Does the device itself have an ID register? If you know you have
> > something in the range ZL30731 to ZL30735, you can ask the hardware
> > what it is, and the driver then does not need any additional
> > information from DT, it can hard code it all based on the ID in the
> > register?
> > 
> > 	Andrew
> > 
> Hi Andrew,
> yes there is ID register that identifies the ID. But what compatible should
> be used?
> 
> microchip,zl3073x was rejected as wildcard and we should use all
> compatibles.

You have two choices really:

1) You list each device with its own compatible, because they are in
fact not compatible. You need to handle each one different, they have
different DT properties, etc. If you do that, please validate the ID
register against the compatible and return -ENODEV if they don't
match.

2) You say the devices are compatible. So the DT compatible just
indicates the family, enough information for the driver to go find the
ID register. This does however require the binding is the same for all
devices. You cannot have one family member listing 10 inputs in its
binding, and another family member listing 20.

If you say your devices are incompatible, and list lots of
compatibles, you can then use constraints in the yaml, based on the
compatible, to limit each family member to what it supports.

My guess is, you are going to take the first route.

	Andrew

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Ivan Vecera]

On 10. 04. 25 11:12 odp., Andrew Lunn wrote:
> On Thu, Apr 10, 2025 at 08:33:31PM +0200, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 7:36 odp., Andrew Lunn wrote:
>>>> Prathosh, could you please bring more light on this?
>>>>
>>>>> Just to clarify, the original driver was written specifically with 2-channel
>>>>> chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
>>>>> However, the final version of the driver will support the entire ZL3073x family
>>>>> ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc
>>>>> ensuring compatibility across all variants.
>>>
>>> Hi Prathosh
>>>
>>> Your email quoting is very odd, i nearly missed this reply.
>>>
>>> Does the device itself have an ID register? If you know you have
>>> something in the range ZL30731 to ZL30735, you can ask the hardware
>>> what it is, and the driver then does not need any additional
>>> information from DT, it can hard code it all based on the ID in the
>>> register?
>>>
>>> 	Andrew
>>>
>> Hi Andrew,
>> yes there is ID register that identifies the ID. But what compatible should
>> be used?
>>
>> microchip,zl3073x was rejected as wildcard and we should use all
>> compatibles.
> 
> You have two choices really:
> 
> 1) You list each device with its own compatible, because they are in
> fact not compatible. You need to handle each one different, they have
> different DT properties, etc. If you do that, please validate the ID
> register against the compatible and return -ENODEV if they don't
> match.
> 
> 2) You say the devices are compatible. So the DT compatible just
> indicates the family, enough information for the driver to go find the
> ID register. This does however require the binding is the same for all
> devices. You cannot have one family member listing 10 inputs in its
> binding, and another family member listing 20.
> 
> If you say your devices are incompatible, and list lots of
> compatibles, you can then use constraints in the yaml, based on the
> compatible, to limit each family member to what it supports.
> 
> My guess is, you are going to take the first route.

Yes, this looks reasonable... in this case should I use 
microchip,zl3073x.yaml like e.g. gpio/gpio-pca95xx.yaml?

Ivan

Re: [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family

[Conor Dooley]

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

On Fri, Apr 11, 2025 at 11:56:15AM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 11:12 odp., Andrew Lunn wrote:
> > On Thu, Apr 10, 2025 at 08:33:31PM +0200, Ivan Vecera wrote:
> > > 
> > > 
> > > On 10. 04. 25 7:36 odp., Andrew Lunn wrote:
> > > > > Prathosh, could you please bring more light on this?
> > > > > 
> > > > > > Just to clarify, the original driver was written specifically with 2-channel
> > > > > > chips in mind (ZL30732) with 10 input and 20 outputs, which led to some confusion of using zl3073x as compatible.
> > > > > > However, the final version of the driver will support the entire ZL3073x family
> > > > > > ZL30731 to ZL30735 and some subset of ZL30732 like ZL80732 etc
> > > > > > ensuring compatibility across all variants.
> > > > 
> > > > Hi Prathosh
> > > > 
> > > > Your email quoting is very odd, i nearly missed this reply.
> > > > 
> > > > Does the device itself have an ID register? If you know you have
> > > > something in the range ZL30731 to ZL30735, you can ask the hardware
> > > > what it is, and the driver then does not need any additional
> > > > information from DT, it can hard code it all based on the ID in the
> > > > register?
> > > > 
> > > > 	Andrew
> > > > 
> > > Hi Andrew,
> > > yes there is ID register that identifies the ID. But what compatible should
> > > be used?
> > > 
> > > microchip,zl3073x was rejected as wildcard and we should use all
> > > compatibles.
> > 
> > You have two choices really:
> > 
> > 1) You list each device with its own compatible, because they are in
> > fact not compatible. You need to handle each one different, they have
> > different DT properties, etc. If you do that, please validate the ID
> > register against the compatible and return -ENODEV if they don't
> > match.
> > 
> > 2) You say the devices are compatible. So the DT compatible just
> > indicates the family, enough information for the driver to go find the
> > ID register. This does however require the binding is the same for all
> > devices. You cannot have one family member listing 10 inputs in its
> > binding, and another family member listing 20.
> > 
> > If you say your devices are incompatible, and list lots of
> > compatibles, you can then use constraints in the yaml, based on the
> > compatible, to limit each family member to what it supports.
> > 
> > My guess is, you are going to take the first route.
> 
> Yes, this looks reasonable... in this case should I use
> microchip,zl3073x.yaml like e.g. gpio/gpio-pca95xx.yaml?

No, please pick one of the compatibles in the file and name the same as
one of those.

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

[PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Ivan Vecera]

Add base MFD driver for Microchip Azurite ZL3073x chip family.
These chips provide DPLL and PHC (PTP) functionality and they can
be connected over I2C or SPI bus.

The MFD driver provide basic communication and synchronization
over the bus and common functionality that are used by the DPLL
driver (later in this series) and by the PTP driver (will be
added later).

The chip family is characterized by following properties:
* 2 separate DPLL units (channels)
* 5 synthesizers
* 10 input pins (references)
* 10 outputs
* 20 output pins (output pin pair shares one output)
* Each reference and output can act in differential or single-ended
  mode (reference or output in differential mode consumes 2 pins)
* Each output is connected to one of the synthesizers
* Each synthesizer is driven by one of the DPLL unit

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
v1->v2:
* fixed header issues
* removed usage of of_match_ptr
* added check for devm_mutex_init
* removed commas after sentinels
* removed variable initialization in zl3073x_i2c_probe()
* moved device tables closer to their users
* renamed zl3073x_dev_alloc() to zl3073x_devm_alloc()
* removed empty zl3073x_dev_exit()
* spidev renamed to spi
* squashed together with device DT bindings
* used dev_err_probe() instead of dev_err() during probe
* added some function documentation
DT bindings:
* spliced to separate files for i2c and spi
* fixed property order in DT bindings' examples
* added description
---
 MAINTAINERS                 |   9 ++++
 drivers/mfd/Kconfig         |  30 +++++++++++
 drivers/mfd/Makefile        |   5 ++
 drivers/mfd/zl3073x-core.c  | 101 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/zl3073x-i2c.c   |  58 +++++++++++++++++++++
 drivers/mfd/zl3073x-spi.c   |  58 +++++++++++++++++++++
 drivers/mfd/zl3073x.h       |  14 +++++
 include/linux/mfd/zl3073x.h |  23 ++++++++
 8 files changed, 298 insertions(+)
 create mode 100644 drivers/mfd/zl3073x-core.c
 create mode 100644 drivers/mfd/zl3073x-i2c.c
 create mode 100644 drivers/mfd/zl3073x-spi.c
 create mode 100644 drivers/mfd/zl3073x.h
 create mode 100644 include/linux/mfd/zl3073x.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0742a10e87c88..5c086e945b148 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15996,6 +15996,15 @@ L:	linux-wireless@vger.kernel.org
 S:	Supported
 F:	drivers/net/wireless/microchip/
 
+MICROCHIP ZL3073X DRIVER
+M:	Ivan Vecera <ivecera@redhat.com>
+M:	Prathosh Satish <Prathosh.Satish@microchip.com>
+L:	netdev@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/dpll/microchip,zl3073x*.yaml
+F:	drivers/mfd/zl3073x*
+F:	include/linux/mfd/zl3073x.h
+
 MICROSEMI MIPS SOCS
 M:	Alexandre Belloni <alexandre.belloni@bootlin.com>
 M:	UNGLinuxDriver@microchip.com
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 22b9363100394..30b36e3ee8f7f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2422,5 +2422,35 @@ config MFD_UPBOARD_FPGA
 	  To compile this driver as a module, choose M here: the module will be
 	  called upboard-fpga.
 
+config MFD_ZL3073X_CORE
+	tristate
+	select MFD_CORE
+
+config MFD_ZL3073X_I2C
+	tristate "Microchip Azurite DPLL/PTP/SyncE with I2C"
+	depends on I2C
+	select MFD_ZL3073X_CORE
+	select REGMAP_I2C
+	help
+	  Support for Microchip Azurite DPLL/PTP/SyncE chip family. This option
+	  supports I2C as the control interface.
+
+	  This driver provides common support for accessing the device.
+	  Additional drivers must be enabled in order to use the functionality
+	  of the device.
+
+config MFD_ZL3073X_SPI
+	tristate "Microchip Azurite DPLL/PTP/SyncE with SPI"
+	depends on SPI
+	select MFD_ZL3073X_CORE
+	select REGMAP_SPI
+	help
+	  Support for Microchip Azurite DPLL/PTP/SyncE chip family. This option
+	  supports SPI as the control interface.
+
+	  This driver provides common support for accessing the device.
+	  Additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 948cbdf42a18b..76e2babc1538f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -290,3 +290,8 @@ obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu_i2c.o rsmu_core.o
 obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
 
 obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
+
+zl3073x-y			:= zl3073x-core.o
+obj-$(CONFIG_MFD_ZL3073X_CORE)	+= zl3073x.o
+obj-$(CONFIG_MFD_ZL3073X_I2C)	+= zl3073x-i2c.o
+obj-$(CONFIG_MFD_ZL3073X_SPI)	+= zl3073x-spi.o
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
new file mode 100644
index 0000000000000..ccb6987d04a20
--- /dev/null
+++ b/drivers/mfd/zl3073x-core.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/mfd/zl3073x.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include "zl3073x.h"
+
+/*
+ * Regmap ranges
+ */
+#define ZL3073x_PAGE_SIZE	128
+#define ZL3073x_NUM_PAGES	16
+#define ZL3073x_PAGE_SEL	0x7F
+
+/*
+ * Regmap range configuration
+ *
+ * The device uses 7-bit addressing and has 16 register pages with
+ * range 0x00-0x7f. The register 0x7f in each page acts as page
+ * selector where bits 0-3 contains currently selected page.
+ */
+static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
+	{
+		.range_min	= 0,
+		.range_max	= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
+		.selector_reg	= ZL3073x_PAGE_SEL,
+		.selector_mask	= GENMASK(3, 0),
+		.selector_shift	= 0,
+		.window_start	= 0,
+		.window_len	= ZL3073x_PAGE_SIZE,
+	},
+};
+
+/*
+ * Regmap config
+ */
+const struct regmap_config zl3073x_regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
+	.ranges			= zl3073x_regmap_ranges,
+	.num_ranges		= ARRAY_SIZE(zl3073x_regmap_ranges),
+};
+
+/**
+ * zl3073x_get_regmap_config - return pointer to regmap config
+ *
+ * Return: pointer to regmap config
+ */
+const struct regmap_config *zl3073x_get_regmap_config(void)
+{
+	return &zl3073x_regmap_config;
+}
+EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
+
+/**
+ * zl3073x_devm_alloc - allocates zl3073x device structure
+ * @dev: pointer to device structure
+ *
+ * Allocates zl3073x device structure as device resource.
+ *
+ * Return: pointer to zl3073x device structure
+ */
+struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev)
+{
+	struct zl3073x_dev *zldev;
+
+	return devm_kzalloc(dev, sizeof(*zldev), GFP_KERNEL);
+}
+EXPORT_SYMBOL_NS_GPL(zl3073x_devm_alloc, "ZL3073X");
+
+/**
+ * zl3073x_dev_init - initialize zl3073x device
+ * @zldev: pointer to zl3073x device
+ *
+ * Common initialization of zl3073x device structure.
+ *
+ * Returns: 0 on success, <0 on error
+ */
+int zl3073x_dev_init(struct zl3073x_dev *zldev)
+{
+	int rc;
+
+	rc = devm_mutex_init(zldev->dev, &zldev->lock);
+	if (rc) {
+		dev_err_probe(zldev->dev, rc, "Failed to initialize mutex\n");
+		return rc;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(zl3073x_dev_init, "ZL3073X");
+
+MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
+MODULE_DESCRIPTION("Microchip ZL3073x core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/zl3073x-i2c.c b/drivers/mfd/zl3073x-i2c.c
new file mode 100644
index 0000000000000..461b583e536b7
--- /dev/null
+++ b/drivers/mfd/zl3073x-i2c.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/i2c.h>
+#include <linux/mfd/zl3073x.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include "zl3073x.h"
+
+static int zl3073x_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct zl3073x_dev *zldev;
+
+	zldev = zl3073x_devm_alloc(dev);
+	if (!zldev)
+		return -ENOMEM;
+
+	zldev->dev = dev;
+	zldev->regmap = devm_regmap_init_i2c(client, zl3073x_get_regmap_config());
+	if (IS_ERR(zldev->regmap)) {
+		dev_err_probe(dev, PTR_ERR(zldev->regmap),
+			      "Failed to initialize register map\n");
+		return PTR_ERR(zldev->regmap);
+	}
+
+	i2c_set_clientdata(client, zldev);
+
+	return zl3073x_dev_init(zldev);
+}
+
+static const struct i2c_device_id zl3073x_i2c_id[] = {
+	{ "zl3073x-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, zl3073x_i2c_id);
+
+static const struct of_device_id zl3073x_i2c_of_match[] = {
+	{ .compatible = "microchip,zl3073x-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, zl3073x_i2c_of_match);
+
+static struct i2c_driver zl3073x_i2c_driver = {
+	.driver = {
+		.name = "zl3073x-i2c",
+		.of_match_table = zl3073x_i2c_of_match,
+	},
+	.probe = zl3073x_i2c_probe,
+	.id_table = zl3073x_i2c_id,
+};
+module_i2c_driver(zl3073x_i2c_driver);
+
+MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
+MODULE_DESCRIPTION("Microchip ZL3073x I2C driver");
+MODULE_IMPORT_NS("ZL3073X");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/zl3073x-spi.c b/drivers/mfd/zl3073x-spi.c
new file mode 100644
index 0000000000000..db976aef74917
--- /dev/null
+++ b/drivers/mfd/zl3073x-spi.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/mfd/zl3073x.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include "zl3073x.h"
+
+static int zl3073x_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct zl3073x_dev *zldev;
+
+	zldev = zl3073x_devm_alloc(dev);
+	if (!zldev)
+		return -ENOMEM;
+
+	zldev->dev = dev;
+	zldev->regmap = devm_regmap_init_spi(spi, zl3073x_get_regmap_config());
+	if (IS_ERR(zldev->regmap)) {
+		dev_err_probe(dev, PTR_ERR(zldev->regmap),
+			      "Failed to initialize register map\n");
+		return PTR_ERR(zldev->regmap);
+	}
+
+	spi_set_drvdata(spi, zldev);
+
+	return zl3073x_dev_init(zldev);
+}
+
+static const struct spi_device_id zl3073x_spi_id[] = {
+	{ "zl3073x-spi" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, zl3073x_spi_id);
+
+static const struct of_device_id zl3073x_spi_of_match[] = {
+	{ .compatible = "microchip,zl3073x-spi" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, zl3073x_spi_of_match);
+
+static struct spi_driver zl3073x_spi_driver = {
+	.driver = {
+		.name = "zl3073x-spi",
+		.of_match_table = zl3073x_spi_of_match,
+	},
+	.probe = zl3073x_spi_probe,
+	.id_table = zl3073x_spi_id,
+};
+module_spi_driver(zl3073x_spi_driver);
+
+MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
+MODULE_DESCRIPTION("Microchip ZL3073x SPI driver");
+MODULE_IMPORT_NS("ZL3073X");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/zl3073x.h b/drivers/mfd/zl3073x.h
new file mode 100644
index 0000000000000..8e8ffa961e4ca
--- /dev/null
+++ b/drivers/mfd/zl3073x.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ZL3073X_CORE_H
+#define __ZL3073X_CORE_H
+
+struct device;
+struct regmap_config;
+struct zl3073x_dev;
+
+struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
+int zl3073x_dev_init(struct zl3073x_dev *zldev);
+const struct regmap_config *zl3073x_get_regmap_config(void);
+
+#endif /* __ZL3073X_CORE_H */
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
new file mode 100644
index 0000000000000..f3f33ef8bfa18
--- /dev/null
+++ b/include/linux/mfd/zl3073x.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __LINUX_MFD_ZL3073X_H
+#define __LINUX_MFD_ZL3073X_H
+
+#include <linux/mutex.h>
+
+struct device;
+struct regmap;
+
+/**
+ * struct zl3073x_dev - zl3073x device
+ * @dev: pointer to device
+ * @regmap: regmap to access HW registers
+ * @lock: lock to be held during access to HW registers
+ */
+struct zl3073x_dev {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct mutex		lock;
+};
+
+#endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

Re: [PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Andy Shevchenko]

On Wed, Apr 09, 2025 at 04:42:39PM +0200, Ivan Vecera wrote:
> Add base MFD driver for Microchip Azurite ZL3073x chip family.
> These chips provide DPLL and PHC (PTP) functionality and they can
> be connected over I2C or SPI bus.
> 
> The MFD driver provide basic communication and synchronization
> over the bus and common functionality that are used by the DPLL
> driver (later in this series) and by the PTP driver (will be
> added later).
> 
> The chip family is characterized by following properties:
> * 2 separate DPLL units (channels)
> * 5 synthesizers
> * 10 input pins (references)
> * 10 outputs
> * 20 output pins (output pin pair shares one output)
> * Each reference and output can act in differential or single-ended
>   mode (reference or output in differential mode consumes 2 pins)
> * Each output is connected to one of the synthesizers
> * Each synthesizer is driven by one of the DPLL unit

...

> +/*
> + * Regmap ranges
> + */
> +#define ZL3073x_PAGE_SIZE	128
> +#define ZL3073x_NUM_PAGES	16
> +#define ZL3073x_PAGE_SEL	0x7F
> +
> +/*
> + * Regmap range configuration
> + *
> + * The device uses 7-bit addressing and has 16 register pages with
> + * range 0x00-0x7f. The register 0x7f in each page acts as page
> + * selector where bits 0-3 contains currently selected page.
> + */
> +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
> +	{
> +		.range_min	= 0,

This still has the same issue, you haven't given a chance to me to reply
in v1 thread. I'm not going to review this as it's not settled down yet.
Let's first discuss the questions you have in v1.

> +		.range_max	= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
> +		.selector_reg	= ZL3073x_PAGE_SEL,
> +		.selector_mask	= GENMASK(3, 0),
> +		.selector_shift	= 0,
> +		.window_start	= 0,
> +		.window_len	= ZL3073x_PAGE_SIZE,
> +	},
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Krzysztof Kozlowski]

On 09/04/2025 17:43, Andy Shevchenko wrote:
>> +/*
>> + * Regmap range configuration
>> + *
>> + * The device uses 7-bit addressing and has 16 register pages with
>> + * range 0x00-0x7f. The register 0x7f in each page acts as page
>> + * selector where bits 0-3 contains currently selected page.
>> + */
>> +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
>> +	{
>> +		.range_min	= 0,
> 
> This still has the same issue, you haven't given a chance to me to reply
> in v1 thread. I'm not going to review this as it's not settled down yet.
> Let's first discuss the questions you have in v1.
> 
I already started reviewing v2, so now we have simultaneous discussions
in v1 and v2...

Best regards,
Krzysztof

Re: [PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Ivan Vecera]

On 10. 04. 25 9:19 dop., Krzysztof Kozlowski wrote:
> On 09/04/2025 17:43, Andy Shevchenko wrote:
>>> +/*
>>> + * Regmap range configuration
>>> + *
>>> + * The device uses 7-bit addressing and has 16 register pages with
>>> + * range 0x00-0x7f. The register 0x7f in each page acts as page
>>> + * selector where bits 0-3 contains currently selected page.
>>> + */
>>> +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
>>> +	{
>>> +		.range_min	= 0,
>>
>> This still has the same issue, you haven't given a chance to me to reply
>> in v1 thread. I'm not going to review this as it's not settled down yet.
>> Let's first discuss the questions you have in v1.
>>

Sorry for that but I don't understand where the issue is... Many drivers 
uses this the same way.
E.g.
drivers/leds/leds-aw200xx.c
drivers/mfd/rsmu_i2c.c
sound/soc/codecs/tas2562.c
...and many others

All of them uses selector register that is present on all pages, wide 
range for register access <0, num_pages*window_size> and window <0, 
window_size>

Do they also do incorrectly or am I missing something?

I.
> I already started reviewing v2, so now we have simultaneous discussions
> in v1 and v2...
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Andrew Lunn]

On Thu, Apr 10, 2025 at 09:52:39AM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 9:19 dop., Krzysztof Kozlowski wrote:
> > On 09/04/2025 17:43, Andy Shevchenko wrote:
> > > > +/*
> > > > + * Regmap range configuration
> > > > + *
> > > > + * The device uses 7-bit addressing and has 16 register pages with
> > > > + * range 0x00-0x7f. The register 0x7f in each page acts as page
> > > > + * selector where bits 0-3 contains currently selected page.
> > > > + */
> > > > +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
> > > > +	{
> > > > +		.range_min	= 0,
> > > 
> > > This still has the same issue, you haven't given a chance to me to reply
> > > in v1 thread. I'm not going to review this as it's not settled down yet.
> > > Let's first discuss the questions you have in v1.
> > > 
> 
> Sorry for that but I don't understand where the issue is... Many drivers
> uses this the same way.
> E.g.
> drivers/leds/leds-aw200xx.c
> drivers/mfd/rsmu_i2c.c
> sound/soc/codecs/tas2562.c
> ...and many others
> 
> All of them uses selector register that is present on all pages, wide range
> for register access <0, num_pages*window_size> and window <0, window_size>
> 
> Do they also do incorrectly or am I missing something?

The bigger point is, you should of asked this as part of the
discussion on the previous version. You should not post a new version
until all discussion has come to an end, you understand all the
comments, or you have persuaded the commentor that the code is in fact
correct.

Posting more versions without having that discussion just wastes
reviewers/Maintainers time, and that is not what you want to do if you
want to get your patch merged.

	Andrew

Re: [PATCH v2 03/14] mfd: Add Microchip ZL3073x support

[Ivan Vecera]

On 10. 04. 25 7:50 odp., Andrew Lunn wrote:
> On Thu, Apr 10, 2025 at 09:52:39AM +0200, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 9:19 dop., Krzysztof Kozlowski wrote:
>>> On 09/04/2025 17:43, Andy Shevchenko wrote:
>>>>> +/*
>>>>> + * Regmap range configuration
>>>>> + *
>>>>> + * The device uses 7-bit addressing and has 16 register pages with
>>>>> + * range 0x00-0x7f. The register 0x7f in each page acts as page
>>>>> + * selector where bits 0-3 contains currently selected page.
>>>>> + */
>>>>> +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
>>>>> +	{
>>>>> +		.range_min	= 0,
>>>>
>>>> This still has the same issue, you haven't given a chance to me to reply
>>>> in v1 thread. I'm not going to review this as it's not settled down yet.
>>>> Let's first discuss the questions you have in v1.
>>>>
>>
>> Sorry for that but I don't understand where the issue is... Many drivers
>> uses this the same way.
>> E.g.
>> drivers/leds/leds-aw200xx.c
>> drivers/mfd/rsmu_i2c.c
>> sound/soc/codecs/tas2562.c
>> ...and many others
>>
>> All of them uses selector register that is present on all pages, wide range
>> for register access <0, num_pages*window_size> and window <0, window_size>
>>
>> Do they also do incorrectly or am I missing something?
> 
> The bigger point is, you should of asked this as part of the
> discussion on the previous version. You should not post a new version
> until all discussion has come to an end, you understand all the
> comments, or you have persuaded the commentor that the code is in fact
> correct.
> 
> Posting more versions without having that discussion just wastes
> reviewers/Maintainers time, and that is not what you want to do if you
> want to get your patch merged.
> 
> 	Andrew

Yeah, excuse me.

I'm very sorry for this impatience.

I.

[PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device

[Ivan Vecera]

Use devlink_alloc() to alloc zl3073x_dev structure and register
the device as a devlink device. Follow-up patches add support for
devlink device info reporting and devlink flash interface will
be later used for flashing firmware and configuration.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v1->v2:
- dependency on NET moved to MFD_ZL3073X_CORE in Kconfig
- devlink register managed way
---
 drivers/mfd/Kconfig        |  2 ++
 drivers/mfd/zl3073x-core.c | 35 +++++++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 30b36e3ee8f7f..3c54b9e2b8003 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2424,6 +2424,8 @@ config MFD_UPBOARD_FPGA
 
 config MFD_ZL3073X_CORE
 	tristate
+	depends on NET
+	select NET_DEVLINK
 	select MFD_CORE
 
 config MFD_ZL3073X_I2C
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index ccb6987d04a20..116c6dd9eebc7 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -8,6 +8,7 @@
 #include <linux/mfd/zl3073x.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <net/devlink.h>
 #include "zl3073x.h"
 
 /*
@@ -58,6 +59,14 @@ const struct regmap_config *zl3073x_get_regmap_config(void)
 }
 EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
 
+static const struct devlink_ops zl3073x_devlink_ops = {
+};
+
+static void zl3073x_devlink_free(void *ptr)
+{
+	devlink_free(ptr);
+}
+
 /**
  * zl3073x_devm_alloc - allocates zl3073x device structure
  * @dev: pointer to device structure
@@ -68,12 +77,25 @@ EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
  */
 struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev)
 {
-	struct zl3073x_dev *zldev;
+	struct devlink *devlink;
+
+	devlink = devlink_alloc(&zl3073x_devlink_ops,
+				sizeof(struct zl3073x_dev), dev);
+	if (!devlink)
+		return NULL;
+
+	if (devm_add_action_or_reset(dev, zl3073x_devlink_free, devlink))
+		return NULL;
 
-	return devm_kzalloc(dev, sizeof(*zldev), GFP_KERNEL);
+	return devlink_priv(devlink);
 }
 EXPORT_SYMBOL_NS_GPL(zl3073x_devm_alloc, "ZL3073X");
 
+static void zl3073x_devlink_unregister(void *ptr)
+{
+	devlink_unregister(ptr);
+}
+
 /**
  * zl3073x_dev_init - initialize zl3073x device
  * @zldev: pointer to zl3073x device
@@ -84,6 +106,7 @@ EXPORT_SYMBOL_NS_GPL(zl3073x_devm_alloc, "ZL3073X");
  */
 int zl3073x_dev_init(struct zl3073x_dev *zldev)
 {
+	struct devlink *devlink;
 	int rc;
 
 	rc = devm_mutex_init(zldev->dev, &zldev->lock);
@@ -92,6 +115,14 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
 		return rc;
 	}
 
+	devlink = priv_to_devlink(zldev);
+	devlink_register(devlink);
+
+	rc = devm_add_action_or_reset(zldev->dev, zl3073x_devlink_unregister,
+				      devlink);
+	if (rc)
+		return rc;
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(zl3073x_dev_init, "ZL3073X");
-- 
2.48.1

Re: [PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device

[kernel test robot]

Hi Ivan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.15-rc2 next-20250417]
[cannot apply to lee-mfd/for-mfd-next lee-mfd/for-mfd-fixes horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ivan-Vecera/dt-bindings-dpll-Add-device-tree-bindings-for-DPLL-device-and-pin/20250409-225519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250409144250.206590-5-ivecera%40redhat.com
patch subject: [PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device
config: nios2-kismet-CONFIG_MFD_ZL3073X_CORE-CONFIG_MFD_ZL3073X_I2C-0-0 (https://download.01.org/0day-ci/archive/20250419/202504192025.BlASOJSt-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250419/202504192025.BlASOJSt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504192025.BlASOJSt-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for MFD_ZL3073X_CORE when selected by MFD_ZL3073X_I2C
   WARNING: unmet direct dependencies detected for MFD_ZL3073X_CORE
     Depends on [n]: HAS_IOMEM [=y] && NET [=n]
     Selected by [y]:
     - MFD_ZL3073X_I2C [=y] && HAS_IOMEM [=y] && I2C [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device

[kernel test robot]

Hi Ivan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.15-rc2 next-20250417]
[cannot apply to lee-mfd/for-mfd-next lee-mfd/for-mfd-fixes horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ivan-Vecera/dt-bindings-dpll-Add-device-tree-bindings-for-DPLL-device-and-pin/20250409-225519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250409144250.206590-5-ivecera%40redhat.com
patch subject: [PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device
config: nios2-kismet-CONFIG_MFD_ZL3073X_CORE-CONFIG_MFD_ZL3073X_SPI-0-0 (https://download.01.org/0day-ci/archive/20250419/202504192124.BZN8TTbm-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250419/202504192124.BZN8TTbm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504192124.BZN8TTbm-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for MFD_ZL3073X_CORE when selected by MFD_ZL3073X_SPI
   WARNING: unmet direct dependencies detected for MFD_ZL3073X_CORE
     Depends on [n]: HAS_IOMEM [=y] && NET [=n]
     Selected by [y]:
     - MFD_ZL3073X_SPI [=y] && HAS_IOMEM [=y] && SPI [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 05/14] mfd: zl3073x: Add register access helpers

[Ivan Vecera]

Add helpers zl3073x_{read,write}_reg() to access device registers.
These functions have to be called with device lock that can be taken
by zl3073x_{lock,unlock}() or a caller can use defined guard.

Locking mechanism of regmap is not sufficient because sometimes is
necessary to perform several register operations at once. This is
especially a case of register mailboxes (more details in patch 7 & 8).
Disable regmap locking mechanism and use this device lock instead.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v1->v2:
* disabled regmap locking
---
 drivers/mfd/zl3073x-core.c  | 90 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/zl3073x.h | 33 ++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 116c6dd9eebc7..f0d85f77a7a76 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -5,9 +5,11 @@
 #include <linux/dev_printk.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/lockdep.h>
 #include <linux/mfd/zl3073x.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/unaligned.h>
 #include <net/devlink.h>
 #include "zl3073x.h"
 
@@ -46,6 +48,7 @@ const struct regmap_config zl3073x_regmap_config = {
 	.max_register		= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
 	.ranges			= zl3073x_regmap_ranges,
 	.num_ranges		= ARRAY_SIZE(zl3073x_regmap_ranges),
+	.disable_locking	= true,
 };
 
 /**
@@ -59,6 +62,93 @@ const struct regmap_config *zl3073x_get_regmap_config(void)
 }
 EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
 
+/**
+ * zl3073x_read_reg - Read value from device register
+ * @zldev: pointer to zl3073x device
+ * @reg: register to be read
+ * @len: number of bytes to read
+ * @value: pointer to place to store value read from the register
+ *
+ * Caller has to hold the device lock that can be obtained
+ * by zl3073x_lock().
+ *
+ * Return: 0 on success or <0 on error
+ */
+int zl3073x_read_reg(struct zl3073x_dev *zldev, unsigned int reg,
+		     unsigned int len, void *value)
+{
+	u8 buf[6];
+	int rc;
+
+	lockdep_assert_held(&zldev->lock);
+
+	rc = regmap_bulk_read(zldev->regmap, reg, buf, len);
+	if (rc)
+		return rc;
+
+	switch (len) {
+	case 1:
+		*(u8 *)value = buf[0];
+		break;
+	case 2:
+		*(u16 *)value = get_unaligned_be16(buf);
+		break;
+	case 4:
+		*(u32 *)value = get_unaligned_be32(buf);
+		break;
+	case 6:
+		*(u64 *)value = get_unaligned_be48(buf);
+		break;
+	default:
+		WARN(true, "Unsupported register size: %u\n", len);
+		break;
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(zl3073x_read_reg);
+
+/**
+ * zl3073x_write_reg - Write value to device register
+ * @zldev: pointer to zl3073x device
+ * @reg: register to be written
+ * @len: number of bytes to write
+ * @value: pointer to value to write to the register
+ *
+ * Caller has to hold the device lock that can be obtained
+ * by zl3073x_lock().
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
+		      unsigned int len, const void *value)
+{
+	u8 buf[6];
+
+	lockdep_assert_held(&zldev->lock);
+
+	switch (len) {
+	case 1:
+		buf[0] = *(u8 *)value;
+		break;
+	case 2:
+		put_unaligned_be16(*(u16 *)value, buf);
+		break;
+	case 4:
+		put_unaligned_be32(*(u32 *)value, buf);
+		break;
+	case 6:
+		put_unaligned_be48(*(u64 *)value, buf);
+		break;
+	default:
+		WARN(true, "Unsupported register size: %u\n", len);
+		break;
+	}
+
+	return regmap_bulk_write(zldev->regmap, reg, buf, len);
+}
+EXPORT_SYMBOL_GPL(zl3073x_write_reg);
+
 static const struct devlink_ops zl3073x_devlink_ops = {
 };
 
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index f3f33ef8bfa18..00dcc73aeeb34 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -3,6 +3,7 @@
 #ifndef __LINUX_MFD_ZL3073X_H
 #define __LINUX_MFD_ZL3073X_H
 
+#include <linux/cleanup.h>
 #include <linux/mutex.h>
 
 struct device;
@@ -20,4 +21,36 @@ struct zl3073x_dev {
 	struct mutex		lock;
 };
 
+/**
+ * zl3073x_lock - Lock the device
+ * @zldev: device structure pointer
+ *
+ * Caller has to take this lock when it needs to access device registers.
+ */
+static inline void zl3073x_lock(struct zl3073x_dev *zldev)
+{
+	mutex_lock(&zldev->lock);
+}
+
+/**
+ * zl3073x_unlock - Unlock the device
+ * @zldev: device structure pointer
+ *
+ * Caller unlocks the device when it does not need to access device
+ * registers anymore.
+ */
+static inline void zl3073x_unlock(struct zl3073x_dev *zldev)
+{
+	mutex_unlock(&zldev->lock);
+}
+
+DEFINE_GUARD(zl3073x, struct zl3073x_dev *, zl3073x_lock(_T),
+	     zl3073x_unlock(_T));
+
+int zl3073x_read_reg(struct zl3073x_dev *zldev, unsigned int reg,
+		     unsigned int len, void *value);
+
+int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
+		      unsigned int len, const void *value);
+
 #endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

[PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Ivan Vecera]

Add several macros to access device registers. These macros
defines a couple of static inline functions to ease an access
device registers. There are two types of registers, the 1st type
is a simple one that is defined by an address and size and the 2nd
type is indexed register that is defined by base address, type,
number of register instances and address stride between instances.

Examples:
__ZL3073X_REG_DEF(reg1, 0x1234, 4, u32);
__ZL3073X_REG_IDX_DEF(idx_reg2, 0x1234, 2, u16, 4, 0x10);

this defines the following functions:
int zl3073x_read_reg1(struct zl3073x_dev *dev, u32 *value);
int zl3073x_write_reg1(struct zl3073x_dev *dev, u32 value);
int zl3073x_read_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
                          u32 *value);
int zl3073x_write_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
                           u32 value);

There are also several shortcut macros to define registers with
certain bit widths: 8, 16, 32 and 48 bits.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 include/linux/mfd/zl3073x.h | 100 ++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 00dcc73aeeb34..405a66a7b3e78 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -3,6 +3,7 @@
 #ifndef __LINUX_MFD_ZL3073X_H
 #define __LINUX_MFD_ZL3073X_H
 
+#include <linux/bug.h>
 #include <linux/cleanup.h>
 #include <linux/mutex.h>
 
@@ -53,4 +54,103 @@ int zl3073x_read_reg(struct zl3073x_dev *zldev, unsigned int reg,
 int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
 		      unsigned int len, const void *value);
 
+/**
+ * __ZL3073X_REG_DEF - Define a device register helpers
+ * @_name: register name
+ * @_addr: register address
+ * @_len: size of register value in bytes
+ * @_type: type of register value
+ *
+ * The macro defines helper functions for particular device register
+ * to access it.
+ *
+ * Example:
+ * __ZL3073X_REG_DEF(sample_reg, 0x1234, 4, u32)
+ *
+ * generates static inline functions:
+ * int zl3073x_read_sample_reg(struct zl3073x_dev *dev, u32 *value);
+ * int zl3073x_write_sample_reg(struct zl3073x_dev *dev, u32 value);
+ *
+ * Note that these functions have to be called with the device lock
+ * taken.
+ */
+#define __ZL3073X_REG_DEF(_name, _addr, _len, _type)			\
+typedef _type zl3073x_##_name##_t;					\
+static inline __maybe_unused						\
+int zl3073x_read_##_name(struct zl3073x_dev *zldev, _type * value)	\
+{									\
+	return zl3073x_read_reg(zldev, _addr, _len, value);		\
+}									\
+static inline __maybe_unused						\
+int zl3073x_write_##_name(struct zl3073x_dev *zldev, _type value)	\
+{									\
+	return zl3073x_write_reg(zldev, _addr, _len, &value);		\
+}
+
+/**
+ * __ZL3073X_REG_IDX_DEF - Define an indexed device register helpers
+ * @_name: register name
+ * @_addr: register address
+ * @_len: size of register value in bytes
+ * @_type: type of register value
+ * @_num: number of register instances
+ * @_stride: address stride between instances
+ *
+ * The macro defines helper functions for particular indexed device
+ * register to access it.
+ *
+ * Example:
+ * __ZL3073X_REG_IDX_DEF(sample_reg, 0x1234, 2, u16, 4, 0x10)
+ *
+ * generates static inline functions:
+ * int zl3073x_read_sample_reg(struct zl3073x_dev *dev, unsigned int idx,
+ *			       u32 *value);
+ * int zl3073x_write_sample_reg(struct zl3073x_dev *dev, unsigned int idx,
+ *				u32 value);
+ *
+ * Note that these functions have to be called with the device lock
+ * taken.
+ */
+#define __ZL3073X_REG_IDX_DEF(_name, _addr, _len, _type, _num, _stride)	\
+typedef _type zl3073x_##_name##_t;					\
+static inline __maybe_unused						\
+int zl3073x_read_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
+			 _type * value)					\
+{									\
+	WARN_ON(idx >= (_num));						\
+	return zl3073x_read_reg(zldev, (_addr) + idx * (_stride), _len,	\
+				value);					\
+}									\
+static inline __maybe_unused						\
+int zl3073x_write_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
+			  _type value)					\
+{									\
+	WARN_ON(idx >= (_num));						\
+	return zl3073x_write_reg(zldev, (_addr) + idx * (_stride),	\
+				 _len, &value);				\
+}
+
+/*
+ * Add register definition shortcuts for 8, 16, 32 and 48 bits
+ */
+#define ZL3073X_REG8_DEF(_name, _addr)	__ZL3073X_REG_DEF(_name, _addr, 1, u8)
+#define ZL3073X_REG16_DEF(_name, _addr)	__ZL3073X_REG_DEF(_name, _addr, 2, u16)
+#define ZL3073X_REG32_DEF(_name, _addr)	__ZL3073X_REG_DEF(_name, _addr, 4, u32)
+#define ZL3073X_REG48_DEF(_name, _addr)	__ZL3073X_REG_DEF(_name, _addr, 6, u64)
+
+/*
+ * Add indexed register definition shortcuts for 8, 16, 32 and 48 bits
+ */
+#define ZL3073X_REG8_IDX_DEF(_name, _addr, _num, _stride)		\
+	__ZL3073X_REG_IDX_DEF(_name, _addr, 1, u8, _num, _stride)
+
+#define ZL3073X_REG16_IDX_DEF(_name, _addr, _num, _stride)		\
+	__ZL3073X_REG_IDX_DEF(_name, _addr, 2, u16, _num, _stride)
+
+#define ZL3073X_REG32_IDX_DEF(_name, _addr, _num, _stride)		\
+	__ZL3073X_REG_IDX_DEF(_name, _addr, 4, u32, _num, _stride)
+
+#define ZL3073X_REG48_IDX_DEF(_name, _addr, _num, _stride)		\
+	__ZL3073X_REG_IDX_DEF(_name, _addr, 6, u64, _num, _stride)
+
 #endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Krzysztof Kozlowski]

On 09/04/2025 16:42, Ivan Vecera wrote:
> Add several macros to access device registers. These macros
> defines a couple of static inline functions to ease an access
> device registers. There are two types of registers, the 1st type
> is a simple one that is defined by an address and size and the 2nd
> type is indexed register that is defined by base address, type,
> number of register instances and address stride between instances.
> 
> Examples:
> __ZL3073X_REG_DEF(reg1, 0x1234, 4, u32);
> __ZL3073X_REG_IDX_DEF(idx_reg2, 0x1234, 2, u16, 4, 0x10);

Why can't you use standard FIELD_ macros? Why inventing the wheel again?

> 
> this defines the following functions:
> int zl3073x_read_reg1(struct zl3073x_dev *dev, u32 *value);
> int zl3073x_write_reg1(struct zl3073x_dev *dev, u32 value);
> int zl3073x_read_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>                           u32 *value);
> int zl3073x_write_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>                            u32 value);

Do not copy code into commit msg. I asked about this last time. Explain
why do you need it, why existing API is not good.

> 
> There are also several shortcut macros to define registers with
> certain bit widths: 8, 16, 32 and 48 bits.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---


...

> + *
> + * Note that these functions have to be called with the device lock
> + * taken.
> + */
> +#define __ZL3073X_REG_IDX_DEF(_name, _addr, _len, _type, _num, _stride)	\
> +typedef _type zl3073x_##_name##_t;					\
> +static inline __maybe_unused						\
> +int zl3073x_read_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
> +			 _type * value)					\
> +{									\
> +	WARN_ON(idx >= (_num));						\

No need to cause panic reboots. Either review your code so this does not
happen or properly handle.

Best regards,
Krzysztof

Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Ivan Vecera]

On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote:
> On 09/04/2025 16:42, Ivan Vecera wrote:
>> Add several macros to access device registers. These macros
>> defines a couple of static inline functions to ease an access
>> device registers. There are two types of registers, the 1st type
>> is a simple one that is defined by an address and size and the 2nd
>> type is indexed register that is defined by base address, type,
>> number of register instances and address stride between instances.
>>
>> Examples:
>> __ZL3073X_REG_DEF(reg1, 0x1234, 4, u32);
>> __ZL3073X_REG_IDX_DEF(idx_reg2, 0x1234, 2, u16, 4, 0x10);
> 
> Why can't you use standard FIELD_ macros? Why inventing the wheel again?

This is not about FIELD_* macros replacement. This is an abstraction to 
access device registers in safe manner. Generated inline functions 
ensures that proper value or pointer to value type is passed by caller.
Also in case of arbitrary zl3073x_{read,write_{,idx}_reg() the does not 
need to know what is the register address.

If the caller just need to read regX or indexed regY it will call just

zl3073x_read_regX(..., &value);
zl3073x_read_regX(..., idx, &value);

instead of

zl3073x_read_reg(..., ZL3073x_REGX_ADDR, &value);
zl3073x_read_reg(..., ZL3073x_REGY_ADDR + (idx * ZL3073X_REGY_STRIDE), 
&value)

The 1st variant is additionally type safe, the caller is warned if it is 
passing u8 * instead of u32 *.

I tried to take similar approach the mlxsw driver took to access device 
registers.

If you are only against such macro usage for static inline functions 
generation, I can avoid them and pre-create them in separate include 
file like zl3073x_regs.h

>> this defines the following functions:
>> int zl3073x_read_reg1(struct zl3073x_dev *dev, u32 *value);
>> int zl3073x_write_reg1(struct zl3073x_dev *dev, u32 value);
>> int zl3073x_read_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>>                            u32 *value);
>> int zl3073x_write_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>>                             u32 value);
> 
> Do not copy code into commit msg. I asked about this last time. Explain
> why do you need it, why existing API is not good.

Will drop... I wanted only show how the macros work and what is their output

>>
>> There are also several shortcut macros to define registers with
>> certain bit widths: 8, 16, 32 and 48 bits.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
> 
> 
> ...
> 
>> + *
>> + * Note that these functions have to be called with the device lock
>> + * taken.
>> + */
>> +#define __ZL3073X_REG_IDX_DEF(_name, _addr, _len, _type, _num, _stride)	\
>> +typedef _type zl3073x_##_name##_t;					\
>> +static inline __maybe_unused						\
>> +int zl3073x_read_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
>> +			 _type * value)					\
>> +{									\
>> +	WARN_ON(idx >= (_num));						\
> 
> No need to cause panic reboots. Either review your code so this does not
> happen or properly handle.

Ack, will replace by

if (idx >= (_num))
	return -EINVAL

Thanks,
Ivan

Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Andy Shevchenko]

On Thu, Apr 10, 2025 at 11:21 AM Ivan Vecera <ivecera@redhat.com> wrote:
> On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote:
> > On 09/04/2025 16:42, Ivan Vecera wrote:

...

> >> +    WARN_ON(idx >= (_num));                                         \
> >
> > No need to cause panic reboots. Either review your code so this does not
> > happen or properly handle.
>
> Ack, will replace by
>
> if (idx >= (_num))
>         return -EINVAL

If these functions are called under regmap_read() / regmap_write() the
above is a dead code. Otherwise you need to configure regmap correctly
(in accordance with the HW registers layout and their abilities to be
written or read or reserved or special combinations).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Ivan Vecera]

On 10. 04. 25 7:53 odp., Andy Shevchenko wrote:
> On Thu, Apr 10, 2025 at 11:21 AM Ivan Vecera <ivecera@redhat.com> wrote:
>> On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote:
>>> On 09/04/2025 16:42, Ivan Vecera wrote:
> 
> ...
> 
>>>> +    WARN_ON(idx >= (_num));                                         \
>>>
>>> No need to cause panic reboots. Either review your code so this does not
>>> happen or properly handle.
>>
>> Ack, will replace by
>>
>> if (idx >= (_num))
>>          return -EINVAL
> 
> If these functions are called under regmap_read() / regmap_write() the
> above is a dead code. Otherwise you need to configure regmap correctly
> (in accordance with the HW registers layout and their abilities to be
> written or read or reserved or special combinations).
> 
Hi Andy,
these functions are not called under regmap_{read,write} but above. Some 
(non-mailboxed) registers are indexed.

E.g. register ref_freq is defined as 4-bytes for 10 input references:
ref0: address 0x144-0x147
ref1: address 0x148-0x14b
...

So the caller (this mfd driver or dpll driver) will access it as:
zl3073x_read_ref_freq(zldev, idx, &value);

and this helper then computes the addr value according base (0x144) and 
provided index with ref number and then calls regmap_*read().

And the 'if' check is just for a sanity check that the caller does not 
provide wrong index and if so return -EINVAL.

I.

[PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

Add register definitions for components versions and report them
during probe.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index f0d85f77a7a76..28f28d00da1cc 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/array_size.h>
+#include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/dev_printk.h>
 #include <linux/device.h>
 #include <linux/export.h>
@@ -13,6 +15,14 @@
 #include <net/devlink.h>
 #include "zl3073x.h"
 
+/*
+ * Register Map Page 0, General
+ */
+ZL3073X_REG16_DEF(id,			0x0001);
+ZL3073X_REG16_DEF(revision,		0x0003);
+ZL3073X_REG16_DEF(fw_ver,		0x0005);
+ZL3073X_REG32_DEF(custom_config_ver,	0x0007);
+
 /*
  * Regmap ranges
  */
@@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
  */
 int zl3073x_dev_init(struct zl3073x_dev *zldev)
 {
+	u16 id, revision, fw_ver;
 	struct devlink *devlink;
+	u32 cfg_ver;
 	int rc;
 
 	rc = devm_mutex_init(zldev->dev, &zldev->lock);
@@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
 		return rc;
 	}
 
+	/* Take device lock */
+	scoped_guard(zl3073x, zldev) {
+		rc = zl3073x_read_id(zldev, &id);
+		if (rc)
+			return rc;
+		rc = zl3073x_read_revision(zldev, &revision);
+		if (rc)
+			return rc;
+		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
+		if (rc)
+			return rc;
+		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
+		if (rc)
+			return rc;
+	}
+
+	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
+		 id, revision, fw_ver);
+	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
+		 FIELD_GET(GENMASK(31, 24), cfg_ver),
+		 FIELD_GET(GENMASK(23, 16), cfg_ver),
+		 FIELD_GET(GENMASK(15, 8), cfg_ver),
+		 FIELD_GET(GENMASK(7, 0), cfg_ver));
+
 	devlink = priv_to_devlink(zldev);
 	devlink_register(devlink);
 
-- 
2.48.1

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Krzysztof Kozlowski]

On 09/04/2025 16:42, Ivan Vecera wrote:
> Add register definitions for components versions and report them
> during probe.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
> index f0d85f77a7a76..28f28d00da1cc 100644
> --- a/drivers/mfd/zl3073x-core.c
> +++ b/drivers/mfd/zl3073x-core.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
>  #include <linux/array_size.h>
> +#include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/dev_printk.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> @@ -13,6 +15,14 @@
>  #include <net/devlink.h>
>  #include "zl3073x.h"
>  
> +/*
> + * Register Map Page 0, General
> + */
> +ZL3073X_REG16_DEF(id,			0x0001);
> +ZL3073X_REG16_DEF(revision,		0x0003);
> +ZL3073X_REG16_DEF(fw_ver,		0x0005);
> +ZL3073X_REG32_DEF(custom_config_ver,	0x0007);
> +
>  /*
>   * Regmap ranges
>   */
> @@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
>   */
>  int zl3073x_dev_init(struct zl3073x_dev *zldev)
>  {
> +	u16 id, revision, fw_ver;
>  	struct devlink *devlink;
> +	u32 cfg_ver;
>  	int rc;
>  
>  	rc = devm_mutex_init(zldev->dev, &zldev->lock);
> @@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
>  		return rc;
>  	}
>  
> +	/* Take device lock */

What is a device lock? Why do you need to comment standard guards/mutexes?

> +	scoped_guard(zl3073x, zldev) {
> +		rc = zl3073x_read_id(zldev, &id);
> +		if (rc)
> +			return rc;
> +		rc = zl3073x_read_revision(zldev, &revision);
> +		if (rc)
> +			return rc;
> +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> +		if (rc)
> +			return rc;
> +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> +		if (rc)
> +			return rc;
> +	}

Nothing improved here. Andrew comments are still valid and do not send
v3 before the discussion is resolved.

> +
> +	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
> +		 id, revision, fw_ver);
> +	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
> +		 FIELD_GET(GENMASK(31, 24), cfg_ver),
> +		 FIELD_GET(GENMASK(23, 16), cfg_ver),
> +		 FIELD_GET(GENMASK(15, 8), cfg_ver),
> +		 FIELD_GET(GENMASK(7, 0), cfg_ver));


Both should be dev_dbg. Your driver should be silent on success.

> +
>  	devlink = priv_to_devlink(zldev);
>  	devlink_register(devlink);
>  


Best regards,
Krzysztof

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 10. 04. 25 9:13 dop., Krzysztof Kozlowski wrote:
> On 09/04/2025 16:42, Ivan Vecera wrote:
>> Add register definitions for components versions and report them
>> during probe.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>>   drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
>> index f0d85f77a7a76..28f28d00da1cc 100644
>> --- a/drivers/mfd/zl3073x-core.c
>> +++ b/drivers/mfd/zl3073x-core.c
>> @@ -1,7 +1,9 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   
>>   #include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>>   #include <linux/bits.h>
>> +#include <linux/cleanup.h>
>>   #include <linux/dev_printk.h>
>>   #include <linux/device.h>
>>   #include <linux/export.h>
>> @@ -13,6 +15,14 @@
>>   #include <net/devlink.h>
>>   #include "zl3073x.h"
>>   
>> +/*
>> + * Register Map Page 0, General
>> + */
>> +ZL3073X_REG16_DEF(id,			0x0001);
>> +ZL3073X_REG16_DEF(revision,		0x0003);
>> +ZL3073X_REG16_DEF(fw_ver,		0x0005);
>> +ZL3073X_REG32_DEF(custom_config_ver,	0x0007);
>> +
>>   /*
>>    * Regmap ranges
>>    */
>> @@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
>>    */
>>   int zl3073x_dev_init(struct zl3073x_dev *zldev)
>>   {
>> +	u16 id, revision, fw_ver;
>>   	struct devlink *devlink;
>> +	u32 cfg_ver;
>>   	int rc;
>>   
>>   	rc = devm_mutex_init(zldev->dev, &zldev->lock);
>> @@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
>>   		return rc;
>>   	}
>>   
>> +	/* Take device lock */
> 
> What is a device lock? Why do you need to comment standard guards/mutexes?

Just to inform code reader, this is a section that accesses device 
registers that are protected by this zl3073x device lock.

>> +	scoped_guard(zl3073x, zldev) {
>> +		rc = zl3073x_read_id(zldev, &id);
>> +		if (rc)
>> +			return rc;
>> +		rc = zl3073x_read_revision(zldev, &revision);
>> +		if (rc)
>> +			return rc;
>> +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
>> +		if (rc)
>> +			return rc;
>> +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
>> +		if (rc)
>> +			return rc;
>> +	}
> 
> Nothing improved here. Andrew comments are still valid and do not send
> v3 before the discussion is resolved.

I'm accessing device registers here and they are protected by the device 
lock. I have to take the lock, register access functions expect this by 
lockdep_assert.

>> +
>> +	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
>> +		 id, revision, fw_ver);
>> +	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
>> +		 FIELD_GET(GENMASK(31, 24), cfg_ver),
>> +		 FIELD_GET(GENMASK(23, 16), cfg_ver),
>> +		 FIELD_GET(GENMASK(15, 8), cfg_ver),
>> +		 FIELD_GET(GENMASK(7, 0), cfg_ver));
> 
> 
> Both should be dev_dbg. Your driver should be silent on success.

+1
will change.

>> +
>>   	devlink = priv_to_devlink(zldev);
>>   	devlink_register(devlink);
>>   
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andrew Lunn]

> > > +	/* Take device lock */
> > 
> > What is a device lock? Why do you need to comment standard guards/mutexes?
> 
> Just to inform code reader, this is a section that accesses device registers
> that are protected by this zl3073x device lock.

I didn't see a reply to my question about the big picture. Why is the
regmap lock not sufficient. Why do you need a device lock.

> 
> > > +	scoped_guard(zl3073x, zldev) {
> > > +		rc = zl3073x_read_id(zldev, &id);
> > > +		if (rc)
> > > +			return rc;
> > > +		rc = zl3073x_read_revision(zldev, &revision);
> > > +		if (rc)
> > > +			return rc;
> > > +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> > > +		if (rc)
> > > +			return rc;
> > > +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> > > +		if (rc)
> > > +			return rc;
> > > +	}
> > 
> > Nothing improved here. Andrew comments are still valid and do not send
> > v3 before the discussion is resolved.
> 
> I'm accessing device registers here and they are protected by the device
> lock. I have to take the lock, register access functions expect this by
> lockdep_assert.

Because lockdep asserts is not a useful answer. Locks are there to
protect you from something. What are you protecting yourself from? If
you cannot answer that question, your locking scheme is built on sand
and very probably broken.

    Andrew

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 10. 04. 25 7:41 odp., Andrew Lunn wrote:
>>>> +	/* Take device lock */
>>>
>>> What is a device lock? Why do you need to comment standard guards/mutexes?
>>
>> Just to inform code reader, this is a section that accesses device registers
>> that are protected by this zl3073x device lock.
> 
> I didn't see a reply to my question about the big picture. Why is the
> regmap lock not sufficient. Why do you need a device lock.
> 
>>
>>>> +	scoped_guard(zl3073x, zldev) {
>>>> +		rc = zl3073x_read_id(zldev, &id);
>>>> +		if (rc)
>>>> +			return rc;
>>>> +		rc = zl3073x_read_revision(zldev, &revision);
>>>> +		if (rc)
>>>> +			return rc;
>>>> +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
>>>> +		if (rc)
>>>> +			return rc;
>>>> +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
>>>> +		if (rc)
>>>> +			return rc;
>>>> +	}
>>>
>>> Nothing improved here. Andrew comments are still valid and do not send
>>> v3 before the discussion is resolved.
>>
>> I'm accessing device registers here and they are protected by the device
>> lock. I have to take the lock, register access functions expect this by
>> lockdep_assert.
> 
> Because lockdep asserts is not a useful answer. Locks are there to
> protect you from something. What are you protecting yourself from? If
> you cannot answer that question, your locking scheme is built on sand
> and very probably broken.
> 
>      Andrew

Hi Andrew,
I have described the locking requirements under different message [v1 
05/28]. Could you please take a look?

Thanks,
Ivan

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andrew Lunn]

On Thu, Apr 10, 2025 at 08:44:43PM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 7:41 odp., Andrew Lunn wrote:
> > > > > +	/* Take device lock */
> > > > 
> > > > What is a device lock? Why do you need to comment standard guards/mutexes?
> > > 
> > > Just to inform code reader, this is a section that accesses device registers
> > > that are protected by this zl3073x device lock.
> > 
> > I didn't see a reply to my question about the big picture. Why is the
> > regmap lock not sufficient. Why do you need a device lock.
> > 
> > > 
> > > > > +	scoped_guard(zl3073x, zldev) {
> > > > > +		rc = zl3073x_read_id(zldev, &id);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_revision(zldev, &revision);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +	}
> > > > 
> > > > Nothing improved here. Andrew comments are still valid and do not send
> > > > v3 before the discussion is resolved.
> > > 
> > > I'm accessing device registers here and they are protected by the device
> > > lock. I have to take the lock, register access functions expect this by
> > > lockdep_assert.
> > 
> > Because lockdep asserts is not a useful answer. Locks are there to
> > protect you from something. What are you protecting yourself from? If
> > you cannot answer that question, your locking scheme is built on sand
> > and very probably broken.
> > 
> >      Andrew
> 
> Hi Andrew,
> I have described the locking requirements under different message [v1
> 05/28]. Could you please take a look?

So a small number of registers in the regmap need special locking. It
was not clear to me what exactly those locking requirements are,
because they don't appear to be described.

But when i look at the code above, the scoped guard gives the
impression that i have to read id, revision, fw_vr and cfg_ver all in
one go without any other reads/writes happening. I strongly suspect
that impression is wrong. The question then becomes, how can i tell
apart reads/writes which do need to be made as one group, form others
which can be arbitrarily ordered with other read/writes.

What i suggest you do is try to work out how to push the locking down
as low as possible. Make the lock cover only what it needs to cover.

Probably for 95% of the registers, the regmap lock is sufficient.

Just throwing out ideas, i've no idea if they are good or not. Create
two regmaps onto your i2c device, covering different register
ranges. The 'normal' one uses standard regmap locking, the second
'special' one has locking disabled. You additionally provide your own
lock functions to the 'normal' one, so you have access to the
lock. When you need to access the mailboxes, take the lock, so you
know the 'normal' regmap cannot access anything, and then use the
'special' regmap to do what you need to do. A structure like this
should help explain what the special steps are for those special
registers, while not scattering wrong ideas about what the locking
scheme actually is all over the code.

	Andrew

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 10. 04. 25 11:54 odp., Andrew Lunn wrote:
> ...
> 
> So a small number of registers in the regmap need special locking. It
> was not clear to me what exactly those locking requirements are,
> because they don't appear to be described.
> 
> But when i look at the code above, the scoped guard gives the
> impression that i have to read id, revision, fw_vr and cfg_ver all in
> one go without any other reads/writes happening. I strongly suspect
> that impression is wrong. The question then becomes, how can i tell
> apart reads/writes which do need to be made as one group, form others
> which can be arbitrarily ordered with other read/writes.
> 
> What i suggest you do is try to work out how to push the locking down
> as low as possible. Make the lock cover only what it needs to cover.
> 
> Probably for 95% of the registers, the regmap lock is sufficient.
> 
> Just throwing out ideas, i've no idea if they are good or not. Create
> two regmaps onto your i2c device, covering different register
> ranges. The 'normal' one uses standard regmap locking, the second
> 'special' one has locking disabled. You additionally provide your own
> lock functions to the 'normal' one, so you have access to the
> lock. When you need to access the mailboxes, take the lock, so you
> know the 'normal' regmap cannot access anything, and then use the
> 'special' regmap to do what you need to do. A structure like this
> should help explain what the special steps are for those special
> registers, while not scattering wrong ideas about what the locking
> scheme actually is all over the code.

Hi Andrew,
the idea looks interesting but there are some caveats and disadvantages.
I thought about it but the idea with two regmaps (one for simple 
registers and one for mailboxes) where the simple one uses implicit 
locking and mailbox one has locking disabled with explicit locking 
requirement. There are two main problems:

1) Regmap cache has to be disabled as it cannot be shared between 
multiple regmaps... so also page selector cannot be cached.

2) You cannot mix access to mailbox registers and to simple registers. 
This means that mailbox accesses have to be wrapped e.g. inside 
scoped_guard()

The first problem is really pain as I would like to extend later the 
driver with proper caching (page selector for now).
The second one brings only confusions for a developer how to properly 
access different types of registers.

I think the best approach would be to use just single regmap for all 
registers with implicit locking enabled and have extra mailbox mutex to 
protect mailbox registers and ensure atomic operations with them.
This will allow to use regmap cache and also intermixing mailbox and 
simple registers' accesses won't be an issue.

@Andy Shevchenko, wdym about it?

Thanks,
Ivan

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andy Shevchenko]

On Tue, Apr 15, 2025 at 12:01:43PM +0200, Ivan Vecera wrote:
> On 10. 04. 25 11:54 odp., Andrew Lunn wrote:

...

> > So a small number of registers in the regmap need special locking. It
> > was not clear to me what exactly those locking requirements are,
> > because they don't appear to be described.
> > 
> > But when i look at the code above, the scoped guard gives the
> > impression that i have to read id, revision, fw_vr and cfg_ver all in
> > one go without any other reads/writes happening. I strongly suspect
> > that impression is wrong. The question then becomes, how can i tell
> > apart reads/writes which do need to be made as one group, form others
> > which can be arbitrarily ordered with other read/writes.
> > 
> > What i suggest you do is try to work out how to push the locking down
> > as low as possible. Make the lock cover only what it needs to cover.
> > 
> > Probably for 95% of the registers, the regmap lock is sufficient.
> > 
> > Just throwing out ideas, i've no idea if they are good or not. Create
> > two regmaps onto your i2c device, covering different register
> > ranges. The 'normal' one uses standard regmap locking, the second
> > 'special' one has locking disabled. You additionally provide your own
> > lock functions to the 'normal' one, so you have access to the
> > lock. When you need to access the mailboxes, take the lock, so you
> > know the 'normal' regmap cannot access anything, and then use the
> > 'special' regmap to do what you need to do. A structure like this
> > should help explain what the special steps are for those special
> > registers, while not scattering wrong ideas about what the locking
> > scheme actually is all over the code.
> 
> Hi Andrew,
> the idea looks interesting but there are some caveats and disadvantages.
> I thought about it but the idea with two regmaps (one for simple registers
> and one for mailboxes) where the simple one uses implicit locking and
> mailbox one has locking disabled with explicit locking requirement. There
> are two main problems:
> 
> 1) Regmap cache has to be disabled as it cannot be shared between multiple
> regmaps... so also page selector cannot be cached.
> 
> 2) You cannot mix access to mailbox registers and to simple registers. This
> means that mailbox accesses have to be wrapped e.g. inside scoped_guard()
> 
> The first problem is really pain as I would like to extend later the driver
> with proper caching (page selector for now).
> The second one brings only confusions for a developer how to properly access
> different types of registers.
> 
> I think the best approach would be to use just single regmap for all
> registers with implicit locking enabled and have extra mailbox mutex to
> protect mailbox registers and ensure atomic operations with them.
> This will allow to use regmap cache and also intermixing mailbox and simple
> registers' accesses won't be an issue.
> 
> @Andy Shevchenko, wdym about it?

Sounds like a good plan to me, but I'm not in the exact area of this driver's
interest, so others may have better suggestions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andrew Lunn]

> Hi Andrew,
> the idea looks interesting but there are some caveats and disadvantages.
> I thought about it but the idea with two regmaps (one for simple registers
> and one for mailboxes) where the simple one uses implicit locking and
> mailbox one has locking disabled with explicit locking requirement. There
> are two main problems:
> 
> 1) Regmap cache has to be disabled as it cannot be shared between multiple
> regmaps... so also page selector cannot be cached.
> 
> 2) You cannot mix access to mailbox registers and to simple registers. This
> means that mailbox accesses have to be wrapped e.g. inside scoped_guard()
> 
> The first problem is really pain as I would like to extend later the driver
> with proper caching (page selector for now).
> The second one brings only confusions for a developer how to properly access
> different types of registers.
> 
> I think the best approach would be to use just single regmap for all
> registers with implicit locking enabled and have extra mailbox mutex to
> protect mailbox registers and ensure atomic operations with them.
> This will allow to use regmap cache and also intermixing mailbox and simple
> registers' accesses won't be an issue.

As i said, it was just an idea, i had no idea if it was a good idea.

What is important is that the scope of the locking becomes clear,
unlike what the first version had. So locking has to be pushed down to
the lower levels so you lock a single register access, or you lock an
mailbox access.

Also, you say this is an MFD partially because GPIOs could be added
later. I assume that GPIO code would have the same locking issue,
which suggests the locking should be in the MFD core, not the
individual drivers stacked on top of it.

	Andrew

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 15. 04. 25 2:57 odp., Andrew Lunn wrote:
>> Hi Andrew,
>> the idea looks interesting but there are some caveats and disadvantages.
>> I thought about it but the idea with two regmaps (one for simple registers
>> and one for mailboxes) where the simple one uses implicit locking and
>> mailbox one has locking disabled with explicit locking requirement. There
>> are two main problems:
>>
>> 1) Regmap cache has to be disabled as it cannot be shared between multiple
>> regmaps... so also page selector cannot be cached.
>>
>> 2) You cannot mix access to mailbox registers and to simple registers. This
>> means that mailbox accesses have to be wrapped e.g. inside scoped_guard()
>>
>> The first problem is really pain as I would like to extend later the driver
>> with proper caching (page selector for now).
>> The second one brings only confusions for a developer how to properly access
>> different types of registers.
>>
>> I think the best approach would be to use just single regmap for all
>> registers with implicit locking enabled and have extra mailbox mutex to
>> protect mailbox registers and ensure atomic operations with them.
>> This will allow to use regmap cache and also intermixing mailbox and simple
>> registers' accesses won't be an issue.
> 
> As i said, it was just an idea, i had no idea if it was a good idea.
> 
> What is important is that the scope of the locking becomes clear,
> unlike what the first version had. So locking has to be pushed down to
> the lower levels so you lock a single register access, or you lock an
> mailbox access.
> 
> Also, you say this is an MFD partially because GPIOs could be added
> later. I assume that GPIO code would have the same locking issue,
> which suggests the locking should be in the MFD core, not the
> individual drivers stacked on top of it.

To work with GPIO block inside chip you use just simple registers not 
mailboxes. But it does not matter. The approach above exposes for 
individual sub-drivers an API (not direct usage of regmap (all registers 
are exposed by function helpers), helpers to enter/leave "mailbox mode" 
that locks/unlocks mailbox mutex)...

Something like this
{
	...
	rc = zl3073x_read_id(zldev, &id); // locked implicitly
	...
	scoped_guard(zl3073x_mailbox)(zldev) { // enter mailbox 'mode'
		rc = zl3073x_mb_ref_set(zldev, ref_idx);
		rc = zl3073x_mb_ref_op(zldev, REF_OP_READ);
		regmap_wait_poll_timeout(...);
		rc = zl3073x_mb_ref_freq(zldev, &freq);
	} // leave mailbox access 'mode'
	...
	rc = zl3073x_write_blahblah(zldev, value);
}

Thanks,
Ivan

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andy Shevchenko]

On Wed, Apr 9, 2025 at 5:43 PM Ivan Vecera <ivecera@redhat.com> wrote:
>
> Add register definitions for components versions and report them
> during probe.

JFYI: disabling regmap lock (independently of having an additional one
or not) is not recommended. With that you actually disable the useful
debugging feature of regmap, your device will not be present in the
(regmap) debugfs after that.

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 10. 04. 25 7:50 odp., Andy Shevchenko wrote:
> On Wed, Apr 9, 2025 at 5:43 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>
>> Add register definitions for components versions and report them
>> during probe.
> 
> JFYI: disabling regmap lock (independently of having an additional one
> or not) is not recommended. With that you actually disable the useful
> debugging feature of regmap, your device will not be present in the
> (regmap) debugfs after that.
> 

I will follow Andrew's recommendation:

1st regmap for direct registers (pages 0-9) with config like:

regmap_config {
	...
	.lock = mutex_lock,
	.unlock = mutex_unlock,
	.lock_arg = &zl3073x_dev->lock
	...
};

2nd regmap for indirect registers (mailboxes) (pages 10-15) with 
disabled locking:

regmap_config {
	...
	.disable_lock = true,
	...
};

For direct registers the lock will be handled automatically by regmap 1.
For indirect registers the lock will be managed explicitly by the driver 
to ensure atomic access to mailbox.

The range for regmap 1: (registers 0x000-0x4FF)
regmap_range_cfg {
	.range_min = 0,
	.range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
	.selector_reg = 0x7f,      /* page selector at each page */
	.selector_shift = 0,       /* no shift in page selector */
	.selector_mask = GENMASK(3, 0),	/* 4 bits for page sel */
	.window_start = 0,         /* 128 regs from 0x00-0x7f */
	.window_len = 128,
};

The range for regmap 2: (registers 0x500-0x77F)
regmap_range_cfg {
	.range_min = 10 * 128,
	.range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
	.selector_reg = 0x7f,      /* page selector at each page */
	.selector_shift = 0,       /* no shift in page selector */
	.selector_mask = GENMASK(3, 0),	/* 4 bits for page sel */
	.window_start = 0,         /* 128 regs from 0x00-0x7f */
	.window_len = 128,
};

Is it now OK?

Thanks,
Ivan

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andrew Lunn]

> 2nd regmap for indirect registers (mailboxes) (pages 10-15) with disabled
> locking:
> 
> regmap_config {
> 	...
> 	.disable_lock = true,
> 	...
> };

Do all registers in pages 10-15 need special locking? Or just a
subset?

	Andrew

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 11. 04. 25 2:31 odp., Andrew Lunn wrote:
>> 2nd regmap for indirect registers (mailboxes) (pages 10-15) with disabled
>> locking:
>>
>> regmap_config {
>> 	...
>> 	.disable_lock = true,
>> 	...
>> };
> 
> Do all registers in pages 10-15 need special locking? Or just a
> subset?
> 
> 	Andrew
> 
All of them... 1 page (>=10) == 1 mailbox.

I.

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera]

On 11. 04. 25 1:19 odp., Ivan Vecera wrote:
> The range for regmap 1: (registers 0x000-0x4FF)
> regmap_range_cfg {
>      .range_min = 0,
>      .range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
>      .selector_reg = 0x7f,      /* page selector at each page */
>      .selector_shift = 0,       /* no shift in page selector */
>      .selector_mask = GENMASK(3, 0),    /* 4 bits for page sel */
>      .window_start = 0,         /* 128 regs from 0x00-0x7f */
>      .window_len = 128,
> };
> 
> The range for regmap 2: (registers 0x500-0x77F)
> regmap_range_cfg {
>      .range_min = 10 * 128,
>      .range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
>      .selector_reg = 0x7f,      /* page selector at each page */
>      .selector_shift = 0,       /* no shift in page selector */
>      .selector_mask = GENMASK(3, 0),    /* 4 bits for page sel */
>      .window_start = 0,         /* 128 regs from 0x00-0x7f */
>      .window_len = 128,
> };
> 
> Is it now OK?

No this is not good... I cannot use 2 ranges.

This is not safe... if the caller use regmap 2 to read/write something 
below 0x500 (by mistake), no mapping is applied and value is directly 
used as register number that's wrong :-(.

Should I use rather single mapping range to cover all pages and ensure 
at driver level that regmap 2 is not used for regs < 0x500?
-or-
what would be the best approach?

I.

Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Andrew Lunn]

61;8000;1cOn Fri, Apr 11, 2025 at 03:17:14PM +0200, Ivan Vecera wrote:
> On 11. 04. 25 1:19 odp., Ivan Vecera wrote:
> > The range for regmap 1: (registers 0x000-0x4FF)
> > regmap_range_cfg {
> >      .range_min = 0,
> >      .range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
> >      .selector_reg = 0x7f,      /* page selector at each page */
> >      .selector_shift = 0,       /* no shift in page selector */
> >      .selector_mask = GENMASK(3, 0),    /* 4 bits for page sel */
> >      .window_start = 0,         /* 128 regs from 0x00-0x7f */
> >      .window_len = 128,
> > };
> > 
> > The range for regmap 2: (registers 0x500-0x77F)
> > regmap_range_cfg {
> >      .range_min = 10 * 128,
> >      .range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
> >      .selector_reg = 0x7f,      /* page selector at each page */
> >      .selector_shift = 0,       /* no shift in page selector */
> >      .selector_mask = GENMASK(3, 0),    /* 4 bits for page sel */
> >      .window_start = 0,         /* 128 regs from 0x00-0x7f */
> >      .window_len = 128,
> > };
> > 
> > Is it now OK?
> 
> No this is not good... I cannot use 2 ranges.
> 
> This is not safe... if the caller use regmap 2 to read/write something below
> 0x500 (by mistake), no mapping is applied and value is directly used as
> register number that's wrong :-(.
> 
> Should I use rather single mapping range to cover all pages and ensure at
> driver level that regmap 2 is not used for regs < 0x500?

I don't know regmap too well, but cannot your mailbox regmap have a
reg_base of 10 * 128. Going blow that would then require a negative
reg, but they are unsigned int.

One of that things the core MFD driver is about is giving you safe
access to shared registers on some sort of bus. So it could well be
your MFD exports an higher level API for mailboxs, a mailbox read and
mailbox write, etc. The regmap below it is not exposed outside of the
MFD core. And the MFD core does all the locking.

	Andrew

[PATCH v2 08/14] mfd: zl3073x: Implement devlink device info

[Ivan Vecera]

Provide ASIC ID, ASIC revision, firmware version and optionally
custom config version through devlink device info.

Sample output:
 # devlink dev
 i2c/1-0070
 # devlink dev info
 i2c/1-0070:
   driver zl3073x-i2c
   versions:
       fixed:
         asic.id 1E94
         asic.rev 300
         fw 7006

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/mfd/zl3073x-core.c | 75 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 28f28d00da1cc..79cf2ddbca228 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -10,7 +10,9 @@
 #include <linux/lockdep.h>
 #include <linux/mfd/zl3073x.h>
 #include <linux/module.h>
+#include <linux/netlink.h>
 #include <linux/regmap.h>
+#include <linux/sprintf.h>
 #include <linux/unaligned.h>
 #include <net/devlink.h>
 #include "zl3073x.h"
@@ -159,7 +161,80 @@ int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
 }
 EXPORT_SYMBOL_GPL(zl3073x_write_reg);
 
+/**
+ * zl3073x_devlink_info_get - Devlink device info callback
+ * @devlink: devlink structure pointer
+ * @req: devlink request pointer to store information
+ * @extack: netlink extack pointer to report errors
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int zl3073x_devlink_info_get(struct devlink *devlink,
+				    struct devlink_info_req *req,
+				    struct netlink_ext_ack *extack)
+{
+	struct zl3073x_dev *zldev = devlink_priv(devlink);
+	u16 id, revision, fw_ver;
+	char buf[16];
+	u32 cfg_ver;
+	int rc;
+
+	guard(zl3073x)(zldev);
+
+	rc = zl3073x_read_id(zldev, &id);
+	if (rc)
+		return rc;
+
+	snprintf(buf, sizeof(buf), "%X", id);
+	rc = devlink_info_version_fixed_put(req,
+					    DEVLINK_INFO_VERSION_GENERIC_ASIC_ID,
+					    buf);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_revision(zldev, &revision);
+	if (rc)
+		return rc;
+
+	snprintf(buf, sizeof(buf), "%X", revision);
+	rc = devlink_info_version_fixed_put(req,
+					    DEVLINK_INFO_VERSION_GENERIC_ASIC_REV,
+					    buf);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_fw_ver(zldev, &fw_ver);
+	if (rc)
+		return rc;
+
+	snprintf(buf, sizeof(buf), "%u", fw_ver);
+	rc = devlink_info_version_fixed_put(req,
+					    DEVLINK_INFO_VERSION_GENERIC_FW,
+					    buf);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
+	if (rc)
+		return rc;
+
+	/* No custom config version */
+	if (cfg_ver == U32_MAX)
+		return rc;
+
+	snprintf(buf, sizeof(buf), "%lu.%lu.%lu.%lu",
+		 FIELD_GET(GENMASK(31, 24), cfg_ver),
+		 FIELD_GET(GENMASK(23, 16), cfg_ver),
+		 FIELD_GET(GENMASK(15, 8), cfg_ver),
+		 FIELD_GET(GENMASK(7, 0), cfg_ver));
+
+	rc = devlink_info_version_running_put(req, "cfg.custom_ver", buf);
+
+	return rc;
+}
+
 static const struct devlink_ops zl3073x_devlink_ops = {
+	.info_get = zl3073x_devlink_info_get,
 };
 
 static void zl3073x_devlink_free(void *ptr)
-- 
2.48.1

[PATCH v2 09/14] mfd: zl3073x: Add macro to wait for register value bits to be cleared

[Ivan Vecera]

Sometimes in communication with the device is necessary to set
certain bit(s) in certain register and then the driver has to
wait until these bits are cleared by the device.

Add the macro for this functionality, it will be used by later
commits.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v1->v2:
* fixed macro documentation
---
 include/linux/mfd/zl3073x.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 405a66a7b3e78..7ccb796f7b9aa 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -5,6 +5,8 @@
 
 #include <linux/bug.h>
 #include <linux/cleanup.h>
+#include <linux/errno.h>
+#include <linux/iopoll.h>
 #include <linux/mutex.h>
 
 struct device;
@@ -153,4 +155,34 @@ int zl3073x_write_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
 #define ZL3073X_REG48_IDX_DEF(_name, _addr, _num, _stride)		\
 	__ZL3073X_REG_IDX_DEF(_name, _addr, 6, u64, _num, _stride)
 
+#define READ_SLEEP_US	10
+#define READ_TIMEOUT_US	2000000
+
+/**
+ * zl3073x_wait_clear_bits - wait for specific bits to be cleared
+ * @_zldev: pointer to zl3073x device
+ * @_reg: register name
+ * @_bits: bits that should be cleared
+ * @_index: optional index for indexed register
+ *
+ * The macro waits up to @READ_TIMEOUT_US microseconds for @_bits in @_reg
+ * to be cleared.
+ *
+ * Return:
+ * -ETIMEDOUT: if timeout occurred
+ *         <0: for other errors occurred during communication
+ *          0: success
+ */
+#define zl3073x_wait_clear_bits(_zldev, _reg, _bits, _index...)		\
+({									\
+	zl3073x_##_reg##_t __val;					\
+	int __rc;							\
+	if (read_poll_timeout(zl3073x_read_##_reg, __rc,		\
+			      __rc || !((_bits) & __val),		\
+			      READ_SLEEP_US, READ_TIMEOUT_US, false,	\
+			      _zldev, ##_index, &__val))		\
+		__rc = -ETIMEDOUT;					\
+	__rc;								\
+})
+
 #endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

[PATCH v2 10/14] mfd: zl3073x: Add functions to work with register mailboxes

[Ivan Vecera]

Registers present in page 10 and higher are organized in so-called
register mailboxes. These mailboxes are used to read and write
configuration of particular object (dpll, output, reference & synth).

Each of these register pages contains mask register that is used by
the driver to indicate an index of requested object to work with and
also semaphore register to indicate what operation is requested.
The rest of registers in the particular register page are latch
registers that are filled by the firmware during read operation or by
the driver prior write operation.

For read operation the driver...
1) ... updates the mailbox mask register with index of particular object
2) ... sets the mailbox semaphore register read bit
3) ... waits for the semaphore register read bit to be cleared by FW
4) ... reads the configuration from latch registers

For write operation the driver...
1) ... writes the requested configuration to latch registers
2) ... sets the mailbox mask register for the DPLL to be updated
3) ... sets the mailbox semaphore register bit for the write operation
4) ... waits for the semaphore register bit to be cleared by FW

Add functions to read and write mailboxes for all supported object types.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/mfd/zl3073x-core.c  | 193 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/zl3073x.h |  12 +++
 2 files changed, 205 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 79cf2ddbca228..f606c51c90fdf 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -25,6 +25,47 @@ ZL3073X_REG16_DEF(revision,		0x0003);
 ZL3073X_REG16_DEF(fw_ver,		0x0005);
 ZL3073X_REG32_DEF(custom_config_ver,	0x0007);
 
+/*
+ * Register Map Page 10, Ref Mailbox
+ */
+ZL3073X_REG16_DEF(ref_mb_mask,			0x502);
+#define REF_MB_MASK				GENMASK(9, 0)
+
+ZL3073X_REG8_DEF(ref_mb_sem,			0x504);
+#define REF_MB_SEM_WR				BIT(0)
+#define REF_MB_SEM_RD				BIT(1)
+
+/*
+ * Register Map Page 12, DPLL Mailbox
+ */
+ZL3073X_REG16_DEF(dpll_mb_mask,			0x602);
+
+ZL3073X_REG8_DEF(dpll_mb_sem,			0x604);
+#define DPLL_MB_SEM_WR				BIT(0)
+#define DPLL_MB_SEM_RD				BIT(1)
+
+/*
+ * Register Map Page 13, Synth Mailbox
+ */
+ZL3073X_REG16_DEF(synth_mb_mask,		0x682);
+
+ZL3073X_REG8_DEF(synth_mb_sem,			0x684);
+#define SYNTH_MB_SEM_WR				BIT(0)
+#define SYNTH_MB_SEM_RD				BIT(1)
+
+ZL3073X_REG16_DEF(synth_freq_base,		0x686);
+ZL3073X_REG32_DEF(synth_freq_mult,		0x688);
+ZL3073X_REG16_DEF(synth_freq_m,			0x68c);
+ZL3073X_REG16_DEF(synth_freq_n,			0x68e);
+
+/*
+ * Register Map Page 14, Output Mailbox
+ */
+ZL3073X_REG16_DEF(output_mb_mask,		0x702);
+ZL3073X_REG8_DEF(output_mb_sem,			0x704);
+#define OUTPUT_MB_SEM_WR			BIT(0)
+#define OUTPUT_MB_SEM_RD			BIT(1)
+
 /*
  * Regmap ranges
  */
@@ -161,6 +202,158 @@ int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
 }
 EXPORT_SYMBOL_GPL(zl3073x_write_reg);
 
+/**
+ * ZL3073X_MB_OP - perform an operation on mailbox of certain type
+ * @_zldev: pointer to device structure
+ * @_type: type of mailbox (dpll, ref or output)
+ * @_index: object index of given type
+ * @_op: operation to perform
+ *
+ * Performs the requested operation on mailbox of certain type and
+ * returns 0 in case of success or negative value otherwise.
+ */
+#define ZL3073X_MB_OP(_zldev, _type, _index, _op)			\
+({									\
+	struct zl3073x_dev *__zldev = (_zldev);				\
+	u16 __mask = BIT(_index);					\
+	u8 __op = (_op);						\
+	int __rc;							\
+	do {								\
+		/* Select requested index in mask register */		\
+		__rc = zl3073x_write_##_type##_mb_mask(__zldev, __mask);\
+		if (__rc)						\
+			break;						\
+		/* Select requested command */				\
+		__rc = zl3073x_write_##_type##_mb_sem(__zldev, __op);	\
+		if (__rc)						\
+			break;						\
+		/* Wait for the command to actually finish */		\
+		__rc = zl3073x_wait_clear_bits(__zldev, _type##_mb_sem,	\
+					       __op);			\
+	} while (0);							\
+	__rc;								\
+})
+
+/**
+ * zl3073x_mb_dpll_read - read given DPLL configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: DPLL index
+ *
+ * Reads configuration of given DPLL into DPLL mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, dpll, index, DPLL_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_dpll_read);
+
+/**
+ * zl3073x_mb_dpll_write - write given DPLL configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: DPLL index
+ *
+ * Writes (commits) configuration of given DPLL from DPLL mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, dpll, index, DPLL_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_dpll_write);
+
+/**
+ * zl3073x_mb_output_read - read given output configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: output index
+ *
+ * Reads configuration of given output into output mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, output, index, OUTPUT_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_output_read);
+
+/**
+ * zl3073x_mb_output_write - write given output configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: DPLL index
+ *
+ * Writes (commits) configuration of given output from output mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, output, index, OUTPUT_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_output_write);
+
+/**
+ * zl3073x_mb_ref_read - read given reference configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: reference index
+ *
+ * Reads configuration of given reference into reference mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, ref, index, REF_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_ref_read);
+
+/**
+ * zl3073x_mb_ref_write - write given reference configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: reference index
+ *
+ * Writes (commits) configuration of given reference from reference mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, ref, index, REF_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_ref_write);
+
+/**
+ * zl3073x_mb_synth_read - read given synth configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: synth index
+ *
+ * Reads configuration of given synth into synth mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, synth, index, SYNTH_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_synth_read);
+
+/**
+ * zl3073x_mb_synth_write - write given synth configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: synth index
+ *
+ * Writes (commits) configuration of given synth from reference mailbox.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index)
+{
+	return ZL3073X_MB_OP(zldev, synth, index, SYNTH_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_synth_write);
+
 /**
  * zl3073x_devlink_info_get - Devlink device info callback
  * @devlink: devlink structure pointer
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 7ccb796f7b9aa..cc80014ebb384 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -185,4 +185,16 @@ int zl3073x_write_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
 	__rc;								\
 })
 
+/*
+ * Mailbox operations
+ */
+int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
+
 #endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

[PATCH v2 11/14] mfd: zl3073x: Add clock_id field

[Ivan Vecera]

Add .clock_id to zl3073x_dev structure that will be used by later
commits introducing DPLL driver. The clock ID is necessary for DPLL
device registration.  To generate such ID use chip ID read during
device initialization for this. For the case where are multiple
zl3073x based chips the chip ID is shifted and lower bits are filled
by an unique value. For I2C case it is I2C device address and for SPI
case it is chip-select value.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/mfd/zl3073x-core.c  | 6 +++++-
 drivers/mfd/zl3073x-i2c.c   | 3 ++-
 drivers/mfd/zl3073x-spi.c   | 3 ++-
 drivers/mfd/zl3073x.h       | 2 +-
 include/linux/mfd/zl3073x.h | 2 ++
 5 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index f606c51c90fdf..a1fb5af5b3d9f 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -467,12 +467,13 @@ static void zl3073x_devlink_unregister(void *ptr)
 /**
  * zl3073x_dev_init - initialize zl3073x device
  * @zldev: pointer to zl3073x device
+ * @dev_id: device ID to be used as part of clock ID
  *
  * Common initialization of zl3073x device structure.
  *
  * Returns: 0 on success, <0 on error
  */
-int zl3073x_dev_init(struct zl3073x_dev *zldev)
+int zl3073x_dev_init(struct zl3073x_dev *zldev, u8 dev_id)
 {
 	u16 id, revision, fw_ver;
 	struct devlink *devlink;
@@ -501,6 +502,9 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
 			return rc;
 	}
 
+	/* Use chip ID and given dev ID as clock ID */
+	zldev->clock_id = ((u64)id << 8) | dev_id;
+
 	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
 		 id, revision, fw_ver);
 	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
diff --git a/drivers/mfd/zl3073x-i2c.c b/drivers/mfd/zl3073x-i2c.c
index 461b583e536b7..9d6b8a84942d3 100644
--- a/drivers/mfd/zl3073x-i2c.c
+++ b/drivers/mfd/zl3073x-i2c.c
@@ -27,7 +27,8 @@ static int zl3073x_i2c_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, zldev);
 
-	return zl3073x_dev_init(zldev);
+	/* Initialize device and use I2C address as dev ID */
+	return zl3073x_dev_init(zldev, client->addr);
 }
 
 static const struct i2c_device_id zl3073x_i2c_id[] = {
diff --git a/drivers/mfd/zl3073x-spi.c b/drivers/mfd/zl3073x-spi.c
index db976aef74917..af98ea35440d7 100644
--- a/drivers/mfd/zl3073x-spi.c
+++ b/drivers/mfd/zl3073x-spi.c
@@ -27,7 +27,8 @@ static int zl3073x_spi_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, zldev);
 
-	return zl3073x_dev_init(zldev);
+	/* Initialize device and use SPI chip select value as dev ID */
+	return zl3073x_dev_init(zldev, spi_get_chipselect(spi, 0));
 }
 
 static const struct spi_device_id zl3073x_spi_id[] = {
diff --git a/drivers/mfd/zl3073x.h b/drivers/mfd/zl3073x.h
index 8e8ffa961e4ca..cdba713c8441f 100644
--- a/drivers/mfd/zl3073x.h
+++ b/drivers/mfd/zl3073x.h
@@ -8,7 +8,7 @@ struct regmap_config;
 struct zl3073x_dev;
 
 struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
-int zl3073x_dev_init(struct zl3073x_dev *zldev);
+int zl3073x_dev_init(struct zl3073x_dev *zldev, u8 dev_id);
 const struct regmap_config *zl3073x_get_regmap_config(void);
 
 #endif /* __ZL3073X_CORE_H */
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index cc80014ebb384..50befd7f03b24 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -16,11 +16,13 @@ struct regmap;
  * struct zl3073x_dev - zl3073x device
  * @dev: pointer to device
  * @regmap: regmap to access HW registers
+ * @clock_id: clock id of the device
  * @lock: lock to be held during access to HW registers
  */
 struct zl3073x_dev {
 	struct device		*dev;
 	struct regmap		*regmap;
+	u64			clock_id;
 	struct mutex		lock;
 };
 
-- 
2.48.1

[PATCH v2 12/14] lib: Allow modules to use strnchrnul

[Ivan Vecera]

Commit 0bee0cece2a6a ("lib/string: add strnchrnul()") added the
mentioned function but did not export it so it cannot be used by
modules.

Export strnchrnul() for modules.

Acked-by: Kees Cook <kees@kernel.org>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 lib/string.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/string.c b/lib/string.c
index eb4486ed40d25..824b3aac86de0 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -363,6 +363,7 @@ char *strnchrnul(const char *s, size_t count, int c)
 		s++;
 	return (char *)s;
 }
+EXPORT_SYMBOL(strnchrnul);
 
 #ifndef __HAVE_ARCH_STRRCHR
 /**
-- 
2.48.1

[PATCH v2 13/14] mfd: zl3073x: Load mfg file into HW if it is present

[Ivan Vecera]

Add support for loading mfg file that can be provided by a user. The mfg
file can be generated by Microchip tool and contains snippets of device
configuration that is different from the one stored in the flash memory
inside the chip.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/mfd/zl3073x-core.c | 110 +++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index a1fb5af5b3d9f..110071b28cab9 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -4,15 +4,19 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
+#include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/firmware.h>
+#include <linux/kstrtox.h>
 #include <linux/lockdep.h>
 #include <linux/mfd/zl3073x.h>
 #include <linux/module.h>
 #include <linux/netlink.h>
 #include <linux/regmap.h>
 #include <linux/sprintf.h>
+#include <linux/string.h>
 #include <linux/unaligned.h>
 #include <net/devlink.h>
 #include "zl3073x.h"
@@ -464,6 +468,108 @@ static void zl3073x_devlink_unregister(void *ptr)
 	devlink_unregister(ptr);
 }
 
+static int zl3073x_fw_parse_line(struct zl3073x_dev *zldev, const char *line)
+{
+#define ZL3073X_FW_WHITESPACES_SIZE	3
+#define ZL3073X_FW_COMMAND_SIZE		1
+	const char *ptr = line;
+	char *endp;
+	u32 delay;
+	u16 addr;
+	u8 val;
+
+	switch (ptr[0]) {
+	case 'X':
+		/* The line looks like this:
+		 * X , ADDR , VAL
+		 * Where:
+		 *  - X means that is a command that needs to be executed
+		 *  - ADDR represents the addr and is always 2 bytes and the
+		 *         value is in hex, for example 0x0232
+		 *  - VAL represents the value that is written and is always 1
+		 *        byte and the value is in hex, for example 0x12
+		 */
+		ptr += ZL3073X_FW_COMMAND_SIZE;
+		ptr += ZL3073X_FW_WHITESPACES_SIZE;
+		addr = simple_strtoul(ptr, &endp, 16);
+
+		ptr = endp;
+		ptr += ZL3073X_FW_WHITESPACES_SIZE;
+		val = simple_strtoul(ptr, NULL, 16);
+
+		/* Write requested value to given register */
+		return zl3073x_write_reg(zldev, addr, 1, &val);
+	case 'W':
+		/* The line looks like this:
+		 * W , DELAY
+		 * Where:
+		 *  - W means that is a wait command
+		 *  - DELAY represents the delay in microseconds and the value
+		 *    is in decimal
+		 */
+		ptr += ZL3073X_FW_COMMAND_SIZE;
+		ptr += ZL3073X_FW_WHITESPACES_SIZE;
+		delay = simple_strtoul(ptr, NULL, 10);
+
+		fsleep(delay);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+#define ZL3073X_MFG_FILE "microchip/zl3073x.mfg"
+
+static void zl3073x_fw_load(struct zl3073x_dev *zldev)
+{
+	const struct firmware *fw;
+	const char *ptr, *end;
+	char buf[128];
+	int rc;
+
+	rc = firmware_request_nowarn(&fw, ZL3073X_MFG_FILE, zldev->dev);
+	if (rc)
+		return;
+
+	dev_info(zldev->dev, "Applying mfg file %s...\n", ZL3073X_MFG_FILE);
+
+	guard(zl3073x)(zldev);
+
+	ptr = fw->data;
+	end = ptr + fw->size;
+	while (ptr < end) {
+		/* Get next end of the line or end of buffer */
+		char *eol = strnchrnul(ptr, end - ptr, '\n');
+		size_t len = eol - ptr;
+
+		/* Check line length */
+		if (len >= sizeof(buf)) {
+			dev_err(zldev->dev, "Line in firmware is too long\n");
+			return;
+		}
+
+		/* Copy line from buffer */
+		memcpy(buf, ptr, len);
+		buf[len] = '\0';
+
+		/* Parse and process the line */
+		rc = zl3073x_fw_parse_line(zldev, buf);
+		if (rc) {
+			dev_err(zldev->dev,
+				"Failed to parse firmware line: %pe\n",
+				ERR_PTR(rc));
+			break;
+		}
+
+		/* Move to next line */
+		ptr = eol + 1;
+	}
+
+	release_firmware(fw);
+}
+
 /**
  * zl3073x_dev_init - initialize zl3073x device
  * @zldev: pointer to zl3073x device
@@ -505,6 +611,9 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev, u8 dev_id)
 	/* Use chip ID and given dev ID as clock ID */
 	zldev->clock_id = ((u64)id << 8) | dev_id;
 
+	/* Load mfg file if present */
+	zl3073x_fw_load(zldev);
+
 	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
 		 id, revision, fw_ver);
 	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
@@ -528,3 +637,4 @@ EXPORT_SYMBOL_NS_GPL(zl3073x_dev_init, "ZL3073X");
 MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
 MODULE_DESCRIPTION("Microchip ZL3073x core driver");
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE(ZL3073X_MFG_FILE);
-- 
2.48.1

[PATCH v2 14/14] mfd: zl3073x: Fetch invariants during probe

[Ivan Vecera]

Several configuration parameters will not be changed in runtime so we
can load them during probe to avoid excessive reads from the hardware.

These parameters will be frequently read by the DPLL driver from this
series and later by PHC/PTP sub-device driver.

Read the following parameters from the device during probe and store
them for later use:

* synthesizers' frequencies and associated DPLL channel
* input pins' enablement and type (single-ended or differential)
* outputs'associated synths, signal format and enablement

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v1->v2:
* fixed and added inline documentation
---
 drivers/mfd/zl3073x-core.c  | 243 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/zl3073x.h | 161 ++++++++++++++++++++++++
 2 files changed, 404 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 110071b28cab9..70fd7a76c6478 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -11,6 +11,7 @@
 #include <linux/firmware.h>
 #include <linux/kstrtox.h>
 #include <linux/lockdep.h>
+#include <linux/math64.h>
 #include <linux/mfd/zl3073x.h>
 #include <linux/module.h>
 #include <linux/netlink.h>
@@ -29,6 +30,20 @@ ZL3073X_REG16_DEF(revision,		0x0003);
 ZL3073X_REG16_DEF(fw_ver,		0x0005);
 ZL3073X_REG32_DEF(custom_config_ver,	0x0007);
 
+/*
+ * Register Map Page 9, Synth and Output
+ */
+ZL3073X_REG8_IDX_DEF(synth_ctrl,		0x480, ZL3073X_NUM_SYNTHS, 1);
+#define SYNTH_CTRL_EN				BIT(0)
+#define SYNTH_CTRL_DPLL_SEL			GENMASK(6, 4)
+
+ZL3073X_REG8_IDX_DEF(output_ctrl,		0x4a8, ZL3073X_NUM_OUTPUTS, 1);
+#define OUTPUT_CTRL_EN				BIT(0)
+#define OUTPUT_CTRL_STOP			BIT(1)
+#define OUTPUT_CTRL_STOP_HIGH			BIT(2)
+#define OUTPUT_CTRL_STOP_HZ			BIT(3)
+#define OUTPUT_CTRL_SYNTH_SEL			GENMASK(6, 4)
+
 /*
  * Register Map Page 10, Ref Mailbox
  */
@@ -39,6 +54,10 @@ ZL3073X_REG8_DEF(ref_mb_sem,			0x504);
 #define REF_MB_SEM_WR				BIT(0)
 #define REF_MB_SEM_RD				BIT(1)
 
+ZL3073X_REG8_DEF(ref_config,			0x50d);
+#define REF_CONFIG_ENABLE			BIT(0)
+#define REF_CONFIG_DIFF_EN			BIT(2)
+
 /*
  * Register Map Page 12, DPLL Mailbox
  */
@@ -70,6 +89,9 @@ ZL3073X_REG8_DEF(output_mb_sem,			0x704);
 #define OUTPUT_MB_SEM_WR			BIT(0)
 #define OUTPUT_MB_SEM_RD			BIT(1)
 
+ZL3073X_REG8_DEF(output_mode,			0x705);
+#define OUTPUT_MODE_SIGNAL_FORMAT		GENMASK(7, 4)
+
 /*
  * Regmap ranges
  */
@@ -570,6 +592,220 @@ static void zl3073x_fw_load(struct zl3073x_dev *zldev)
 	release_firmware(fw);
 }
 
+/**
+ * zl3073x_input_state_fetch - get input state
+ * @zldev: pointer to zl3073x_dev structure
+ * @index: input pin index to fetch state for
+ *
+ * Function fetches information for the given input reference that are
+ * invariant and stores them for later use.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_input_state_fetch(struct zl3073x_dev *zldev, u8 index)
+{
+	struct zl3073x_input *input;
+	u8 ref_config;
+	int rc;
+
+	if (index >= ZL3073X_NUM_INPUTS)
+		return -EINVAL;
+
+	input = &zldev->input[index];
+
+	/* If the input is differential then the configuration for N-pin
+	 * reference is ignored and P-pin config is used for both.
+	 */
+	if (zl3073x_is_n_pin(index) &&
+	    zl3073x_input_is_diff(zldev, index - 1)) {
+		input->enabled = zl3073x_input_is_enabled(zldev, index - 1);
+		input->diff = true;
+
+		return 0;
+	}
+
+	/* Read reference configuration into mailbox */
+	rc = zl3073x_mb_ref_read(zldev, index);
+	if (rc)
+		return rc;
+
+	/* Read reference config register */
+	rc = zl3073x_read_ref_config(zldev, &ref_config);
+	if (rc)
+		return rc;
+
+	/* Store info about input reference enablement and if it is
+	 * configured in differential mode or not.
+	 */
+	input->enabled = FIELD_GET(REF_CONFIG_ENABLE, ref_config);
+	input->diff = FIELD_GET(REF_CONFIG_DIFF_EN, ref_config);
+
+	return rc;
+}
+
+/**
+ * zl3073x_output_state_fetch - get output state
+ * @zldev: pointer to zl3073x_dev structure
+ * @index: output index to fetch state for
+ *
+ * Function fetches information for the given output (not output pin)
+ * that are invariant and stores them for later use.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_output_state_fetch(struct zl3073x_dev *zldev, u8 index)
+{
+	struct zl3073x_output *output;
+	u8 output_ctrl, output_mode;
+	int rc;
+
+	if (index >= ZL3073X_NUM_OUTPUTS)
+		return -EINVAL;
+
+	output = &zldev->output[index];
+
+	/* Read output control register */
+	rc = zl3073x_read_output_ctrl(zldev, index, &output_ctrl);
+	if (rc)
+		return rc;
+
+	/* Store info about output enablement and synthesizer the output
+	 * is connected to.
+	 */
+	output->enabled = FIELD_GET(OUTPUT_CTRL_EN, output_ctrl);
+	output->synth = FIELD_GET(OUTPUT_CTRL_SYNTH_SEL, output_ctrl);
+
+	/* Load given output config into mailbox */
+	rc = zl3073x_mb_output_read(zldev, index);
+	if (rc)
+		return rc;
+
+	/* Read output mode from mailbox */
+	rc = zl3073x_read_output_mode(zldev, &output_mode);
+	if (rc)
+		return rc;
+
+	/* Extract and store output signal format */
+	output->signal_format = FIELD_GET(OUTPUT_MODE_SIGNAL_FORMAT,
+					  output_mode);
+
+	return rc;
+}
+
+/**
+ * zl3073x_synth_state_fetch - get synth state
+ * @zldev: pointer to zl3073x_dev structure
+ * @index: synth index to fetch state for
+ *
+ * Function fetches information for the given synthesizer that are
+ * invariant and stores them for later use.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_synth_state_fetch(struct zl3073x_dev *zldev, u8 index)
+{
+	u16 base, numerator, denominator;
+	u8 synth_ctrl;
+	u32 mult;
+	int rc;
+
+	/* Read synth control register */
+	rc = zl3073x_read_synth_ctrl(zldev, index, &synth_ctrl);
+	if (rc)
+		return rc;
+
+	/* Extract and store DPLL channel the synth is driven by */
+	zldev->synth[index].dpll = FIELD_GET(SYNTH_CTRL_DPLL_SEL, synth_ctrl);
+
+	dev_dbg(zldev->dev, "SYNTH%u is connected to DPLL%u\n", index,
+		zldev->synth[index].dpll);
+
+	/* Read synth configuration into mailbox */
+	rc = zl3073x_mb_synth_read(zldev, index);
+	if (rc)
+		return rc;
+
+	/* The output frequency is determined by the following formula:
+	 * base * multiplier * numerator / denominator
+	 *
+	 * Therefore get all this number and calculate the output frequency
+	 */
+	rc = zl3073x_read_synth_freq_base(zldev, &base);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_synth_freq_mult(zldev, &mult);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_synth_freq_m(zldev, &numerator);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_synth_freq_n(zldev, &denominator);
+	if (rc)
+		return rc;
+
+	/* Check denominator for zero to avoid div by 0 */
+	if (!denominator) {
+		dev_err(zldev->dev,
+			"Zero divisor for SYNTH%u retrieved from device\n",
+			index);
+		return -ENODEV;
+	}
+
+	/* Compute and store synth frequency */
+	zldev->synth[index].freq = mul_u64_u32_div(mul_u32_u32(base, mult),
+						   numerator, denominator);
+
+	dev_dbg(zldev->dev, "SYNTH%u frequency: %llu Hz\n", index,
+		zldev->synth[index].freq);
+
+	return rc;
+}
+
+static int
+zl3073x_dev_state_fetch(struct zl3073x_dev *zldev)
+{
+	int rc;
+	u8 i;
+
+	for (i = 0; i < ZL3073X_NUM_INPUTS; i++) {
+		rc = zl3073x_input_state_fetch(zldev, i);
+		if (rc) {
+			dev_err(zldev->dev,
+				"Failed to fetch input state: %pe\n",
+				ERR_PTR(rc));
+			return rc;
+		}
+	}
+
+	for (i = 0; i < ZL3073X_NUM_SYNTHS; i++) {
+		rc = zl3073x_synth_state_fetch(zldev, i);
+		if (rc) {
+			dev_err(zldev->dev,
+				"Failed to fetch synth state: %pe\n",
+				ERR_PTR(rc));
+			return rc;
+		}
+	}
+
+	for (i = 0; i < ZL3073X_NUM_OUTPUTS; i++) {
+		rc = zl3073x_output_state_fetch(zldev, i);
+		if (rc) {
+			dev_err(zldev->dev,
+				"Failed to fetch output state: %pe\n",
+				ERR_PTR(rc));
+			return rc;
+		}
+	}
+
+	return rc;
+}
+
 /**
  * zl3073x_dev_init - initialize zl3073x device
  * @zldev: pointer to zl3073x device
@@ -614,6 +850,13 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev, u8 dev_id)
 	/* Load mfg file if present */
 	zl3073x_fw_load(zldev);
 
+	/* Fetch device state */
+	scoped_guard(zl3073x, zldev) {
+		rc = zl3073x_dev_state_fetch(zldev);
+		if (rc)
+			return rc;
+	}
+
 	dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
 		 id, revision, fw_ver);
 	dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 50befd7f03b24..ec265d2e935bb 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -12,18 +12,75 @@
 struct device;
 struct regmap;
 
+/*
+ * Hardware limits for ZL3073x chip family
+ */
+#define ZL3073X_NUM_INPUTS	10
+#define ZL3073X_NUM_OUTPUTS	10
+#define ZL3073X_NUM_SYNTHS	5
+
+/**
+ * struct zl3073x_input - input invariant info
+ * @enabled: input is enabled or disabled
+ * @diff: true if input is differential
+ */
+struct zl3073x_input {
+	bool	enabled;
+	bool	diff;
+};
+
+/**
+ * struct zl3073x_output - output invariant info
+ * @enabled: output is enabled or disabled
+ * @synth: synthesizer the output is connected to
+ * @signal_format: output signal format
+ */
+struct zl3073x_output {
+	bool	enabled;
+	u8	synth;
+	u8	signal_format;
+#define OUTPUT_MODE_SIGNAL_FORMAT_DISABLED	0
+#define OUTPUT_MODE_SIGNAL_FORMAT_LVDS		1
+#define OUTPUT_MODE_SIGNAL_FORMAT_DIFFERENTIAL	2
+#define OUTPUT_MODE_SIGNAL_FORMAT_LOWVCM	3
+#define OUTPUT_MODE_SIGNAL_FORMAT_TWO		4
+#define OUTPUT_MODE_SIGNAL_FORMAT_ONE_P		5
+#define OUTPUT_MODE_SIGNAL_FORMAT_ONE_N		6
+#define OUTPUT_MODE_SIGNAL_FORMAT_TWO_INV	7
+#define OUTPUT_MODE_SIGNAL_FORMAT_TWO_N_DIV	12
+#define OUTPUT_MODE_SIGNAL_FORMAT_TWO_N_DIV_INV	15
+};
+
+/**
+ * struct zl3073x_synth - synthesizer invariant info
+ * @freq: synthesizer frequency
+ * @dpll: ID of DPLL the synthesizer is driven by
+ */
+struct zl3073x_synth {
+	u64	freq;
+	u8	dpll;
+};
+
 /**
  * struct zl3073x_dev - zl3073x device
  * @dev: pointer to device
  * @regmap: regmap to access HW registers
  * @clock_id: clock id of the device
  * @lock: lock to be held during access to HW registers
+ * @input: array of inputs
+ * @output: array of outputs
+ * @synth: array of synthesizers
  */
 struct zl3073x_dev {
 	struct device		*dev;
 	struct regmap		*regmap;
 	u64			clock_id;
 	struct mutex		lock;
+
+	/* Invariants */
+	struct zl3073x_input	input[ZL3073X_NUM_INPUTS];
+	struct zl3073x_output	output[ZL3073X_NUM_OUTPUTS];
+	struct zl3073x_synth	synth[ZL3073X_NUM_SYNTHS];
 };
 
 /**
@@ -199,4 +256,108 @@ int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
 int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
 int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
 
+static inline
+bool zl3073x_is_n_pin(u8 index)
+{
+	/* P-pins indices are even while N-pins are odd */
+	return index & 1;
+}
+
+static inline
+bool zl3073x_is_p_pin(u8 index)
+{
+	return !zl3073x_is_n_pin(index);
+}
+
+/**
+ * zl3073x_input_is_diff - check if the given input ref is differential
+ * @zldev: pointer to zl3073x device
+ * @index: output index
+ *
+ * Return: true if input is differential, false if input is single-ended
+ */
+static inline
+bool zl3073x_input_is_diff(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->input[index].diff;
+}
+
+/**
+ * zl3073x_input_is_enabled - check if the given input ref is enabled
+ * @zldev: pointer to zl3073x device
+ * @index: input index
+ *
+ * Return: true if input is enabled, false if input is disabled
+ */
+static inline
+bool zl3073x_input_is_enabled(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->input[index].enabled;
+}
+
+/**
+ * zl3073x_output_is_enabled - check if the given output is enabled
+ * @zldev: pointer to zl3073x device
+ * @index: output index
+ *
+ * Return: true if output is enabled, false if output is disabled
+ */
+static inline
+u8 zl3073x_output_is_enabled(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->output[index].enabled;
+}
+
+/**
+ * zl3073x_output_signal_format_get - get output signal format
+ * @zldev: pointer to zl3073x device
+ * @index: output index
+ *
+ * Return: signal format of given output
+ */
+static inline
+u8 zl3073x_output_signal_format_get(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->output[index].signal_format;
+}
+
+/**
+ * zl3073x_output_synth_get - get synth connected to given output
+ * @zldev: pointer to zl3073x device
+ * @index: output index
+ *
+ * Return: index of synth connected to given output.
+ */
+static inline
+u8 zl3073x_output_synth_get(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->output[index].synth;
+}
+
+/**
+ * zl3073x_synth_dpll_get - get DPLL ID the synth is driven by
+ * @zldev: pointer to zl3073x device
+ * @index: synth index
+ *
+ * Return: ID of DPLL the given synthetizer is driven by
+ */
+static inline
+u64 zl3073x_synth_dpll_get(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->synth[index].dpll;
+}
+
+/**
+ * zl3073x_synth_freq_get - get synth current freq
+ * @zldev: pointer to zl3073x device
+ * @index: synth index
+ *
+ * Return: frequency of given synthetizer
+ */
+static inline
+u64 zl3073x_synth_freq_get(struct zl3073x_dev *zldev, u8 index)
+{
+	return zldev->synth[index].freq;
+}
+
 #endif /* __LINUX_MFD_ZL3073X_H */
-- 
2.48.1

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Jakub Kicinski]

On Wed,  9 Apr 2025 16:42:36 +0200 Ivan Vecera wrote:
> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> provides DPLL and PTP functionality. This series bring first part
> that adds the common MFD driver that provides an access to the bus
> that can be either I2C or SPI.
> 
> The next series will bring the DPLL driver that will covers DPLL
> functionality. And another ones will bring PTP driver and flashing
> capability via devlink.
> 
> Testing was done by myself and by Prathosh Satish on Microchip EDS2
> development board with ZL30732 DPLL chip connected over I2C bus.

The DPLL here is for timing, right? Not digital logic?
After a brief glance I'm wondering why mfd, PHC + DPLL 
is a pretty common combo. Am I missing something?

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Ivan Vecera]

On 10. 04. 25 2:17 dop., Jakub Kicinski wrote:
> On Wed,  9 Apr 2025 16:42:36 +0200 Ivan Vecera wrote:
>> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
>> provides DPLL and PTP functionality. This series bring first part
>> that adds the common MFD driver that provides an access to the bus
>> that can be either I2C or SPI.
>>
>> The next series will bring the DPLL driver that will covers DPLL
>> functionality. And another ones will bring PTP driver and flashing
>> capability via devlink.
>>
>> Testing was done by myself and by Prathosh Satish on Microchip EDS2
>> development board with ZL30732 DPLL chip connected over I2C bus.
> 
> The DPLL here is for timing, right? Not digital logic?
> After a brief glance I'm wondering why mfd, PHC + DPLL
> is a pretty common combo. Am I missing something?

Well, you are right, this is not pretty common combo right now. But how 
many DPLL implementations we have now in kernel?

There are 3 mlx5, ice and ptp_ocp. The first two are ethernet NIC 
drivers that re-expose (translate) DPLL API provided by their firmwares 
and the 3rd timecard that acts primarily as PTP clock.

Azurite is primarly the DPLL chip with multiple DPLL channels and one of 
its use-case is time synchronization or signal synchronization. Other 
one can be PTP clock and even GPIO controller where some of input or 
output pins can be configured not to receive or send periodic signal but 
can act is GPIO inputs or outputs (depends on wiring and usage).

So I have taken an approach to have common MFD driver that provides a 
synchronized access to device registers and to have another drivers for 
particular functionality in well bounded manner (DPLL sub-device (MFD 
cell) for each DPLL channel, PTP cell for channel that is configured to 
provide PTP clock and potentially GPIO controller cell but this is 
out-of-scope now).

Btw. I would be interesting to see a NIC that just exposes an access to 
I2C bus (implements I2C read/write by NIC firmware) instead of exposing 
complete DPLL API from the firmware. Just an idea.

Thanks,
Ivan

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Andrew Lunn]

> Btw. I would be interesting to see a NIC that just exposes an access to I2C
> bus (implements I2C read/write by NIC firmware) instead of exposing complete
> DPLL API from the firmware. Just an idea.

The mellanox driver does expose an standard Linux I2C bus, with the
firmware wiggling the bits on the wire. But i've no idea what devices
are hanging off the bus.

	Andrew

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Jakub Kicinski]

On Thu, 10 Apr 2025 11:18:24 +0200 Ivan Vecera wrote:
> On 10. 04. 25 2:17 dop., Jakub Kicinski wrote:
> > On Wed,  9 Apr 2025 16:42:36 +0200 Ivan Vecera wrote:  
> >> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> >> provides DPLL and PTP functionality. This series bring first part
> >> that adds the common MFD driver that provides an access to the bus
> >> that can be either I2C or SPI.
> >>
> >> The next series will bring the DPLL driver that will covers DPLL
> >> functionality. And another ones will bring PTP driver and flashing
> >> capability via devlink.
> >>
> >> Testing was done by myself and by Prathosh Satish on Microchip EDS2
> >> development board with ZL30732 DPLL chip connected over I2C bus.  
> > 
> > The DPLL here is for timing, right? Not digital logic?
> > After a brief glance I'm wondering why mfd, PHC + DPLL
> > is a pretty common combo. Am I missing something?  
> 
> Well, you are right, this is not pretty common combo right now. But how 
> many DPLL implementations we have now in kernel?
> 
> There are 3 mlx5, ice and ptp_ocp. The first two are ethernet NIC 
> drivers that re-expose (translate) DPLL API provided by their firmwares 
> and the 3rd timecard that acts primarily as PTP clock.
> 
> Azurite is primarly the DPLL chip with multiple DPLL channels and one of 
> its use-case is time synchronization or signal synchronization. Other 
> one can be PTP clock and even GPIO controller where some of input or 
> output pins can be configured not to receive or send periodic signal but 
> can act is GPIO inputs or outputs (depends on wiring and usage).
> 
> So I have taken an approach to have common MFD driver that provides a 
> synchronized access to device registers and to have another drivers for 
> particular functionality in well bounded manner (DPLL sub-device (MFD 
> cell) for each DPLL channel, PTP cell for channel that is configured to 
> provide PTP clock and potentially GPIO controller cell but this is 
> out-of-scope now).

Okay, my understanding was that if you need to reuse the component
drivers across multiple different SoCs or devices, and there is no
"natural" bus then MFD is the go to. OTOH using MFD as a software
abstraction/to organize your code is a pointless complication.
(We're going to merge the MFD parts via Lee's tree and the all actual
drivers via netdev?) Admittedly that's just my feeling and not based 
on any real info or experience. I defer to Lee and others to pass
judgment.

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Ivan Vecera]

On 11. 04. 25 12:57 dop., Jakub Kicinski wrote:
> On Thu, 10 Apr 2025 11:18:24 +0200 Ivan Vecera wrote:
>> On 10. 04. 25 2:17 dop., Jakub Kicinski wrote:
>>> On Wed,  9 Apr 2025 16:42:36 +0200 Ivan Vecera wrote:
>>>> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
>>>> provides DPLL and PTP functionality. This series bring first part
>>>> that adds the common MFD driver that provides an access to the bus
>>>> that can be either I2C or SPI.
>>>>
>>>> The next series will bring the DPLL driver that will covers DPLL
>>>> functionality. And another ones will bring PTP driver and flashing
>>>> capability via devlink.
>>>>
>>>> Testing was done by myself and by Prathosh Satish on Microchip EDS2
>>>> development board with ZL30732 DPLL chip connected over I2C bus.
>>>
>>> The DPLL here is for timing, right? Not digital logic?
>>> After a brief glance I'm wondering why mfd, PHC + DPLL
>>> is a pretty common combo. Am I missing something?
>>
>> Well, you are right, this is not pretty common combo right now. But how
>> many DPLL implementations we have now in kernel?
>>
>> There are 3 mlx5, ice and ptp_ocp. The first two are ethernet NIC
>> drivers that re-expose (translate) DPLL API provided by their firmwares
>> and the 3rd timecard that acts primarily as PTP clock.
>>
>> Azurite is primarly the DPLL chip with multiple DPLL channels and one of
>> its use-case is time synchronization or signal synchronization. Other
>> one can be PTP clock and even GPIO controller where some of input or
>> output pins can be configured not to receive or send periodic signal but
>> can act is GPIO inputs or outputs (depends on wiring and usage).
>>
>> So I have taken an approach to have common MFD driver that provides a
>> synchronized access to device registers and to have another drivers for
>> particular functionality in well bounded manner (DPLL sub-device (MFD
>> cell) for each DPLL channel, PTP cell for channel that is configured to
>> provide PTP clock and potentially GPIO controller cell but this is
>> out-of-scope now).
> 
> Okay, my understanding was that if you need to reuse the component
> drivers across multiple different SoCs or devices, and there is no
> "natural" bus then MFD is the go to. OTOH using MFD as a software
> abstraction/to organize your code is a pointless complication.
> (We're going to merge the MFD parts via Lee's tree and the all actual
> drivers via netdev?) Admittedly that's just my feeling and not based
> on any real info or experience. I defer to Lee and others to pass
> judgment.

I followed an example of rsmu mfd driver that provides an access to the 
bus (i2c/spi) via regmap and ptp_clockmatrix platform driver for the PTP 
functionality of the RSMU chip. The ptp_clockmatrix device is also 
instantiated only from rsmu mfd and it is not shared by multiple mfd 
drivers.

I.

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Lee Jones]

On Wed, 09 Apr 2025, Ivan Vecera wrote:

> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> provides DPLL and PTP functionality. This series bring first part
> that adds the common MFD driver that provides an access to the bus
> that can be either I2C or SPI.
> 
> The next series will bring the DPLL driver that will covers DPLL
> functionality. And another ones will bring PTP driver and flashing
> capability via devlink.
> 
> Testing was done by myself and by Prathosh Satish on Microchip EDS2
> development board with ZL30732 DPLL chip connected over I2C bus.
> 
> Patch breakdown
> ===============
> Patch 1 - Common DT schema for DPLL device and pin
> Patch 3 - Basic support for I2C, SPI and regmap
> Patch 4 - Devlink registration
> Patches 5-6 - Helpers for accessing device registers
> Patches 7-8 - Component versions reporting via devlink dev info
> Patches 9-10 - Helpers for accessing register mailboxes
> Patch 11 - Clock ID generation for DPLL driver
> Patch 12 - Export strnchrnul function for modules
>            (used by next patch)
> Patch 13 - Support for MFG config initialization file
> Patch 14 - Fetch invariant register values used by DPLL and later by
>            PTP driver
> 
> Ivan Vecera (14):
>   dt-bindings: dpll: Add device tree bindings for DPLL device and pin
>   dt-bindings: dpll: Add support for Microchip Azurite chip family
>   mfd: Add Microchip ZL3073x support
>   mfd: zl3073x: Register itself as devlink device
>   mfd: zl3073x: Add register access helpers
>   mfd: zl3073x: Add macros for device registers access
>   mfd: zl3073x: Add components versions register defs
>   mfd: zl3073x: Implement devlink device info
>   mfd: zl3073x: Add macro to wait for register value bits to be cleared
>   mfd: zl3073x: Add functions to work with register mailboxes
>   mfd: zl3073x: Add clock_id field
>   lib: Allow modules to use strnchrnul
>   mfd: zl3073x: Load mfg file into HW if it is present
>   mfd: zl3073x: Fetch invariants during probe
> 
>  .../devicetree/bindings/dpll/dpll-device.yaml |  76 ++
>  .../devicetree/bindings/dpll/dpll-pin.yaml    |  44 +
>  .../bindings/dpll/microchip,zl3073x-i2c.yaml  |  74 ++
>  .../bindings/dpll/microchip,zl3073x-spi.yaml  |  77 ++
>  MAINTAINERS                                   |  11 +
>  drivers/mfd/Kconfig                           |  32 +
>  drivers/mfd/Makefile                          |   5 +
>  drivers/mfd/zl3073x-core.c                    | 883 ++++++++++++++++++
>  drivers/mfd/zl3073x-i2c.c                     |  59 ++
>  drivers/mfd/zl3073x-spi.c                     |  59 ++
>  drivers/mfd/zl3073x.h                         |  14 +
>  include/linux/mfd/zl3073x.h                   | 363 +++++++
>  lib/string.c                                  |   1 +
>  13 files changed, 1698 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>  create mode 100644 drivers/mfd/zl3073x-core.c
>  create mode 100644 drivers/mfd/zl3073x-i2c.c
>  create mode 100644 drivers/mfd/zl3073x-spi.c
>  create mode 100644 drivers/mfd/zl3073x.h
>  create mode 100644 include/linux/mfd/zl3073x.h

FWIW, I'm not planning to even look at this until Andy and Krzysztof are
satisfied.  What I will say is that all of those horrible macros will
have to be removed and no abstractions will be accepted unless they are
accompanied with very good reasons as to why they are required.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Lee Jones]

On Wed, 09 Apr 2025, Ivan Vecera wrote:

> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> provides DPLL and PTP functionality. This series bring first part
> that adds the common MFD driver that provides an access to the bus
> that can be either I2C or SPI.
> 
> The next series will bring the DPLL driver that will covers DPLL
> functionality. And another ones will bring PTP driver and flashing
> capability via devlink.
> 
> Testing was done by myself and by Prathosh Satish on Microchip EDS2
> development board with ZL30732 DPLL chip connected over I2C bus.
> 
> Patch breakdown
> ===============
> Patch 1 - Common DT schema for DPLL device and pin
> Patch 3 - Basic support for I2C, SPI and regmap
> Patch 4 - Devlink registration
> Patches 5-6 - Helpers for accessing device registers
> Patches 7-8 - Component versions reporting via devlink dev info
> Patches 9-10 - Helpers for accessing register mailboxes
> Patch 11 - Clock ID generation for DPLL driver
> Patch 12 - Export strnchrnul function for modules
>            (used by next patch)
> Patch 13 - Support for MFG config initialization file
> Patch 14 - Fetch invariant register values used by DPLL and later by
>            PTP driver
> 
> Ivan Vecera (14):
>   dt-bindings: dpll: Add device tree bindings for DPLL device and pin
>   dt-bindings: dpll: Add support for Microchip Azurite chip family
>   mfd: Add Microchip ZL3073x support
>   mfd: zl3073x: Register itself as devlink device
>   mfd: zl3073x: Add register access helpers
>   mfd: zl3073x: Add macros for device registers access
>   mfd: zl3073x: Add components versions register defs
>   mfd: zl3073x: Implement devlink device info
>   mfd: zl3073x: Add macro to wait for register value bits to be cleared
>   mfd: zl3073x: Add functions to work with register mailboxes
>   mfd: zl3073x: Add clock_id field
>   lib: Allow modules to use strnchrnul
>   mfd: zl3073x: Load mfg file into HW if it is present
>   mfd: zl3073x: Fetch invariants during probe
> 
>  .../devicetree/bindings/dpll/dpll-device.yaml |  76 ++
>  .../devicetree/bindings/dpll/dpll-pin.yaml    |  44 +
>  .../bindings/dpll/microchip,zl3073x-i2c.yaml  |  74 ++
>  .../bindings/dpll/microchip,zl3073x-spi.yaml  |  77 ++
>  MAINTAINERS                                   |  11 +
>  drivers/mfd/Kconfig                           |  32 +
>  drivers/mfd/Makefile                          |   5 +
>  drivers/mfd/zl3073x-core.c                    | 883 ++++++++++++++++++
>  drivers/mfd/zl3073x-i2c.c                     |  59 ++
>  drivers/mfd/zl3073x-spi.c                     |  59 ++
>  drivers/mfd/zl3073x.h                         |  14 +
>  include/linux/mfd/zl3073x.h                   | 363 +++++++
>  lib/string.c                                  |   1 +
>  13 files changed, 1698 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>  create mode 100644 drivers/mfd/zl3073x-core.c
>  create mode 100644 drivers/mfd/zl3073x-i2c.c
>  create mode 100644 drivers/mfd/zl3073x-spi.c
>  create mode 100644 drivers/mfd/zl3073x.h
>  create mode 100644 include/linux/mfd/zl3073x.h

Not only are all of the added abstractions and ugly MACROs hard to read
and troublesome to maintain, they're also completely unnecessary at this
(driver) level.  Nicely authored, easy to read / maintain code wins over
clever code 95% of the time.  Exporting functions to pass around
pointers to private structures is also a non-starter.

After looking at the code, there doesn't appear to be any inclusion of
the MFD core header.  How can this be an MFD if you do not use the MFD
API?  MFD is not a dumping area for common code and call-backs.  It's a
subsystem used to neatly separate out and share resources between
functionality that just happens to share the same hardware chip.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Ivan Vecera]

On 11. 04. 25 9:26 dop., Lee Jones wrote:
> On Wed, 09 Apr 2025, Ivan Vecera wrote:
> 
>> Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
>> provides DPLL and PTP functionality. This series bring first part
>> that adds the common MFD driver that provides an access to the bus
>> that can be either I2C or SPI.
>>
>> The next series will bring the DPLL driver that will covers DPLL
>> functionality. And another ones will bring PTP driver and flashing
>> capability via devlink.
>>
>> Testing was done by myself and by Prathosh Satish on Microchip EDS2
>> development board with ZL30732 DPLL chip connected over I2C bus.
>>
>> Patch breakdown
>> ===============
>> Patch 1 - Common DT schema for DPLL device and pin
>> Patch 3 - Basic support for I2C, SPI and regmap
>> Patch 4 - Devlink registration
>> Patches 5-6 - Helpers for accessing device registers
>> Patches 7-8 - Component versions reporting via devlink dev info
>> Patches 9-10 - Helpers for accessing register mailboxes
>> Patch 11 - Clock ID generation for DPLL driver
>> Patch 12 - Export strnchrnul function for modules
>>             (used by next patch)
>> Patch 13 - Support for MFG config initialization file
>> Patch 14 - Fetch invariant register values used by DPLL and later by
>>             PTP driver
>>
>> Ivan Vecera (14):
>>    dt-bindings: dpll: Add device tree bindings for DPLL device and pin
>>    dt-bindings: dpll: Add support for Microchip Azurite chip family
>>    mfd: Add Microchip ZL3073x support
>>    mfd: zl3073x: Register itself as devlink device
>>    mfd: zl3073x: Add register access helpers
>>    mfd: zl3073x: Add macros for device registers access
>>    mfd: zl3073x: Add components versions register defs
>>    mfd: zl3073x: Implement devlink device info
>>    mfd: zl3073x: Add macro to wait for register value bits to be cleared
>>    mfd: zl3073x: Add functions to work with register mailboxes
>>    mfd: zl3073x: Add clock_id field
>>    lib: Allow modules to use strnchrnul
>>    mfd: zl3073x: Load mfg file into HW if it is present
>>    mfd: zl3073x: Fetch invariants during probe
>>
>>   .../devicetree/bindings/dpll/dpll-device.yaml |  76 ++
>>   .../devicetree/bindings/dpll/dpll-pin.yaml    |  44 +
>>   .../bindings/dpll/microchip,zl3073x-i2c.yaml  |  74 ++
>>   .../bindings/dpll/microchip,zl3073x-spi.yaml  |  77 ++
>>   MAINTAINERS                                   |  11 +
>>   drivers/mfd/Kconfig                           |  32 +
>>   drivers/mfd/Makefile                          |   5 +
>>   drivers/mfd/zl3073x-core.c                    | 883 ++++++++++++++++++
>>   drivers/mfd/zl3073x-i2c.c                     |  59 ++
>>   drivers/mfd/zl3073x-spi.c                     |  59 ++
>>   drivers/mfd/zl3073x.h                         |  14 +
>>   include/linux/mfd/zl3073x.h                   | 363 +++++++
>>   lib/string.c                                  |   1 +
>>   13 files changed, 1698 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
>>   create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml
>>   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-i2c.yaml
>>   create mode 100644 Documentation/devicetree/bindings/dpll/microchip,zl3073x-spi.yaml
>>   create mode 100644 drivers/mfd/zl3073x-core.c
>>   create mode 100644 drivers/mfd/zl3073x-i2c.c
>>   create mode 100644 drivers/mfd/zl3073x-spi.c
>>   create mode 100644 drivers/mfd/zl3073x.h
>>   create mode 100644 include/linux/mfd/zl3073x.h
> 
> Not only are all of the added abstractions and ugly MACROs hard to read
> and troublesome to maintain, they're also completely unnecessary at this
> (driver) level.  Nicely authored, easy to read / maintain code wins over
> clever code 95% of the time.  Exporting functions to pass around
> pointers to private structures is also a non-starter.

If you mean regmap_config exported to zl3073x-i2c/spi modules then these 
three modules could be squashed together into single module.

> After looking at the code, there doesn't appear to be any inclusion of
> the MFD core header.  How can this be an MFD if you do not use the MFD
> API?  MFD is not a dumping area for common code and call-backs.  It's a
> subsystem used to neatly separate out and share resources between
> functionality that just happens to share the same hardware chip.

You are right, the v2 series was spliced into 2 separate series as the 
bot warns about too big (>15 commits) series. The real MFD API usage is 
present in the second one (or in patch 14 of the v1 patch-set) when MFD 
cells are created for DPLL functional blocks.

And yes, I chose the MFD for the common part because DPLL and PTP 
functions share the same registers concurrently. And MFD could be the 
right place for exposing these shared resources and providing 
synchronized access to them. The device has also GPIO functionality that 
could be potentially covered in the future.

Or would you prefer to implement all these functional block in one 
monolithic driver? Would it be better for maintenance?

Thanks,
Ivan

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Michal Schmidt]

On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@kernel.org> wrote:
> On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > provides DPLL and PTP functionality. This series bring first part
> > that adds the common MFD driver that provides an access to the bus
> > that can be either I2C or SPI.
> > [...]
>
> Not only are all of the added abstractions and ugly MACROs hard to read
> and troublesome to maintain, they're also completely unnecessary at this
> (driver) level.  Nicely authored, easy to read / maintain code wins over
> clever code 95% of the time.

Hello Lee,

IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
clever and easy to read / maintain. On one line I can see the register
name, size and address. For the indexed registers also their count and
the stride. It's almost like looking at a datasheet. And the
type-checking for accessing the registers using the correct size is
nice. I even liked the paranoid WARN_ON for checking the index
overflows.

The weak point is the non-obvious usage in call sites. Seeing:
  rc = zl3073x_read_id(zldev, &id);
can be confusing. One will not find the function with cscope or grep.
Nothing immediately suggests that there's macro magic behind it.
What if usage had to be just slightly more explicit?:
  rc = ZL3073X_READ(id, zldev, &id);

I could immediately see that ZL3073X_READ is a macro. Its definition
would be near the definitions of the ZL3073X_REG*_DEF macros, so I
could correctly guess these things are related.
The 1st argument of the ZL3073X_READ macro is the register name.
(There would be a ZL3073X_READ_IDX with one more argument for indexed
registers.)
In vim, having the cursor on the 1st argument (id) and pressing gD
takes me to the corresponding ZL3073X_REG16_DEF line.

Would it still be too ugly in your view?

Michal

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Michal Schmidt]

On Fri, Apr 11, 2025 at 4:27 PM Michal Schmidt <mschmidt@redhat.com> wrote:
> On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@kernel.org> wrote:
> > On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > > provides DPLL and PTP functionality. This series bring first part
> > > that adds the common MFD driver that provides an access to the bus
> > > that can be either I2C or SPI.
> > > [...]
> >
> > Not only are all of the added abstractions and ugly MACROs hard to read
> > and troublesome to maintain, they're also completely unnecessary at this
> > (driver) level.  Nicely authored, easy to read / maintain code wins over
> > clever code 95% of the time.
>
> Hello Lee,
>
> IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
> clever and easy to read / maintain. On one line I can see the register
> name, size and address. For the indexed registers also their count and
> the stride. It's almost like looking at a datasheet. And the
> type-checking for accessing the registers using the correct size is
> nice. I even liked the paranoid WARN_ON for checking the index
> overflows.
>
> The weak point is the non-obvious usage in call sites. Seeing:
>   rc = zl3073x_read_id(zldev, &id);
> can be confusing. One will not find the function with cscope or grep.
> Nothing immediately suggests that there's macro magic behind it.
> What if usage had to be just slightly more explicit?:
>   rc = ZL3073X_READ(id, zldev, &id);
>
> I could immediately see that ZL3073X_READ is a macro. Its definition
> would be near the definitions of the ZL3073X_REG*_DEF macros, so I
> could correctly guess these things are related.
> The 1st argument of the ZL3073X_READ macro is the register name.
> (There would be a ZL3073X_READ_IDX with one more argument for indexed
> registers.)
> In vim, having the cursor on the 1st argument (id) and pressing gD
> takes me to the corresponding ZL3073X_REG16_DEF line.
>
> Would it still be too ugly in your view?

And if having "id" as both the register name and the local variable
name is irritating, the registers can get a prefix, e.g. the register
name could be defined as REG_id.

Michal

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Rob Herring]

On Fri, Apr 11, 2025 at 04:27:08PM +0200, Michal Schmidt wrote:
> On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@kernel.org> wrote:
> > On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > > provides DPLL and PTP functionality. This series bring first part
> > > that adds the common MFD driver that provides an access to the bus
> > > that can be either I2C or SPI.
> > > [...]
> >
> > Not only are all of the added abstractions and ugly MACROs hard to read
> > and troublesome to maintain, they're also completely unnecessary at this
> > (driver) level.  Nicely authored, easy to read / maintain code wins over
> > clever code 95% of the time.
> 
> Hello Lee,
> 
> IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
> clever and easy to read / maintain. On one line I can see the register
> name, size and address. For the indexed registers also their count and
> the stride. It's almost like looking at a datasheet. And the
> type-checking for accessing the registers using the correct size is
> nice. I even liked the paranoid WARN_ON for checking the index
> overflows.

If this is much better, define (and upstream) something for everyone to 
use rather than a one-off in some driver. It doesn't matter how great it 
is if it is different from everyone else. The last thing I want to do is 
figure out how register accesses work when looking at an unfamilar 
driver.

> The weak point is the non-obvious usage in call sites. Seeing:
>   rc = zl3073x_read_id(zldev, &id);
> can be confusing. One will not find the function with cscope or grep.

Exactly.

> Nothing immediately suggests that there's macro magic behind it.
> What if usage had to be just slightly more explicit?:
>   rc = ZL3073X_READ(id, zldev, &id);
> 
> I could immediately see that ZL3073X_READ is a macro. Its definition
> would be near the definitions of the ZL3073X_REG*_DEF macros, so I
> could correctly guess these things are related.
> The 1st argument of the ZL3073X_READ macro is the register name.
> (There would be a ZL3073X_READ_IDX with one more argument for indexed
> registers.)
> In vim, having the cursor on the 1st argument (id) and pressing gD
> takes me to the corresponding ZL3073X_REG16_DEF line.
> 
> Would it still be too ugly in your view?

If you have opinions on how register accesses should look, how to do 
them in rust is being defined now.

Rob

Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

[Lee Jones]

On Fri, 11 Apr 2025, Rob Herring wrote:

> On Fri, Apr 11, 2025 at 04:27:08PM +0200, Michal Schmidt wrote:
> > On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@kernel.org> wrote:
> > > On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > > > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > > > provides DPLL and PTP functionality. This series bring first part
> > > > that adds the common MFD driver that provides an access to the bus
> > > > that can be either I2C or SPI.
> > > > [...]
> > >
> > > Not only are all of the added abstractions and ugly MACROs hard to read
> > > and troublesome to maintain, they're also completely unnecessary at this
> > > (driver) level.  Nicely authored, easy to read / maintain code wins over
> > > clever code 95% of the time.
> > 
> > Hello Lee,
> > 
> > IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
> > clever and easy to read / maintain. On one line I can see the register
> > name, size and address. For the indexed registers also their count and
> > the stride. It's almost like looking at a datasheet. And the
> > type-checking for accessing the registers using the correct size is
> > nice. I even liked the paranoid WARN_ON for checking the index
> > overflows.
> 
> If this is much better, define (and upstream) something for everyone to 
> use rather than a one-off in some driver. It doesn't matter how great it 
> is if it is different from everyone else. The last thing I want to do is 
> figure out how register accesses work when looking at an unfamilar 
> driver.

Exactly right.  The issue isn't that defining register accesses using
MACROs is a bad idea generally.  I've seen it done before in downstream
BSPs and the like.  It's that this solution isn't following the
conventions already in carved for such things in the Mainline kernel.
To engineers already used to the current conventions, this is much
harder to follow and interact with.

As Rob says, if this is truly better, discuss the idea with a much
wider audience and have it applied broadly across all areas.  We shall
not be bucking the trend or trend setting here in MFD.

-- 
Lee Jones [李琼斯]