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

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

quoted

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

quoted

+/* SPDX-License-Identifier: GPL-2.0 */

Shouldn't be // ?

quoted

IIUC, this applies to .h files only, and /* */ is preferred for .c files.

Other way around.

Documentation/process/license-rules.rst

OK got it

quoted

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.

quoted

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

I'll replace it with the equivalent

#define WAIT_PCLK(n, rate) \
    ndelay(DIV_ROUND_UP(DIV_ROUND_UP(1000000000, rate), n) + 10)

quoted

quoted

quoted

+static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
+{
+       dev_dbg(i2c->dev, "STOP\n");

Hmm... Can't use FTRACE ?

quoted

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.

OK, I will just drop it.

quoted

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.

quoted

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

OK, I see what you mean now. But for this driver, creating a fixed
rate clock with no controls whatsoever, only to interrogate it for its
rate (which we retrieved from the ACPI device node in the first place)
seems rather pointless to me. Am I missing something here?