| Message ID | 20220713105910.931983-2-lewis.hanly@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Polarfire SoC GPIO support | expand |
Hey Lewis, Couple comments. On 13/07/2022 11:59, lewis.hanly@microchip.com wrote: > From: Lewis Hanly <lewis.hanly@microchip.com> > > Add a driver to support the Polarfire SoC gpio controller. > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > --- > drivers/gpio/Kconfig | 9 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-mpfs.c | 379 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 389 insertions(+) > create mode 100644 drivers/gpio/gpio-mpfs.c > > + > +static int mpfs_gpio_probe(struct platform_device *pdev) > +{ > + struct clk *clk; > + struct device *dev = &pdev->dev; > + struct device_node *node = pdev->dev.of_node; > + struct device_node *irq_parent; > + struct gpio_irq_chip *girq; > + struct irq_domain *parent; > + struct mpfs_gpio_chip *mpfs_gpio; > + int i, ret, ngpio; > + > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL); > + if (!mpfs_gpio) > + return -ENOMEM; > + > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(mpfs_gpio->base)) > + return PTR_ERR(mpfs_gpio->base); > + > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable clock\n"); > + return ret; > + } > + > + mpfs_gpio->clk = clk; > + > + ngpio = of_irq_count(node); > + if (ngpio > NUM_GPIO) { > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > + NUM_GPIO); This should fit on one line > + ret = -ENXIO; > + goto cleanup_clock; > + } > + > + irq_parent = of_irq_find_parent(node); > + if (!irq_parent) { > + dev_err(dev, "no IRQ parent node\n"); > + ret = -ENODEV; > + goto cleanup_clock; > + } > + parent = irq_find_host(irq_parent); > + if (!parent) { > + dev_err(dev, "no IRQ parent domain\n"); > + ret = -ENODEV; > + goto cleanup_clock; > + } > + > + /* Get the interrupt numbers. netdev style comment > + * Clear/Disable All interrupts before enabling parent interrupts. > + */ > + for (i = 0; i < ngpio; i++) { > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); You need to handle the case where platform_get_irq returns an error right? > + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, 1); > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY), > + MPFS_GPIO_EN_INT, 0); > + } > + > + raw_spin_lock_init(&mpfs_gpio->lock); > + > + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input; > + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output; > + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction; > + mpfs_gpio->gc.get = mpfs_gpio_get; > + mpfs_gpio->gc.set = mpfs_gpio_set; > + mpfs_gpio->gc.base = -1; > + mpfs_gpio->gc.ngpio = ngpio; > + mpfs_gpio->gc.label = dev_name(dev); > + mpfs_gpio->gc.parent = dev; > + mpfs_gpio->gc.owner = THIS_MODULE; > + > + /* Get a pointer to the gpio_irq_chip */ I doubt this comment is needed > + girq = &mpfs_gpio->gc.irq; > + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip); > + girq->fwnode = of_node_to_fwnode(node); > + girq->parent_domain = parent; > + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq; > + girq->handler = handle_bad_irq; > + girq->default_type = IRQ_TYPE_NONE; > + > + ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio); Can we use a devm_ variant instead here... > + if (ret) > + goto cleanup_clock; > + > + platform_set_drvdata(pdev, mpfs_gpio); > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio); > + > + return 0; > + > +cleanup_clock: > + clk_disable_unprepare(mpfs_gpio->clk); > + return ret; > +} > + > +static int mpfs_gpio_remove(struct platform_device *pdev) > +{ > + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev); > + > + gpiochip_remove(&mpfs_gpio->gc); ... and drop this? > + clk_disable_unprepare(mpfs_gpio->clk); > + > + return 0; > +} > + > +static const struct of_device_id mpfs_gpio_match[] = { > + { .compatible = "microchip,mpfs-gpio", }, > + { /* end of list */ }, Don't need a comma after the sentinel > +}; > + > +static struct platform_driver mpfs_gpio_driver = { > + .probe = mpfs_gpio_probe, > + .driver = { > + .name = "microchip,mpfs-gpio", > + .of_match_table = of_match_ptr(mpfs_gpio_match), > + }, > + .remove = mpfs_gpio_remove, > +}; > +builtin_platform_driver(mpfs_gpio_driver) Mising semicolon? Thanks, Conor.
On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > Add a driver to support the Polarfire SoC gpio controller. GPIO ... Below my comments, I have tried hard not to duplicate what Conor already mentioned. So consider this as additional part. ... > +config GPIO_POLARFIRE_SOC > + bool "Microchip FPGA GPIO support" Why not tristate? > + depends on OF_GPIO Why? > + select GPIOLIB_IRQCHIP > + select IRQ_DOMAIN_HIERARCHY More naturally this line looks if put before GPIOLB_IRQCHIP one. > + select GPIO_GENERIC > + help > + Say yes here to support the GPIO device on Microchip FPGAs. When allowing it to be a module, add a text that tells how the driver will be called. ... > +/* > + * Microchip PolarFire SoC (MPFS) GPIO controller driver > + * > + * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Lewis Hanly <lewis.hanly@microchip.com> > + * This line is not needed. > + */ ... > +#include <linux/of.h> > +#include <linux/of_irq.h> Why? Also don't forget mod_devicetable.h. ... > +#define NUM_GPIO 32 > +#define BYTE_BOUNDARY 0x04 Without namespace? ... > + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); > + Unneeded line. > + if (gpio_cfg & MPFS_GPIO_EN_IN) > + return 1; > + > + return 0; Don't we have specific definitions for the directions? ... > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > + int gpio_index = irqd_to_hwirq(data); > + u32 interrupt_type; > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); This line naturally looks better before interrupt_type definition. Try to keep the "longest line first" everywhere in the driver. > + u32 gpio_cfg; > + unsigned long flags; ... > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH; > + break; > + Unneeded line here and everywhere in the similar cases in the entire code. > + case IRQ_TYPE_EDGE_FALLING: > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG; > + break; > + > + case IRQ_TYPE_EDGE_RISING: > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS; > + break; > + > + case IRQ_TYPE_LEVEL_HIGH: > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH; > + break; > + > + case IRQ_TYPE_LEVEL_LOW: > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW; > + break; > + } ... > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY), > + MPFS_GPIO_EN_INT, 1); Too many parentheses. ... > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY), > + MPFS_GPIO_EN_INT, 0); Ditto. ... > +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, > + const struct cpumask *dest, > + bool force) > +{ > + if (data->parent_data) > + return irq_chip_set_affinity_parent(data, dest, force); > + > + return -EINVAL; > +} Why do you need this? Isn't it taken care of by the IRQ chip core? ... > + struct clk *clk; > + struct device *dev = &pdev->dev; > + struct device_node *node = pdev->dev.of_node; > + struct device_node *irq_parent; Why do you need these? > + struct gpio_irq_chip *girq; > + struct irq_domain *parent; > + struct mpfs_gpio_chip *mpfs_gpio; > + int i, ret, ngpio; ... > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > + return PTR_ERR(clk); > + } return dev_err_probe(...); It's not only convenient, but here it fixes a bug. > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable clock\n"); > + return ret; return dev_err_probe(...); > + } Make it managed as well in addition to gpiochip_add_data(), otherwise you will have wrong ordering. ... > + ngpio = of_irq_count(node); > + if (ngpio > NUM_GPIO) { > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > + NUM_GPIO); > + ret = -ENXIO; > + goto cleanup_clock; return dev_err_probe(...); > + } > + > + irq_parent = of_irq_find_parent(node); > + if (!irq_parent) { > + dev_err(dev, "no IRQ parent node\n"); > + ret = -ENODEV; > + goto cleanup_clock; Ditto. > + } > + parent = irq_find_host(irq_parent); > + if (!parent) { > + dev_err(dev, "no IRQ parent domain\n"); > + ret = -ENODEV; > + goto cleanup_clock; Ditto. > + } Why do you need all these? Seems a copy'n'paste from gpio-sifive, which is the only GPIO driver using this pattern. ... > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY), > + MPFS_GPIO_EN_INT, 0); Too many parentheses. > + girq->fwnode = of_node_to_fwnode(node); This is an interesting way of ...->fwnode = dev_fwnode(dev); ... > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio); Noise. ... > + .of_match_table = of_match_ptr(mpfs_gpio_match), Please, avoid using of_match_ptr(). It brings more harm than usefulness.
On 13/07/2022 12:59, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > Below my comments, I have tried hard not to duplicate what Conor > already mentioned. So consider this as additional part. > >> +#define NUM_GPIO 32 >> +#define BYTE_BOUNDARY 0x04 > > Without namespace? Does byte_boundary even need to be defined? is incrementing an address by 0x4 not kinda obvious on its own as to what it is doing? >> + if (gpio_cfg & MPFS_GPIO_EN_IN) >> + return 1; >> + >> + return 0; > > Don't we have specific definitions for the directions? FWIW Lewis, they're GPIO_LINE_DIRECTION_IN & GPIO_LINE_DIRECTION_OUT I thought something like this would surely exist but wasn't sure. Thanks for pointing it out Andy. Conor.
On Wed, Jul 13, 2022 at 7:44 PM <Conor.Dooley@microchip.com> wrote: > On 13/07/2022 12:59, Andy Shevchenko wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe ... > >> +#define BYTE_BOUNDARY 0x04 > > > > Without namespace? > > Does byte_boundary even need to be defined? > is incrementing an address by 0x4 not kinda obvious on its own > as to what it is doing? The less magic is the better. Btw, have you considered gpio-regmap? Can it be utilized here?
Thanks Andy for the feedback. Points noted and updates will be in next revision. On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > Add a driver to support the Polarfire SoC gpio controller. > > GPIO > > ... > > Below my comments, I have tried hard not to duplicate what Conor > already mentioned. So consider this as additional part. > > ... > > > +config GPIO_POLARFIRE_SOC > > + bool "Microchip FPGA GPIO support" > > Why not tristate? OK. > > > + depends on OF_GPIO > > Why? > > > + select GPIOLIB_IRQCHIP > > + select IRQ_DOMAIN_HIERARCHY > > More naturally this line looks if put before GPIOLB_IRQCHIP one. Yes > > > + select GPIO_GENERIC > > + help > > + Say yes here to support the GPIO device on Microchip > > FPGAs. > > When allowing it to be a module, add a text that tells how the driver > will be called. Not loading as a module. > > ... > > > +/* > > + * Microchip PolarFire SoC (MPFS) GPIO controller driver > > + * > > + * Copyright (c) 2018-2022 Microchip Technology Inc. and its > > subsidiaries > > + * > > + * Author: Lewis Hanly <lewis.hanly@microchip.com> > > + * > > This line is not needed. OK > > > + */ > > ... > > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > Why? Not sure, will check again. > > Also don't forget mod_devicetable.h. Can't see why I need this header. > > ... > > > +#define NUM_GPIO 32 > > +#define BYTE_BOUNDARY 0x04 > > Without namespace? > > ... > > > + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY)); > > + > > Unneeded line. > > > + if (gpio_cfg & MPFS_GPIO_EN_IN) > > + return 1; > > + > > + return 0; > > Don't we have specific definitions for the directions? No defines in file. > > ... > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > > + int gpio_index = irqd_to_hwirq(data); > > + u32 interrupt_type; > > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > > This line naturally looks better before interrupt_type definition. > Try to keep the "longest line first" everywhere in the driver. OK > > > + u32 gpio_cfg; > > + unsigned long flags; > > ... > > > + switch (type) { > > + case IRQ_TYPE_EDGE_BOTH: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH; > > + break; > > + > > Unneeded line here and everywhere in the similar cases in the entire > code. OK > > > + case IRQ_TYPE_EDGE_FALLING: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG; > > + break; > > + > > + case IRQ_TYPE_EDGE_RISING: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS; > > + break; > > + > > + case IRQ_TYPE_LEVEL_HIGH: > > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH; > > + break; > > + > > + case IRQ_TYPE_LEVEL_LOW: > > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW; > > + break; > > + } > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 1); > > Too many parentheses. I do think it makes reading easier. > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 0); > > Ditto. > > ... > > > +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, > > + const struct cpumask *dest, > > + bool force) > > +{ > > + if (data->parent_data) > > + return irq_chip_set_affinity_parent(data, dest, > > force); > > + > > + return -EINVAL; > > +} > > Why do you need this? Isn't it taken care of by the IRQ chip core? Yes I believe we do/should, data->parent_data is used in irq_chip_set_affinity_parent(..) without checking so hence checked before calling. > > ... > > > + struct clk *clk; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct device_node *irq_parent; > > Why do you need these? Yes they are needed. Both of the same type but used in different places. I don't think I can reuse. > > > + struct gpio_irq_chip *girq; > > + struct irq_domain *parent; > > + struct mpfs_gpio_chip *mpfs_gpio; > > + int i, ret, ngpio; > > ... > > > + clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > > + return PTR_ERR(clk); > > + } > > return dev_err_probe(...); > > It's not only convenient, but here it fixes a bug. Will use return dev_err_probe. > > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + return ret; > > return dev_err_probe(...); Yes > > > + } > > Make it managed as well in addition to gpiochip_add_data(), otherwise > you will have wrong ordering. > > ... > > > + ngpio = of_irq_count(node); > > + if (ngpio > NUM_GPIO) { > > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > > + NUM_GPIO); > > + ret = -ENXIO; > > + goto cleanup_clock; > > return dev_err_probe(...); I need to cleanup clock before returning, will use dev_err_probe. > > > + } > > + > > + irq_parent = of_irq_find_parent(node); > > + if (!irq_parent) { > > + dev_err(dev, "no IRQ parent node\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > Ditto. > > > + } > > + parent = irq_find_host(irq_parent); > > + if (!parent) { > > + dev_err(dev, "no IRQ parent domain\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > Ditto. > > > + } > > Why do you need all these? Seems a copy'n'paste from gpio-sifive, > which is the only GPIO driver using this pattern. Yes I believe we do need all this information. Yes it is similiar implementation to sifive. Not the only driver using this pattern, check gpio-ixp4xxx.c > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 0); > > Too many parentheses. > > > > + girq->fwnode = of_node_to_fwnode(node); > > This is an interesting way of > > ...->fwnode = dev_fwnode(dev); > > > ... > > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", > > ngpio); > > Noise. Maybe, but useful information to know the ngpio. > > ... > > > + .of_match_table = of_match_ptr(mpfs_gpio_match), > > Please, avoid using of_match_ptr(). It brings more harm than > usefulness. OK > > -- > With Best Regards, > Andy Shevchenko
Thanks Conor. On Wed, 2022-07-13 at 11:26 +0000, Conor Dooley - M52691 wrote: > Hey Lewis, > Couple comments. > > On 13/07/2022 11:59, lewis.hanly@microchip.com wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > Add a driver to support the Polarfire SoC gpio controller. > > > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > > --- > > drivers/gpio/Kconfig | 9 + > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-mpfs.c | 379 > > +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 389 insertions(+) > > create mode 100644 drivers/gpio/gpio-mpfs.c > > > > + > > +static int mpfs_gpio_probe(struct platform_device *pdev) > > +{ > > + struct clk *clk; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct device_node *irq_parent; > > + struct gpio_irq_chip *girq; > > + struct irq_domain *parent; > > + struct mpfs_gpio_chip *mpfs_gpio; > > + int i, ret, ngpio; > > + > > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL); > > + if (!mpfs_gpio) > > + return -ENOMEM; > > + > > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(mpfs_gpio->base)) > > + return PTR_ERR(mpfs_gpio->base); > > + > > + clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > > + return PTR_ERR(clk); > > + } > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + return ret; > > + } > > + > > + mpfs_gpio->clk = clk; > > + > > + ngpio = of_irq_count(node); > > + if (ngpio > NUM_GPIO) { > > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > > + NUM_GPIO); > > This should fit on one line Can do. > > > + ret = -ENXIO; > > + goto cleanup_clock; > > + } > > + > > + irq_parent = of_irq_find_parent(node); > > + if (!irq_parent) { > > + dev_err(dev, "no IRQ parent node\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + parent = irq_find_host(irq_parent); > > + if (!parent) { > > + dev_err(dev, "no IRQ parent domain\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + > > + /* Get the interrupt numbers. > > netdev style comment > > > + * Clear/Disable All interrupts before enabling parent > > interrupts. > > + */ > > + for (i = 0; i < ngpio; i++) { > > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); > > You need to handle the case where platform_get_irq returns an error > right? Was not going to as I don't think it breaks if error. I will do a little more checking if required. > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, > > 1); > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 0); > > + } > > + > > + raw_spin_lock_init(&mpfs_gpio->lock); > > + > > + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input; > > + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output; > > + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction; > > + mpfs_gpio->gc.get = mpfs_gpio_get; > > + mpfs_gpio->gc.set = mpfs_gpio_set; > > + mpfs_gpio->gc.base = -1; > > + mpfs_gpio->gc.ngpio = ngpio; > > + mpfs_gpio->gc.label = dev_name(dev); > > + mpfs_gpio->gc.parent = dev; > > + mpfs_gpio->gc.owner = THIS_MODULE; > > + > > + /* Get a pointer to the gpio_irq_chip */ > > I doubt this comment is needed OK. > > > + girq = &mpfs_gpio->gc.irq; > > + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip); > > + girq->fwnode = of_node_to_fwnode(node); > > + girq->parent_domain = parent; > > + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq; > > + girq->handler = handle_bad_irq; > > + girq->default_type = IRQ_TYPE_NONE; > > + > > + ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio); > > Can we use a devm_ variant instead here... Maybe, will check. > > > + if (ret) > > + goto cleanup_clock; > > + > > + platform_set_drvdata(pdev, mpfs_gpio); > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", > > ngpio); > > + > > + return 0; > > + > > +cleanup_clock: > > + clk_disable_unprepare(mpfs_gpio->clk); > > + return ret; > > +} > > + > > +static int mpfs_gpio_remove(struct platform_device *pdev) > > +{ > > + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev); > > + > > + gpiochip_remove(&mpfs_gpio->gc); > > ... and drop this? eh, OK. > > > + clk_disable_unprepare(mpfs_gpio->clk); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id mpfs_gpio_match[] = { > > + { .compatible = "microchip,mpfs-gpio", }, > > + { /* end of list */ }, > > Don't need a comma after the sentinel OK. > > > +}; > > + > > +static struct platform_driver mpfs_gpio_driver = { > > + .probe = mpfs_gpio_probe, > > + .driver = { > > + .name = "microchip,mpfs-gpio", > > + .of_match_table = of_match_ptr(mpfs_gpio_match), > > + }, > > + .remove = mpfs_gpio_remove, > > +}; > > +builtin_platform_driver(mpfs_gpio_driver) > > Mising semicolon? Can add.. > > Thanks, > Conor.
On Wed, 2022-07-13 at 19:50 +0200, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Jul 13, 2022 at 7:44 PM <Conor.Dooley@microchip.com> wrote: > > On 13/07/2022 12:59, Andy Shevchenko wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > ... > > > > > +#define BYTE_BOUNDARY 0x04 > > > > > > Without namespace? > > > > Does byte_boundary even need to be defined? > > is incrementing an address by 0x4 not kinda obvious on its own > > as to what it is doing? > > The less magic is the better. > > Btw, have you considered gpio-regmap? Can it be utilized here? Yes I have considered regmap, our register map is not mapped out to fully utilize regmap. We could use for one/two registers but not fully. > > -- > With Best Regards, > Andy Shevchenko
On Wed, Jul 13, 2022 at 10:44 PM <Lewis.Hanly@microchip.com> wrote: > On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote: > > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: ... > > > +config GPIO_POLARFIRE_SOC > > > + bool "Microchip FPGA GPIO support" > > > > Why not tristate? > OK. (1) ... > > > + help > > > + Say yes here to support the GPIO device on Microchip > > > FPGAs. > > > > When allowing it to be a module, add a text that tells how the driver > > will be called. > Not loading as a module. I didn't get this. Together with (1) it makes nonsense. What did you mean by (1) _or_ by this? ... > > Also don't forget mod_devicetable.h. > Can't see why I need this header. Because you are using data types from it. ... > > > + if (gpio_cfg & MPFS_GPIO_EN_IN) > > > + return 1; > > > + > > > + return 0; > > > > Don't we have specific definitions for the directions? > No defines in file. We have. Please, check again. ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > > BYTE_BOUNDARY), > > > + MPFS_GPIO_EN_INT, 1); > > > > Too many parentheses. > I do think it makes reading easier. You can multiply inside mpfs_gpio_assign_bit(), no? ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > > BYTE_BOUNDARY), > > > + MPFS_GPIO_EN_INT, 0); Ditto. ... > > > +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, > > > + const struct cpumask *dest, > > > + bool force) > > > +{ > > > + if (data->parent_data) > > > + return irq_chip_set_affinity_parent(data, dest, > > > force); > > > + > > > + return -EINVAL; > > > +} > > > > Why do you need this? Isn't it taken care of by the IRQ chip core? > Yes I believe we do/should, data->parent_data is used in > irq_chip_set_affinity_parent(..) without checking so hence checked > before calling. I mean the entire function. Isn't it the default in the IRQ chip core? Or can it be made as a default with some flags set? ... > > > + struct device_node *node = pdev->dev.of_node; > > > + struct device_node *irq_parent; > > > > Why do you need these? > Yes they are needed. Both of the same type but used in different > places. I don't think I can reuse. This is related to the pattern of how you are enumerating IRQs. If the pattern is changed, it would be not needed anymore. ... > > > + if (ngpio > NUM_GPIO) { > > > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > > > + NUM_GPIO); > > > + ret = -ENXIO; > > > + goto cleanup_clock; > > > > return dev_err_probe(...); > I need to cleanup clock before returning, will use dev_err_probe. Have you read what I wrote above? I wrote that you need to wrap the clk_disable_unprepare() into devm, so you may use as I pointed out, i.e. return dev_err_probe() everywhere in the ->probe(). > > > + } ... > > Why do you need all these? Seems a copy'n'paste from gpio-sifive, > > which is the only GPIO driver using this pattern. > Yes I believe we do need all this information. Yes it is similiar > implementation to sifive. Not the only driver using this pattern, check > gpio-ixp4xxx.c My question: why do you need this? What is so special about this driver? ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * > > > BYTE_BOUNDARY), > > > + MPFS_GPIO_EN_INT, 0); > > > > Too many parentheses. As per above. ... > > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", > > > ngpio); > > > > Noise. > Maybe, but useful information to know the ngpio. Nope. Use sysfs / debugfs / etc. No need to noise the log.
On 13/07/2022 23:14, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Jul 13, 2022 at 10:44 PM <Lewis.Hanly@microchip.com> wrote: >> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote: >>> On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: >>>> + if (gpio_cfg & MPFS_GPIO_EN_IN) >>>> + return 1; >>>> + >>>> + return 0; >>> >>> Don't we have specific definitions for the directions? >> No defines in file. > > We have. Please, check again. I said what they were on another reply Lewis: https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/driver.h#L25
OK, I have gone through all comments and taken on board your review comments. Version 3 will follow later today. On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > Add a driver to support the Polarfire SoC gpio controller. > > GPIO > > ... > > Below my comments, I have tried hard not to duplicate what Conor > already mentioned. So consider this as additional part. > > ... > > > +config GPIO_POLARFIRE_SOC > > + bool "Microchip FPGA GPIO support" > > Why not tristate? Not a module, compile time kernel driver allways for Polarfire SoC > > > + depends on OF_GPIO > > Why? > > > + select GPIOLIB_IRQCHIP > > + select IRQ_DOMAIN_HIERARCHY > > More naturally this line looks if put before GPIOLB_IRQCHIP one. OK > > > + select GPIO_GENERIC > > + help > > + Say yes here to support the GPIO device on Microchip > > FPGAs. > > When allowing it to be a module, add a text that tells how the driver > will be called. > > ... > > > +/* > > + * Microchip PolarFire SoC (MPFS) GPIO controller driver > > + * > > + * Copyright (c) 2018-2022 Microchip Technology Inc. and its > > subsidiaries > > + * > > + * Author: Lewis Hanly <lewis.hanly@microchip.com> > > + * > > This line is not needed. OK > > > + */ > > ... > > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > Why? I am using data defined in these headers. > > Also don't forget mod_devicetable.h. OK > > ... > > > +#define NUM_GPIO 32 > > +#define BYTE_BOUNDARY 0x04 > > Without namespace? > > ... > > > + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY)); > > + > > Unneeded line. > > > + if (gpio_cfg & MPFS_GPIO_EN_IN) > > + return 1; > > + > > + return 0; > > Don't we have specific definitions for the directions? > > ... > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > > + int gpio_index = irqd_to_hwirq(data); > > + u32 interrupt_type; > > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > > This line naturally looks better before interrupt_type definition. > Try to keep the "longest line first" everywhere in the driver. > > > + u32 gpio_cfg; > > + unsigned long flags; > > ... > > > + switch (type) { > > + case IRQ_TYPE_EDGE_BOTH: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH; > > + break; > > + > > Unneeded line here and everywhere in the similar cases in the entire > code. > > > + case IRQ_TYPE_EDGE_FALLING: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG; > > + break; > > + > > + case IRQ_TYPE_EDGE_RISING: > > + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS; > > + break; > > + > > + case IRQ_TYPE_LEVEL_HIGH: > > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH; > > + break; > > + > > + case IRQ_TYPE_LEVEL_LOW: > > + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW; > > + break; > > + } > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 1); > > Too many parentheses. Yes updated in next revision, using macro rather than * BYTE_BOUNDARY. > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 0); > > Ditto. > > ... > > > +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, > > + const struct cpumask *dest, > > + bool force) > > +{ > > + if (data->parent_data) > > + return irq_chip_set_affinity_parent(data, dest, > > force); > > + > > + return -EINVAL; > > +} > > Why do you need this? Isn't it taken care of by the IRQ chip core? irq_chip_set_affinity could be setup directly at irq_chip strcuture initialization, but I am checking parent_data before calling. So I would say yes I do need this. > > ... > > > + struct clk *clk; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct device_node *irq_parent; > > Why do you need these? Yes I, for setting up the hierarchical interrupt controller. > > > + struct gpio_irq_chip *girq; > > + struct irq_domain *parent; > > + struct mpfs_gpio_chip *mpfs_gpio; > > + int i, ret, ngpio; > > ... > > > + clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > > + return PTR_ERR(clk); > > + } > > return dev_err_probe(...); > > It's not only convenient, but here it fixes a bug. Using dev_err_probe instead of dev_err. > > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + return ret; > > return dev_err_probe(...); > > > + } > > Make it managed as well in addition to gpiochip_add_data(), otherwise > you will have wrong ordering. OK. > > ... > > > + ngpio = of_irq_count(node); > > + if (ngpio > NUM_GPIO) { > > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", > > + NUM_GPIO); > > + ret = -ENXIO; > > + goto cleanup_clock; > > return dev_err_probe(...); > > > + } > > + > > + irq_parent = of_irq_find_parent(node); > > + if (!irq_parent) { > > + dev_err(dev, "no IRQ parent node\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > Ditto. > > > + } > > + parent = irq_find_host(irq_parent); > > + if (!parent) { > > + dev_err(dev, "no IRQ parent domain\n"); > > + ret = -ENODEV; > > + goto cleanup_clock; > > Ditto. > > > + } > > Why do you need all these? Seems a copy'n'paste from gpio-sifive, > which is the only GPIO driver using this pattern. As above setup of hierarchical interrupt controller, very similiar to the sifive architecture. > > ... > > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * > > BYTE_BOUNDARY), > > + MPFS_GPIO_EN_INT, 0); > > Too many parentheses. > > > > + girq->fwnode = of_node_to_fwnode(node); > > This is an interesting way of > > ...->fwnode = dev_fwnode(dev); > > > ... > > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", > > ngpio); > > Noise. Noise removed. > > ... > > > + .of_match_table = of_match_ptr(mpfs_gpio_match), > > Please, avoid using of_match_ptr(). It brings more harm than > usefulness. OK > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b01961999ced..004b377b73f6 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -490,6 +490,15 @@ config GPIO_PMIC_EIC_SPRD help Say yes here to support Spreadtrum PMIC EIC device. +config GPIO_POLARFIRE_SOC + bool "Microchip FPGA GPIO support" + depends on OF_GPIO + select GPIOLIB_IRQCHIP + select IRQ_DOMAIN_HIERARCHY + select GPIO_GENERIC + help + Say yes here to support the GPIO device on Microchip FPGAs. + config GPIO_PXA bool "PXA GPIO support" depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 14352f6dfe8e..3b8b6703e593 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -119,6 +119,7 @@ obj-$(CONFIG_GPIO_PCI_IDIO_16) += gpio-pci-idio-16.o obj-$(CONFIG_GPIO_PISOSR) += gpio-pisosr.o obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PMIC_EIC_SPRD) += gpio-pmic-eic-sprd.o +obj-$(CONFIG_GPIO_POLARFIRE_SOC) += gpio-mpfs.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c new file mode 100644 index 000000000000..1b342f307d85 --- /dev/null +++ b/drivers/gpio/gpio-mpfs.c @@ -0,0 +1,379 @@ +// SPDX-License-Identifier: (GPL-2.0) +/* + * Microchip PolarFire SoC (MPFS) GPIO controller driver + * + * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries + * + * Author: Lewis Hanly <lewis.hanly@microchip.com> + * + */ + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/gpio/driver.h> +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +#define NUM_GPIO 32 +#define BYTE_BOUNDARY 0x04 +#define MPFS_GPIO_EN_INT 3 +#define MPFS_GPIO_EN_OUT_BUF BIT(2) +#define MPFS_GPIO_EN_IN BIT(1) +#define MPFS_GPIO_EN_OUT BIT(0) + +#define MPFS_GPIO_TYPE_INT_EDGE_BOTH 0x80 +#define MPFS_GPIO_TYPE_INT_EDGE_NEG 0x60 +#define MPFS_GPIO_TYPE_INT_EDGE_POS 0x40 +#define MPFS_GPIO_TYPE_INT_LEVEL_LOW 0x20 +#define MPFS_GPIO_TYPE_INT_LEVEL_HIGH 0x00 +#define MPFS_GPIO_TYPE_INT_MASK GENMASK(7, 5) +#define MPFS_IRQ_REG 0x80 +#define MPFS_INP_REG 0x84 +#define MPFS_OUTP_REG 0x88 + +struct mpfs_gpio_chip { + void __iomem *base; + struct clk *clk; + raw_spinlock_t lock; + struct gpio_chip gc; + unsigned int irq_number[NUM_GPIO]; +}; + +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value) +{ + unsigned long reg = readl(addr); + + __assign_bit(bit_offset, ®, value); + writel(reg, addr); +} + +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + gpio_cfg |= MPFS_GPIO_EN_IN; + gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF); + writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF; + gpio_cfg &= ~MPFS_GPIO_EN_IN; + writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, gpio_index, value); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static int mpfs_gpio_get_direction(struct gpio_chip *gc, + unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + + if (gpio_cfg & MPFS_GPIO_EN_IN) + return 1; + + return 0; +} + +static int mpfs_gpio_get(struct gpio_chip *gc, + unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + + return !!(readl(mpfs_gpio->base + MPFS_INP_REG) & BIT(gpio_index)); +} + +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, + gpio_index, value); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); +} + +static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + int gpio_index = irqd_to_hwirq(data); + u32 interrupt_type; + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + unsigned long flags; + + switch (type) { + case IRQ_TYPE_EDGE_BOTH: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH; + break; + + case IRQ_TYPE_EDGE_FALLING: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG; + break; + + case IRQ_TYPE_EDGE_RISING: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS; + break; + + case IRQ_TYPE_LEVEL_HIGH: + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH; + break; + + case IRQ_TYPE_LEVEL_LOW: + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW; + break; + } + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + gpio_cfg &= ~MPFS_GPIO_TYPE_INT_MASK; + gpio_cfg |= interrupt_type; + writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY)); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static void mpfs_gpio_irq_enable(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(data); + int gpio_index = hwirq % NUM_GPIO; + + gpiochip_enable_irq(gc, hwirq); + irq_chip_enable_parent(data); + + mpfs_gpio_direction_input(gc, gpio_index); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY), + MPFS_GPIO_EN_INT, 1); +} + +static void mpfs_gpio_irq_disable(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(data); + int gpio_index = hwirq % NUM_GPIO; + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY), + MPFS_GPIO_EN_INT, 0); + + irq_chip_disable_parent(data); + gpiochip_disable_irq(gc, hwirq); +} + +static void mpfs_gpio_irq_eoi(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(data) % NUM_GPIO; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + /* Clear pending interrupt */ + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1); + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + irq_chip_eoi_parent(data); +} + +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, + const struct cpumask *dest, + bool force) +{ + if (data->parent_data) + return irq_chip_set_affinity_parent(data, dest, force); + + return -EINVAL; +} + +static const struct irq_chip mpfs_gpio_irqchip = { + .name = "mpfs", + .irq_set_type = mpfs_gpio_irq_set_type, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_enable = mpfs_gpio_irq_enable, + .irq_disable = mpfs_gpio_irq_disable, + .irq_eoi = mpfs_gpio_irq_eoi, + .irq_set_affinity = mpfs_gpio_irq_set_affinity, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, + unsigned int child, + unsigned int child_type, + unsigned int *parent, + unsigned int *parent_type) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + struct irq_data *d = irq_get_irq_data(mpfs_gpio->irq_number[child]); + *parent_type = IRQ_TYPE_NONE; + *parent = irqd_to_hwirq(d); + + return 0; +} + +static int mpfs_gpio_probe(struct platform_device *pdev) +{ + struct clk *clk; + struct device *dev = &pdev->dev; + struct device_node *node = pdev->dev.of_node; + struct device_node *irq_parent; + struct gpio_irq_chip *girq; + struct irq_domain *parent; + struct mpfs_gpio_chip *mpfs_gpio; + int i, ret, ngpio; + + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL); + if (!mpfs_gpio) + return -ENOMEM; + + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(mpfs_gpio->base)) + return PTR_ERR(mpfs_gpio->base); + + clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "devm_clk_get failed\n"); + return PTR_ERR(clk); + } + + ret = clk_prepare_enable(clk); + if (ret) { + dev_err(&pdev->dev, "failed to enable clock\n"); + return ret; + } + + mpfs_gpio->clk = clk; + + ngpio = of_irq_count(node); + if (ngpio > NUM_GPIO) { + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", + NUM_GPIO); + ret = -ENXIO; + goto cleanup_clock; + } + + irq_parent = of_irq_find_parent(node); + if (!irq_parent) { + dev_err(dev, "no IRQ parent node\n"); + ret = -ENODEV; + goto cleanup_clock; + } + parent = irq_find_host(irq_parent); + if (!parent) { + dev_err(dev, "no IRQ parent domain\n"); + ret = -ENODEV; + goto cleanup_clock; + } + + /* Get the interrupt numbers. + * Clear/Disable All interrupts before enabling parent interrupts. + */ + for (i = 0; i < ngpio; i++) { + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY), + MPFS_GPIO_EN_INT, 0); + } + + raw_spin_lock_init(&mpfs_gpio->lock); + + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input; + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output; + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction; + mpfs_gpio->gc.get = mpfs_gpio_get; + mpfs_gpio->gc.set = mpfs_gpio_set; + mpfs_gpio->gc.base = -1; + mpfs_gpio->gc.ngpio = ngpio; + mpfs_gpio->gc.label = dev_name(dev); + mpfs_gpio->gc.parent = dev; + mpfs_gpio->gc.owner = THIS_MODULE; + + /* Get a pointer to the gpio_irq_chip */ + girq = &mpfs_gpio->gc.irq; + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip); + girq->fwnode = of_node_to_fwnode(node); + girq->parent_domain = parent; + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq; + girq->handler = handle_bad_irq; + girq->default_type = IRQ_TYPE_NONE; + + ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio); + if (ret) + goto cleanup_clock; + + platform_set_drvdata(pdev, mpfs_gpio); + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio); + + return 0; + +cleanup_clock: + clk_disable_unprepare(mpfs_gpio->clk); + return ret; +} + +static int mpfs_gpio_remove(struct platform_device *pdev) +{ + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev); + + gpiochip_remove(&mpfs_gpio->gc); + clk_disable_unprepare(mpfs_gpio->clk); + + return 0; +} + +static const struct of_device_id mpfs_gpio_match[] = { + { .compatible = "microchip,mpfs-gpio", }, + { /* end of list */ }, +}; + +static struct platform_driver mpfs_gpio_driver = { + .probe = mpfs_gpio_probe, + .driver = { + .name = "microchip,mpfs-gpio", + .of_match_table = of_match_ptr(mpfs_gpio_match), + }, + .remove = mpfs_gpio_remove, +}; +builtin_platform_driver(mpfs_gpio_driver)