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: <x2ptxjqmcui6uh6qhjur5bymb6cjgikekt7bo2bnyoqbble4kh@zanxmiq3sau6>
Date: Tue, 5 Nov 2024 22:10:39 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>
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

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://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
> 
> 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.

...

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

> +{
> +	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'?

...

> +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()

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