Re: [PATCH 3/6] iio: temperature: ltc2983: convert to dev_err_probe()

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, Nuno Sa <nuno.sa@analog.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH 3/6] iio: temperature: ltc2983: convert to dev_err_probe()
Date: Mon, 26 Feb 2024 09:41:06 +0100	[thread overview]
Message-ID: <7ae2a3cf0fefec976de2b77d02d970efcea45abc.camel@gmail.com> (raw)
In-Reply-To: <20240224184341.791e5263@jic23-huawei>

On Sat, 2024-02-24 at 18:44 +0000, Jonathan Cameron wrote:
> On Thu, 22 Feb 2024 13:55:54 +0100
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Use dev_err_probe() in the probe() path. While at it, made some simple
> > improvements:
> >  * Declare a struct device *dev helper. This also makes the style more
> >    consistent (some places the helper was used and not in other places);
> >  * Explicitly included the err.h and errno.h headers;
> >  * Removed an useless else if().
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> Hmm. Up to you whether you rebase on top of the device_for_each_child_node_scoped()
> patch - mostly depends if you give the new version a reviewed by or not!
> 

Already gave my tag. I see you're already doing some dev_err_probe() in there so I
can just add the missing ones after rebasing. 

> If they land in the other order I can fix it up whilst applying.
> After that series is in place though the number of places this will do
> 
> 	dev_err_probe(dev, ret, "message\n");
> 	return ERR_PTR(ret);
> 
> Makes me wonder whether
> 	return ERR_PTR(dev_err_probe(dev, ret, "message\n")); is
> too ugly or not?
> 

Yeah, IMO, that's a bit ugly :). We may making it slightly better for the driver by
adding a macro for the above.

> Maybe we need a dev_err_probe_ret_ptr() but that's also ugly.
> 

I almost send a patch for that as well. I think this driver is indeed a good
candidate for introducing an helper like this. My thought on naming was something
like dev_errp_probe() though.

> One comment inline which is why I didn't just pick this up today and send
> a new version of this patch in my series.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/temperature/ltc2983.c | 190 ++++++++++++++++++++------------------
> >  1 file changed, 98 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/ltc2983.c
> > b/drivers/iio/temperature/ltc2983.c
> > index 23f2d43fc040..4b096aa3fbd8 100644
> > --- a/drivers/iio/temperature/ltc2983.c
> > +++ b/drivers/iio/temperature/ltc2983.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/bitfield.h>
> >  #include <linux/completion.h>
> >  #include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> >  #include <linux/kernel.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/interrupt.h>
> > @@ -656,11 +658,12 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child,
> > struct ltc2983_data
> >  			 const struct ltc2983_sensor *sensor)
> >  {
> >  	struct ltc2983_thermocouple *thermo;
> > +	struct device *dev = &st->spi->dev;
> >  	struct fwnode_handle *ref;
> >  	u32 oc_current;
> >  	int ret;
> >  
> > -	thermo = devm_kzalloc(&st->spi->dev, sizeof(*thermo), GFP_KERNEL);
> > +	thermo = devm_kzalloc(dev, sizeof(*thermo), GFP_KERNEL);
> >  	if (!thermo)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > @@ -687,8 +690,9 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child,
> > struct ltc2983_data
> >  					LTC2983_THERMOCOUPLE_OC_CURR(3);
> >  			break;
> >  		default:
> > -			dev_err(&st->spi->dev,
> > -				"Invalid open circuit current:%u", oc_current);
> > +			dev_err_probe(dev, -EINVAL,
> > +				      "Invalid open circuit current:%u",
> > +				      oc_current);
> Hmm. I'm in two minds on these. 
> We don't get the advantage of return dev_error_probe() and I'm not seeing these
> hitting EPROBE_DEFER so getting the debug advantages.

I would argue on logging (output) consistency (I think Andy pointed that in some
other series - I think my IIO backend stuff).

- Nuno Sá