On Mon, May 27, 2024 at 11:10:32AM +0200, Oliver Neukum wrote:
>
>
> On 23.05.24 01:01, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >
> > Add support for the AXP717. It does USB BC 1.2 detection similar to the
> > AXP813, however it is otherwise very different from previous AXP
> > devices.
> >
> > Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > ---
>
> > +static int axp717_usb_init(struct axp20x_usb_power *power)
> > +{
> > + int ret;
> > +
> > + ret = regmap_update_bits(power->regmap, AXP717_ADC_CH_EN_CONTROL,
> > + AXP717_ADC_EN_VSYS_VOLT |
> > + AXP717_ADC_EN_VBUS_VOLT,
> > + AXP717_ADC_EN_VSYS_VOLT |
> > + AXP717_ADC_EN_VBUS_VOLT);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> This is a bit silly.
Agreed, I'll fix.
>
> > +}
> > +
> > static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
> > int intval)
> > {
> > @@ -307,6 +417,20 @@ static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
> > return -EINVAL;
> > }
> > +static int axp717_usb_power_set_voltage_min(struct axp20x_usb_power *power,
> > + int intval)
> > +{
> > + int val;
> > +
> > + if (intval < 3880000 || intval > 5080000)
>
> Do you really want raw numbers here?
>
Raw numbers are used throughout the driver, but it probably makes
sense to document *why* these numbers are here and what they mean, so
I'll do that.
> [..]
> > +static int axp717_usb_power_set_property(struct power_supply *psy,
> > + enum power_supply_property psp,
> > + const union power_supply_propval *val)
> > +{
> > + struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> > +
> > + switch (psp) {
> > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> > + return axp717_usb_power_set_input_current_limit(power, val->intval);
> > +
> > + case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> > + return axp717_usb_power_set_voltage_min(power, val->intval);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return -EINVAL;
>
> This is also a bit silly.
That's how it's done above, but I might as well just remove the default
statement so it returns -EINVAL if neither of the other two conditions
aren't true.
>
> Regards
> Oliver
>
I'll resubmit with the changes mentioned above.
Thank you,
Chris
