2023-08-08 17:54:17

by Wenhua Lin

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

Automatic calculation through matching nodes,
subsequent projects can avoid modifying driver files.

Signed-off-by: Wenhua Lin <[email protected]>
---
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
* 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,
-};

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;
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;
default:
break;
}
--
2.17.1

2023-08-08 18:52:17

by Hugo Villeneuve

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

On Tue, 8 Aug 2023 11:31:06 +0800
Wenhua Lin <[email protected]> wrote:

> Automatic calculation through matching nodes,
> subsequent projects can avoid modifying driver files.
>
> Signed-off-by: Wenhua Lin <[email protected]>
> ---
> 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
> * 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,
> -};
>
> 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;
> 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;

Hi,
this probably should go in a separate patch as a fix or an
improvement with proper comments explaining the reason?

> case SPRD_EIC_SYNC:
> sprd_eic->chip.get = sprd_eic_get;
> break;
> case SPRD_EIC_LATCH:
> + fallthrough;

ditto.

Hugo Villeneuve


> default:
> break;
> }
> --
> 2.17.1
>

2023-08-08 19:06:54

by Hugo Villeneuve

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

On Tue, 8 Aug 2023 11:31:06 +0800
Wenhua Lin <[email protected]> wrote:

> Automatic calculation through matching nodes,
> subsequent projects can avoid modifying driver files.
>
> Signed-off-by: Wenhua Lin <[email protected]>
> ---
> 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
> * contains 8 EICs.
> */
> -#define SPRD_EIC_MAX_BANK 3
> +#define SPRD_EIC_MAX_BANK 8

Hi,
it seems as tough the commit title doesn't reflect the fact that you
not only modify the calculation method, but also the maximum number of
banks?


> #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,
> -};
>
> 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;
> 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;
> default:
> break;
> }
> --
> 2.17.1
>

2023-08-08 19:10:32

by Andy Shevchenko

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

On Tue, Aug 08, 2023 at 11:31:06AM +0800, Wenhua Lin wrote:
> Automatic calculation through matching nodes,
> subsequent projects can avoid modifying driver files.

You sent three patches which has to be meant a series in three independent
(unlinked) messages. Do not forget to use --thread so it become a such.

...

> struct sprd_eic *sprd_eic;
> struct resource *res;
> int ret, i;
> + u16 num_banks = 0;

Preserve reversed xmas tree ordering.

...

> @@ -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;
> default:
> break;

Pointless changes. And actually create the code less maintainable.

--
With Best Regards,
Andy Shevchenko

2023-08-09 03:07:15

by Baolin Wang

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

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 <[email protected]>
> ---
> 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.

2023-08-09 06:32:17

by wenhua lin

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

Hi Hugo:
We will re-split the patch submission and explain our reasons for
modification in the submission information, thank you very much for
your review.

Hugo Villeneuve <[email protected]> 于2023年8月9日周三 01:44写道：
>
> On Tue, 8 Aug 2023 11:31:06 +0800
> Wenhua Lin <[email protected]> wrote:
>
> > Automatic calculation through matching nodes,
> > subsequent projects can avoid modifying driver files.
> >
> > Signed-off-by: Wenhua Lin <[email protected]>
> > ---
> > 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
> > * 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,
> > -};
> >
> > 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;
> > 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;
>
> Hi,
> this probably should go in a separate patch as a fix or an
> improvement with proper comments explaining the reason?
>
> > case SPRD_EIC_SYNC:
> > sprd_eic->chip.get = sprd_eic_get;
> > break;
> > case SPRD_EIC_LATCH:
> > + fallthrough;
>
> ditto.
>
> Hugo Villeneuve
>
>
> > default:
> > break;
> > }
> > --
> > 2.17.1
> >

2023-08-09 06:36:07

by wenhua lin

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

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 <[email protected]> 于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 <[email protected]>
> > ---
> > 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.

2023-08-09 13:34:18

by Hugo Villeneuve

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

On Wed, 9 Aug 2023 14:08:47 +0800
wenhua lin <[email protected]> wrote:

> 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.

Hi,
please do not top-post and insert your answers in-line to make it
easier to read. There is good advice here:

https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions

Thank you,
Hugo Villeneuve


> Baolin Wang <[email protected]> 于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 <[email protected]>
> > > ---
> > > 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.