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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]