Re: [PATCH] clk: rs9: Fix I2C accessors

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Marek,

thanks for the patch.

Am Sonntag, 11. September 2022, 01:20:15 CEST schrieb Marek Vasut:
> Add custom I2C accessors to this driver, since the regular I2C regmap ones
> do not generate the exact I2C transfers required by the chip. On I2C write,
> it is mandatory to send transfer length first, on read the chip returns the
> transfer length in first byte. Instead of always reading back 8 bytes, which
> is the default and also the size of the entire register file, set BCP
> register to 1 to read out 1 byte which is less wasteful.
> 
> Fixes: 892e0ddea1aa6 ("clk: rs9: Add Renesas 9-series PCIe clock generator
> driver") Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/clk/clk-renesas-pcie.c | 53 +++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index 4f5df1fc74b46..711ba443f33b7 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -90,6 +90,49 @@ static const struct regmap_access_table
> rs9_writeable_table = { .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges),
>  };
> 
> +static int rs9_regmap_i2c_write(void *context, unsigned int reg, unsigned
> int val) +{
> +	struct i2c_client *i2c = context;
> +	const u8 data[3] = { reg, 1, val };
> +	const int count = ARRAY_SIZE(data);
> +	int ret;
> +
> +	ret = i2c_master_send(i2c, data, count);
> +	if (ret == count)
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;
> +}
> +
> +static int rs9_regmap_i2c_read(void *context,
> +			       unsigned int reg, unsigned int *val)
> +{
> +	struct i2c_client *i2c = context;
> +	struct i2c_msg xfer[2];
> +	u8 data[2];
> +	int ret;
> +
> +	xfer[0].addr = i2c->addr;
> +	xfer[0].flags = 0;
> +	xfer[0].len = 1;
> +	xfer[0].buf = (void *)&reg;

This does work only for little-endian machines, right?

> +
> +	xfer[1].addr = i2c->addr;
> +	xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN;
> +	xfer[1].len = 1;
> +	xfer[1].buf = (void *)data;
> +
> +	ret = i2c_transfer(i2c->adapter, xfer, 2);
> +	if (ret == 2)
> +		return 0;

You are missing setting 'val' here.

> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;
> +}
> +
>  static const struct regmap_config rs9_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -97,6 +140,8 @@ static const struct regmap_config rs9_regmap_config = {
>  	.max_register = 0x8,
>  	.rd_table = &rs9_readable_table,
>  	.wr_table = &rs9_writeable_table,
> +	.reg_write = rs9_regmap_i2c_write,
> +	.reg_read = rs9_regmap_i2c_read,
>  };
> 
>  static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
> @@ -242,11 +287,17 @@ static int rs9_probe(struct i2c_client *client)
>  			return ret;
>  	}
> 
> -	rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config);
> +	rs9->regmap = devm_regmap_init(&client->dev, NULL,
> +				       client, 
&rs9_regmap_config);
>  	if (IS_ERR(rs9->regmap))
>  		return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap),
>  				     "Failed to allocate register 
map\n");
> 
> +	/* Always read back 1 Byte via I2C */
> +	ret = regmap_write(rs9->regmap, RS9_REG_BCP, 1);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Register clock */
>  	for (i = 0; i < rs9->chip_info->num_clks; i++) {
>  		snprintf(name, 5, "DIF%d", i);


For some reason this doesn't work with cache being enabled. No idea why this 
happens though.

Best regards,
Alexander