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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Sat, Nov 02, 2019 at 03:12:44PM -0300, Matheus Castello wrote:
> 
> 
> Em 11/1/19 1:52 PM, Matheus Castello escreveu:
> > 
> > 
> > Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu:
> > > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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@castello.eng.br>
> > > > ---
> > > >   drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
> > > >   1 file changed, 74 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/power/supply/max17040_battery.c
> > > > b/drivers/power/supply/max17040_battery.c
> > > > index 75459f76d02c..802575342c72 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,22 @@ 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;
> > > > +
> > > > +    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;
> > > > +    ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static void max17040_get_vcell(struct i2c_client *client)
> > > >   {
> > > >       struct max17040_chip *chip = i2c_get_clientdata(client);
> > > > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct
> > > > i2c_client *client)
> > > >       u16 soc;
> > > > 
> > > >       soc = max17040_read_reg(client, MAX17040_SOC);
> > > > -
> > > >       chip->soc = (soc >> 8);
> > > >   }
> > > > 
> > > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct
> > > > i2c_client *client)
> > > >           chip->status = POWER_SUPPLY_STATUS_FULL;
> > > >   }
> > > > 
> > > > +static int max17040_get_of_data(struct max17040_chip *chip)
> > > > +{
> > > > +    struct device *dev = &chip->client->dev;
> > > > +    struct device_node *np = dev->of_node;
> > > > +    int ret = 0;
> > > > +
> > > > +    if (of_property_read_u32(np, "maxim,alert-low-soc-level",
> > > > +            &chip->low_soc_alert_threshold)) {
> > > 
> > > Please align the line break with line above. checkpatch --strict might
> > > give you hints about this.
> > > >> +        chip->low_soc_alert_threshold =
> > > MAX17040_ATHD_DEFAULT_POWER_UP;
> > > > +    /* check if low_soc_alert_threshold is between 1% and 32% */
> > > 
> > > The comment looks misleading here, like it belongs to previous block.
> > > Maybe put it inside else if {} block?
> > > 
> > > > +    } else if (chip->low_soc_alert_threshold <= 0 ||
> > > > +            chip->low_soc_alert_threshold >= 33){
> > > 
> > > Missing space before {.
> > > 
> > > > +        ret = -EINVAL;
> > > > +    }
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static void max17040_check_changes(struct i2c_client *client)
> > > >   {
> > > >       max17040_get_vcell(client);
> > > > @@ -192,6 +230,10 @@ static irqreturn_t
> > > > max17040_thread_handler(int id, void *dev)
> > > >       /* send uevent */
> > > >       power_supply_changed(chip->battery);
> > > > 
> > > > +    /* reset alert bit */
> > > > +    max17040_set_low_soc_threshold_alert(client,
> > > > +        chip->low_soc_alert_threshold);
> > > 
> > > Unless the continuation exceeds 80 character limit, please align it with
> > > previous line.
> > > 
> > > > +
> > > >       return IRQ_HANDLED;
> > > >   }
> > > > 
> > > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
> > > >       struct i2c_adapter *adapter = client->adapter;
> > > >       struct power_supply_config psy_cfg = {};
> > > >       struct max17040_chip *chip;
> > > > +    int ret;
> > > > 
> > > >       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> > > >           return -EIO;
> > > > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client
> > > > *client,
> > > > 
> > > >       chip->client = client;
> > > >       chip->pdata = client->dev.platform_data;
> > > > +    ret = max17040_get_of_data(chip);
> > > > +    if (ret) {
> > > > +        dev_err(&client->dev,
> > > > +            "failed: low SOC alert OF data out of bounds\n");
> > > > +        return ret;
> > > > +    }
> > > > 
> > > >       i2c_set_clientdata(client, chip);
> > > >       psy_cfg.drv_data = chip;
> > > > @@ -242,20 +291,31 @@ static int max17040_probe(struct
> > > > i2c_client *client,
> > > > 
> > > >       /* check interrupt */
> > > >       if (client->irq) {
> > > > -        int ret;
> > > > -        unsigned int flags;
> > > > -
> > > > -        dev_info(&client->dev, "IRQ: enabled\n");
> > > > -        flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
> > > > -        ret = devm_request_threaded_irq(&client->dev,
> > > > client->irq, NULL,
> > > > -                        max17040_thread_handler, flags,
> > > > -                        chip->battery->desc->name,
> > > > -                        chip);
> > > > -
> > > > -        if (ret) {
> > > > -            client->irq = 0;
> > > > +        if (of_device_is_compatible(client->dev.of_node,
> > > > +            "maxim,max77836-battery")) {
> > > 
> > > Alignment.
> > > 
> > > > +            ret = max17040_set_low_soc_threshold_alert(client,
> > > > +                chip->low_soc_alert_threshold);
> > > 
> > > Ditto.
> > > 
> 
> I am working to fix the alignments issues using the checkpath strict and I
> have a doubt. Here for example if I fix the check "Alignment should match
> open parenthesis" it will pass the 80 characters limit and will show me a
> warning.

Indeed which is a hint that the code readabiltiy is affected by long
function name + two indentations + long variable name. You can split it
to different function (covering the IRQ case) which will reduce one
indentation.

Alternatively do not align it but add few more tabs before chip->:
		ret = max17040_set_low_soc_threshold_alert(client,
						chip->low_soc_alert_threshold);
Renaming low_soc_alert_threshold to something shorter can help as well
(soc_alert seems enough descriptive).


Best regards,
Krzysztof