Hi,

On Sun, Sep 25, 2022 at 11:26:24AM +0200, saravanan sekar wrote:
> On 11/09/22 15:31, Sebastian Reichel wrote:
> > On Wed, Jun 15, 2022 at 04:53:56PM +0200, Saravanan Sekar wrote:
> > > mp2733 is updated version of mp2629 battery charge management
> > > which supports USB fast-charge and higher range of input voltage.
> > > 
> > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > ---
> > > [...]
> > >   	psy_cfg.drv_data = charger;
> > > -	psy_cfg.attr_grp = mp2629_charger_sysfs_groups;
> > > +	if (charger->chip_info->has_impedance)
> > > +		psy_cfg.attr_grp = mp2629_charger_sysfs_groups;
> > > +
> > > +	if (charger->chip_info->has_fast_charge)
> > > +		psy_cfg.attr_grp = mp2733_charger_sysfs_groups;
> > > +
> > >   	charger->battery = devm_power_supply_register(dev,
> > >   					 &mp2629_battery_desc, &psy_cfg);
> > >   	if (IS_ERR(charger->battery)) {
> > 
> > Instead of having has_impedance and has_fast_charge feature
> > flag that are mutual exclusive, store the device type and
> > use if/else or switch statement to chose the correct attr_grp.
> 
> these flags are not really mutual exclusive, limitation only for
> application between mp2629 and mp2629. In future another chipset on
> same series shall have both or none, so I would consider to control
> flags with functionality rather than chipset!
> 
> Please feedback if still I have to consider your proposal.

I'm fine with this being flag based, but your code assumes that the
flags are mutual exclusive, since psy_cfg.attr_grp is overwritten.
This is bad style and needs to be fixed:

+       if (charger->chip_info->has_impedance)
+               psy_cfg.attr_grp = mp2629_charger_sysfs_groups;
+
+       if (charger->chip_info->has_fast_charge)
+               psy_cfg.attr_grp = mp2733_charger_sysfs_groups;

-- Sebastian