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á
