| Message ID | 20230525152203.32190-6-Jonathan.Cameron@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/6] i2c: acpi: set slave mode flag | expand |
On Thu, May 25, 2023 at 6:24 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > Needs tidying up - hopefully can do clock right > using on going work from Niyas > https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management For the current code base the easiest way is to switch to _optional for clock, or request them based on the type of the fwnode. (Personal preference is the _optional() API to call). For the reset isn't it transparent already so we got a dummy control (as for regulator)?
On Sat, 27 May 2023 00:16:36 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, May 25, 2023 at 6:24 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > Needs tidying up - hopefully can do clock right > > using on going work from Niyas > > https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management > > For the current code base the easiest way is to switch to _optional > for clock, or request them based on the type of the fwnode. (Personal > preference is the _optional() API to call). Absolutely agree that would the way to go if people want to support my crazy. However, that will leave the input clock frequency unknown which means we'll program a garbage value into one of the device registers. Doesn't matter to me, but not good in general. This is avoiding for now the questions of: 1) Why devm for a clock we hold for 2 lines of code, none of which have an error return path... 2) clk_get_rate() is documented as not guaranteed to do anything for a clk until enabled, so this is relying on it being enabled by someone else or a quirk of the the chip. > For the reset isn't it > transparent already so we got a dummy control (as for regulator)? I don't think so, but maybe I'm missing something. There is a devm_reset_control_get_optional() though, similar to the clock one that returns a NULL if not present. I'll use that here to make this a slightly less ugly hack. If I can handle clocks nicely using Niyas' work then can revisit whether the i2c and aspeed maintainers would accept making the reset optional. Jonathan > >
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index adbb93444d7a..df20382970f3 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -986,20 +986,21 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev) if (IS_ERR(bus->base)) return PTR_ERR(bus->base); - parent_clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(parent_clk)) - return PTR_ERR(parent_clk); - bus->parent_clk_frequency = clk_get_rate(parent_clk); + // parent_clk = devm_clk_get(&pdev->dev, NULL); + // if (IS_ERR(parent_clk))// + // return PTR_ERR(parent_clk); + bus->parent_clk_frequency = 1000000;//clk_get_rate(parent_clk); /* We just need the clock rate, we don't actually use the clk object. */ - devm_clk_put(&pdev->dev, parent_clk); + //devm_clk_put(&pdev->dev, parent_clk); bus->rst = devm_reset_control_get_shared(&pdev->dev, NULL); if (IS_ERR(bus->rst)) { - dev_err(&pdev->dev, + dev_warn(&pdev->dev, "missing or invalid reset controller device tree entry\n"); - return PTR_ERR(bus->rst); + bus->rst = 0; + } else { + reset_control_deassert(bus->rst); } - reset_control_deassert(bus->rst); ret = device_property_read_u32(&pdev->dev, "bus-frequency", &bus->bus_frequency);
Needs tidying up - hopefully can do clock right using on going work from Niyas https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management Don't think ACPI provide an equivalent reset deasert / assert. _RST doesn't fit that model. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)