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

[Sebastian Reichel]

Hi,

On Wed, Nov 27, 2019 at 10:06:47PM -0300, Matheus Castello wrote:
> [...]
> > > @@ -256,14 +303,26 @@ static int max17040_probe(struct i2c_client *client,
> > > 
> > >   	/* check interrupt */
> > >   	if (client->irq) {
> > > -		int ret;
> > > -
> > > -		ret = max17040_enable_alert_irq(chip);
> > > -
> > > -		if (ret) {
> > > -			client->irq = 0;
> > > +		if (of_device_is_compatible(client->dev.of_node,
> > > +					    "maxim,max77836-battery")) {
> > > +			ret = max17040_set_low_soc_alert(client,
> > > +							 chip->low_soc_alert);
> > > +			if (ret) {
> > > +				dev_err(&client->dev,
> > > +					"Failed to set low SOC alert: err %d\n",
> > > +					ret);
> > > +				return ret;
> > > +			}
> > > +
> > > +			ret = max17040_enable_alert_irq(chip);
> > > +			if (ret) {
> > > +				client->irq = 0;
> > > +				dev_warn(&client->dev,
> > > +					 "Failed to get IRQ err %d\n", ret);
> > > +			}
> > > +		} else {
> > >   			dev_warn(&client->dev,
> > > -				 "Failed to get IRQ err %d\n", ret);
> > > +				 "Device not compatible for IRQ");
> > 
> > Something is odd here. Either this should be part of the first
> > patch ("max17040: Add IRQ handler for low SOC alert"), or both
> > device types support the IRQ and this check should be removed.
> 
> The first patch add the IRQ without the configuration of the low SoC alert,
> using the default state of charge level. This patch is working with
> registers to config the low state of charge level, so it was proposed to
> just try to write registers in the models compatible with that
> (maxim,max77836-battery).
> 
> Maybe join the first patch to this one, and let DT binding be the first
> patch on the series so we can already test compatible here, let me know what
> you think about it.

Assuming the !max77836 do not have any interrupt support, you can
just add the OF check in the first patch in "if (client->irq)", so
that it reads 

if (client->irq && of_device_is_compatible(...)) {
    ...
}

-- Sebastian

Attachment: signature.asc
Description: PGP signature