Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Nov 28, 2022 at 08:03:40PM +0800, Binbin Zhou wrote:
> 在 2022/11/25 18:41, Andy Shevchenko 写道:
> > On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:

...

> > Missing bits.h.
> 
> Is it needed? I found it already included in I2c.h.

The rule of thumb is to include headers we are the direct user of.
Exceptions are the headers that are guaranteed to be included by
others. i2c.h doesn't guarantee this and many other inclusions
while using them for itself or simply included them wrongly.

...

> > > +struct ls2x_i2c_dev {
> > > +	struct device		*dev;
> > > +	void __iomem		*base;
> > > +	int			irq;
> > > +	u32			bus_clk_rate;
> > > +	struct completion	cmd_complete;
> > > +	struct i2c_adapter	adapter;
> > Check if moving this to be the first field makes code generation better
> > (bloat-o-meter is your friend).
> 
> vmlinux.old: original order
> 
> vmlinux:  adapter to be the first field
> 
> [zhoubinbin@kernelserver github]$ scripts/bloat-o-meter vmlinux.old vmlinux
> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-8 (-8)
> Function                                     old     new   delta
> ls2x_i2c_remove                               36      32      -4
> ls2x_i2c_probe                               424     420      -4
> 
> Total: Before=19302026, After=19302018, chg -0.00%

Good, up to you (personally I find usually better to have embedded structures,
which are parent objects in terms of OOP, to be first members).

> > > +	unsigned int		suspended:1;
> > > +};
> > > +	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
> > > +}

...

> > > +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
> > > +{
> > > +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
> > > +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
> > > +
> > > +	reinit_completion(&dev->cmd_complete);
> > > +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
> > Why is this needed?
> In the ls2x I2C controller, the bit 0 of TXR indicates the read/write status
> when transferring the address.

Yes, I understand this. I don't understand why do you need this twice.

> > > +	writeb(addr, dev->base + I2C_LS2X_TXR);
> > > +
> > > +	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
> > > +}

...

> > > +	for (retry = 0; retry < adap->retries; retry++) {
> > > +
> > Unneeded blank line.
> > 
> > > +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> > > +		if (ret != -EAGAIN)
> > > +			return ret;
> > > +
> > > +		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
> > > +		udelay(100);
> > > +	}
> > Can something from iopoll.h be utilized here?
> I think udelay() should be suitable because it is just the time interval
> between two retry.

Read again my comment. Also thanks for pointing out that this is atomic. Is
this really needs to be atomic?

...

> > > +	/*
> > > +	 * The I2C controller has a fixed I2C bus frequency by default, but to
> > > +	 * be compatible with more client devices, we can obtain the set I2C
> > > +	 * bus frequency from ACPI or FDT.
> > > +	 */
> > > +	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
> > > +	if (!dev->bus_clk_rate)
> > > +		device_property_read_u32(&pdev->dev, "clock-frequency",
> > > +					&dev->bus_clk_rate);
> > This should be done via

> >          i2c_parse_fw_timings(&pdev->dev, ...);

> > no?
> 
> Yes, I get it，and the i2c_ls2x_adjust_bus_speed() function will be
> introduced to calculate i2c bus_freq_hz.

Yep, depends on your needs. It might be that some of the drivers are using
the code that you may reuse (by moving to i2c-core-acpi.c).

-- 
With Best Regards,
Andy Shevchenko