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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
