[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZkNijKz0N7PPvmeU@smile.fi.intel.com>
Date: Tue, 14 May 2024 16:09:32 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: michael.hennerich@...log.com, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
lars@...afoo.de, jic23@...nel.org, robh@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
lgirdwood@...il.com, broonie@...nel.org, nuno.sa@...log.com,
marcelo.schmitt@...log.com, bigunclemax@...il.com,
dlechner@...libre.com, okan.sahin@...log.com, fr0st61te@...il.com,
alisa.roman@...log.com, marcus.folkesson@...il.com,
schnelle@...ux.ibm.com, liambeguin@...il.com
Subject: Re: [PATCH v8 6/6] iio: adc: ad7192: Add AD7194 support
On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> only chip that has no channel sequencer, num_slots should remain
> undefined.
>
> Also modify config AD7192 description for better scaling.
Some non-critical, mostly style related comments below.
..
This...
> +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> + /* 10th bit corresponds to CON18(Pseudo) */
..should be (you have broken indentation on the comment, btw):
/* 10th bit corresponds to CON18(Pseudo) */
#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
But no need to resend because of this, let's wait others to comment, and
if everything fine I think Jonathan can massage this when applying.
..
> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
Not sure that the comment has any value here.
..
> +static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
> +{
> + if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid AIN channel: %u\n", ain);
> +
> + return 0;
While this uses traditional pattern, it might be better looking in a form of
if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
return 0;
return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);
But at the same time I would rather expect this to be in the caller and here
to have a boolean function
static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
{
return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
}
> +}
..
> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);
return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);
?
Or with limit
return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
num_channels);
..
> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);
ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);
?
Or
ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
GFP_KERNEL);
?
..
> + device_for_each_child_node_scoped(dev, child) {
> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + ain, ARRAY_SIZE(ain));
> + if (ret == 0) {
And here I would rather go for the traditional pattern, i.e.
if (ret) {
...
} else {
...
}
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + ret = ad7194_validate_ain_channel(dev, ain[1]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan_diff;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->channel2 = ain[1];
> + ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Missing channel property\n");
> +
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->address = AD7194_CH(ain[0]);
> + }
> + ad7194_channels++;
> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists