Re: [PATCH v3 5/6] power: supply: Add support for mp2733 battery charger

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux