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

Rob Herring
2025-04-11T15:58:18Z

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