linux-kernel - Re: [PATCH v8 7/7] i2c: Add driver for the RTL9300 I2C controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5b915330-ef77-418d-82b1-9d751f1c157e@alliedtelesis.co.nz>
Date: Wed, 6 Nov 2024 11:27:07 +1300
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, lee@...nel.org,
 sre@...nel.org, tsbogend@...ha.franken.de, linux-i2c@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v8 7/7] i2c: Add driver for the RTL9300 I2C controller


On 6/11/24 10:10, Andi Shyti wrote:
> Hi Chris,
>
> On Fri, Nov 01, 2024 at 09:03:50AM +1300, Chris Packham wrote:
>> Add support for the I2C controller on the RTL9300 SoC. There are two I2C
>> controllers in the RTL9300 that are part of the Ethernet switch register
>> block. Each of these controllers owns a SCL pin (GPIO8 for the fiorst
>> I2C controller, GPIO17 for the second). There are 8 possible SDA pins
>> (GPIO9-16) that can be assigned to either I2C controller. This
>> relationship is represented in the device tree with a child node for
>> each SDA line in use.
>>
>> This is based on the openwrt implementation[1] but has been
>> significantly modified
>>
>> [1] - https://scanmail.trustwave.com/?c=20988&d=2Imq58SgjkO2w5EzbSeL1kys6iYwJJIG5Ij2dyaU8A&u=https%3a%2f%2fgit%2eopenwrt%2eorg%2f%3fp%3dopenwrt%2fopenwrt%2egit%3ba%3dblob%3bf%3dtarget%2flinux%2frealtek%2ffiles-5%2e15%2fdrivers%2fi2c%2fbusses%2fi2c-rtl9300%2ec
>>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> Looks good. As a self reminder:
>
> Reviewed-by: Andi Shyti <andi.shyti@...nel.org>
>
> Some comments below, though.
>
> ...
>
>> +#define RTL9300_I2C_MST_CTRL1			0x0
>> +#define  RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS	8
>> +#define  RTL9300_I2C_MST_CTRL1_MEM_ADDR_MASK	GENMASK(31, 8)
>> +#define  RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_OFS	4
>> +#define  RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_MASK	GENMASK(6, 4)
>> +#define  RTL9300_I2C_MST_CTRL1_GPIO_SCL_SEL	BIT(3)
>> +#define  RTL9300_I2C_MST_CTRL1_RWOP		BIT(2)
>> +#define  RTL9300_I2C_MST_CTRL1_I2C_FAIL		BIT(1)
>> +#define  RTL9300_I2C_MST_CTRL1_I2C_TRIG		BIT(0)
>> +#define RTL9300_I2C_MST_CTRL2			0x4
>> +#define  RTL9300_I2C_MST_CTRL2_RD_MODE		BIT(15)
>> +#define  RTL9300_I2C_MST_CTRL2_DEV_ADDR_OFS	8
>> +#define  RTL9300_I2C_MST_CTRL2_DEV_ADDR_MASK	GENMASK(14, 8)
>> +#define  RTL9300_I2C_MST_CTRL2_DATA_WIDTH_OFS	4
>> +#define  RTL9300_I2C_MST_CTRL2_DATA_WIDTH_MASK	GENMASK(7, 4)
>> +#define  RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_OFS	2
>> +#define  RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_MASK	GENMASK(3, 2)
>> +#define  RTL9300_I2C_MST_CTRL2_SCL_FREQ_OFS	0
>> +#define  RTL9300_I2C_MST_CTRL2_SCL_FREQ_MASK	GENMASK(1, 0)
>> +#define RTL9300_I2C_MST_DATA_WORD0		0x8
>> +#define RTL9300_I2C_MST_DATA_WORD1		0xc
>> +#define RTL9300_I2C_MST_DATA_WORD2		0x10
>> +#define RTL9300_I2C_MST_DATA_WORD3		0x14
> Not everything here is perfectly aligned, but I'm not going to
> be too picky.
>
> ...

Since I'm making other changes I'll tidy up the alignment.

>> +static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
>> +				int size, union i2c_smbus_data *data, int len)
> You could align this a little better.

Ack.

>> +{
>> +	u32 val, mask;
>> +	int ret;
> ...
>
>> +static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
>> +				  char read_write, u8 command, int size,
>> +				  union i2c_smbus_data *data)
>> +{
>> +	struct rtl9300_i2c_chan *chan = i2c_get_adapdata(adap);
>> +	struct rtl9300_i2c *i2c = chan->i2c;
>> +	int len = 0, ret;
> ...
>
>> +	ret = rtl9300_i2c_execute_xfer(i2c, read_write, size, data, len);
> do we want to bail out if len is '0'?
>
> ...

I think it'll be OK. In the write case the len should be set via 
data->block[0]. I the read case we will pass len=0 which will cause 
rtl9300_i2c_execute_xfer() to grab all 16 bytes from the i2c controller 
registers but for the read block data the i2c device should provide the 
correct number bytes in byte 0.

>> +static int rtl9300_i2c_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rtl9300_i2c_chan *chan;
>> +	struct rtl9300_i2c *i2c;
>> +	struct i2c_adapter *adap;
> "chan" and "adap" can be declared inside
> the device_for_each_child_node()
Ack.
>> +	u32 clock_freq, sda_pin;
>> +	int ret, i = 0;
>> +	struct fwnode_handle *child;
> ...
>
>> +	device_for_each_child_node(dev, child) {
>> +		chan = &i2c->chans[i];
>> +		adap = &chan->adap;
>> +
> Thanks,
> Andi