Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Oct 19, 2020 at 5:11 PM Daniel Palmer <daniel@0x0f.com> wrote:
>
> This adds a driver that supports the GPIO block found in
> MStar/SigmaStar ARMv7 SoCs.
>
> The controller seems to support 128 lines but where they
> are wired up differs between chips and no currently known
> chip uses anywhere near 128 lines so there needs to be some
> per-chip data to collect together what lines actually have
> physical pins attached and map the right names to them.
>
> The core peripherals seem to use the same lines on the
> currently known chips but the lines used for the sensor
> interface, lcd controller etc pins seem to be totally
> different between the infinity and mercury chips
>
> The code tries to collect all of the re-usable names,
> offsets etc together so that it's easy to build the extra
> per-chip data for other chips in the future.
>
> So far this only supports the MSC313 and MSC313E chips.
>
> Support for the SSC8336N (mercury5) is trivial to add once
> all of the lines have been mapped out.

...

> +config GPIO_MSC313
> +       bool "MStar MSC313 GPIO support"

Why boolean?

> +       default y if ARCH_MSTARV7

Simply
       default ARCH_MSTARV7
should work as well.

Are you planning to extend this to other boards?

> +       depends on ARCH_MSTARV7
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say Y here to support GPIO on MStar MSC313 and later SoCs.

Please, be more specific. Also it's recommended to have a module name
to be included (but let's understand first why it's not a module)

...

> +/*
> + * Copyright (C) 2020 Daniel Palmer<daniel@thingy.jp>
> + */

One line.

...

> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/gpio/driver.h>

I believe this should be reworked.
For example, it misses mod_devicetable.h, bits.h, io.h, types.h, etc, but has

...

> +/* These bits need to be saved to correctly restore the
> + * gpio state when resuming from suspend to memory.
> + */

/*
 * For this subsystem the comment style for multi-line
 * like this.
 */

...

> +#ifdef CONFIG_MACH_INFINITY

Does it make any sense?

> +#endif

...

> +       return readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset])
> +                       & MSC313_GPIO_IN;

Usual pattern is to leave operators on the previous line.

...

> +static struct irq_chip msc313_gpio_irqchip = {
> +       .name = "GPIO",

Is this name good enough?

> +       .irq_eoi = irq_chip_eoi_parent,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = irq_chip_unmask_parent,
> +       .irq_set_type = irq_chip_set_type_parent,
> +};

...

> +static int msc313e_gpio_child_to_parent_hwirq(struct gpio_chip *chip,
> +                                            unsigned int child,
> +                                            unsigned int child_type,
> +                                            unsigned int *parent,
> +                                            unsigned int *parent_type)
> +{
> +       struct msc313_gpio *priv = gpiochip_get_data(chip);
> +       unsigned int offset = priv->gpio_data->offsets[child];
> +       int ret = -EINVAL;
> +
> +       /* only the spi0 pins have interrupts on the parent
> +        * on all of the known chips and so far they are all
> +        * mapped to the same place
> +        */

Comment style!

> +       if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
> +               *parent_type = child_type;
> +               *parent = ((offset - OFF_SPI0_CZ) >> 2) + 28;
> +               ret = 0;
> +       }
> +
> +       return ret;

Oh, can, for a starter, we use a regular (not-so-twisted) pattern

  if (...)
    return -EINVAL;
  ...
  return 0;

?

> +}

...

> +       gpio->saved = devm_kzalloc(&pdev->dev, gpio->gpio_data->num * sizeof(*gpio->saved), GFP_KERNEL);

devm_kcalloc()

> +       if (!gpio->saved)
> +               return -ENOMEM;

...

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +       if (IS_ERR(gpio->base))
> +               return PTR_ERR(gpio->base);

...

> +       gpiochip->label = DRIVER_NAME;

Not good. When you use user space how do you distinguish if more than
one chip appears in the system?

...

> +       ret = gpiochip_add_data(gpiochip, gpio);
> +       return ret;

return ...(...);

Why not devm_gpiochip_add_data() ?

...

> +static const struct of_device_id msc313_gpio_of_match[] = {

> +#ifdef CONFIG_MACH_INFINITY

To me this makes no sense.

> +       {
> +               .compatible = "mstar,msc313-gpio",
> +               .data = &msc313_data,
> +       },
> +#endif
> +       { }
> +};

...

> +/* The GPIO controller loses the state of the registers when the
> + * SoC goes into suspend to memory mode so we need to save some
> + * of the register bits before suspending and put it back when resuming
> + */

Comment style!

> +

Redundant blank line.

...

> +static int __maybe_unused msc313_gpio_resume(struct device *dev)
> +{

> +}

> +

Redundant blank line.

> +static SIMPLE_DEV_PM_OPS(msc313_gpio_ops, msc313_gpio_suspend, msc313_gpio_resume);

...

> +static struct platform_driver msc313_gpio_driver = {
> +       .driver = {
> +               .name = DRIVER_NAME,
> +               .of_match_table = msc313_gpio_of_match,
> +               .pm = &msc313_gpio_ops,

You still allow to unbind.

> +       },
> +       .probe = msc313_gpio_probe,
> +};

> +

Redundant blank line.

> +builtin_platform_driver(msc313_gpio_driver);

Why?

-- 
With Best Regards,
Andy Shevchenko