| Message ID | 20200225124152.270914-4-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support ADIS16475 and similar IMUs | expand |
On Tue, 25 Feb 2020 13:41:50 +0100 Nuno Sá <nuno.sa@analog.com> wrote: > This patch adds a `regmap_update_bits()` like API to the ADIS library. > It provides locked and unlocked variant. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> Mostly fine, but I wonder if we can avoid the need to have comments on handling of 1 and 8 byte values by explicitly avoiding them happening. Thanks, Jonathan > --- > drivers/iio/imu/adis.c | 26 +++++++++++++++ > include/linux/iio/imu/adis.h | 61 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > index a8afd01de4f3..fa0ee35d96f0 100644 > --- a/drivers/iio/imu/adis.c > +++ b/drivers/iio/imu/adis.c > @@ -223,6 +223,32 @@ int __adis_read_reg(struct adis *adis, unsigned int reg, > return ret; > } > EXPORT_SYMBOL_GPL(__adis_read_reg); > +/** > + * __adis_update_bits_base() - ADIS Update bits function - Unlocked version > + * @adis: The adis device > + * @reg: The address of the lower of the two registers > + * @mask: Bitmask to change > + * @val: Value to be written > + * @size: Size of the register to update > + * > + * Updates the desired bits of @reg in accordance with @mask and @val. > + */ > +int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask, > + const u32 val, u8 size) > +{ > + int ret; > + u32 __val; > + > + ret = __adis_read_reg(adis, reg, &__val, size); > + if (ret) > + return ret; > + > + __val &= ~mask; > + __val |= val & mask; > + > + return __adis_write_reg(adis, reg, __val, size); > +} > +EXPORT_SYMBOL_GPL(__adis_update_bits_base); > > #ifdef CONFIG_DEBUG_FS > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > index b4c35d137e2a..07073f698718 100644 > --- a/include/linux/iio/imu/adis.h > +++ b/include/linux/iio/imu/adis.h > @@ -303,6 +303,67 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg, > return ret; > } > > +int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask, > + const u32 val, u8 size); > +/** > + * adis_update_bits_base() - ADIS Update bits function - Locked version > + * @adis: The adis device > + * @reg: The address of the lower of the two registers > + * @mask: Bitmask to change > + * @val: Value to be written > + * @size: Size of the register to update > + * > + * Updates the desired bits of @reg in accordance with @mask and @val. > + */ > +static inline int adis_update_bits_base(struct adis *adis, unsigned int reg, > + const u32 mask, const u32 val, u8 size) > +{ > + int ret; > + > + mutex_lock(&adis->state_lock); > + ret = __adis_update_bits_base(adis, reg, mask, val, size); > + mutex_unlock(&adis->state_lock); > + return ret; > +} > + > +/** > + * adis_update_bits() - Wrapper macro for adis_update_bits_base - Locked version > + * @adis: The adis device > + * @reg: The address of the lower of the two registers > + * @mask: Bitmask to change > + * @val: Value to be written > + * > + * This macro evaluates the sizeof of @val at compile time and calls > + * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for > + * @val can lead to undesired behavior if the register to update is 16bit. Also > + * note that a 64bit value will be treated as an integer. In the same way, > + * a char is seen as a short. Are these 'edge' conditions desirable? If not can we use the compile time checking tricks to trigger a build failure if they occur? BUILD_BUG_ON(sizeof(val) == 1) etc. > + */ > +#define adis_update_bits(adis, reg, mask, val) ({ \ > + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ > + adis_update_bits_base(adis, reg, mask, val, 4), \ > + adis_update_bits_base(adis, reg, mask, val, 2)); \ > +}) > + > +/** > + * adis_update_bits() - Wrapper macro for adis_update_bits_base > + * @adis: The adis device > + * @reg: The address of the lower of the two registers > + * @mask: Bitmask to change > + * @val: Value to be written > + * > + * This macro evaluates the sizeof of @val at compile time and calls > + * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for > + * @val can lead to undesired behavior if the register to update is 16bit. Also > + * note that a 64bit value will be treated as an integer. In the same way, > + * a char is seen as a short. > + */ > +#define __adis_update_bits(adis, reg, mask, val) ({ \ > + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ > + __adis_update_bits_base(adis, reg, mask, val, 4), \ > + __adis_update_bits_base(adis, reg, mask, val, 2)); \ > +}) > + > int adis_enable_irq(struct adis *adis, bool enable); > int __adis_check_status(struct adis *adis); > int __adis_initial_startup(struct adis *adis);
On Tue, 2020-03-03 at 20:48 +0000, Jonathan Cameron wrote: > On Tue, 25 Feb 2020 13:41:50 +0100 > Nuno Sá <nuno.sa@analog.com> wrote: > > > This patch adds a `regmap_update_bits()` like API to the ADIS > > library. > > It provides locked and unlocked variant. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > Mostly fine, but I wonder if we can avoid the need to have comments > on handling of 1 and 8 byte values by explicitly avoiding them > happening. > > Thanks, > > Jonathan > > > --- > > drivers/iio/imu/adis.c | 26 +++++++++++++++ > > include/linux/iio/imu/adis.h | 61 > > ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 87 insertions(+) > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > > index a8afd01de4f3..fa0ee35d96f0 100644 > > --- a/drivers/iio/imu/adis.c > > +++ b/drivers/iio/imu/adis.c > > @@ -223,6 +223,32 @@ int __adis_read_reg(struct adis *adis, > > unsigned int reg, > > return ret; > > } > > EXPORT_SYMBOL_GPL(__adis_read_reg); > > +/** > > + * __adis_update_bits_base() - ADIS Update bits function - > > Unlocked version > > + * @adis: The adis device > > + * @reg: The address of the lower of the two registers > > + * @mask: Bitmask to change > > + * @val: Value to be written > > + * @size: Size of the register to update > > + * > > + * Updates the desired bits of @reg in accordance with @mask and > > @val. > > + */ > > +int __adis_update_bits_base(struct adis *adis, unsigned int reg, > > const u32 mask, > > + const u32 val, u8 size) > > +{ > > + int ret; > > + u32 __val; > > + > > + ret = __adis_read_reg(adis, reg, &__val, size); > > + if (ret) > > + return ret; > > + > > + __val &= ~mask; > > + __val |= val & mask; > > + > > + return __adis_write_reg(adis, reg, __val, size); > > +} > > +EXPORT_SYMBOL_GPL(__adis_update_bits_base); > > > > #ifdef CONFIG_DEBUG_FS > > > > diff --git a/include/linux/iio/imu/adis.h > > b/include/linux/iio/imu/adis.h > > index b4c35d137e2a..07073f698718 100644 > > --- a/include/linux/iio/imu/adis.h > > +++ b/include/linux/iio/imu/adis.h > > @@ -303,6 +303,67 @@ static inline int adis_read_reg_32(struct adis > > *adis, unsigned int reg, > > return ret; > > } > > > > +int __adis_update_bits_base(struct adis *adis, unsigned int reg, > > const u32 mask, > > + const u32 val, u8 size); > > +/** > > + * adis_update_bits_base() - ADIS Update bits function - Locked > > version > > + * @adis: The adis device > > + * @reg: The address of the lower of the two registers > > + * @mask: Bitmask to change > > + * @val: Value to be written > > + * @size: Size of the register to update > > + * > > + * Updates the desired bits of @reg in accordance with @mask and > > @val. > > + */ > > +static inline int adis_update_bits_base(struct adis *adis, > > unsigned int reg, > > + const u32 mask, const u32 val, > > u8 size) > > +{ > > + int ret; > > + > > + mutex_lock(&adis->state_lock); > > + ret = __adis_update_bits_base(adis, reg, mask, val, size); > > + mutex_unlock(&adis->state_lock); > > + return ret; > > +} > > + > > +/** > > + * adis_update_bits() - Wrapper macro for adis_update_bits_base - > > Locked version > > + * @adis: The adis device > > + * @reg: The address of the lower of the two registers > > + * @mask: Bitmask to change > > + * @val: Value to be written > > + * > > + * This macro evaluates the sizeof of @val at compile time and > > calls > > + * adis_update_bits_base() accordingly. Be aware that using > > MACROS/DEFINES for > > + * @val can lead to undesired behavior if the register to update > > is 16bit. Also > > + * note that a 64bit value will be treated as an integer. In the > > same way, > > + * a char is seen as a short. > > Are these 'edge' conditions desirable? If not can we use the compile > time checking tricks to trigger a build failure if they occur? > BUILD_BUG_ON(sizeof(val) == 1) etc. So, I guess there's no arm in the 'edge' conditions if users know what they are doing :). But I have no problems in making/forcing the right types by adding the compile time checks... Will add it in v2 - Nuno Sá > > + */ > > +#define adis_update_bits(adis, reg, mask, val) ({ > > \ > > + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ > > + adis_update_bits_base(adis, reg, mask, val, > > 4), \ > > + adis_update_bits_base(adis, reg, mask, val, 2)); \ > > +}) > > + > > +/** > > + * adis_update_bits() - Wrapper macro for adis_update_bits_base > > + * @adis: The adis device > > + * @reg: The address of the lower of the two registers > > + * @mask: Bitmask to change > > + * @val: Value to be written > > + * > > + * This macro evaluates the sizeof of @val at compile time and > > calls > > + * adis_update_bits_base() accordingly. Be aware that using > > MACROS/DEFINES for > > + * @val can lead to undesired behavior if the register to update > > is 16bit. Also > > + * note that a 64bit value will be treated as an integer. In the > > same way, > > + * a char is seen as a short. > > + */ > > +#define __adis_update_bits(adis, reg, mask, val) ({ > > \ > > + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ > > + __adis_update_bits_base(adis, reg, mask, val, 4), \ > > + __adis_update_bits_base(adis, reg, mask, val, 2)); \ > > +}) > > + > > int adis_enable_irq(struct adis *adis, bool enable); > > int __adis_check_status(struct adis *adis); > > int __adis_initial_startup(struct adis *adis);
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c index a8afd01de4f3..fa0ee35d96f0 100644 --- a/drivers/iio/imu/adis.c +++ b/drivers/iio/imu/adis.c @@ -223,6 +223,32 @@ int __adis_read_reg(struct adis *adis, unsigned int reg, return ret; } EXPORT_SYMBOL_GPL(__adis_read_reg); +/** + * __adis_update_bits_base() - ADIS Update bits function - Unlocked version + * @adis: The adis device + * @reg: The address of the lower of the two registers + * @mask: Bitmask to change + * @val: Value to be written + * @size: Size of the register to update + * + * Updates the desired bits of @reg in accordance with @mask and @val. + */ +int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask, + const u32 val, u8 size) +{ + int ret; + u32 __val; + + ret = __adis_read_reg(adis, reg, &__val, size); + if (ret) + return ret; + + __val &= ~mask; + __val |= val & mask; + + return __adis_write_reg(adis, reg, __val, size); +} +EXPORT_SYMBOL_GPL(__adis_update_bits_base); #ifdef CONFIG_DEBUG_FS diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h index b4c35d137e2a..07073f698718 100644 --- a/include/linux/iio/imu/adis.h +++ b/include/linux/iio/imu/adis.h @@ -303,6 +303,67 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg, return ret; } +int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask, + const u32 val, u8 size); +/** + * adis_update_bits_base() - ADIS Update bits function - Locked version + * @adis: The adis device + * @reg: The address of the lower of the two registers + * @mask: Bitmask to change + * @val: Value to be written + * @size: Size of the register to update + * + * Updates the desired bits of @reg in accordance with @mask and @val. + */ +static inline int adis_update_bits_base(struct adis *adis, unsigned int reg, + const u32 mask, const u32 val, u8 size) +{ + int ret; + + mutex_lock(&adis->state_lock); + ret = __adis_update_bits_base(adis, reg, mask, val, size); + mutex_unlock(&adis->state_lock); + return ret; +} + +/** + * adis_update_bits() - Wrapper macro for adis_update_bits_base - Locked version + * @adis: The adis device + * @reg: The address of the lower of the two registers + * @mask: Bitmask to change + * @val: Value to be written + * + * This macro evaluates the sizeof of @val at compile time and calls + * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for + * @val can lead to undesired behavior if the register to update is 16bit. Also + * note that a 64bit value will be treated as an integer. In the same way, + * a char is seen as a short. + */ +#define adis_update_bits(adis, reg, mask, val) ({ \ + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ + adis_update_bits_base(adis, reg, mask, val, 4), \ + adis_update_bits_base(adis, reg, mask, val, 2)); \ +}) + +/** + * adis_update_bits() - Wrapper macro for adis_update_bits_base + * @adis: The adis device + * @reg: The address of the lower of the two registers + * @mask: Bitmask to change + * @val: Value to be written + * + * This macro evaluates the sizeof of @val at compile time and calls + * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for + * @val can lead to undesired behavior if the register to update is 16bit. Also + * note that a 64bit value will be treated as an integer. In the same way, + * a char is seen as a short. + */ +#define __adis_update_bits(adis, reg, mask, val) ({ \ + __builtin_choose_expr(sizeof(val) == 8 || sizeof(val) == 4, \ + __adis_update_bits_base(adis, reg, mask, val, 4), \ + __adis_update_bits_base(adis, reg, mask, val, 2)); \ +}) + int adis_enable_irq(struct adis *adis, bool enable); int __adis_check_status(struct adis *adis); int __adis_initial_startup(struct adis *adis);
This patch adds a `regmap_update_bits()` like API to the ADIS library. It provides locked and unlocked variant. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/imu/adis.c | 26 +++++++++++++++ include/linux/iio/imu/adis.h | 61 ++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)