Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver

[Ceclan Dumitru <mitrutzceclan@xxxxxxxxx>]

On 12/14/23 14:30, Jonathan Cameron wrote:
> On Tue, 12 Dec 2023 12:44:36 +0200
> Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote:
> 
>> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
>> which can be used in high precision, low noise single channel
>> applications or higher speed multiplexed applications. The Sigma-Delta
>> ADC is intended primarily for measurement of signals close to DC but also
>> delivers outstanding performance with input bandwidths out to ~10kHz.
>>
>> Reviewed-by: Michael Walle <michael@xxxxxxxx> # for gpio-regmap
>> Signed-off-by: Dumitru Ceclan <mitrutzceclan@xxxxxxxxx>
> Hi
> 
> Given it seems like you'll be doing a v9, one quick comment from me below.
> 
> Jonathan
> 
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> new file mode 100644
>> index 000000000000..96918b24a10a
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -0,0 +1,964 @@
> ...
> 
>> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>> +{
> 
> ...
> 
>> +
>> +	if (st->info->has_temp) {
>> +		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
>> +		chan_st_priv = &channels_st_priv_arr[chan_index];
>> +		chan_st_priv->ain =
>> +			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
>> +		chan_st_priv->cfg.bipolar = false;
>> +		chan_st_priv->cfg.input_buf = true;
>> +		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
>> +		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>> +
>> +		chan_index++;
>> +	}
>> +
>> +	device_for_each_child_node(dev, child) {
>> +		chan = &chan_arr[chan_index];
>> +		chan_st_priv = &channels_st_priv_arr[chan_index];
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     ain, ARRAY_SIZE(ain));
>> +		if (ret) {
>> +			fwnode_handle_put(child);
>> +			return ret;
>> +		}
>> +
>> +		if (ain[0] >= st->info->num_inputs ||
>> +		    ain[1] >= st->info->num_inputs) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "Input pin number out of range for pair (%d %d).\n",
>> +					     ain[0], ain[1]);
>> +		}
>> +
>> +		ret = fwnode_property_match_property_string(child,
>> +							    "adi,reference-select",
>> +							    ad7173_ref_sel_str,
>> +							    ARRAY_SIZE(ad7173_ref_sel_str));
>> +
>> +		if (ret < 0)
>> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
>> +		else
>> +			ref_sel = ret;
> Simpler pattern for properties with a default is not to check the error code.
> 
> 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> 
> 		fwnode_property_match_property_String(child, ...
> 
> so only if it succeeds is the value overridden.

Where exactly would the value be overridden, the function does not have
an argument passed for the found index. The function is written to
return either the found index or a negative error.

The proposed pattern would just ignore the returned index and would
always leave ref_sel to default. Am I missing something?

I can see in the thread where it was introduced that you proposed:
"Looking at the usecases I wonder if it would be better to pass in

an unsigned int *ret which is only updated on a match?"

But on the iio togreg branch that was suggested I could the function on,
it does not have that parameter.