| Message ID | 20240217164249.921878-16-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
On Sat, Feb 17, 2024 at 04:42:49PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Switching to the _scoped() version removes the need for manual > calling of fwnode_handle_put() in the paths where the code > exits the loop early. In this case that's all in error paths. ... > span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, > tmp[1] / 1000); > - if (span < 0) { > - fwnode_handle_put(child); > + if (span < 0) > return dev_err_probe(dev, -EINVAL, > "output range not valid:[%d %d]\n", > tmp[0], tmp[1]); Last minute observation, should not we return span instead of -EINVAL? (Haven't checked the semantics of the former though.) ... > + if (ret) > return dev_err_probe(dev, -EINVAL, > "failed to set chan settings\n"); Ditto.
On Sat, 2024-02-17 at 16:42 +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Switching to the _scoped() version removes the need for manual > calling of fwnode_handle_put() in the paths where the code > exits the loop early. In this case that's all in error paths. > > Cc: Nuno Sá <nuno.sa@analog.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- Tested-by: Nuno Sa <nuno.sa@analog.com> Reviewed-by: Nuno Sa <nuno.sa@analog.com> > v4: Moved alignment changes back to patch 4. > v3: Tweaked the alignment after comments from Andy. > > drivers/iio/dac/ltc2688.c | 24 ++++++------------------ > 1 file changed, 6 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c > index fc8eb53c65be..b71df03fc13b 100644 > --- a/drivers/iio/dac/ltc2688.c > +++ b/drivers/iio/dac/ltc2688.c > @@ -746,26 +746,21 @@ static int ltc2688_span_lookup(const struct > ltc2688_state *st, int min, int max) > static int ltc2688_channel_config(struct ltc2688_state *st) > { > struct device *dev = &st->spi->dev; > - struct fwnode_handle *child; > u32 reg, clk_input, val, tmp[2]; > int ret, span; > > - device_for_each_child_node(dev, child) { > + device_for_each_child_node_scoped(dev, child) { > struct ltc2688_chan *chan; > > ret = fwnode_property_read_u32(child, "reg", ®); > - if (ret) { > - fwnode_handle_put(child); > + if (ret) > return dev_err_probe(dev, ret, > "Failed to get reg property\n"); > - } > > - if (reg >= LTC2688_DAC_CHANNELS) { > - fwnode_handle_put(child); > + if (reg >= LTC2688_DAC_CHANNELS) > return dev_err_probe(dev, -EINVAL, > "reg bigger than: %d\n", > LTC2688_DAC_CHANNELS); > - } > > val = 0; > chan = &st->channels[reg]; > @@ -786,12 +781,10 @@ static int ltc2688_channel_config(struct ltc2688_state > *st) > if (!ret) { > span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, > tmp[1] / 1000); > - if (span < 0) { > - fwnode_handle_put(child); > + if (span < 0) > return dev_err_probe(dev, -EINVAL, > "output range not > valid:[%d %d]\n", > tmp[0], tmp[1]); > - } > > val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span); > } > @@ -800,17 +793,14 @@ static int ltc2688_channel_config(struct ltc2688_state > *st) > &clk_input); > if (!ret) { > if (clk_input >= LTC2688_CH_TGP_MAX) { > - fwnode_handle_put(child); > return dev_err_probe(dev, -EINVAL, > "toggle-dither-input inv > value(%d)\n", > clk_input); > } > > ret = ltc2688_tgp_clk_setup(st, chan, child, > clk_input); > - if (ret) { > - fwnode_handle_put(child); > + if (ret) > return ret; > - } > > /* > * 0 means software toggle which is the default mode. > @@ -844,11 +834,9 @@ static int ltc2688_channel_config(struct ltc2688_state > *st) > > ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg), > val); > - if (ret) { > - fwnode_handle_put(child); > + if (ret) > return dev_err_probe(dev, -EINVAL, > "failed to set chan > settings\n"); > - } > } > > return 0;
On Mon, 19 Feb 2024 13:48:27 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Feb 17, 2024 at 04:42:49PM +0000, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Switching to the _scoped() version removes the need for manual > > calling of fwnode_handle_put() in the paths where the code > > exits the loop early. In this case that's all in error paths. > > ... > > > span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, > > tmp[1] / 1000); > > - if (span < 0) { > > - fwnode_handle_put(child); > > + if (span < 0) > > return dev_err_probe(dev, -EINVAL, > > "output range not valid:[%d %d]\n", > > tmp[0], tmp[1]); > > Last minute observation, should not we return span instead of -EINVAL? > (Haven't checked the semantics of the former though.) It returns 0 or -EINVAL, so yes we should use span here. > > ... > > > + if (ret) > > return dev_err_probe(dev, -EINVAL, > > "failed to set chan settings\n"); > > Ditto. Definitely on that one. I'll aim to fold those two in whilst picking this up with a note in the patch description. (or I'll incorporate them if I do a v5 for other reasons!) Thanks, Jonathan > >
diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c index fc8eb53c65be..b71df03fc13b 100644 --- a/drivers/iio/dac/ltc2688.c +++ b/drivers/iio/dac/ltc2688.c @@ -746,26 +746,21 @@ static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max) static int ltc2688_channel_config(struct ltc2688_state *st) { struct device *dev = &st->spi->dev; - struct fwnode_handle *child; u32 reg, clk_input, val, tmp[2]; int ret, span; - device_for_each_child_node(dev, child) { + device_for_each_child_node_scoped(dev, child) { struct ltc2688_chan *chan; ret = fwnode_property_read_u32(child, "reg", ®); - if (ret) { - fwnode_handle_put(child); + if (ret) return dev_err_probe(dev, ret, "Failed to get reg property\n"); - } - if (reg >= LTC2688_DAC_CHANNELS) { - fwnode_handle_put(child); + if (reg >= LTC2688_DAC_CHANNELS) return dev_err_probe(dev, -EINVAL, "reg bigger than: %d\n", LTC2688_DAC_CHANNELS); - } val = 0; chan = &st->channels[reg]; @@ -786,12 +781,10 @@ static int ltc2688_channel_config(struct ltc2688_state *st) if (!ret) { span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, tmp[1] / 1000); - if (span < 0) { - fwnode_handle_put(child); + if (span < 0) return dev_err_probe(dev, -EINVAL, "output range not valid:[%d %d]\n", tmp[0], tmp[1]); - } val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span); } @@ -800,17 +793,14 @@ static int ltc2688_channel_config(struct ltc2688_state *st) &clk_input); if (!ret) { if (clk_input >= LTC2688_CH_TGP_MAX) { - fwnode_handle_put(child); return dev_err_probe(dev, -EINVAL, "toggle-dither-input inv value(%d)\n", clk_input); } ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input); - if (ret) { - fwnode_handle_put(child); + if (ret) return ret; - } /* * 0 means software toggle which is the default mode. @@ -844,11 +834,9 @@ static int ltc2688_channel_config(struct ltc2688_state *st) ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg), val); - if (ret) { - fwnode_handle_put(child); + if (ret) return dev_err_probe(dev, -EINVAL, "failed to set chan settings\n"); - } } return 0;