Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
User-Agent: Cyrus-JMAP/3.1.7-186-gf4cb3c3-fmstable-20190904v1
Mime-Version: 1.0
Message-Id: <40601711-5fcf-40a0-bfc2-ae5043948a41@www.fastmail.com>
In-Reply-To: <20190905011732.16059-1-rashmica.g@gmail.com>
References: <20190905011732.16059-1-rashmica.g@gmail.com>
Date:   Thu, 05 Sep 2019 13:27:23 +0930
From:   "Andrew Jeffery" <andrew@aj.id.au>
To:     "Rashmica Gupta" <rashmica.g@gmail.com>,
        "Linus Walleij" <linus.walleij@linaro.org>
Cc:     "Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
        "Joel Stanley" <joel@jms.id.au>, linux-gpio@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/4] gpio: Add in ast2600 details to Aspeed driver
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Thu, 5 Sep 2019, at 10:47, Rashmica Gupta wrote:
> The ast2600 is a new generation of SoC from ASPEED. Similarly to the
> ast2400 and ast2500, it has a GPIO controller for it's 3.6V GPIO pins.
> Additionally, it has a GPIO controller for 36 1.8V GPIO pins. These
> voltages are fixed and cannot be configured via pinconf, so we need two
> separate drivers for them.

Working backwards, we don't really have multiple drivers, just different
configurations for the same driver. So I think this should be reworded.

Also it's not really the voltage differences that are driving the different
configurations but rather that there are two separate sets of registers
in the 2600 with overlapping bank names (they happen to be split into
3.3V and 1.8V groups). The key point being that there aren't just more
GPIO registers tacked on the end of the original 3.3V group.

> 
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
>  drivers/gpio/gpio-aspeed.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 16c6eaf70857..4723b8780a8c 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -662,12 +662,14 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
>  	struct irq_chip *ic = irq_desc_get_chip(desc);
>  	struct aspeed_gpio *data = gpiochip_get_data(gc);
> -	unsigned int i, p, girq;
> +	unsigned int i, p, girq, banks;
>  	unsigned long reg;
> +	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  
>  	chained_irq_enter(ic, desc);
>  
> -	for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
> +	banks = DIV_ROUND_UP(gpio->config->nr_gpios, 32);
> +	for (i = 0; i < banks; i++) {
>  		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
>  
>  		reg = ioread32(bank_reg(data, bank, reg_irq_status));
> @@ -1108,9 +1110,33 @@ static const struct aspeed_gpio_config ast2500_config =
>  	/* 232 for simplicity, actual number is 228 (4-GPIO hole in GPIOAB) */
>  	{ .nr_gpios = 232, .props = ast2500_bank_props, };
>  
> +static const struct aspeed_bank_props ast2600_bank_props[] = {
> +	/*     input	  output   */
> +	{5, 0xffffffff,  0x0000ffff}, /* U/V/W/X */
> +	{6, 0xffff0000,  0x0fff0000}, /* Y/Z */
> +	{ },
> +};
> +
> +static const struct aspeed_gpio_config ast2600_config =
> +	/* 208 3.6V GPIOs */
> +	{ .nr_gpios = 208, .props = ast2600_bank_props, };
> +
> +static const struct aspeed_bank_props ast2600_1_8v_bank_props[] = {
> +	/*     input	  output   */
> +	{1, 0x0000000f,  0x0000000f}, /* E */

If there are 36 GPIOs then this configuration is suggesting that all of them
are capable of input and output. A handy observation here is that the first
36 GPIOs of the 3.3V GPIO controller in the 2600 also have both capabilities,
so we can re-use the 3.3V configuration if we can limit the number of GPIOs
somehow.

The devicetree binding already describes an `ngpios` property so perhaps
we could make use of this to use the same properties struct instance for both
controllers in the 2600: Require that the property be present for 2600-
compatible devicetree nodes and test for its presence in probe(), then fall
back to the hard-coded value in the config struct if it is not (this keeps
devicetree compatibility for the 2400 and 2500 drivers).

This way we can eliminate the aspeed,ast2600-1-8v-gpio compatible string
below (we just use aspeed,ast2600-gpio for both controllers).

Thoughts?

Andrew

> +	{ },
> +};
> +
> +static const struct aspeed_gpio_config ast2600_1_8v_config =
> +	/* 36 1.8V GPIOs */
> +	{ .nr_gpios = 36, .props = ast2600_1_8v_bank_props, };
> +
>  static const struct of_device_id aspeed_gpio_of_table[] = {
>  	{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
>  	{ .compatible = "aspeed,ast2500-gpio", .data = &ast2500_config, },
> +	{ .compatible = "aspeed,ast2600-gpio", .data = &ast2600_config, },
> +	{ .compatible = "aspeed,ast2600-1-8v-gpio",
> +	  .data = &ast2600_1_8v_config, },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, aspeed_gpio_of_table);
> -- 
> 2.20.1
> 
>