Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC
From: Andy Shevchenko
Date: Wed Jan 12 2022 - 10:25:22 EST
On Wed, Jan 12, 2022 at 11:50 AM Cristian Pop <cristian.pop@xxxxxxxxxx> wrote:
>
> This allows remote reading and writing of the GPIOs. This is useful in
> application that run on another PC, at system level, where multiple iio
> devices and GPIO devices are integrated together.
Should it be called GPIO-IIO proxy or something like that?
...
> +/*
> + * one-bit-adc-dac
> + *
These two lines make no sense.
> + * Copyright 2022 Analog Devices Inc.
> + */
...
> +enum ch_direction {
> + CH_IN,
> + CH_OUT,
> +};
How is it different from the corresponding GPIO flag?
...
> +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long info)
Can be compressed to a fewer LOCs.
...
> + return sprintf(label, "%s\n", st->labels[ch]);
In a few releases the sysfs_emit() is present and must be used.
...
> + fwnode = dev_fwnode(device);
No need. See below.
...
> + child_num = device_get_child_node_count(device);
Error checks?
...
> + st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);
You should use devm_kcalloc() instead, it does slightly more than
simple multiplication.
> + if (!st->labels)
> + return -ENOMEM;
...
> + fwnode_for_each_child_node(fwnode, child) {
device_for_each_...
> + if (fwnode_property_read_u32(child, "reg", &crt_ch))
> + continue;
> +
> + if (crt_ch >= num_channels)
> + continue;
> +
> + if (fwnode_property_read_string(child, "label", &label))
> + continue;
> +
> + chan = &channels[crt_ch];
> + st->labels[--i] = label;
> + }
...
> +MODULE_LICENSE("Dual BSD/GPL");
> \ No newline at end of file
Aiaiai.
--
With Best Regards,
Andy Shevchenko