On Tue, Feb 20, 2018 at 8:04 PM, Ard Biesheuvel
[off-list ref] wrote:

On 20 February 2018 at 14:02, Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Feb 20, 2018 at 11:08 AM, Ard Biesheuvel
[off-list ref] wrote:

quoted

quoted

+/* 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

quoted

quoted

+#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().

quoted

quoted

+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.

quoted

quoted

+       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 {

quoted

+               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