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

[Lee Jones]

On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>On Mon, 29 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 | 10 ++++++++--
> >>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index d495737..66347be 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -374,7 +374,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");
> >>>>@@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>>
> >>>What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >>>PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >>>directly without all of this faffing about.
> >>>
> >>>   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >>>
> >>
> >>Because "irq_clr_method" is of boolean type.
> >>And macros which you are referring to is,
> >>
> >>#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> >>#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> >>
> >>
> >>And also, I feel it is more cleaner approach with the current code as
> >>register definition and userflag are maintained separately.
> >
> >I see your point, although it's a shame we have to have this code in
> >its place.
> >
> >One thing I think you can do though is rid chip->irq_clr_method, just
> >use the one you already have in pdata.
> >
> 
> Looking at the current code,
> Yes, this can be done, but I have to do some more changes around it,
> to make code cleaner,
> 
> change the signature of
> 
> static int device_irq_init_800(struct pm80x_chip *chip)
> 
> TO
> 
> static int device_irq_init_800(struct pm80x_chip *chip, struct
> pm80x_platform_data *pdata)
> 
> 
> and then only use pdata->irq_clr_method.
> 
> 
> How do you want to get this inside? V6 version? or separate patch?
> 
> I have one more cleanup patch in the queue, which I am planning to
> submit today, if you are ok then I can submit along with that.

Ideally not.  Don't you save the 'struct device' into *chip?  You
should use that to fetch the pdata, like:

pdata = dev_get_platdata(chip->dev);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/