Messages in this thread | | | From | Linus Walleij <> | Date | Thu, 5 Nov 2020 10:40:21 +0100 | Subject | Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver |
| |
On Mon, Oct 19, 2020 at 4: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 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. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
This looks really nice, the generic hierarchical IRQchip does its job very well here.
I saw you would send another version but this already looks like merge material.
Certainly any remaining issues can be ironed out in-tree.
> +config GPIO_MSC313 > + bool "MStar MSC313 GPIO support" > + default y if ARCH_MSTARV7 > + depends on ARCH_MSTARV7 > + select GPIOLIB_IRQCHIP
select IRQ_DOMAIN_HIERARCHY
since you are dependent on this.
(Else people will soon report problems with randconfig...)
> +/* The parent interrupt controller needs the GIC interrupt type set to GIC_SPI > + * so we need to provide the fwspec. Essentially gpiochip_populate_parent_fwspec_twocell > + * that puts GIC_SPI into the first cell. > + */ > +static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc, > + unsigned int parent_hwirq, > + unsigned int parent_type) > +{ > + struct irq_fwspec *fwspec; > + > + fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL); > + if (!fwspec) > + return NULL; > + > + fwspec->fwnode = gc->irq.parent_domain->fwnode; > + fwspec->param_count = 3; > + fwspec->param[0] = GIC_SPI; > + fwspec->param[1] = parent_hwirq; > + fwspec->param[2] = parent_type; > + > + return fwspec; > +}
Clever. Looping in Marc Z so he can say if this looks allright to him.
> +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 > + */ > + if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) { > + *parent_type = child_type; > + *parent = ((offset - OFF_SPI0_CZ) >> 2) + 28; > + ret = 0; > + } > + > + return ret; > +}
Neat!
> +static int msc313_gpio_probe(struct platform_device *pdev) > +{ > + const struct msc313_gpio_data *match_data; > + struct msc313_gpio *gpio; > + struct gpio_chip *gpiochip; > + struct gpio_irq_chip *gpioirqchip; > + struct resource *res; > + struct irq_domain *parent_domain; > + struct device_node *parent_node; > + int ret; > + > + match_data = of_device_get_match_data(&pdev->dev);
There is a lot of referencing &pdev->dev.
I would add a local variable like this:
struct device *dev = &pdev->dev
and replace all &pdev->dev with that. It will make the code more compact and easier to read.
Yours, Linus Walleij
| |