Re: [PATCH 2/2] i2c: add support for Socionext SynQuacer I2C controller

[Andy Shevchenko]

On Tue, Feb 20, 2018 at 8:04 PM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 20 February 2018 at 14:02, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> On Tue, Feb 20, 2018 at 11:08 AM, Ard Biesheuvel
>> <ard.biesheuvel@xxxxxxxxxx> wrote:

>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>
>> Shouldn't be // ?

> IIUC, this applies to .h files only, and /* */ is preferred for .c files.

Other way around.

Documentation/process/license-rules.rst

>>> +#define WAIT_PCLK(n, rate)     ndelay((((1000000000 +  (rate) - 1) / \
>>> +                                        (rate) + n - 1) / n) + 10)
>>
>> This split makes it harder to catch the calculus.
>> Also, you can use DIV_ROUND_UP(), though it longer, but adds a bit of
>> clarity to the calculus.

> Yeah. This was present in the original code, and I tried to avoid
> touching it :-)

Yeah, but below there are several instances with DIV_ROUND_UP().

>>> +static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
>>> +{
>>> +       dev_dbg(i2c->dev, "STOP\n");
>>
>> Hmm... Can't use FTRACE ?

> What do you mean?

The message kinda useless, esp. if you can enable functional tracing.
I saw a lot of debugging messages in the code, perhaps it makes sense
at some point to make some trace points in I2C core and leave only HW
related here.

>>> +       if (dev_of_node(&pdev->dev)) {
>>> +               i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>>> +               if (IS_ERR(i2c->clk)) {
>>> +                       dev_err(&pdev->dev, "cannot get clock\n");
>>> +                       return PTR_ERR(i2c->clk);
>>> +               }
>>> +               dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>>> +
>>> +               i2c->clkrate = clk_get_rate(i2c->clk);
>>> +               dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>>> +               clk_prepare_enable(i2c->clk);
>>> +       } else {
>>
>>> +               ret = device_property_read_u32(&pdev->dev,
>>> +                                              "socionext,pclk-rate",
>>> +                                              &i2c->clkrate);
>>
>> I suppose for ACPI we just register a fixed rate clock and use it in
>> the driver in the same way as in OF case.
>> I guess at some point we even can provide a generic clock provider for
>> ACPI based on rate property.

> Is there a question here? Do you want me to change anything?

Is it opener for discussion. At least in the drivers we have done for
x86 we do the way I described.

See, for example, drivers/mfd/intel-lpss.c

-- 
With Best Regards,
Andy Shevchenko