2024-01-04 02:43:42

by Wenhua Lin

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

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



2024-01-04 02:43:51

by Wenhua Lin

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

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 <[email protected]>
---
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


2024-01-04 02:44:17

by Wenhua Lin

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

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 <[email protected]>
---
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


2024-01-04 02:44:41

by Wenhua Lin

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

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 <[email protected]>
---
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


2024-01-04 02:44:46

by Wenhua Lin

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

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 <[email protected]>
---
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


2024-01-04 13:00:41

by Andy Shevchenko

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

On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-04 13:01:50

by Andy Shevchenko

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

On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-05 02:11:42

by wenhua lin

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

On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-05 02:14:20

by wenhua lin

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

On Thu, Jan 4, 2024 at 9:01 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-05 02:18:38

by Chunyan Zhang

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

On Fri, 5 Jan 2024 at 10:11, wenhua lin <[email protected]> wrote:
>
> On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-05 02:28:24

by wenhua lin

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

On Fri, Jan 5, 2024 at 10:18 AM Chunyan Zhang <[email protected]> wrote:
>
> On Fri, 5 Jan 2024 at 10:11, wenhua lin <[email protected]> wrote:
> >
> > On Thu, Jan 4, 2024 at 9:00 PM Andy Shevchenko
> > <[email protected]> wrote:
> > >
> > > On Thu, Jan 4, 2024 at 4:43 AM Wenhua Lin <[email protected]> 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

2024-01-05 03:26:25

by Chunyan Zhang

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

On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <[email protected]> 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 <[email protected]>
> ---
> 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
>

2024-01-08 07:37:12

by wenhua lin

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

On Fri, Jan 5, 2024 at 11:26 AM Chunyan Zhang <[email protected]> wrote:
>
> On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >

2024-01-08 08:56:26

by Chunyan Zhang

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

On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <[email protected]> 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 <[email protected]>
> ---
> 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
>

2024-01-09 07:59:30

by wenhua lin

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

On Mon, Jan 8, 2024 at 4:56 PM Chunyan Zhang <[email protected]> wrote:
>
> On Thu, 4 Jan 2024 at 10:43, Wenhua Lin <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >