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

[Marek Vasut <marex@denx.de>]

On 9/28/22 09:35, Alexander Stein wrote:
> Hi Marek,

Hi,

> thanks for the update.

Thanks for the reviews.

> 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.

The device does send you the amount of data in first byte of I2C READ, 
so I think I2C_M_RECV_LEN is the right flag here.

I had a look into the above topic too before sending V3, and I think you 
can use regcache_cache_bypass(..., true) right after devm_regmap_init() 
and then again regcache_cache_bypass(..., false) close to the end of 
probe() callback to deal with the problem you're describing.

Would that work ?