| Message ID | 20230525152203.32190-4-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: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;
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 --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);
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(-)