diff mbox series

[RFC,5/6] HACK: i2c: aspeed: Comment the clock and reset out.

Message ID 20230525152203.32190-6-Jonathan.Cameron@huawei.com
State Superseded
Headers show
Series i2c: Enabling use of aspeed-i2c with ACPI | expand

Commit Message

Jonathan Cameron May 25, 2023, 3:22 p.m. UTC
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(-)

Comments

Andy Shevchenko May 26, 2023, 9:16 p.m. UTC | #1
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)?
Jonathan Cameron May 30, 2023, 2:40 p.m. UTC | #2
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 mbox series

Patch

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