| 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
| ||
|
Message-ID: <abdf389cfd049f60b951447c047bbd186fa19469.camel@microchip.com>
Date: Wed, 13 Jul 2022 20:44:03 +0000
From: <Lewis.Hanly@...rochip.com>
To: <andy.shevchenko@...il.com>
CC: <linux-riscv@...ts.infradead.org>, <Conor.Dooley@...rochip.com>,
<brgl@...ev.pl>, <linux-gpio@...r.kernel.org>,
<linus.walleij@...aro.org>, <palmer@...belt.com>, <maz@...nel.org>,
<linux-kernel@...r.kernel.org>, <Daire.McNamara@...rochip.com>
Subject: Re: [PATCH v2 1/1] gpio: mpfs: add polarfire soc gpio support
Thanks Andy for the feedback.
Points noted and updates will be in next revision.
On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@...rochip.com> wrote:
> > From: Lewis Hanly <lewis.hanly@...rochip.com>
> >
> > Add a driver to support the Polarfire SoC gpio controller.
>
> GPIO
>
> ...
>
> Below my comments, I have tried hard not to duplicate what Conor
> already mentioned. So consider this as additional part.
>
> ...
>
> > +config GPIO_POLARFIRE_SOC
> > + bool "Microchip FPGA GPIO support"
>
> Why not tristate?
OK.
>
> > + depends on OF_GPIO
>
> Why?
>
> > + select GPIOLIB_IRQCHIP
> > + select IRQ_DOMAIN_HIERARCHY
>
> More naturally this line looks if put before GPIOLB_IRQCHIP one.
Yes
>
> > + select GPIO_GENERIC
> > + help
> > + Say yes here to support the GPIO device on Microchip
> > FPGAs.
>
> When allowing it to be a module, add a text that tells how the driver
> will be called.
Not loading as a module.
>
> ...
>
> > +/*
> > + * Microchip PolarFire SoC (MPFS) GPIO controller driver
> > + *
> > + * Copyright (c) 2018-2022 Microchip Technology Inc. and its
> > subsidiaries
> > + *
> > + * Author: Lewis Hanly <lewis.hanly@...rochip.com>
> > + *
>
> This line is not needed.
OK
>
> > + */
>
> ...
>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
>
> Why?
Not sure, will check again.
>
> Also don't forget mod_devicetable.h.
Can't see why I need this header.
>
> ...
>
> > +#define NUM_GPIO 32
> > +#define BYTE_BOUNDARY 0x04
>
> Without namespace?
>
> ...
>
> > + gpio_cfg = readl(mpfs_gpio->base + (gpio_index *
> > BYTE_BOUNDARY));
> > +
>
> Unneeded line.
>
> > + if (gpio_cfg & MPFS_GPIO_EN_IN)
> > + return 1;
> > +
> > + return 0;
>
> Don't we have specific definitions for the directions?
No defines in file.
>
> ...
>
> > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> > + int gpio_index = irqd_to_hwirq(data);
> > + u32 interrupt_type;
> > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
>
> This line naturally looks better before interrupt_type definition.
> Try to keep the "longest line first" everywhere in the driver.
OK
>
> > + u32 gpio_cfg;
> > + unsigned long flags;
>
> ...
>
> > + switch (type) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH;
> > + break;
> > +
>
> Unneeded line here and everywhere in the similar cases in the entire
> code.
OK
>
> > + case IRQ_TYPE_EDGE_FALLING:
> > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG;
> > + break;
> > +
> > + case IRQ_TYPE_EDGE_RISING:
> > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS;
> > + break;
> > +
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH;
> > + break;
> > +
> > + case IRQ_TYPE_LEVEL_LOW:
> > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> > + break;
> > + }
>
> ...
>
> > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index *
> > BYTE_BOUNDARY),
> > + MPFS_GPIO_EN_INT, 1);
>
> Too many parentheses.
I do think it makes reading easier.
>
> ...
>
> > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index *
> > BYTE_BOUNDARY),
> > + MPFS_GPIO_EN_INT, 0);
>
> Ditto.
>
> ...
>
> > +static int mpfs_gpio_irq_set_affinity(struct irq_data *data,
> > + const struct cpumask *dest,
> > + bool force)
> > +{
> > + if (data->parent_data)
> > + return irq_chip_set_affinity_parent(data, dest,
> > force);
> > +
> > + return -EINVAL;
> > +}
>
> Why do you need this? Isn't it taken care of by the IRQ chip core?
Yes I believe we do/should, data->parent_data is used in
irq_chip_set_affinity_parent(..) without checking so hence checked
before calling.
>
> ...
>
> > + struct clk *clk;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct device_node *irq_parent;
>
> Why do you need these?
Yes they are needed. Both of the same type but used in different
places. I don't think I can reuse.
>
> > + struct gpio_irq_chip *girq;
> > + struct irq_domain *parent;
> > + struct mpfs_gpio_chip *mpfs_gpio;
> > + int i, ret, ngpio;
>
> ...
>
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "devm_clk_get failed\n");
> > + return PTR_ERR(clk);
> > + }
>
> return dev_err_probe(...);
>
> It's not only convenient, but here it fixes a bug.
Will use return dev_err_probe.
>
> > + ret = clk_prepare_enable(clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to enable clock\n");
> > + return ret;
>
> return dev_err_probe(...);
Yes
>
> > + }
>
> Make it managed as well in addition to gpiochip_add_data(), otherwise
> you will have wrong ordering.
>
> ...
>
> > + ngpio = of_irq_count(node);
> > + if (ngpio > NUM_GPIO) {
> > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> > + NUM_GPIO);
> > + ret = -ENXIO;
> > + goto cleanup_clock;
>
> return dev_err_probe(...);
I need to cleanup clock before returning, will use dev_err_probe.
>
> > + }
> > +
> > + irq_parent = of_irq_find_parent(node);
> > + if (!irq_parent) {
> > + dev_err(dev, "no IRQ parent node\n");
> > + ret = -ENODEV;
> > + goto cleanup_clock;
>
> Ditto.
>
> > + }
> > + parent = irq_find_host(irq_parent);
> > + if (!parent) {
> > + dev_err(dev, "no IRQ parent domain\n");
> > + ret = -ENODEV;
> > + goto cleanup_clock;
>
> Ditto.
>
> > + }
>
> Why do you need all these? Seems a copy'n'paste from gpio-sifive,
> which is the only GPIO driver using this pattern.
Yes I believe we do need all this information. Yes it is similiar
implementation to sifive. Not the only driver using this pattern, check
gpio-ixp4xxx.c
>
> ...
>
> > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i *
> > BYTE_BOUNDARY),
> > + MPFS_GPIO_EN_INT, 0);
>
> Too many parentheses.
>
>
> > + girq->fwnode = of_node_to_fwnode(node);
>
> This is an interesting way of
>
> ...->fwnode = dev_fwnode(dev);
>
>
> ...
>
> > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n",
> > ngpio);
>
> Noise.
Maybe, but useful information to know the ngpio.
>
> ...
>
> > + .of_match_table = of_match_ptr(mpfs_gpio_match),
>
> Please, avoid using of_match_ptr(). It brings more harm than
> usefulness.
OK
>
> --
> With Best Regards,
> Andy Shevchenko
Powered by blists - more mailing lists