Hi Andy Shevchenko,
Thanks for the feedback.
> Subject: Re: [PATCH] regulator: pv880x0: Simplify probe()
>
> On Sat, Aug 26, 2023 at 12:49:19PM +0100, Biju Das wrote:
> > Replace pv88080_types->pv88080_compatible_regmap in OF/ID tables and
> > simplify the probe() by replacing of_match_node() and ID lookup for
> > retrieving match data by i2c_get_match_data(). After this there is no
> > user of enum pv88080_types. So drop it.
>
> ...
>
> > #ifdef CONFIG_OF
> > static const struct of_device_id pv88080_dt_ids[] = {
> > - { .compatible = "pvs,pv88080", .data = (void *)TYPE_PV88080_AA },
> > - { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > - { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > + { .compatible = "pvs,pv88080", .data = &pv88080_aa_regs },
> > + { .compatible = "pvs,pv88080-aa", .data = &pv88080_aa_regs },
> > + { .compatible = "pvs,pv88080-ba", .data = &pv88080_ba_regs },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, pv88080_dt_ids); #endif
>
> With this patch it makes sense to get rid of ugly ifdeffery, correct header
> inclusions (if needed) and Kconfig.
>
> See similar patches
> $ git log --no-merges --grep "Make use of device properties"
OK, I will have a look.
>
> > +static const struct i2c_device_id pv88080_i2c_id[] = {
> > + { "pv88080", (kernel_ulong_t)&pv88080_aa_regs },
> > + { "pv88080-aa", (kernel_ulong_t)&pv88080_aa_regs },
> > + { "pv88080-ba", (kernel_ulong_t)&pv88080_ba_regs },
>
> > + {},
>
> Please, remove trailing comma in the terminator entry. Same you can do for
> other ID tables and in other patches you prepared.
OK.
>
> > +};
>
> ...
>
> > -static const struct i2c_device_id pv88080_i2c_id[] = {
> > - { "pv88080", TYPE_PV88080_AA },
> > - { "pv88080-aa", TYPE_PV88080_AA },
> > - { "pv88080-ba", TYPE_PV88080_BA },
> > - {},
> > -};
> > -MODULE_DEVICE_TABLE(i2c, pv88080_i2c_id);
> > -
>
> Why do you move this one up? Shouldn't it be other way around, i.e. moving
> the other closer to its user, namely here?
Agreed.
Cheers,
Biju
>
