[PATCH v10 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

[Christophe JAILLET]

Le 18/04/2023 à 12:09, Ryan Chen a écrit :
> Hello Christophe,
> 
>> Subject: Re: [PATCH v10 2/2] i2c: aspeed: support ast2600 i2c new register
>> mode driver
>>
>> (resending, my mail client removed some addresses. Sorry for the duplicated
>> message for the others)
>>
>>


>>   > +    dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
>>   > +        readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
>>   > +    if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
>> AST2600_I2CC_BUS_BUSY_STS) {
>>   > +        dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",
>>
>> recover?
> 
> Sorry, Do you mean modify the wording "Can’t recover bus"?

Yes.
English is not my native language but "Can't recovery bus" sounds 
strange to me.

>>   > +master_out:
>>   > +    if (i2c_bus->mode == DMA_MODE) {
>>   > +        kfree(i2c_bus->master_safe_buf);
>>   > +        i2c_bus->master_safe_buf = NULL;
>>
>> Alignement.
> 
> Sorry, I can't catch the alignment could you help point out or give me example?

2 tabs in front of kfree
1 tab + 4 spaces in front of i2c_bus->master_safe_buf

when tabs are configures as 8 spaces, it looks strange.

>>   > +
>>   > +    writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>>   > +    i2c_bus->slave = client;
>>   > +    /* Set slave addr. */
>>   > +    writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
>>   > +            i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
>>
>> Nit: alignement
> Sorry, what should I do for alignment.

1 tab in front of writel
3 tabs in front of i2c_bus->reg_base

when tabs are configures as 8 spaces, it looks strange.

Look at the writel just a lines below. These ones look nice :)

>>   > +    /* i2c timeout counter: use base clk4 1Mhz,
>>   > +     * per unit: 1/(1000/4096) = 4096us
>>   > +     */
>>   > +    ret = of_property_read_u32(dev->of_node,
>>   > +                            "i2c-scl-clk-low-timeout-us",
>>   > +                            &i2c_bus->timeout);
>>
>> Strange layout.
> 
> Sorry, could you point out the strange? It just property read for timeout.

"i2c-scl-clk-low-timeout-us" could fit on the previous line (but up to 
you to do it or not).

The 2 last lines are way to much on the right when tabs are 8 spaces.

>>
>>   > +    if (ret < 0)
>>   > +        i2c_bus->timeout = 0;
>>
>> Nit: This is not really needed since i2c_bus is kzalloc()'ed.
> 
> Got it, will remove it.

I would say that it is a matter of taste. If it improves reading, I 
think that it is fine as-is. If you want to save a few lines of code, it 
can be removed.


CJ