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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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