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

[Binbin Zhou <zhoubinbin@loongson.cn>]

Hi Andy:

在 2022/11/28 21:24, Andy Shevchenko 写道:
> 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.
OK, I get it.
>
> ...
>
>>>> +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.

Are you saying that the "is_read" variable in ls2x_i2c_xfer_one() 
already indicates the read/write state of data transfer?

I just didn't think it was necessary to take "is_read" as an argument to 
ls2x_i2c_start() at the time, since we could get it from "msg".

Thanks.

Binbin

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