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

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

[Wenhua Lin]

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;
 	case SPRD_EIC_SYNC:
 		sprd_eic->chip.get = sprd_eic_get;
 		break;
 	case SPRD_EIC_LATCH:
+		fallthrough;
 	default:
 		break;
 	}
-- 
2.17.1

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

[Andy Shevchenko]

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

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

[Hugo Villeneuve]

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
> 

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

[Hugo Villeneuve]

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
> 

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

[Baolin Wang]

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.

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

[wenhua lin]

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

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

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

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

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

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

Baolin Wang <baolin.wang@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.

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

[wenhua lin]

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

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

[Hugo Villeneuve]

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.