Re: [PATCH v3 1/2] iio: adc: vf610_adc: use devm_* and dev_err_probe() to simple code

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 8 Nov 2024 14:12:29 -0500
Frank Li <Frank.li@nxp.com> wrote:

> On Fri, Nov 08, 2024 at 07:13:20AM +0100, Christophe JAILLET wrote:
> > Le 07/11/2024 à 20:49, Frank Li a écrit :  
> > > On Thu, Nov 07, 2024 at 08:38:20PM +0100, Christophe JAILLET wrote:  
> > > > Le 07/11/2024 à 20:18, Frank Li a écrit :  
> > > > > Use devm_* and dev_err_probe() simplify probe function and remove
> > > > > vf610_adc_remove(). Change type of 'vref_uv' to int because
> > > > > regulator_get_voltage() return type is int.
> > > > >
> > > > > Reviewed-by: Haibo Chen <haibo.chen-3arQi8VN3Tc-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> > > > > Signed-off-by: Frank Li <Frank.Li-3arQi8VN3Tc-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> > > > > ---  
> >
> > ...
> >  
> > > >  
> > > > >    	info->vref = devm_regulator_get(&pdev->dev, "vref");  
> > > >
> > > > With the change to devm_regulator_get_enable_read_voltage(), is it still
> > > > needed?  
> > >
> > > Suspend function need vref to disable regulator.  
> >
> > Ok.
> >
> > But why switch to devm_regulator_get_enable_read_voltage() then?
> > Shouldn't keeping regulator_get_voltage() be enough and simpler?  
> 
> Avoid goto err after devm_regulator_get_enable_read_voltage(), if use
> regulator_enable(), it needs regulator_disable() in err handle branch after
> it.

Don't get the same regulator twice - that works but is likely to be error
prone in the long run and is a bit too subtle.

This isn't an appropriate use of devm_regulator_get_enable_read_voltage()
Instead just use a local callback and
devm_add_action_or_reset() to disable the regulator in the devm tear down path.

Jonathan

> 
> Frank
> 
> >
> > CJ
> >  
> > >  
> > > >
> > > > CJ
> > > >  
> > > > >    	if (IS_ERR(info->vref))
> > > > >    		return PTR_ERR(info->vref);
> > > > > -	ret = regulator_enable(info->vref);
> > > > > -	if (ret)
> > > > > -		return ret;
> > > > > -
> > > > > -	info->vref_uv = regulator_get_voltage(info->vref);
> > > > > +	info->vref_uv = devm_regulator_get_enable_read_voltage(&pdev->dev, "vref");
> > > > > +	if (info->vref_uv < 0)
> > > > > +		return info->vref_uv;
> > > > >    	device_property_read_u32_array(dev, "fsl,adck-max-frequency", info->max_adck_rate, 3);  
> > > >
> > > > ...
> > > >  
> > >
> > >  
> >