Message-ID: <d7576a0bb9a8d5326d77ae434131540b4359bd2a.camel@gmail.com>
Date: Thu, 16 Oct 2025 16:26:28 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Herve Codina <herve.codina@...tlin.com>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>, Jonathan Cameron	
 <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá	 <nuno.sa@...log.com>, Andy Shevchenko
 <andy@...nel.org>, Rob Herring	 <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley	 <conor+dt@...nel.org>, Geert
 Uytterhoeven <geert+renesas@...der.be>, Magnus Damm
 <magnus.damm@...il.com>, Liam Girdwood <lgirdwood@...il.com>, Mark Brown	
 <broonie@...nel.org>, linux-iio@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Pascal Eberhard <pascal.eberhard@...com>, 
 Miquel Raynal <miquel.raynal@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

On Thu, 2025-10-16 at 16:02 +0200, Herve Codina wrote:
> Hi Nuno,
> 
> On Thu, 16 Oct 2025 10:24:36 +0100
> Nuno Sá <noname.nuno@...il.com> wrote:
> 
> > On Wed, 2025-10-15 at 21:14 +0200, Herve Codina wrote:
> > > Hi Nuno,
> > > 
> > > On Wed, 15 Oct 2025 16:21:09 +0100
> > > Nuno Sá <noname.nuno@...il.com> wrote:
> > > 
> > > ...  
> > > >   
> > > > > +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> > > > > +	if (ret)
> > > > > +		goto poweroff_adc_core0;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzn1_adc->pclk);
> > > > > +	if (ret)
> > > > > +		goto poweroff_adc_core1;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> > > > > +	if (ret)
> > > > > +		goto disable_pclk;
> > > > > +
> > > > > +	ret = rzn1_adc_power(rzn1_adc, true);
> > > > > +	if (ret)
> > > > > +		goto disable_adc_clk;    
> > > > 
> > > > Can we use devm_actions() on the above to avoid the complex error path
> > > > plus
> > > > the
> > > > .remove() callback?  
> > > 
> > > rzn1_adc_enable() is used by the driver pm_runtime_resume() function.
> > > 
> > > I don't think that devm_add_actions_or_reset() will help here.
> > > 
> > > In my understanding, devm_* functions are use to perform some operations
> > > automatically on device removal.
> > > 
> > > The purpose of the error path here is to restore a correct state if
> > > rzn1_adc_enable() failed when it is called from pm_runtime_resume().
> > > 
> > > In that case no device removal is involved to trig any action set by
> > > devm_add_actions_or_reset().
> > > 
> > > Maybe I am wrong. Did I miss something?  
> > 
> > Nope, I see now what's your intent.
> 
> Ok, no change planned for the next iteration related to this error path.
> 
> > 
> > >   
> > > >   
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +disable_adc_clk:
> > > > > +	clk_disable_unprepare(rzn1_adc->adc_clk);
> > > > > +disable_pclk:
> > > > > +	clk_disable_unprepare(rzn1_adc->pclk);
> > > > > +poweroff_adc_core1:
> > > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> > > > > +poweroff_adc_core0:
> > > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> > > > > +	return ret;
> > > > > +}
> > > > > +  
> > > 
> > > ...
> > >   
> > > > > +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > > > +					 struct iio_dev *indio_dev)
> > > > > +{
> > > > > +	int adc_used;
> > > > > +
> > > > > +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > > > +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > > > +
> > > > > +	switch (adc_used) {
> > > > > +	case 0x01:
> > > > > +		indio_dev->channels = rzn1_adc1_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc1_channels);
> > > > > +		return 0;
> > > > > +	case 0x02:
> > > > > +		indio_dev->channels = rzn1_adc2_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc2_channels);
> > > > > +		return 0;
> > > > > +	case 0x03:
> > > > > +		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > > > +		return 0;
> > > > > +	default:
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC
> > > > > core
> > > > > used\n");
> > > > > +	return -ENODEV;    
> > > > 
> > > > dev_err_probe()?  
> > > 
> > > Why? the error returned is a well known value: -ENODEV.
> > > 
> > > dev_err_probe() should be involved when -EPROBE_DEFER is a potential error
> > > code.
> > > 
> > > IMHO, dev_err() here is correct.  
> > 
> > If I'm not missing nothing this function is called during probe so I do
> > think
> > dev_err_probe() should be used. Not only unifies logging style during probe
> > it
> > also has the small benefit of doing:
> > 
> > return dev_err_probe(...) saving a line of code.
> > 
> > You can see that, at least in IIO, we even have some patches just converting
> > drivers probe() to use dev_err_probe().
> 
> Right, I will use dev_err_probe() in the next iteration.
> 
> > 
> > >   
> > > >   
> > > > > +}
> > > > > +
> > > > > +static int rzn1_adc_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct iio_dev *indio_dev;
> > > > > +	struct rzn1_adc *rzn1_adc;
> > > > > +	int ret;
> > > > > +
> > > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> > > > > +	if (!indio_dev)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	rzn1_adc = iio_priv(indio_dev);
> > > > > +	rzn1_adc->dev = dev;
> > > > > +	mutex_init(&rzn1_adc->lock);    
> > > > 
> > > > devm_mutex_init()  
> > > 
> > > Yes, I will update in the next iteration.
> > >   
> > > >   
> > > > > +
> > > > > +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > > > +	if (IS_ERR(rzn1_adc->regs))
> > > > > +		return PTR_ERR(rzn1_adc->regs);
> > > > > +
> > > > > +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> > > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > > "Failed to
> > > > > get pclk\n");
> > > > > +
> > > > > +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> > > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > > "Failed to
> > > > > get adc-clk\n");
> > > > > +
> > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > > > adc_core[0],  
> > > > > +					   "adc1-avdd", "adc1-vref");
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > > > adc_core[1],  
> > > > > +					   "adc2-avdd", "adc2-vref");
> > > > > +	if (ret)
> > > > > +		return ret;    
> > > > 
> > > > Hmm, is avdd really an optional regulator? I mean can the ADC power up
> > > > at
> > > > all
> > > > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > > > properly
> > > > scale the sample without it.  
> > > 
> > > Where do you see that avdd is an optional regulator?  
> > 
> > You are using devm_regulator_get_optional(). That's for optional regulators.
> > 
> 
> Indeed I use devm_regulator_get_optional().
> 
> We have two similar function to get regulators:
> - devm_regulator_get() and
> - devm_regulator_get_optional().
> 
> devm_regulator_get() returns a dummy regulator if the regulator is not
> described in the device-tree. The calling code has no way to known if
> the regulator was present or not.

Yeah because it's mandatory and the part cannot work without power :). So we
should not be allowed to operate without a regulator.

> 
> On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> regulator is not described.
> 
> That's pretty confusing but it is the reality.
> 
> I use devm_regulator_get_optional() but check for -ENODEV to see if the
> regulator is provided or not.
> 
> In order to use the ADC core (is_used flag), I need both the AVDD and the
> VREF regulator available.

And that is why I don't get why are we allowed to proceed if there's no
regulators? That seems wrong to me. 

So I think the regulators should be mandatory in the bindings and a dummy
regulator should also not be allowed in this case because that should get you 
-EINVAL when calling regulator_get_voltage().

> 
> > >   
> > > > 
> > > > Also, can't we have getting and enabling the regulator together? Then,
> > > > we
> > > > could
> > > > use some of the modern helpers to simplify the code (ok I see you use
> > > > them
> > > > in
> > > > the PM callbacks).  
> > > 
> > > Yes, I rely on PM callbacks to handle those regulators.
> > >   
> > > >   
> > > > > +
> > > > > +	platform_set_drvdata(pdev, indio_dev);
> > > > > +
> > > > > +	indio_dev->name = dev_name(dev);    
> > > > 
> > > > dev_name() should not be used for the above. It's typically the part
> > > > name so
> > > > I
> > > > guess in here "rzn1-adc" would be the appropriate one.  
> > > 
> > > I thought it was more related to the instance and so having a different
> > > name
> > > for each instance was better.
> > > 
> > > Some other IIO drivers use dev_name() here.
> > > 
> > > But well, if you confirm that a fixed string should be used and so all
> > > instances have the same string, no problem, I will update my indio_dev-
> > > >name.  
> > 
> > It is a fixed string, typically the part name. David Lechner not that long
> > ago
> > actually sent some patch or documented somewhere why not to use dev_name().
> > To
> > identify different instances we have a 'label' property.
> 
> Right, I will set indio_dev->name to the "rzn1-adc" fixed string.
> 
> > 
> > >   
> > > >   
> > > > > +	indio_dev->info = &rzn1_adc_info;
> > > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_enable(rzn1_adc);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > > > > +	pm_runtime_use_autosuspend(dev);
> > > > > +	pm_runtime_get_noresume(dev);
> > > > > +	pm_runtime_set_active(dev);
> > > > > +	pm_runtime_enable(dev);    
> > > > 
> > > > There's a devm_pm_runtime_enable() API now.  
> > > 
> > > Will look to use it in the next iteration.
> > >   
> > > >   
> > > > > +
> > > > > +	ret = devm_iio_device_register(dev, indio_dev);
> > > > > +	if (ret)
> > > > > +		goto disable;
> > > > > +
> > > > > +	pm_runtime_mark_last_busy(dev);
> > > > > +	pm_runtime_put_autosuspend(dev);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +disable:
> > > > > +	pm_runtime_disable(dev);
> > > > > +	pm_runtime_put_noidle(dev);
> > > > > +	pm_runtime_set_suspended(dev);
> > > > > +	pm_runtime_dont_use_autosuspend(dev);
> > > > > +
> > > > > +	rzn1_adc_disable(rzn1_adc);
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static void rzn1_adc_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > > > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > > > > +
> > > > > +	pm_runtime_disable(rzn1_adc->dev);
> > > > > +	pm_runtime_set_suspended(rzn1_adc->dev);
> > > > > +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> > > > > +
> > > > > +	rzn1_adc_disable(rzn1_adc);
> > > > > +}    
> > > > 
> > > > I'm fairly confident we can sanely go without .remove().  
> > > 
> > > Will see what I can be do for the next iteration.
> > > 
> > > Maybe I will ask some questions if I need some clarification around
> > > pm_runtime but let me first try to go further in that direction.  
> > 
> > Yeah, maybe you can come up with something but given how you use pm to
> > enable/disable stuff I'm also not sure the above is easily doable.
> > 
> 
> Hum, do you think it's worth a try?

Not sure. But it got me thinking about all this handling in the pm runtime
routines. So if in the resume() call you fail at some point and then disable
stuff in your return path and then we get an unbind won't things (clocks and
regulators) be unbalanced leading to splats? In fact by just looking at the
unbind path [1] I can see:

1. We call pm_runtime_get_sync(dev) which can fail;
2. Later on we call pm_runtime_put_sync(dev).

Not really sure if there's special handling in the pm core to be aware that
resuming failed (the refcount seems to be incremented [2] before resuming so...)

Maybe I would keep it simple and get and enable clocks/regulators during probe
and only care of rzn1_adc_power() in the runtime routines. My 2 cents.

[1]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/dd.c#L1249
[2]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/power/runtime.c#L1189

- Nuno Sá

Powered by blists - more mailing lists