On 12/14/23 14:30, Jonathan Cameron wrote:
> On Tue, 12 Dec 2023 12:44:36 +0200
> Dumitru Ceclan <[email protected]> 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 <[email protected]> # for gpio-regmap
>> Signed-off-by: Dumitru Ceclan <[email protected]>
> 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.
On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote:
> On 12/14/23 14:30, Jonathan Cameron wrote:
> > On Tue, 12 Dec 2023 12:44:36 +0200
> > Dumitru Ceclan <[email protected]> wrote:
...
> >> + ret = fwnode_property_match_property_string(child,
> >> + "adi,reference-select",
> >> + ad7173_ref_sel_str,
> >> + ARRAY_SIZE(ad7173_ref_sel_str));
> >> +
Redundant blank line.
> >> + 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.
Yeah, with the current API we can have one check (no 'else' branch):
ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
ret = ...
if (ret >= 0)
ref_sel = ret;
But your approach is good to me.
...
It's always possible to change prototype, and now of course is the best time
as all the users are provided in the single tree. That said, patches are
welcome if this is what we want. (My proposal was to return index in case of
no error, but at the same time leave it in the returned code, so it will be
aligned with other match functions of fwnode.
But this in either way will complicate the implementation. And I don't find
critical to have if-else in each caller as some of them may do something
different on the error case, when option is mandatory. In such cases we
usually don't provide output if we know that an error condition occurs.
--
With Best Regards,
Andy Shevchenko
On Thu, 14 Dec 2023 16:47:29 +0200
Andy Shevchenko <[email protected]> wrote:
> On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote:
> > On 12/14/23 14:30, Jonathan Cameron wrote:
> > > On Tue, 12 Dec 2023 12:44:36 +0200
> > > Dumitru Ceclan <[email protected]> wrote:
>
> ...
>
> > >> + ret = fwnode_property_match_property_string(child,
> > >> + "adi,reference-select",
> > >> + ad7173_ref_sel_str,
> > >> + ARRAY_SIZE(ad7173_ref_sel_str));
>
> > >> +
>
> Redundant blank line.
>
> > >> + 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.
>
> Yeah, with the current API we can have one check (no 'else' branch):
>
> ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> ret = ...
> if (ret >= 0)
> ref_sel = ret;
>
Yeah. I was clearly lacking in coffee or just being an idiot that day!
> But your approach is good to me.
>
> ...
>
> It's always possible to change prototype, and now of course is the best time
> as all the users are provided in the single tree. That said, patches are
> welcome if this is what we want. (My proposal was to return index in case of
> no error, but at the same time leave it in the returned code, so it will be
> aligned with other match functions of fwnode.
>
> But this in either way will complicate the implementation. And I don't find
> critical to have if-else in each caller as some of them may do something
> different on the error case, when option is mandatory. In such cases we
> usually don't provide output if we know that an error condition occurs.
I'm fine with it being as it is. Was just having a slow brain day.
J
>