* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-08 3:31 [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number Wenhua Lin
@ 2023-08-08 13:22 ` Andy Shevchenko
2023-08-08 17:34 ` Hugo Villeneuve
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2023-08-08 13:22 UTC (permalink / raw)
To: Wenhua Lin
Cc: Linus Walleij, Bartosz Golaszewski, Orson Zhai, Baolin Wang,
Chunyan Zhang, linux-serial, linux-kernel, wenhua lin,
Xiongpeng Wu
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-08 3:31 [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number Wenhua Lin
2023-08-08 13:22 ` Andy Shevchenko
@ 2023-08-08 17:34 ` Hugo Villeneuve
2023-08-08 17:44 ` Hugo Villeneuve
2023-08-09 1:23 ` Baolin Wang
3 siblings, 0 replies; 8+ messages in thread
From: Hugo Villeneuve @ 2023-08-08 17:34 UTC (permalink / raw)
To: Wenhua Lin
Cc: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Orson Zhai,
Baolin Wang, Chunyan Zhang, linux-serial, linux-kernel,
wenhua lin, Xiongpeng Wu
On Tue, 8 Aug 2023 11:31:06 +0800
Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> Automatic calculation through matching nodes,
> subsequent projects can avoid modifying driver files.
>
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-08 3:31 [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number Wenhua Lin
2023-08-08 13:22 ` Andy Shevchenko
2023-08-08 17:34 ` Hugo Villeneuve
@ 2023-08-08 17:44 ` Hugo Villeneuve
2023-08-09 6:11 ` wenhua lin
2023-08-09 1:23 ` Baolin Wang
3 siblings, 1 reply; 8+ messages in thread
From: Hugo Villeneuve @ 2023-08-08 17:44 UTC (permalink / raw)
To: Wenhua Lin
Cc: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Orson Zhai,
Baolin Wang, Chunyan Zhang, linux-serial, linux-kernel,
wenhua lin, Xiongpeng Wu
On Tue, 8 Aug 2023 11:31:06 +0800
Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> Automatic calculation through matching nodes,
> subsequent projects can avoid modifying driver files.
>
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-08 17:44 ` Hugo Villeneuve
@ 2023-08-09 6:11 ` wenhua lin
0 siblings, 0 replies; 8+ messages in thread
From: wenhua lin @ 2023-08-09 6:11 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: Wenhua Lin, Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Orson Zhai, Baolin Wang, Chunyan Zhang, linux-serial,
linux-kernel, Xiongpeng Wu
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 <hugo@hugovil.com> 于2023年8月9日周三 01:44写道：
>
> On Tue, 8 Aug 2023 11:31:06 +0800
> Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> > Automatic calculation through matching nodes,
> > subsequent projects can avoid modifying driver files.
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-08 3:31 [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number Wenhua Lin
` (2 preceding siblings ...)
2023-08-08 17:44 ` Hugo Villeneuve
@ 2023-08-09 1:23 ` Baolin Wang
2023-08-09 6:08 ` wenhua lin
3 siblings, 1 reply; 8+ messages in thread
From: Baolin Wang @ 2023-08-09 1:23 UTC (permalink / raw)
To: Wenhua Lin, Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Orson Zhai, Chunyan Zhang
Cc: linux-serial, linux-kernel, wenhua lin, Xiongpeng Wu
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@unisoc.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-09 1:23 ` Baolin Wang
@ 2023-08-09 6:08 ` wenhua lin
2023-08-09 13:02 ` Hugo Villeneuve
0 siblings, 1 reply; 8+ messages in thread
From: wenhua lin @ 2023-08-09 6:08 UTC (permalink / raw)
To: Baolin Wang
Cc: Wenhua Lin, Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Orson Zhai, Chunyan Zhang, linux-serial, linux-kernel,
Xiongpeng Wu
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@linux.alibaba.com> 于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@unisoc.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic number
2023-08-09 6:08 ` wenhua lin
@ 2023-08-09 13:02 ` Hugo Villeneuve
0 siblings, 0 replies; 8+ messages in thread
From: Hugo Villeneuve @ 2023-08-09 13:02 UTC (permalink / raw)
To: wenhua lin
Cc: Baolin Wang, Wenhua Lin, Linus Walleij, Bartosz Golaszewski,
Andy Shevchenko, Orson Zhai, Chunyan Zhang, linux-serial,
linux-kernel, Xiongpeng Wu
On Wed, 9 Aug 2023 14:08:47 +0800
wenhua lin <wenhua.lin1994@gmail.com> 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 <baolin.wang@linux.alibaba.com> 于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@unisoc.com>
> > > ---
> > > 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.
^ permalink raw reply [flat|nested] 8+ messages in thread