On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
[off-list ref] wrote:

On 23 February 2018 at 12:27, Andy Shevchenko [off-list ref] wrote:

quoted

On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
[off-list ref] wrote:

quoted

This is a cleaned up version of the I2C controller driver for
the Fujitsu F_I2C IP, which was never supported upstream, and
has now been incorporated into the Socionext SynQuacer SoC.

quoted

quoted

+       return DIV_ROUND_UP((bit_count * 9 + 10 * num) * 3, 200) + 10;

When I suggested to drop parens, I also suggested to swap second pair
arguments (b/c I was thinking that parens to prevent confusion + vs
*), like

9 * bit_count + 10 * num, or
bit_count * 9 + num * 10.

Though, it is up to you, I still consider that + vs. * operator
precedence is quite obvious.

I can change it if you like.

I guess slightly better to change, thanks.

quoted

quoted

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

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

quoted

quoted

+       i2c->irq = platform_get_irq(pdev, 0);
+       if (i2c->irq <= 0) {

< 0 ?

On some platforms IRQ == 0 might be valid.

Are you sure about that?

Yes. I fixed some cases on one of a such.

http://yarchive.net/comp/linux/no_irq.html

I agree with Linus from software IRQ (and nowadays luckily we are
using IRQ descriptors), but I disagree with him from hardware
prospective.
0 is totally valid HW IRQ line. In hardware there is no descriptor
(except, yes, MSI and alike cases), it's just a wire with an index.

So, while drivers are getting better in code prospective (though I
don't see many of them comparing this to 0), the IRQ framework is
changing itself as well.

At which circumstances we might get 0 in the first place?

Second question, doesn't request_irq() fail on irq==0 if it's not
supported as valid by platform?

quoted

quoted

+               dev_err(&pdev->dev, "no IRQ resource found\n");
+               return -ENODEV;
+       }

-- 
With Best Regards,
Andy Shevchenko