| Message ID | 1387831563-13535-4-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
> Convert this driver to use irqdomains so that the PMIC's child > devices can be converted to devicetree. > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/mfd/pm8921-core.c | 184 ++++++++++++++------------------------ > include/linux/mfd/pm8xxx/irq.h | 36 -------- > include/linux/mfd/pm8xxx/pm8921.h | 30 ------- > 3 files changed, 65 insertions(+), 185 deletions(-) > delete mode 100644 include/linux/mfd/pm8xxx/irq.h > delete mode 100644 include/linux/mfd/pm8xxx/pm8921.h > <snip> > @@ -56,8 +56,7 @@ > struct pm_irq_chip { > struct device *dev; > spinlock_t pm_irq_lock; > - unsigned int devirq; > - unsigned int irq_base; > + struct irq_domain *domain; It's probably best to explicitly specify 'irqdomain' here in order to eliminate any possibility of ambiguity. > unsigned int num_irqs; > unsigned int num_blocks; > unsigned int num_masters; > @@ -138,7 +137,7 @@ static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip, int block) > for (i = 0; i < 8; i++) { > if (bits & (1 << i)) { > pmirq = block * 8 + i; > - irq = pmirq + chip->irq_base; > + irq = irq_find_mapping(chip->domain, pmirq); Going by this patch only, it appears you're calling irq_find_mapping() before you've called irq_create_mapping(). This won't work, so unless you've called the latter in a previous patch, you should ensure that you do so. > generic_handle_irq(irq); > } > } > @@ -197,12 +196,11 @@ static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc) > static void pm8xxx_irq_mask_ack(struct irq_data *d) > { > struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); > - unsigned int pmirq = d->irq - chip->irq_base; > - int master, irq_bit; > + unsigned int pmirq = irqd_to_hwirq(d); Why don't you call it hwirq instead of pmirq? > + int irq_bit; > u8 block, config; > > block = pmirq / 8; Ah, I guess you're keeping the original naming convention. > - master = block / 8; What was the point in this anyway? Was it completely superfluous? <snip> > +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq) > +{ > + struct pm_irq_chip *chip; > + unsigned int nirqs = 256; No magic numbers please. > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip) + sizeof(u8) * nirqs, > + GFP_KERNEL); What does the sizeof(u8) add here? <snip> > + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > +} Can't you use the MFD core instead?
On 01/06/14 07:53, Lee Jones wrote: > >> @@ -56,8 +56,7 @@ >> struct pm_irq_chip { >> struct device *dev; >> spinlock_t pm_irq_lock; >> - unsigned int devirq; >> - unsigned int irq_base; >> + struct irq_domain *domain; > It's probably best to explicitly specify 'irqdomain' here in order to > eliminate any possibility of ambiguity. Ok. Renamed. > >> unsigned int num_irqs; >> unsigned int num_blocks; >> unsigned int num_masters; >> @@ -138,7 +137,7 @@ static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip, int block) >> for (i = 0; i < 8; i++) { >> if (bits & (1 << i)) { >> pmirq = block * 8 + i; >> - irq = pmirq + chip->irq_base; >> + irq = irq_find_mapping(chip->domain, pmirq); > Going by this patch only, it appears you're calling irq_find_mapping() > before you've called irq_create_mapping(). This won't work, so unless > you've called the latter in a previous patch, you should ensure that > you do so. > Interrupts seem to work. I think that's because the mapping is created when the consumer drivers call request_irq(). From what I can tell, if we call irq_find_mapping() and there is no mapping associated with it then we have a spurious irq. If that happens we'll call handle_generic_irq() with 0 and that will cause handle_bad_irq() to be called and a debug message to be logged. That seems like a good outcome. >> - master = block / 8; > What was the point in this anyway? Was it completely superfluous? I think it was just copy/paste without thinking if the variables are actually used. > >> +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq) >> +{ >> + struct pm_irq_chip *chip; >> + unsigned int nirqs = 256; > No magic numbers please. Done. > >> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip) + sizeof(u8) * nirqs, >> + GFP_KERNEL); > What does the sizeof(u8) add here? > This was just keeping the same code that was already there. I will do sizeof(chip->config[0]) instead which is more future proof if that array changes type later on. > >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); >> +} > Can't you use the MFD core instead? > Are you suggesting using mfd_add_devices()? At first glance it looks like that would require an array of mfd_cell structures that do nothing besides match compatible strings in the DT. Using of_platform_populate() achieves the same goal and doesn't require an array of mfd_cell structures for each different pm8xxx chip that comes along, meaning simpler code.
> >> unsigned int num_irqs; > >> unsigned int num_blocks; > >> unsigned int num_masters; > >> @@ -138,7 +137,7 @@ static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip, int block) > >> for (i = 0; i < 8; i++) { > >> if (bits & (1 << i)) { > >> pmirq = block * 8 + i; > >> - irq = pmirq + chip->irq_base; > >> + irq = irq_find_mapping(chip->domain, pmirq); > > Going by this patch only, it appears you're calling irq_find_mapping() > > before you've called irq_create_mapping(). This won't work, so unless > > you've called the latter in a previous patch, you should ensure that > > you do so. > > > > Interrupts seem to work. I think that's because the mapping is created > when the consumer drivers call request_irq(). > > From what I can tell, if we call irq_find_mapping() and there is no > mapping associated with it then we have a spurious irq. If that happens > we'll call handle_generic_irq() with 0 and that will cause > handle_bad_irq() to be called and a debug message to be logged. That > seems like a good outcome. I would try to adhere to the documentation in case we are missing something or some of the semantics change. Please read: Documentation/IRQ-domain.txt. Specifically, "=== irq_domain usage ===" from line 39, which says to call irq_create_mapping() to indeed, create the mapping. > > What does the sizeof(u8) add here? > > > > This was just keeping the same code that was already there. I will do > sizeof(chip->config[0]) instead which is more future proof if that array > changes type later on. Ah, now I see what it's doing. Perhaps brackets would be of use to ensure readers aren't confused. I also think the sizeof() would be helpful too, so: chip = devm_kzalloc(&pdev->dev, sizeof(*chip) + (sizeof(chip->config[0]) * nirqs), GFP_KERNEL); > >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > >> +} > > Can't you use the MFD core instead? > > > > Are you suggesting using mfd_add_devices()? At first glance it looks > like that would require an array of mfd_cell structures that do nothing > besides match compatible strings in the DT. Using of_platform_populate() > achieves the same goal and doesn't require an array of mfd_cell > structures for each different pm8xxx chip that comes along, meaning > simpler code. I'm inclined to agree, but playing Devil's advocate here, as a device using the MFD subsystem it's often clearer to readers and other people looking for examples if the MFD core functionality is used. For instance, I now have no idea what devices the PM8xxx encompasses without looking at the DTS file. A small cell structure is a small price to pay for code clarity IMHO.
On 01/07, Lee Jones wrote: > > >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > >> +} > > > Can't you use the MFD core instead? > > > > > > > Are you suggesting using mfd_add_devices()? At first glance it looks > > like that would require an array of mfd_cell structures that do nothing > > besides match compatible strings in the DT. Using of_platform_populate() > > achieves the same goal and doesn't require an array of mfd_cell > > structures for each different pm8xxx chip that comes along, meaning > > simpler code. > > I'm inclined to agree, but playing Devil's advocate here, as a device > using the MFD subsystem it's often clearer to readers and other people > looking for examples if the MFD core functionality is used. For > instance, I now have no idea what devices the PM8xxx encompasses > without looking at the DTS file. A small cell structure is a small > price to pay for code clarity IMHO. > Why not just put that information in the binding document? And how is this different from adding a bunch of C files to match a set of compatible strings that a dts file has just so that we can add all the devices on a board (think board files for an SoC). Sure it documents the devices on a board, but we've been moving away from that by using of_platform_populate(). IMHO the code is clear. I want to add all subnodes of this device's node as children struct devices. Using of_platform_populate() says that, whereas mfd_add_devices() says I want to add these specific subnodes of this device's node. Also, as more drivers are written and more bindings are ratified this platform driver will need to be updated with more cells and more compatible strings, causing more inter-tree dependencies and more patches. Please, let's avoid this if we can.
On Tue, 07 Jan 2014, Stephen Boyd wrote: > On 01/07, Lee Jones wrote: > > > >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > > >> +} > > > > Can't you use the MFD core instead? > > > > > > > > > > Are you suggesting using mfd_add_devices()? At first glance it looks > > > like that would require an array of mfd_cell structures that do nothing > > > besides match compatible strings in the DT. Using of_platform_populate() > > > achieves the same goal and doesn't require an array of mfd_cell > > > structures for each different pm8xxx chip that comes along, meaning > > > simpler code. > > > > I'm inclined to agree, but playing Devil's advocate here, as a device > > using the MFD subsystem it's often clearer to readers and other people > > looking for examples if the MFD core functionality is used. For > > instance, I now have no idea what devices the PM8xxx encompasses > > without looking at the DTS file. A small cell structure is a small > > price to pay for code clarity IMHO. > > > > Why not just put that information in the binding document? And > how is this different from adding a bunch of C files to match a > set of compatible strings that a dts file has just so that we can > add all the devices on a board (think board files for an SoC). > Sure it documents the devices on a board, but we've been moving > away from that by using of_platform_populate(). > > IMHO the code is clear. I want to add all subnodes of this > device's node as children struct devices. Using > of_platform_populate() says that, whereas mfd_add_devices() says > I want to add these specific subnodes of this device's node. > > Also, as more drivers are written and more bindings are ratified > this platform driver will need to be updated with more cells and > more compatible strings, causing more inter-tree dependencies and > more patches. Please, let's avoid this if we can. Okay, that's fine. There are a few discussions floating around about this. If I find some time, I'll have to have a think about the pros and cons of either approach. This is okay for now.
diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c index 7964932..5d1d509 100644 --- a/drivers/mfd/pm8921-core.c +++ b/drivers/mfd/pm8921-core.c @@ -17,15 +17,15 @@ #include <linux/interrupt.h> #include <linux/irqchip/chained_irq.h> #include <linux/irq.h> +#include <linux/irqdomain.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/err.h> #include <linux/ssbi.h> +#include <linux/of_platform.h> #include <linux/mfd/core.h> -#include <linux/mfd/pm8xxx/pm8921.h> #include <linux/mfd/pm8xxx/core.h> -#include <linux/mfd/pm8xxx/irq.h> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB @@ -56,8 +56,7 @@ struct pm_irq_chip { struct device *dev; spinlock_t pm_irq_lock; - unsigned int devirq; - unsigned int irq_base; + struct irq_domain *domain; unsigned int num_irqs; unsigned int num_blocks; unsigned int num_masters; @@ -138,7 +137,7 @@ static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip, int block) for (i = 0; i < 8; i++) { if (bits & (1 << i)) { pmirq = block * 8 + i; - irq = pmirq + chip->irq_base; + irq = irq_find_mapping(chip->domain, pmirq); generic_handle_irq(irq); } } @@ -197,12 +196,11 @@ static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc) static void pm8xxx_irq_mask_ack(struct irq_data *d) { struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); - unsigned int pmirq = d->irq - chip->irq_base; - int master, irq_bit; + unsigned int pmirq = irqd_to_hwirq(d); + int irq_bit; u8 block, config; block = pmirq / 8; - master = block / 8; irq_bit = pmirq % 8; config = chip->config[pmirq] | PM_IRQF_MASK_ALL | PM_IRQF_CLR; @@ -212,12 +210,11 @@ static void pm8xxx_irq_mask_ack(struct irq_data *d) static void pm8xxx_irq_unmask(struct irq_data *d) { struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); - unsigned int pmirq = d->irq - chip->irq_base; - int master, irq_bit; + unsigned int pmirq = irqd_to_hwirq(d); + int irq_bit; u8 block, config; block = pmirq / 8; - master = block / 8; irq_bit = pmirq % 8; config = chip->config[pmirq]; @@ -227,12 +224,11 @@ static void pm8xxx_irq_unmask(struct irq_data *d) static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) { struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); - unsigned int pmirq = d->irq - chip->irq_base; - int master, irq_bit; + unsigned int pmirq = irqd_to_hwirq(d); + int irq_bit; u8 block, config; block = pmirq / 8; - master = block / 8; irq_bit = pmirq % 8; chip->config[pmirq] = (irq_bit << PM_IRQF_BITS_SHIFT) @@ -287,12 +283,9 @@ static int pm8xxx_get_irq_stat(struct pm_irq_chip *chip, int irq) int pmirq, rc; u8 block, bits, bit; unsigned long flags; + struct irq_data *irq_data = irq_get_irq_data(irq); - if (chip == NULL || irq < chip->irq_base || - irq >= chip->irq_base + chip->num_irqs) - return -EINVAL; - - pmirq = irq - chip->irq_base; + pmirq = irq_data->hwirq; block = pmirq / 8; bit = pmirq % 8; @@ -321,64 +314,55 @@ bail_out: return rc; } -static struct pm_irq_chip *pm8xxx_irq_init(struct device *dev, - const struct pm8xxx_irq_platform_data *pdata) +static struct lock_class_key pm8xxx_irq_lock_class; + +static int pm8xxx_irq_domain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hwirq) { - struct pm_irq_chip *chip; - int devirq, rc; - unsigned int pmirq; + struct pm_irq_chip *chip = d->host_data; - if (!pdata) { - pr_err("No platform data\n"); - return ERR_PTR(-EINVAL); - } + irq_set_lockdep_class(irq, &pm8xxx_irq_lock_class); + irq_set_chip_and_handler(irq, &pm8xxx_irq_chip, handle_level_irq); + irq_set_chip_data(irq, chip); +#ifdef CONFIG_ARM + set_irq_flags(irq, IRQF_VALID); +#else + irq_set_noprobe(irq); +#endif + return 0; +} - devirq = pdata->devirq; - if (devirq < 0) { - pr_err("missing devirq\n"); - rc = devirq; - return ERR_PTR(-EINVAL); - } +static const struct irq_domain_ops pm8xxx_irq_domain_ops = { + .xlate = irq_domain_xlate_twocell, + .map = pm8xxx_irq_domain_map, +}; - chip = kzalloc(sizeof(struct pm_irq_chip) - + sizeof(u8) * pdata->irq_cdata.nirqs, GFP_KERNEL); - if (!chip) { - pr_err("Cannot alloc pm_irq_chip struct\n"); - return ERR_PTR(-EINVAL); - } +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq) +{ + struct pm_irq_chip *chip; + unsigned int nirqs = 256; + + chip = devm_kzalloc(&pdev->dev, sizeof(*chip) + sizeof(u8) * nirqs, + GFP_KERNEL); + if (!chip) + return -ENOMEM; - chip->dev = dev; - chip->devirq = devirq; - chip->irq_base = pdata->irq_base; - chip->num_irqs = pdata->irq_cdata.nirqs; + chip->dev = &pdev->dev; + chip->num_irqs = nirqs; chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8); chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8); spin_lock_init(&chip->pm_irq_lock); - for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) { - irq_set_chip_and_handler(chip->irq_base + pmirq, - &pm8xxx_irq_chip, - handle_level_irq); - irq_set_chip_data(chip->irq_base + pmirq, chip); -#ifdef CONFIG_ARM - set_irq_flags(chip->irq_base + pmirq, IRQF_VALID); -#else - irq_set_noprobe(chip->irq_base + pmirq); -#endif - } + chip->domain = irq_domain_add_linear(pdev->dev.of_node, nirqs, + &pm8xxx_irq_domain_ops, + chip); + if (!chip->domain) + return -ENODEV; - irq_set_irq_type(devirq, pdata->irq_trigger_flag); - irq_set_handler_data(devirq, chip); - irq_set_chained_handler(devirq, pm8xxx_irq_handler); - irq_set_irq_wake(devirq, 1); + irq_set_handler_data(irq, chip); + irq_set_chained_handler(irq, pm8xxx_irq_handler); + irq_set_irq_wake(irq, 1); - return chip; -} - -static int pm8xxx_irq_exit(struct pm_irq_chip *chip) -{ - irq_set_chained_handler(chip->devirq, NULL); - kfree(chip); return 0; } @@ -432,42 +416,18 @@ static struct pm8xxx_drvdata pm8921_drvdata = { .pmic_read_irq_stat = pm8921_read_irq_stat, }; -static int pm8921_add_subdevices(const struct pm8921_platform_data - *pdata, - struct pm8921 *pmic, - u32 rev) -{ - int ret = 0, irq_base = 0; - struct pm_irq_chip *irq_chip; - - if (pdata->irq_pdata) { - pdata->irq_pdata->irq_cdata.nirqs = PM8921_NR_IRQS; - pdata->irq_pdata->irq_cdata.rev = rev; - irq_base = pdata->irq_pdata->irq_base; - irq_chip = pm8xxx_irq_init(pmic->dev, pdata->irq_pdata); - - if (IS_ERR(irq_chip)) { - pr_err("Failed to init interrupts ret=%ld\n", - PTR_ERR(irq_chip)); - return PTR_ERR(irq_chip); - } - pmic->irq_chip = irq_chip; - } - return ret; -} - static int pm8921_probe(struct platform_device *pdev) { - const struct pm8921_platform_data *pdata = dev_get_platdata(&pdev->dev); struct pm8921 *pmic; int rc; u8 val; + unsigned int irq; u32 rev; - if (!pdata) { - pr_err("missing platform data\n"); - return -EINVAL; - } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; pmic = devm_kzalloc(&pdev->dev, sizeof(struct pm8921), GFP_KERNEL); if (!pmic) { @@ -498,37 +458,23 @@ static int pm8921_probe(struct platform_device *pdev) pm8921_drvdata.pm_chip_data = pmic; platform_set_drvdata(pdev, &pm8921_drvdata); - rc = pm8921_add_subdevices(pdata, pmic, rev); - if (rc) { - pr_err("Cannot add subdevices rc=%d\n", rc); - goto err; - } + rc = pm8xxx_irq_init(pdev, irq); + if (rc) + return rc; - /* gpio might not work if no irq device is found */ - WARN_ON(pmic->irq_chip == NULL); + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); +} +static int pm8921_remove_child(struct device *dev, void *unused) +{ + platform_device_unregister(to_platform_device(dev)); return 0; - -err: - mfd_remove_devices(pmic->dev); - return rc; } static int pm8921_remove(struct platform_device *pdev) { - struct pm8xxx_drvdata *drvdata; - struct pm8921 *pmic = NULL; - - drvdata = platform_get_drvdata(pdev); - if (drvdata) - pmic = drvdata->pm_chip_data; - if (pmic) - mfd_remove_devices(pmic->dev); - if (pmic->irq_chip) { - pm8xxx_irq_exit(pmic->irq_chip); - pmic->irq_chip = NULL; - } - + device_for_each_child(&pdev->dev, NULL, pm8921_remove_child); + irq_set_chained_handler(platform_get_irq(pdev, 0), NULL); return 0; } diff --git a/include/linux/mfd/pm8xxx/irq.h b/include/linux/mfd/pm8xxx/irq.h deleted file mode 100644 index 1a11b02..0000000 --- a/include/linux/mfd/pm8xxx/irq.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright (c) 2011, Code Aurora Forum. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ -/* - * Qualcomm PMIC irq 8xxx driver header file - * - */ - -#ifndef __MFD_PM8XXX_IRQ_H -#define __MFD_PM8XXX_IRQ_H - -#include <linux/errno.h> -#include <linux/err.h> - -struct pm8xxx_irq_core_data { - u32 rev; - int nirqs; -}; - -struct pm8xxx_irq_platform_data { - int irq_base; - struct pm8xxx_irq_core_data irq_cdata; - int devirq; - int irq_trigger_flag; -}; - -#endif /* __MFD_PM8XXX_IRQ_H */ diff --git a/include/linux/mfd/pm8xxx/pm8921.h b/include/linux/mfd/pm8xxx/pm8921.h deleted file mode 100644 index 00fa3de..0000000 --- a/include/linux/mfd/pm8xxx/pm8921.h +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (c) 2011, Code Aurora Forum. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ -/* - * Qualcomm PMIC 8921 driver header file - * - */ - -#ifndef __MFD_PM8921_H -#define __MFD_PM8921_H - -#include <linux/mfd/pm8xxx/irq.h> - -#define PM8921_NR_IRQS 256 - -struct pm8921_platform_data { - int irq_base; - struct pm8xxx_irq_platform_data *irq_pdata; -}; - -#endif
Convert this driver to use irqdomains so that the PMIC's child devices can be converted to devicetree. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/mfd/pm8921-core.c | 184 ++++++++++++++------------------------ include/linux/mfd/pm8xxx/irq.h | 36 -------- include/linux/mfd/pm8xxx/pm8921.h | 30 ------- 3 files changed, 65 insertions(+), 185 deletions(-) delete mode 100644 include/linux/mfd/pm8xxx/irq.h delete mode 100644 include/linux/mfd/pm8xxx/pm8921.h