Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
> > As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> > (page 0) controls the method of clearing interrupt
> > status of 88pm800 family of devices;
> >
> >    0: clear on read
> >    1: clear on write
> >
> > If pdata is not coming from board file, then set the
> > default irq clear method to "irq clear on write"
> >
> > Also, as suggested by "Lee Jones" renaming variable field
> > to appropriate name.
> >
> > Signed-off-by: Zhao Ye <zhaoy@xxxxxxxxxxx>
> > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@xxxxxxxxxx>
> > ---
> >   drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >   include/linux/mfd/88pm80x.h |  6 ++++--
> >   2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> > index 40fd014..e0cd7ad 100644
> > --- a/drivers/mfd/88pm800.c
> > +++ b/drivers/mfd/88pm800.c
> > @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >   {
> >   	struct regmap *map = chip->regmap;
> >   	unsigned long flags = IRQF_ONESHOT;
> > -	int data, mask, ret = -EINVAL;
> > +	int irq_clr_mode, mask, ret = -EINVAL;
> >
> >   	if (!map || !chip->irq) {
> >   		dev_err(chip->dev, "incorrect parameters\n");
> > @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >   	}
> >
> >   	/*
> > -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> > -	 * default.
> > +	 * irq_clr_on_wr defines the way of clearing interrupt by
> > +	 * read/write(0/1).  It's read-clear by default.
> >   	 */
> >   	mask =
> >   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
> >   	    PM800_WAKEUP2_INT_MASK;
> >
> > -	data = PM800_WAKEUP2_INT_CLEAR;
> > -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> > +	irq_clr_mode = (chip->irq_clr_on_wr) ?
>
> Drop the brackets.

Ok

> > +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> > +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
> >   	if (ret < 0)
> >   		goto out;
> > @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
> >   	}
> >
> >   	chip->regmap_irq_chip = &pm800_irq_chip;
> > +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>
> You have protection around pdata everywhere else in the file, I
> suggest you supply some here too.

Actually it is not really needed, as the PATCH 1/1 introduces


        if (!pdata && !np) {
                dev_err(&client->dev,
                        "pm80x requires platform data or of_node\n");
                return -EINVAL;
        }

        if (!pdata && !np) {
                dev_err(&client->dev,
                        "pm80x requires platform data or of_node\n");
                return -EINVAL;
        }


So there is no way you can have pdata = NULL beyond this point.


> >   	ret = device_irq_init_800(chip);
> >   	if (ret < 0) {
> > @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
> >   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >   		if (!pdata)
> >   			return -ENOMEM;
> > +
> > +		/* by default, set irq clear method on write */
> > +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;

Yes certainly better way of doing it :)
I will change it.


> >   	}
> >
> >   	ret = pm80x_init(client);
> > diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> > index 97cb283..94b3dcd 100644
> > --- a/include/linux/mfd/88pm80x.h
> > +++ b/include/linux/mfd/88pm80x.h
> > @@ -77,6 +77,8 @@ enum {
> >   #define PM800_WAKEUP2		(0x0E)
> >   #define PM800_WAKEUP2_INV_INT		(1 << 0)
> >   #define PM800_WAKEUP2_INT_CLEAR		(1 << 1)
> > +#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
> > +#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)
> >   #define PM800_WAKEUP2_INT_MASK		(1 << 2)
>
> Use the BIT() macro.

I thought about this, but the whole file doesn't use it, so I also
chose not to.


> >   #define PM800_POWER_UP_LOG	(0x10)
> > @@ -300,7 +302,7 @@ struct pm80x_chip {
> >   	struct regmap_irq_chip_data *irq_data;
> >   	int type;
> >   	int irq;
> > -	int irq_mode;
> > +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>
> Whitespace issue.

Didn't see any...and I also ran checkpatch.

> Shouldn't this be a bool?

Just was not sure about any older board file interface.
Ideally it should be bool only.

> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> and call the variable irq_clear_method, or something.
>
> Much clearer that way I think.

We have slowly decided to almost hardcode it to one value if there is
no board file. I feel we should just keep it to simple.

If you still insist, I can implement.


Thanks,
Vaibhav
--
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/