Re: [PATCH v2] meson saradc: add iio device attrib to switch channel 7 mux

[George Stark]

On 5/28/23 17:55, Jonathan Cameron wrote:
> On Sun, 28 May 2023 00:48:54 +0300
> George Stark <gnstark@xxxxxxxxxxxxxx> 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 <GNStark@xxxxxxxxxxxxxx>
> 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 = {
>