Re: [PATCH v3 net-next 3/8] mfd: Add Microchip ZL3073x support

[Ivan Vecera <ivecera@redhat.com>]

On 17. 04. 25 3:13 odp., Andrew Lunn wrote:
> On Wed, Apr 16, 2025 at 08:19:25PM +0200, Ivan Vecera wrote:
>> On Wed, Apr 16, 2025 at 7:11 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>>      > +++ b/include/linux/mfd/zl3073x_regs.h
>>      > @@ -0,0 +1,105 @@
>>      > +/* SPDX-License-Identifier: GPL-2.0-only */
>>      > +
>>      > +#ifndef __LINUX_MFD_ZL3073X_REGS_H
>>      > +#define __LINUX_MFD_ZL3073X_REGS_H
>>      > +
>>      > +#include <asm/byteorder.h>
>>      > +#include <linux/lockdep.h>
>>
>>      lockdep?
>>
>>
>> lockdep_assert*() is used in later introduced helpers.
> 
> nitpicking, but you generally add headers as they are needed.

+1


>>      > +#include <linux/mfd/zl3073x.h>
>>      > +#include <linux/regmap.h>
>>      > +#include <linux/types.h>
>>      > +#include <linux/unaligned.h>
>>      > +
>>      > +/* Registers are mapped at offset 0x100 */
>>      > +#define ZL_RANGE_OFF        0x100
>>      > +#define ZL_PAGE_SIZE        0x80
>>      > +#define ZL_REG_ADDR(_pg, _off) (ZL_RANGE_OFF + (_pg) * ZL_PAGE_SIZE +
>>      (_off))
>>      > +
>>      > +/**************************
>>      > + * Register Page 0, General
>>      > + **************************/
>>      > +
>>      > +/*
>>      > + * Register 'id'
>>      > + * Page: 0, Offset: 0x01, Size: 16 bits
>>      > + */
>>      > +#define ZL_REG_ID ZL_REG_ADDR(0, 0x01)
>>      > +
>>      > +static inline __maybe_unused int
>>      > +zl3073x_read_id(struct zl3073x_dev *zldev, u16 *value)
>>      > +{
>>      > +     __be16 temp;
>>      > +     int rc;
>>      > +
>>      > +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_ID, &temp, sizeof
>>      (temp));
>>      > +     if (rc)
>>      > +             return rc;
>>      > +
>>      > +     *value = be16_to_cpu(temp);
>>      > +     return rc;
>>      > +}
>>
>>      It seems odd these are inline functions in a header file.
>>
>>
>> There are going to be used by dpll_zl3073x sub-driver in series part 2.
> 
> The subdriver needs to know the ID, firmware version, etc?

No

> Anyway, look around. How many other MFD, well actually, any sort of
> driver at all, have a bunch of low level helpers as inline functions
> in a header? You are aiming to write a plain boring driver which looks
> like every other driver in Linux....

Well, I took inline functions approach as this is safer than macro usage
and each register have own very simple implementation with type and
range control (in case of indexed registers).

It is safer to use:
zl3073x_read_ref_config(..., &v);
...
zl3073x_read_ref_config(..., &v);

than:
zl3073x_read_reg8(..., ZL_REG_REF_CONFIG, &v);
...
zl3073x_read_reg16(..., ZL_REG_REF_CONFIG, &v); /* wrong */

With inline function defined for each register the mistake in the 
example cannot happen and also compiler checks that 'v' has correct type.

> Think about your layering. What does the MFD need to offer to sub
> drivers so they can work? For lower registers, maybe just
> zl3073x_read_u8(), zl3073x_read_u16() & zl3073x_read_read_u32(). Write
> variants as well. Plus the API needed to safely access the mailbox.
> Export these using one of the EXPORT_SYMBOL_GPL() variants, so the sub
> drivers can access them. The #defines for the registers numbers can be
> placed into a shared header file.

Would it be acceptable for you something like this:

int zl3073x_read_reg{8,16,32,48}(..., unsigned int reg, u{8,16,32,64} *v);
int zl3073x_write_reg{8,16,32,48}(..., unsigned int reg, u{8,16,32,64} v);
int zl3073x_read_idx_reg{8,16,32,48}(..., unsigned int reg, unsigned int 
idx, unsigned int max_idx, unsigned int stride, u{8,16,32,64} *v);
int zl3073x_write_idx_reg{8,16,32,48}(..., unsigned int reg, unsigned 
int idx, unsigned int max_idx, unsigned int stride, u{8,16,32,64} v);

/* Simple non-indexed register */
#define ZL_REG_ID	ZL_REG_ADDR(0 /* page */, 0x01 /* offset */
#define ZL_REG_ID_BITS	8

/* Simple indexed register */
#define ZL_REG_REF_STATUS	ZL_REG_ADDR(2, 0x44)
#define ZL_REG_REF_STATUS_BITS	16
#define ZL_REG_REF_STATUS_ITEMS	10
#define ZL_REG_REF_STATUS_STRIDE 2

/* Read macro for non-indexed register */
#define ZL3073X_READ_REG(_zldev, _reg, _v)	\
	__PASTE(zl3073x_read_reg, _reg##_BITS)(_zldev, _reg, _v)

/* For indexed one */
#define ZL3073X_READ_IDX_REG(_zldev, _reg, _idx, _v)	\
	__PASTE(zl3073x_read_idx_reg, _reg##_BITS)(_zldev, _reg, _v,
						   _idx, \
						   _reg##_ITEMS, \
						   _reg##_STRIDE, _v)

This would allow to call simply
ZL3073X_READ_REG(zldev, ZL_REG_ID, &val);
or
ZL3073X_READ_IDX_REG(zldev, ZL_REG_REF_STATUS, 4);

and caller does not need to know register size and range constraints.

WDYT?

Thanks,
Ivan