On 6/21/2012 4:41 PM, Lars-Peter Clausen wrote:
On 06/21/2012 04:16 PM, Jonathan Cameron wrote:
On 6/18/2012 6:14 PM, Lars-Peter Clausen wrote:
This patch adds support for the Analog Devices AD7265 and AD7266
Analog-to-Digital converters.
Mostly fine. I'm unconvinced the complexity around the
channel array creation is necessary. Might save a little on
data but loses in readability!
Jonathan
[...]
+static int __devinit ad7266_alloc_channels(struct iio_dev *indio_dev)
+{
+ struct ad7266_state *st = iio_priv(indio_dev);
+ const struct iio_chan_spec *channel_template;
+ struct iio_chan_spec *channels;
+ unsigned long *scan_masks;
+ unsigned int num_channels;
+
I'm unclear on why all this complexity is necessary.
We aren't dealing with huge numbers of channels here.
Why can't we just have a couple of const static arrays and
pick between them? This is just nasty to read for a
fairly small gain.
Hm, I doubt that the code looks less messy if we do it that way. It would
add another level of indention and we'd have the same code twice once for
the fixed case and once for the non-fixed case. It would more or less look
like this:
I'd still be happier with that than the memcpy. That implies that stuff
in these is not const, whereas it is for a given setup.
if (!st->fixed_addr) {
if (st->mode == AD7266_MODE_SINGLE_ENDED) {
if (st->range == AD7266_RANGE_VREF) {
channel_template = ad7266_channels_u;
num_channels = ARRAY_SIZE(ad7266_channels_u);
} else {
channel_template = ad7266_channels_s;
num_channels = ARRAY_SIZE(ad7266_channels_s);
}
scan_masks = ad7266_available_scan_masks;
} else {
channel_template = ad7266_channels_diff;
num_channels = ARRAY_SIZE(ad7266_channels_diff);
scan_masks = ad7266_available_scan_masks_diff;
}
else {
if (st->mode == AD7266_MODE_SINGLE_ENDED) {
if (st->range == AD7266_RANGE_VREF) {
channel_template = ad7266_channels_u_fixed;
num_channels = ARRAY_SIZE(ad7266_channels_u_fixed);
} else {
channel_template = ad7266_channels_s_fixed;
num_channels = ARRAY_SIZE(ad7266_channels_s_fixed);
}
} else {
channel_template = ad7266_channels_diff_fixed;
num_channels = ARRAY_SIZE(ad7266_channels_diff_fixed);
}
indio_dev->available_scan_masks = ad7266_available_scan_masks_fixed;
}
Then use an array (little fiddly)
struct chan_set {
struct iio_chan_spec *chans;
int num_chans;
int *scan_masks;
};
//index 0 is fixed_addr,
//index 1 is (st->mode == AD7266_MODE_SINGLE_ENDED)
//index 2 is (st->range == AD7266_RANGE_VREF)
static const
struct chan_set bob[2][2][2]
= {{{{ad7266_channels_u, ARRAY_SIZE(ad7266_channels_u),
ad7266_available_scan_masks},
{ad7266_channels_s, ARRAY_SIZE(ad7266_channels_s),
ad7266_available_scan_masks}},
{{ad7266_channels_diff, ARRAY_SIZE(ad7266_channels_diff),
ad7266_available_scan_masks_diff},
{ad7266_channels_diff, ARRAY_SIZE(ad7266_channels_diff),
ad7266_available_scan_masks_diff}},
{{{ad7266_channels_u_fixed, ARRAY_SIZE(ad7266_channels_u_fixed),
ad7266_available_scan_masks_fixed}},
{ad7266_channels_s_fixed, ARRAY_SIZE(ad7266_channels_s_fixed),
ad7266_available_scan_masks_fixed}},
{{ad7266_channels_channels_diff_fixed,
ARRAY_SIZE(ad7266_channels_diff_fixed), ad7266_available_scan_masks_fixed},
{ad7266_channels_channels_diff_fixed,
ARRAY_SIZE(ad7266_channels_diff_fixed),
ad7266_available_scan_masks_fixed}}}};
channel_template = bob[fixed_addr][st->mode ==
AD7266_MODE_SINGLE_ENDED][st->range == AD7266_RANGE_VREF].chans;
etc.
Hmm. not all that clean here, but does make it explicit that we are
selecting from amongst a set of constant data.
+ if (st->mode == AD7266_MODE_SINGLE_ENDED) {
+ if (st->range == AD7266_RANGE_VREF) {
+ channel_template = ad7266_channels_u;
+ num_channels = ARRAY_SIZE(ad7266_channels_u);
+ } else {
+ channel_template = ad7266_channels_s;
+ num_channels = ARRAY_SIZE(ad7266_channels_s);
+ }
+ scan_masks = ad7266_available_scan_masks;
+ } else {
+ channel_template = ad7266_channels_diff;
+ num_channels = ARRAY_SIZE(ad7266_channels_diff);
+ scan_masks = ad7266_available_scan_masks_diff;
+ }
+
+ if (!st->fixed_addr) {
+ indio_dev->available_scan_masks = scan_masks;
+ indio_dev->masklength = indio_dev->num_channels;
+ } else {
check patch is moaning at me about this next line being two long.
But breaking the line here certainly does not improve readability. The 80
chars is more of a soft limit.
It doesn't improved readability, but it also doesn't significantly worsen
it. Perhaps shortening the naming might be an idea.
ad7266_av_scan_masks_fixed
seems detailed enough to me!
+ indio_dev->available_scan_masks = ad7266_available_scan_masks_fixed;
+ indio_dev->masklength = 2;
+ num_channels = 2;
+ }
+
+ channels = kcalloc(num_channels + 1, sizeof(*channels),
+ GFP_KERNEL);
+ if (!channels)
+ return -ENOMEM;
+
+ memcpy(channels, channel_template, num_channels * sizeof(*channels));
+ channels[num_channels] =
+ (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(num_channels);
+
+ indio_dev->num_channels = num_channels + 1;
+ indio_dev->channels = channels;
+
+ return 0;
+}
+
[...]
Thanks,
- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
