diff mbox series

[RFC,3/6] i2c: aspeed: switch to generic fw properties.

Message ID 20230525152203.32190-4-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
Not tested on device tree but works nicely for ACPI :)

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko May 26, 2023, 9:11 p.m. UTC | #1
On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Not tested on device tree but works nicely for ACPI :)

Needs a better commit message obviously :-)

...

> -       ret = of_property_read_u32(pdev->dev.of_node,
> +       ret = device_property_read_u32(&pdev->dev,
>                                    "bus-frequency", &bus->bus_frequency);

Oh, please avoid double effort, i.e. go further and use I²C core APIs
for the timings. Oh, wait, do they use non-standard property?!

...

> +       bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> +               device_get_match_data(&pdev->dev);

Personally I prefer using pointers in driver_data so we can avoid
ambiguity for the 0/NULL value returned by this call. But if 0 value
is considered invalid here, it's probably fine.

> +       if (!bus->get_clk_reg_val)
>                 bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> -       else
> -               bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> -                               match->data;
Jonathan Cameron May 30, 2023, 2:16 p.m. UTC | #2
On Sat, 27 May 2023 00:11:09 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Not tested on device tree but works nicely for ACPI :)  

I was planning to abandon these as 'on list for anyone who
cared' but now you've reviewed them I guess I better do
an RFC v2 :)

> 
> Needs a better commit message obviously :-)

:)

> 
> ...
> 
> > -       ret = of_property_read_u32(pdev->dev.of_node,
> > +       ret = device_property_read_u32(&pdev->dev,
> >                                    "bus-frequency", &bus->bus_frequency);  
> 
> Oh, please avoid double effort, i.e. go further and use I²C core APIs
> for the timings. Oh, wait, do they use non-standard property?!

yup :(

Though it is documented as having a default of 100kHz in the devicetree
binding so the original code shouldn't be calling dev_err() and should
just do:

	bus->frequency = I2C_MAX_STANDARD_MODE_FREQ;
	device_property_read_u32(&pdev->dev,
				 "bus-frequency, &bus->frequency);

Fixing that is an unrelated change though. I'll do it for dt
in a precusor patch then carry that forward to here.

> 
> ...
> 
> > +       bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> > +               device_get_match_data(&pdev->dev);  
> 
> Personally I prefer using pointers in driver_data so we can avoid
> ambiguity for the 0/NULL value returned by this call. But if 0 value
> is considered invalid here, it's probably fine.

It is a pointer, just a function pointer rather than to a structure.
I could wrap it up in a structure but that would be an unrelated
driver change so at very least a separate patch. 

> 
> > +       if (!bus->get_clk_reg_val)
> >                 bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> > -       else
> > -               bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> > -                               match->data;  
> 
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 04155c9a50a5..73508fb9dc08 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -18,9 +18,8 @@ 
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 
@@ -975,7 +974,6 @@  MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
 
 static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 {
-	const struct of_device_id *match;
 	struct aspeed_i2c_bus *bus;
 	struct clk *parent_clk;
 	int irq, ret;
@@ -1003,7 +1001,7 @@  static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	}
 	reset_control_deassert(bus->rst);
 
-	ret = of_property_read_u32(pdev->dev.of_node,
+	ret = device_property_read_u32(&pdev->dev,
 				   "bus-frequency", &bus->bus_frequency);
 	if (ret < 0) {
 		dev_err(&pdev->dev,
@@ -1011,12 +1009,10 @@  static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 		bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
-	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
-	if (!match)
+	bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
+		device_get_match_data(&pdev->dev);
+	if (!bus->get_clk_reg_val)
 		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
-	else
-		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
-				match->data;
 
 	/* Initialize the I2C adapter */
 	spin_lock_init(&bus->lock);