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

Alexander Stein
2022-09-28T07:36:56Z

Hi Marek,

thanks for the update.

Am Dienstag, 27. September 2022, 23:44:14 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>
> ---
> V2: Fix endianness handling in rs9_regmap_i2c_read() i2c_transfer
> V3: - Disable regcache, the driver does a couple of I2C writes on boot
> and that is all, so it only adds complexity
> - Set regmap max_register to RS9_REG_BCP which is the correct one
> ---
> 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 | 60 ++++++++++++++++++++++++++++++++--
> 1 file changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index 4f5df1fc74b46..138aaab05fc8a 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -90,13 +90,61 @@ 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 txdata = reg;
> + u8 rxdata[2];
> + int ret;
> +
> + xfer[0].addr = i2c->addr;
> + xfer[0].flags = 0;
> + xfer[0].len = 1;
> + xfer[0].buf = (void *)&txdata;
> +
> + xfer[1].addr = i2c->addr;
> + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN;
> + xfer[1].len = 1;

I'm still in favor of removing I2C_M_RECV_LEN and setting len=2. This
currently works only, because there is no read access between
devm_regmap_init() call and regmap_write() to RS9_REG_BCP.
Enabling cache later on again this will corrupt the stack.

Best regards,
Alexander

> + xfer[1].buf = (void *)rxdata;
> +
> + ret = i2c_transfer(i2c->adapter, xfer, 2);
> + if (ret < 0)
> + return ret;
> + if (ret != 2)
> + return -EIO;
> +
> + *val = rxdata[1];
> + return 0;
> +}
> +
> static const struct regmap_config rs9_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> - .cache_type = REGCACHE_FLAT,
> - .max_register = 0x8,
> + .cache_type = REGCACHE_NONE,
> + .max_register = RS9_REG_BCP,
> .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 +290,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);