Main changes in v3 are Andy feedback on v2. Still note that I'm not
adding a new variant for dev_err_* that takes an error pointer and return
an int. I prefer to defer that if we really want such a variant.
Anyways, here it goes v3 log:
v1:
* https://lore.kernel.org/all/[email protected]/
v2:
* https://lore.kernel.org/all/[email protected]/
v3:
* Patch 1:
- Removed parenthesis around macros;
* Patch 2:
- Removed local struct device *dev helper;
- Added missing \n to printk().
* Patch 4:
- Make sure to not double error code printing;
- Added missing new line.
---
Nuno Sa (4):
dev_printk: add new dev_err_probe() helpers
iio: temperature: ltc2983: convert to dev_err_probe()
iio: backend: make use of dev_err_cast_probe()
iio: common: scmi_iio: convert to dev_err_probe()
drivers/iio/common/scmi_sensors/scmi_iio.c | 45 +++--
drivers/iio/industrialio-backend.c | 8 +-
drivers/iio/temperature/ltc2983.c | 260 +++++++++++++----------------
include/linux/dev_printk.h | 8 +
4 files changed, 143 insertions(+), 178 deletions(-)
---
base-commit: 234cb065ad82915ff8d06ce01e01c3e640b674d2
change-id: 20240404-dev-add_dev_errp_probe-69e7524c2803
--
Thanks!
- Nuno Sá
Using dev_err_cast_probe() to simplify the code.
Signed-off-by: Nuno Sa <[email protected]>
---
drivers/iio/industrialio-backend.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 929aff4040ed..efe05be284b6 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -561,11 +561,9 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
}
fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
- if (IS_ERR(fwnode)) {
- dev_err_probe(dev, PTR_ERR(fwnode),
- "Cannot get Firmware reference\n");
- return ERR_CAST(fwnode);
- }
+ if (IS_ERR(fwnode))
+ return dev_err_cast_probe(dev, fwnode,
+ "Cannot get Firmware reference\n");
guard(mutex)(&iio_back_lock);
list_for_each_entry(back, &iio_back_list, entry) {
--
2.45.2
This is similar to dev_err_probe() but for cases where an ERR_PTR() or
ERR_CAST() is to be returned simplifying patterns like:
dev_err_probe(dev, ret, ...);
return ERR_PTR(ret)
or
dev_err_probe(dev, PTR_ERR(ptr), ...);
return ERR_CAST(ptr)
Signed-off-by: Nuno Sa <[email protected]>
---
include/linux/dev_printk.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index ae80a303c216..ca32b5bb28eb 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -277,4 +277,12 @@ do { \
__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
+#define dev_err_ptr_probe(dev, ___err, fmt, ...) \
+ ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__))
+
+/* Simple helper for dev_err_probe() when ERR_CAST() is to be returned. */
+#define dev_err_cast_probe(dev, ___err_ptr, fmt, ...) \
+ ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, ##__VA_ARGS__))
+
#endif /* _DEVICE_PRINTK_H_ */
--
2.45.2
Make use of dev_err_probe() and dev_err_ptr_probe() to simplify error paths
during probe.
Signed-off-by: Nuno Sa <[email protected]>
---
drivers/iio/common/scmi_sensors/scmi_iio.c | 45 +++++++++++++-----------------
1 file changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 0c2caf3570db..7190eaede7fb 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -626,12 +626,10 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
&sensor->sensor_info->id,
&sensor->sensor_update_nb);
- if (ret) {
- dev_err(&iiodev->dev,
- "Error in registering sensor update notifier for sensor %s err %d",
- sensor->sensor_info->name, ret);
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_err_ptr_probe(&iiodev->dev, ret,
+ "Error in registering sensor update notifier for sensor %s\n",
+ sensor->sensor_info->name);
scmi_iio_set_timestamp_channel(&iio_channels[i], i);
iiodev->channels = iio_channels;
@@ -653,10 +651,9 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
return -ENODEV;
sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph);
- if (IS_ERR(sensor_ops)) {
- dev_err(dev, "SCMI device has no sensor interface\n");
- return PTR_ERR(sensor_ops);
- }
+ if (IS_ERR(sensor_ops))
+ return dev_err_probe(dev, PTR_ERR(sensor_ops),
+ "SCMI device has no sensor interface\n");
nr_sensors = sensor_ops->count_get(ph);
if (!nr_sensors) {
@@ -667,8 +664,8 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
for (i = 0; i < nr_sensors; i++) {
sensor_info = sensor_ops->info_get(ph, i);
if (!sensor_info) {
- dev_err(dev, "SCMI sensor %d has missing info\n", i);
- return -EINVAL;
+ return dev_err_probe(dev, -EINVAL,
+ "SCMI sensor %d has missing info\n", i);
}
/* This driver only supports 3-axis accel and gyro, skipping other sensors */
@@ -683,29 +680,25 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
scmi_iio_dev = scmi_alloc_iiodev(sdev, sensor_ops, ph,
sensor_info);
if (IS_ERR(scmi_iio_dev)) {
- dev_err(dev,
- "failed to allocate IIO device for sensor %s: %ld\n",
- sensor_info->name, PTR_ERR(scmi_iio_dev));
- return PTR_ERR(scmi_iio_dev);
+ return dev_err_probe(dev, PTR_ERR(scmi_iio_dev),
+ "failed to allocate IIO device for sensor %s\n",
+ sensor_info->name);
}
err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
scmi_iio_dev,
&scmi_iio_buffer_ops);
if (err < 0) {
- dev_err(dev,
- "IIO buffer setup error at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
+ return dev_err_probe(dev, err,
+ "IIO buffer setup error at sensor %s\n",
+ sensor_info->name);
}
err = devm_iio_device_register(dev, scmi_iio_dev);
- if (err) {
- dev_err(dev,
- "IIO device registration failed at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
- }
+ if (err)
+ return dev_err_probe(dev, err,
+ "IIO device registration failed at sensor %s\n",
+ sensor_info->name);
}
return err;
}
--
2.45.2
Use dev_err_probe() (and variants) in the probe() path. While at it, made
some simple improvements:
* Explicitly included the err.h and errno.h headers;
* Removed some unnecessary line breaks;
* Removed a redundant 'else';
* Added some missing \n to prink.
Signed-off-by: Nuno Sa <[email protected]>
---
drivers/iio/temperature/ltc2983.c | 260 +++++++++++++++++---------------------
1 file changed, 113 insertions(+), 147 deletions(-)
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 24d19f3c7292..21f2cfc55bf8 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>
@@ -432,10 +434,9 @@ __ltc2983_custom_sensor_new(struct ltc2983_data *st, const struct fwnode_handle
else
n_entries = fwnode_property_count_u64(fn, propname);
/* n_entries must be an even number */
- if (!n_entries || (n_entries % 2) != 0) {
- dev_err(dev, "Number of entries either 0 or not even\n");
- return ERR_PTR(-EINVAL);
- }
+ if (!n_entries || (n_entries % 2) != 0)
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Number of entries either 0 or not even\n");
new_custom = devm_kzalloc(dev, sizeof(*new_custom), GFP_KERNEL);
if (!new_custom)
@@ -443,19 +444,17 @@ __ltc2983_custom_sensor_new(struct ltc2983_data *st, const struct fwnode_handle
new_custom->size = n_entries * n_size;
/* check Steinhart size */
- if (is_steinhart && new_custom->size != LTC2983_CUSTOM_STEINHART_SIZE) {
- dev_err(dev, "Steinhart sensors size(%zu) must be %u\n", new_custom->size,
- LTC2983_CUSTOM_STEINHART_SIZE);
- return ERR_PTR(-EINVAL);
- }
+ if (is_steinhart && new_custom->size != LTC2983_CUSTOM_STEINHART_SIZE)
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Steinhart sensors size(%zu) must be %u\n",
+ new_custom->size, LTC2983_CUSTOM_STEINHART_SIZE);
+
/* 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) {
- dev_err(dev, "No space left(%d) for new custom sensor(%zu)",
- st->custom_table_size, new_custom->size);
- return ERR_PTR(-EINVAL);
- }
+ (LTC2983_CUST_SENS_TBL_END_REG - LTC2983_CUST_SENS_TBL_START_REG) + 1)
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "No space left(%d) for new custom sensor(%zu)\n",
+ st->custom_table_size, new_custom->size);
/* allocate the table */
if (is_steinhart)
@@ -688,21 +687,19 @@ 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);
- return ERR_PTR(-EINVAL);
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid open circuit current:%u\n",
+ oc_current);
}
thermo->sensor_config |= LTC2983_THERMOCOUPLE_OC_CHECK(1);
}
/* validate channel index */
if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermocouple",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chann:%d for differential thermocouple\n",
+ sensor->chan);
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,cold-junction-handle", 0);
@@ -710,14 +707,13 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
ref = NULL;
} else {
ret = fwnode_property_read_u32(ref, "reg", &thermo->cold_junction_chan);
- if (ret) {
+ if (ret)
/*
* This would be catched later but we can just return
* the error right away.
*/
- dev_err(&st->spi->dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ return dev_err_ptr_probe(&st->spi->dev, ret,
+ "Property reg must be given\n");
}
/* check custom sensor */
@@ -753,16 +749,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_err_cast_probe(dev, ref,
+ "Property adi,rsense-handle missing or invalid\n");
ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
- if (ret) {
- dev_err(dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_err_ptr_probe(dev, ret,
+ "Property reg must be given\n");
ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
if (!ret) {
@@ -781,19 +775,19 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->sensor_config = LTC2983_RTD_N_WIRES(3);
break;
default:
- dev_err(dev, "Invalid number of wires:%u\n", n_wires);
- return ERR_PTR(-EINVAL);
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Invalid number of wires:%u\n",
+ n_wires);
}
}
if (fwnode_property_read_bool(child, "adi,rsense-share")) {
/* Current rotation is only available with rsense sharing */
if (fwnode_property_read_bool(child, "adi,current-rotate")) {
- if (n_wires == 2 || n_wires == 3) {
- dev_err(dev,
- "Rotation not allowed for 2/3 Wire RTDs");
- return ERR_PTR(-EINVAL);
- }
+ if (n_wires == 2 || n_wires == 3)
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Rotation not allowed for 2/3 Wire RTDs\n");
+
rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
} else {
rtd->sensor_config |= LTC2983_RTD_R_SHARE(1);
@@ -816,29 +810,22 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK)
== LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
- (rtd->r_sense_chan <= min)) {
+ (rtd->r_sense_chan <= min))
/* kelvin rsense*/
- dev_err(dev,
- "Invalid rsense chann:%d to use in kelvin rsense",
- rtd->r_sense_chan);
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Invalid rsense chann:%d to use in kelvin rsense\n",
+ rtd->r_sense_chan);
- return ERR_PTR(-EINVAL);
- }
-
- if (sensor->chan < min || sensor->chan > max) {
- dev_err(dev, "Invalid chann:%d for the rtd config",
- sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < min || sensor->chan > max)
+ return dev_err_ptr_probe(dev, -EINVAL,
+ "Invalid chann:%d for the rtd config\n",
+ sensor->chan);
} else {
/* same as differential case */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for RTD", sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chann:%d for RTD\n",
+ sensor->chan);
}
/* check custom sensor */
@@ -886,10 +873,9 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->excitation_current = 0x08;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid value for excitation current(%u)\n",
+ excitation_current);
}
}
@@ -913,16 +899,14 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_err_cast_probe(dev, ref,
+ "Property adi,rsense-handle missing or invalid\n");
ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
- if (ret) {
- dev_err(dev, "rsense channel must be configured...\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_err_ptr_probe(dev, ret,
+ "rsense channel must be configured...\n");
if (fwnode_property_read_bool(child, "adi,single-ended")) {
thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
@@ -937,12 +921,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
}
/* validate channel index */
if (!(thermistor->sensor_config & LTC2983_THERMISTOR_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor\n",
+ sensor->chan);
/* check custom sensor */
if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART) {
@@ -981,12 +963,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
switch (excitation_current) {
case 0:
/* auto range */
- if (sensor->type >=
- LTC2983_SENSOR_THERMISTOR_STEINHART) {
- dev_err(&st->spi->dev,
- "Auto Range not allowed for custom sensors\n");
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Auto Range not allowed for custom sensors\n");
+
thermistor->excitation_current = 0x0c;
break;
case 250:
@@ -1023,10 +1003,9 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
thermistor->excitation_current = 0x0b;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid value for excitation current(%u)\n",
+ excitation_current);
}
}
@@ -1056,12 +1035,11 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
/* validate channel index */
if (!(diode->sensor_config & LTC2983_DIODE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor\n",
+ sensor->chan);
+
/* set common parameters */
diode->sensor.fault_handler = ltc2983_common_fault_handler;
diode->sensor.assign_chan = ltc2983_diode_assign_chan;
@@ -1083,10 +1061,9 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
diode->excitation_current = 0x03;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid value for excitation current(%u)\n",
+ excitation_current);
}
}
@@ -1111,17 +1088,15 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct fwnode_handle *child,
return ERR_PTR(-ENOMEM);
/* validate channel index */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chann:%d for r_sense",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chann:%d for r_sense\n",
+ sensor->chan);
ret = fwnode_property_read_u32(child, "adi,rsense-val-milli-ohms", &temp);
- if (ret) {
- dev_err(&st->spi->dev, "Property adi,rsense-val-milli-ohms missing\n");
- return ERR_PTR(-EINVAL);
- }
+ if (ret)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Property adi,rsense-val-milli-ohms missing\n");
/*
* Times 1000 because we have milli-ohms and __convert_to_raw
* expects scales of 1000000 which are used for all other
@@ -1149,12 +1124,11 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
if (fwnode_property_read_bool(child, "adi,single-ended"))
adc->single_ended = true;
- if (!adc->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential adc\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!adc->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chan:%d for differential adc\n",
+ sensor->chan);
+
/* set common parameters */
adc->sensor.assign_chan = ltc2983_adc_assign_chan;
adc->sensor.fault_handler = ltc2983_common_fault_handler;
@@ -1175,12 +1149,10 @@ static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
if (fwnode_property_read_bool(child, "adi,single-ended"))
temp->single_ended = true;
- if (!temp->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!temp->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
+ "Invalid chan:%d for differential temp\n",
+ sensor->chan);
temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp",
false, 4096, true);
@@ -1296,8 +1268,8 @@ static int ltc2983_reg_access(struct iio_dev *indio_dev,
if (readval)
return regmap_read(st->regmap, reg, readval);
- else
- return regmap_write(st->regmap, reg, writeval);
+
+ return regmap_write(st->regmap, reg, writeval);
}
static irqreturn_t ltc2983_irq_handler(int irq, void *data)
@@ -1330,10 +1302,9 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
device_property_read_u32(dev, "adi,filter-notch-freq", &st->filter_notch_freq);
st->num_channels = device_get_child_node_count(dev);
- if (!st->num_channels) {
- dev_err(&st->spi->dev, "At least one channel must be given!");
- return -EINVAL;
- }
+ if (!st->num_channels)
+ return dev_err_probe(&st->spi->dev, -EINVAL,
+ "At least one channel must be given!\n");
st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors),
GFP_KERNEL);
@@ -1438,19 +1409,17 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
time = wait_for_completion_timeout(&st->completion,
msecs_to_jiffies(wait_time));
- if (!time) {
- dev_err(&st->spi->dev, "EEPROM command timed out\n");
- return -ETIMEDOUT;
- }
+ if (!time)
+ return dev_err_probe(&st->spi->dev, -ETIMEDOUT,
+ "EEPROM command timed out\n");
ret = regmap_read(st->regmap, status_reg, &val);
if (ret)
return ret;
- if (val & status_fail_mask) {
- dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
- return -EINVAL;
- }
+ if (val & status_fail_mask)
+ return dev_err_probe(&st->spi->dev, -EINVAL,
+ "EEPROM command failed: 0x%02X\n", val);
return 0;
}
@@ -1464,10 +1433,9 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
ret = regmap_read_poll_timeout(st->regmap, LTC2983_STATUS_REG, status,
LTC2983_STATUS_UP(status) == 1, 25000,
25000 * 10);
- if (ret) {
- dev_err(&st->spi->dev, "Device startup timed out\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(&st->spi->dev, ret,
+ "Device startup timed out\n");
ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
LTC2983_NOTCH_FREQ_MASK,
@@ -1583,10 +1551,9 @@ static int ltc2983_probe(struct spi_device *spi)
return -ENODEV;
st->regmap = devm_regmap_init_spi(spi, <c2983_regmap_config);
- if (IS_ERR(st->regmap)) {
- dev_err(&spi->dev, "Failed to initialize regmap\n");
- return PTR_ERR(st->regmap);
- }
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+ "Failed to initialize regmap\n");
mutex_init(&st->lock);
init_completion(&st->completion);
@@ -1624,10 +1591,9 @@ static int ltc2983_probe(struct spi_device *spi)
ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
IRQF_TRIGGER_RISING, st->info->name, st);
- if (ret) {
- dev_err(&spi->dev, "failed to request an irq, %d", ret);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "failed to request an irq\n");
if (st->info->has_eeprom) {
ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
--
2.45.2
On Thu, 2024-06-06 at 13:17 +0300, Andy Shevchenko wrote:
> On Thu, Jun 06, 2024 at 09:22:38AM +0200, Nuno Sa wrote:
> > Use dev_err_probe() (and variants) in the probe() path. While at it, made
> > some simple improvements:
> > * Explicitly included the err.h and errno.h headers;
> > * Removed some unnecessary line breaks;
> > * Removed a redundant 'else';
> > * Added some missing \n to prink.
>
> ...
>
> > - if (ret) {
> > + if (ret)
> > /*
> > * This would be catched later but we can just
> > return
> > * the error right away.
> > */
> > - dev_err(&st->spi->dev, "Property reg must be
> > given\n");
> > - return ERR_PTR(ret);
> > - }
> > + return dev_err_ptr_probe(&st->spi->dev, ret,
> > + "Property reg must be
> > given\n");
>
> Even if it becomes a one line of code, it's still a multiline branch, due to
> comment. I think {} is better to be there. What does checkpatch say about
> this?
Checkpatch is fine about it...
>
>
> ...
>
> > + return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
>
> You can make all these lines shorter by using
>
> struct device *dev = &st->spi->dev; // or analogue
>
> at the top of the function.
>
Well, I had that in v2 (making the whole driver coherent with the local struct
device helper but you kind of "complained" for a precursor patch (on a
devm_kzalloc() call). So basically I deferred that change for a follow up patch.
- Nuno Sá
>
On Thu, Jun 06, 2024 at 09:22:38AM +0200, Nuno Sa wrote:
> Use dev_err_probe() (and variants) in the probe() path. While at it, made
> some simple improvements:
> * Explicitly included the err.h and errno.h headers;
> * Removed some unnecessary line breaks;
> * Removed a redundant 'else';
> * Added some missing \n to prink.
...
> - if (ret) {
> + if (ret)
> /*
> * This would be catched later but we can just return
> * the error right away.
> */
> - dev_err(&st->spi->dev, "Property reg must be given\n");
> - return ERR_PTR(ret);
> - }
> + return dev_err_ptr_probe(&st->spi->dev, ret,
> + "Property reg must be given\n");
Even if it becomes a one line of code, it's still a multiline branch, due to
comment. I think {} is better to be there. What does checkpatch say about this?
...
> + return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
You can make all these lines shorter by using
struct device *dev = &st->spi->dev; // or analogue
at the top of the function.
> + "Invalid chann:%d for RTD\n",
> + sensor->chan);
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 06, 2024 at 02:27:03PM +0200, Nuno S? wrote:
> On Thu, 2024-06-06 at 13:17 +0300, Andy Shevchenko wrote:
> > On Thu, Jun 06, 2024 at 09:22:38AM +0200, Nuno Sa wrote:
...
> > > + return dev_err_ptr_probe(&st->spi->dev, -EINVAL,
> >
> > You can make all these lines shorter by using
> >
> > struct device *dev = &st->spi->dev; // or analogue
> >
> > at the top of the function.
> >
>
> Well, I had that in v2 (making the whole driver coherent with the local struct
> device helper but you kind of "complained" for a precursor patch (on a
> devm_kzalloc() call). So basically I deferred that change for a follow up patch.
Hmm... I don't remember the story behind this, but probably it's good to have
this done one (precursor) or the other way (follow up). Just check how many
changes will be done, whichever diff is shorter, choose that one.
--
With Best Regards,
Andy Shevchenko
On Thu, 2024-06-06 at 17:12 +0300, Andy Shevchenko wrote:
> On Thu, Jun 06, 2024 at 02:27:03PM +0200, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 13:17 +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 06, 2024 at 09:22:38AM +0200, Nuno Sa wrote:
>
> ...
>
> > > > + return dev_err_ptr_probe(&st->spi->dev, -
> > > > EINVAL,
> > >
> > > You can make all these lines shorter by using
> > >
> > > struct device *dev = &st->spi->dev; // or analogue
> > >
> > > at the top of the function.
> > >
> >
> > Well, I had that in v2 (making the whole driver coherent with the local
> > struct
> > device helper but you kind of "complained" for a precursor patch (on a
> > devm_kzalloc() call). So basically I deferred that change for a follow up
> > patch.
>
> Hmm... I don't remember the story behind this, but probably it's good to have
> this done one (precursor) or the other way (follow up). Just check how many
> changes will be done, whichever diff is shorter, choose that one.
>
Well that has not much to do with the current series. I would prefer to have a
follow up when we're done with the current changes. Right now I would really
prefer to focus on the new dev_err_* APIs and see if anything else is needed for
this to be acceptable.
- Nuno Sá
On Thu, 6 Jun 2024 09:22:38 +0200
Nuno Sa <[email protected]> wrote:
> Use dev_err_probe() (and variants) in the probe() path. While at it, made
> some simple improvements:
> * Explicitly included the err.h and errno.h headers;
> * Removed some unnecessary line breaks;
> * Removed a redundant 'else';
> * Added some missing \n to prink.
>
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> @@ -1296,8 +1268,8 @@ static int ltc2983_reg_access(struct iio_dev *indio_dev,
>
> if (readval)
> return regmap_read(st->regmap, reg, readval);
> - else
> - return regmap_write(st->regmap, reg, writeval);
> +
> + return regmap_write(st->regmap, reg, writeval);
> }
Unrelated.
Otherwise updates look correct to me.
On Thu, 6 Jun 2024 09:22:37 +0200
Nuno Sa <[email protected]> wrote:
> This is similar to dev_err_probe() but for cases where an ERR_PTR() or
> ERR_CAST() is to be returned simplifying patterns like:
>
> dev_err_probe(dev, ret, ...);
> return ERR_PTR(ret)
> or
> dev_err_probe(dev, PTR_ERR(ptr), ...);
> return ERR_CAST(ptr)
>
> Signed-off-by: Nuno Sa <[email protected]>
I'm convinced this is worth doing but would like inputs from others
before I pick this series up.
Jonathan
> ---
> include/linux/dev_printk.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
> index ae80a303c216..ca32b5bb28eb 100644
> --- a/include/linux/dev_printk.h
> +++ b/include/linux/dev_printk.h
> @@ -277,4 +277,12 @@ do { \
>
> __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
>
> +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
> +#define dev_err_ptr_probe(dev, ___err, fmt, ...) \
> + ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__))
> +
> +/* Simple helper for dev_err_probe() when ERR_CAST() is to be returned. */
> +#define dev_err_cast_probe(dev, ___err_ptr, fmt, ...) \
> + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, ##__VA_ARGS__))
> +
> #endif /* _DEVICE_PRINTK_H_ */
>
On Sat, 2024-06-08 at 19:06 +0100, Jonathan Cameron wrote:
> On Thu, 6 Jun 2024 09:22:38 +0200
> Nuno Sa <[email protected]> wrote:
>
> > Use dev_err_probe() (and variants) in the probe() path. While at it, made
> > some simple improvements:
> > * Explicitly included the err.h and errno.h headers;
> > * Removed some unnecessary line breaks;
> > * Removed a redundant 'else';
> > * Added some missing \n to prink.
> >
> > Signed-off-by: Nuno Sa <[email protected]>
> > ---
>
>
> > @@ -1296,8 +1268,8 @@ static int ltc2983_reg_access(struct iio_dev
> > *indio_dev,
> >
> > if (readval)
> > return regmap_read(st->regmap, reg, readval);
> > - else
> > - return regmap_write(st->regmap, reg, writeval);
> > +
> > + return regmap_write(st->regmap, reg, writeval);
> > }
>
> Unrelated.
>
> Otherwise updates look correct to me.
Yeah, I know. It was simple enough that I sneaked it in and did mentioned it in
the commit message hoping it would make the change acceptable in here :)
- Nuno Sá