Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 20 Oct 2020 15:00:17 +0300 | Subject | Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver |
| |
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
| |