Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number

[wenhua lin]

Hi baolin:
1.Please describe the problem in detail, not only what you did.

We will describe the reason for the modification by supplementing the
commit message.

2.Can you explicit on which controller can support 8 banks in the commit
log? And you did not change all the related comments in this file.

The old project EIC controller can support maximum 3 banks,
now our new project eic controller can support maximum 8 banks，
and each bank contains 8 EICs.
We will modify the comment in this file.

3.If you want to introduce a readable macro, that's fine, but it should be
split into a separate patch.
4.This change looks good to me, and this seems a software bug in the
original driver. So I think this change should be moved into a separate
patch with a suitable Fixes tag.
5.Do not add unreated changes that you did not mentioned 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.

Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> 于2023年8月9日周三 09:24写道：
>
>
>
> On 8/8/2023 11:31 AM, Wenhua Lin wrote:
> > Automatic calculation through matching nodes,
> > subsequent projects can avoid modifying driver files.
>
> Please describe the problem in detail, not only what you did.
>
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@xxxxxxxxxx>
> > ---
> >   drivers/gpio/gpio-eic-sprd.c | 49 +++++++++++++++++++-----------------
> >   1 file changed, 26 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> > index 84352a6f4973..0d85d9e80848 100644
> > --- a/drivers/gpio/gpio-eic-sprd.c
> > +++ b/drivers/gpio/gpio-eic-sprd.c
> > @@ -50,10 +50,10 @@
> >   #define SPRD_EIC_SYNC_DATA          0x1c
> >
> >   /*
> > - * The digital-chip EIC controller can support maximum 3 banks, and each bank
> > + * The digital-chip EIC controller can support maximum 8 banks, and each bank
>
> Can you explicit on which controller can support 8 banks in the commit
> log? And you did not change all the related comments in this file.
>
> >    * contains 8 EICs.
> >    */
> > -#define SPRD_EIC_MAX_BANK            3
> > +#define SPRD_EIC_MAX_BANK            8
> >   #define SPRD_EIC_PER_BANK_NR                8
> >   #define SPRD_EIC_DATA_MASK          GENMASK(7, 0)
> >   #define SPRD_EIC_BIT(x)                     ((x) & (SPRD_EIC_PER_BANK_NR - 1))
> > @@ -99,33 +99,32 @@ struct sprd_eic {
> >
> >   struct sprd_eic_variant_data {
> >       enum sprd_eic_type type;
> > -     u32 num_eics;
> >   };
> >
> > +#define SPRD_EIC_VAR_DATA(soc_name)                          \
> > +static const struct sprd_eic_variant_data soc_name##_eic_dbnc_data = {       \
> > +     .type = SPRD_EIC_DEBOUNCE,                                      \
> > +};                                                                   \
> > +                                                                     \
> > +static const struct sprd_eic_variant_data soc_name##_eic_latch_data = {      \
> > +     .type = SPRD_EIC_LATCH,                                         \
> > +};                                                                   \
> > +                                                                     \
> > +static const struct sprd_eic_variant_data soc_name##_eic_async_data = {      \
> > +     .type = SPRD_EIC_ASYNC,                                         \
> > +};                                                                   \
> > +                                                                     \
> > +static const struct sprd_eic_variant_data soc_name##_eic_sync_data = {       \
> > +     .type = SPRD_EIC_SYNC,                                          \
> > +}
> > +
> > +SPRD_EIC_VAR_DATA(sc9860);
> > +
> >   static const char *sprd_eic_label_name[SPRD_EIC_MAX] = {
> >       "eic-debounce", "eic-latch", "eic-async",
> >       "eic-sync",
> >   };
> >
> > -static const struct sprd_eic_variant_data sc9860_eic_dbnc_data = {
> > -     .type = SPRD_EIC_DEBOUNCE,
> > -     .num_eics = 8,
> > -};
> > -
> > -static const struct sprd_eic_variant_data sc9860_eic_latch_data = {
> > -     .type = SPRD_EIC_LATCH,
> > -     .num_eics = 8,
> > -};
> > -
> > -static const struct sprd_eic_variant_data sc9860_eic_async_data = {
> > -     .type = SPRD_EIC_ASYNC,
> > -     .num_eics = 8,
> > -};
> > -
> > -static const struct sprd_eic_variant_data sc9860_eic_sync_data = {
> > -     .type = SPRD_EIC_SYNC,
> > -     .num_eics = 8,
> > -};
>
> If you want to introduce a readable macro, that's fine, but it should be
> split into a separate patch.
>
> >   static inline void __iomem *sprd_eic_offset_base(struct sprd_eic *sprd_eic,
> >                                                unsigned int bank)
> > @@ -583,6 +582,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
> >       struct sprd_eic *sprd_eic;
> >       struct resource *res;
> >       int ret, i;
> > +     u16 num_banks = 0;
> >
> >       pdata = of_device_get_match_data(&pdev->dev);
> >       if (!pdata) {
> > @@ -613,12 +613,13 @@ static int sprd_eic_probe(struct platform_device *pdev)
> >                       break;
> >
> >               sprd_eic->base[i] = devm_ioremap_resource(&pdev->dev, res);
> > +             num_banks++;
> >               if (IS_ERR(sprd_eic->base[i]))
> >                       return PTR_ERR(sprd_eic->base[i]);
> >       }
> >
> >       sprd_eic->chip.label = sprd_eic_label_name[sprd_eic->type];
> > -     sprd_eic->chip.ngpio = pdata->num_eics;
> > +     sprd_eic->chip.ngpio = num_banks * SPRD_EIC_PER_BANK_NR;
>
> This change looks good to me, and this seems a software bug in the
> original driver. So I think this change should be moved into a separate
> patch with a suitable Fixes tag.
>
> >       sprd_eic->chip.base = -1;
> >       sprd_eic->chip.parent = &pdev->dev;
> >       sprd_eic->chip.direction_input = sprd_eic_direction_input;
> > @@ -630,10 +631,12 @@ static int sprd_eic_probe(struct platform_device *pdev)
> >               sprd_eic->chip.set = sprd_eic_set;
> >               fallthrough;
> >       case SPRD_EIC_ASYNC:
> > +             fallthrough;
> >       case SPRD_EIC_SYNC:
> >               sprd_eic->chip.get = sprd_eic_get;
> >               break;
> >       case SPRD_EIC_LATCH:
> > +             fallthrough;
>
> Do not add unreated changes that you did not mentioned in the commit log.