On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@xxxxxxxx> 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@xxxxxxxx>
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
