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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 19 Mar 2023 15:36:21 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> 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.
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@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
> > >     
>