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);
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);
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>
---
include/linux/mfd/zl3073x.h | 100 ++++++++++++++++++++++++++++++++++++
1 file changed, 100 insertions(+)
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 00dcc73aeeb34..405a66a7b3e78 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -3,6 +3,7 @@
#ifndef __LINUX_MFD_ZL3073X_H
#define __LINUX_MFD_ZL3073X_H
+#include <linux/bug.h>
#include <linux/cleanup.h>
#include <linux/mutex.h>
@@ -53,4 +54,103 @@ int zl3073x_read_reg(struct zl3073x_dev *zldev, unsigned int reg,
int zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg,
unsigned int len, const void *value);
+/**
+ * __ZL3073X_REG_DEF - Define a device register helpers
+ * @_name: register name
+ * @_addr: register address
+ * @_len: size of register value in bytes
+ * @_type: type of register value
+ *
+ * The macro defines helper functions for particular device register
+ * to access it.
+ *
+ * Example:
+ * __ZL3073X_REG_DEF(sample_reg, 0x1234, 4, u32)
+ *
+ * generates static inline functions:
+ * int zl3073x_read_sample_reg(struct zl3073x_dev *dev, u32 *value);
+ * int zl3073x_write_sample_reg(struct zl3073x_dev *dev, u32 value);
+ *
+ * Note that these functions have to be called with the device lock
+ * taken.
+ */
+#define __ZL3073X_REG_DEF(_name, _addr, _len, _type) \
+typedef _type zl3073x_##_name##_t; \
+static inline __maybe_unused \
+int zl3073x_read_##_name(struct zl3073x_dev *zldev, _type * value) \
+{ \
+ return zl3073x_read_reg(zldev, _addr, _len, value); \
+} \
+static inline __maybe_unused \
+int zl3073x_write_##_name(struct zl3073x_dev *zldev, _type value) \
+{ \
+ return zl3073x_write_reg(zldev, _addr, _len, &value); \
+}
+
+/**
+ * __ZL3073X_REG_IDX_DEF - Define an indexed device register helpers
+ * @_name: register name
+ * @_addr: register address
+ * @_len: size of register value in bytes
+ * @_type: type of register value
+ * @_num: number of register instances
+ * @_stride: address stride between instances
+ *
+ * The macro defines helper functions for particular indexed device
+ * register to access it.
+ *
+ * Example:
+ * __ZL3073X_REG_IDX_DEF(sample_reg, 0x1234, 2, u16, 4, 0x10)
+ *
+ * generates static inline functions:
+ * int zl3073x_read_sample_reg(struct zl3073x_dev *dev, unsigned int idx,
+ * u32 *value);
+ * int zl3073x_write_sample_reg(struct zl3073x_dev *dev, unsigned int idx,
+ * u32 value);
+ *
+ * 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)); \
+ return zl3073x_read_reg(zldev, (_addr) + idx * (_stride), _len, \
+ value); \
+} \
+static inline __maybe_unused \
+int zl3073x_write_##_name(struct zl3073x_dev *zldev, unsigned int idx, \
+ _type value) \
+{ \
+ WARN_ON(idx >= (_num)); \
+ return zl3073x_write_reg(zldev, (_addr) + idx * (_stride), \
+ _len, &value); \
+}
+
+/*
+ * Add register definition shortcuts for 8, 16, 32 and 48 bits
+ */
+#define ZL3073X_REG8_DEF(_name, _addr) __ZL3073X_REG_DEF(_name, _addr, 1, u8)
+#define ZL3073X_REG16_DEF(_name, _addr) __ZL3073X_REG_DEF(_name, _addr, 2, u16)
+#define ZL3073X_REG32_DEF(_name, _addr) __ZL3073X_REG_DEF(_name, _addr, 4, u32)
+#define ZL3073X_REG48_DEF(_name, _addr) __ZL3073X_REG_DEF(_name, _addr, 6, u64)
+
+/*
+ * Add indexed register definition shortcuts for 8, 16, 32 and 48 bits
+ */
+#define ZL3073X_REG8_IDX_DEF(_name, _addr, _num, _stride) \
+ __ZL3073X_REG_IDX_DEF(_name, _addr, 1, u8, _num, _stride)
+
+#define ZL3073X_REG16_IDX_DEF(_name, _addr, _num, _stride) \
+ __ZL3073X_REG_IDX_DEF(_name, _addr, 2, u16, _num, _stride)
+
+#define ZL3073X_REG32_IDX_DEF(_name, _addr, _num, _stride) \
+ __ZL3073X_REG_IDX_DEF(_name, _addr, 4, u32, _num, _stride)
+
+#define ZL3073X_REG48_IDX_DEF(_name, _addr, _num, _stride) \
+ __ZL3073X_REG_IDX_DEF(_name, _addr, 6, u64, _num, _stride)
+
#endif /* __LINUX_MFD_ZL3073X_H */
--
2.48.1On 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
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
On Thu, Apr 10, 2025 at 11:21 AM Ivan Vecera <ivecera@redhat.com> wrote: > On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote: > > On 09/04/2025 16:42, Ivan Vecera wrote: ... > >> + 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 If these functions are called under regmap_read() / regmap_write() the above is a dead code. Otherwise you need to configure regmap correctly (in accordance with the HW registers layout and their abilities to be written or read or reserved or special combinations). -- With Best Regards, Andy Shevchenko
On 10. 04. 25 7:53 odp., Andy Shevchenko wrote:
> On Thu, Apr 10, 2025 at 11:21 AM Ivan Vecera <ivecera@redhat.com> wrote:
>> On 10. 04. 25 9:17 dop., Krzysztof Kozlowski wrote:
>>> On 09/04/2025 16:42, Ivan Vecera wrote:
>
> ...
>
>>>> + 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
>
> If these functions are called under regmap_read() / regmap_write() the
> above is a dead code. Otherwise you need to configure regmap correctly
> (in accordance with the HW registers layout and their abilities to be
> written or read or reserved or special combinations).
>
Hi Andy,
these functions are not called under regmap_{read,write} but above. Some
(non-mailboxed) registers are indexed.
E.g. register ref_freq is defined as 4-bytes for 10 input references:
ref0: address 0x144-0x147
ref1: address 0x148-0x14b
...
So the caller (this mfd driver or dpll driver) will access it as:
zl3073x_read_ref_freq(zldev, idx, &value);
and this helper then computes the addr value according base (0x144) and
provided index with ref number and then calls regmap_*read().
And the 'if' check is just for a sanity check that the caller does not
provide wrong index and if so return -EINVAL.
I.
© 2016 - 2025 Red Hat, Inc.