On Fri, Apr 11, 2025 at 04:27:08PM +0200, Michal Schmidt wrote: > On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <l...@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
