linux-kernel - Re: [PATCH v2 2/4] iio: temperature: ltc2983: convert to dev_err

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <adb68427a410afc07733ae0f05af1cbefc57a288.camel@gmail.com>
Date: Thu, 02 May 2024 13:41:55 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, nuno.sa@...log.com
Cc: Petr Mladek <pmladek@...e.com>, Chris Down <chris@...isdown.name>, John
 Ogness <john.ogness@...utronix.de>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>,  Jonathan Cameron <jic23@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, Olivier Moysan
 <olivier.moysan@...s.st.com>, Andi Shyti <andi.shyti@...nel.org>, Jyoti
 Bhayana <jbhayana@...gle.com>, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 2/4] iio: temperature: ltc2983: convert to
 dev_err_probe()

On Tue, 2024-04-23 at 18:42 +0300, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 05:20:31PM +0200, Nuno Sa via B4 Relay wrote:
> > From: Nuno Sa <nuno.sa@...log.com>
> > 
> > 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();
> >  * Removed some unnecessary line breaks.
> 
> ...
> 
> >  	/* Check space on the table. */
> >  	if (st->custom_table_size + new_custom->size >
> > -	    (LTC2983_CUST_SENS_TBL_END_REG -
> > -	     LTC2983_CUST_SENS_TBL_START_REG) + 1) {
> 
> > +	    (LTC2983_CUST_SENS_TBL_END_REG - LTC2983_CUST_SENS_TBL_START_REG) +
> > 1)
> 
> Semi-unrelated change?


Yeah, indeed. One of those cases where the old limit does hurt readability (IMO)

> 
> ...
> 
> > +		return dev_err_ptr_probe(dev, -EINVAL,
> > +				      "Invalid chann:%d for differential
> > thermocouple",
> 
> While at it, add missing \n.

Will do for all the places...

> 
> > +				      sensor->chan);
> 
> ...
> 
> > +		return dev_err_cast_probe(dev, ref,
> > +					  "Property adi,rsense-handle missing or
> > invalid");
> 
> Ditto.
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid number of wires:%u\n",
> > +					      n_wires);
> 
> Can be compressed in terms of LoCs?
> 
> ...
> 
> > +				return dev_err_ptr_probe(dev, -EINVAL,
> > +						      "Rotation not allowed for
> > 2/3 Wire RTDs");
> 
> \n
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid rsense chann:%d to use in
> > kelvin rsense",
> > +					      rtd->r_sense_chan);
> 
> Ditto.
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid chann:%d for the rtd
> > config",
> 
> Ditto.
> 
> > +					      sensor->chan);
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid chann:%d for RTD",
> 
> Ditto.
> 
> > +					      sensor->chan);
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid value for excitation
> > current(%u)",
> 
> Ditto.
> 
> > +					      excitation_current);
> 
> ...
> 
> > +	if (IS_ERR(ref))
> > +		return dev_err_cast_probe(dev, ref,
> > +					  "Property adi,rsense-handle missing or
> > invalid");
> 
> Ditto.
> 
> ...
> 
> > +		return dev_err_ptr_probe(dev, -EINVAL,
> > +				      "Invalid chann:%d for differential
> > thermistor",
> > +				      sensor->chan);
> 
> 
> Ditto.
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid value for excitation
> > current(%u)",
> > +					      excitation_current);
> 
> Ditto.
> 
> ...
> 
> > +		return dev_err_ptr_probe(dev, -EINVAL,
> > +				      "Invalid chann:%d for differential
> > thermistor",
> > +				      sensor->chan);
> 
> Ditto.
> 
> ...
> 
> > +			return dev_err_ptr_probe(dev, -EINVAL,
> > +					      "Invalid value for excitation
> > current(%u)",
> > +					      excitation_current);
> 
> Ditto.
> 
> ...
> 
> > +		return dev_err_ptr_probe(dev, -EINVAL,
> > +				      "Invalid chann:%d for r_sense",
> > +				      sensor->chan);
> 
> Ditto.
> 
> ...
> 
> > +	if (!st->num_channels)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "At least one channel must be given!");
> 
> Ditto.
> 
> ...
> 
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "EEPROM command failed: 0x%02X\n", val);
> 
> One line?
> 
> ...
> 
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to initialize regmap\n");
> 
> Wondering about Andi's proposal in conjunction with %pe to be in use
> 
> 		return dev_???(dev, st->regmap, "Failed to initialize regmap\n");
> 
> where it returns an int and uses const void * as an error pointer for %pe.

Yeah, I would like to avoid including that variation in this series (unless everyone
agrees and requires it now). We already have tons of cases where we do
dev_err_probe(dev, PTR_ERR(), ...). Do we want to change all of them or not having
more? Personally, I'm not seeing as a big deal to have to do the PTR_ERR(). Yes,
internally we will go back to ERR_PTR() but still...

> 
> 
> 
> > -	st->iio_chan = devm_kzalloc(&spi->dev,
> > +	st->iio_chan = devm_kzalloc(dev,
> >  				    st->iio_channels * sizeof(*st->iio_chan),
> >  				    GFP_KERNEL);
> 
> Separate change to devm_kzalloc() before this patch?
> In that patch you may also introduce a temporary struct device *dev.
> 

If the introduction of the temporary struct device *dev is too much to be included in
here I may just remove it and send a patch afterwards.. (note I'm adding more
temporary *dev in other places to be consistent throughout the driver.
>