On Sat, 27 May 2023 00:11:09 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron > <Jonathan.Cameron@xxxxxxxxxx> 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; > >
