From: Linus Walleij <linus.walleij@linaro.org>
To: Daniel Palmer <daniel@0x0f.com>, Marc Zyngier <maz@kernel.org>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
arm-kernel@lists.infradead.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver
Date: Thu, 5 Nov 2020 10:40:21 +0100 [thread overview]
Message-ID: <CACRpkdZNr6sDqJhg3KcX0bCbcd8fh2gXFYbS1r2H2Sq+vGqjUw@mail.gmail.com> (raw)
In-Reply-To: <20201019141008.871177-4-daniel@0x0f.com>
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
next prev parent reply other threads:[~2020-11-05 9:40 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-19 14:10 [PATCH v2 0/5] Add GPIO support for MStar/SigmaStar ARMv7 Daniel Palmer
2020-10-19 14:10 ` [PATCH v2 1/5] dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver Daniel Palmer
2020-10-26 13:46 ` Rob Herring
2020-10-26 15:15 ` Daniel Palmer
2020-10-19 14:10 ` [PATCH v2 2/5] dt-bindings: gpio: Binding for MStar MSC313 GPIO controller Daniel Palmer
2020-10-26 13:48 ` Rob Herring
2020-10-19 14:10 ` [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver Daniel Palmer
2020-10-20 12:00 ` Andy Shevchenko
2020-10-20 13:26 ` Daniel Palmer
2020-11-05 9:30 ` Linus Walleij
2020-11-05 9:40 ` Linus Walleij [this message]
2020-11-05 12:08 ` Marc Zyngier
2020-11-05 15:23 ` Daniel Palmer
2020-11-05 15:43 ` Marc Zyngier
2020-11-10 14:02 ` Linus Walleij
2020-11-10 14:19 ` Marc Zyngier
2020-11-11 14:09 ` Linus Walleij
2020-11-11 14:19 ` Marc Zyngier
2020-10-19 14:10 ` [PATCH v2 4/5] ARM: mstar: Add gpio controller to MStar base dtsi Daniel Palmer
2020-11-05 9:43 ` Linus Walleij
2020-10-19 14:10 ` [PATCH v2 5/5] ARM: mstar: Fill in GPIO controller properties for infinity Daniel Palmer
2020-11-05 9:44 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACRpkdZNr6sDqJhg3KcX0bCbcd8fh2gXFYbS1r2H2Sq+vGqjUw@mail.gmail.com \
--to=linus.walleij@linaro.org \
--cc=arm-kernel@lists.infradead.org \
--cc=daniel@0x0f.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).