Re: [PATCH] iio: adc: palmas: Take probe fully device managed.

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 19 Mar 2023 15:21:06 +0100
Patrik Dahlström <risca@dalakolonin.se> 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.

Thanks

Jonathan


> 
> On Sat, Mar 18, 2023 at 04:30:39PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > 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@dalakolonin.se/
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> > ---
> >  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
> >