Patch adds two sysfs nodes: chan7_mux to set mux state
and chan7_mux_available to show available mux states.
Mux can be used to debug and calibrate adc by
switching and measuring well-known inputs like GND, Vdd etc.
Signed-off-by: George Stark <[email protected]>
---
drivers/iio/adc/meson_saradc.c | 65 ++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index e05e51900c35..6959a0064551 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/nvmem-consumer.h>
@@ -320,6 +321,7 @@ struct meson_sar_adc_priv {
bool temperature_sensor_calibrated;
u8 temperature_sensor_coefficient;
u16 temperature_sensor_adc_val;
+ u8 chan7_mux_sel;
};
static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
@@ -483,6 +485,7 @@ static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
+ priv->chan7_mux_sel = sel;
usleep_range(10, 20);
}
@@ -1130,8 +1133,70 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
return ret;
}
+static const char * const chan7_vol[] = {
+ "gnd",
+ "vdd/4",
+ "vdd/2",
+ "vdd*3/4",
+ "vdd",
+ "ch7_input",
+};
+
+static ssize_t chan7_mux_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+ unsigned int index = priv->chan7_mux_sel;
+
+ if (index >= ARRAY_SIZE(chan7_vol))
+ index = ARRAY_SIZE(chan7_vol) - 1;
+
+ return sysfs_emit(buf, "%s\n", chan7_vol[index]);
+}
+
+static ssize_t chan7_mux_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ int i;
+
+ i = sysfs_match_string(chan7_vol, buf);
+ if (i < 0)
+ return -EINVAL;
+ meson_sar_adc_set_chan7_mux(indio_dev, i);
+ return count;
+}
+
+static IIO_DEVICE_ATTR_RW(chan7_mux, -1);
+
+static ssize_t chan7_mux_available_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int i, len = 0;
+
+ for (i = 0; i < ARRAY_SIZE(chan7_vol); i++)
+ len += sysfs_emit_at(buf, len, "%s ", chan7_vol[i]);
+
+ return len;
+}
+
+static IIO_DEVICE_ATTR_RO(chan7_mux_available, -1);
+
+static struct attribute *meson_sar_adc_attrs[] = {
+ &iio_dev_attr_chan7_mux_available.dev_attr.attr,
+ &iio_dev_attr_chan7_mux.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group meson_sar_adc_attr_group = {
+ .attrs = meson_sar_adc_attrs,
+};
+
static const struct iio_info meson_sar_adc_iio_info = {
.read_raw = meson_sar_adc_iio_info_read_raw,
+ .attrs = &meson_sar_adc_attr_group,
};
static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
--
2.38.4
On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
> Patch adds two sysfs nodes: chan7_mux to set mux state
> and chan7_mux_available to show available mux states.
> Mux can be used to debug and calibrate adc by
> switching and measuring well-known inputs like GND, Vdd etc.
Thank you for an update, my comments below.
...
> ---
Missing changelog, what has been done in v2, how it's different to v1.
> drivers/iio/adc/meson_saradc.c | 65 ++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
...
> +static const char * const chan7_vol[] = {
> + "gnd",
> + "vdd/4",
> + "vdd/2",
> + "vdd*3/4",
> + "vdd",
> + "ch7_input",
> +};
> +
> +static ssize_t chan7_mux_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> + unsigned int index = priv->chan7_mux_sel;
> +
> + if (index >= ARRAY_SIZE(chan7_vol))
> + index = ARRAY_SIZE(chan7_vol) - 1;
I think this is incorrect and prone to error in the future in case this array
will be extended. What I would expect is to return something like "unknown".
> + return sysfs_emit(buf, "%s\n", chan7_vol[index]);
> +}
> +
> +static ssize_t chan7_mux_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + int i;
> +
> + i = sysfs_match_string(chan7_vol, buf);
> + if (i < 0)
> + return -EINVAL;
Do not shadow the error code if it's not justified.
return i;
> + meson_sar_adc_set_chan7_mux(indio_dev, i);
> + return count;
> +}
> +
Redundant blank line.
> +static IIO_DEVICE_ATTR_RW(chan7_mux, -1);
> +
> +static ssize_t chan7_mux_available_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int i, len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(chan7_vol); i++)
> + len += sysfs_emit_at(buf, len, "%s ", chan7_vol[i]);
> +
> + return len;
> +}
> +
Ditto.
> +static IIO_DEVICE_ATTR_RO(chan7_mux_available, -1);
--
With Best Regards,
Andy Shevchenko
On Sun, May 28, 2023 at 01:55:20PM +0300, Andy Shevchenko wrote:
> On Sun, May 28, 2023 at 01:46:37PM +0300, Andy Shevchenko wrote:
> > On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
...
And last but not least (I just noticed how Cc and To is formed in your email),
you may utilize my "smart" script [1] or ideas from it for sending patches to
the Linux kernel related mailing lists. It will automatically provide Cc and
To with a good approximation.
For v3 the command line can be (assuming your patch is on the top of the
current branch and the script is in the one of the $PATH folders):
ge2maintainer.sh -c 1 -v3 HEAD~0 --annotate
this will call for editor, so you would be able to add Changelog after cutter
'---' line.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
--
With Best Regards,
Andy Shevchenko
On Sun, May 28, 2023 at 01:46:37PM +0300, Andy Shevchenko wrote:
> On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
...
> > +static const char * const chan7_vol[] = {
> > + "gnd",
> > + "vdd/4",
> > + "vdd/2",
> > + "vdd*3/4",
> > + "vdd",
> > + "ch7_input",
> > +};
One more thing to discuss (Jonathan, what's your opinion?) I think the
following easier to understand and has less problematic characters in the names
(in case of sysfs direct use from shelll):
static const char * const chan7_vol[] = {
"gnd", // alternatively GND
"0.25vdd", // alternatively 0.25_vdd, 0.25Vdd, 0.25_Vdd
"0.5vdd",
"0.75vdd",
"vdd", // Vdd
"ch7_input",
};
That said, my personal preference:
static const char * const chan7_vol[] = {
"GND",
"0.25Vdd",
"0.5Vdd",
"0.75Vdd",
"Vdd",
"ch7_input",
};
--
With Best Regards,
Andy Shevchenko
On Sun, 28 May 2023 00:48:54 +0300
George Stark <[email protected]> wrote:
> Patch adds two sysfs nodes: chan7_mux to set mux state
> and chan7_mux_available to show available mux states.
> Mux can be used to debug and calibrate adc by
> switching and measuring well-known inputs like GND, Vdd etc.
>
> Signed-off-by: George Stark <[email protected]>
A few key things here.
1) ABI docs missing (Documentation/ABI/testing/sysfs-bus-iio-*
Without that it's hard to review new ABI.2
2) We are very conservative when it comes to adopting new ABI as the
reality is that userspace has no idea what to do with it.
Designing interfaces that work for a wide range of devices is hard
but necessary to enable general purpose software.
Based on the limited description we have here, I'm not understanding why
you don't just express this as a set of channels. One channel per mux
setting, with the in_voltageX_label providing the information on what the
channel is connected to.
This is an interesting facility, so good to enable for high precision calibration
but we still want to map it to standards signals. Userspace doesn't
care that these are all being measured via the same input 7 - which
is itself probably an input to a MUX.
Jonathan
> ---
> drivers/iio/adc/meson_saradc.c | 65 ++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index e05e51900c35..6959a0064551 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -11,6 +11,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/nvmem-consumer.h>
> @@ -320,6 +321,7 @@ struct meson_sar_adc_priv {
> bool temperature_sensor_calibrated;
> u8 temperature_sensor_coefficient;
> u16 temperature_sensor_adc_val;
> + u8 chan7_mux_sel;
> };
>
> static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> @@ -483,6 +485,7 @@ static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
> MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
>
> + priv->chan7_mux_sel = sel;
> usleep_range(10, 20);
> }
>
> @@ -1130,8 +1133,70 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
> return ret;
> }
>
> +static const char * const chan7_vol[] = {
> + "gnd",
> + "vdd/4",
> + "vdd/2",
> + "vdd*3/4",
> + "vdd",
> + "ch7_input",
> +};
> +
> +static ssize_t chan7_mux_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> + unsigned int index = priv->chan7_mux_sel;
> +
> + if (index >= ARRAY_SIZE(chan7_vol))
> + index = ARRAY_SIZE(chan7_vol) - 1;
> +
> + return sysfs_emit(buf, "%s\n", chan7_vol[index]);
> +}
> +
> +static ssize_t chan7_mux_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + int i;
> +
> + i = sysfs_match_string(chan7_vol, buf);
> + if (i < 0)
> + return -EINVAL;
> + meson_sar_adc_set_chan7_mux(indio_dev, i);
> + return count;
> +}
> +
> +static IIO_DEVICE_ATTR_RW(chan7_mux, -1);
> +
> +static ssize_t chan7_mux_available_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int i, len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(chan7_vol); i++)
> + len += sysfs_emit_at(buf, len, "%s ", chan7_vol[i]);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR_RO(chan7_mux_available, -1);
> +
> +static struct attribute *meson_sar_adc_attrs[] = {
> + &iio_dev_attr_chan7_mux_available.dev_attr.attr,
> + &iio_dev_attr_chan7_mux.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group meson_sar_adc_attr_group = {
> + .attrs = meson_sar_adc_attrs,
> +};
> +
> static const struct iio_info meson_sar_adc_iio_info = {
> .read_raw = meson_sar_adc_iio_info_read_raw,
> + .attrs = &meson_sar_adc_attr_group,
> };
>
> static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
On Sun, 28 May 2023 13:55:20 +0300
Andy Shevchenko <[email protected]> wrote:
> On Sun, May 28, 2023 at 01:46:37PM +0300, Andy Shevchenko wrote:
> > On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
>
> ...
>
> > > +static const char * const chan7_vol[] = {
> > > + "gnd",
> > > + "vdd/4",
> > > + "vdd/2",
> > > + "vdd*3/4",
> > > + "vdd",
> > > + "ch7_input",
> > > +};
>
> One more thing to discuss (Jonathan, what's your opinion?) I think the
> following easier to understand and has less problematic characters in the names
> (in case of sysfs direct use from shelll):
I suspect no one would use these particulary inputs directly from the shell,
but agreed that avoiding / where not absolutely necessary is a good idea.
Jonathan
>
> static const char * const chan7_vol[] = {
> "gnd", // alternatively GND
> "0.25vdd", // alternatively 0.25_vdd, 0.25Vdd, 0.25_Vdd
> "0.5vdd",
> "0.75vdd",
> "vdd", // Vdd
> "ch7_input",
> };
>
> That said, my personal preference:
>
> static const char * const chan7_vol[] = {
> "GND",
> "0.25Vdd",
> "0.5Vdd",
> "0.75Vdd",
> "Vdd",
> "ch7_input",
> };
>
On 5/28/23 17:55, Jonathan Cameron wrote:
> On Sun, 28 May 2023 00:48:54 +0300
> George Stark <[email protected]> wrote:
>
>> Patch adds two sysfs nodes: chan7_mux to set mux state
>> and chan7_mux_available to show available mux states.
>> Mux can be used to debug and calibrate adc by
>> switching and measuring well-known inputs like GND, Vdd etc.
>>
>> Signed-off-by: George Stark <[email protected]>
> A few key things here.
> 1) ABI docs missing (Documentation/ABI/testing/sysfs-bus-iio-*
> Without that it's hard to review new ABI.2
> 2) We are very conservative when it comes to adopting new ABI as the
> reality is that userspace has no idea what to do with it.
> Designing interfaces that work for a wide range of devices is hard
> but necessary to enable general purpose software.
>
> Based on the limited description we have here, I'm not understanding why
> you don't just express this as a set of channels. One channel per mux
> setting, with the in_voltageX_label providing the information on what the
> channel is connected to.
>
> This is an interesting facility, so good to enable for high precision calibration
> but we still want to map it to standards signals. Userspace doesn't
> care that these are all being measured via the same input 7 - which
> is itself probably an input to a MUX.
>
> Jonathan
Hello Jonathan
Thanks for the review.
Your idea of exposing the mux setting as iio channels is very
interesting and at least worth trying.
The sysfs approach was chosen because of the code changes are simple and
neat (compare to channels approach).
Also calibration by using those mux inputs are already supported in the
driver (performed at probe stage) so I expect very special usecases for
those mux settings like debug or device production stage tests. In those
usescases hardware specific knowledge is required anyway.
Best regards
George
>> ---
>> drivers/iio/adc/meson_saradc.c | 65 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
>> index e05e51900c35..6959a0064551 100644
>> --- a/drivers/iio/adc/meson_saradc.c
>> +++ b/drivers/iio/adc/meson_saradc.c
>> @@ -11,6 +11,7 @@
>> #include <linux/delay.h>
>> #include <linux/io.h>
>> #include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/nvmem-consumer.h>
>> @@ -320,6 +321,7 @@ struct meson_sar_adc_priv {
>> bool temperature_sensor_calibrated;
>> u8 temperature_sensor_coefficient;
>> u16 temperature_sensor_adc_val;
>> + u8 chan7_mux_sel;
>> };
>>
>> static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
>> @@ -483,6 +485,7 @@ static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
>> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
>> MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
>>
>> + priv->chan7_mux_sel = sel;
>> usleep_range(10, 20);
>> }
>>
>> @@ -1130,8 +1133,70 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
>> return ret;
>> }
>>
>> +static const char * const chan7_vol[] = {
>> + "gnd",
>> + "vdd/4",
>> + "vdd/2",
>> + "vdd*3/4",
>> + "vdd",
>> + "ch7_input",
>> +};
>> +
>> +static ssize_t chan7_mux_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> + unsigned int index = priv->chan7_mux_sel;
>> +
>> + if (index >= ARRAY_SIZE(chan7_vol))
>> + index = ARRAY_SIZE(chan7_vol) - 1;
>> +
>> + return sysfs_emit(buf, "%s\n", chan7_vol[index]);
>> +}
>> +
>> +static ssize_t chan7_mux_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + int i;
>> +
>> + i = sysfs_match_string(chan7_vol, buf);
>> + if (i < 0)
>> + return -EINVAL;
>> + meson_sar_adc_set_chan7_mux(indio_dev, i);
>> + return count;
>> +}
>> +
>> +static IIO_DEVICE_ATTR_RW(chan7_mux, -1);
>> +
>> +static ssize_t chan7_mux_available_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int i, len = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(chan7_vol); i++)
>> + len += sysfs_emit_at(buf, len, "%s ", chan7_vol[i]);
>> +
>> + return len;
>> +}
>> +
>> +static IIO_DEVICE_ATTR_RO(chan7_mux_available, -1);
>> +
>> +static struct attribute *meson_sar_adc_attrs[] = {
>> + &iio_dev_attr_chan7_mux_available.dev_attr.attr,
>> + &iio_dev_attr_chan7_mux.dev_attr.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group meson_sar_adc_attr_group = {
>> + .attrs = meson_sar_adc_attrs,
>> +};
>> +
>> static const struct iio_info meson_sar_adc_iio_info = {
>> .read_raw = meson_sar_adc_iio_info_read_raw,
>> + .attrs = &meson_sar_adc_attr_group,
>> };
>>
>> static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
>
On 5/28/23 13:46, Andy Shevchenko wrote:
Hello Andy
> On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
>> Patch adds two sysfs nodes: chan7_mux to set mux state
>> and chan7_mux_available to show available mux states.
>> Mux can be used to debug and calibrate adc by
>> switching and measuring well-known inputs like GND, Vdd etc.
> Thank you for an update, my comments below.
>
> ...
>
>> ---
> Missing changelog, what has been done in v2, how it's different to v1.
Ok I'll keep it on mind
>
>> drivers/iio/adc/meson_saradc.c | 65 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 65 insertions(+)
> ...
>
>> +static const char * const chan7_vol[] = {
>> + "gnd",
>> + "vdd/4",
>> + "vdd/2",
>> + "vdd*3/4",
>> + "vdd",
>> + "ch7_input",
>> +};
>> +
>> +static ssize_t chan7_mux_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> + unsigned int index = priv->chan7_mux_sel;
>> +
>> + if (index >= ARRAY_SIZE(chan7_vol))
>> + index = ARRAY_SIZE(chan7_vol) - 1;
> I think this is incorrect and prone to error in the future in case this array
> will be extended. What I would expect is to return something like "unknown".
I agree this part is unclean. Actually the register's last 3 (out of 8)
possible values
are stand for the same mux input "ch7_input". So theoretically we can
read from register
a value out of array bounds. There should be a comment at least.
About the question of naming mux inputs from the other letter (vdd/2 vs
0.5Vdd etc).
While I fully agree with you that point is better than slash but mixing
letter cases... should we?
e.g. this is how iio_info output looks like now:
...
voltage7: (input)
3 channel-specific attributes found:
attr 0: mean_raw value: 0
attr 1: raw value: 1
attr 2: scale value: 0.439453125
4 device-specific attributes found:
attr 0: chan7_mux value: gnd
attr 1: chan7_mux_available value: gnd vdd/4 vdd/2
vdd*3/4 vdd ch7_input
attr 2: current_timestamp_clock value: realtime
attr 3: waiting_for_supplier value: 0
or naming with Jonathan's approach
/sys/devices/platform/soc/fe000000.bus/fe002c00.adc/iio:device0/in_voltage_0.5vdd_raw
Best regards
George
>> + return sysfs_emit(buf, "%s\n", chan7_vol[index]);
>> +}
>> +
>> +static ssize_t chan7_mux_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + int i;
>> +
>> + i = sysfs_match_string(chan7_vol, buf);
>> + if (i < 0)
>> + return -EINVAL;
> Do not shadow the error code if it's not justified.
>
> return i;
>
>> + meson_sar_adc_set_chan7_mux(indio_dev, i);
>> + return count;
>> +}
>> +
> Redundant blank line.
>
>> +static IIO_DEVICE_ATTR_RW(chan7_mux, -1);
>> +
>> +static ssize_t chan7_mux_available_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int i, len = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(chan7_vol); i++)
>> + len += sysfs_emit_at(buf, len, "%s ", chan7_vol[i]);
>> +
>> + return len;
>> +}
>> +
> Ditto.
>
>> +static IIO_DEVICE_ATTR_RO(chan7_mux_available, -1);
Hi George,
On Sun, May 28, 2023 at 11:52 PM George Stark <[email protected]> wrote:
[...]
> > Based on the limited description we have here, I'm not understanding why
> > you don't just express this as a set of channels. One channel per mux
> > setting, with the in_voltageX_label providing the information on what the
> > channel is connected to.
> >
> > This is an interesting facility, so good to enable for high precision calibration
> > but we still want to map it to standards signals. Userspace doesn't
> > care that these are all being measured via the same input 7 - which
> > is itself probably an input to a MUX.
> >
> > Jonathan
>
> Hello Jonathan
>
> Thanks for the review.
>
> Your idea of exposing the mux setting as iio channels is very
> interesting and at least worth trying.
> The sysfs approach was chosen because of the code changes are simple and
> neat (compare to channels approach).
> Also calibration by using those mux inputs are already supported in the
> driver (performed at probe stage) so I expect very special usecases for
> those mux settings like debug or device production stage tests. In those
> usescases hardware specific knowledge is required anyway.
Another downside to the debugfs approach is user support:
If someone reports odd values on ADC channel 7 then we need to make
sure to double check if the mux has been altered from userspace (the
calibration during initialization ensures to leave channel 7 in a
consistent state, while a user may change the mux, forget about it and
then complain that values are wrong).
Best regards,
Martin
Mon, May 29, 2023 at 01:31:40AM +0300, George Stark kirjoitti:
> On 5/28/23 13:46, Andy Shevchenko wrote:
> > On Sun, May 28, 2023 at 12:48:54AM +0300, George Stark wrote:
...
> > > + if (index >= ARRAY_SIZE(chan7_vol))
> > > + index = ARRAY_SIZE(chan7_vol) - 1;
> > I think this is incorrect and prone to error in the future in case this array
> > will be extended. What I would expect is to return something like "unknown".
>
> I agree this part is unclean. Actually the register's last 3 (out of 8)
> possible values are stand for the same mux input "ch7_input". So
> theoretically we can read from register a value out of array bounds. There
> should be a comment at least.
You can add there in the array to extend it up to 8 entries, so it will be
clear. And for the rest you will return unknown / unsupported / etc.
...
> > > +static const char * const chan7_vol[] = {
> > > + "gnd",
> > > + "vdd/4",
> > > + "vdd/2",
> > > + "vdd*3/4",
> > > + "vdd",
> > > + "ch7_input",
> > > +};
> About the question of naming mux inputs from the other letter (vdd/2 vs
> 0.5Vdd etc).
> While I fully agree with you that point is better than slash but mixing
> letter cases... should we?
What's wrong with that?
> e.g. this is how iio_info output looks like now:
> ...
> ?? ???? ??? voltage7:? (input)
> ?? ???? ??? 3 channel-specific attributes found:
> ?? ???? ??? ??? attr? 0: mean_raw value: 0
> ?? ???? ??? ??? attr? 1: raw value: 1
> ?? ???? ??? ??? attr? 2: scale value: 0.439453125
> ?? ???? 4 device-specific attributes found:
> ?? ???? ??? ??? attr? 0: chan7_mux value: gnd
> ?? ???? ??? ??? attr? 1: chan7_mux_available value: gnd vdd/4 vdd/2 vdd*3/4
> vdd ch7_input
> ?? ???? ??? ??? attr? 2: current_timestamp_clock value: realtime
>
> ?? ???? ??? ??? attr? 3: waiting_for_supplier value: 0
>
> or naming with Jonathan's approach
> /sys/devices/platform/soc/fe000000.bus/fe002c00.adc/iio:device0/in_voltage_0.5vdd_raw
See the alternative I proposed is to have _ delimiter. But that might make
parsing a bit harder in user space.
--
With Best Regards,
Andy Shevchenko
On Mon, 29 May 2023 22:04:18 +0200
Martin Blumenstingl <[email protected]> wrote:
> Hi George,
>
> On Sun, May 28, 2023 at 11:52 PM George Stark <[email protected]> wrote:
> [...]
> > > Based on the limited description we have here, I'm not understanding why
> > > you don't just express this as a set of channels. One channel per mux
> > > setting, with the in_voltageX_label providing the information on what the
> > > channel is connected to.
> > >
> > > This is an interesting facility, so good to enable for high precision calibration
> > > but we still want to map it to standards signals. Userspace doesn't
> > > care that these are all being measured via the same input 7 - which
> > > is itself probably an input to a MUX.
> > >
> > > Jonathan
> >
> > Hello Jonathan
> >
> > Thanks for the review.
> >
> > Your idea of exposing the mux setting as iio channels is very
> > interesting and at least worth trying.
> > The sysfs approach was chosen because of the code changes are simple and
> > neat (compare to channels approach).
> > Also calibration by using those mux inputs are already supported in the
> > driver (performed at probe stage) so I expect very special usecases for
> > those mux settings like debug or device production stage tests. In those
> > usescases hardware specific knowledge is required anyway.
Agreed that using channels for this is more complex code wise, but the
avoidance of custom ABI is usually worthwhile. However I get your point
that this is weird debug / corner case paths anyway.
However...
> Another downside to the debugfs approach is user support:
> If someone reports odd values on ADC channel 7 then we need to make
> sure to double check if the mux has been altered from userspace (the
> calibration during initialization ensures to leave channel 7 in a
> consistent state, while a user may change the mux, forget about it and
> then complain that values are wrong).
This problem raises red flags for me. If the channel wasn't otherwise
useful then treating it as a weird debug thing is fine, but if a normal
read can end up returning weird answers because of leftover configuration
that is custom ABI, so standard code isn't even aware of it, that's bad.
Hence I would not want to see any path under which that can happen.
A read of in_voltage7_raw should continue to return the normal value
whatever this patch adds.
Jonathan
>
>
> Best regards,
> Martin