Re: [PATCH v3 net-next 3/8] mfd: Add Microchip ZL3073x support
From: Ivan Vecera
Date: Thu Apr 17 2025 - 11:13:03 EST
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@xxxxxxx> 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