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

[Ivan Vecera <ivecera@redhat.com>]

On 17. 04. 25 4:50 odp., Ivan Vecera wrote:
> 
> 
> 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?

Or another approach could be:

struct zl_reg {
	unsigned int addr;
	unsigned int bits;
	unsigned int items;
	unsigned int stride;
};

#define ZL_DEF_IDX_REG(_name, _addr, _bits, _items, _stride)	\
static const struct zl_reg _name = {
	.addr = _addr,
	.bits = _bits,
	.items = _items,
	.stride = _stride,
}

#define ZL_DEF_REG(_name, _addr, _bits)	\
	ZL_DEF_IDX_REG(_name, _addr, _bits, 1, 0)

static inline int zl_read_reg(..., const struct zl_reg reg, void *value)
{
	int rc;
	/* Check that all fields in reg are known constant at build time */
	BUILD_BUG_ON(!const_true(reg.addr));
	BUILD_BUG_ON(reg.items != 1);
	switch (reg.bits) {
	case 8:
		rc = zl_read_reg8(zldev, reg.addr, value);
	...
	case 48:
		rc = zl_read_reg48(zldev, reg.addr, value);
	}
	return rc;
}

static inline int zl_read_idx_reg(..., const struct zl_reg reg,
				  unsigned int idx, void *value)
{
	unsigned int addr;
	int rc;
	/* Check that all fields in reg are known constant at build time */
	BUILD_BUG_ON(!const_true(reg.addr));
	BUILD_BUG_ON(reg.items > 1);
	
	if (idx >= reg.items)
		return -EINVAL;

	addr = reg.addr + (idx * reg.stride);
	switch (reg.bits) {
	case 8:
		rc = zl_read_reg8(zldev, addr, value);
	...
	case 48:
		rc = zl_read_reg48(zldev, addr, value);
	}
	return rc;
}

// page 0, offset 0x01, bits 8
ZL_DEF_REG(id, ZL_REG_ADDR(0, 0x01, 8);

// page 2, offset 0x55, bits 16, 10 items, 2 bytes between them
ZL_DEF_IDX_REG(ref_config, ZL_REG_ADDR(2, 0x55, 16, 10, 2))

Caller:
rc = zl_read_reg(zldev, id, &val);
rc = zl_read_idx_reg(zldev, ref_config, 5, &val);

Passing constant structures ensures that all 'reg' fields are known
during compilation and zl_read_*reg() functions are rendered into tiny 
assembler output.

WDYT?

Ivan