Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 10 Dec 2020 16:23:02 +0200 | Subject | Re: [PATCH v4 3/5] gpio: msc313: MStar MSC313 GPIO driver |
| |
On Sun, Nov 29, 2020 at 1:10 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 have enough register for 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.
...
> +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_irq.h> > +#include <linux/gpio/driver.h> > +#include <linux/module.h> > +#include <linux/platform_device.h>
Perhaps ordered?
...
> + /* > + * 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 > + */
You have a different comment style here (no capital letter, no period).
> + if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
Why not traditional pattern, i.e.
if (...) return -EINVAL; ...
?
> + *parent_type = child_type; > + *parent = ((offset - OFF_SPI0_CZ) >> 2) + 28; > + return 0; > + } > + > + return -EINVAL;
...
> + ret = devm_gpiochip_add_data(dev, gpiochip, gpio); > + return ret;
Purpose?
return devm_...(...);
...
> +static int msc313_gpio_remove(struct platform_device *pdev) > +{ > + return 0; > +}
Purpose?
...
> +static const struct of_device_id msc313_gpio_of_match[] = {
> +#ifdef CONFIG_MACH_INFINITY
What's the point? Are you expecting two drivers for the same IP?
> + { > + .compatible = "mstar,msc313-gpio", > + .data = &msc313_data, > + }, > +#endif > + { } > +};
...
> +static struct platform_driver msc313_gpio_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = msc313_gpio_of_match, > + .pm = &msc313_gpio_ops, > + }, > + .probe = msc313_gpio_probe, > + .remove = msc313_gpio_remove, > +};
> +
Redundant blank line.
> +builtin_platform_driver(msc313_gpio_driver);
-- With Best Regards, Andy Shevchenko
| |