| Message ID | 20230808033106.2174-1-Wenhua.Lin@unisoc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] gpio: sprd: Modify the calculation method of eic number | expand |
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.
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 >
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 >
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.
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.
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 > >
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.
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; }
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(-)