On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>> <ard.biesheuvel@xxxxxxxxxx> 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
