On Wed, May 29, 2024 at 10:22:33AM -0500, Chris Morgan wrote:
> 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.
I'm going to go back to the drawing board with this, so for now I'm
withdrawing this patch series. Basically moving on to the next logical
step of working with the battery it became obvious I should have done
this using an ADC controller just like the existing axp usb and battery
drivers. So I'll start with that and then resubmit a redone version of
this once I have the ADC code in place.
Thank you,
Chris.
>
> Thank you,
> Chris
