[PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
From: Ard Biesheuvel <hidden>
Date: 2018-02-26 09:59:49
Also in:
linux-devicetree, linux-i2c, lkml
On 23 February 2018 at 13:12, Andy Shevchenko [off-list ref] wrote:
On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel [off-list ref] wrote:quoted
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 hunk ↗ jump to hunk
quoted
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
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?quoted
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.
I don't think this is better. The generic DT I2C 'clock-frequency' property denotes the bus clock rate not the rate of the clock that feeds the IP block. This is rather different from the UART bindings. Also, I don't want to support 'socionext,pclk-rate' for DT platforms, only for ACPI platforms.
quoted
quoted
quoted
+ i2c->irq = platform_get_irq(pdev, 0); + if (i2c->irq <= 0) {< 0 ? On some platforms IRQ == 0 might be valid.quoted
Are you sure about that?Yes. I fixed some cases on one of a such.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?
Yes, I suppose it does. I'll change it to '< 0'