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@xxxxxxxxx>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > ---
> > > [...]
> > > 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
Attachment:
signature.asc
Description: PGP signature
