Re: [PATCH 4/8] i2c: rtl9300: introduce a property for 8 bit width reg address

[Rustam Adilov <adilov@disroot.org>]

Hello,
On 2026-03-15 21:31, Chris Packham wrote:
> Hi Rustam,
> 
> (sorry I might have prematurely hit send on an earlier reply)
> 
> On 14/03/2026 21:26, Rustam Adilov wrote:
>> In RTL9607C i2c controller, in order to indicate that the width of
>> memory address is 8 bits, 0 is written to MEM_ADDR_WIDTH field as
>> opposed to 1 for RTL9300 and RTL9310.
>>
>> Introduce a new property to a driver data to indicate what value
>> need to written to MEM_ADDR_WIDTH field for this case.
>>
>> Signed-off-by: Rustam Adilov <adilov@disroot.org>
>> ---
>>   drivers/i2c/busses/i2c-rtl9300.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
>> index 2525b57a9d03..86a82f2c3ce0 100644
>> --- a/drivers/i2c/busses/i2c-rtl9300.c
>> +++ b/drivers/i2c/busses/i2c-rtl9300.c
>> @@ -60,6 +60,7 @@ struct rtl9300_i2c_drv_data {
>>   	u32 wd_reg;
>>   	u8 max_nchan;
>>   	u8 max_data_len;
>> +	u8 reg_addr_8bit_len;
>>   };
>>   
>>   #define RTL9300_I2C_MUX_NCHAN	8
>> @@ -105,6 +106,7 @@ struct rtl9300_i2c_xfer {
>>   #define RTL9300_I2C_MST_DATA_WORD2			0x10
>>   #define RTL9300_I2C_MST_DATA_WORD3			0x14
>>   #define RTL9300_I2C_MST_GLB_CTRL			0x384
>> +#define RTL9300_REG_ADDR_8BIT_LEN			1
>>   
>>   #define RTL9310_I2C_MST_IF_CTRL				0x1004
>>   #define RTL9310_I2C_MST_IF_SEL				0x1008
>> @@ -299,6 +301,7 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>>   				  union i2c_smbus_data *data)
>>   {
>>   	struct rtl9300_i2c_chan *chan = i2c_get_adapdata(adap);
>> +	const struct rtl9300_i2c_drv_data *drv_data;
>>   	struct rtl9300_i2c *i2c = chan->i2c;
>>   	struct rtl9300_i2c_xfer xfer = {0};
>>   	int ret;
>> @@ -308,6 +311,7 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>>   
>>   	guard(rtl9300_i2c)(i2c);
>>   
>> +	drv_data = device_get_match_data(i2c->dev);
>>   	ret = rtl9300_i2c_config_chan(i2c, chan);
>>   	if (ret)
>>   		return ret;
>> @@ -315,7 +319,7 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>>   	xfer.dev_addr = addr & 0x7f;
>>   	xfer.write = (read_write == I2C_SMBUS_WRITE);
>>   	xfer.reg_addr = command;
>> -	xfer.reg_addr_len = 1;
>> +	xfer.reg_addr_len = drv_data->reg_addr_8bit_len;
> 
> For this one I wonder if we could come up with something that involves a 
> subtraction for the rtl9607? len = 1 just makes sense to me so maybe 
> RTL_9300_ADDR_LEN(1) which expands to 1 - 0 on the rtl9300 and 1 - 1 on 
> the rtl9607 would be easier to follow.

I am not entirely sure i understand this approach. Is RTL_9300_ADDR_LEN(1)
gonna be a "#define"? Would like to know before i proceed with possible
changes for v2.

And wouldn't something like this work just as well:

xfer.reg_addr_len = 1 - drv_data->subtract_len;

But then, i honestly think "drv_data->reg_addr_8bit_len" would be a bit less
cluttery?

> Not a deal breaker just thought I'd mention it.
> 
>>   
>>   	switch (size) {
>>   	case I2C_SMBUS_BYTE:
>> @@ -501,6 +505,7 @@ static const struct rtl9300_i2c_drv_data rtl9300_i2c_drv_data = {
>>   	.wd_reg = RTL9300_I2C_MST_DATA_WORD0,
>>   	.max_nchan = RTL9300_I2C_MUX_NCHAN,
>>   	.max_data_len = RTL9300_I2C_MAX_DATA_LEN,
>> +	.reg_addr_8bit_len = RTL9300_REG_ADDR_8BIT_LEN,
>>   };
>>   
>>   static const struct rtl9300_i2c_drv_data rtl9310_i2c_drv_data = {
>> @@ -524,6 +529,7 @@ static const struct rtl9300_i2c_drv_data rtl9310_i2c_drv_data = {
>>   	.wd_reg = RTL9310_I2C_MST_DATA_CTRL,
>>   	.max_nchan = RTL9310_I2C_MUX_NCHAN,
>>   	.max_data_len = RTL9300_I2C_MAX_DATA_LEN,
>> +	.reg_addr_8bit_len = RTL9300_REG_ADDR_8BIT_LEN,
>>   };
>>   
>>   static const struct of_device_id i2c_rtl9300_dt_ids[] = {