Re: [PATCH 10/11] boards: enclustra-sa2: configure SI5338

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, Sep 17, 2025 at 05:22:13PM +0200, David Picard wrote:
> Configure the SI5338 clock generator on the ST1 baseboard.
> 
> Signed-off-by: David Picard <david.picard@clermont.in2p3.fr>
> ---
>  arch/arm/boards/enclustra-sa2/Makefile             |   2 +-
>  .../boards/enclustra-sa2/Si5338-RevB-Registers.h   | 433 +++++++++++++++++++++
>  arch/arm/boards/enclustra-sa2/board.c              |   6 +
>  arch/arm/boards/enclustra-sa2/si5338_config.c      | 326 ++++++++++++++++
>  arch/arm/boards/enclustra-sa2/si5338_config.h      |  22 ++
>  arch/arm/mach-socfpga/Kconfig                      |   4 +
>  6 files changed, 792 insertions(+), 1 deletion(-)
> 
> +/**
> + * @brief Write a single byte to a register in the SI5338
> + * @param[in] dev The I²C device.
> + * @param[in] addr The register address.
> + * @param[in] data The byte to be written to the register.
> + * @return 0 on success, a negative value from `asm-generic/errno.h` on error.
> + */
> +static int i2c_write_simple(struct device *dev, u8 addr, u8 data)
> +{
> +	int ret;
> +	struct i2c_client *client;
> +
> +	client = to_i2c_client(dev);
> +	u8 buffer[2];
> +
> +	buffer[0] = addr;
> +	buffer[1] = data;
> +
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = client->addr,
> +			.buf = buffer,
> +			.len = 2,
> +			.flags = 0,
> +		}
> +	};
> +	debug("%s() >> dev addr = 0x%02x\n", __func__, client->addr);
> +
> +	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +	if (ret < 0) {
> +		printf("%s() >> ERROR: SI5338 write failed addr: %02x, data: %02x\n",
> +			   __func__, addr,
> +			   data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

This looks like it could greatly benefit from regmap. Look for
regmap_init_i2c(). It will give you most of the boiler plate here for
free.

The above will become regmap_write()

> +
> +/**
> + * @brief Change some bits in a register in the SI5338
> + * @param[in] dev The I²C device.
> + * @param[in] addr The register address.
> + * @param[in] data The byte to be written to the register.
> + * @param[in] mask Sets which bits in the register will change.
> + *
> + * The bits in the register are allowed to change if the corresponding bit in \a
> + *mask is 1.
> + *
> + * @return 0 on success, a negative value from `asm-generic/errno.h` on error.
> + */
> +static int i2c_write_masked(struct device *dev, u8 addr, u8 data, u8 mask)

Could be replaced by regmap_update_bits(), regmap_set_bits(),
regmap_clear_bits().

> +static int i2c_read_register(struct device *dev, u8 addr, u8 *data)

regmap_read()

> diff --git a/arch/arm/mach-socfpga/Kconfig b/arch/arm/mach-socfpga/Kconfig
> index fbd7d5c4abb1c6d598adf9209535f6f8fbd060e2..5060fc9b72af39d9e3d8ab3eca4b6fae37e62cef 100644
> --- a/arch/arm/mach-socfpga/Kconfig
> +++ b/arch/arm/mach-socfpga/Kconfig
> @@ -39,6 +39,10 @@ config MACH_SOCFPGA_ENCLUSTRA_SA2
>  	select BITREV
>  	bool "Enclustra SA2"
>  
> +config MACH_SOCFPGA_ENCLUSTRA_SA2_SI5338
> +	depends on MACH_SOCFPGA_ENCLUSTRA_SA2
> +	bool "Configure the SI5338 clock generator on ST1 baseboard"
> +

Why is this optional? Is this not available on all board variants?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |