From: Linus Walleij <linus.walleij@linaro.org>
To: Daniel Palmer <daniel@0x0f.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>
Subject: Re: [PATCH 3/5] gpio: msc313: MStar MSC313 GPIO driver
Date: Fri, 16 Oct 2020 18:56:23 +0200 [thread overview]
Message-ID: <CACRpkdYmdZ81q_tsXRQ56aFjGsvV3AwJ8_hiu31mD14DGiK84A@mail.gmail.com> (raw)
In-Reply-To: <20201011024831.3868571-4-daniel@0x0f.com>
Hi Daniel,
thanks for your patch!
Some comments below, we need some work but keep at it.
On Sun, Oct 11, 2020 at 4:48 AM 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>
(...)
> +config GPIO_MSC313
> + bool "MStar MSC313 GPIO support"
> + default y if ARCH_MSTARV7
> + depends on ARCH_MSTARV7
> + select GPIO_GENERIC
Selecting GPIO_GENERIC, that is good.
But you're not using it, because you can't.
This chip does not have the bits lined up nicely
in one register, instead there seems to be something
like one register per line, right?
So skip GPIO_GENERIC.
> +#define MSC313_GPIO_IN BIT(0)
> +#define MSC313_GPIO_OUT BIT(4)
> +#define MSC313_GPIO_OEN BIT(5)
> +
> +#define MSC313_GPIO_BITSTOSAVE (MSC313_GPIO_OUT | MSC313_GPIO_OEN)
Some comment here telling us why these need saving and
not others.
> +#define FUART_NAMES \
> + MSC313_PINNAME_FUART_RX, \
> + MSC313_PINNAME_FUART_TX, \
> + MSC313_PINNAME_FUART_CTS, \
> + MSC313_PINNAME_FUART_RTS
> +
> +#define OFF_FUART_RX 0x50
> +#define OFF_FUART_TX 0x54
> +#define OFF_FUART_CTS 0x58
> +#define OFF_FUART_RTS 0x5c
> +
> +#define FUART_OFFSETS \
> + OFF_FUART_RX, \
> + OFF_FUART_TX, \
> + OFF_FUART_CTS, \
> + OFF_FUART_RTS
This looks a bit strange. The GPIO driver should not really
have to know about any other use cases for pins than
GPIO. But I guess it is intuitive for the driver.
> +#define SD_NAMES \
> + MSC313_PINNAME_SD_CLK, \
> + MSC313_PINNAME_SD_CMD, \
> + MSC313_PINNAME_SD_D0, \
> + MSC313_PINNAME_SD_D1, \
> + MSC313_PINNAME_SD_D2, \
> + MSC313_PINNAME_SD_D3
> +
> +#define OFF_SD_CLK 0x140
> +#define OFF_SD_CMD 0x144
> +#define OFF_SD_D0 0x148
> +#define OFF_SD_D1 0x14cchild_to_parent_hwirq
> +#define OFF_SD_D2 0x150
> +#define OFF_SD_D3 0x154
> +
> +#define SD_OFFSETS \
> + OFF_SD_CLK, \
> + OFF_SD_CMD, \
> + OFF_SD_D0, \
> + OFF_SD_D1, \
> + OFF_SD_D2, \
> + OFF_SD_D3
> +
> +#define I2C1_NAMES \
> + MSC313_PINNAME_I2C1_SCL, \
> + MSC313_PINNAME_I2C1_SCA
> +
> +#define OFF_I2C1_SCL 0x188
> +#define OFF_I2C1_SCA 0x18c
> +
> +#define I2C1_OFFSETS \
> + OFF_I2C1_SCL, \
> + OFF_I2C1_SCA
> +
> +#define SPI0_NAMES \
> + MSC313_PINNAME_SPI0_CZ, \
> + MSC313_PINNAME_SPI0_CK, \
> + MSC313_PINNAME_SPI0_DI, \
> + MSC313_PINNAME_SPI0_DO
> +
> +#define OFF_SPI0_CZ 0x1c0
> +#define OFF_SPI0_CK 0x1c4
> +#define OFF_SPI0_DI 0x1c8
> +#define OFF_SPI0_DO 0x1cc
> +
> +#define SPI0_OFFSETS \
> + OFF_SPI0_CZ, \
> + OFF_SPI0_CK, \
> + OFF_SPI0_DI, \
> + OFF_SPI0_DO
Same with all these. I suppose it is the offsets of stuff
that would be there unless we were using it for GPIO.
> +static int msc313_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct msc313_gpio *gpio = gpiochip_get_data(chip);
> +> +
> + return gpio->irqs[offset];
> +}
Please do not use custom IRQ handling like this.
As there seems to be one IRQ per line, look into using
select GPIOLIB_IRQCHIP
select IRQ_DOMAIN_HIERARCHY
See for example in gpio-ixp4xx.c how we deal with
hiearchical GPIO IRQs.
> + gpiochip->to_irq = msc313_gpio_to_irq;
> + gpiochip->base = -1;
> + gpiochip->ngpio = gpio->gpio_data->num;
> + gpiochip->names = gpio->gpio_data->names;
> +
> + for (i = 0; i < gpiochip->ngpio; i++)
> + gpio->irqs[i] = of_irq_get_byname(pdev->dev.of_node, gpio->gpio_data->names[i]);
Use hierarchical generic GPIO IRQs for these.
Assign ->fwnode, ->parent_domain, ->child_to_parent_hwirq,
and probably also ->handler on the struct gpio_irq_chip *.
Skip assigning gpiochip->to_irq, the generic code will
handle that.
Again see gpio-ixp4xx.c for an example.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-10-16 16:57 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-11 2:48 [PATCH 0/5] Add GPIO support for MStar/SigmaStar ARMv7 Daniel Palmer
2020-10-11 2:48 ` [PATCH 1/5] dt-bindings: gpio: Binding for MStar MSC313 GPIO controller Daniel Palmer
2020-10-12 16:10 ` Rob Herring
2020-10-16 16:36 ` Linus Walleij
2020-10-19 16:13 ` Krzysztof Kozlowski
2020-11-05 9:13 ` Linus Walleij
2020-10-11 2:48 ` [PATCH 2/5] dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver Daniel Palmer
2020-10-12 16:11 ` Rob Herring
2020-10-14 9:45 ` Daniel Palmer
2020-10-14 12:17 ` Rob Herring
2020-10-11 2:48 ` [PATCH 3/5] gpio: msc313: MStar " Daniel Palmer
2020-10-16 16:56 ` Linus Walleij [this message]
2020-10-17 1:57 ` Daniel Palmer
2020-10-21 11:07 ` Daniel Palmer
2020-11-05 9:21 ` Linus Walleij
2020-11-05 9:31 ` Willy Tarreau
2020-11-05 9:42 ` Linus Walleij
2020-11-05 15:39 ` Daniel Palmer
2020-10-11 2:48 ` [PATCH 4/5] ARM: mstar: Add gpio controller to MStar base dtsi Daniel Palmer
2020-10-11 2:48 ` [PATCH 5/5] ARM: mstar: Fill in GPIO controller properties for infinity Daniel Palmer
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=CACRpkdYmdZ81q_tsXRQ56aFjGsvV3AwJ8_hiu31mD14DGiK84A@mail.gmail.com \
--to=linus.walleij@linaro.org \
--cc=daniel@0x0f.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.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).