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