| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdYmdZ81q_tsXRQ56aFjGsvV3AwJ8_hiu31mD14DGiK84A@mail.gmail.com>
Date: Fri, 16 Oct 2020 18:56:23 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Daniel Palmer <daniel@...f.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] gpio: msc313: MStar MSC313 GPIO driver
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@...f.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@...f.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
Powered by blists - more mailing lists