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

[Ard Biesheuvel]

On 26 February 2018 at 11:35, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
> <ard.biesheuvel@xxxxxxxxxx> wrote:
>> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>>> <ard.biesheuvel@xxxxxxxxxx> wrote:
>>>> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>>>>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@xxxxxxxxxx> wrote:
>> ...
>>>>>> +       ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>>>>>> +                                      &speed_khz);
>>>>>> +       if (ret) {
>>>>>> +               dev_err(&pdev->dev,
>>>>>> +                       "Missing clock-frequency property\n");
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +       speed_khz /= 1000;
>>>
>>>>>> +       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);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>> +       }
>>>>>
>>>>> Okay, I got this case. It's more likely the one in 8250_dw.c.
>>>>>
>>>>> Can you do the similar way?
>>>
>>>> Could you elaborate?
>>>
>>> --- 8< --- 8< --- 8< ---
>>>         device_property_read_u32(dev, "clock-frequency", &p->uartclk);
>>>
>>>        /* If there is separate baudclk, get the rate from it. */
>>>        data->clk = devm_clk_get(dev, "baudclk");
>>> ...
>>>        if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>>>                return -EPROBE_DEFER;
>>>        if (!IS_ERR_OR_NULL(data->clk)) {
>>>                err = clk_prepare_enable(data->clk);
>>>                if (err)
>>>                        dev_warn(dev, "could not enable optional baudclk: %d\n",
>>>                                 err);
>>>                else
>>>                        p->uartclk = clk_get_rate(data->clk);
>>>        }
>>>
>>>        /* If no clock rate is defined, fail. */
>>>        if (!p->uartclk) {
>>>                dev_err(dev, "clock rate not defined\n");
>>>                err = -EINVAL;
>>>                goto err_clk;
>>> --- 8< --- 8< --- 8< ---
>>>
>>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>>> above and you are almost done.
>>>
>>
>> I don't think this is better.
>
> It's a pattern over ACPI vs. clk cases at least for now.
> But hold on. We have already an example of dealing with ACPI /
> non-ACPI cases for I2C controllers â i2c-designware-platdrv.c.
> Check how it's done there.
>
> I actually totally forgot about ACPI slaves described in the table. We
> need to take into account the ones with lowest bus speed.
>

Wow, that code is absolutely terrible.

So even while _DSD device properties require vendor prefixes, which
are lacking in this case, and the fact that the ACPI flavor of the
Designware I2C controller now provides two different ways to get the
timing parameters (using device properties or using SSCN/FMCN/etc ACPI
methods), you think this is a shining example of how this should be
implemented?

Also, I still think implementing a clock device using rate X just to
interrogate it for its rate (returning X) is absolutely pointless.

So what I can do is invent an ACPI method that returns the PCLK rate.
Would that work for you?

As for the max I2C speed: I think that looks sane, so I can
incorporate that as well.

>>  The generic DT I2C 'clock-frequency'
>> property denotes the bus clock rate not the rate of the clock that
>> feeds the IP block. This is rather different from the UART bindings.
>
>> Also, I don't want to support 'socionext,pclk-rate' for DT platforms,
>> only for ACPI platforms.
>
> Unfortunately, we are strong against deviations between DT and ACPI in
> case of device properties.
> You may provide a clock and use devm_clk_get() without any concerns
> from where this comes from.
>
> You won't find any deviation in DW I2C driver since there is none,
> while driver works for non-ACPI platforms as well.
>
> --
> With Best Regards,
> Andy Shevchenko