[PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Trimarchi]

The following functions are supported:
- write, read potentiometer value

Value are exported in DBm because it's used as an audio
attenuator

Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf

Change-Id: I24f6b33cfa92ce16f489ff763f1df26126a1a7f2
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 .../bindings/iio/potentiometer/ds1807.txt          |  17 +++
 drivers/iio/potentiometer/Kconfig                  |  10 ++
 drivers/iio/potentiometer/Makefile                 |   1 +
 drivers/iio/potentiometer/ds1807.c                 | 156 +++++++++++++++++++++
 4 files changed, 184 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
 create mode 100644 drivers/iio/potentiometer/ds1807.c

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
new file mode 100644
index 0000000..3e30f4c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
@@ -0,0 +1,17 @@
+* Maxim Integrated DS1807 digital potentiometer driver
+
+The node for this driver must be a child node of a I2C controller, hence
+all mandatory properties for your controller must be specified. See directory:
+
+        Documentation/devicetree/bindings/i2c
+
+for more details.
+
+Required properties:
+	- compatible: "maxim,ds1807",
+
+Example:
+ds1807: ds1807@1 {
+	reg = <0x28>;
+	compatible = "maxim,ds1807";
+};
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 79ec2eb..92e5a6b 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -25,6 +25,16 @@ config DS1803
 	  To compile this driver as a module, choose M here: the
 	  module will be called ds1803.
 
+config DS1807
+	tristate "Maxim Integrated DS1807 Digital Potentiometer driver"
+	depends on I2C
+	help
+	  Say yes here to build support for the Maxim Integrated DS1807
+	  digital potentiometer chip. Value are reported back in DBm
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ds1807.
+
 config MAX5481
         tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
         depends on SPI
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 4af6578..3c409bb 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AD5272) += ad5272.o
 obj-$(CONFIG_DS1803) += ds1803.o
+obj-$(CONFIG_DS1807) += ds1807.o
 obj-$(CONFIG_MAX5481) += max5481.o
 obj-$(CONFIG_MAX5487) += max5487.o
 obj-$(CONFIG_MCP4018) += mcp4018.o
diff --git a/drivers/iio/potentiometer/ds1807.c b/drivers/iio/potentiometer/ds1807.c
new file mode 100644
index 0000000..acb4884
--- /dev/null
+++ b/drivers/iio/potentiometer/ds1807.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Maxim Integrated DS1807 digital potentiometer driver
+ * Copyright (c) 2018 Michael Trimarchi
+ *
+ * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf
+ *
+ * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
+ * ds1807	2	65		45			0101xxx
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#define DS1807_MAX_VALUE	64
+#define DS1807_MUTE		-90
+#define DS1807_WRITE(chan)	(0xa8 | ((chan) + 1))
+
+#define DS1807_CHANNEL(ch) {					\
+	.type = IIO_CHAN_INFO_HARDWAREGAIN,			\
+	.indexed = 1,						\
+	.output = 1,						\
+	.channel = (ch),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),	\
+}
+
+static const struct iio_chan_spec ds1807_channels[] = {
+	DS1807_CHANNEL(0),
+	DS1807_CHANNEL(1),
+};
+
+struct ds1807_data {
+	struct i2c_client *client;
+};
+
+static int ds1807_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ds1807_data *data = iio_priv(indio_dev);
+	int pot = chan->channel;
+	int ret;
+	u8 result[ARRAY_SIZE(ds1807_channels)];
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = i2c_master_recv(data->client, result,
+				indio_dev->num_channels);
+		if (ret < 0)
+			return ret;
+
+		*val2 = 0;
+		if (result[pot] == DS1807_MAX_VALUE)
+			*val = DS1807_MUTE;
+		else
+			*val = -result[pot];
+
+		return IIO_VAL_INT_PLUS_MICRO_DB;
+	}
+
+	return -EINVAL;
+}
+
+static int ds1807_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct ds1807_data *data = iio_priv(indio_dev);
+	int pot = chan->channel;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		if (val2 != 0 || val < DS1807_MUTE || val > 0)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = (val == DS1807_MUTE) ? DS1807_MAX_VALUE : -val;
+
+	return i2c_smbus_write_byte_data(data->client, DS1807_WRITE(pot), val);
+}
+
+static const struct iio_info ds1807_info = {
+	.read_raw = ds1807_read_raw,
+	.write_raw = ds1807_write_raw,
+};
+
+static int ds1807_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct ds1807_data *data;
+	struct iio_dev *indio_dev;
+	int channel, ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, indio_dev);
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &ds1807_info;
+	indio_dev->channels = ds1807_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ds1807_channels);
+	indio_dev->name = client->name;
+
+	/* reset device gain */
+	for (channel = 0; channel < indio_dev->num_channels; channel++) {
+		ret = i2c_smbus_write_byte_data(client, DS1807_WRITE(channel),
+						DS1807_MUTE);
+		if (ret < 0)
+			return -ENODEV;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id ds1807_dt_ids[] = {
+	{ .compatible = "maxim,ds1807", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ds1807_dt_ids);
+#endif /* CONFIG_OF */
+
+static const struct i2c_device_id ds1807_id[] = {
+	{ "ds1807", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ds1807_id);
+
+static struct i2c_driver ds1807_driver = {
+	.driver = {
+		.name	= "ds1807",
+		.of_match_table = of_match_ptr(ds1807_dt_ids),
+	},
+	.probe		= ds1807_probe,
+	.id_table	= ds1807_id,
+};
+
+module_i2c_driver(ds1807_driver);
+
+MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
+MODULE_DESCRIPTION("DS1807 digital potentiometer");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Sun,  6 May 2018 15:30:47 +0200
Michael Trimarchi <michael@amarulasolutions.com> wrote:

> The following functions are supported:
> - write, read potentiometer value
> 
> Value are exported in DBm because it's used as an audio
> attenuator

This is interesting.  The problem is that there is no way for
userspace to know that it is reporting in DBm rather than
reporting a linear gain or a straight forward resistance.

This is rather closer in operation to the analog front end
driver I took the other day than to the other potentiometers
we have drivers for.

Anyhow, how to solve this?  Two options come to mind.
1) Look up table mapping to linear gain as per current ABI
2) Add a new channel type to represent the fact we are
looking at a logarithmic value, letting us handle it as DB.

Lars, this area is probably more familiar to you than it is
to me, what do you think?

Jonathan
> 
> Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf
> 
> Change-Id: I24f6b33cfa92ce16f489ff763f1df26126a1a7f2
Please remember to drop these change IDs on patches to the mailing list
They don't mean anything to us unfortunately!

> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  .../bindings/iio/potentiometer/ds1807.txt          |  17 +++
>  drivers/iio/potentiometer/Kconfig                  |  10 ++
>  drivers/iio/potentiometer/Makefile                 |   1 +
>  drivers/iio/potentiometer/ds1807.c                 | 156 +++++++++++++++++++++
>  4 files changed, 184 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
>  create mode 100644 drivers/iio/potentiometer/ds1807.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
> new file mode 100644
> index 0000000..3e30f4c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
> @@ -0,0 +1,17 @@
> +* Maxim Integrated DS1807 digital potentiometer driver

This might be a good candidate for the trivial-device.txt binding file.

> +
> +The node for this driver must be a child node of a I2C controller, hence
> +all mandatory properties for your controller must be specified. See directory:
> +
> +        Documentation/devicetree/bindings/i2c
> +
> +for more details.
> +
> +Required properties:
> +	- compatible: "maxim,ds1807",
> +
> +Example:
> +ds1807: ds1807@1 {
> +	reg = <0x28>;
> +	compatible = "maxim,ds1807";
> +};
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 79ec2eb..92e5a6b 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -25,6 +25,16 @@ config DS1803
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ds1803.
>  
> +config DS1807
> +	tristate "Maxim Integrated DS1807 Digital Potentiometer driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for the Maxim Integrated DS1807
> +	  digital potentiometer chip. Value are reported back in DBm
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ds1807.
> +
>  config MAX5481
>          tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
>          depends on SPI
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 4af6578..3c409bb 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AD5272) += ad5272.o
>  obj-$(CONFIG_DS1803) += ds1803.o
> +obj-$(CONFIG_DS1807) += ds1807.o
>  obj-$(CONFIG_MAX5481) += max5481.o
>  obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4018) += mcp4018.o
> diff --git a/drivers/iio/potentiometer/ds1807.c b/drivers/iio/potentiometer/ds1807.c
> new file mode 100644
> index 0000000..acb4884
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ds1807.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Maxim Integrated DS1807 digital potentiometer driver
> + * Copyright (c) 2018 Michael Trimarchi
> + *
> + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf
> + *
> + * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
> + * ds1807	2	65		45			0101xxx
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#define DS1807_MAX_VALUE	64
> +#define DS1807_MUTE		-90
> +#define DS1807_WRITE(chan)	(0xa8 | ((chan) + 1))
> +
> +#define DS1807_CHANNEL(ch) {					\
> +	.type = IIO_CHAN_INFO_HARDWAREGAIN,			\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (ch),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),	\
> +}
> +
> +static const struct iio_chan_spec ds1807_channels[] = {
> +	DS1807_CHANNEL(0),
> +	DS1807_CHANNEL(1),
> +};
> +
> +struct ds1807_data {
> +	struct i2c_client *client;
> +};
> +
> +static int ds1807_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ds1807_data *data = iio_priv(indio_dev);
> +	int pot = chan->channel;
> +	int ret;
> +	u8 result[ARRAY_SIZE(ds1807_channels)];
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		ret = i2c_master_recv(data->client, result,
> +				indio_dev->num_channels);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val2 = 0;
> +		if (result[pot] == DS1807_MAX_VALUE)
> +			*val = DS1807_MUTE;
> +		else
> +			*val = -result[pot];
> +
> +		return IIO_VAL_INT_PLUS_MICRO_DB;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ds1807_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ds1807_data *data = iio_priv(indio_dev);
> +	int pot = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		if (val2 != 0 || val < DS1807_MUTE || val > 0)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val = (val == DS1807_MUTE) ? DS1807_MAX_VALUE : -val;
> +
> +	return i2c_smbus_write_byte_data(data->client, DS1807_WRITE(pot), val);
> +}
> +
> +static const struct iio_info ds1807_info = {
> +	.read_raw = ds1807_read_raw,
> +	.write_raw = ds1807_write_raw,
> +};
> +
> +static int ds1807_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct ds1807_data *data;
> +	struct iio_dev *indio_dev;
> +	int channel, ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &ds1807_info;
> +	indio_dev->channels = ds1807_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ds1807_channels);
> +	indio_dev->name = client->name;
> +
> +	/* reset device gain */
> +	for (channel = 0; channel < indio_dev->num_channels; channel++) {
> +		ret = i2c_smbus_write_byte_data(client, DS1807_WRITE(channel),
> +						DS1807_MUTE);
> +		if (ret < 0)
> +			return -ENODEV;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id ds1807_dt_ids[] = {
> +	{ .compatible = "maxim,ds1807", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ds1807_dt_ids);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id ds1807_id[] = {
> +	{ "ds1807", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ds1807_id);
> +
> +static struct i2c_driver ds1807_driver = {
> +	.driver = {
> +		.name	= "ds1807",
> +		.of_match_table = of_match_ptr(ds1807_dt_ids),
> +	},
> +	.probe		= ds1807_probe,
> +	.id_table	= ds1807_id,
> +};
> +
> +module_i2c_driver(ds1807_driver);
> +
> +MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
> +MODULE_DESCRIPTION("DS1807 digital potentiometer");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi

On Sun, May 6, 2018 at 7:37 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun,  6 May 2018 15:30:47 +0200
> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>
>> The following functions are supported:
>> - write, read potentiometer value
>>
>> Value are exported in DBm because it's used as an audio
>> attenuator
>
> This is interesting.  The problem is that there is no way for
> userspace to know that it is reporting in DBm rather than
> reporting a linear gain or a straight forward resistance.
>

We have already drivers/iio/amplifiers/ad8366.c
Each step is a db (negative) and last step is mapped to some type of
mute (-90db)

> This is rather closer in operation to the analog front end
> driver I took the other day than to the other potentiometers
> we have drivers for.
>
> Anyhow, how to solve this?  Two options come to mind.
> 1) Look up table mapping to linear gain as per current ABI
> 2) Add a new channel type to represent the fact we are
> looking at a logarithmic value, letting us handle it as DB.
>
> Lars, this area is probably more familiar to you than it is
> to me, what do you think?
>
> Jonathan
>>
>> Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf
>>
>> Change-Id: I24f6b33cfa92ce16f489ff763f1df26126a1a7f2
> Please remember to drop these change IDs on patches to the mailing list
> They don't mean anything to us unfortunately!
>

Yes sorry, I will drop it

>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> ---
>>  .../bindings/iio/potentiometer/ds1807.txt          |  17 +++
>>  drivers/iio/potentiometer/Kconfig                  |  10 ++
>>  drivers/iio/potentiometer/Makefile                 |   1 +
>>  drivers/iio/potentiometer/ds1807.c                 | 156 +++++++++++++++++++++
>>  4 files changed, 184 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
>>  create mode 100644 drivers/iio/potentiometer/ds1807.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
>> new file mode 100644
>> index 0000000..3e30f4c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1807.txt
>> @@ -0,0 +1,17 @@
>> +* Maxim Integrated DS1807 digital potentiometer driver
>
> This might be a good candidate for the trivial-device.txt binding file.
>

No, it's not because I have in mind to support:
* zero crossing
* combined channel option using dts (means write them together)

Michael

>> +
>> +The node for this driver must be a child node of a I2C controller, hence
>> +all mandatory properties for your controller must be specified. See directory:
>> +
>> +        Documentation/devicetree/bindings/i2c
>> +
>> +for more details.
>> +
>> +Required properties:
>> +     - compatible: "maxim,ds1807",
>> +
>> +Example:
>> +ds1807: ds1807@1 {
>> +     reg = <0x28>;
>> +     compatible = "maxim,ds1807";
>> +};
>> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
>> index 79ec2eb..92e5a6b 100644
>> --- a/drivers/iio/potentiometer/Kconfig
>> +++ b/drivers/iio/potentiometer/Kconfig
>> @@ -25,6 +25,16 @@ config DS1803
>>         To compile this driver as a module, choose M here: the
>>         module will be called ds1803.
>>
>> +config DS1807
>> +     tristate "Maxim Integrated DS1807 Digital Potentiometer driver"
>> +     depends on I2C
>> +     help
>> +       Say yes here to build support for the Maxim Integrated DS1807
>> +       digital potentiometer chip. Value are reported back in DBm
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called ds1807.
>> +
>>  config MAX5481
>>          tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
>>          depends on SPI
>> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
>> index 4af6578..3c409bb 100644
>> --- a/drivers/iio/potentiometer/Makefile
>> +++ b/drivers/iio/potentiometer/Makefile
>> @@ -6,6 +6,7 @@
>>  # When adding new entries keep the list in alphabetical order
>>  obj-$(CONFIG_AD5272) += ad5272.o
>>  obj-$(CONFIG_DS1803) += ds1803.o
>> +obj-$(CONFIG_DS1807) += ds1807.o
>>  obj-$(CONFIG_MAX5481) += max5481.o
>>  obj-$(CONFIG_MAX5487) += max5487.o
>>  obj-$(CONFIG_MCP4018) += mcp4018.o
>> diff --git a/drivers/iio/potentiometer/ds1807.c b/drivers/iio/potentiometer/ds1807.c
>> new file mode 100644
>> index 0000000..acb4884
>> --- /dev/null
>> +++ b/drivers/iio/potentiometer/ds1807.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Maxim Integrated DS1807 digital potentiometer driver
>> + * Copyright (c) 2018 Michael Trimarchi
>> + *
>> + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1807.pdf
>> + *
>> + * DEVID     #Wipers #Positions      Resistor Opts (kOhm)    i2c address
>> + * ds1807    2       65              45                      0101xxx
>> + *
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +
>> +#define DS1807_MAX_VALUE     64
>> +#define DS1807_MUTE          -90
>> +#define DS1807_WRITE(chan)   (0xa8 | ((chan) + 1))
>> +
>> +#define DS1807_CHANNEL(ch) {                                 \
>> +     .type = IIO_CHAN_INFO_HARDWAREGAIN,                     \
>> +     .indexed = 1,                                           \
>> +     .output = 1,                                            \
>> +     .channel = (ch),                                        \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),  \
>> +}
>> +
>> +static const struct iio_chan_spec ds1807_channels[] = {
>> +     DS1807_CHANNEL(0),
>> +     DS1807_CHANNEL(1),
>> +};
>> +
>> +struct ds1807_data {
>> +     struct i2c_client *client;
>> +};
>> +
>> +static int ds1807_read_raw(struct iio_dev *indio_dev,
>> +                         struct iio_chan_spec const *chan,
>> +                         int *val, int *val2, long mask)
>> +{
>> +     struct ds1807_data *data = iio_priv(indio_dev);
>> +     int pot = chan->channel;
>> +     int ret;
>> +     u8 result[ARRAY_SIZE(ds1807_channels)];
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_HARDWAREGAIN:
>> +             ret = i2c_master_recv(data->client, result,
>> +                             indio_dev->num_channels);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             *val2 = 0;
>> +             if (result[pot] == DS1807_MAX_VALUE)
>> +                     *val = DS1807_MUTE;
>> +             else
>> +                     *val = -result[pot];
>> +
>> +             return IIO_VAL_INT_PLUS_MICRO_DB;
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static int ds1807_write_raw(struct iio_dev *indio_dev,
>> +                          struct iio_chan_spec const *chan,
>> +                          int val, int val2, long mask)
>> +{
>> +     struct ds1807_data *data = iio_priv(indio_dev);
>> +     int pot = chan->channel;
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_HARDWAREGAIN:
>> +             if (val2 != 0 || val < DS1807_MUTE || val > 0)
>> +                     return -EINVAL;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     val = (val == DS1807_MUTE) ? DS1807_MAX_VALUE : -val;
>> +
>> +     return i2c_smbus_write_byte_data(data->client, DS1807_WRITE(pot), val);
>> +}
>> +
>> +static const struct iio_info ds1807_info = {
>> +     .read_raw = ds1807_read_raw,
>> +     .write_raw = ds1807_write_raw,
>> +};
>> +
>> +static int ds1807_probe(struct i2c_client *client,
>> +                      const struct i2c_device_id *id)
>> +{
>> +     struct device *dev = &client->dev;
>> +     struct ds1807_data *data;
>> +     struct iio_dev *indio_dev;
>> +     int channel, ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     i2c_set_clientdata(client, indio_dev);
>> +
>> +     data = iio_priv(indio_dev);
>> +     data->client = client;
>> +
>> +     indio_dev->dev.parent = dev;
>> +     indio_dev->info = &ds1807_info;
>> +     indio_dev->channels = ds1807_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(ds1807_channels);
>> +     indio_dev->name = client->name;
>> +
>> +     /* reset device gain */
>> +     for (channel = 0; channel < indio_dev->num_channels; channel++) {
>> +             ret = i2c_smbus_write_byte_data(client, DS1807_WRITE(channel),
>> +                                             DS1807_MUTE);
>> +             if (ret < 0)
>> +                     return -ENODEV;
>> +     }
>> +
>> +     return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id ds1807_dt_ids[] = {
>> +     { .compatible = "maxim,ds1807", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ds1807_dt_ids);
>> +#endif /* CONFIG_OF */
>> +
>> +static const struct i2c_device_id ds1807_id[] = {
>> +     { "ds1807", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ds1807_id);
>> +
>> +static struct i2c_driver ds1807_driver = {
>> +     .driver = {
>> +             .name   = "ds1807",
>> +             .of_match_table = of_match_ptr(ds1807_dt_ids),
>> +     },
>> +     .probe          = ds1807_probe,
>> +     .id_table       = ds1807_id,
>> +};
>> +
>> +module_i2c_driver(ds1807_driver);
>> +
>> +MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
>> +MODULE_DESCRIPTION("DS1807 digital potentiometer");
>> +MODULE_LICENSE("GPL v2");
>



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Lars-Peter Clausen]

On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
> On Sun,  6 May 2018 15:30:47 +0200
> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> 
>> The following functions are supported:
>> - write, read potentiometer value
>>
>> Value are exported in DBm because it's used as an audio
>> attenuator
> 
> This is interesting.  The problem is that there is no way for
> userspace to know that it is reporting in DBm rather than
> reporting a linear gain or a straight forward resistance.
> 
> This is rather closer in operation to the analog front end
> driver I took the other day than to the other potentiometers
> we have drivers for.
> 
> Anyhow, how to solve this?  Two options come to mind.
> 1) Look up table mapping to linear gain as per current ABI
> 2) Add a new channel type to represent the fact we are
> looking at a logarithmic value, letting us handle it as DB.

Yeah, I guess it is a bit difficult. I don't think this should be a separate
type since we are still describing the same thing, just the scale is
logarithmic rather than linear. Translation table doesn't work either since
your values would get ridiculous small/large. We could add a db suffix to
the type, but that's just terrible. I guess the best we can do is have a
scale attribute that says 1dB.

On the other hand we could ask whether this should be a IIO driver since the
device seems to very application specific for audio. If it is only ever
going to be used in an audio subsystem you'd probably want to make this an
ALSA driver, so you can easily integrate it and don't have to write an
additional bridge driver. ALSA for example has native support for
logarithmic controls.

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Lars-Peter Clausen]

On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
> On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
>> On Sun,  6 May 2018 15:30:47 +0200
>> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>>
>>> The following functions are supported:
>>> - write, read potentiometer value
>>>
>>> Value are exported in DBm because it's used as an audio
>>> attenuator
>>
>> This is interesting.  The problem is that there is no way for
>> userspace to know that it is reporting in DBm rather than
>> reporting a linear gain or a straight forward resistance.
>>
>> This is rather closer in operation to the analog front end
>> driver I took the other day than to the other potentiometers
>> we have drivers for.
>>
>> Anyhow, how to solve this?  Two options come to mind.
>> 1) Look up table mapping to linear gain as per current ABI
>> 2) Add a new channel type to represent the fact we are
>> looking at a logarithmic value, letting us handle it as DB.
> 
> Yeah, I guess it is a bit difficult. I don't think this should be a separate
> type since we are still describing the same thing, just the scale is
> logarithmic rather than linear. Translation table doesn't work either since
> your values would get ridiculous small/large. We could add a db suffix to
> the type, but that's just terrible. I guess the best we can do is have a
> scale attribute that says 1dB.

The other problem of course is that dB is a relative unit. The ratio of one
value to another. Whereas our normal scale refers to an absolute value.

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Mon, 7 May 2018 18:55:16 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:  
> >> On Sun,  6 May 2018 15:30:47 +0200
> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> >>  
> >>> The following functions are supported:
> >>> - write, read potentiometer value
> >>>
> >>> Value are exported in DBm because it's used as an audio
> >>> attenuator  
> >>
> >> This is interesting.  The problem is that there is no way for
> >> userspace to know that it is reporting in DBm rather than
> >> reporting a linear gain or a straight forward resistance.
> >>
> >> This is rather closer in operation to the analog front end
> >> driver I took the other day than to the other potentiometers
> >> we have drivers for.
> >>
> >> Anyhow, how to solve this?  Two options come to mind.
> >> 1) Look up table mapping to linear gain as per current ABI
> >> 2) Add a new channel type to represent the fact we are
> >> looking at a logarithmic value, letting us handle it as DB.  
> > 
> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> > type since we are still describing the same thing, just the scale is
> > logarithmic rather than linear. Translation table doesn't work either since
> > your values would get ridiculous small/large. We could add a db suffix to
> > the type, but that's just terrible. I guess the best we can do is have a
> > scale attribute that says 1dB.  
> 
> The other problem of course is that dB is a relative unit. The ratio of one
> value to another. Whereas our normal scale refers to an absolute value.
I'm really not keen on this.  We have done the separate types
for humidity already, where we had relative (which is a ratio) and absolute
(which isn't).  It's not pretty though.

Potentially we could define a new attribute that says this one is
is db or linear but that's ugly too.

As you asked, are we looking at a part that gets used for anything other
than audio or not?  If just audio, alsa driver does indeed make more sense.

Jonathan

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi

On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 7 May 2018 18:55:16 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
>> >> On Sun,  6 May 2018 15:30:47 +0200
>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>> >>
>> >>> The following functions are supported:
>> >>> - write, read potentiometer value
>> >>>
>> >>> Value are exported in DBm because it's used as an audio
>> >>> attenuator
>> >>
>> >> This is interesting.  The problem is that there is no way for
>> >> userspace to know that it is reporting in DBm rather than
>> >> reporting a linear gain or a straight forward resistance.
>> >>
>> >> This is rather closer in operation to the analog front end
>> >> driver I took the other day than to the other potentiometers
>> >> we have drivers for.
>> >>
>> >> Anyhow, how to solve this?  Two options come to mind.
>> >> 1) Look up table mapping to linear gain as per current ABI
>> >> 2) Add a new channel type to represent the fact we are
>> >> looking at a logarithmic value, letting us handle it as DB.
>> >
>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
>> > type since we are still describing the same thing, just the scale is
>> > logarithmic rather than linear. Translation table doesn't work either since
>> > your values would get ridiculous small/large. We could add a db suffix to
>> > the type, but that's just terrible. I guess the best we can do is have a
>> > scale attribute that says 1dB.
>>
>> The other problem of course is that dB is a relative unit. The ratio of one
>> value to another. Whereas our normal scale refers to an absolute value.
> I'm really not keen on this.  We have done the separate types
> for humidity already, where we had relative (which is a ratio) and absolute
> (which isn't).  It's not pretty though.
>
> Potentially we could define a new attribute that says this one is
> is db or linear but that's ugly too.
>
> As you asked, are we looking at a part that gets used for anything other
> than audio or not?  If just audio, alsa driver does indeed make more sense.
>

This can be used in audio but even in other field. It's just a potentiometer.
Can I know what is wrong to use the same approch of audio ampliefier that we
have already in the iio tree?

Michael

> Jonathan



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi Jonathan


On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
> Hi
>
> On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On Mon, 7 May 2018 18:55:16 +0200
>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
>>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
>>> >> On Sun,  6 May 2018 15:30:47 +0200
>>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>>> >>
>>> >>> The following functions are supported:
>>> >>> - write, read potentiometer value
>>> >>>
>>> >>> Value are exported in DBm because it's used as an audio
>>> >>> attenuator
>>> >>
>>> >> This is interesting.  The problem is that there is no way for
>>> >> userspace to know that it is reporting in DBm rather than
>>> >> reporting a linear gain or a straight forward resistance.
>>> >>
>>> >> This is rather closer in operation to the analog front end
>>> >> driver I took the other day than to the other potentiometers
>>> >> we have drivers for.
>>> >>
>>> >> Anyhow, how to solve this?  Two options come to mind.
>>> >> 1) Look up table mapping to linear gain as per current ABI
>>> >> 2) Add a new channel type to represent the fact we are
>>> >> looking at a logarithmic value, letting us handle it as DB.
>>> >
>>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
>>> > type since we are still describing the same thing, just the scale is
>>> > logarithmic rather than linear. Translation table doesn't work either since
>>> > your values would get ridiculous small/large. We could add a db suffix to
>>> > the type, but that's just terrible. I guess the best we can do is have a
>>> > scale attribute that says 1dB.
>>>
>>> The other problem of course is that dB is a relative unit. The ratio of one
>>> value to another. Whereas our normal scale refers to an absolute value.
>> I'm really not keen on this.  We have done the separate types
>> for humidity already, where we had relative (which is a ratio) and absolute
>> (which isn't).  It's not pretty though.
>>
>> Potentially we could define a new attribute that says this one is
>> is db or linear but that's ugly too.
>>
>> As you asked, are we looking at a part that gets used for anything other
>> than audio or not?  If just audio, alsa driver does indeed make more sense.
>>
>
> This can be used in audio but even in other field. It's just a potentiometer.
> Can I know what is wrong to use the same approch of audio ampliefier that we
> have already in the iio tree?
>

cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
-10.000000 dB
echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain

uname -a
Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux

Michael

> Michael
>
>> Jonathan
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Wed, 9 May 2018 11:19:51 +0200
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi Jonathan
> 
> 
> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> > Hi
> >
> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> >> On Mon, 7 May 2018 18:55:16 +0200
> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>  
> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:  
> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:  
> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> >>> >>  
> >>> >>> The following functions are supported:
> >>> >>> - write, read potentiometer value
> >>> >>>
> >>> >>> Value are exported in DBm because it's used as an audio
> >>> >>> attenuator  
> >>> >>
> >>> >> This is interesting.  The problem is that there is no way for
> >>> >> userspace to know that it is reporting in DBm rather than
> >>> >> reporting a linear gain or a straight forward resistance.
> >>> >>
> >>> >> This is rather closer in operation to the analog front end
> >>> >> driver I took the other day than to the other potentiometers
> >>> >> we have drivers for.
> >>> >>
> >>> >> Anyhow, how to solve this?  Two options come to mind.
> >>> >> 1) Look up table mapping to linear gain as per current ABI
> >>> >> 2) Add a new channel type to represent the fact we are
> >>> >> looking at a logarithmic value, letting us handle it as DB.  
> >>> >
> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> >>> > type since we are still describing the same thing, just the scale is
> >>> > logarithmic rather than linear. Translation table doesn't work either since
> >>> > your values would get ridiculous small/large. We could add a db suffix to
> >>> > the type, but that's just terrible. I guess the best we can do is have a
> >>> > scale attribute that says 1dB.  
> >>>
> >>> The other problem of course is that dB is a relative unit. The ratio of one
> >>> value to another. Whereas our normal scale refers to an absolute value.  
> >> I'm really not keen on this.  We have done the separate types
> >> for humidity already, where we had relative (which is a ratio) and absolute
> >> (which isn't).  It's not pretty though.
> >>
> >> Potentially we could define a new attribute that says this one is
> >> is db or linear but that's ugly too.
> >>
> >> As you asked, are we looking at a part that gets used for anything other
> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> >>  
> >
> > This can be used in audio but even in other field. It's just a potentiometer.
> > Can I know what is wrong to use the same approch of audio ampliefier that we
> > have already in the iio tree?
> >  
> 
> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> -10.000000 dB
> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain

Wow, somehow that entire thing had slipped my mind.  I guess we went
through the whole question of how to support dB scales years ago
and it has just been very little used.

Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
comments that need cleaning up.

It is going to be a little odd as the only potentiometer (I think) that
is acting as a scale free attenuator, rather that being controlled on the basis
of resistance, but for the part that seems to make sense so fair enough.

I'm slightly curious to know what you have this wired up to though?
Are the inputs and outputs invisible to the kernel or is this feeding
into another device?

If we are feeding another device then the work recently done on a
generic AFE driver may be useful.  At somepoint we'll need a version
of that which deals with standalone amplifiers and attenuators anyway,
but I don't know if it is useful to you.

Jonathan

Jonathan


> 
> uname -a
> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> 
> Michael
> 
> > Michael
> >  
> >> Jonathan  
> >
> >
> >
> > --
> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > | COO  -  Founder                                      Cruquiuskade 47 |
> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > |                  [`as] http://www.amarulasolutions.com               |  
> 
> 
> 

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi

On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 9 May 2018 11:19:51 +0200
> Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
>
>> Hi Jonathan
>>
>>
>> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
>> <michael@amarulasolutions.com> wrote:
>> > Hi
>> >
>> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> >> On Mon, 7 May 2018 18:55:16 +0200
>> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
>> >>
>> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
>> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
>> >>> >> On Sun,  6 May 2018 15:30:47 +0200
>> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>> >>> >>
>> >>> >>> The following functions are supported:
>> >>> >>> - write, read potentiometer value
>> >>> >>>
>> >>> >>> Value are exported in DBm because it's used as an audio
>> >>> >>> attenuator
>> >>> >>
>> >>> >> This is interesting.  The problem is that there is no way for
>> >>> >> userspace to know that it is reporting in DBm rather than
>> >>> >> reporting a linear gain or a straight forward resistance.
>> >>> >>
>> >>> >> This is rather closer in operation to the analog front end
>> >>> >> driver I took the other day than to the other potentiometers
>> >>> >> we have drivers for.
>> >>> >>
>> >>> >> Anyhow, how to solve this?  Two options come to mind.
>> >>> >> 1) Look up table mapping to linear gain as per current ABI
>> >>> >> 2) Add a new channel type to represent the fact we are
>> >>> >> looking at a logarithmic value, letting us handle it as DB.
>> >>> >
>> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
>> >>> > type since we are still describing the same thing, just the scale is
>> >>> > logarithmic rather than linear. Translation table doesn't work either since
>> >>> > your values would get ridiculous small/large. We could add a db suffix to
>> >>> > the type, but that's just terrible. I guess the best we can do is have a
>> >>> > scale attribute that says 1dB.
>> >>>
>> >>> The other problem of course is that dB is a relative unit. The ratio of one
>> >>> value to another. Whereas our normal scale refers to an absolute value.
>> >> I'm really not keen on this.  We have done the separate types
>> >> for humidity already, where we had relative (which is a ratio) and absolute
>> >> (which isn't).  It's not pretty though.
>> >>
>> >> Potentially we could define a new attribute that says this one is
>> >> is db or linear but that's ugly too.
>> >>
>> >> As you asked, are we looking at a part that gets used for anything other
>> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
>> >>
>> >
>> > This can be used in audio but even in other field. It's just a potentiometer.
>> > Can I know what is wrong to use the same approch of audio ampliefier that we
>> > have already in the iio tree?
>> >
>>
>> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
>> -10.000000 dB
>> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
>
> Wow, somehow that entire thing had slipped my mind.  I guess we went
> through the whole question of how to support dB scales years ago
> and it has just been very little used.
>
> Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> comments that need cleaning up.
>
> It is going to be a little odd as the only potentiometer (I think) that
> is acting as a scale free attenuator, rather that being controlled on the basis
> of resistance, but for the part that seems to make sense so fair enough.
>
> I'm slightly curious to know what you have this wired up to though?

I'm design GIOTTO ;) an audio module that use those to control the
volume. It's a dsd
native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
Everything already
run under linux. The idea is to create an audio card and connect iio
device to the volume
to change dsd volume

> Are the inputs and outputs invisible to the kernel or is this feeding
> into another device?

I think a reply above. Anyway we don't want to have driver duplication
and I think should be land
there

>
> If we are feeding another device then the work recently done on a
> generic AFE driver may be useful.  At somepoint we'll need a version

Can you point to it? I need to read about ;)

Michael

> of that which deals with standalone amplifiers and attenuators anyway,
> but I don't know if it is useful to you.
>
> Jonathan
>
> Jonathan
>
>
>>
>> uname -a
>> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
>>
>> Michael
>>
>> > Michael
>> >
>> >> Jonathan
>> >
>> >
>> >
>> > --
>> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
>> > | COO  -  Founder                                      Cruquiuskade 47 |
>> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
>> > |                  [`as] http://www.amarulasolutions.com               |
>>
>>
>>
>



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Sat, 12 May 2018 11:50:23 +0200
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi
> 
> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Wed, 9 May 2018 11:19:51 +0200
> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> >  
> >> Hi Jonathan
> >>
> >>
> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> >> <michael@amarulasolutions.com> wrote:  
> >> > Hi
> >> >
> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> >> >> On Mon, 7 May 2018 18:55:16 +0200
> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> >>  
> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:  
> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:  
> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> >> >>> >>  
> >> >>> >>> The following functions are supported:
> >> >>> >>> - write, read potentiometer value
> >> >>> >>>
> >> >>> >>> Value are exported in DBm because it's used as an audio
> >> >>> >>> attenuator  
> >> >>> >>
> >> >>> >> This is interesting.  The problem is that there is no way for
> >> >>> >> userspace to know that it is reporting in DBm rather than
> >> >>> >> reporting a linear gain or a straight forward resistance.
> >> >>> >>
> >> >>> >> This is rather closer in operation to the analog front end
> >> >>> >> driver I took the other day than to the other potentiometers
> >> >>> >> we have drivers for.
> >> >>> >>
> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
> >> >>> >> 2) Add a new channel type to represent the fact we are
> >> >>> >> looking at a logarithmic value, letting us handle it as DB.  
> >> >>> >
> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> >> >>> > type since we are still describing the same thing, just the scale is
> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
> >> >>> > scale attribute that says 1dB.  
> >> >>>
> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
> >> >>> value to another. Whereas our normal scale refers to an absolute value.  
> >> >> I'm really not keen on this.  We have done the separate types
> >> >> for humidity already, where we had relative (which is a ratio) and absolute
> >> >> (which isn't).  It's not pretty though.
> >> >>
> >> >> Potentially we could define a new attribute that says this one is
> >> >> is db or linear but that's ugly too.
> >> >>
> >> >> As you asked, are we looking at a part that gets used for anything other
> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> >> >>  
> >> >
> >> > This can be used in audio but even in other field. It's just a potentiometer.
> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
> >> > have already in the iio tree?
> >> >  
> >>
> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> >> -10.000000 dB
> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain  
> >
> > Wow, somehow that entire thing had slipped my mind.  I guess we went
> > through the whole question of how to support dB scales years ago
> > and it has just been very little used.
> >
> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> > comments that need cleaning up.
> >
> > It is going to be a little odd as the only potentiometer (I think) that
> > is acting as a scale free attenuator, rather that being controlled on the basis
> > of resistance, but for the part that seems to make sense so fair enough.
> >
> > I'm slightly curious to know what you have this wired up to though?  
> 
> I'm design GIOTTO ;) an audio module that use those to control the
> volume. It's a dsd
> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
> Everything already
> run under linux. The idea is to create an audio card and connect iio
> device to the volume
> to change dsd volume
> 
> > Are the inputs and outputs invisible to the kernel or is this feeding
> > into another device?  
> 
> I think a reply above. Anyway we don't want to have driver duplication
> and I think should be land
> there
> 
> >
> > If we are feeding another device then the work recently done on a
> > generic AFE driver may be useful.  At somepoint we'll need a version  
> 
> Can you point to it? I need to read about ;)

https://patchwork.kernel.org/patch/10358131/

Should be in linux-next by now ready for the next merge window.
As it turns out, probably not relevant in your case you will
probably want to have the sound card as a consumer so that
the volume control maps nicely via usual interface etc.

Perhaps cc the relevant sound lists and maintainers on next version.
I don't want to tread on anyone's toes if they are of the view that
it shouldn't be done this way (should be fine from previous conversations
with a few of them!)

Jonathan

> 
> Michael
> 
> > of that which deals with standalone amplifiers and attenuators anyway,
> > but I don't know if it is useful to you.
> >
> > Jonathan
> >
> > Jonathan
> >
> >  
> >>
> >> uname -a
> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> >>
> >> Michael
> >>  
> >> > Michael
> >> >  
> >> >> Jonathan  
> >> >
> >> >
> >> >
> >> > --
> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> >> > | COO  -  Founder                                      Cruquiuskade 47 |
> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> >> > |                  [`as] http://www.amarulasolutions.com               |  
> >>
> >>
> >>  
> >  
> 
> 
> 

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi

On Sat, May 12, 2018 at 1:07 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 12 May 2018 11:50:23 +0200
> Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
>
>> Hi
>>
>> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> > On Wed, 9 May 2018 11:19:51 +0200
>> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
>> >
>> >> Hi Jonathan
>> >>
>> >>
>> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
>> >> <michael@amarulasolutions.com> wrote:
>> >> > Hi
>> >> >
>> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> >> >> On Mon, 7 May 2018 18:55:16 +0200
>> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
>> >> >>
>> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
>> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
>> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
>> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
>> >> >>> >>
>> >> >>> >>> The following functions are supported:
>> >> >>> >>> - write, read potentiometer value
>> >> >>> >>>
>> >> >>> >>> Value are exported in DBm because it's used as an audio
>> >> >>> >>> attenuator
>> >> >>> >>
>> >> >>> >> This is interesting.  The problem is that there is no way for
>> >> >>> >> userspace to know that it is reporting in DBm rather than
>> >> >>> >> reporting a linear gain or a straight forward resistance.
>> >> >>> >>
>> >> >>> >> This is rather closer in operation to the analog front end
>> >> >>> >> driver I took the other day than to the other potentiometers
>> >> >>> >> we have drivers for.
>> >> >>> >>
>> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
>> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
>> >> >>> >> 2) Add a new channel type to represent the fact we are
>> >> >>> >> looking at a logarithmic value, letting us handle it as DB.
>> >> >>> >
>> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
>> >> >>> > type since we are still describing the same thing, just the scale is
>> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
>> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
>> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
>> >> >>> > scale attribute that says 1dB.
>> >> >>>
>> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
>> >> >>> value to another. Whereas our normal scale refers to an absolute value.
>> >> >> I'm really not keen on this.  We have done the separate types
>> >> >> for humidity already, where we had relative (which is a ratio) and absolute
>> >> >> (which isn't).  It's not pretty though.
>> >> >>
>> >> >> Potentially we could define a new attribute that says this one is
>> >> >> is db or linear but that's ugly too.
>> >> >>
>> >> >> As you asked, are we looking at a part that gets used for anything other
>> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
>> >> >>
>> >> >
>> >> > This can be used in audio but even in other field. It's just a potentiometer.
>> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
>> >> > have already in the iio tree?
>> >> >
>> >>
>> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
>> >> -10.000000 dB
>> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
>> >
>> > Wow, somehow that entire thing had slipped my mind.  I guess we went
>> > through the whole question of how to support dB scales years ago
>> > and it has just been very little used.
>> >
>> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
>> > comments that need cleaning up.
>> >
>> > It is going to be a little odd as the only potentiometer (I think) that
>> > is acting as a scale free attenuator, rather that being controlled on the basis
>> > of resistance, but for the part that seems to make sense so fair enough.
>> >
>> > I'm slightly curious to know what you have this wired up to though?
>>
>> I'm design GIOTTO ;) an audio module that use those to control the
>> volume. It's a dsd
>> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
>> Everything already
>> run under linux. The idea is to create an audio card and connect iio
>> device to the volume
>> to change dsd volume
>>
>> > Are the inputs and outputs invisible to the kernel or is this feeding
>> > into another device?
>>
>> I think a reply above. Anyway we don't want to have driver duplication
>> and I think should be land
>> there
>>
>> >
>> > If we are feeding another device then the work recently done on a
>> > generic AFE driver may be useful.  At somepoint we'll need a version
>>
>> Can you point to it? I need to read about ;)
>
> https://patchwork.kernel.org/patch/10358131/
>
> Should be in linux-next by now ready for the next merge window.
> As it turns out, probably not relevant in your case you will
> probably want to have the sound card as a consumer so that
> the volume control maps nicely via usual interface etc.
>
> Perhaps cc the relevant sound lists and maintainers on next version.
> I don't want to tread on anyone's toes if they are of the view that
> it shouldn't be done this way (should be fine from previous conversations
> with a few of them!)

Yes it's a good idea. I will try but I need to move my development
board on latest
kernel.

Michael

>
> Jonathan
>
>>
>> Michael
>>
>> > of that which deals with standalone amplifiers and attenuators anyway,
>> > but I don't know if it is useful to you.
>> >
>> > Jonathan
>> >
>> > Jonathan
>> >
>> >
>> >>
>> >> uname -a
>> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
>> >>
>> >> Michael
>> >>
>> >> > Michael
>> >> >
>> >> >> Jonathan
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
>> >> > | COO  -  Founder                                      Cruquiuskade 47 |
>> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
>> >> > |                  [`as] http://www.amarulasolutions.com               |
>> >>
>> >>
>> >>
>> >
>>
>>
>>
>



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi Jonathan

Very old thread. I have created Giotto Tune based on this design and
now try to attach to ds1807. I have done some hack on the current
interface to have it working in alsa. I have some trouble with the API

  fragment@3 {
                target = <&sound>;
                __overlay__ {
                        compatible = "bcm2708,bcm2708-audio-giotto";
                        i2s-controller = <&i2s>;
                        io-channels = <&volume 0>;
                        #io-channel-cells = <1>;
                        nreset = <&gpio 5 1>;
                        status = "okay";
                };
        };

        fragment@4 {
                target = <&spidev0>;
                __overlay__ {
                        status = "disabled";
                };
        };

        fragment@5 {
                target = <&i2c1>;
                __overlay__ {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        status = "okay";

                        volume: ds1807@1 {
                                compatible = "maxim,ds1807";
                                reg = <0x28>;
                                status = "okay";
                                #io-channel-cells = <1>;
                        };
                };
        };

This is the overlay and this is the change of the API

-static int iio_channel_write(struct iio_channel *chan, int val, int val2,
+static int iio_channel_write(struct iio_channel *chan, int index, int
val, int val2,
                             enum iio_chan_info_enum info)
 {
        return chan->indio_dev->info->write_raw(chan->indio_dev,
-                                               chan->channel, val, val2, info);
+                                               &chan->channel[index],
val, val2, info);
 }

+int iio_write_channel_attribute(struct iio_channel *chan, int index,
int val, int val2,
+                                enum iio_chan_info_enum attribute)
+{
+       int ret;
+
+       mutex_lock(&chan->indio_dev->info_exist_lock);
+       if (chan->indio_dev->info == NULL) {
+               ret = -ENODEV;
+               goto err_unlock;
+       }
+
+       if (index < 0 || index > chan->indio_dev->num_channels)
+               return -EINVAL;
+
+       ret = iio_channel_write(chan, index, val, val2, attribute);
+err_unlock:
+       mutex_unlock(&chan->indio_dev->info_exist_lock);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(iio_write_channel_attribute);

I order to trigger both controls

+       ret = iio_write_channel_attribute(&data->iio_ch[0], 0,
+                                         val, 0,
+                                         IIO_CHAN_INFO_HARDWAREGAIN);
+       if (ret < 0)
+                return ret;
+
+       ret = iio_write_channel_attribute(&data->iio_ch[0], 1,
+                                         val, 0,
+                                         IIO_CHAN_INFO_HARDWAREGAIN);
+       if (ret < 0)
+                return ret;
+

The problem is that we can have one iio_dev and multiple raw value for
each one. Is this the correct way?

Michael

On Sat, May 12, 2018 at 2:10 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi
>
> On Sat, May 12, 2018 at 1:07 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 12 May 2018 11:50:23 +0200
> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> >
> >> Hi
> >>
> >> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> >> > On Wed, 9 May 2018 11:19:51 +0200
> >> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> >> >
> >> >> Hi Jonathan
> >> >>
> >> >>
> >> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> >> >> <michael@amarulasolutions.com> wrote:
> >> >> > Hi
> >> >> >
> >> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> >> >> >> On Mon, 7 May 2018 18:55:16 +0200
> >> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> >> >>
> >> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
> >> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
> >> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> >> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> >> >> >>> >>
> >> >> >>> >>> The following functions are supported:
> >> >> >>> >>> - write, read potentiometer value
> >> >> >>> >>>
> >> >> >>> >>> Value are exported in DBm because it's used as an audio
> >> >> >>> >>> attenuator
> >> >> >>> >>
> >> >> >>> >> This is interesting.  The problem is that there is no way for
> >> >> >>> >> userspace to know that it is reporting in DBm rather than
> >> >> >>> >> reporting a linear gain or a straight forward resistance.
> >> >> >>> >>
> >> >> >>> >> This is rather closer in operation to the analog front end
> >> >> >>> >> driver I took the other day than to the other potentiometers
> >> >> >>> >> we have drivers for.
> >> >> >>> >>
> >> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
> >> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
> >> >> >>> >> 2) Add a new channel type to represent the fact we are
> >> >> >>> >> looking at a logarithmic value, letting us handle it as DB.
> >> >> >>> >
> >> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> >> >> >>> > type since we are still describing the same thing, just the scale is
> >> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
> >> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
> >> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
> >> >> >>> > scale attribute that says 1dB.
> >> >> >>>
> >> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
> >> >> >>> value to another. Whereas our normal scale refers to an absolute value.
> >> >> >> I'm really not keen on this.  We have done the separate types
> >> >> >> for humidity already, where we had relative (which is a ratio) and absolute
> >> >> >> (which isn't).  It's not pretty though.
> >> >> >>
> >> >> >> Potentially we could define a new attribute that says this one is
> >> >> >> is db or linear but that's ugly too.
> >> >> >>
> >> >> >> As you asked, are we looking at a part that gets used for anything other
> >> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> >> >> >>
> >> >> >
> >> >> > This can be used in audio but even in other field. It's just a potentiometer.
> >> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
> >> >> > have already in the iio tree?
> >> >> >
> >> >>
> >> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> >> >> -10.000000 dB
> >> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> >> >
> >> > Wow, somehow that entire thing had slipped my mind.  I guess we went
> >> > through the whole question of how to support dB scales years ago
> >> > and it has just been very little used.
> >> >
> >> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> >> > comments that need cleaning up.
> >> >
> >> > It is going to be a little odd as the only potentiometer (I think) that
> >> > is acting as a scale free attenuator, rather that being controlled on the basis
> >> > of resistance, but for the part that seems to make sense so fair enough.
> >> >
> >> > I'm slightly curious to know what you have this wired up to though?
> >>
> >> I'm design GIOTTO ;) an audio module that use those to control the
> >> volume. It's a dsd
> >> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
> >> Everything already
> >> run under linux. The idea is to create an audio card and connect iio
> >> device to the volume
> >> to change dsd volume
> >>
> >> > Are the inputs and outputs invisible to the kernel or is this feeding
> >> > into another device?
> >>
> >> I think a reply above. Anyway we don't want to have driver duplication
> >> and I think should be land
> >> there
> >>
> >> >
> >> > If we are feeding another device then the work recently done on a
> >> > generic AFE driver may be useful.  At somepoint we'll need a version
> >>
> >> Can you point to it? I need to read about ;)
> >
> > https://patchwork.kernel.org/patch/10358131/
> >
> > Should be in linux-next by now ready for the next merge window.
> > As it turns out, probably not relevant in your case you will
> > probably want to have the sound card as a consumer so that
> > the volume control maps nicely via usual interface etc.
> >
> > Perhaps cc the relevant sound lists and maintainers on next version.
> > I don't want to tread on anyone's toes if they are of the view that
> > it shouldn't be done this way (should be fine from previous conversations
> > with a few of them!)
>
> Yes it's a good idea. I will try but I need to move my development
> board on latest
> kernel.
>
> Michael
>
> >
> > Jonathan
> >
> >>
> >> Michael
> >>
> >> > of that which deals with standalone amplifiers and attenuators anyway,
> >> > but I don't know if it is useful to you.
> >> >
> >> > Jonathan
> >> >
> >> > Jonathan
> >> >
> >> >
> >> >>
> >> >> uname -a
> >> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> >> >>
> >> >> Michael
> >> >>
> >> >> > Michael
> >> >> >
> >> >> >> Jonathan
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> >> >> > | COO  -  Founder                                      Cruquiuskade 47 |
> >> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> >> >> > |                  [`as] http://www.amarulasolutions.com               |
> >> >>
> >> >>
> >> >>
> >> >
> >>
> >>
> >>
> >
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Sat, 23 Feb 2019 10:04:31 +0100
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi Jonathan
> 
> Very old thread. I have created Giotto Tune based on this design and
> now try to attach to ds1807. I have done some hack on the current
> interface to have it working in alsa. I have some trouble with the API
> 
>   fragment@3 {
>                 target = <&sound>;
>                 __overlay__ {
>                         compatible = "bcm2708,bcm2708-audio-giotto";
>                         i2s-controller = <&i2s>;
>                         io-channels = <&volume 0>;

This is a list, not just one.

>                         #io-channel-cells = <1>;
>                         nreset = <&gpio 5 1>;
>                         status = "okay";
>                 };
>         };
> 
>         fragment@4 {
>                 target = <&spidev0>;
>                 __overlay__ {
>                         status = "disabled";
>                 };
>         };
> 
>         fragment@5 {
>                 target = <&i2c1>;
>                 __overlay__ {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         status = "okay";
> 
>                         volume: ds1807@1 {
>                                 compatible = "maxim,ds1807";
>                                 reg = <0x28>;
>                                 status = "okay";
>                                 #io-channel-cells = <1>;
>                         };
>                 };
>         };
> 
> This is the overlay and this is the change of the API
> 
> -static int iio_channel_write(struct iio_channel *chan, int val, int val2,
> +static int iio_channel_write(struct iio_channel *chan, int index, int
> val, int val2,
>                              enum iio_chan_info_enum info)
>  {
>         return chan->indio_dev->info->write_raw(chan->indio_dev,
> -                                               chan->channel, val, val2, info);
> +                                               &chan->channel[index],
> val, val2, info);
>  }
> 
> +int iio_write_channel_attribute(struct iio_channel *chan, int index,
> int val, int val2,
> +                                enum iio_chan_info_enum attribute)
> +{
> +       int ret;
> +
> +       mutex_lock(&chan->indio_dev->info_exist_lock);
> +       if (chan->indio_dev->info == NULL) {
> +               ret = -ENODEV;
> +               goto err_unlock;
> +       }
> +
> +       if (index < 0 || index > chan->indio_dev->num_channels)
> +               return -EINVAL;
> +
> +       ret = iio_channel_write(chan, index, val, val2, attribute);
> +err_unlock:
> +       mutex_unlock(&chan->indio_dev->info_exist_lock);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_write_channel_attribute);
> 
> I order to trigger both controls
> 
> +       ret = iio_write_channel_attribute(&data->iio_ch[0], 0,
> +                                         val, 0,
> +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> +       if (ret < 0)
> +                return ret;
> +
> +       ret = iio_write_channel_attribute(&data->iio_ch[0], 1,
> +                                         val, 0,
> +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> +       if (ret < 0)
> +                return ret;
> +
> 
> The problem is that we can have one iio_dev and multiple raw value for
> each one. 

Absolutely we can, but you need to register multiple channels and get them
all, not just one and then try and build off that.

> Is this the correct way?

No.  You need to have multiple channels listed, not just the one.

There are examples in Documentation/bindings/iio/iio-bindings.txt

I may be misunderstanding what you are trying to do.

Jonathan


> 
> Michael
> 
> On Sat, May 12, 2018 at 2:10 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi
> >
> > On Sat, May 12, 2018 at 1:07 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Sat, 12 May 2018 11:50:23 +0200
> > > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > >  
> > >> Hi
> > >>
> > >> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > >> > On Wed, 9 May 2018 11:19:51 +0200
> > >> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > >> >  
> > >> >> Hi Jonathan
> > >> >>
> > >> >>
> > >> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> > >> >> <michael@amarulasolutions.com> wrote:  
> > >> >> > Hi
> > >> >> >
> > >> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > >> >> >> On Mon, 7 May 2018 18:55:16 +0200
> > >> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> > >> >> >>  
> > >> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:  
> > >> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:  
> > >> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> > >> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> > >> >> >>> >>  
> > >> >> >>> >>> The following functions are supported:
> > >> >> >>> >>> - write, read potentiometer value
> > >> >> >>> >>>
> > >> >> >>> >>> Value are exported in DBm because it's used as an audio
> > >> >> >>> >>> attenuator  
> > >> >> >>> >>
> > >> >> >>> >> This is interesting.  The problem is that there is no way for
> > >> >> >>> >> userspace to know that it is reporting in DBm rather than
> > >> >> >>> >> reporting a linear gain or a straight forward resistance.
> > >> >> >>> >>
> > >> >> >>> >> This is rather closer in operation to the analog front end
> > >> >> >>> >> driver I took the other day than to the other potentiometers
> > >> >> >>> >> we have drivers for.
> > >> >> >>> >>
> > >> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
> > >> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
> > >> >> >>> >> 2) Add a new channel type to represent the fact we are
> > >> >> >>> >> looking at a logarithmic value, letting us handle it as DB.  
> > >> >> >>> >
> > >> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> > >> >> >>> > type since we are still describing the same thing, just the scale is
> > >> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
> > >> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
> > >> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
> > >> >> >>> > scale attribute that says 1dB.  
> > >> >> >>>
> > >> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
> > >> >> >>> value to another. Whereas our normal scale refers to an absolute value.  
> > >> >> >> I'm really not keen on this.  We have done the separate types
> > >> >> >> for humidity already, where we had relative (which is a ratio) and absolute
> > >> >> >> (which isn't).  It's not pretty though.
> > >> >> >>
> > >> >> >> Potentially we could define a new attribute that says this one is
> > >> >> >> is db or linear but that's ugly too.
> > >> >> >>
> > >> >> >> As you asked, are we looking at a part that gets used for anything other
> > >> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> > >> >> >>  
> > >> >> >
> > >> >> > This can be used in audio but even in other field. It's just a potentiometer.
> > >> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
> > >> >> > have already in the iio tree?
> > >> >> >  
> > >> >>
> > >> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> > >> >> -10.000000 dB
> > >> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain  
> > >> >
> > >> > Wow, somehow that entire thing had slipped my mind.  I guess we went
> > >> > through the whole question of how to support dB scales years ago
> > >> > and it has just been very little used.
> > >> >
> > >> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> > >> > comments that need cleaning up.
> > >> >
> > >> > It is going to be a little odd as the only potentiometer (I think) that
> > >> > is acting as a scale free attenuator, rather that being controlled on the basis
> > >> > of resistance, but for the part that seems to make sense so fair enough.
> > >> >
> > >> > I'm slightly curious to know what you have this wired up to though?  
> > >>
> > >> I'm design GIOTTO ;) an audio module that use those to control the
> > >> volume. It's a dsd
> > >> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
> > >> Everything already
> > >> run under linux. The idea is to create an audio card and connect iio
> > >> device to the volume
> > >> to change dsd volume
> > >>  
> > >> > Are the inputs and outputs invisible to the kernel or is this feeding
> > >> > into another device?  
> > >>
> > >> I think a reply above. Anyway we don't want to have driver duplication
> > >> and I think should be land
> > >> there
> > >>  
> > >> >
> > >> > If we are feeding another device then the work recently done on a
> > >> > generic AFE driver may be useful.  At somepoint we'll need a version  
> > >>
> > >> Can you point to it? I need to read about ;)  
> > >
> > > https://patchwork.kernel.org/patch/10358131/
> > >
> > > Should be in linux-next by now ready for the next merge window.
> > > As it turns out, probably not relevant in your case you will
> > > probably want to have the sound card as a consumer so that
> > > the volume control maps nicely via usual interface etc.
> > >
> > > Perhaps cc the relevant sound lists and maintainers on next version.
> > > I don't want to tread on anyone's toes if they are of the view that
> > > it shouldn't be done this way (should be fine from previous conversations
> > > with a few of them!)  
> >
> > Yes it's a good idea. I will try but I need to move my development
> > board on latest
> > kernel.
> >
> > Michael
> >  
> > >
> > > Jonathan
> > >  
> > >>
> > >> Michael
> > >>  
> > >> > of that which deals with standalone amplifiers and attenuators anyway,
> > >> > but I don't know if it is useful to you.
> > >> >
> > >> > Jonathan
> > >> >
> > >> > Jonathan
> > >> >
> > >> >  
> > >> >>
> > >> >> uname -a
> > >> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> > >> >>
> > >> >> Michael
> > >> >>  
> > >> >> > Michael
> > >> >> >  
> > >> >> >> Jonathan  
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> > --
> > >> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > >> >> > | COO  -  Founder                                      Cruquiuskade 47 |
> > >> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > >> >> > |                  [`as] http://www.amarulasolutions.com               |  
> > >> >>
> > >> >>
> > >> >>  
> > >> >  
> > >>
> > >>
> > >>  
> > >  
> >
> >
> >
> > --
> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > | COO  -  Founder                                      Cruquiuskade 47 |
> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > |                  [`as] http://www.amarulasolutions.com               |  
> 
> 
> 

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Michael Nazzareno Trimarchi]

Hi Jonathan

On Sun, Mar 3, 2019 at 6:12 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 23 Feb 2019 10:04:31 +0100
> Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
>
> > Hi Jonathan
> >
> > Very old thread. I have created Giotto Tune based on this design and
> > now try to attach to ds1807. I have done some hack on the current
> > interface to have it working in alsa. I have some trouble with the API
> >
> >   fragment@3 {
> >                 target = <&sound>;
> >                 __overlay__ {
> >                         compatible = "bcm2708,bcm2708-audio-giotto";
> >                         i2s-controller = <&i2s>;
> >                         io-channels = <&volume 0>;
>
> This is a list, not just one.

with the driver I have tried <&volume 1> <&volume 0>;
>
> >                         #io-channel-cells = <1>;
> >                         nreset = <&gpio 5 1>;
> >                         status = "okay";
> >                 };
> >         };
> >
> >         fragment@4 {
> >                 target = <&spidev0>;
> >                 __overlay__ {
> >                         status = "disabled";
> >                 };
> >         };
> >
> >         fragment@5 {
> >                 target = <&i2c1>;
> >                 __overlay__ {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         status = "okay";
> >
> >                         volume: ds1807@1 {
> >                                 compatible = "maxim,ds1807";
> >                                 reg = <0x28>;
> >                                 status = "okay";
> >                                 #io-channel-cells = <1>;
> >                         };
> >                 };
> >         };
> >
> > This is the overlay and this is the change of the API
> >
> > -static int iio_channel_write(struct iio_channel *chan, int val, int val2,
> > +static int iio_channel_write(struct iio_channel *chan, int index, int
> > val, int val2,
> >                              enum iio_chan_info_enum info)
> >  {
> >         return chan->indio_dev->info->write_raw(chan->indio_dev,
> > -                                               chan->channel, val, val2, info);
> > +                                               &chan->channel[index],
> > val, val2, info);
> >  }
> >
> > +int iio_write_channel_attribute(struct iio_channel *chan, int index,
> > int val, int val2,
> > +                                enum iio_chan_info_enum attribute)
> > +{
> > +       int ret;
> > +
> > +       mutex_lock(&chan->indio_dev->info_exist_lock);
> > +       if (chan->indio_dev->info == NULL) {
> > +               ret = -ENODEV;
> > +               goto err_unlock;
> > +       }
> > +
> > +       if (index < 0 || index > chan->indio_dev->num_channels)

You mean that num_channels is taken in account when you request each of us.
Can one channel has multiple raw output?

> > +               return -EINVAL;
> > +
> > +       ret = iio_channel_write(chan, index, val, val2, attribute);
> > +err_unlock:
> > +       mutex_unlock(&chan->indio_dev->info_exist_lock);
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_write_channel_attribute);
> >
> > I order to trigger both controls
> >
> > +       ret = iio_write_channel_attribute(&data->iio_ch[0], 0,
> > +                                         val, 0,
> > +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> > +       if (ret < 0)
> > +                return ret;
> > +
> > +       ret = iio_write_channel_attribute(&data->iio_ch[0], 1,
> > +                                         val, 0,
> > +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> > +       if (ret < 0)
> > +                return ret;
> > +
> >
> > The problem is that we can have one iio_dev and multiple raw value for
> > each one.
>
> Absolutely we can, but you need to register multiple channels and get them
> all, not just one and then try and build off that.

Ok I need to check back again

>
> > Is this the correct way?
>
> No.  You need to have multiple channels listed, not just the one.
>
> There are examples in Documentation/bindings/iio/iio-bindings.txt
>

I have followed it but not with expected result. I will try to go in a
mainline kernel

Michael
> I may be misunderstanding what you are trying to do.
>
> Jonathan
>
>
> >
> > Michael
> >
> > On Sat, May 12, 2018 at 2:10 PM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi
> > >
> > > On Sat, May 12, 2018 at 1:07 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > > > On Sat, 12 May 2018 11:50:23 +0200
> > > > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > > >
> > > >> Hi
> > > >>
> > > >> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> > > >> > On Wed, 9 May 2018 11:19:51 +0200
> > > >> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > > >> >
> > > >> >> Hi Jonathan
> > > >> >>
> > > >> >>
> > > >> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> > > >> >> <michael@amarulasolutions.com> wrote:
> > > >> >> > Hi
> > > >> >> >
> > > >> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > > >> >> >> On Mon, 7 May 2018 18:55:16 +0200
> > > >> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > >> >> >>
> > > >> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:
> > > >> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:
> > > >> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> > > >> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> > > >> >> >>> >>
> > > >> >> >>> >>> The following functions are supported:
> > > >> >> >>> >>> - write, read potentiometer value
> > > >> >> >>> >>>
> > > >> >> >>> >>> Value are exported in DBm because it's used as an audio
> > > >> >> >>> >>> attenuator
> > > >> >> >>> >>
> > > >> >> >>> >> This is interesting.  The problem is that there is no way for
> > > >> >> >>> >> userspace to know that it is reporting in DBm rather than
> > > >> >> >>> >> reporting a linear gain or a straight forward resistance.
> > > >> >> >>> >>
> > > >> >> >>> >> This is rather closer in operation to the analog front end
> > > >> >> >>> >> driver I took the other day than to the other potentiometers
> > > >> >> >>> >> we have drivers for.
> > > >> >> >>> >>
> > > >> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
> > > >> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
> > > >> >> >>> >> 2) Add a new channel type to represent the fact we are
> > > >> >> >>> >> looking at a logarithmic value, letting us handle it as DB.
> > > >> >> >>> >
> > > >> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> > > >> >> >>> > type since we are still describing the same thing, just the scale is
> > > >> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
> > > >> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
> > > >> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
> > > >> >> >>> > scale attribute that says 1dB.
> > > >> >> >>>
> > > >> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
> > > >> >> >>> value to another. Whereas our normal scale refers to an absolute value.
> > > >> >> >> I'm really not keen on this.  We have done the separate types
> > > >> >> >> for humidity already, where we had relative (which is a ratio) and absolute
> > > >> >> >> (which isn't).  It's not pretty though.
> > > >> >> >>
> > > >> >> >> Potentially we could define a new attribute that says this one is
> > > >> >> >> is db or linear but that's ugly too.
> > > >> >> >>
> > > >> >> >> As you asked, are we looking at a part that gets used for anything other
> > > >> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> > > >> >> >>
> > > >> >> >
> > > >> >> > This can be used in audio but even in other field. It's just a potentiometer.
> > > >> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
> > > >> >> > have already in the iio tree?
> > > >> >> >
> > > >> >>
> > > >> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> > > >> >> -10.000000 dB
> > > >> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> > > >> >
> > > >> > Wow, somehow that entire thing had slipped my mind.  I guess we went
> > > >> > through the whole question of how to support dB scales years ago
> > > >> > and it has just been very little used.
> > > >> >
> > > >> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> > > >> > comments that need cleaning up.
> > > >> >
> > > >> > It is going to be a little odd as the only potentiometer (I think) that
> > > >> > is acting as a scale free attenuator, rather that being controlled on the basis
> > > >> > of resistance, but for the part that seems to make sense so fair enough.
> > > >> >
> > > >> > I'm slightly curious to know what you have this wired up to though?
> > > >>
> > > >> I'm design GIOTTO ;) an audio module that use those to control the
> > > >> volume. It's a dsd
> > > >> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
> > > >> Everything already
> > > >> run under linux. The idea is to create an audio card and connect iio
> > > >> device to the volume
> > > >> to change dsd volume
> > > >>
> > > >> > Are the inputs and outputs invisible to the kernel or is this feeding
> > > >> > into another device?
> > > >>
> > > >> I think a reply above. Anyway we don't want to have driver duplication
> > > >> and I think should be land
> > > >> there
> > > >>
> > > >> >
> > > >> > If we are feeding another device then the work recently done on a
> > > >> > generic AFE driver may be useful.  At somepoint we'll need a version
> > > >>
> > > >> Can you point to it? I need to read about ;)
> > > >
> > > > https://patchwork.kernel.org/patch/10358131/
> > > >
> > > > Should be in linux-next by now ready for the next merge window.
> > > > As it turns out, probably not relevant in your case you will
> > > > probably want to have the sound card as a consumer so that
> > > > the volume control maps nicely via usual interface etc.
> > > >
> > > > Perhaps cc the relevant sound lists and maintainers on next version.
> > > > I don't want to tread on anyone's toes if they are of the view that
> > > > it shouldn't be done this way (should be fine from previous conversations
> > > > with a few of them!)
> > >
> > > Yes it's a good idea. I will try but I need to move my development
> > > board on latest
> > > kernel.
> > >
> > > Michael
> > >
> > > >
> > > > Jonathan
> > > >
> > > >>
> > > >> Michael
> > > >>
> > > >> > of that which deals with standalone amplifiers and attenuators anyway,
> > > >> > but I don't know if it is useful to you.
> > > >> >
> > > >> > Jonathan
> > > >> >
> > > >> > Jonathan
> > > >> >
> > > >> >
> > > >> >>
> > > >> >> uname -a
> > > >> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> > > >> >>
> > > >> >> Michael
> > > >> >>
> > > >> >> > Michael
> > > >> >> >
> > > >> >> >> Jonathan
> > > >> >> >
> > > >> >> >
> > > >> >> >
> > > >> >> > --
> > > >> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > >> >> > | COO  -  Founder                                      Cruquiuskade 47 |
> > > >> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > >> >> > |                  [`as] http://www.amarulasolutions.com               |
> > > >> >>
> > > >> >>
> > > >> >>
> > > >> >
> > > >>
> > > >>
> > > >>
> > > >
> > >
> > >
> > >
> > > --
> > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > |                  [`as] http://www.amarulasolutions.com               |
> >
> >
> >
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Jonathan Cameron]

On Mon, 4 Mar 2019 10:27:13 +0100
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi Jonathan
> 
> On Sun, Mar 3, 2019 at 6:12 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 23 Feb 2019 10:04:31 +0100
> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> >  
> > > Hi Jonathan
> > >
> > > Very old thread. I have created Giotto Tune based on this design and
> > > now try to attach to ds1807. I have done some hack on the current
> > > interface to have it working in alsa. I have some trouble with the API
> > >
> > >   fragment@3 {
> > >                 target = <&sound>;
> > >                 __overlay__ {
> > >                         compatible = "bcm2708,bcm2708-audio-giotto";
> > >                         i2s-controller = <&i2s>;
> > >                         io-channels = <&volume 0>;  
> >
> > This is a list, not just one.  
> 
> with the driver I have tried <&volume 1> <&volume 0>;
> >  
> > >                         #io-channel-cells = <1>;
> > >                         nreset = <&gpio 5 1>;
> > >                         status = "okay";
> > >                 };
> > >         };
> > >
> > >         fragment@4 {
> > >                 target = <&spidev0>;
> > >                 __overlay__ {
> > >                         status = "disabled";
> > >                 };
> > >         };
> > >
> > >         fragment@5 {
> > >                 target = <&i2c1>;
> > >                 __overlay__ {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > >                         status = "okay";
> > >
> > >                         volume: ds1807@1 {
> > >                                 compatible = "maxim,ds1807";
> > >                                 reg = <0x28>;
> > >                                 status = "okay";
> > >                                 #io-channel-cells = <1>;
> > >                         };
> > >                 };
> > >         };
> > >
> > > This is the overlay and this is the change of the API
> > >
> > > -static int iio_channel_write(struct iio_channel *chan, int val, int val2,
> > > +static int iio_channel_write(struct iio_channel *chan, int index, int
> > > val, int val2,
> > >                              enum iio_chan_info_enum info)
> > >  {
> > >         return chan->indio_dev->info->write_raw(chan->indio_dev,
> > > -                                               chan->channel, val, val2, info);
> > > +                                               &chan->channel[index],
> > > val, val2, info);
> > >  }
> > >
> > > +int iio_write_channel_attribute(struct iio_channel *chan, int index,
> > > int val, int val2,
> > > +                                enum iio_chan_info_enum attribute)
> > > +{
> > > +       int ret;
> > > +
> > > +       mutex_lock(&chan->indio_dev->info_exist_lock);
> > > +       if (chan->indio_dev->info == NULL) {
> > > +               ret = -ENODEV;
> > > +               goto err_unlock;
> > > +       }
> > > +
> > > +       if (index < 0 || index > chan->indio_dev->num_channels)  
> 
> You mean that num_channels is taken in account when you request each of us.
> Can one channel has multiple raw output?

No. A channel is basically a wire on the side of the chip. It can't
have multiple values at the same time.

> 
> > > +               return -EINVAL;
> > > +
> > > +       ret = iio_channel_write(chan, index, val, val2, attribute);
> > > +err_unlock:
> > > +       mutex_unlock(&chan->indio_dev->info_exist_lock);
> > > +
> > > +       return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_write_channel_attribute);
> > >
> > > I order to trigger both controls
> > >
> > > +       ret = iio_write_channel_attribute(&data->iio_ch[0], 0,
> > > +                                         val, 0,
> > > +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> > > +       if (ret < 0)
> > > +                return ret;
> > > +
> > > +       ret = iio_write_channel_attribute(&data->iio_ch[0], 1,
> > > +                                         val, 0,
> > > +                                         IIO_CHAN_INFO_HARDWAREGAIN);
> > > +       if (ret < 0)
> > > +                return ret;
> > > +
> > >
> > > The problem is that we can have one iio_dev and multiple raw value for
> > > each one.  
> >
> > Absolutely we can, but you need to register multiple channels and get them
> > all, not just one and then try and build off that.  
> 
> Ok I need to check back again
> 
> >  
> > > Is this the correct way?  
> >
> > No.  You need to have multiple channels listed, not just the one.
> >
> > There are examples in Documentation/bindings/iio/iio-bindings.txt
> >  
> 
> I have followed it but not with expected result. I will try to go in a
> mainline kernel
> 

Cool. Let us know if you don't get anywhere.

Jonathan

> Michael
> > I may be misunderstanding what you are trying to do.
> >
> > Jonathan
> >
> >  
> > >
> > > Michael
> > >
> > > On Sat, May 12, 2018 at 2:10 PM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> wrote:  
> > > >
> > > > Hi
> > > >
> > > > On Sat, May 12, 2018 at 1:07 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > > On Sat, 12 May 2018 11:50:23 +0200
> > > > > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > > > >  
> > > > >> Hi
> > > > >>
> > > > >> On Sat, May 12, 2018 at 11:45 AM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > >> > On Wed, 9 May 2018 11:19:51 +0200
> > > > >> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > > > >> >  
> > > > >> >> Hi Jonathan
> > > > >> >>
> > > > >> >>
> > > > >> >> On Wed, May 9, 2018 at 11:01 AM, Michael Nazzareno Trimarchi
> > > > >> >> <michael@amarulasolutions.com> wrote:  
> > > > >> >> > Hi
> > > > >> >> >
> > > > >> >> > On Mon, May 7, 2018 at 7:17 PM, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > >> >> >> On Mon, 7 May 2018 18:55:16 +0200
> > > > >> >> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > >> >> >>  
> > > > >> >> >>> On 05/07/2018 06:44 PM, Lars-Peter Clausen wrote:  
> > > > >> >> >>> > On 05/06/2018 07:37 PM, Jonathan Cameron wrote:  
> > > > >> >> >>> >> On Sun,  6 May 2018 15:30:47 +0200
> > > > >> >> >>> >> Michael Trimarchi <michael@amarulasolutions.com> wrote:
> > > > >> >> >>> >>  
> > > > >> >> >>> >>> The following functions are supported:
> > > > >> >> >>> >>> - write, read potentiometer value
> > > > >> >> >>> >>>
> > > > >> >> >>> >>> Value are exported in DBm because it's used as an audio
> > > > >> >> >>> >>> attenuator  
> > > > >> >> >>> >>
> > > > >> >> >>> >> This is interesting.  The problem is that there is no way for
> > > > >> >> >>> >> userspace to know that it is reporting in DBm rather than
> > > > >> >> >>> >> reporting a linear gain or a straight forward resistance.
> > > > >> >> >>> >>
> > > > >> >> >>> >> This is rather closer in operation to the analog front end
> > > > >> >> >>> >> driver I took the other day than to the other potentiometers
> > > > >> >> >>> >> we have drivers for.
> > > > >> >> >>> >>
> > > > >> >> >>> >> Anyhow, how to solve this?  Two options come to mind.
> > > > >> >> >>> >> 1) Look up table mapping to linear gain as per current ABI
> > > > >> >> >>> >> 2) Add a new channel type to represent the fact we are
> > > > >> >> >>> >> looking at a logarithmic value, letting us handle it as DB.  
> > > > >> >> >>> >
> > > > >> >> >>> > Yeah, I guess it is a bit difficult. I don't think this should be a separate
> > > > >> >> >>> > type since we are still describing the same thing, just the scale is
> > > > >> >> >>> > logarithmic rather than linear. Translation table doesn't work either since
> > > > >> >> >>> > your values would get ridiculous small/large. We could add a db suffix to
> > > > >> >> >>> > the type, but that's just terrible. I guess the best we can do is have a
> > > > >> >> >>> > scale attribute that says 1dB.  
> > > > >> >> >>>
> > > > >> >> >>> The other problem of course is that dB is a relative unit. The ratio of one
> > > > >> >> >>> value to another. Whereas our normal scale refers to an absolute value.  
> > > > >> >> >> I'm really not keen on this.  We have done the separate types
> > > > >> >> >> for humidity already, where we had relative (which is a ratio) and absolute
> > > > >> >> >> (which isn't).  It's not pretty though.
> > > > >> >> >>
> > > > >> >> >> Potentially we could define a new attribute that says this one is
> > > > >> >> >> is db or linear but that's ugly too.
> > > > >> >> >>
> > > > >> >> >> As you asked, are we looking at a part that gets used for anything other
> > > > >> >> >> than audio or not?  If just audio, alsa driver does indeed make more sense.
> > > > >> >> >>  
> > > > >> >> >
> > > > >> >> > This can be used in audio but even in other field. It's just a potentiometer.
> > > > >> >> > Can I know what is wrong to use the same approch of audio ampliefier that we
> > > > >> >> > have already in the iio tree?
> > > > >> >> >  
> > > > >> >>
> > > > >> >> cat /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain
> > > > >> >> -10.000000 dB
> > > > >> >> echo -10 > /sys/bus/iio/devices/iio:device1/out_altvoltage0_hardwaregain  
> > > > >> >
> > > > >> > Wow, somehow that entire thing had slipped my mind.  I guess we went
> > > > >> > through the whole question of how to support dB scales years ago
> > > > >> > and it has just been very little used.
> > > > >> >
> > > > >> > Hmm. Sorry for my absent mindedness! Anyhow, there are a few additional
> > > > >> > comments that need cleaning up.
> > > > >> >
> > > > >> > It is going to be a little odd as the only potentiometer (I think) that
> > > > >> > is acting as a scale free attenuator, rather that being controlled on the basis
> > > > >> > of resistance, but for the part that seems to make sense so fair enough.
> > > > >> >
> > > > >> > I'm slightly curious to know what you have this wired up to though?  
> > > > >>
> > > > >> I'm design GIOTTO ;) an audio module that use those to control the
> > > > >> volume. It's a dsd
> > > > >> native sound card that demultiplex i2s to L and R dsd on a pcm1795a.
> > > > >> Everything already
> > > > >> run under linux. The idea is to create an audio card and connect iio
> > > > >> device to the volume
> > > > >> to change dsd volume
> > > > >>  
> > > > >> > Are the inputs and outputs invisible to the kernel or is this feeding
> > > > >> > into another device?  
> > > > >>
> > > > >> I think a reply above. Anyway we don't want to have driver duplication
> > > > >> and I think should be land
> > > > >> there
> > > > >>  
> > > > >> >
> > > > >> > If we are feeding another device then the work recently done on a
> > > > >> > generic AFE driver may be useful.  At somepoint we'll need a version  
> > > > >>
> > > > >> Can you point to it? I need to read about ;)  
> > > > >
> > > > > https://patchwork.kernel.org/patch/10358131/
> > > > >
> > > > > Should be in linux-next by now ready for the next merge window.
> > > > > As it turns out, probably not relevant in your case you will
> > > > > probably want to have the sound card as a consumer so that
> > > > > the volume control maps nicely via usual interface etc.
> > > > >
> > > > > Perhaps cc the relevant sound lists and maintainers on next version.
> > > > > I don't want to tread on anyone's toes if they are of the view that
> > > > > it shouldn't be done this way (should be fine from previous conversations
> > > > > with a few of them!)  
> > > >
> > > > Yes it's a good idea. I will try but I need to move my development
> > > > board on latest
> > > > kernel.
> > > >
> > > > Michael
> > > >  
> > > > >
> > > > > Jonathan
> > > > >  
> > > > >>
> > > > >> Michael
> > > > >>  
> > > > >> > of that which deals with standalone amplifiers and attenuators anyway,
> > > > >> > but I don't know if it is useful to you.
> > > > >> >
> > > > >> > Jonathan
> > > > >> >
> > > > >> > Jonathan
> > > > >> >
> > > > >> >  
> > > > >> >>
> > > > >> >> uname -a
> > > > >> >> Linux linaro-alip 4.4.93 #7 SMP Sun May 6 13:23:08 CEST 2018 armv7l GNU/Linux
> > > > >> >>
> > > > >> >> Michael
> > > > >> >>  
> > > > >> >> > Michael
> > > > >> >> >  
> > > > >> >> >> Jonathan  
> > > > >> >> >
> > > > >> >> >
> > > > >> >> >
> > > > >> >> > --
> > > > >> >> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > >> >> > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > >> >> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > >> >> > |                  [`as] http://www.amarulasolutions.com               |  
> > > > >> >>
> > > > >> >>
> > > > >> >>  
> > > > >> >  
> > > > >>
> > > > >>
> > > > >>  
> > > > >  
> > > >
> > > >
> > > >
> > > > --
> > > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > |                  [`as] http://www.amarulasolutions.com               |  
> > >
> > >
> > >  
> >  
> 
> 

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Lars-Peter Clausen]

On 05/06/2018 03:30 PM, Michael Trimarchi wrote:
[..]
> +static int ds1807_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ds1807_data *data = iio_priv(indio_dev);
> +	int pot = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		if (val2 != 0 || val < DS1807_MUTE || val > 0)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val = (val == DS1807_MUTE) ? DS1807_MAX_VALUE : -val;

Writing anything between -63 and -90 will cause a bogus value to be written
to the register. Might want to change this to

if (val < -DS1807_MAX_VALUE)
	val = -DS1807_MAX_VALUE;

So anything smaller than -63dB gets rounded down to the mute setting.

> +
> +	return i2c_smbus_write_byte_data(data->client, DS1807_WRITE(pot), val);
> +}
> +
> +static const struct iio_info ds1807_info = {
> +	.read_raw = ds1807_read_raw,
> +	.write_raw = ds1807_write_raw,
> +};
> +
> +static int ds1807_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct ds1807_data *data;
> +	struct iio_dev *indio_dev;
> +	int channel, ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);

clientdata never seems to be used, so this could be removed.

> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &ds1807_info;
> +	indio_dev->channels = ds1807_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ds1807_channels);
> +	indio_dev->name = client->name;
> +
> +	/* reset device gain */
> +	for (channel = 0; channel < indio_dev->num_channels; channel++) {
> +		ret = i2c_smbus_write_byte_data(client, DS1807_WRITE(channel),
> +						DS1807_MUTE);
You could use the write both channels at the same time feature here and get
rid of the loop.

> +		if (ret < 0)
> +			return -ENODEV;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1807

[Lars-Peter Clausen]

On 05/06/2018 03:30 PM, Michael Trimarchi wrote:

> +#define DS1807_CHANNEL(ch) {					\
> +	.type = IIO_CHAN_INFO_HARDWAREGAIN,			\

I don't think this is a channel type.

> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (ch),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),	\
> +}