lkml.org 
[lkml]   [2015]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH-V5 3/4] mfd: 88pm800: Set default interrupt clear method


    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@marvell.com>
    >>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
    >>>> ---
    >>>> 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.


    Thanks,
    Vaibhav


    \
     
     \ /
      Last update: 2015-07-07 13:01    [W:5.145 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site