[PATCH V3 0/4] gpio: eic-sprd: Modification of UNISOC Platform EIC Driver

[PATCH V3 0/4] gpio: eic-sprd: Modification of UNISOC Platform EIC Driver

[Wenhua Lin]

Recently, some bugs have been discovered during use, and
patch2 are bug fixes. Also, this patchset add optimization:
patch1 can support eic debouce wake-up system and
patch3 optimization the calculation method of eic number,
and patch4 Support 8 banks EIC controller.

Change in V3:
-Using thread send 4 patches 

-Change title and commit message in PATCH 1/4.
-Delete fixes tag in PATCH 1/4.

-Change commit message in PATCH 2/4.

-Move num_banks++ to the back of sprd_eic->base in PATCH 3/4.
-Delete fixes tag in PATCH 3/4.
-Modify misindented issue in PATCH 3/4.
-Preserve reversed xmas tree order in PATCH 3/4.

-Change related comments in PATCH 4/4.

Wenhua Lin (4):
  gpio: eic-sprd: Keep the clock rtc_1k on
  gpio: eic-sprd: Clear interrupt after set the interrupt type
  gpio: eic-sprd: Modify the calculation method of eic number
  gpio: eic-sprd: Support 8 banks EIC controller

 drivers/gpio/gpio-eic-sprd.c | 65 +++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[Wenhua Lin]

The eic debounce does not have a clock of rtc_1k in the sleep state,
but the eic debounce will be used to wake up the system, therefore the
clock of rtc_1k needs to be kept open.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 drivers/gpio/gpio-eic-sprd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index be7f2fa5aa7b..bdcb3510a208 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -24,6 +24,7 @@
 #define SPRD_EIC_DBNC_IC		0x24
 #define SPRD_EIC_DBNC_TRIG		0x28
 #define SPRD_EIC_DBNC_CTRL0		0x40
+#define SPRD_EIC_DBNC_FORCE_CLK		0x8000
 
 #define SPRD_EIC_LATCH_INTEN		0x0
 #define SPRD_EIC_LATCH_INTRAW		0x4
@@ -223,6 +224,7 @@ static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
 	u32 value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
 
 	value |= (debounce / 1000) & SPRD_EIC_DBNC_MASK;
+	value |= SPRD_EIC_DBNC_FORCE_CLK;
 	writel_relaxed(value, base + reg);
 
 	return 0;
-- 
2.17.1

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[Andy Shevchenko]

On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> The eic debounce does not have a clock of rtc_1k in the sleep state,
> but the eic debounce will be used to wake up the system, therefore the
> clock of rtc_1k needs to be kept open.

...

> +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000

BIT(15) ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[wenhua lin]

On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> >
> > The eic debounce does not have a clock of rtc_1k in the sleep state,
> > but the eic debounce will be used to wake up the system, therefore the
> > clock of rtc_1k needs to be kept open.
>
> ...
>
> > +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000
>
> BIT(15) ?
>

Yes, writing 1 to bit15 of the register can ensure that the clock of
rtc_1k remains normally on.

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[Chunyan Zhang]

On Fri, 5 Jan 2024 at 10:11, wenhua lin <wenhua.lin1994@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> > >
> > > The eic debounce does not have a clock of rtc_1k in the sleep state,
> > > but the eic debounce will be used to wake up the system, therefore the
> > > clock of rtc_1k needs to be kept open.
> >
> > ...
> >
> > > +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000
> >
> > BIT(15) ?
> >
>
> Yes, writing 1 to bit15 of the register can ensure that the clock of
> rtc_1k remains normally on.

Andy's comment means that you should use BIT(15) instead of 0x8000.

>
> > --
> > With Best Regards,
> > Andy Shevchenko

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[wenhua lin]

On Fri, Jan 5, 2024 at 10:18 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Fri, 5 Jan 2024 at 10:11, wenhua lin <wenhua.lin1994@gmail.com> wrote:
> >
> > On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> > > >
> > > > The eic debounce does not have a clock of rtc_1k in the sleep state,
> > > > but the eic debounce will be used to wake up the system, therefore the
> > > > clock of rtc_1k needs to be kept open.
> > >
> > > ...
> > >
> > > > +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000
> > >
> > > BIT(15) ?
> > >
> >
> > Yes, writing 1 to bit15 of the register can ensure that the clock of
> > rtc_1k remains normally on.
>
> Andy's comment means that you should use BIT(15) instead of 0x8000.
>

OK, thank you very much for your explanation, I will make changes in patch v4.

> >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[Chunyan Zhang]

On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> The eic debounce does not have a clock of rtc_1k in the sleep state,
> but the eic debounce will be used to wake up the system, therefore the
> clock of rtc_1k needs to be kept open.

It seems that this issue is not in the latest SoCs. I would suggest
not changing for the time being.

Thanks,
Chunyan


>
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
>  drivers/gpio/gpio-eic-sprd.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index be7f2fa5aa7b..bdcb3510a208 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
> @@ -24,6 +24,7 @@
>  #define SPRD_EIC_DBNC_IC               0x24
>  #define SPRD_EIC_DBNC_TRIG             0x28
>  #define SPRD_EIC_DBNC_CTRL0            0x40
> +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000
>
>  #define SPRD_EIC_LATCH_INTEN           0x0
>  #define SPRD_EIC_LATCH_INTRAW          0x4
> @@ -223,6 +224,7 @@ static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
>         u32 value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
>
>         value |= (debounce / 1000) & SPRD_EIC_DBNC_MASK;
> +       value |= SPRD_EIC_DBNC_FORCE_CLK;
>         writel_relaxed(value, base + reg);
>
>         return 0;
> --
> 2.17.1
>

Re: [PATCH V3 1/4] gpio: eic-sprd: Keep the clock rtc_1k on

[wenhua lin]

On Fri, Jan 5, 2024 at 11:26 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> >
> > The eic debounce does not have a clock of rtc_1k in the sleep state,
> > but the eic debounce will be used to wake up the system, therefore the
> > clock of rtc_1k needs to be kept open.
>
> It seems that this issue is not in the latest SoCs. I would suggest
> not changing for the time being.
>

OK, we will delete this modification on pacth v4.

> Thanks,
> Chunyan
>
>
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> > ---
> >  drivers/gpio/gpio-eic-sprd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> > index be7f2fa5aa7b..bdcb3510a208 100644
> > --- a/drivers/gpio/gpio-eic-sprd.c
> > +++ b/drivers/gpio/gpio-eic-sprd.c
> > @@ -24,6 +24,7 @@
> >  #define SPRD_EIC_DBNC_IC               0x24
> >  #define SPRD_EIC_DBNC_TRIG             0x28
> >  #define SPRD_EIC_DBNC_CTRL0            0x40
> > +#define SPRD_EIC_DBNC_FORCE_CLK                0x8000
> >
> >  #define SPRD_EIC_LATCH_INTEN           0x0
> >  #define SPRD_EIC_LATCH_INTRAW          0x4
> > @@ -223,6 +224,7 @@ static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
> >         u32 value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
> >
> >         value |= (debounce / 1000) & SPRD_EIC_DBNC_MASK;
> > +       value |= SPRD_EIC_DBNC_FORCE_CLK;
> >         writel_relaxed(value, base + reg);
> >
> >         return 0;
> > --
> > 2.17.1
> >

[PATCH V3 2/4] gpio: eic-sprd: Clear interrupt after set the interrupt type

[Wenhua Lin]

The initialization state of the eic module is a high level trigger.
If it is currently a high level, the interrupt condition is met at
this time, and the eic interrupt has a latch capability, which will
cause an interrupt to occur after booting. To avoid this, When setting
the eic interrupt trigger type, clear the interrupt once.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 drivers/gpio/gpio-eic-sprd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index bdcb3510a208..e492157e5154 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -384,29 +384,34 @@ static int sprd_eic_irq_set_type(struct irq_data *data, unsigned int flow_type)
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_EDGE_FALLING:
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_EDGE_BOTH:
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_LEVEL_HIGH:
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 1);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_level_irq);
 			break;
 		case IRQ_TYPE_LEVEL_LOW:
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 1);
 			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_level_irq);
 			break;
 		default:
@@ -419,29 +424,34 @@ static int sprd_eic_irq_set_type(struct irq_data *data, unsigned int flow_type)
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_EDGE_FALLING:
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_EDGE_BOTH:
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_edge_irq);
 			break;
 		case IRQ_TYPE_LEVEL_HIGH:
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 1);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_level_irq);
 			break;
 		case IRQ_TYPE_LEVEL_LOW:
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 1);
 			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
 			irq_set_handler_locked(data, handle_level_irq);
 			break;
 		default:
-- 
2.17.1

[PATCH V3 3/4] gpio: eic-sprd: Modify the calculation method of eic number

[Wenhua Lin]

When the soc changes, the corresponding gpio-eic-sprd.c
code needs to be modified, and the corresponding
Document must also be modified, which is quite troublesome.
To avoid modifying the driver file, the number of eics
is automatically calculated by matching dts nodes.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 drivers/gpio/gpio-eic-sprd.c | 45 ++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index e492157e5154..1ca3c444957c 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -109,34 +109,32 @@ static struct sprd_eic *to_sprd_eic(struct notifier_block *nb)
 
 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)
 {
@@ -607,6 +605,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
 	struct gpio_irq_chip *irq;
 	struct sprd_eic *sprd_eic;
 	struct resource *res;
+	u16 num_banks = 0;
 	int ret, i;
 
 	pdata = of_device_get_match_data(dev);
@@ -640,10 +639,12 @@ static int sprd_eic_probe(struct platform_device *pdev)
 		sprd_eic->base[i] = devm_ioremap_resource(dev, res);
 		if (IS_ERR(sprd_eic->base[i]))
 			return PTR_ERR(sprd_eic->base[i]);
+
+		num_banks++;
 	}
 
 	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 = dev;
 	sprd_eic->chip.direction_input = sprd_eic_direction_input;
-- 
2.17.1

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

[Andy Shevchenko]

On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> When the soc changes, the corresponding gpio-eic-sprd.c

SoC

> code needs to be modified, and the corresponding
> Document must also be modified, which is quite troublesome.
> To avoid modifying the driver file, the number of eics
> is automatically calculated by matching dts nodes.

-- 
With Best Regards,
Andy Shevchenko

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

[wenhua lin]

On Thu, Jan 4, 2024 at 9:01 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> >
> > When the soc changes, the corresponding gpio-eic-sprd.c
>
> SoC
>

Thank you very much for your review.
I will fix this issue in patch v4.

> > code needs to be modified, and the corresponding
> > Document must also be modified, which is quite troublesome.
> > To avoid modifying the driver file, the number of eics
> > is automatically calculated by matching dts nodes.
>
> --
> With Best Regards,
> Andy Shevchenko

[PATCH V3 4/4] gpio: eic-sprd: Support 8 banks EIC controller

[Wenhua Lin]

In order to solve the problem of insufficient eic,
it supports 8 banks of eic controller, each bank contains 8 eic.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 drivers/gpio/gpio-eic-sprd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index 1ca3c444957c..715c7d581d7f 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -52,10 +52,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))
@@ -627,9 +627,9 @@ static int sprd_eic_probe(struct platform_device *pdev)
 
 	for (i = 0; i < SPRD_EIC_MAX_BANK; i++) {
 		/*
-		 * We can have maximum 3 banks EICs, and each EIC has
+		 * We can have maximum 8 banks EICs, and each EIC has
 		 * its own base address. But some platform maybe only
-		 * have one bank EIC, thus base[1] and base[2] can be
+		 * have one bank EIC, thus base[1] to base[7] can be
 		 * optional.
 		 */
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
-- 
2.17.1

Re: [PATCH V3 4/4] gpio: eic-sprd: Support 8 banks EIC controller

[Chunyan Zhang]

On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> In order to solve the problem of insufficient eic,
> it supports 8 banks of eic controller, each bank contains 8 eic.
>
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
>  drivers/gpio/gpio-eic-sprd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index 1ca3c444957c..715c7d581d7f 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
> @@ -52,10 +52,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

This change seems not backward compatible.

Also this is not flexible to support more SoCs which may have more
than 8 banks (if we have this kind of SoCs in the future).

I would suggest adding a new item like 'bank_nums' into sprd_eic_variant_data.

Thanks,
Chunyan

>  #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))
> @@ -627,9 +627,9 @@ static int sprd_eic_probe(struct platform_device *pdev)
>
>         for (i = 0; i < SPRD_EIC_MAX_BANK; i++) {
>                 /*
> -                * We can have maximum 3 banks EICs, and each EIC has
> +                * We can have maximum 8 banks EICs, and each EIC has
>                  * its own base address. But some platform maybe only
> -                * have one bank EIC, thus base[1] and base[2] can be
> +                * have one bank EIC, thus base[1] to base[7] can be
>                  * optional.
>                  */
>                 res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> --
> 2.17.1
>

Re: [PATCH V3 4/4] gpio: eic-sprd: Support 8 banks EIC controller

[wenhua lin]

On Mon, Jan 8, 2024 at 4:56 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
> >
> > In order to solve the problem of insufficient eic,
> > it supports 8 banks of eic controller, each bank contains 8 eic.
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> > ---
> >  drivers/gpio/gpio-eic-sprd.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> > index 1ca3c444957c..715c7d581d7f 100644
> > --- a/drivers/gpio/gpio-eic-sprd.c
> > +++ b/drivers/gpio/gpio-eic-sprd.c
> > @@ -52,10 +52,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
>
> This change seems not backward compatible.
>
> Also this is not flexible to support more SoCs which may have more
> than 8 banks (if we have this kind of SoCs in the future).
>
> I would suggest adding a new item like 'bank_nums' into sprd_eic_variant_data.
>
> Thanks,
> Chunyan
>

We will refer to this plan for modifications.

> >  #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))
> > @@ -627,9 +627,9 @@ static int sprd_eic_probe(struct platform_device *pdev)
> >
> >         for (i = 0; i < SPRD_EIC_MAX_BANK; i++) {
> >                 /*
> > -                * We can have maximum 3 banks EICs, and each EIC has
> > +                * We can have maximum 8 banks EICs, and each EIC has
> >                  * its own base address. But some platform maybe only
> > -                * have one bank EIC, thus base[1] and base[2] can be
> > +                * have one bank EIC, thus base[1] to base[7] can be
> >                  * optional.
> >                  */
> >                 res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > --
> > 2.17.1
> >