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 <nuno.sa@analog.com>
---
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.2On Thu, 6 Jun 2024 09:22:38 +0200 Nuno Sa <nuno.sa@analog.com> 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 <nuno.sa@analog.com> > --- > @@ -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 Sat, 2024-06-08 at 19:06 +0100, Jonathan Cameron wrote: > On Thu, 6 Jun 2024 09:22:38 +0200 > Nuno Sa <nuno.sa@analog.com> 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 <nuno.sa@analog.com> > > --- > > > > @@ -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á
On Mon, 10 Jun 2024 09:11:28 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-06-08 at 19:06 +0100, Jonathan Cameron wrote: > > On Thu, 6 Jun 2024 09:22:38 +0200 > > Nuno Sa <nuno.sa@analog.com> 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 <nuno.sa@analog.com> > > > --- > > > > > > > @@ -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 :) > lol. I didn't read the commit message. Fair enough. > - 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, 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 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á
© 2016 - 2026 Red Hat, Inc.