Re: [PATCH v5 4/4] drivers/soc/litex: make 'litex_[set|get]_reg()' methods private

["Gabriel L. Somlo" <gsomlo@gmail.com>]

On Sun, Dec 27, 2020 at 11:13:20AM -0500, Gabriel Somlo wrote:
> The 'litex_[set|get]_reg()' methods use the 'reg_size' parameter to
> specify the width of the LiteX CSR (MMIO) register being accessed.
> 
> Since 'u64' is the widest data being supported, the value of 'reg_size'
> MUST be between 1 and sizeof(u64), which SHOULD be checked at runtime
> if these methods are publicly available for use by other LiteX device
> drivers.
> 
> At the same time, none of the existing (or foreseeable) LiteX device
> drivers have a need to access registers whose size is unknown during
> compilation. As such, all LiteX device drivers should use fixed-width
> accessor methods such as 'litex_[write|read][8|16|32|64]()'.
> 
> This patch renames 'litex_[set|get]_reg()' to '_litex_[set|get]_reg()',
> indicating that they should NOT be directly called from outside of
> the 'include/linux/litex.h' header file.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/soc/litex/Kconfig |  2 +-
>  include/linux/litex.h     | 35 ++++++++++++++++++++---------------
>  2 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> index 973f8d2fe1a7..b9b3d51ea7df 100644
> --- a/drivers/soc/litex/Kconfig
> +++ b/drivers/soc/litex/Kconfig
> @@ -11,7 +11,7 @@ config LITEX_SOC_CONTROLLER
>  	select LITEX
>  	help
>  	  This option enables the SoC Controller Driver which verifies
> -	  LiteX CSR access and provides common litex_get_reg/litex_set_reg
> +	  LiteX CSR access and provides common litex_[read|write]*
>  	  accessors.
>  	  All drivers that use functions from litex.h must depend on
>  	  LITEX.
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> index 3456d527f644..14748fa86a99 100644
> --- a/include/linux/litex.h
> +++ b/include/linux/litex.h
> @@ -59,21 +59,25 @@ static inline u32 _read_litex_subregister(void __iomem *addr)
>  	((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
>  
>  /*
> - * The purpose of `litex_set_reg`/`litex_get_reg` is to implement the logic
> - * of writing to/reading from the LiteX CSR in a single place that can be
> - * then reused by all LiteX drivers.
> + * The purpose of `_litex_[set|get]_reg()` is to implement the logic of
> + * writing to/reading from the LiteX CSR in a single place that can be then
> + * reused by all LiteX drivers via the `litex_[write|read][8|16|32|64]()`
> + * accessors for the appropriate data width.
> + * NOTE: direct use of `_litex_[set|get]_reg()` by LiteX drivers is strongly
> + * discouraged, as they perform no error checking on the requested data width!
>   */
>  
>  /**
> - * litex_set_reg() - Writes the value to the LiteX CSR (Control&Status Register)
> + * _litex_set_reg() - Writes a value to the LiteX CSR (Control&Status Register)
>   * @reg: Address of the CSR
>   * @reg_size: The width of the CSR expressed in the number of bytes
>   * @val: Value to be written to the CSR
>   *
>   * This function splits a single (possibly multi-byte) LiteX CSR write into
>   * a series of subregister writes with a proper offset.
> + * NOTE: caller is responsible for ensuring (0 < reg_size < sizeof(u64)).

This will be fixed in v6 to read "(0 < reg_size <= sizeof(u64))".

>   */
> -static inline void litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
> +static inline void _litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
>  {
>  	u8 shift = _litex_num_subregs(reg_size) * LITEX_SUBREG_SIZE_BIT;
>  
> @@ -85,7 +89,7 @@ static inline void litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
>  }
>  
>  /**
> - * litex_get_reg() - Reads the value of the LiteX CSR (Control&Status Register)
> + * _litex_get_reg() - Reads a value of the LiteX CSR (Control&Status Register)
>   * @reg: Address of the CSR
>   * @reg_size: The width of the CSR expressed in the number of bytes
>   *
> @@ -93,8 +97,9 @@ static inline void litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
>   *
>   * This function generates a series of subregister reads with a proper offset
>   * and joins their results into a single (possibly multi-byte) LiteX CSR value.
> + * NOTE: caller is responsible for ensuring (0 < reg_size < sizeof(u64)).

Ditto.

Thanks,
--Gabriel

>   */
> -static inline u64 litex_get_reg(void __iomem *reg, size_t reg_size)
> +static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
>  {
>  	u64 r;
>  	u8 i;
> @@ -110,42 +115,42 @@ static inline u64 litex_get_reg(void __iomem *reg, size_t reg_size)
>  
>  static inline void litex_write8(void __iomem *reg, u8 val)
>  {
> -	litex_set_reg(reg, sizeof(u8), val);
> +	_litex_set_reg(reg, sizeof(u8), val);
>  }
>  
>  static inline void litex_write16(void __iomem *reg, u16 val)
>  {
> -	litex_set_reg(reg, sizeof(u16), val);
> +	_litex_set_reg(reg, sizeof(u16), val);
>  }
>  
>  static inline void litex_write32(void __iomem *reg, u32 val)
>  {
> -	litex_set_reg(reg, sizeof(u32), val);
> +	_litex_set_reg(reg, sizeof(u32), val);
>  }
>  
>  static inline void litex_write64(void __iomem *reg, u64 val)
>  {
> -	litex_set_reg(reg, sizeof(u64), val);
> +	_litex_set_reg(reg, sizeof(u64), val);
>  }
>  
>  static inline u8 litex_read8(void __iomem *reg)
>  {
> -	return litex_get_reg(reg, sizeof(u8));
> +	return _litex_get_reg(reg, sizeof(u8));
>  }
>  
>  static inline u16 litex_read16(void __iomem *reg)
>  {
> -	return litex_get_reg(reg, sizeof(u16));
> +	return _litex_get_reg(reg, sizeof(u16));
>  }
>  
>  static inline u32 litex_read32(void __iomem *reg)
>  {
> -	return litex_get_reg(reg, sizeof(u32));
> +	return _litex_get_reg(reg, sizeof(u32));
>  }
>  
>  static inline u64 litex_read64(void __iomem *reg)
>  {
> -	return litex_get_reg(reg, sizeof(u64));
> +	return _litex_get_reg(reg, sizeof(u64));
>  }
>  
>  #endif /* _LINUX_LITEX_H */
> -- 
> 2.26.2
>