| Message ID | 201401091252.s09CqsOr042762@swsrvapps-02.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Anthony, On Thu, Jan 09, 2014 at 12:51:37PM +0000, Anthony Olech wrote: > The touchscreen component driver for the da9052/3 Dialog PMICs > is changed to use device managed memory allocation. > > This results in simpler error paths as failures in the probe() > function do not require explicit calls to free the devm_... > allocated memory. > The allocation functions used in this driver are: > devm_kzalloc() > devm_input_allocate_device() > devm_request_threaded_irq() > > Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn> > Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com> > --- > This patch is relative to linux-next repository tag next-20140109 > > Many thanks to Huqiu Liu who instigated this patch. > > drivers/input/touchscreen/da9052_tsi.c | 62 ++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 32 deletions(-) > > diff --git a/drivers/input/touchscreen/da9052_tsi.c b/drivers/input/touchscreen/da9052_tsi.c > index ab64d58..dcc4cf1 100644 > --- a/drivers/input/touchscreen/da9052_tsi.c > +++ b/drivers/input/touchscreen/da9052_tsi.c > @@ -233,18 +233,30 @@ static int da9052_ts_probe(struct platform_device *pdev) > struct da9052_tsi *tsi; > struct input_dev *input_dev; > int error; > + int pdown_irq; > + int ready_irq; > > da9052 = dev_get_drvdata(pdev->dev.parent); > if (!da9052) > return -EINVAL; > > - tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL); > - input_dev = input_allocate_device(); > - if (!tsi || !input_dev) { > - error = -ENOMEM; > - goto err_free_mem; > - } > + pdown_irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_PENDOWN); > + if (pdown_irq < 0) > + return pdown_irq; ... > > - error = da9052_request_irq(tsi->da9052, DA9052_IRQ_PENDOWN, > - "pendown-irq", da9052_ts_pendwn_irq, tsi); > + error = devm_request_threaded_irq(&pdev->dev, pdown_irq, > + NULL, da9052_ts_pendwn_irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + "pendown-irq", tsi); I am uncomfortable with the touchscreen portion of this driver ignoring the framework of it's MFD core and mixing native IRQ management with the ones done through the core. What would happen if somebody changes da9052_request_irq() to do some thing more than it is doing now so that your open-coded duplicate of the same in da9052_ts_probe() is no longer equivalent? Or da9052_disable_irq() no longer works correctly with IRQs allocated by this sub-module? Thanks.
> -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > Sent: 09 January 2014 19:13 > To: Opensource [Anthony Olech] > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu; > David Dajun Chen > Subject: Re: [PATCH V1] input: use device managed memory in da9052 > touchscreen driver > > Hi Anthony, > > On Thu, Jan 09, 2014 at 12:51:37PM +0000, Anthony Olech wrote: > > The touchscreen component driver for the da9052/3 Dialog PMICs is > > changed to use device managed memory allocation. > > > > This results in simpler error paths as failures in the probe() > > function do not require explicit calls to free the devm_... > > allocated memory. > > The allocation functions used in this driver are: > > devm_kzalloc() > > devm_input_allocate_device() > > devm_request_threaded_irq() > > > > Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn> > > Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com> > > --- > > This patch is relative to linux-next repository tag next-20140109 > > > > Many thanks to Huqiu Liu who instigated this patch. > > > > drivers/input/touchscreen/da9052_tsi.c | 62 ++++++++++++++++----------- > ----- > > 1 file changed, 30 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/input/touchscreen/da9052_tsi.c > > b/drivers/input/touchscreen/da9052_tsi.c > > index ab64d58..dcc4cf1 100644 > > --- a/drivers/input/touchscreen/da9052_tsi.c > > +++ b/drivers/input/touchscreen/da9052_tsi.c > > @@ -233,18 +233,30 @@ static int da9052_ts_probe(struct > platform_device *pdev) > > struct da9052_tsi *tsi; > > struct input_dev *input_dev; > > int error; > > + int pdown_irq; > > + int ready_irq; > > > > da9052 = dev_get_drvdata(pdev->dev.parent); > > if (!da9052) > > return -EINVAL; > > > > - tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL); > > - input_dev = input_allocate_device(); > > - if (!tsi || !input_dev) { > > - error = -ENOMEM; > > - goto err_free_mem; > > - } > > + pdown_irq = regmap_irq_get_virq(da9052->irq_data, > DA9052_IRQ_PENDOWN); > > + if (pdown_irq < 0) > > + return pdown_irq; > ... > > > > - error = da9052_request_irq(tsi->da9052, DA9052_IRQ_PENDOWN, > > - "pendown-irq", da9052_ts_pendwn_irq, tsi); > > + error = devm_request_threaded_irq(&pdev->dev, pdown_irq, > > + NULL, da9052_ts_pendwn_irq, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + "pendown-irq", tsi); > > I am uncomfortable with the touchscreen portion of this driver ignoring the > framework of it's MFD core and mixing native IRQ management with the > ones done through the core. > > What would happen if somebody changes da9052_request_irq() to do some > thing more than it is doing now so that your open-coded duplicate of the > same in da9052_ts_probe() is no longer equivalent? Or > da9052_disable_irq() no longer works correctly with IRQs allocated by this > sub-module? > Thanks. > -- > Dmitry Hi Dmitry, unfortunately the PMIC is a multifunction device and the component drivers come under different subsystem maintainers. Thus it is not possible to do one patch in one go to change them all. Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 10, 2014 at 09:02:10AM +0000, Opensource [Anthony Olech] wrote: > > -----Original Message----- > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > > > I am uncomfortable with the touchscreen portion of this driver ignoring the > > framework of it's MFD core and mixing native IRQ management with the > > ones done through the core. > > > > What would happen if somebody changes da9052_request_irq() to do some > > thing more than it is doing now so that your open-coded duplicate of the > > same in da9052_ts_probe() is no longer equivalent? Or > > da9052_disable_irq() no longer works correctly with IRQs allocated by this > > sub-module? > > Thanks. > > -- > > Dmitry > Hi Dmitry, > > unfortunately the PMIC is a multifunction device and the component > drivers come under different subsystem maintainers. Thus it is not > possible to do one patch in one go to change them all. This could be arranged if you really want to do that. You could post a patch series and have various maintainers ack it and then Samuel could take it all through MFD tree. This is up to you however. The change does not fix any bugs and error unwinding paths in the input portion of the driver are simple enough, so I do not see a very strong reason for moving to managed devices. I said that I would not be opposed to this (given that conversion is solid), but that was never a request form me. Thanks.
diff --git a/drivers/input/touchscreen/da9052_tsi.c b/drivers/input/touchscreen/da9052_tsi.c index ab64d58..dcc4cf1 100644 --- a/drivers/input/touchscreen/da9052_tsi.c +++ b/drivers/input/touchscreen/da9052_tsi.c @@ -233,18 +233,30 @@ static int da9052_ts_probe(struct platform_device *pdev) struct da9052_tsi *tsi; struct input_dev *input_dev; int error; + int pdown_irq; + int ready_irq; da9052 = dev_get_drvdata(pdev->dev.parent); if (!da9052) return -EINVAL; - tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL); - input_dev = input_allocate_device(); - if (!tsi || !input_dev) { - error = -ENOMEM; - goto err_free_mem; - } + pdown_irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_PENDOWN); + if (pdown_irq < 0) + return pdown_irq; + + ready_irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_TSIREADY); + if (ready_irq < 0) + return ready_irq; + + tsi = devm_kzalloc(&pdev->dev, sizeof(struct da9052_tsi), GFP_KERNEL); + if (!tsi) + return -ENOMEM; + + input_dev = devm_input_allocate_device(&pdev->dev); + if (!input_dev) + return -ENOMEM; + platform_set_drvdata(pdev, tsi); tsi->da9052 = da9052; tsi->dev = input_dev; tsi->stopped = true; @@ -274,20 +286,24 @@ static int da9052_ts_probe(struct platform_device *pdev) /* Disable ADC */ da9052_ts_adc_toggle(tsi, false); - error = da9052_request_irq(tsi->da9052, DA9052_IRQ_PENDOWN, - "pendown-irq", da9052_ts_pendwn_irq, tsi); + error = devm_request_threaded_irq(&pdev->dev, pdown_irq, + NULL, da9052_ts_pendwn_irq, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + "pendown-irq", tsi); if (error) { dev_err(tsi->da9052->dev, "Failed to register PENDWN IRQ: %d\n", error); - goto err_free_mem; + return error; } - error = da9052_request_irq(tsi->da9052, DA9052_IRQ_TSIREADY, - "tsiready-irq", da9052_ts_datardy_irq, tsi); + error = devm_request_threaded_irq(&pdev->dev, ready_irq, + NULL, da9052_ts_datardy_irq, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + "tsiready-irq", tsi); if (error) { dev_err(tsi->da9052->dev, "Failed to register TSIRDY IRQ :%d\n", error); - goto err_free_pendwn_irq; + return error; } /* Mask PEN_DOWN and TSI_READY events */ @@ -296,25 +312,13 @@ static int da9052_ts_probe(struct platform_device *pdev) error = da9052_configure_tsi(tsi); if (error) - goto err_free_datardy_irq; + return error; error = input_register_device(tsi->dev); if (error) - goto err_free_datardy_irq; - - platform_set_drvdata(pdev, tsi); + return error; return 0; - -err_free_datardy_irq: - da9052_free_irq(tsi->da9052, DA9052_IRQ_TSIREADY, tsi); -err_free_pendwn_irq: - da9052_free_irq(tsi->da9052, DA9052_IRQ_PENDOWN, tsi); -err_free_mem: - kfree(tsi); - input_free_device(input_dev); - - return error; } static int da9052_ts_remove(struct platform_device *pdev) @@ -323,12 +327,6 @@ static int da9052_ts_remove(struct platform_device *pdev) da9052_reg_write(tsi->da9052, DA9052_LDO9_REG, 0x19); - da9052_free_irq(tsi->da9052, DA9052_IRQ_TSIREADY, tsi); - da9052_free_irq(tsi->da9052, DA9052_IRQ_PENDOWN, tsi); - - input_unregister_device(tsi->dev); - kfree(tsi); - return 0; }
The touchscreen component driver for the da9052/3 Dialog PMICs is changed to use device managed memory allocation. This results in simpler error paths as failures in the probe() function do not require explicit calls to free the devm_... allocated memory. The allocation functions used in this driver are: devm_kzalloc() devm_input_allocate_device() devm_request_threaded_irq() Suggested-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn> Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com> --- This patch is relative to linux-next repository tag next-20140109 Many thanks to Huqiu Liu who instigated this patch. drivers/input/touchscreen/da9052_tsi.c | 62 ++++++++++++++++---------------- 1 file changed, 30 insertions(+), 32 deletions(-)