| Message ID | 1371549692-7361-2-git-send-email-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/18/2013 04:01 AM, J Keerthy wrote: > Check if interrupts property exists and then only request irq. > On some boards INT line might not be connected to a valid > irq line on the application processor. Hence keeping a check > before requesting irq. When there is no interrupts property, surely i2c->irq == 0, which is an invalid IRQ, and hence there's no need to check this before copying the value? In other words, I think this whole patch could just be: + palmas->irq = i2c->irq; right? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Tuesday, June 18, 2013 9:22 PM > To: J, KEERTHY > Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; > swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- > doc@vger.kernel.org; gg@slimlogic.co.uk > Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property > exists and then only request irq > > On 06/18/2013 04:01 AM, J Keerthy wrote: > > Check if interrupts property exists and then only request irq. > > On some boards INT line might not be connected to a valid irq line on > > the application processor. Hence keeping a check before requesting > > irq. > > When there is no interrupts property, surely i2c->irq == 0, which is an > invalid IRQ, and hence there's no need to check this before copying the > value? The intent here is NOT to request irq with 0 or Invalid IRQ. The board File will not populate the interrupts entry if the INT line is not Connected. Hence the patch checks for the 'interrupts' property. This is essential since I do not want the probe to return error Just because the i2c->irq == 0. The explicit check for the property Ensures that the board does not have the INT line connected to A valid interrupt line on the other side. > > In other words, I think this whole patch could just be: > > + palmas->irq = i2c->irq; > > right? Just this will cause a probe failure. That is not what is needed. Instead the probe should continue skipping irq part. Regards, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2013 10:54 AM, J, KEERTHY wrote: > Hi Stephen, > >> -----Original Message----- >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] >> Sent: Tuesday, June 18, 2013 9:22 PM >> To: J, KEERTHY >> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; >> ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; >> swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- >> doc@vger.kernel.org; gg@slimlogic.co.uk >> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property >> exists and then only request irq >> >> On 06/18/2013 04:01 AM, J Keerthy wrote: >>> Check if interrupts property exists and then only request irq. >>> On some boards INT line might not be connected to a valid irq line on >>> the application processor. Hence keeping a check before requesting >>> irq. >> >> When there is no interrupts property, surely i2c->irq == 0, which is an >> invalid IRQ, and hence there's no need to check this before copying the >> value? > > The intent here is NOT to request irq with 0 or Invalid IRQ. Sure. > The board File will not populate the interrupts entry if the INT line is not > Connected. Do you mean the interrupts DT property won't be present if there is no interrupt. If so, sure. > Hence the patch checks for the 'interrupts' property. That shouldn't be necessary; IIRC, the I2C core has already parsed the interrupts property if there was one, and if there wasn't, it has set i2c->irq to some invalid value already. So, you simply need to check the value in i2c->irq, and don't need to look at the DT at all. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Tuesday, June 18, 2013 10:38 PM > To: J, KEERTHY > Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; > swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- > doc@vger.kernel.org; gg@slimlogic.co.uk > Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property > exists and then only request irq > > On 06/18/2013 10:54 AM, J, KEERTHY wrote: > > Hi Stephen, > > > >> -----Original Message----- > >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] > >> Sent: Tuesday, June 18, 2013 9:22 PM > >> To: J, KEERTHY > >> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > >> ldewangan@nvidia.com; sameo@linux.intel.com; > >> grant.likely@secretlab.ca; swarren@nvidia.com; > >> linux-kernel@vger.kernel.org; linux- doc@vger.kernel.org; > >> gg@slimlogic.co.uk > >> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts > property > >> exists and then only request irq > >> > >> On 06/18/2013 04:01 AM, J Keerthy wrote: > >>> Check if interrupts property exists and then only request irq. > >>> On some boards INT line might not be connected to a valid irq line > >>> on the application processor. Hence keeping a check before > >>> requesting irq. > >> > >> When there is no interrupts property, surely i2c->irq == 0, which is > >> an invalid IRQ, and hence there's no need to check this before > >> copying the value? > > > > The intent here is NOT to request irq with 0 or Invalid IRQ. > > Sure. > > > The board File will not populate the interrupts entry if the INT line > > is not Connected. > > Do you mean the interrupts DT property won't be present if there is no > interrupt. If so, sure. Yes. > > > Hence the patch checks for the 'interrupts' property. > > That shouldn't be necessary; IIRC, the I2C core has already parsed the > interrupts property if there was one, and if there wasn't, it has set > i2c->irq to some invalid value already. > > So, you simply need to check the value in i2c->irq, and don't need to > look at the DT at all. Instead of checking the Invalid irq value which most likely can be 0. I am not sure. I am explicitly checking if the interrupts property exists or not. If not present then It throws out a warning. Either there is no Valid INT line connection or the DeviceTree was not populated fully. This additional piece of information is good to have in the driver IMHO. Let me know if this is rational enough to have in the driver. Regards, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2013 11:19 AM, J, KEERTHY wrote: > > >> -----Original Message----- >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] >> Sent: Tuesday, June 18, 2013 10:38 PM >> To: J, KEERTHY >> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; >> ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; >> swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- >> doc@vger.kernel.org; gg@slimlogic.co.uk >> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property >> exists and then only request irq >> >> On 06/18/2013 10:54 AM, J, KEERTHY wrote: >>> Hi Stephen, >>> >>>> -----Original Message----- >>>> From: Stephen Warren [mailto:swarren@wwwdotorg.org] >>>> Sent: Tuesday, June 18, 2013 9:22 PM >>>> To: J, KEERTHY >>>> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; >>>> ldewangan@nvidia.com; sameo@linux.intel.com; >>>> grant.likely@secretlab.ca; swarren@nvidia.com; >>>> linux-kernel@vger.kernel.org; linux- doc@vger.kernel.org; >>>> gg@slimlogic.co.uk >>>> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts >> property >>>> exists and then only request irq >>>> >>>> On 06/18/2013 04:01 AM, J Keerthy wrote: >>>>> Check if interrupts property exists and then only request irq. >>>>> On some boards INT line might not be connected to a valid irq line >>>>> on the application processor. Hence keeping a check before >>>>> requesting irq. >>>> >>>> When there is no interrupts property, surely i2c->irq == 0, which is >>>> an invalid IRQ, and hence there's no need to check this before >>>> copying the value? >>> >>> The intent here is NOT to request irq with 0 or Invalid IRQ. >> >> Sure. >> >>> The board File will not populate the interrupts entry if the INT line >>> is not Connected. >> >> Do you mean the interrupts DT property won't be present if there is no >> interrupt. If so, sure. > > Yes. > >> >>> Hence the patch checks for the 'interrupts' property. >> >> That shouldn't be necessary; IIRC, the I2C core has already parsed the >> interrupts property if there was one, and if there wasn't, it has set >> i2c->irq to some invalid value already. >> >> So, you simply need to check the value in i2c->irq, and don't need to >> look at the DT at all. > > Instead of checking the Invalid irq value which most likely can be 0. > I am not sure. > I am explicitly checking if the interrupts property exists or not. > > If not present then It throws out a warning. Either there is no > Valid INT line connection or the DeviceTree was not populated fully. > > This additional piece of information is good to have in the driver > IMHO. Let me know if this is rational enough to have in the driver. No, you should just check the IRQ number. Consider this: If the device was instantiated from a board file *or* a device tree, i2c->irq is correctly set. Hence, checking that value works in both cases. If you check the interrupts DT property, that will only work if the device was instantiated from device tree, and not if it was instantiated from a board file; the property will never exist in the board file case, and hence you'll never be able to have a board file provide an interrupt. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Tuesday, June 18, 2013 10:53 PM > To: J, KEERTHY > Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; > swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- > doc@vger.kernel.org; gg@slimlogic.co.uk > Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property > exists and then only request irq > > On 06/18/2013 11:19 AM, J, KEERTHY wrote: > > > > > >> -----Original Message----- > >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] > >> Sent: Tuesday, June 18, 2013 10:38 PM > >> To: J, KEERTHY > >> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > >> ldewangan@nvidia.com; sameo@linux.intel.com; > >> grant.likely@secretlab.ca; swarren@nvidia.com; > >> linux-kernel@vger.kernel.org; linux- doc@vger.kernel.org; > >> gg@slimlogic.co.uk > >> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts > property > >> exists and then only request irq > >> > >> On 06/18/2013 10:54 AM, J, KEERTHY wrote: > >>> Hi Stephen, > >>> > >>>> -----Original Message----- > >>>> From: Stephen Warren [mailto:swarren@wwwdotorg.org] > >>>> Sent: Tuesday, June 18, 2013 9:22 PM > >>>> To: J, KEERTHY > >>>> Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > >>>> ldewangan@nvidia.com; sameo@linux.intel.com; > >>>> grant.likely@secretlab.ca; swarren@nvidia.com; > >>>> linux-kernel@vger.kernel.org; linux- doc@vger.kernel.org; > >>>> gg@slimlogic.co.uk > >>>> Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts > >> property > >>>> exists and then only request irq > >>>> > >>>> On 06/18/2013 04:01 AM, J Keerthy wrote: > >>>>> Check if interrupts property exists and then only request irq. > >>>>> On some boards INT line might not be connected to a valid irq > line > >>>>> on the application processor. Hence keeping a check before > >>>>> requesting irq. > >>>> > >>>> When there is no interrupts property, surely i2c->irq == 0, which > >>>> is an invalid IRQ, and hence there's no need to check this before > >>>> copying the value? > >>> > >>> The intent here is NOT to request irq with 0 or Invalid IRQ. > >> > >> Sure. > >> > >>> The board File will not populate the interrupts entry if the INT > >>> line is not Connected. > >> > >> Do you mean the interrupts DT property won't be present if there is > >> no interrupt. If so, sure. > > > > Yes. > > > >> > >>> Hence the patch checks for the 'interrupts' property. > >> > >> That shouldn't be necessary; IIRC, the I2C core has already parsed > >> the interrupts property if there was one, and if there wasn't, it > has > >> set > >> i2c->irq to some invalid value already. > >> > >> So, you simply need to check the value in i2c->irq, and don't need > to > >> look at the DT at all. > > > > Instead of checking the Invalid irq value which most likely can be 0. > > I am not sure. > > I am explicitly checking if the interrupts property exists or not. > > > > If not present then It throws out a warning. Either there is no Valid > > INT line connection or the DeviceTree was not populated fully. > > > > This additional piece of information is good to have in the driver > > IMHO. Let me know if this is rational enough to have in the driver. > > No, you should just check the IRQ number. Hmmm...so something like (!i2c->irq) > > Consider this: > > If the device was instantiated from a board file *or* a device tree, > i2c->irq is correctly set. Hence, checking that value works in both > cases. > > If you check the interrupts DT property, that will only work if the > device was instantiated from device tree, and not if it was > instantiated from a board file; the property will never exist in the > board file case, and hence you'll never be able to have a board file > provide an interrupt. The board file approach is getting deprecated for this. I Myself removed board file related pdata stuff in one of the patches. http://www.mail-archive.com/linux-omap@vger.kernel.org/msg90598.html So going the DeviceTree way. Regards, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 18, 2013 at 11:22:41AM -0600, Stephen Warren wrote: > If the device was instantiated from a board file *or* a device tree, > i2c->irq is correctly set. Hence, checking that value works in both cases. The same thing will happen with any other firmware interface that gets introduced in the future - one of the goals with factoring all this out into the bus code is that it means the driver doesn't need to have any special handling. > If you check the interrupts DT property, that will only work if the > device was instantiated from device tree, and not if it was instantiated > from a board file; the property will never exist in the board file case, > and hence you'll never be able to have a board file provide an interrupt. Exactly.
On 06/18/2013 11:33 AM, J, KEERTHY wrote: > Stephen Warren wrote at Tuesday, June 18, 2013 10:53 PM: ...>> No, you should just check the IRQ number. > > Hmmm...so something like (!i2c->irq) Yes. >> Consider this: >> >> If the device was instantiated from a board file *or* a device tree, >> i2c->irq is correctly set. Hence, checking that value works in both >> cases. >> >> If you check the interrupts DT property, that will only work if the >> device was instantiated from device tree, and not if it was >> instantiated from a board file; the property will never exist in the >> board file case, and hence you'll never be able to have a board file >> provide an interrupt. > > The board file approach is getting deprecated for this. I > Myself removed board file related pdata stuff in one of the patches. > > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg90598.html > > So going the DeviceTree way. Even if you're 100% sure this driver will only ever work with DT (which seems like a bad assumption to make no matter what the circumstance), it'd still be best to detect whether an IRQ was specified in a generic way. That way, nobody will read this driver, assume the code is generic, and just copy/paste it without thinking. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Wednesday, June 19, 2013 12:42 AM > To: J, KEERTHY > Cc: linux-omap@vger.kernel.org; broonie@kernel.org; > ldewangan@nvidia.com; sameo@linux.intel.com; grant.likely@secretlab.ca; > swarren@nvidia.com; linux-kernel@vger.kernel.org; linux- > doc@vger.kernel.org; gg@slimlogic.co.uk > Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property > exists and then only request irq > > On 06/18/2013 11:33 AM, J, KEERTHY wrote: > > Stephen Warren wrote at Tuesday, June 18, 2013 10:53 PM: > ...>> No, you should just check the IRQ number. > > > > Hmmm...so something like (!i2c->irq) > > Yes. Ok. > > >> Consider this: > >> > >> If the device was instantiated from a board file *or* a device tree, > >> i2c->irq is correctly set. Hence, checking that value works in both > >> cases. > >> > >> If you check the interrupts DT property, that will only work if the > >> device was instantiated from device tree, and not if it was > >> instantiated from a board file; the property will never exist in the > >> board file case, and hence you'll never be able to have a board file > >> provide an interrupt. > > > > The board file approach is getting deprecated for this. I Myself > > removed board file related pdata stuff in one of the patches. > > > > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg90598.html > > > > So going the DeviceTree way. > > Even if you're 100% sure this driver will only ever work with DT (which > seems like a bad assumption to make no matter what the circumstance), > it'd still be best to detect whether an IRQ was specified in a generic > way. That way, nobody will read this driver, assume the code is > generic, and just copy/paste it without thinking. Ok. I understand. I will incorporate this. Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c index 62fa728..69c4e62 100644 --- a/drivers/mfd/palmas.c +++ b/drivers/mfd/palmas.c @@ -236,6 +236,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c, { struct palmas *palmas; struct palmas_platform_data *pdata; + struct device_node *regnode = NULL; struct device_node *node = i2c->dev.of_node; int ret = 0, i; unsigned int reg, addr; @@ -262,7 +263,6 @@ static int palmas_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, palmas); palmas->dev = &i2c->dev; palmas->id = id->driver_data; - palmas->irq = i2c->irq; for (i = 0; i < PALMAS_NUM_CLIENTS; i++) { if (i == 0) @@ -290,6 +290,15 @@ static int palmas_i2c_probe(struct i2c_client *i2c, } } + regnode = of_parse_phandle(node, "interrupts", 0); + + if (!regnode) { + dev_warn(palmas->dev, "IRQ missing: skipping irq request\n"); + goto no_irq; + } else { + palmas->irq = i2c->irq; + } + /* Change interrupt line output polarity */ if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH) reg = PALMAS_POLARITY_CTRL_INT_POLARITY; @@ -316,6 +325,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c, if (ret < 0) goto err; +no_irq: slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE); addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE, PALMAS_PRIMARY_SECONDARY_PAD1);
Check if interrupts property exists and then only request irq. On some boards INT line might not be connected to a valid irq line on the application processor. Hence keeping a check before requesting irq. Boot tested on OMAP5-UEVM board. Signed-off-by: J Keerthy <j-keerthy@ti.com> --- drivers/mfd/palmas.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)