Re: [PATCH v4 7/7] power: bq27xxx_battery: Add interrupt handling support

[Andrew F. Davis]

On 09/15/2015 01:47 PM, Andreas Dannenberg wrote:
> On Mon, Sep 14, 2015 at 04:26:13PM -0500, Andrew F. Davis wrote:
> > Some devices have a pin that can generate an interrupt when
> > the battery's status changes. Add an interrupt handler to
> > read the new battery status.
> >
> > Signed-off-by: Andrew F. Davis <afd@xxxxxx>
> > ---
> >   drivers/power/bq27xxx_battery.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> > index 10be8a6..29fdcd7 100644
> > --- a/drivers/power/bq27xxx_battery.c
> > +++ b/drivers/power/bq27xxx_battery.c
> > @@ -48,6 +48,7 @@
> >   #include <linux/idr.h>
> >   #include <linux/i2c.h>
> >   #include <linux/slab.h>
> > +#include <linux/interrupt.h>
> >   #include <asm/unaligned.h>
> >
> >   #include <linux/power/bq27xxx_battery.h>
> > @@ -732,6 +733,15 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
> >   	di->last_update = jiffies;
> >   }
> >
> > +static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
> > +{
> > +	struct bq27xxx_device_info *di = data;
> > +
> > +	bq27xxx_battery_update(di);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >   static void bq27xxx_battery_poll(struct work_struct *work)
> >   {
> >   	struct bq27xxx_device_info *di =
> > @@ -1137,6 +1147,19 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
> >
> >   	i2c_set_clientdata(client, di);
> >
> > +	if (client->irq) {
> > +		retval = devm_request_threaded_irq(&client->dev, client->irq,
> > +				NULL, bq27xxx_battery_irq_handler_thread,
> > +				IRQF_ONESHOT,
> > +				name, di);
>
> Hi Andrew,
> I think it'll be better if you explicitly specify the rising vs. falling
> edge interrupt flags according to exactly what the bq27xxx will trigger
> on to ensure the corresponding GPIO module is always configured properly.

That was how I was going to have it, but this may be device dependent, so
I decided to leave it up to the interrupt controller and set it with DT:

interrupts = <28 IRQ_TYPE_EDGE_FALLING>;

> While we are at it, was the platform-data based initialization in
> conjunction with interrupts tested?

The only time we initialize with platform-data is when we are wired
with HDQ, in that case this IRQ code does not get called, so nothing
changes.

Regards,
Andrew

> Regards,
>
> --
> Andreas Dannenberg
> Texas Instruments Inc
>
>
> > +		if (retval) {
> > +			dev_err(&client->dev,
> > +				"Unable to register IRQ %d error %d\n",
> > +				client->irq, retval);
> > +			return retval;
> > +		}
> > +	}
> > +
> >   	return 0;
> >
> >   batt_failed:
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/