| 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
| ||
|
Date: Tue, 17 Mar 2015 11:25:24 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Chao Xie <chao.xie@...vell.com>, Rob Herring <robh@...nel.org>,
Andrew Ruder <andy@...uder.net>,
Eric Miao <eric.y.miao@...il.com>
Cc: Alexandre Courbot <gnurou@...il.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Chao Xie <xiechao_mail@....com>,
Haojian Zhuang <haojian.zhuang@...il.com>
Subject: Re: [PATCH V2] gpio: mmp: add GPIO driver for Marvell MMP series
On Fri, Mar 6, 2015 at 3:04 AM, Chao Xie <chao.xie@...vell.com> wrote:
> From: Chao Xie <chao.xie@...vell.com>
>
> For some old PXA series, they used PXA GPIO driver.
> The IP of GPIO changes since PXA988 which is Marvell MMP
> series.
>
> It will use new way to control the GPIO level, direction
> and edge status.
>
> Signed-off-by: Chao Xie <chao.xie@...vell.com>
First can some of the MMP people comment on this driver please?
(Eric/Haojian)
So this driver duplicates drivers/gpio/gpio-pxa.c in some sense but
is nicer and cleaner and thus an incentive for me to merge it.
At the same time I don't want two drivers for essentially the same
hardware and in that way I would prefer to clean up the PXA driver.
But I don't know how close the PXA and MMP drivers really are.
Will it also cover MMP2 that is currently using by the PXA driver?
Is it really complicated to migrate gpio-pxa to GPIOLIB_IRQCHIP?
I guess you should also delete the compatible string and
compatibility code in drivers/gpio/gpio-pxa.c (as a separate patch
in this series) and merge this along with some defconfig
changes that activates this by default for the MMP machines?
> +config GPIO_MMP
> + bool "MMP GPIO support"
> + depends on ARCH_MMP
> + select GPIOLIB_IRQCHIP
NICE!
> +struct mmp_gpio_bank {
> + void __iomem *reg_bank;
> + u32 irq_mask;
> + u32 irq_rising_edge;
> + u32 irq_falling_edge;
I can't see why you're keeping all these settings of the edges and mask
in these variables. Why can't you just keep the state in the hardware
registers?
> +};
> +
> +struct mmp_gpio_chip {
> + struct gpio_chip chip;
> + void __iomem *reg_base;
> + int irq;
Do you need to keep irq around if you use devm_* to request the
IRQ?
> + unsigned int ngpio;
This is already stored in struct gpio_chip, why
store it a second time in this struct?
> + unsigned int nbank;
> + struct mmp_gpio_bank *banks;
To repeat an eternal story: why do you have to register several
banks under the same gpio_chip? I guess it's because they share
a single IRQ line, but wanna make sure....
> +};
> +
> +static int mmp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> + struct mmp_gpio_chip *mmp_chip =
> + container_of(chip, struct mmp_gpio_chip, chip);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
Rewrite this:
> + u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
> +
> + writel(bit, bank->reg_bank + GCDR);
Like this:
#include <linux/bitops.h>
writel(BIT(mmp_gpio_to_bank_offset(offset)), bank->reg_bank + GCDR);
> +static int mmp_gpio_direction_output(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct mmp_gpio_chip *mmp_chip =
> + container_of(chip, struct mmp_gpio_chip, chip);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
> +
> + /* Set value first. */
> + writel(bit, bank->reg_bank + (value ? GPSR : GPCR));
> +
> + writel(bit, bank->reg_bank + GSDR);
Use the same pattern with BIT() as described above.
> +static int mmp_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct mmp_gpio_chip *mmp_chip =
> + container_of(chip, struct mmp_gpio_chip, chip);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
> + u32 gplr;
> +
> + gplr = readl(bank->reg_bank + GPLR);
> +
> + return !!(gplr & bit);
Use the same pattern with BIT() as described above.
return !!(readl(bank->reg_bank + GPLR) & BIT(mmp_gpio_to_bank_offset(offset)));
> +static void mmp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct mmp_gpio_chip *mmp_chip =
> + container_of(chip, struct mmp_gpio_chip, chip);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
> + u32 gpdr;
> +
> + gpdr = readl(bank->reg_bank + GPDR);
> + /* Is it configured as output? */
> + if (gpdr & bit)
> + writel(bit, bank->reg_bank + (value ? GPSR : GPCR));
Use the same pattern with BIT() as described above.
> +static int mmp_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> + struct mmp_gpio_chip *mmp_chip = irq_data_get_irq_chip_data(d);
> + int gpio = irqd_to_hwirq(d);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(gpio)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(gpio));
> +
> + if (type & IRQ_TYPE_EDGE_RISING) {
> + bank->irq_rising_edge |= bit;
> + writel(bit, bank->reg_bank + GSRER);
> + } else {
> + bank->irq_rising_edge &= ~bit;
> + writel(bit, bank->reg_bank + GCRER);
> + }
> +
> + if (type & IRQ_TYPE_EDGE_FALLING) {
> + bank->irq_falling_edge |= bit;
> + writel(bit, bank->reg_bank + GSFER);
> + } else {
> + bank->irq_falling_edge &= ~bit;
> + writel(bit, bank->reg_bank + GCFER);
> + }
Use the same pattern with BIT() as described above.
> +static void mmp_gpio_demux_handler(unsigned int irq, struct irq_desc *desc)
Why call it a demux handler ... just call it irq_handler().
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct mmp_gpio_chip *mmp_chip = irq_get_handler_data(irq);
> + struct mmp_gpio_bank *bank;
> + int i, n;
> + u32 gedr;
> + unsigned long pending = 0;
> +
> + chained_irq_enter(chip, desc);
> +
> + for (i = 0; i < mmp_chip->nbank; i++) {
> + bank = &mmp_chip->banks[i];
> +
> + gedr = readl(bank->reg_bank + GEDR);
> + writel(gedr, bank->reg_bank + GEDR);
> + gedr = gedr & bank->irq_mask;
> +
> + if (!gedr)
> + continue;
> + pending = gedr;
> + for_each_set_bit(n, &pending, BANK_GPIO_NUMBER)
> + generic_handle_irq(irq_find_mapping(
> + mmp_chip->chip.irqdomain,
> + mmp_bank_to_gpio(i, n)));
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
This function looks really nice, given that the same IRQ line goes to
all banks... which seems to be the case :( (unfortunate HW design).
> +static void mmp_mask_muxed_gpio(struct irq_data *d)
> +{
> + struct mmp_gpio_chip *mmp_chip = irq_data_get_irq_chip_data(d);
> + int gpio = irqd_to_hwirq(d);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(gpio)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(gpio));
> +
> + bank->irq_mask &= ~bit;
> +
> + /* Clear the bit of rising and falling edge detection. */
> + writel(bit, bank->reg_bank + GCRER);
> + writel(bit, bank->reg_bank + GCFER);
> +}
Use BIT() macro.
> +static void mmp_unmask_muxed_gpio(struct irq_data *d)
> +{
> + struct mmp_gpio_chip *mmp_chip = irq_data_get_irq_chip_data(d);
> + int gpio = irqd_to_hwirq(d);
> + struct mmp_gpio_bank *bank =
> + &mmp_chip->banks[mmp_gpio_to_bank_idx(gpio)];
> + u32 bit = (1 << mmp_gpio_to_bank_offset(gpio));
> +
> + bank->irq_mask |= bit;
> +
> + /* Set the bit of rising and falling edge detection if the gpio has. */
> + writel(bit & bank->irq_rising_edge, bank->reg_bank + GSRER);
> + writel(bit & bank->irq_falling_edge, bank->reg_bank + GSFER);
> +}
Use BIT() macro.
> +
> +static struct irq_chip mmp_muxed_gpio_chip = {
> + .name = "mmp-gpio-irqchip",
> + .irq_mask = mmp_mask_muxed_gpio,
> + .irq_unmask = mmp_unmask_muxed_gpio,
> + .irq_set_type = mmp_gpio_irq_type,
> + .flags = IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static struct of_device_id mmp_gpio_dt_ids[] = {
> + { .compatible = "marvell,mmp-gpio"},
> + {}
> +};
So the same functionality in the other compatible driver should
be deleted as a separate patch.
> +static int mmp_gpio_probe_dt(struct platform_device *pdev,
> + struct mmp_gpio_chip *mmp_chip)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *child;
> + u32 offset;
> + int i, nbank, ret;
> +
> + if (!np)
> + return -EINVAL;
> +
> + nbank = of_get_child_count(np);
> + if (nbank == 0)
> + return -EINVAL;
> +
> + mmp_chip->banks = devm_kzalloc(&pdev->dev,
> + sizeof(*mmp_chip->banks) * nbank,
> + GFP_KERNEL);
> + if (!mmp_chip->banks)
> + return -ENOMEM;
> +
> + i = 0;
> + for_each_child_of_node(np, child) {
> + ret = of_property_read_u32(child, "reg-offset", &offset);
> + if (ret)
> + return ret;
> + mmp_chip->banks[i].reg_bank = mmp_chip->reg_base + offset;
> + i++;
> + }
> +
> + mmp_chip->nbank = nbank;
> + mmp_chip->ngpio = mmp_chip->nbank * BANK_GPIO_NUMBER;
> +
> + return 0;
> +}
Soon we havce so many drivers like this that we should abstract out a
routine like this into gpiolib-of.c
> +static int mmp_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mmp_gpio_platform_data *pdata;
> + struct device_node *np;
> + struct mmp_gpio_chip *mmp_chip;
> + struct mmp_gpio_bank *bank;
> + struct resource *res;
> + struct clk *clk;
> + int irq, i, ret;
> + void __iomem *base;
> +
> + pdata = dev_get_platdata(dev);
> + np = pdev->dev.of_node;
> + if (!np && !pdata)
> + return -EINVAL;
> +
> + mmp_chip = devm_kzalloc(dev, sizeof(*mmp_chip), GFP_KERNEL);
> + if (!mmp_chip)
> + return -ENOMEM;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> + base = devm_ioremap_resource(dev, res);
> + if (!base)
> + return -EINVAL;
> +
> + mmp_chip->irq = irq;
Why keep this around?
And why are you not calling devm_request_irq()???
> + mmp_chip->reg_base = base;
> +
> + ret = mmp_gpio_probe_dt(pdev, mmp_chip);
> + if (ret) {
> + dev_err(dev, "Fail to initialize gpio unit, error %d.\n", ret);
> + return ret;
> + }
> +
> + clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(dev, "Fail to get gpio clock, error %ld.\n",
> + PTR_ERR(clk));
> + return PTR_ERR(clk);
> + }
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + dev_err(dev, "Fail to enable gpio clock, error %d.\n", ret);
> + return ret;
> + }
> +
> + /* Initialize the gpio chip */
> + mmp_chip->chip.label = dev_name(dev);
> + mmp_chip->chip.ngpio = mmp_chip->ngpio;
> + mmp_chip->chip.dev = dev;
> + mmp_chip->chip.base = 0;
> +
> + mmp_chip->chip.direction_input = mmp_gpio_direction_input;
> + mmp_chip->chip.direction_output = mmp_gpio_direction_output;
> + mmp_chip->chip.get = mmp_gpio_get;
> + mmp_chip->chip.set = mmp_gpio_set;
> +
> + gpiochip_add(&mmp_chip->chip);
> +
> + /* clear all GPIO edge detects */
> + for (i = 0; i < mmp_chip->nbank; i++) {
> + bank = &mmp_chip->banks[i];
> + writel(0xffffffff, bank->reg_bank + GCFER);
> + writel(0xffffffff, bank->reg_bank + GCRER);
> + /* Unmask edge detection to AP. */
> + writel(0xffffffff, bank->reg_bank + GAPMASK);
> + }
> +
> + ret = gpiochip_irqchip_add(&mmp_chip->chip, &mmp_muxed_gpio_chip, 0,
> + handle_simple_irq, IRQ_TYPE_NONE);
> + if (ret) {
> + dev_err(dev, "failed to add irqchip\n");
> + clk_disable_unprepare(clk);
> + gpiochip_remove(&mmp_chip->chip);
> + return ret;
> + }
> +
> + gpiochip_set_chained_irqchip(&mmp_chip->chip, &mmp_muxed_gpio_chip,
> + mmp_chip->irq, mmp_gpio_demux_handler);
mmp_chip->irq has not been requested AFAICT!
> +
> + return 0;
> +}
> +
> +static struct platform_driver mmp_gpio_driver = {
> + .probe = mmp_gpio_probe,
> + .driver = {
> + .name = "mmp-gpio",
> + .of_match_table = mmp_gpio_dt_ids,
> + },
> +};
> +
> +/*
> + * gpio driver register needs to be done before
> + * machine_init functions access gpio APIs.
> + * Hence register the driver in postcore initcall.
> + */
> +static int __init mmp_gpio_init(void)
> +{
> + return platform_driver_register(&mmp_gpio_driver);
> +}
> +postcore_initcall(mmp_gpio_init);
Why do you have to have this so early?
I *strongly* prefer module_* initcalls at device_initcall()
level maybe using deferred probe if need be. It is better
if the init order is not relied upon to synchronize drivers.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists