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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <375bf803-a5d5-4778-938a-b8218b116375@gmail.com>
Date:   Thu, 14 Dec 2023 14:57:35 +0200
From:   Ceclan Dumitru <mitrutzceclan@...il.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     linus.walleij@...aro.org, brgl@...ev.pl, andy@...nel.org,
        linux-gpio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
        Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Leonard Göhrs <l.goehrs@...gutronix.de>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Haibo Chen <haibo.chen@....com>,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        Ceclan Dumitru <dumitru.ceclan@...log.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver



On 12/14/23 14:30, Jonathan Cameron wrote:
> On Tue, 12 Dec 2023 12:44:36 +0200
> Dumitru Ceclan <mitrutzceclan@...il.com> 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@...le.cc> # for gpio-regmap
>> Signed-off-by: Dumitru Ceclan <mitrutzceclan@...il.com>
> 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.