Re: [PATCH v2 1/1] gpio: mpfs: add polarfire soc gpio support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Jul 13, 2022 at 10:44 PM <Lewis.Hanly@xxxxxxxxxxxxx> wrote:
> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@xxxxxxxxxxxxx> 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