Re: [PATCH v2 3/6] iio: adis: Add adis_update_bits() APIs

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Mar 2020 13:53:09 +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>

hmm. This feels a bit 'too clever' but hopefully we won't get any subtle
bugs due to wrong local variable types etc.

I'll pick this up when the rest of the series is ready.

Jonathan

> ---
> Changes in v2:
>  * Add BUILD_BUG_ON() to avoid invalid types.
> 
>  drivers/iio/imu/adis.c       | 26 ++++++++++++++++
>  include/linux/iio/imu/adis.h | 59 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 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 ed41c6b96d14..94031b3fc9d5 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -333,6 +333,65 @@ 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.
> + */
> +#define adis_update_bits(adis, reg, mask, val) ({			\
> +	BUILD_BUG_ON(sizeof(val) == 1 || sizeof(val) == 8);		\
> +	__builtin_choose_expr(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.
> + */
> +#define __adis_update_bits(adis, reg, mask, val) ({			\
> +	BUILD_BUG_ON(sizeof(val) == 1 || sizeof(val) == 8);		\
> +	__builtin_choose_expr(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);