Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 14 Jul 2022 00:14:25 +0200 | Subject | Re: [PATCH v2 1/1] gpio: mpfs: add polarfire soc gpio support |
| |
On Wed, Jul 13, 2022 at 10:44 PM <Lewis.Hanly@microchip.com> wrote: > On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote: > > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote:
...
> > > +config GPIO_POLARFIRE_SOC > > > + bool "Microchip FPGA GPIO support" > > > > Why not tristate? > OK.
(1)
...
> > > + 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.
I didn't get this. Together with (1) it makes nonsense. What did you mean by (1) _or_ by this?
...
> > Also don't forget mod_devicetable.h. > Can't see why I need this header.
Because you are using data types from it.
...
> > > + if (gpio_cfg & MPFS_GPIO_EN_IN) > > > + return 1; > > > + > > > + return 0; > > > > Don't we have specific definitions for the directions? > No defines in file.
We have. Please, check again.
...
> > > + 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.
You can multiply inside mpfs_gpio_assign_bit(), no?
...
> > > + 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.
I mean the entire function. Isn't it the default in the IRQ chip core? Or can it be made as a default with some flags set?
...
> > > + 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.
This is related to the pattern of how you are enumerating IRQs. If the pattern is changed, it would be not needed anymore.
...
> > > + 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.
Have you read what I wrote above? I wrote that you need to wrap the clk_disable_unprepare() into devm, so you may use as I pointed out, i.e. return dev_err_probe() everywhere in the ->probe().
> > > + }
...
> > 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
My question: why do you need this? What is so special about this driver?
...
> > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * > > > BYTE_BOUNDARY), > > > + MPFS_GPIO_EN_INT, 0); > > > > Too many parentheses.
As per above.
...
> > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", > > > ngpio); > > > > Noise. > Maybe, but useful information to know the ngpio.
Nope. Use sysfs / debugfs / etc. No need to noise the log.
-- With Best Regards, Andy Shevchenko
| |