[PATCH V4 14/15] power: supply: axp20x_battery: add support for AXP717

[Chris Morgan]

On Tue, Aug 27, 2024 at 06:24:42PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Aug 21, 2024 at 04:54:55PM GMT, Chris Morgan wrote:
> > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +		ret = iio_read_channel_processed(axp20x_batt->batt_v,
> > +						 &val->intval);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* IIO framework gives mV but Power Supply framework gives uV */
> > +		val->intval *= 1000;
> > +		return 0;
> 
> I see you followed the existing pattern for these two drivers. Can
> you please add another patch, which converts both drivers to the
> following pattern:
> 
> return iio_read_channel_processed_scale(adc_chan, &val->intval, 1000);

Would it be okay if I sent a patch series on top of this one (rather
than rebasing now that some of these are starting to get pulled)?

It's probably a simple enough change so I don't mind.

> 
> [...]
> 
> > +static int axp717_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> > +					  int val)
> > +{
> > +	switch (val) {
> > +	case 4000000:
> > +		val = AXP717_CHRG_CV_4_0V;
> > +		break;
> > +
> > +	case 4100000:
> > +		val = AXP717_CHRG_CV_4_1V;
> > +		break;
> > +
> > +	case 4200000:
> > +		val = AXP717_CHRG_CV_4_2V;
> > +		break;
> > +
> > +	default:
> > +		/*
> > +		 * AXP717 can go up to 4.35, 4.4, and 5.0 volts which
> > +		 * seem too high for lithium batteries, so do not allow.
> > +		 */
> > +		return -EINVAL;
> 
> 4.35V and 4.4V batteries exists. You can find them when you search
> for LiHV (Lithium High Voltage).
> 

Do you think I should add it then? Full disclosure, I basically opted
to not add this because while this series was written more or less
exclusively off of the datasheet I did peek at the BSP implementation
and I think they restricted these voltages. Again, as a fast-follow
once these patches finish getting pulled (I can start work on it now
though).

> [...]
> 
> Otherwise LGTM,
> 
> -- Sebastian

Thank you,
Chris