Re: [PATCH v2 1/3] mfd: intel-m10-bmc: add m10bmc_sys_update_bits API

[Lee Jones <lee.jones@linaro.org>]

On Mon, 25 Jul 2022, Tianfei Zhang wrote:

> Adds register access helper functions for M10BMC.
> m10bmc_raw_update_bits() uses to update m10bmc register
> bits per addr.
> m10bmc_sys_update_bits() uses to update m10bmc system
> register bits per offset.
> 
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  include/linux/mfd/intel-m10-bmc.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index f0044b14136e..8e434886a5a1 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -133,6 +133,8 @@ struct intel_m10bmc {
>   *
>   * m10bmc_raw_read - read m10bmc register per addr
>   * m10bmc_sys_read - read m10bmc system register per offset
> + * m10bmc_raw_update_bits - update m10bmc register bits per addr
> + * m10bmc_sys_update_bits - update m10bmc system register bits per offset
>   */
>  static inline int
>  m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> @@ -148,6 +150,20 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>  	return ret;
>  }
>  
> +static inline int
> +m10bmc_raw_update_bits(struct intel_m10bmc *m10bmc, unsigned int addr,
> +		       unsigned int msk, unsigned int val)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(m10bmc->regmap, addr, msk, val);
> +	if (ret)
> +		dev_err(m10bmc->dev, "fail to update reg bits %x: %d\n",
> +			addr, ret);
> +
> +	return ret;
> +}

I really do dislike these 'abstraction for the sake of abstraction'
layers.  Why can't you just use the Regmap API in-place?

>  /*
>   * The base of the system registers could be configured by HW developers, and
>   * in HW SPEC, the base is not added to the addresses of the system registers.
> @@ -158,5 +174,7 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>   */
>  #define m10bmc_sys_read(m10bmc, offset, val) \
>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> +#define m10bmc_sys_update_bits(m10bmc, offset, msk, val) \
> +	m10bmc_raw_update_bits(m10bmc, M10BMC_SYS_BASE + (offset), msk, val)
>  
>  #endif /* __MFD_INTEL_M10_BMC_H */

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog