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