Message-ID: <CAB9BWhdLJp9NWp2+no=x9ZnJ1H32ZUfebiuK8BA3_wywFufqSg@mail.gmail.com>
Date:   Wed, 9 Aug 2023 14:00:18 +0800
From:   wenhua lin <wenhua.lin1994@...il.com>
To:     Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc:     Wenhua Lin <Wenhua.Lin@...soc.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Andy Shevchenko <andy@...nel.org>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xiongpeng Wu <xiongpeng.wu@...soc.com>
Subject: Re: [PATCH] gpio: sprd: Two-dimensional arrays maintain pmic eic

Hi baolin:
1.We have recorded the offset, no need two-dimensional array unless you
have other strong reasons.

One-dimensional array reg[CACHE_NR_REGS] , the array to cache the EIC
registers., pmic eic has different channels, if the pmic eic does not
increase the offset two-dimensional array to maintain separately, it
will cause one of the eic channels to close the interrupt enable, and
it will be synchronized Disable other eic channel interrupt enable.

2.Why? you did not explain this in the commit log.
We will re-split the patch submission and explain our reasons for
modification in the submission information, thank you very much for
your review

3.Ditto. Why?
> +     pmic_eic->chip.can_sleep = true;
We will re-split the patch submission, thank you very much for your review

Baolin Wang <baolin.wang@...ux.alibaba.com> 于2023年8月7日周一 17:27写道：
>
>
>
> On 8/7/2023 5:18 PM, Wenhua Lin wrote:
> > Maintain the registers of each pmic eic through a Two-dimensional
> > array to avoid mutual interference.
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@...soc.com>
>
> NAK. See below.
>
> > ---
> >   drivers/gpio/gpio-pmic-eic-sprd.c | 23 +++++++++++++----------
> >   1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-pmic-eic-sprd.c b/drivers/gpio/gpio-pmic-eic-sprd.c
> > index c3e4d90f6b18..8d67d130cbcf 100644
> > --- a/drivers/gpio/gpio-pmic-eic-sprd.c
> > +++ b/drivers/gpio/gpio-pmic-eic-sprd.c
> > @@ -57,7 +57,7 @@ struct sprd_pmic_eic {
> >       struct gpio_chip chip;
> >       struct regmap *map;
> >       u32 offset;
> > -     u8 reg[CACHE_NR_REGS];
> > +     u8 reg[SPRD_PMIC_EIC_NR][CACHE_NR_REGS];
>
> We have recorded the offset, no need two-dimensional array unless you
> have other strong reasons.
>
> >       struct mutex buslock;
> >       int irq;
> >   };
> > @@ -151,8 +151,8 @@ static void sprd_pmic_eic_irq_mask(struct irq_data *data)
> >       struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
> >       u32 offset = irqd_to_hwirq(data);
> >
> > -     pmic_eic->reg[REG_IE] = 0;
> > -     pmic_eic->reg[REG_TRIG] = 0;
> > +     pmic_eic->reg[offset][REG_IE] = 0;
> > +     pmic_eic->reg[offset][REG_TRIG] = 0;
> >
> >       gpiochip_disable_irq(chip, offset);
> >   }
> > @@ -165,8 +165,8 @@ static void sprd_pmic_eic_irq_unmask(struct irq_data *data)
> >
> >       gpiochip_enable_irq(chip, offset);
> >
> > -     pmic_eic->reg[REG_IE] = 1;
> > -     pmic_eic->reg[REG_TRIG] = 1;
> > +     pmic_eic->reg[offset][REG_IE] = 1;
> > +     pmic_eic->reg[offset][REG_TRIG] = 1;
> >   }
> >
> >   static int sprd_pmic_eic_irq_set_type(struct irq_data *data,
> > @@ -174,13 +174,14 @@ static int sprd_pmic_eic_irq_set_type(struct irq_data *data,
> >   {
> >       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> >       struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
> > +     u32 offset = irqd_to_hwirq(data);
> >
> >       switch (flow_type) {
> >       case IRQ_TYPE_LEVEL_HIGH:
> > -             pmic_eic->reg[REG_IEV] = 1;
> > +             pmic_eic->reg[offset][REG_IEV] = 1;
> >               break;
> >       case IRQ_TYPE_LEVEL_LOW:
> > -             pmic_eic->reg[REG_IEV] = 0;
> > +             pmic_eic->reg[offset][REG_IEV] = 0;
> >               break;
> >       case IRQ_TYPE_EDGE_RISING:
> >       case IRQ_TYPE_EDGE_FALLING:
> > @@ -222,15 +223,15 @@ static void sprd_pmic_eic_bus_sync_unlock(struct irq_data *data)
> >                       sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_IEV, 1);
> >       } else {
> >               sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_IEV,
> > -                                  pmic_eic->reg[REG_IEV]);
> > +                                  pmic_eic->reg[offset][REG_IEV]);
> >       }
> >
> >       /* Set irq unmask */
> >       sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_IE,
> > -                          pmic_eic->reg[REG_IE]);
> > +                          pmic_eic->reg[offset][REG_IE]);
> >       /* Generate trigger start pulse for debounce EIC */
> >       sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_TRIG,
> > -                          pmic_eic->reg[REG_TRIG]);
> > +                          pmic_eic->reg[offset][REG_TRIG]);
> >
> >       mutex_unlock(&pmic_eic->buslock);
> >   }
> > @@ -335,6 +336,7 @@ static int sprd_pmic_eic_probe(struct platform_device *pdev)
> >
> >       ret = devm_request_threaded_irq(&pdev->dev, pmic_eic->irq, NULL,
> >                                       sprd_pmic_eic_irq_handler,
> > +                                     IRQF_TRIGGER_LOW |
>
> Why? you did not explain this in the commit log.
>
> >                                       IRQF_ONESHOT | IRQF_NO_SUSPEND,
> >                                       dev_name(&pdev->dev), pmic_eic);
> >       if (ret) {
> > @@ -352,6 +354,7 @@ static int sprd_pmic_eic_probe(struct platform_device *pdev)
> >       pmic_eic->chip.set_config = sprd_pmic_eic_set_config;
> >       pmic_eic->chip.set = sprd_pmic_eic_set;
> >       pmic_eic->chip.get = sprd_pmic_eic_get;
> > +     pmic_eic->chip.can_sleep = true;
>
> Ditto. Why?
>
> Please DO NOT squash different fixes into one patch.

Powered by blists - more mailing lists