Re: [PATCH 2/9] clk: si5341: Wait for DEVICE_READY on startup

[Mike Looijmans <mike.looijmans@topic.nl>]

One remark below.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 11-03-2021 23:24, Robert Hancock wrote:
> The Si5341 datasheet warns that before accessing any other registers,
> including the PAGE register, we need to wait for the DEVICE_READY register
> to indicate the device is ready, or the process of the device loading its
> state from NVM can be corrupted. Wait for DEVICE_READY on startup before
> continuing initialization. This is done using a raw I2C register read
> prior to setting up regmap to avoid any potential unwanted automatic PAGE
> register accesses from regmap at this stage.
>
> Fixes: 3044a860fd ("clk: Add Si5341/Si5340 driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   drivers/clk/clk-si5341.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index e0446e66fa64..f210860fb96e 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -94,6 +94,7 @@ struct clk_si5341_output_config {
>   #define SI5341_STATUS		0x000C
>   #define SI5341_SOFT_RST		0x001C
>   #define SI5341_IN_SEL		0x0021
> +#define SI5341_DEVICE_READY	0x00FE
>   #define SI5341_XAXB_CFG		0x090E
>   #define SI5341_IN_EN		0x0949
>   #define SI5341_INX_TO_PFD_EN	0x094A
> @@ -1189,6 +1190,31 @@ static const struct regmap_range_cfg si5341_regmap_ranges[] = {
>   	},
>   };
>   
> +static int si5341_wait_device_ready(struct i2c_client *client)
> +{
> +	int count;
> +
> +	/* Datasheet warns: Any attempt to read or write any register other
> +	 * than DEVICE_READY before DEVICE_READY reads as 0x0F may corrupt the
> +	 * NVM programming and may corrupt the register contents, as they are
> +	 * read from NVM. Note that this includes accesses to the PAGE register.
> +	 * Also: DEVICE_READY is available on every register page, so no page
> +	 * change is needed to read it.
> +	 * Do this outside regmap to avoid automatic PAGE register access.
> +	 */
> +	for (count = 0; count < 10; ++count) {
> +		s32 result = i2c_smbus_read_byte_data(client,
> +						      SI5341_DEVICE_READY);
> +		if (result < 0)
> +			return result;
> +		if (result == 0x0F)
> +			return 0;
> +		usleep_range(1000, 20000);
> +	}
> +	dev_err(&client->dev, "timeout waiting for DEVICE_READY\n");

The "timeout" here is random between 10 and 200 milliseconds.

The datasheet says that the device may take 300ms to initialize, so I 
guess 300 milliseconds would be a good timeout.

I'm also pretty sure there's a built-in kernel function to poll a 
register with timeout that you should use here.


> +	return -EIO;
> +}
> +
>   static const struct regmap_config si5341_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
> @@ -1385,6 +1411,11 @@ static int si5341_probe(struct i2c_client *client,
>   
>   	data->i2c_client = client;
>   
> +	/* Must be done before otherwise touching hardware */
> +	err = si5341_wait_device_ready(client);
> +	if (err)
> +		return err;
> +
>   	for (i = 0; i < SI5341_NUM_INPUTS; ++i) {
>   		input = devm_clk_get(&client->dev, si5341_input_clock_names[i]);
>   		if (IS_ERR(input)) {


-- 
Mike Looijmans