[PATCH] iio: add ds1077 programmable oscillator driver

[PATCH] iio: add ds1077 programmable oscillator driver

[Peter Meerwald]

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/frequency/Kconfig        |  10 +
 drivers/iio/frequency/Makefile       |   1 +
 drivers/iio/frequency/ds1077.c       | 425 +++++++++++++++++++++++++++++++++++
 include/linux/iio/frequency/ds1077.h |  22 ++
 4 files changed, 458 insertions(+)
 create mode 100644 drivers/iio/frequency/ds1077.c
 create mode 100644 include/linux/iio/frequency/ds1077.h

diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 6aaa33e..f1cdb15 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -7,6 +7,16 @@
 
 menu "Frequency Synthesizers DDS/PLL"
 
+config DS1077
+	tristate "Maxim DS1077 Programmable Fixed-Frequency Oscillator"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Maxim DS1077
+	  Programmable Fixed-Frequency Oscillator.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ds1077.
+
 menu "Clock Generator/Distribution"
 
 config AD9523
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 00d26e5..095dcad 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_AD9523) += ad9523.o
 obj-$(CONFIG_ADF4350) += adf4350.o
+obj-$(CONFIG_DS1077) += ds1077.o
diff --git a/drivers/iio/frequency/ds1077.c b/drivers/iio/frequency/ds1077.c
new file mode 100644
index 0000000..a0bc532
--- /dev/null
+++ b/drivers/iio/frequency/ds1077.c
@@ -0,0 +1,425 @@
+/*
+ * ds1077.c - Support for Maxim DS1077 programmable fixed-frequency
+ * oscillator
+ *
+ * Copyright 2013 Peter Meerwald <pmeerw@pmeerw.net>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * IIO driver for the DS1077 with 7-bit I2C slave address 0x58
+ *
+ * Driver can optionally use two GPIOs specified via platform data; these
+ * allow to enable/disable output 1 and to enter power-down mode (TODO).
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/frequency/ds1077.h>
+
+#define DS1077_DRV_NAME "ds1077"
+
+#define DS1077_DIV 0x01
+#define DS1077_MUX 0x02
+#define DS1077_BUS 0x0d
+#define DS1077_E2 0x3f
+
+#define DS1077_DIV1_MASK 0x0040
+#define DS1077_M1_MASK 0x0180
+#define DS1077_M1_SHIFT 7
+#define DS1077_M0_MASK 0x0600
+#define DS1077_M0_SHIFT 9
+#define DS1077_N1_MASK 0xffc0
+#define DS1077_N1_SHIFT 6
+#define DS1077_EN0_MASK 0x0800
+#define DS1077_SEL0_MASK 0x1000
+#define DS1077_PDN0_MASK 0x2000
+#define DS1077_PDN1_MASK 0x4000
+#define DS1077_WC_MASK 0x08
+
+#define DS1077_DIV1(data) ((data)->mux & DS1077_DIV1_MASK)
+#define DS1077_M1(data) (((data)->mux & DS1077_M1_MASK) >> DS1077_M1_SHIFT)
+#define DS1077_M0(data) (((data)->mux & DS1077_M0_MASK) >> DS1077_M0_SHIFT)
+#define DS1077_N1(data) ((data)->div >> DS1077_N1_SHIFT)
+#define DS1077_EN0(data) ((data)->mux & DS1077_EN0_MASK)
+#define DS1077_SEL0(data) ((data)->mux & DS1077_SEL0_MASK)
+#define DS1077_PDN0(data) ((data)->mux & DS1077_PDN0_MASK)
+#define DS1077_PDN1(data) ((data)->mux & DS1077_PDN1_MASK)
+
+#define DS1077_FREQDIFF(f, m, d) ((div) ? abs((long) ((f) - (m)/(d))) : (m))
+
+struct ds1077_data {
+	struct i2c_client *client;
+	struct ds1077_platform_data *pdata;
+	struct mutex lock;
+	unsigned long master_freq;
+	bool en0;
+	bool en1;
+	bool pdn;
+	int gpio_ctrl0;
+	int gpio_ctrl1;
+	u16 mux;
+	u16 div;
+};
+
+static unsigned long ds1077_master_freq[] = {
+	133333000,
+	125000000,
+	120000000,
+	100000000,
+	66666000
+};
+
+static const struct i2c_device_id ds1077_id[] = {
+	{ "ds1077-133", 0 },
+	{ "ds1077-125", 1 },
+	{ "ds1077-120", 2 },
+	{ "ds1077-100", 3 },
+	{ "ds1077-66", 4 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ds1077_id);
+
+static int ds1077_store_eeprom(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ds1077_data *data = iio_priv(indio_dev);
+	bool state;
+	int ret;
+
+	ret = strtobool(buf, &state);
+	if (ret < 0)
+		return ret;
+
+	if (!state)
+		return 0;
+
+	ret = i2c_smbus_write_byte(data->client, DS1077_E2);
+
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(store_eeprom, S_IWUSR, NULL, ds1077_store_eeprom, 0);
+
+static struct attribute *ds1077_attributes[] = {
+	&iio_dev_attr_store_eeprom.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ds1077_attribute_group = {
+	.attrs = ds1077_attributes,
+};
+
+static const struct iio_chan_spec ds1077_channels[] = {
+	{
+		.type = IIO_ALTVOLTAGE,
+		.output = 1,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
+		.type = IIO_ALTVOLTAGE,
+		.output = 1,
+		.indexed = 1,
+		.channel = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static int ds1077_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	int ret = -EINVAL;
+	struct ds1077_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->channel == 0) {
+			*val = data->en0;
+			return IIO_VAL_INT;
+		} else if (chan->channel == 1) {
+			*val = data->en1;
+			return IIO_VAL_INT;
+		}
+		break;
+	case IIO_CHAN_INFO_FREQUENCY:
+		if (chan->channel == 0) {
+			*val = data->master_freq / (1 << DS1077_M0(data));
+			return IIO_VAL_INT;
+		} else if (chan->channel == 1) {
+			*val = data->master_freq / (1 << DS1077_M1(data)) /
+			    (DS1077_DIV1(data) ? 1 : (DS1077_N1(data) + 2));
+			return IIO_VAL_INT;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static unsigned ds1077_find_prescaler(unsigned long mf, unsigned long f)
+{
+	unsigned long p = (mf + mf/2) / f;
+	unsigned m = 0;
+
+	if (p >= 8)
+		m = 3;
+	else if (p >= 4)
+		m = 2;
+	else if (p >= 2)
+		m = 1;
+
+	return m;
+}
+
+static void ds1077_find_divider(struct ds1077_data *data, unsigned long f)
+{
+	unsigned m = 0, n = 0;
+
+	unsigned long div = data->master_freq / f;
+	if (DS1077_FREQDIFF(f, data->master_freq, div) >
+	    DS1077_FREQDIFF(f, data->master_freq, div + 1))
+		div++;
+	if (div <= 1)
+		data->mux |= DS1077_DIV1_MASK;
+	else {
+		data->mux &= ~DS1077_DIV1_MASK;
+
+		m = ds1077_find_prescaler(div, 1025);
+		div /= (1 << m);
+
+		if (div >= 2)
+			n = min(div - 2, 1023ul);
+	}
+
+	data->div = n << DS1077_N1_SHIFT;
+	data->mux = (data->mux & ~(DS1077_M1_MASK)) |
+		(m << DS1077_M1_SHIFT);
+}
+
+static int ds1077_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	int ret = -EINVAL;
+	struct ds1077_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->channel == 0) {
+			if (!!val == data->en0)
+				return 0;
+			if (val)
+				data->mux |= DS1077_EN0_MASK |
+					DS1077_SEL0_MASK;
+			else
+				data->mux &= ~(DS1077_EN0_MASK |
+					DS1077_SEL0_MASK);
+			ret = i2c_smbus_write_word_data(data->client,
+				DS1077_MUX, cpu_to_be16(data->mux));
+			if (ret < 0)
+				return ret;
+			data->en0 = !data->en0;
+			return 0;
+		} else if (chan->channel == 1) {
+			if (!!val == data->en1)
+				return 0;
+			if (!data->pdata ||
+				!gpio_is_valid(data->pdata->gpio_ctrl1))
+				goto out;
+			gpio_set_value(data->pdata->gpio_ctrl1, !val);
+			data->en1 = !data->en1;
+			return 0;
+		}
+		break;
+	case IIO_CHAN_INFO_FREQUENCY:
+		if (val <= 0 || val > data->master_freq)
+			goto out;
+
+		if (chan->channel == 0) {
+			u16 m0 = ds1077_find_prescaler(data->master_freq,
+				val);
+			data->mux = (data->mux & ~DS1077_M0_MASK) |
+				(m0 << DS1077_M0_SHIFT);
+		} else if (chan->channel == 1)
+			ds1077_find_divider(data, val);
+
+		mutex_lock(&data->lock);
+		ret = i2c_smbus_write_word_data(data->client, DS1077_MUX,
+			cpu_to_be16(data->mux));
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_smbus_write_word_data(data->client, DS1077_DIV,
+			cpu_to_be16(data->div));
+		if (ret < 0)
+			return ret;
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		break;
+	}
+
+out:
+	return ret;
+}
+
+static const struct iio_info ds1077_info = {
+	.read_raw = ds1077_read_raw,
+	.write_raw = ds1077_write_raw,
+	.attrs = &ds1077_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+static int ds1077_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct ds1077_platform_data *pdata = client->dev.platform_data;
+	struct ds1077_data *data;
+	struct iio_dev *indio_dev;
+	u8 bus;
+	int ret;
+
+	indio_dev = iio_device_alloc(sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->pdata = pdata;
+	mutex_init(&data->lock);
+
+	data->master_freq = ds1077_master_freq[id->driver_data];
+
+	ret = i2c_smbus_read_word_data(client, DS1077_MUX);
+	if (ret < 0)
+		goto error_free_dev;
+	data->mux = be16_to_cpu(ret);
+
+	if (DS1077_PDN0(data) || DS1077_PDN1(data)) {
+		dev_err(&client->dev, "unsupported power-down muxing");
+		goto error_free_dev;
+	}
+
+	if ((!DS1077_EN0(data) && DS1077_SEL0(data)) ||
+		(DS1077_EN0(data) && !DS1077_SEL0(data))) {
+		dev_err(&client->dev, "unsupported mode muxing");
+		goto error_free_dev;
+	}
+
+	ret = i2c_smbus_read_word_data(client, DS1077_DIV);
+	if (ret < 0)
+		goto error_free_dev;
+	data->div = be16_to_cpu(ret);
+
+	if (pdata) {
+		if (gpio_is_valid(pdata->gpio_ctrl0)) {
+			ret = gpio_request(pdata->gpio_ctrl0, "ds1077 ctrl0");
+			if (ret) {
+				dev_err(&client->dev, "failed to request ctrl0 GPIO%d",
+					pdata->gpio_ctrl0);
+				goto error_free_gpio0;
+			}
+			gpio_direction_output(pdata->gpio_ctrl0, 0);
+		}
+
+		if (gpio_is_valid(pdata->gpio_ctrl1)) {
+			ret = gpio_request(pdata->gpio_ctrl0, "ds1077 ctrl1");
+			if (ret) {
+				dev_err(&client->dev, "failed to request ctrl1 GPIO%d",
+					pdata->gpio_ctrl1);
+				goto error_free_gpio1;
+			}
+			gpio_direction_output(pdata->gpio_ctrl1, pdata->ctrl1);
+		}
+	}
+
+	/* disable automatic EEPROM update on write */
+	ret = i2c_smbus_read_byte_data(client, DS1077_BUS);
+	if (ret < 0)
+		goto error_free_gpio1;
+	bus = ret;
+	if (!(bus & DS1077_WC_MASK)) {
+		ret = i2c_smbus_write_byte_data(client, DS1077_BUS,
+			bus | DS1077_WC_MASK);
+		if (ret < 0)
+			goto error_free_gpio1;
+	}
+
+	data->en0 = DS1077_EN0(data);
+	data->en1 = pdata ? !pdata->ctrl1 : true;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &ds1077_info;
+	indio_dev->channels = ds1077_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ds1077_channels);
+	indio_dev->name = DS1077_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		goto error_free_gpio1;
+
+	dev_info(&client->dev, "%s registered\n", id->name);
+
+	return 0;
+
+error_free_gpio1:
+	if (pdata && gpio_is_valid(pdata->gpio_ctrl1))
+		gpio_free(pdata->gpio_ctrl1);
+error_free_gpio0:
+	if (pdata && gpio_is_valid(pdata->gpio_ctrl0))
+		gpio_free(pdata->gpio_ctrl0);
+error_free_dev:
+	iio_device_free(indio_dev);
+	return ret;
+}
+
+static int ds1077_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ds1077_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	if (data->pdata) {
+		if (gpio_is_valid(data->pdata->gpio_ctrl1))
+			gpio_free(data->pdata->gpio_ctrl1);
+		if (gpio_is_valid(data->pdata->gpio_ctrl0))
+			gpio_free(data->pdata->gpio_ctrl0);
+	}
+	iio_device_free(indio_dev);
+
+	return 0;
+}
+
+static struct i2c_driver ds1077_driver = {
+	.driver = {
+		.name   = DS1077_DRV_NAME,
+		.owner  = THIS_MODULE,
+	},
+	.probe  = ds1077_probe,
+	.remove = ds1077_remove,
+	.id_table = ds1077_id,
+};
+
+module_i2c_driver(ds1077_driver);
+
+MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
+MODULE_DESCRIPTION("Maxim DS1077 programmable fixed-frequency oscillator driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/frequency/ds1077.h b/include/linux/iio/frequency/ds1077.h
new file mode 100644
index 0000000..1dcb41e
--- /dev/null
+++ b/include/linux/iio/frequency/ds1077.h
@@ -0,0 +1,22 @@
+/*
+ * ds1077.h - Support for Maxim DS1077 programmable fixed-frequency
+ * oscillator
+ *
+ * Copyright 2013 Peter Meerwald <pmeerw@pmeerw.net>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ */
+
+#ifndef IIO_FREQUENCY_DS1077_H_
+#define IIO_FREQUENCY_DS1077_H_
+
+struct ds1077_platform_data {
+	int gpio_ctrl0;
+	int gpio_ctrl1;
+	bool ctrl1;
+};
+
+#endif /* IIO_FREQUENCY_DS1077_H_ */
+
-- 
1.8.3.1

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Lars-Peter Clausen]

On 06/22/2013 11:54 PM, Peter Meerwald wrote:
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>

Hm, I wonder where we should draw the line between what should be
implemented as a IIO driver and what should be implemented as a clk API
driver. This one looks like it actually belongs into the clk framework.

> ---
>  drivers/iio/frequency/Kconfig        |  10 +
>  drivers/iio/frequency/Makefile       |   1 +
>  drivers/iio/frequency/ds1077.c       | 425 +++++++++++++++++++++++++++++++++++
>  include/linux/iio/frequency/ds1077.h |  22 ++
>  4 files changed, 458 insertions(+)
>  create mode 100644 drivers/iio/frequency/ds1077.c
>  create mode 100644 include/linux/iio/frequency/ds1077.h
> 
[...}

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Peter Meerwald]

Hello,

> Hm, I wonder where we should draw the line between what should be
> implemented as a IIO driver and what should be implemented as a clk API
> driver. This one looks like it actually belongs into the clk framework.

several IIO drivers have overlap with other kernel subsystems

I think the 'line' depends on the intended use/application of the driver, 
not so much on characteristics of the hardware; why do you think it 
belongs to clk?

the ds1077 is a small, separate chip which can generate a frequency; using 
IIO I can easily control that frequency from userspace

clk seems to be targetted more at integrated clocksources that get 
activated automatically when needed by other components (maybe I am wrong)

regards, p.

> > ---
> >  drivers/iio/frequency/Kconfig        |  10 +
> >  drivers/iio/frequency/Makefile       |   1 +
> >  drivers/iio/frequency/ds1077.c       | 425 +++++++++++++++++++++++++++++++++++
> >  include/linux/iio/frequency/ds1077.h |  22 ++
> >  4 files changed, 458 insertions(+)
> >  create mode 100644 drivers/iio/frequency/ds1077.c
> >  create mode 100644 include/linux/iio/frequency/ds1077.h
> > 
> [...}
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Lars-Peter Clausen]

On 06/24/2013 07:47 PM, Peter Meerwald wrote:
> Hello,
> 
>> Hm, I wonder where we should draw the line between what should be
>> implemented as a IIO driver and what should be implemented as a clk API
>> driver. This one looks like it actually belongs into the clk framework.
> 
> several IIO drivers have overlap with other kernel subsystems

Which is not a necessarily good.

> 
> I think the 'line' depends on the intended use/application of the driver, 
> not so much on characteristics of the hardware; why do you think it 
> belongs to clk?

But the usecase might differ from board to board and that's when you get a
problem. One user wants a clk driver another a IIO driver.

> 
> the ds1077 is a small, separate chip which can generate a frequency; using 
> IIO I can easily control that frequency from userspace
> 
> clk seems to be targetted more at integrated clocksources that get 
> activated automatically when needed by other components (maybe I am wrong)


I think there is a userspace consumer for the clk API in the making.

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Jonathan Cameron]

On 06/24/2013 06:57 PM, Lars-Peter Clausen wrote:
> On 06/24/2013 07:47 PM, Peter Meerwald wrote:
>> Hello,
>>
>>> Hm, I wonder where we should draw the line between what should be
>>> implemented as a IIO driver and what should be implemented as a clk API
>>> driver. This one looks like it actually belongs into the clk framework.
>>
>> several IIO drivers have overlap with other kernel subsystems
> 
> Which is not a necessarily good.
> 
>>
>> I think the 'line' depends on the intended use/application of the driver, 
>> not so much on characteristics of the hardware; why do you think it 
>> belongs to clk?
> 
> But the usecase might differ from board to board and that's when you get a
> problem. One user wants a clk driver another a IIO driver.
> 
>>
>> the ds1077 is a small, separate chip which can generate a frequency; using 
>> IIO I can easily control that frequency from userspace
>>
>> clk seems to be targetted more at integrated clocksources that get 
>> activated automatically when needed by other components (maybe I am wrong)
> 
> 
> I think there is a userspace consumer for the clk API in the making.

Just to jump on the end of this conversation, sorry Peter but this one definitely
looks to me like it belongs clk rather than IIO. If there wasn't a userspace
API in the making, I'd suggest now was the time to propose one as you
clearly have an application for one.

Jonathan

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Peter Meerwald]

Hello,

> > But the usecase might differ from board to board and that's when you get a
> > problem. One user wants a clk driver another a IIO driver.

so because eventually someone might want a clk driver it is preferrable to 
have no driver at all?

> >> the ds1077 is a small, separate chip which can generate a frequency; using 
> >> IIO I can easily control that frequency from userspace
> >>
> >> clk seems to be targetted more at integrated clocksources that get 
> >> activated automatically when needed by other components (maybe I am wrong)

> > I think there is a userspace consumer for the clk API in the making.

are you referring to this? https://patchwork.kernel.org/patch/2551831/
I'm trying to figure out the status...

> Just to jump on the end of this conversation, sorry Peter but this one definitely
> looks to me like it belongs clk rather than IIO. If there wasn't a userspace
> API in the making, I'd suggest now was the time to propose one as you
> clearly have an application for one.

fine; so far no reasons were given
does that mean iio frequency is deprecated?
is there a fundamental difference between hardware supported in iio 
frequency and the ds1077?

thanks, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Jonathan Cameron]

On 06/30/2013 06:48 AM, Peter Meerwald wrote:
> Hello,
> 
>>> But the usecase might differ from board to board and that's when you get a
>>> problem. One user wants a clk driver another a IIO driver.
> 
> so because eventually someone might want a clk driver it is preferrable to 
> have no driver at all?
> 
>>>> the ds1077 is a small, separate chip which can generate a frequency; using 
>>>> IIO I can easily control that frequency from userspace
>>>>
>>>> clk seems to be targetted more at integrated clocksources that get 
>>>> activated automatically when needed by other components (maybe I am wrong)
> 
>>> I think there is a userspace consumer for the clk API in the making.
> 
> are you referring to this? https://patchwork.kernel.org/patch/2551831/
> I'm trying to figure out the status...
No idea on this I'm afraid, Lars?
> 
>> Just to jump on the end of this conversation, sorry Peter but this one definitely
>> looks to me like it belongs clk rather than IIO. If there wasn't a userspace
>> API in the making, I'd suggest now was the time to propose one as you
>> clearly have an application for one.
> 
> fine; so far no reasons were given
> does that mean iio frequency is deprecated?

DDS devices definitely don't fit in clk (though they are all still in staging atm).
Perhaps for these types of parts but as your driver has highlighted there is always
a blurred line between IIO and various other subsystems when deciding where to put
a driver.  Note the next bit is for less familiar readers! Mostly
it goes with the primary function of the driver, unless someone has a usecase that
requires functionality that makes no sense in the obvious host subsystem.

Hence ADC drivers directed at hardware monitoring usually go in the direction of hwmon,
but if there is a desire for very fast reading, then hwmon is inappropriate.
Note that for hwmon we of course have the bridge driver iio-hwmon which allows us
to provide hwmon functionality from an IIO driver.

For input historically Dmitry refused all accelerometers on the basis that at
the time these were not really being used for human input.  Later, he has decided
(with support from many including me) to take drivers for devices that were
very much input directed (3D accelerometers with features like double tap detection).
However, Dmitry has also been very much a driver of the (admittedly stalled) work
on iio-input with a view that these accelerometers in particular are used for other
purposes.

So as you see it is a bit fuzzy at best, and sometimes I find myself deliberating
for some time on whether to push a driver in another direction from IIO or not.
I agreed to the existing frequency drivers on the basis that they didn't fit well
elsewhere, but if there is now a userspace clock interface in the works, perhaps
it is time to revisit these.

> is there a fundamental difference between hardware supported in iio 
> frequency and the ds1077?

Not a clear one, no.  The ds1077 to my reading is a very simple example of a programable
clock but ultimately I 'think' the other two are similar if more complex beasts with
perhaps rather different target uses. Note that whether they should go in IIO has was
raised at merge time for them. Honestly I'm more than a little lost in the relevant datasheets. I can verify the code is
good without really have a clue what they are for ;)
http://marc.info/?l=linux-iio&m=132352819207242&w=2


Michael, do you think it is time to revisit whether these belong in IIO or could be
handled fully using the clk subsystem now?  I 'think' it might now be rather more
flexible than we last visited this.

Sorry for the slow response, I fully admit I was hoping someone else with clearer
views on this would jump in and give a clear argument one way or the other!

If there is a blured edge region where things don't fit in clk but do provide
functionality that does, then perhaps we need to do a similar job to iio-hwmon?
This might be true for some of the DDS parts which can be used as clocks, but
would be a very expensive way of doing that.

Jonathan

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Peter Meerwald]

Hello Jonathan,

> >>>> the ds1077 is a small, separate chip which can generate a frequency; using 
> >>>> IIO I can easily control that frequency from userspace
> >>>>
> >>>> clk seems to be targetted more at integrated clocksources that get 
> >>>> activated automatically when needed by other components (maybe I am wrong)
> > 
> >>> I think there is a userspace consumer for the clk API in the making.
> > 
> > are you referring to this? https://patchwork.kernel.org/patch/2551831/
> > I'm trying to figure out the status...
> No idea on this I'm afraid, Lars?

I've been in contact with the person who proposed above patch (Soren);
the proposal was rejected and he is not following up on it to get 
it mainline due to lack of time/interest, it is going to be used in his 
vendor tree only

> >> Just to jump on the end of this conversation, sorry Peter but this one definitely
> >> looks to me like it belongs clk rather than IIO. If there wasn't a userspace
> >> API in the making, I'd suggest now was the time to propose one as you
> >> clearly have an application for one.
> > 
> > fine; so far no reasons were given
> > does that mean iio frequency is deprecated?

> So as you see it is a bit fuzzy at best, and sometimes I find myself deliberating
> for some time on whether to push a driver in another direction from IIO or not.
> I agreed to the existing frequency drivers on the basis that they didn't fit well
> elsewhere, but if there is now a userspace clock interface in the works, perhaps
> it is time to revisit these.

thank you for the background information

> > is there a fundamental difference between hardware supported in iio 
> > frequency and the ds1077?

> Not a clear one, no.  The ds1077 to my reading is a very simple example of a programable
> clock but ultimately I 'think' the other two are similar if more complex beasts with
> perhaps rather different target uses. Note that whether they should go in IIO has was
> raised at merge time for them. Honestly I'm more than a little lost in the relevant datasheets. I can verify the code is

ok, understood

> If there is a blured edge region where things don't fit in clk but do provide
> functionality that does, then perhaps we need to do a similar job to iio-hwmon?
> This might be true for some of the DDS parts which can be used as clocks, but
> would be a very expensive way of doing that.

I'll keep looking at clk to see what's going on there (if anything :)

thanks, regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH] iio: add ds1077 programmable oscillator driver

[Jonathan Cameron]

On 07/02/2013 09:49 PM, Peter Meerwald wrote:
> Hello Jonathan,
> 
>>>>>> the ds1077 is a small, separate chip which can generate a frequency; using 
>>>>>> IIO I can easily control that frequency from userspace
>>>>>>
>>>>>> clk seems to be targetted more at integrated clocksources that get 
>>>>>> activated automatically when needed by other components (maybe I am wrong)
>>>
>>>>> I think there is a userspace consumer for the clk API in the making.
>>>
>>> are you referring to this? https://patchwork.kernel.org/patch/2551831/
>>> I'm trying to figure out the status...
>> No idea on this I'm afraid, Lars?
> 
> I've been in contact with the person who proposed above patch (Soren);
> the proposal was rejected and he is not following up on it to get 
> it mainline due to lack of time/interest, it is going to be used in his 
> vendor tree only
I've just had a read of the thread. I can see why he gave up on it given
the resistence.

So what is your usecase for this chip that isn't just providing a clock to
some other on board component? (and hence a case where userspace control is
necessary)

> 
>>>> Just to jump on the end of this conversation, sorry Peter but this one definitely
>>>> looks to me like it belongs clk rather than IIO. If there wasn't a userspace
>>>> API in the making, I'd suggest now was the time to propose one as you
>>>> clearly have an application for one.
>>>
>>> fine; so far no reasons were given
>>> does that mean iio frequency is deprecated?
> 
>> So as you see it is a bit fuzzy at best, and sometimes I find myself deliberating
>> for some time on whether to push a driver in another direction from IIO or not.
>> I agreed to the existing frequency drivers on the basis that they didn't fit well
>> elsewhere, but if there is now a userspace clock interface in the works, perhaps
>> it is time to revisit these.
> 
> thank you for the background information
> 
>>> is there a fundamental difference between hardware supported in iio 
>>> frequency and the ds1077?
> 
>> Not a clear one, no.  The ds1077 to my reading is a very simple example of a programable
>> clock but ultimately I 'think' the other two are similar if more complex beasts with
>> perhaps rather different target uses. Note that whether they should go in IIO has was
>> raised at merge time for them. Honestly I'm more than a little lost in the relevant datasheets. I can verify the code is
> 
> ok, understood
> 
>> If there is a blured edge region where things don't fit in clk but do provide
>> functionality that does, then perhaps we need to do a similar job to iio-hwmon?
>> This might be true for some of the DDS parts which can be used as clocks, but
>> would be a very expensive way of doing that.
> 
> I'll keep looking at clk to see what's going on there (if anything :)
> 
> thanks, regards, p.
>