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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux