linux-kernel - Re: [PATCH v1 01/43] gpio: ep93xx: split device in multiple

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZHlK9EkHlLD7DDG7@surfacebook>
Date:   Fri, 2 Jun 2023 04:50:44 +0300
From:   andy.shevchenko@...il.com
To:     Nikita Shubin <nikita.shubin@...uefel.me>
Cc:     Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hartley Sweeten <hsweeten@...ionengravers.com>,
        Russell King <linux@...linux.org.uk>,
        Lukasz Majewski <lukma@...x.de>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Michael Peters <mpeters@...eddedts.com>,
        Kris Bahnsen <kris@...eddedts.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org
Subject: Re: [PATCH v1 01/43] gpio: ep93xx: split device in multiple

Thu, Jun 01, 2023 at 08:33:52AM +0300, Nikita Shubin kirjoitti:
> This prepares 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.

First of all, check if you added In-Reply-to email header to the previous
thread, at least `b4` downloaded 188 messages in this one so far. Second,
the previous was kinda v0, while we usually assume that non-versioned series
is v1. This is a bit ambiguous.

...

> +		GPIO_LOOKUP_IDX("gpio-ep93xx.4", 1,	NULL, 1, GPIO_ACTIVE_HIGH),

TAB used instead of space.

...

>  struct device __init *ep93xx_init_devices(void)
>  {
>  	struct device *parent;
> +	int i;

It's unsigned, right?

> +	for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_device); i++)
> +		platform_device_register(ep93xx_gpio_device[i]);

...

>  	writeb(eic->int_debounce,
> -	       epg->base + eic->irq_offset + EP93XX_INT_DEBOUNCE_OFFSET);
> +	       eic->base + EP93XX_INT_DEBOUNCE_OFFSET);

Now this can be a single line. Also some other cases may be optimized.

...

> +	void __iomem *intr = devm_platform_ioremap_resource_byname(pdev, "intr");

It's less error prone if the assignment is split from definition and moved
closer to its (first) user...

> +

...here.

> +	if (IS_ERR(intr))
> +		return PTR_ERR(intr);

...

> +	egc->eic = devm_kcalloc(dev, 1,
> +				sizeof(*egc->eic),
> +				GFP_KERNEL);

Why kcalloc(1), is this a part that will be (slightly) modified in the next
patches in the series?

> +	if (!egc->eic)
> +		return -ENOMEM;
>  
...

> +		irq = platform_get_irq(pdev, 0);

No return value check?

> +		ret = devm_request_irq(dev, irq,
> +				ep93xx_ab_irq_handler,
> +				IRQF_SHARED, gc->label, gc);
> +		if (ret) {
> +			dev_err(dev, "error requesting IRQ : %d\n", irq);
> +			return ret;

If it's soslely part of the ->probe() flow, you may use dev_err_probe().

> +		}
>  
> +		girq->parents[0] = irq;

...

>  		for (i = 0; i < girq->num_parents; i++) {
> +			irq = platform_get_irq(pdev, i);
> +			if (irq <= 0)

== 0 is never happen case. Why?

> +				continue;
> +
> +			girq->parents[i] = irq;
>  		}

> +	ret = bgpio_init(gc, &pdev->dev, 1, data, NULL, NULL, dir, NULL, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to init generic GPIO\n");
> +		return ret;

		return dev_err_probe(...);

>  	}

...

> +	if (platform_irq_count(pdev) > 0) {

Do you need this check?

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

If it's an error, why continuing?

> +	}

-- 
With Best Regards,
Andy Shevchenko