Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

[Ivan Vecera <ivecera@redhat.com>]

On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote:
> On 09/04/2025 16:42, Ivan Vecera wrote:
>> Add several macros to access device registers. These macros
>> defines a couple of static inline functions to ease an access
>> device registers. There are two types of registers, the 1st type
>> is a simple one that is defined by an address and size and the 2nd
>> type is indexed register that is defined by base address, type,
>> number of register instances and address stride between instances.
>>
>> Examples:
>> __ZL3073X_REG_DEF(reg1, 0x1234, 4, u32);
>> __ZL3073X_REG_IDX_DEF(idx_reg2, 0x1234, 2, u16, 4, 0x10);
> 
> Why can't you use standard FIELD_ macros? Why inventing the wheel again?

This is not about FIELD_* macros replacement. This is an abstraction to 
access device registers in safe manner. Generated inline functions 
ensures that proper value or pointer to value type is passed by caller.
Also in case of arbitrary zl3073x_{read,write_{,idx}_reg() the does not 
need to know what is the register address.

If the caller just need to read regX or indexed regY it will call just

zl3073x_read_regX(..., &value);
zl3073x_read_regX(..., idx, &value);

instead of

zl3073x_read_reg(..., ZL3073x_REGX_ADDR, &value);
zl3073x_read_reg(..., ZL3073x_REGY_ADDR + (idx * ZL3073X_REGY_STRIDE), 
&value)

The 1st variant is additionally type safe, the caller is warned if it is 
passing u8 * instead of u32 *.

I tried to take similar approach the mlxsw driver took to access device 
registers.

If you are only against such macro usage for static inline functions 
generation, I can avoid them and pre-create them in separate include 
file like zl3073x_regs.h

>> this defines the following functions:
>> int zl3073x_read_reg1(struct zl3073x_dev *dev, u32 *value);
>> int zl3073x_write_reg1(struct zl3073x_dev *dev, u32 value);
>> int zl3073x_read_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>>                            u32 *value);
>> int zl3073x_write_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>>                             u32 value);
> 
> Do not copy code into commit msg. I asked about this last time. Explain
> why do you need it, why existing API is not good.

Will drop... I wanted only show how the macros work and what is their output

>>
>> There are also several shortcut macros to define registers with
>> certain bit widths: 8, 16, 32 and 48 bits.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
> 
> 
> ...
> 
>> + *
>> + * Note that these functions have to be called with the device lock
>> + * taken.
>> + */
>> +#define __ZL3073X_REG_IDX_DEF(_name, _addr, _len, _type, _num, _stride)	\
>> +typedef _type zl3073x_##_name##_t;					\
>> +static inline __maybe_unused						\
>> +int zl3073x_read_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
>> +			 _type * value)					\
>> +{									\
>> +	WARN_ON(idx >= (_num));						\
> 
> No need to cause panic reboots. Either review your code so this does not
> happen or properly handle.

Ack, will replace by

if (idx >= (_num))
	return -EINVAL

Thanks,
Ivan