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

[Krzysztof Kozlowski <krzk@kernel.org>]

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 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.

> 
> 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.

Best regards,
Krzysztof