Date:   Wed, 11 Jan 2017 11:53:27 -0600
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Keerthy <j-keerthy@...com>, <linus.walleij@...aro.org>,
        <t-kristo@...com>
CC:     <linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <gnurou@...il.com>
Subject: Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios
 in one gpio chip



On 01/10/2017 11:00 PM, Keerthy wrote:
> The Davinci GPIO driver is implemented to work with one monolithic
> Davinci GPIO platform device which may have up to Y(144) gpios.
> The Davinci GPIO driver instantiates number of GPIO chips with
> max 32 gpio pins per each during initialization and one IRQ domain.
> So, the current GPIO's  opjects structure is:
> 
> <platform device> Davinci GPIO controller
>  |- <gpio0_chip0> ------|
>  ...                    |--- irq_domain (hwirq [0..143])
>  |- <gpio0_chipN> ------|
> 
> Current driver creates one chip for every 32 GPIOs in a controller.
> This was a limitation earlier now there is no need for that. Hence
> redesigning the driver to create one gpio chip for all the ngpio
> in the controller.
> 
> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).
> 
> The previous discussion on this can be found here:
> https://www.spinics.net/lists/linux-omap/msg132869.html

nice rework.

> 
> Signed-off-by: Keerthy <j-keerthy@...com>
> ---
> 
> Boot tested on Davinci platform.
> 
>  drivers/gpio/gpio-davinci.c                | 127 +++++++++++++++++------------
>  include/linux/platform_data/gpio-davinci.h |  13 ++-
>  2 files changed, 84 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 26b874a..6c1c00a 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -63,11 +63,13 @@ static inline int __davinci_direction(struct gpio_chip *chip,
>  			unsigned offset, bool out, int value)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
>  	unsigned long flags;
>  	u32 temp;
> -	u32 mask = 1 << offset;
> +	int bank = offset / 32;
> +	u32 mask = 1 << (offset % 32);
>  
> +	g = d->regs[bank];
>  	spin_lock_irqsave(&d->lock, flags);
>  	temp = readl_relaxed(&g->dir);
>  	if (out) {
> @@ -103,9 +105,12 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
>  static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
> +	int bank = offset / 32;
>  
> -	return !!((1 << offset) & readl_relaxed(&g->in_data));
> +	g = d->regs[bank];
> +
> +	return !!((1 << (offset % 32)) & readl_relaxed(&g->in_data));

(1 << (offset % 32) is __gpio_mask()

>  }
>  
>  /*
> @@ -115,9 +120,13 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>  davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
> +	int bank = offset / 32;
>  
> -	writel_relaxed((1 << offset), value ? &g->set_data : &g->clr_data);
> +	g = d->regs[bank];
> +
> +	writel_relaxed((1 << (offset % 32)),
> +		       value ? &g->set_data : &g->clr_data);
>  }
>  
>  static struct davinci_gpio_platform_data *
> @@ -165,7 +174,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>  	if (gpiospec->args[0] > pdata->ngpio)
>  		return -EINVAL;
>  
> -	if (gc != &chips[gpiospec->args[0] / 32].chip)
> +	if (gc != &chips->chip)
>  		return -EINVAL;
>  
>  	if (flags)
> @@ -177,11 +186,11 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>  
>  static int davinci_gpio_probe(struct platform_device *pdev)
>  {
> -	int i, base;
> +	static int ctrl_num;
> +	int gpio, bank;
>  	unsigned ngpio, nbank;
>  	struct davinci_gpio_controller *chips;
>  	struct davinci_gpio_platform_data *pdata;
> -	struct davinci_gpio_regs __iomem *regs;
>  	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	char label[MAX_LABEL_SIZE];
> @@ -220,38 +229,30 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  	if (IS_ERR(gpio_base))
>  		return PTR_ERR(gpio_base);
>  
> -	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> -		snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", i);
> -		chips[i].chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
> -		if (!chips[i].chip.label)
> +	snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
> +	chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
> +		if (!chips->chip.label)
>  			return -ENOMEM;
>  
> -		chips[i].chip.direction_input = davinci_direction_in;
> -		chips[i].chip.get = davinci_gpio_get;
> -		chips[i].chip.direction_output = davinci_direction_out;
> -		chips[i].chip.set = davinci_gpio_set;
> +	chips->chip.direction_input = davinci_direction_in;
> +	chips->chip.get = davinci_gpio_get;
> +	chips->chip.direction_output = davinci_direction_out;
> +	chips->chip.set = davinci_gpio_set;
>  
> -		chips[i].chip.base = base;
> -		chips[i].chip.ngpio = ngpio - base;
> -		if (chips[i].chip.ngpio > 32)
> -			chips[i].chip.ngpio = 32;
> +	chips->chip.ngpio = ngpio;
>  
>  #ifdef CONFIG_OF_GPIO
> -		chips[i].chip.of_gpio_n_cells = 2;
> -		chips[i].chip.of_xlate = davinci_gpio_of_xlate;
> -		chips[i].chip.parent = dev;
> -		chips[i].chip.of_node = dev->of_node;
> +	chips->chip.of_gpio_n_cells = 2;
> +	chips->chip.of_xlate = davinci_gpio_of_xlate;

I think It's not necessary to have custom .xlate() and
it can be removed, then gpiolib will assign default one of_gpio_simple_xlate().

> +	chips->chip.parent = dev;
> +	chips->chip.of_node = dev->of_node;
>  #endif
> -		spin_lock_init(&chips[i].lock);
> -
> -		regs = gpio_base + offset_array[i];
> -		if (!regs)
> -			return -ENXIO;
> -		chips[i].regs = regs;
> +	spin_lock_init(&chips->lock);
>  
> -		gpiochip_add_data(&chips[i].chip, &chips[i]);
> -	}
> +	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
> +		chips->regs[bank] = gpio_base + offset_array[bank];
>  
> +	gpiochip_add_data(&chips->chip, chips);
>  	platform_set_drvdata(pdev, chips);
>  	davinci_gpio_irq_setup(pdev);
>  	return 0;
> @@ -312,16 +313,19 @@ static int gpio_irq_type(struct irq_data *d, unsigned trigger)
>  
>  static void gpio_irq_handler(struct irq_desc *desc)
>  {
> -	unsigned int irq = irq_desc_get_irq(desc);
>  	struct davinci_gpio_regs __iomem *g;
>  	u32 mask = 0xffff;
> +	int bank_num;
>  	struct davinci_gpio_controller *d;
> +	struct davinci_gpio_irq_data *irqdata;
>  
> -	d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
> -	g = (struct davinci_gpio_regs __iomem *)d->regs;
> +	irqdata = (struct davinci_gpio_irq_data *)irq_desc_get_handler_data(desc);
> +	bank_num = irqdata->bank_num;
> +	g = irqdata->regs;
> +	d = irqdata->chip;
>  
>  	/* we only care about one bank */
> -	if (irq & 1)
> +	if ((bank_num % 2) == 1)
>  		mask <<= 16;
>  
>  	/* temporarily mask (level sensitive) parent IRQ */
> @@ -329,6 +333,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
>  	while (1) {
>  		u32		status;
>  		int		bit;
> +		irq_hw_number_t hw_irq;
>  
>  		/* ack any irqs */
>  		status = readl_relaxed(&g->intstat) & mask;
> @@ -341,9 +346,13 @@ static void gpio_irq_handler(struct irq_desc *desc)
>  		while (status) {
>  			bit = __ffs(status);
>  			status &= ~BIT(bit);
> +			/* Max number of gpios per controller is 144 so
> +			 * hw_irq will be in [0..143]
> +			 */
> +			hw_irq = (bank_num / 2) * 32 + bit;
> +
>  			generic_handle_irq(
> -				irq_find_mapping(d->irq_domain,
> -						 d->chip.base + bit));
> +				irq_find_mapping(d->irq_domain, hw_irq));
>  		}
>  	}
>  	chained_irq_exit(irq_desc_get_chip(desc), desc);
> @@ -355,7 +364,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>  
>  	if (d->irq_domain)
> -		return irq_create_mapping(d->irq_domain, d->chip.base + offset);
> +		return irq_create_mapping(d->irq_domain, offset);
>  	else
>  		return -ENXIO;
>  }
> @@ -369,7 +378,7 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
>  	 * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
>  	 */
>  	if (offset < d->gpio_unbanked)
> -		return d->gpio_irq + offset;
> +		return d->base_irq + offset;
>  	else
>  		return -ENODEV;
>  }
> @@ -382,7 +391,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>  
>  	d = (struct davinci_gpio_controller *)irq_data_get_irq_handler_data(data);
>  	g = (struct davinci_gpio_regs __iomem *)d->regs;
> -	mask = __gpio_mask(data->irq - d->gpio_irq);
> +	mask = __gpio_mask(data->irq - d->base_irq);
>  
>  	if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>  		return -EINVAL;
> @@ -401,7 +410,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>  {
>  	struct davinci_gpio_controller *chips =
>  				(struct davinci_gpio_controller *)d->host_data;
> -	struct davinci_gpio_regs __iomem *g = chips[hw / 32].regs;
> +	struct davinci_gpio_regs __iomem *g = chips->regs[hw / 32];
>  
>  	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
>  				"davinci_gpio");
> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	struct irq_domain	*irq_domain = NULL;
>  	const struct of_device_id *match;
>  	struct irq_chip *irq_chip;
> +	struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];

You declare irqdata as array here but it has not been used anywhere
except for assignment. Could you remove this array and MAX_BANKED_IRQS define?

Seems you can just use struct davinci_gpio_irq_data *irqdata;

>  	gpio_get_irq_chip_cb_t gpio_get_irq_chip;
>  
>  	/*
> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 * IRQs, while the others use banked IRQs, would need some setup
>  	 * tweaks to recognize hardware which can do that.
>  	 */
> -	for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
> -		chips[bank].chip.to_irq = gpio_to_irq_banked;
> -		chips[bank].irq_domain = irq_domain;
> -	}
> +	chips->chip.to_irq = gpio_to_irq_banked;
> +	chips->irq_domain = irq_domain;
>  
>  	/*
>  	 * AINTC can handle direct/unbanked IRQs for GPIOs, with the GPIO
> @@ -526,9 +534,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 */
>  	if (pdata->gpio_unbanked) {
>  		/* pass "bank 0" GPIO IRQs to AINTC */
> -		chips[0].chip.to_irq = gpio_to_irq_unbanked;
> -		chips[0].gpio_irq = bank_irq;
> -		chips[0].gpio_unbanked = pdata->gpio_unbanked;
> +		chips->chip.to_irq = gpio_to_irq_unbanked;
> +		chips->base_irq = bank_irq;
> +		chips->gpio_unbanked = pdata->gpio_unbanked;
>  		binten = GENMASK(pdata->gpio_unbanked / 16, 0);
>  
>  		/* AINTC handles mask/unmask; GPIO handles triggering */
> @@ -538,14 +546,14 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  		irq_chip->irq_set_type = gpio_irq_type_unbanked;
>  
>  		/* default trigger: both edges */
> -		g = chips[0].regs;
> +		g = chips->regs[0];
>  		writel_relaxed(~0, &g->set_falling);
>  		writel_relaxed(~0, &g->set_rising);
>  
>  		/* set the direct IRQs up to use that irqchip */
>  		for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
>  			irq_set_chip(irq, irq_chip);
> -			irq_set_handler_data(irq, &chips[gpio / 32]);
> +			irq_set_handler_data(irq, chips);
>  			irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
>  		}
>  
> @@ -558,7 +566,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 */
>  	for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
>  		/* disabled by default, enabled only as needed */
> -		g = chips[bank / 2].regs;
> +		g = chips->regs[bank / 2];
>  		writel_relaxed(~0, &g->clr_falling);
>  		writel_relaxed(~0, &g->clr_rising);
>  
> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  		 * gpio irqs. Pass the irq bank's corresponding controller to
>  		 * the chained irq handler.
>  		 */
> +		irqdata[bank] = devm_kzalloc(&pdev->dev,
> +					     sizeof(struct
> +						    davinci_gpio_irq_data),
> +					     GFP_KERNEL);
> +		if (!irqdata[bank])
> +			return -ENOMEM;
> +
> +		irqdata[bank]->regs = g;
> +		irqdata[bank]->bank_num = bank;
> +		irqdata[bank]->chip = chips;
> +
>  		irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
> -						 &chips[gpio / 32]);
> +						 irqdata[bank]);
>  
>  		binten |= BIT(bank);
>  	}
> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> index 18127c4..ca09686 100644
> --- a/include/linux/platform_data/gpio-davinci.h
> +++ b/include/linux/platform_data/gpio-davinci.h
> @@ -21,19 +21,28 @@
>  
>  #include <asm-generic/gpio.h>
>  
> +#define MAX_BANKS		5

probably MAX_REGS_BANKS will be better, as it defines
max number of idnetical registers sets and not number of gpio banks.

> +#define MAX_BANKED_IRQS		9
> +
>  struct davinci_gpio_platform_data {
>  	u32	ngpio;
>  	u32	gpio_unbanked;
>  };
>  
> +struct davinci_gpio_irq_data {
> +	void __iomem			*regs;
> +	struct davinci_gpio_controller	*chip;
> +	int				bank_num;
> +};
> +
>  struct davinci_gpio_controller {
>  	struct gpio_chip	chip;
>  	struct irq_domain	*irq_domain;
>  	/* Serialize access to GPIO registers */
>  	spinlock_t		lock;
> -	void __iomem		*regs;
> +	void __iomem		*regs[MAX_BANKS];
>  	int			gpio_unbanked;
> -	unsigned		gpio_irq;
> +	unsigned int		base_irq;
>  };
>  
>  /*
> 

-- 
regards,
-grygorii

Powered by blists - more mailing lists