Re: [PATCH v4 01/42] gpio: ep93xx: split device in multiple

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Fri, Sep 15, 2023 at 11:10:43AM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> 
> Prepare ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
> 
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
> 
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.

...

>  #include <linux/reboot.h>
>  #include <linux/usb/ohci_pdriver.h>
>  #include <linux/random.h>
> +#include <linux/ioport.h>

Can it be squeezed to the most ordered part?

...

> +static struct resource ep93xx_a_gpio_resources[] = {
> +	DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE,        0x04, "data"),

You can also use " + 0x00", but it's up to you.

> +	DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE + 0x10, 0x04, "dir"),
> +	DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE + 0x90, 0x1c, "intr"),
>  	DEFINE_RES_IRQ(IRQ_EP93XX_GPIO_AB),
> +};

...

>  	.dev_id		= "i2c-gpio.0",
>  	.table		= {
>  		/* Use local offsets on gpiochip/port "G" */
> -		GPIO_LOOKUP_IDX("G", 1, NULL, 0,
> +		GPIO_LOOKUP_IDX("gpio-ep93xx.6", 1, NULL, 0,
>  				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),
> -		GPIO_LOOKUP_IDX("G", 0, NULL, 1,
> +		GPIO_LOOKUP_IDX("gpio-ep93xx.6", 0, NULL, 1,
>  				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),

Here the terminator is missing, I believe you need to have a separate fix patch
done first to prepend this one.

>  	},

...

>  #include <linux/seq_file.h>
> +#include <linux/interrupt.h>

Please, squeeze this to the most sorted part.

...

>  static void ep93xx_gpio_irq_mask_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
> -	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -	int port_mask = BIT(d->irq & 7);

	irq_hw_number_t hwirq = irqd_to_hwirq(d);

> +	int port_mask = BIT(hwirq);
>  
>  	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
>  		eic->int_type2 ^= port_mask; /* switch edge direction */
>  
>  	eic->int_unmasked &= ~port_mask;
> -	ep93xx_gpio_update_int_params(epg, eic);
> +	ep93xx_gpio_update_int_params(eic);
>  
> -	writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET);
> +	writeb(port_mask, eic->base + EP93XX_INT_EOI_OFFSET);

>  	gpiochip_disable_irq(gc, irqd_to_hwirq(d));

	gpiochip_disable_irq(gc, hwirq);

>  }

...

>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
> -	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
>  
> -	eic->int_unmasked &= ~BIT(d->irq & 7);
> -	ep93xx_gpio_update_int_params(epg, eic);
> +	eic->int_unmasked &= ~BIT(irqd_to_hwirq(d));
> +	ep93xx_gpio_update_int_params(eic);
>  	gpiochip_disable_irq(gc, irqd_to_hwirq(d));
>  }

As per above.

...

> @@ -203,11 +164,10 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
> -	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
>  
>  	gpiochip_enable_irq(gc, irqd_to_hwirq(d));
> -	eic->int_unmasked |= BIT(d->irq & 7);
> -	ep93xx_gpio_update_int_params(epg, eic);
> +	eic->int_unmasked |= BIT(irqd_to_hwirq(d));
> +	ep93xx_gpio_update_int_params(eic);
>  }

As per above.

...

> +	irq_hw_number_t offset = irqd_to_hwirq(d);

(or use hwirq name to be consistent in the code).

...

> +	girq->parents = devm_kcalloc(dev, girq->num_parents,
> +				   sizeof(*girq->parents),

I would put this one to the previous line.

> +				   GFP_KERNEL);
> +	if (!girq->parents)
> +		return -ENOMEM;

...

> +			irq = platform_get_irq(pdev, i);

This will issue a warning or error message if IRQ is not found...

> +			if (irq < 0)

> +				continue;

...but this suggests that that is optional. Hence

			irq = platform_get_irq_optional(pdev, i);

?

...

> +	if (platform_irq_count(pdev) > 0) {
> +		dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev));
> +		ret = ep93xx_setup_irqs(pdev, egc);
> +		if (ret)
> +			dev_err_probe(&pdev->dev, ret, "setup irqs failed");

Hmm... Missing return?

>  	}

-- 
With Best Regards,
Andy Shevchenko