Re: [PATCH v2 4/7] iio: temperature: ltc2983: Use fwnode_property_present() for optional properties

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassed software validation for RTD rotation when `adi,number-of-wires` is absent.
- [Medium] The commit only partially implements its stated goal, leaving multiple instances of the exact same error-swallowing anti-pattern throughout the driver.
--

commit 94ec4e6144c071e0f0444b0ad792dffbdf874d09
Author: Liviu Stan <liviu.stan@analog.com>

iio: temperature: ltc2983: Use fwnode_property_present() for optional properties

This commit attempts to fix the silent swallowing of error codes when optional
properties are present but malformed. It updates the property reads for
adi,sensor-oc-current-microamp and adi,number-of-wires to check for presence
first.

> Checking fwnode_property_read_u32() return value with if (!ret)
> silently swallows meaningful error codes when a property is present
> but malformed. Use fwnode_property_present() first so that absence
> uses the default while a present but unreadable property returns
> a proper error.

Does this commit miss updating other optional properties that share the same
error-swallowing pattern?

For example, in ltc2983_rtd_new(), adi,excitation-current-microamp still
falls back to default values on any error:

	ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
				       &excitation_current);
	if (ret) {
		/* default to 5uA */
		rtd->excitation_current = 1;

Are there other instances, like in ltc2983_diode_new(), that also need this
update to fully implement the stated goal?

> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
[ ... ]
> @@ -760,8 +766,12 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
>  		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) {
> +	if (fwnode_property_present(child, "adi,number-of-wires")) {
> +		ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
> +		if (ret)
> +			return dev_err_ptr_probe(dev, ret,
> +						 "Failed to read adi,number-of-wires\n");
> +
>  		switch (n_wires) {
>  		case 2:
>  			rtd->sensor_config = LTC2983_RTD_N_WIRES(0);

If the adi,number-of-wires property is absent, n_wires remains 0. Since
the hardware configuration correctly defaults to a 2-wire setup
(sensor_config is zero-initialized), does leaving n_wires as 0 introduce a
regression where it bypasses the later validation for RTD rotation?

Further down in ltc2983_rtd_new():

	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)
				return dev_err_ptr_probe(dev, -EINVAL,
							 "Rotation not allowed for 2/3 Wire RTDs\n");

If n_wires is 0, this check is bypassed and current rotation is allowed for
a 2-wire RTD configuration. Should n_wires be explicitly set to 2 when the
property is absent to enforce this hardware constraint?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514144712.64374-1-liviu.stan@analog.com?part=4