Re: [PATCH 06/12] gpio: Add Aspeed driver

From: Andrew Jeffery
Date: Thu Aug 11 2016 - 20:55:10 EST

Next message: Sebastian Reichel: "Re: [PATCH] max17042_battery: fix model download bug."
Previous message: Dave Chinner: "Re: [LKP] [lkp] [xfs] 68a9f5e700: aim7.jobs-per-min -13.6% regression"
In reply to: Linus Walleij: "Re: [PATCH 06/12] gpio: Add Aspeed driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 2016-08-11 at 11:20 +0200, Linus Walleij wrote:
> On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@xxxxxxxx>
> wrote:
>
> >
> > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-
> > aspeed/Kconfig
> > index 25a0ae01429e..a52de9d3adfb 100644
> > --- a/arch/arm/mach-aspeed/Kconfig
> > +++ b/arch/arm/mach-aspeed/Kconfig
> > @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED
> > ÂÂÂÂÂÂÂÂselect ASPEED_WATCHDOG
> > ÂÂÂÂÂÂÂÂselect MOXART_TIMER
> > ÂÂÂÂÂÂÂÂselect PINCTRL
> > +ÂÂÂÂÂÂÂselect GPIOLIB
> > +ÂÂÂÂÂÂÂselect GPIO_ASPEED
> Again this needs to be a separate patch to ARM SoC.
>
> >
> > +ÂÂÂÂÂÂÂselect GPIO_SYSFS
> NAK over my dead body. I strongly discourage the use of the
> GPIO sysfs, use the new character device, see
> toos/gpio/* for examples.
>
> >
> > +config GPIO_ASPEED
> > +ÂÂÂÂÂÂÂbool "Aspeed GPIO support"
> > +ÂÂÂÂÂÂÂdepends on (ARCH_ASPEED || COMPILE_TEST) && OF
> > +ÂÂÂÂÂÂÂselect GENERIC_IRQ_CHIP
> Why are you using GENERIC_IRQ_CHIP but not
> GPIOLIB_IRQCHIP? Well I guess I may find out by
> reading the code...
>
> >
> > +ÂÂÂÂÂÂÂhelp
> > +ÂÂÂÂÂÂÂÂÂSay Y here to support Aspeed AST2400 and AST2500 GPIO
> > controllers.
> > +
> > Âconfig GPIO_BCM_KONA
> > ÂÂÂÂÂÂÂÂbool "Broadcom Kona GPIO"
> > ÂÂÂÂÂÂÂÂdepends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST)
> > @@ -1072,7 +1079,6 @@ config GPIO_SODAVILLE
> > ÂÂÂÂÂÂÂÂselect GENERIC_IRQ_CHIP
> > ÂÂÂÂÂÂÂÂhelp
> > ÂÂÂÂÂÂÂÂÂÂSay Y here to support Intel Sodaville GPIO.
> > -
> > Âendmenu
> Drop this unrelated whitespace change.
>
> >
> > +#define GPIO_BANK(x)ÂÂÂ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x)ÂÂÂÂBIT(GPIO_OFFSET(x))
> Clever, maybe needs some comments on how they work?
> Or is it obvious from context?
>
> >
> > +#define GPIO_DATAÂÂÂÂÂÂ0x00
> > +#define GPIO_DIRÂÂÂÂÂÂÂ0x04
> > +
> > +#define GPIO_IRQ_ENABLEÂÂÂÂÂÂÂÂ0x00
> > +#define GPIO_IRQ_TYPE0 0x04
> > +#define GPIO_IRQ_TYPE1 0x08
> > +#define GPIO_IRQ_TYPE2 0x0c
> > +#define GPIO_IRQ_STATUSÂÂÂÂÂÂÂÂ0x10
> >
> > +static inline struct aspeed_gpio *to_aspeed_gpio(struct gpio_chip
> > *chip)
> > +{
> > +ÂÂÂÂÂÂÂreturn container_of(chip, struct aspeed_gpio, chip);
> > +}
> NAK rewrite your code to use devm_gpiochip_add_data() and
> then use gpiochip_get_data() inline in every function that
> needs to get the state container from the gpiochip. Read
> the code in the upstream kernel for *any* driver because I
> think I changed this virtually everywhere.
>
> (...)
> >
> > +static void __aspeed_gpio_irq_set_mask(struct irq_data *d, bool
> > set)
> Why __underscoring. It is just confusing, drop the underscores.
> The function does set the mask.
>
> >
> > +{
> > +ÂÂÂÂÂÂÂconst struct aspeed_gpio_bank *bank;
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio;
> > +ÂÂÂÂÂÂÂunsigned long flags;
> > +ÂÂÂÂÂÂÂu32 reg, bit;
> > +ÂÂÂÂÂÂÂvoid *addr;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂrc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂÂÂÂaddr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE);
> > +
> > +ÂÂÂÂÂÂÂspin_lock_irqsave(&gpio->lock, flags);
> > +
> > +ÂÂÂÂÂÂÂreg = ioread32(addr);
> > +ÂÂÂÂÂÂÂif (set)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg |= bit;
> > +ÂÂÂÂÂÂÂelse
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg &= bit;
> > +ÂÂÂÂÂÂÂiowrite32(reg, addr);
> Hm, if this was done with regmap it would be regmap_update_bits()
> simply ... maybe you should just throw a 32bit MMIO regmap over
> the registers? (Not required, just an idea to simplify stuff...)
>
> >
> > +static int aspeed_gpio_set_type(struct irq_data *d, unsigned int
> > type)
> > +{
> > +ÂÂÂÂÂÂÂu32 type0, type1, type2, bit, reg;
> > +ÂÂÂÂÂÂÂconst struct aspeed_gpio_bank *bank;
> > +ÂÂÂÂÂÂÂirq_flow_handler_t handler;
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio;
> > +ÂÂÂÂÂÂÂunsigned long flags;
> > +ÂÂÂÂÂÂÂvoid *addr;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂrc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL;
> > +
> > +ÂÂÂÂÂÂÂtype0 = type1 = type2 = 0;
> Assign zero when declaring instead:
>
> u32 type0 = 0;
> u32 type1 = 0;
> ...
>
> >
> > +
> > +ÂÂÂÂÂÂÂswitch (type & IRQ_TYPE_SENSE_MASK) {
> > +ÂÂÂÂÂÂÂcase IRQ_TYPE_EDGE_BOTH:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtype2 |= bit;
> > +ÂÂÂÂÂÂÂcase IRQ_TYPE_EDGE_RISING:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtype0 |= bit;
> > +ÂÂÂÂÂÂÂcase IRQ_TYPE_EDGE_FALLING:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂhandler = handle_edge_irq;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂcase IRQ_TYPE_LEVEL_HIGH:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtype0 |= bit;
> > +ÂÂÂÂÂÂÂcase IRQ_TYPE_LEVEL_LOW:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtype1 |= bit;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂhandler = handle_level_irq;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂdefault:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL;
> > +ÂÂÂÂÂÂÂ}
> Very nice, this looks exactly as it should, handling the different
> IRQs very nicely.
>
> >
> > +ÂÂÂÂÂÂÂspin_lock_irqsave(&gpio->lock, flags);
> > +
> > +ÂÂÂÂÂÂÂaddr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0);
> > +ÂÂÂÂÂÂÂreg = ioread32(addr);
> > +ÂÂÂÂÂÂÂreg = (reg & ~bit) | type0;
> > +ÂÂÂÂÂÂÂiowrite32(reg, addr);
> > +
> > +ÂÂÂÂÂÂÂaddr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1);
> > +ÂÂÂÂÂÂÂreg = ioread32(addr);
> > +ÂÂÂÂÂÂÂreg = (reg & ~bit) | type1;
> > +ÂÂÂÂÂÂÂiowrite32(reg, addr);
> > +
> > +ÂÂÂÂÂÂÂaddr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2);
> > +ÂÂÂÂÂÂÂreg = ioread32(addr);
> > +ÂÂÂÂÂÂÂreg = (reg & ~bit) | type2;
> > +ÂÂÂÂÂÂÂiowrite32(reg, addr);
> > +
> > +ÂÂÂÂÂÂÂspin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +ÂÂÂÂÂÂÂirq_set_handler_locked(d, handler);
> > +
> > +ÂÂÂÂÂÂÂreturn 0;
> > +}
> Overall very nice .set_type().
>
> >
> > +static void aspeed_gpio_irq_handler(struct irq_desc *desc)
> > +{
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio = irq_desc_get_handler_data(desc);
> > +ÂÂÂÂÂÂÂstruct irq_chip *chip = irq_desc_get_chip(desc);
> > +ÂÂÂÂÂÂÂunsigned int i, p, girq;
> > +ÂÂÂÂÂÂÂunsigned long reg;
> > +
> > +ÂÂÂÂÂÂÂchained_irq_enter(chip, desc);
> > +
> > +ÂÂÂÂÂÂÂfor (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst struct aspeed_gpio_bank *bank =
> > &aspeed_gpio_banks[i];
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = ioread32(bank_irq_reg(gpio, bank,
> > GPIO_IRQ_STATUS));
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfor_each_set_bit(p, &reg, 32) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgirq = irq_find_mapping(gpio->irq_domain, i
> > * 32 + p);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgeneric_handle_irq(girq);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂchained_irq_exit(chip, desc);
> > +}
> This looks so generic so I think you should be using
> GPIOLIB_IRQCHIP.
>
> >
> > +
> > +static struct irq_chip aspeed_gpio_irqchip = {
> > +ÂÂÂÂÂÂÂ.nameÂÂÂÂÂÂÂÂÂÂÂ= "aspeed-gpio",
> > +ÂÂÂÂÂÂÂ.irq_ackÂÂÂÂÂÂÂÂ= aspeed_gpio_irq_ack,
> > +ÂÂÂÂÂÂÂ.irq_maskÂÂÂÂÂÂÂ= aspeed_gpio_irq_mask,
> > +ÂÂÂÂÂÂÂ.irq_unmaskÂÂÂÂÂ= aspeed_gpio_irq_unmask,
> > +ÂÂÂÂÂÂÂ.irq_set_typeÂÂÂ= aspeed_gpio_set_type,
> > +};
> There is a missing .request/.release resources marking the lines
> as irq to the GPIO core. But if you switch to using GPIOLIB_IRQCHIP
> the core will handle all that for you.
>
> >
> > +static int aspeed_gpio_to_irq(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio = to_aspeed_gpio(chip);
> > +
> > +ÂÂÂÂÂÂÂreturn irq_find_mapping(gpio->irq_domain, offset);
> > +}
> > +
> > +static void aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct platform_device *pdev)
> > +{
> > +ÂÂÂÂÂÂÂint i, irq;
> > +
> > +ÂÂÂÂÂÂÂ/* request our upstream IRQ */
> > +ÂÂÂÂÂÂÂgpio->irq = platform_get_irq(pdev, 0);
> > +ÂÂÂÂÂÂÂif (gpio->irq < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂÂÂÂ/* establish our irq domain to provide IRQs for each
> > extended bank */
> > +ÂÂÂÂÂÂÂgpio->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpio->chip.ngpio, &irq_domain_simple_ops,
> > NULL);
> > +ÂÂÂÂÂÂÂif (!gpio->irq_domain)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂÂÂÂfor (i = 0; i < gpio->chip.ngpio; i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirq = irq_create_mapping(gpio->irq_domain, i);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirq_set_chip_data(irq, gpio);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirq_set_chip_and_handler(irq, &aspeed_gpio_irqchip,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂhandle_simple_irq);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirq_set_probe(irq);
> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂirq_set_chained_handler_and_data(gpio->irq,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂaspeed_gpio_irq_handler, gpio);
> > +}
> Also all this goes away with GPILIB_IRQCHIP, so use it.
>
> See e.g. drivers/gpio/gpio-pl061.c for an example of a similar
> driver doing this.
>
> >
> > +static int __init aspeed_gpio_probe(struct platform_device *pdev)
> > +{
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio;
> > +ÂÂÂÂÂÂÂstruct resource *res;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂgpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > +ÂÂÂÂÂÂÂif (!gpio)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +ÂÂÂÂÂÂÂif (!res)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
> > +
> > +ÂÂÂÂÂÂÂgpio->base = devm_ioremap_resource(&pdev->dev, res);
> > +ÂÂÂÂÂÂÂif (!gpio->base)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂspin_lock_init(&gpio->lock);
> > +
> > +ÂÂÂÂÂÂÂgpio->chip.ngpio = ARRAY_SIZE(aspeed_gpio_banks) * 32;
> > +
> > +ÂÂÂÂÂÂÂgpio->chip.parent = &pdev->dev;
> > +ÂÂÂÂÂÂÂgpio->chip.direction_input = aspeed_gpio_dir_in;
> > +ÂÂÂÂÂÂÂgpio->chip.direction_output = aspeed_gpio_dir_out;
> Please add gpio->chip.get_direction() to complete the picture.
>
> >
> > +ÂÂÂÂÂÂÂgpio->chip.request = aspeed_gpio_request;
> > +ÂÂÂÂÂÂÂgpio->chip.free = aspeed_gpio_free;
> > +ÂÂÂÂÂÂÂgpio->chip.get = aspeed_gpio_get;
> > +ÂÂÂÂÂÂÂgpio->chip.set = aspeed_gpio_set;
> > +ÂÂÂÂÂÂÂgpio->chip.to_irq = aspeed_gpio_to_irq;
> .to_irq goes away with GPILIB_IRQCHIP.
>
> >
> > +ÂÂÂÂÂÂÂgpio->chip.label = dev_name(&pdev->dev);
> > +ÂÂÂÂÂÂÂgpio->chip.base = -1;
> > +
> > +ÂÂÂÂÂÂÂplatform_set_drvdata(pdev, gpio);
> > +
> > +ÂÂÂÂÂÂÂrc = gpiochip_add(&gpio->chip);
> > +ÂÂÂÂÂÂÂif (rc < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> Use devm_gpiochip_add_data(), then gpiochip_irqchip_add().
>
> >
> > +static int aspeed_gpio_remove(struct platform_device *pdev)
> > +{
> > +ÂÂÂÂÂÂÂstruct aspeed_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +ÂÂÂÂÂÂÂgpiochip_remove(&gpio->chip);
> > +ÂÂÂÂÂÂÂreturn 0;
> > +}
> And then this is not even needed. (devm*)
>
> Yours,
> Linus Walleij

Thanks, I will clean up the issues. I brought the patch into the series
because I figured that's what made sense. I should have looked at it a
bit closer.

Cheers,

Andrew

Attachment: signature.asc
Description: This is a digitally signed message part

Next message: Sebastian Reichel: "Re: [PATCH] max17042_battery: fix model download bug."
Previous message: Dave Chinner: "Re: [LKP] [lkp] [xfs] 68a9f5e700: aim7.jobs-per-min -13.6% regression"
In reply to: Linus Walleij: "Re: [PATCH 06/12] gpio: Add Aspeed driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]