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
