Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 23 Feb 2018 15:12:26 +0200 | Subject | Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller |
| |
On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> 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.
>>> + 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.
>>> + 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.
>>> + 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?
>>> + dev_err(&pdev->dev, "no IRQ resource found\n"); >>> + return -ENODEV; >>> + }
-- With Best Regards, Andy Shevchenko
| |