| Message ID | 20240217164249.921878-10-jic23@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
On Sat, Feb 17, 2024 at 04:42:43PM +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. > > Note this includes a probable fix as in one path an error message was > printed with ret == 0. > > Took advantage of dev_err_probe() to futher simplify things given no > longer a need for the goto err. ... > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; I believe with struct device *dev = &indio_dev->dev; you can make the below neater. Also see some side notes. > - struct fwnode_handle *child; > const char *name; > int val, scan_index = 0, ret; > bool differential; > u32 vin[2]; ... > if (!ret) { Not a fan of this pattern, below we have two different patterns for the cases like this :-( > + if (strlen(name) >= STM32_ADC_CH_SZ) > + return dev_err_probe(&indio_dev->dev, -EINVAL, > + "Label %s exceeds %d characters\n", > + name, STM32_ADC_CH_SZ); > + > strscpy(adc->chan_name[val], name, STM32_ADC_CH_SZ); > ret = stm32_adc_populate_int_ch(indio_dev, name, val); > if (ret == -ENOENT) > continue; > else if (ret) This 'else' is redundant. > + return ret; > + } else if (ret != -EINVAL) This also... > + return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n"); ...if you first do like if (ret && ret != -EINVAL) return dev_err_probe(...); if (!ret) { Another option if (ret) { if (ret != -EINVAL) return dev_err_probe(...); } else { ... > differential = false; > ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2); ARRAY_SIZE()? > /* diff-channels is optional */ ... > if (!ret) { > + if (vin[0] != val || vin[1] >= adc_info->max_channels) > + return dev_err_probe(&indio_dev->dev, -EINVAL, > + "Invalid channel in%d-in%d\n", > + vin[0], vin[1]); > } else if (ret != -EINVAL) { > - dev_err(&indio_dev->dev, "Invalid diff-channels property %d\n", ret); > - goto err; > + return dev_err_probe(&indio_dev->dev, ret, > + "Invalid diff-channels property\n"); > } As per above?
On Mon, 19 Feb 2024 13:45:03 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Feb 17, 2024 at 04:42:43PM +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. > > > > Note this includes a probable fix as in one path an error message was > > printed with ret == 0. > > > > Took advantage of dev_err_probe() to futher simplify things given no > > longer a need for the goto err. > > ... > > > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > > I believe with > > struct device *dev = &indio_dev->dev; > > you can make the below neater. Agreed. Given the users of indio_dev are all the error messages I'm touching anyway, this is in scope for this patch I think. > Also see some side notes. > > > - struct fwnode_handle *child; > > const char *name; > > int val, scan_index = 0, ret; > > bool differential; > > u32 vin[2]; > > ... > > > if (!ret) { > > Not a fan of this pattern, below we have two different patterns for the cases > like this :-( Likewise not a fan. Tidying this up is unrelated enough though that I won't do it in this series. > > > + if (strlen(name) >= STM32_ADC_CH_SZ) > > + return dev_err_probe(&indio_dev->dev, -EINVAL, > > + "Label %s exceeds %d characters\n", > > + name, STM32_ADC_CH_SZ); > > + > > strscpy(adc->chan_name[val], name, STM32_ADC_CH_SZ); > > ret = stm32_adc_populate_int_ch(indio_dev, name, val); > > if (ret == -ENOENT) > > continue; > > else if (ret) > > > This 'else' is redundant. > > > + return ret; > > + } else if (ret != -EINVAL) > I shouldn't have dropped the bracket here, so I'll put that back for now. > This also... > > > + return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n"); > > ...if you first do like > > if (ret && ret != -EINVAL) > return dev_err_probe(...); > if (!ret) { > > Another option > > if (ret) { > if (ret != -EINVAL) > return dev_err_probe(...); > } else { > > ... > > > differential = false; > > ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2); > > ARRAY_SIZE()? > > > /* diff-channels is optional */ > > ... > > > if (!ret) { > > + if (vin[0] != val || vin[1] >= adc_info->max_channels) > > + return dev_err_probe(&indio_dev->dev, -EINVAL, > > + "Invalid channel in%d-in%d\n", > > + vin[0], vin[1]); > > } else if (ret != -EINVAL) { > > - dev_err(&indio_dev->dev, "Invalid diff-channels property %d\n", ret); > > - goto err; > > + return dev_err_probe(&indio_dev->dev, ret, > > + "Invalid diff-channels property\n"); > > } > > As per above? > Agree with all comments, but as you said side notes. Stuff to cleanup in a series doing a wider driver clean up. Thanks, Jonathan
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index b5d3c9cea5c4..9fd94a792f42 100644 --- a/drivers/iio/adc/stm32-adc.c +++ b/drivers/iio/adc/stm32-adc.c @@ -2187,58 +2187,50 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels) { const struct stm32_adc_info *adc_info = adc->cfg->adc_info; - struct fwnode_handle *child; const char *name; int val, scan_index = 0, ret; bool differential; u32 vin[2]; - device_for_each_child_node(&indio_dev->dev, child) { + device_for_each_child_node_scoped(&indio_dev->dev, child) { ret = fwnode_property_read_u32(child, "reg", &val); - if (ret) { - dev_err(&indio_dev->dev, "Missing channel index %d\n", ret); - goto err; - } + if (ret) + return dev_err_probe(&indio_dev->dev, ret, + "Missing channel index\n"); ret = fwnode_property_read_string(child, "label", &name); /* label is optional */ if (!ret) { - if (strlen(name) >= STM32_ADC_CH_SZ) { - dev_err(&indio_dev->dev, "Label %s exceeds %d characters\n", - name, STM32_ADC_CH_SZ); - ret = -EINVAL; - goto err; - } + if (strlen(name) >= STM32_ADC_CH_SZ) + return dev_err_probe(&indio_dev->dev, -EINVAL, + "Label %s exceeds %d characters\n", + name, STM32_ADC_CH_SZ); + strscpy(adc->chan_name[val], name, STM32_ADC_CH_SZ); ret = stm32_adc_populate_int_ch(indio_dev, name, val); if (ret == -ENOENT) continue; else if (ret) - goto err; - } else if (ret != -EINVAL) { - dev_err(&indio_dev->dev, "Invalid label %d\n", ret); - goto err; - } + return ret; + } else if (ret != -EINVAL) + return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n"); - if (val >= adc_info->max_channels) { - dev_err(&indio_dev->dev, "Invalid channel %d\n", val); - ret = -EINVAL; - goto err; - } + if (val >= adc_info->max_channels) + return dev_err_probe(&indio_dev->dev, -EINVAL, + "Invalid channel %d\n", val); differential = false; ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2); /* diff-channels is optional */ if (!ret) { differential = true; - if (vin[0] != val || vin[1] >= adc_info->max_channels) { - dev_err(&indio_dev->dev, "Invalid channel in%d-in%d\n", - vin[0], vin[1]); - goto err; - } + if (vin[0] != val || vin[1] >= adc_info->max_channels) + return dev_err_probe(&indio_dev->dev, -EINVAL, + "Invalid channel in%d-in%d\n", + vin[0], vin[1]); } else if (ret != -EINVAL) { - dev_err(&indio_dev->dev, "Invalid diff-channels property %d\n", ret); - goto err; + return dev_err_probe(&indio_dev->dev, ret, + "Invalid diff-channels property\n"); } stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val, @@ -2247,11 +2239,9 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, val = 0; ret = fwnode_property_read_u32(child, "st,min-sample-time-ns", &val); /* st,min-sample-time-ns is optional */ - if (ret && ret != -EINVAL) { - dev_err(&indio_dev->dev, "Invalid st,min-sample-time-ns property %d\n", - ret); - goto err; - } + if (ret && ret != -EINVAL) + return dev_err_probe(&indio_dev->dev, ret, + "Invalid st,min-sample-time-ns property\n"); stm32_adc_smpr_init(adc, channels[scan_index].channel, val); if (differential) @@ -2261,11 +2251,6 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, } return scan_index; - -err: - fwnode_handle_put(child); - - return ret; } static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping)