Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Mon, 3 Jun 2019 at 00:42, Matheus Castello <matheus@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote:
> > On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@xxxxxxxxxxxxxxx> wrote:
> >>
> >> For configuration of fuel gauge alert for a low level state of charge
> >> interrupt we add a function to config level threshold and a device tree
> >> binding property to set it in flatned device tree node.
> >>
> >> Now we can use "maxim,alert-low-soc-level" property with the values from
> >> 1% up to 32% to configure alert interrupt threshold.
> >>
> >> Signed-off-by: Matheus Castello <matheus@xxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++--
> >>   1 file changed, 49 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> >> index b7433e9ca7c2..2f4851608cfe 100644
> >> --- a/drivers/power/supply/max17040_battery.c
> >> +++ b/drivers/power/supply/max17040_battery.c
> >> @@ -29,6 +29,9 @@
> >>   #define MAX17040_DELAY         1000
> >>   #define MAX17040_BATTERY_FULL  95
> >>
> >> +#define MAX17040_ATHD_MASK             0xFFC0
> >> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4
> >> +
> >>   struct max17040_chip {
> >>          struct i2c_client               *client;
> >>          struct delayed_work             work;
> >> @@ -43,6 +46,8 @@ struct max17040_chip {
> >>          int soc;
> >>          /* State Of Charge */
> >>          int status;
> >> +       /* Low alert threshold from 32% to 1% of the State of Charge */
> >> +       u32 low_soc_alert_threshold;
> >>   };
> >>
> >>   static int max17040_get_property(struct power_supply *psy,
> >> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client)
> >>          max17040_write_reg(client, MAX17040_CMD, 0x0054);
> >>   }
> >>
> >> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
> >> +       u32 level)
> >> +{
> >> +       int ret;
> >> +       u16 data;
> >> +
> >> +       /* check if level is between 1% and 32% */
> >> +       if (level > 0 && level < 33) {
> >> +               level = 32 - level;
> >> +               data = max17040_read_reg(client, MAX17040_RCOMP);
> >> +               /* clear the alrt bit and set LSb 5 bits */
> >> +               data &= MAX17040_ATHD_MASK;
> >> +               data |= level;
> >> +               max17040_write_reg(client, MAX17040_RCOMP, data);
> >> +               ret = 0;
>
> I will put the return of max17040_write_reg on ret, instead of ret = 0.
>
> >> +       } else {
> >> +               ret = -EINVAL;
> >> +       }
> >
> > This is unusual way of handling error... when you parse DTS, you
> > accept any value for "level" (even incorrect one). You validate the
> > value later when setting it and show an error... however you ignore
> > the error of max17040_write_reg() here... This is correct but looks
> > unusual.
> >
>
> Ok, so would it be better to check the level value in
> "max17040_get_of_data" and return an error there if the input is wrong?

I think yes. It looks more natural - validate the value as early as
possible and fail the probe which gives the information about point of
failure.

Best regards,
Krzysztof