On Sun, 19 Mar 2023 15:36:21 +0000 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Sun, 19 Mar 2023 15:21:06 +0100 > Patrik Dahlström <risca@xxxxxxxxxxxxxx> wrote: > > > The changes look good and I've tested it on Omap5-uevm board: > > * module loads and unloads without issues > > * I'm able to read ADC values > > * the values change when I turn my potentiometer > > > > Feel free to add the relevant tags, e.g. Tested-by or Reviewed-by. I'm > > still new to the kernel development process. > Hi Patrik, > > Both make sense here given your comments. You tried it so Tested-by > and you said it looks good which is Reviewed-by > > I failed to cc the original author of this driver though, so +CC HNS for that > and this will have to wait for your fix to be available in upstream so it > will take a while. > > If you are sending additional patches on top of this and your patch, > state that in the cover letter for those additional patches as I'll probably > forget otherwise and wonder why they don't apply. Hi Patrik, Applied this version because I wanted the link that automatically includes to include your TB and RB comment above. At the moment I'm cheating a bit and using Greg's char-misc-testing tree because I want to get this and your series on top of it into the hands of 0-day asap with the intent to sneak out a last minute pull request on Friday or Saturday. Jonathan > > Thanks > > Jonathan > > > > > > On Sat, Mar 18, 2023 at 04:30:39PM +0000, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > > > > Review of a recent fix highlighted that this driver could be trivially > > > converted to be entirely devm managed. > > > > > > That fix should be applied to resolve the fix in a fashion easy to back port > > > even though this change removes the relevant code. > > > > > > [1] https://patchwork.kernel.org/project/linux-iio/patch/20230313205029.1881745-1-risca@xxxxxxxxxxxxxx/ > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > Cc: Signed-off-by: Patrik Dahlström <risca@xxxxxxxxxxxxxx> > > > --- > > > drivers/iio/adc/palmas_gpadc.c | 110 +++++++++++++-------------------- > > > 1 file changed, 42 insertions(+), 68 deletions(-) > > > > > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > > > index 849a697a467e..2921186458e0 100644 > > > --- a/drivers/iio/adc/palmas_gpadc.c > > > +++ b/drivers/iio/adc/palmas_gpadc.c > > > @@ -493,6 +493,11 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev, > > > return 0; > > > } > > > > > > +static void palmas_disable_wakeup(void *dev) > > > +{ > > > + device_wakeup_disable(dev); > > > +} > > > + > > > static int palmas_gpadc_probe(struct platform_device *pdev) > > > { > > > struct palmas_gpadc *adc; > > > @@ -532,36 +537,30 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > > > > adc->auto_conversion_period = gpadc_pdata->auto_conversion_period_ms; > > > adc->irq = palmas_irq_get_virq(adc->palmas, PALMAS_GPADC_EOC_SW_IRQ); > > > - if (adc->irq < 0) { > > > - dev_err(adc->dev, > > > - "get virq failed: %d\n", adc->irq); > > > - ret = adc->irq; > > > - goto out; > > > - } > > > - ret = request_threaded_irq(adc->irq, NULL, > > > - palmas_gpadc_irq, > > > - IRQF_ONESHOT, dev_name(adc->dev), > > > - adc); > > > - if (ret < 0) { > > > - dev_err(adc->dev, > > > - "request irq %d failed: %d\n", adc->irq, ret); > > > - goto out; > > > - } > > > + if (adc->irq < 0) > > > + return dev_err_probe(adc->dev, adc->irq, "get virq failed\n"); > > > + > > > + ret = devm_request_threaded_irq(&pdev->dev, adc->irq, NULL, > > > + palmas_gpadc_irq, > > > + IRQF_ONESHOT, dev_name(adc->dev), > > > + adc); > > > + if (ret < 0) > > > + return dev_err_probe(adc->dev, ret, > > > + "request irq %d failed\n", adc->irq); > > > > > > if (gpadc_pdata->adc_wakeup1_data) { > > > memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data, > > > sizeof(adc->wakeup1_data)); > > > adc->wakeup1_enable = true; > > > adc->irq_auto_0 = platform_get_irq(pdev, 1); > > > - ret = request_threaded_irq(adc->irq_auto_0, NULL, > > > - palmas_gpadc_irq_auto, > > > - IRQF_ONESHOT, > > > - "palmas-adc-auto-0", adc); > > > - if (ret < 0) { > > > - dev_err(adc->dev, "request auto0 irq %d failed: %d\n", > > > - adc->irq_auto_0, ret); > > > - goto out_irq_free; > > > - } > > > + ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, > > > + NULL, palmas_gpadc_irq_auto, > > > + IRQF_ONESHOT, > > > + "palmas-adc-auto-0", adc); > > > + if (ret < 0) > > > + return dev_err_probe(adc->dev, ret, > > > + "request auto0 irq %d failed\n", > > > + adc->irq_auto_0); > > > } > > > > > > if (gpadc_pdata->adc_wakeup2_data) { > > > @@ -569,15 +568,14 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > sizeof(adc->wakeup2_data)); > > > adc->wakeup2_enable = true; > > > adc->irq_auto_1 = platform_get_irq(pdev, 2); > > > - ret = request_threaded_irq(adc->irq_auto_1, NULL, > > > - palmas_gpadc_irq_auto, > > > - IRQF_ONESHOT, > > > - "palmas-adc-auto-1", adc); > > > - if (ret < 0) { > > > - dev_err(adc->dev, "request auto1 irq %d failed: %d\n", > > > - adc->irq_auto_1, ret); > > > - goto out_irq_auto0_free; > > > - } > > > + ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, > > > + NULL, palmas_gpadc_irq_auto, > > > + IRQF_ONESHOT, > > > + "palmas-adc-auto-1", adc); > > > + if (ret < 0) > > > + return dev_err_probe(adc->dev, ret, > > > + "request auto1 irq %d failed\n", > > > + adc->irq_auto_1); > > > } > > > > > > /* set the current source 0 (value 0/5/15/20 uA => 0..3) */ > > > @@ -608,11 +606,10 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > indio_dev->channels = palmas_gpadc_iio_channel; > > > indio_dev->num_channels = ARRAY_SIZE(palmas_gpadc_iio_channel); > > > > > > - ret = iio_device_register(indio_dev); > > > - if (ret < 0) { > > > - dev_err(adc->dev, "iio_device_register() failed: %d\n", ret); > > > - goto out_irq_auto1_free; > > > - } > > > + ret = devm_iio_device_register(&pdev->dev, indio_dev); > > > + if (ret < 0) > > > + return dev_err_probe(adc->dev, ret, > > > + "iio_device_register() failed\n"); > > > > > > device_set_wakeup_capable(&pdev->dev, 1); > > > for (i = 0; i < PALMAS_ADC_CH_MAX; i++) { > > > @@ -620,36 +617,14 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > palmas_gpadc_calibrate(adc, i); > > > } > > > > > > - if (adc->wakeup1_enable || adc->wakeup2_enable) > > > + if (adc->wakeup1_enable || adc->wakeup2_enable) { > > > device_wakeup_enable(&pdev->dev); > > > - > > > - return 0; > > > - > > > -out_irq_auto1_free: > > > - if (gpadc_pdata->adc_wakeup2_data) > > > - free_irq(adc->irq_auto_1, adc); > > > -out_irq_auto0_free: > > > - if (gpadc_pdata->adc_wakeup1_data) > > > - free_irq(adc->irq_auto_0, adc); > > > -out_irq_free: > > > - free_irq(adc->irq, adc); > > > -out: > > > - return ret; > > > -} > > > - > > > -static int palmas_gpadc_remove(struct platform_device *pdev) > > > -{ > > > - struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); > > > - struct palmas_gpadc *adc = iio_priv(indio_dev); > > > - > > > - if (adc->wakeup1_enable || adc->wakeup2_enable) > > > - device_wakeup_disable(&pdev->dev); > > > - iio_device_unregister(indio_dev); > > > - free_irq(adc->irq, adc); > > > - if (adc->wakeup1_enable) > > > - free_irq(adc->irq_auto_0, adc); > > > - if (adc->wakeup2_enable) > > > - free_irq(adc->irq_auto_1, adc); > > > + ret = devm_add_action_or_reset(&pdev->dev, > > > + palmas_disable_wakeup, > > > + &pdev->dev); > > > + if (ret) > > > + return ret; > > > + } > > > > > > return 0; > > > } > > > @@ -834,7 +809,6 @@ MODULE_DEVICE_TABLE(of, of_palmas_gpadc_match_tbl); > > > > > > static struct platform_driver palmas_gpadc_driver = { > > > .probe = palmas_gpadc_probe, > > > - .remove = palmas_gpadc_remove, > > > .driver = { > > > .name = MOD_NAME, > > > .pm = pm_sleep_ptr(&palmas_pm_ops), > > > -- > > > 2.40.0 > > > >
