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

[Krzysztof Kozlowski]

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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > Â 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