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@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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á