Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Aug 2023 13:44:42 -0400
From:   Hugo Villeneuve <hugo@hugovil.com>
To:     Wenhua Lin <Wenhua.Lin@unisoc.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Andy Shevchenko <andy@kernel.org>,
        Orson Zhai <orsonzhai@gmail.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        wenhua lin <wenhua.lin1994@gmail.com>,
        Xiongpeng Wu <xiongpeng.wu@unisoc.com>
Message-Id: <20230808134442.75bb6f04b5612c07d3b7d731@hugovil.com>
In-Reply-To: <20230808033106.2174-1-Wenhua.Lin@unisoc.com>
References: <20230808033106.2174-1-Wenhua.Lin@unisoc.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH 1/3] gpio: sprd: Modify the calculation method of eic
 number
Precedence: bulk

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
>