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

[Andy Shevchenko]

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.

>  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