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@xxxxxxxxxx> wrote:
> On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@xxxxxxxxxx> 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