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
