Thanks for the review David.
On 04/03/2025 11:25, David Lechner wrote:
On Mon, Mar 3, 2025 at 12:32 PM Matti Vaittinen
<mazziesaccount@xxxxxxxxx> wrote:
There are ADC ICs which may have some of the AIN pins usable for other
functions. These ICs may have some of the AIN pins wired so that they
should not be used for ADC.
(Preferred?) way for marking pins which can be used as ADC inputs is to
add corresponding channels@N nodes in the device tree as described in
the ADC binding yaml.
Add couple of helper functions which can be used to retrieve the channel
information from the device node.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
+ *
+ * Return: Number of found channels on succes. Negative value to indicate
s/succes/success/
Thanks!
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+ const struct iio_chan_spec *template,
+ int max_chan_id,
+ struct iio_chan_spec **cs)
+{
+ struct iio_chan_spec *chan_array, *chan;
+ int num_chan = 0, ret;
+
+ num_chan = iio_adc_device_num_channels(dev);
+ if (num_chan < 1)
+ return num_chan;
+
+ chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
+ GFP_KERNEL);
+ if (!chan_array)
+ return -ENOMEM;
+
+ chan = &chan_array[0];
+
+ device_for_each_child_node_scoped(dev, child) {
+ u32 ch;
+
+ if (!fwnode_name_eq(child, "channel"))
+ continue;
+
+ ret = fwnode_property_read_u32(child, "reg", &ch);
+ if (ret)
+ return ret;
+
+ if (max_chan_id != -1 && ch > max_chan_id)
+ return -ERANGE;
+
Should we use return dev_err_probe() on these to help with debugging a bad dtb?
I am not fan of using dev_err_probe() in a 'library code'. This is
because we never know if there'll be some odd use-case where this is not
called from the probe.
All in all, I'd leave adding most of the debugs to the callers -
especially because we do not expect to have bad device-trees after the
initial 'development stage' of a board. The board 'development stage'
should really reveal bugs which prevent the channels from being
registered - and after the DT is correct, these debug prints become
unnecessary (albeit minor) binary bloat.
+ *chan = *template;
+ chan->channel = ch;
+ chan++;
+ }
+
+ *cs = chan_array;
+
+ return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");
We can make this less verbose by setting #define
DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do
EXPORT_SYMBOL_GPL() throughout the rest of the file.
I am not sure what to think of this. I use the good old 'ctrl + ]' in my
editor when I need to check how a function was supposed to be used. That
jumps to the spot of code where the function is. I'd like to see the
namespace mentioned there in order to not accidentally miss the fact the
function belongs to one.
OTOH, I do like simplifications. Yet, the added simplification might not
warrant the namespace not being visible in the function definition.
Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).
I had some lengthy discussion about this with Andy and Jonathan during
earlier review versions. In short, I don't like the idea of very
fragmented namespaces in IIO, which will just complicate the drivers
without providing any obvious benefit.
https://lore.kernel.org/all/20250222174842.57c091c5@jic23-huawei/
+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+ const struct iio_chan_spec *template,
+ int max_chan_id,
+ struct iio_chan_spec **cs);
+
There are some different opinions on this, but on the last patch I did
introducing a new namespace, the consensus seems to be that putting
the MODULE_IMPORT_NS() in the header file was convenient so that users
of the API don't have to remember to both include the header and add
the import macro.
I do like this suggestion, and I believe this would be the balance
between getting the benefit of hiding part of the symbols - while not
unnecessarily complicating the callers. I know some people are opposing
it though. My personal opinion is that having the MODULE_IMPORT_NS() in
a header would be neatly simplifying the calling code with very little
harm, especially here where including the header hardly has use-cases
outside the IIO ADC.
Unfortunately, the "safety" seems to often be a synonym for just "making
it intentionally hard". As Finnish people say: "Kärsi, kärsi,
kirkkaamman kruunun saat". :)
(Roughly translated as "Suffer, suffer, you will get a brighter crown").
Let's hear what Jonathan thinks of your suggestion.
Thanks!
-- Matti
