2014-10-15 20:03:51

by Octavian Purdila

[permalink] [raw]
Subject: [PATH v3 0/4] i2c: show and change bus frequency via sysfs

This patch series adds support to show and change the bus frequency
via sysfs, by exposing files to show the minimum, maximum and current
frequency as well as allowing the frequency to be changed. This allows
the user to view or change the bus frequency on a per bus level.

Changes since v3:

* use DEVICE_ATTR_RO and make i2c_adapter_visible_attr static

* implement sysfs bus frequency support for the diolan u2c bus

Octavian Purdila (4):
i2c: document the existing i2c sysfs ABI
i2c: document struct i2c_adapter
i2c: show and change bus frequency via sysfs
i2c: i2c-diolan-u2c: sysfs bus frequency support

Documentation/ABI/testing/sysfs-bus-i2c | 75 +++++++++++++++++++++++++++
drivers/i2c/busses/i2c-diolan-u2c.c | 49 ++++++++++++------
drivers/i2c/i2c-core.c | 90 +++++++++++++++++++++++++++++++++
include/linux/i2c.h | 32 ++++++++++--
4 files changed, 228 insertions(+), 18 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

--
1.9.1


2014-10-15 20:03:54

by Octavian Purdila

[permalink] [raw]
Subject: [PATH v3 1/4] i2c: document the existing i2c sysfs ABI

This patch adds Documentation/ABI/testing/sysfs-bus-i2c which
documents the existing i2c sysfs ABI.

Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-i2c | 45 +++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
new file mode 100644
index 0000000..8075585
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -0,0 +1,45 @@
+What: /sys/bus/i2c/devices/i2c-X
+KernelVersion: since at least 2.6.12
+Contact: [email protected]
+Description:
+ This entry represents a registered i2c bus. X is the
+ bus number and its format is "%d".
+
+What: /sys/bus/i2c/devices/i2c-X/Y
+What: /sys/bus/i2c/devices/Y
+KernelVersion: since at least 2.6.12
+Contact: [email protected]
+Description:
+ An i2c device attached to bus X. Format of Y is
+ "%d-%04x" where the first number is the bus number (X)
+ and the second number is the device i2c address.
+
+What: /sys/bus/i2c/devices/i2c-X/new_device
+KernelVersion: 2.6.31
+Contact: [email protected]
+Description:
+ Write only entry that allows instantiating a
+ new i2c device on bus X. This is to be used when
+ enumeration mechanism such as ACPI or DT are not
+ present or not used for this device.
+ Format: "%s %hi\n" where the first argument is the
+ device name (no spaces allowed) and the second is the
+ i2c address of the device.
+
+What: /sys/bus/i2c/devices/i2c-X/delete_device
+KernelVersion: 2.6.31
+Contact: [email protected]
+Description:
+ Write only entry that allows the removal of an i2c
+ device from bus X.
+ Format: "%s %hi\n" where the first argument is the
+ device name (no spaces allowed) and the second is the
+ i2c address of the device.
+
+What: /sys/bus/i2c/devices/i2c-X/i2c-Y
+What: /sys/bus/i2c/devices/i2c-Y
+KernelVersion: 3.13
+Contact: [email protected]
+Description:
+ An i2c device attached to bus X that is enumerated via
+ ACPI. Y is the ACPI device name and its format is "%s".
--
1.9.1

2014-10-15 20:03:59

by Octavian Purdila

[permalink] [raw]
Subject: [PATH v3 3/4] i2c: show and change bus frequency via sysfs

This patch adds three new sysfs files: frequency, frequency_min and
frequency_max which allows the user to view or change the bus
frequency on a per bus level.

This is required for the case where multiple busses have the same
adapter driver in which case a module parameter does not allow
controlling the bus speed individually (e.g. USB I2C adapters).

Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-i2c | 30 +++++++++++
drivers/i2c/i2c-core.c | 90 +++++++++++++++++++++++++++++++++
include/linux/i2c.h | 16 ++++++
3 files changed, 136 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
index 8075585..70af81f 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -43,3 +43,33 @@ Contact: [email protected]
Description:
An i2c device attached to bus X that is enumerated via
ACPI. Y is the ACPI device name and its format is "%s".
+
+What: /sys/bus/i2c/devices/i2c-X/frequency
+KernelVersion: 3.19
+Contact: [email protected]
+Description:
+ The current bus frequency for bus X. Can be updated if
+ the bus supports it. The unit is HZ and format is
+ "%d\n".
+ If the bus does not support changing the frequency
+ then this entry will be read-only.
+ If the bus does not support showing the frequency than
+ this entry will not be visible.
+ When updating the bus frequency that value must be in
+ the range defined by bus_frequency_min and
+ bus_frequency_max otherwise writing to this entry will
+ fail with -EINVAL.
+ The bus may not support all of the frequencies in the
+ min-max range and may round the frequency to the
+ closest supported one. The user must read the entry
+ after writing it to retrieve the actual set frequency.
+
+What: /sys/bus/i2c/devices/i2c-X/frequency_min
+What: /sys/bus/i2c/devices/i2c-X/frequency_max
+KernelVersion: 3.19
+Contact: [email protected]
+Description:
+ Minimum and maximum frequencies for bus X. The unit is
+ HZ and format is "%d\n".
+ If the bus does not support changing the frequency
+ these entries will not be visible.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 632057a..27c29eb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -940,15 +940,97 @@ static DEVICE_ATTR(new_device, S_IWUSR, NULL, i2c_sysfs_new_device);
static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, S_IWUSR, NULL,
i2c_sysfs_delete_device);

+static ssize_t
+i2c_sysfs_freq_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", adap->freq);
+}
+
+static ssize_t
+i2c_sysfs_freq_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct i2c_adapter *adap = to_i2c_adapter(dev);
+ unsigned int freq;
+ int ret;
+
+ if (kstrtouint(buf, 0, &freq) || freq < adap->min_freq ||
+ freq > adap->max_freq)
+ return -EINVAL;
+
+ i2c_lock_adapter(adap);
+ ret = adap->set_freq(adap, &freq);
+ i2c_unlock_adapter(adap);
+
+ if (ret)
+ return ret;
+
+ adap->freq = freq;
+
+ return count;
+}
+
+static DEVICE_ATTR(frequency, S_IRUGO, i2c_sysfs_freq_show,
+ i2c_sysfs_freq_store);
+
+static ssize_t frequency_min_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", adap->min_freq);
+}
+
+static DEVICE_ATTR_RO(frequency_min);
+
+static ssize_t frequency_max_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", adap->max_freq);
+}
+
+static DEVICE_ATTR_RO(frequency_max);
+
+static umode_t i2c_adapter_visible_attr(struct kobject *kobj,
+ struct attribute *attr, int idx)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct i2c_adapter *adap = to_i2c_adapter(dev);
+ umode_t mode = attr->mode;
+
+ if (attr == &dev_attr_frequency_min.attr)
+ return adap->min_freq ? mode : 0;
+
+ if (attr == &dev_attr_frequency_max.attr)
+ return adap->max_freq ? mode : 0;
+
+ if (attr == &dev_attr_frequency.attr) {
+ if (adap->set_freq)
+ mode |= S_IWUSR;
+ return adap->freq ? mode : 0;
+ }
+
+ return mode;
+}
+
static struct attribute *i2c_adapter_attrs[] = {
&dev_attr_name.attr,
&dev_attr_new_device.attr,
&dev_attr_delete_device.attr,
+ &dev_attr_frequency.attr,
+ &dev_attr_frequency_min.attr,
+ &dev_attr_frequency_max.attr,
NULL
};

static struct attribute_group i2c_adapter_attr_group = {
.attrs = i2c_adapter_attrs,
+ .is_visible = i2c_adapter_visible_attr,
};

static const struct attribute_group *i2c_adapter_attr_groups[] = {
@@ -1262,6 +1344,14 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
struct device *dev = &adapter->dev;
int id;

+ if (adapter->set_freq) {
+ if (!adapter->freq || !adapter->min_freq || !adapter->max_freq)
+ return -EINVAL;
+ } else {
+ if (adapter->min_freq || adapter->max_freq)
+ return -EINVAL;
+ }
+
if (dev->of_node) {
id = of_alias_get_id(dev->of_node, "i2c");
if (id >= 0) {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 36041e2..5d893f5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -431,6 +431,17 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap);
* usb->interface->dev, platform_device->dev etc.)
* @name: name of this i2c bus
* @bus_recovery_info: see struct i2c_bus_recovery_info. Optional.
+ * @set_freq: set the bus frequency (in HZ) and returns the actual set
+ * value. Since not all the frequencies in the min - max interval
+ * may be valid the driver may round the frequency to the closest
+ * supported one. Optional but must be set if min_freq or
+ * max_freq is set.
+ * @min_freq: minimum bus frequency. Optional but must be set if
+ * set_freq is set.
+ * @max_freq: maximum bus frequency. Optional but must be set if
+ * set_freq is set.
+ * @freq: initial bus frequency. Optional but must be set if set_freq
+ * is set.
*/
struct i2c_adapter {
struct module *owner;
@@ -438,6 +449,11 @@ struct i2c_adapter {
const struct i2c_algorithm *algo; /* the algorithm to access the bus */
void *algo_data;

+ unsigned int freq;
+ unsigned int min_freq;
+ unsigned int max_freq;
+ int (*set_freq)(struct i2c_adapter *a, unsigned int *freq);
+
/* data fields that are valid for all devices */
struct rt_mutex bus_lock;

--
1.9.1

2014-10-15 20:04:03

by Octavian Purdila

[permalink] [raw]
Subject: [PATH v3 4/4] i2c: i2c-diolan-u2c: sysfs bus frequency support

Add support for showing and changing the bus frequency via
sysfs.

Tested on a DLN2 adapter run in U2C-12 compatibility mode.

Cc: Guenter Roeck <[email protected]>
Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/i2c/busses/i2c-diolan-u2c.c | 49 +++++++++++++++++++++++++------------
1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
index b19a310..ff4e120 100644
--- a/drivers/i2c/busses/i2c-diolan-u2c.c
+++ b/drivers/i2c/busses/i2c-diolan-u2c.c
@@ -71,6 +71,9 @@
#define U2C_I2C_FREQ_STD 100000
#define U2C_I2C_FREQ(s) (1000000 / (2 * (s - 1) + 10))

+#define U2C_I2C_MIN_FREQ U2C_I2C_FREQ(U2C_I2C_SPEED_2KHZ)
+#define U2C_I2C_MAX_FREQ U2C_I2C_FREQ_FAST
+
#define DIOLAN_USB_TIMEOUT 100 /* in ms */
#define DIOLAN_SYNC_TIMEOUT 20 /* in ms */

@@ -298,31 +301,24 @@ static void diolan_get_serial(struct i2c_diolan_u2c *dev)
}
}

-static int diolan_init(struct i2c_diolan_u2c *dev)
+static int diolan_set_freq(struct i2c_adapter *adapter, unsigned int *frequency)
{
+ struct i2c_diolan_u2c *dev = i2c_get_adapdata(adapter);
int speed, ret;

- if (frequency >= 200000) {
+ if (*frequency >= 200000) {
speed = U2C_I2C_SPEED_FAST;
- frequency = U2C_I2C_FREQ_FAST;
- } else if (frequency >= 100000 || frequency == 0) {
+ *frequency = U2C_I2C_FREQ_FAST;
+ } else if (*frequency >= 100000 || *frequency == 0) {
speed = U2C_I2C_SPEED_STD;
- frequency = U2C_I2C_FREQ_STD;
+ *frequency = U2C_I2C_FREQ_STD;
} else {
- speed = U2C_I2C_SPEED(frequency);
+ speed = U2C_I2C_SPEED(*frequency);
if (speed > U2C_I2C_SPEED_2KHZ)
speed = U2C_I2C_SPEED_2KHZ;
- frequency = U2C_I2C_FREQ(speed);
+ *frequency = U2C_I2C_FREQ(speed);
}

- dev_info(&dev->interface->dev,
- "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
- dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
-
- diolan_flush_input(dev);
- diolan_fw_version(dev);
- diolan_get_serial(dev);
-
/* Set I2C speed */
ret = diolan_set_speed(dev, speed);
if (ret < 0)
@@ -336,6 +332,23 @@ static int diolan_init(struct i2c_diolan_u2c *dev)
if (speed != U2C_I2C_SPEED_FAST)
ret = diolan_set_clock_synch_timeout(dev, DIOLAN_SYNC_TIMEOUT);

+ return 0;
+}
+
+static int diolan_init(struct i2c_diolan_u2c *dev)
+{
+ int ret;
+
+ diolan_flush_input(dev);
+ diolan_fw_version(dev);
+ diolan_get_serial(dev);
+
+ ret = diolan_set_freq(&dev->adapter, &frequency);
+
+ dev_info(&dev->interface->dev,
+ "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
+ dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
+
return ret;
}

@@ -471,6 +484,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
dev->adapter.owner = THIS_MODULE;
dev->adapter.class = I2C_CLASS_HWMON;
dev->adapter.algo = &diolan_usb_algorithm;
+ dev->adapter.min_freq = U2C_I2C_MIN_FREQ;
+ dev->adapter.max_freq = U2C_I2C_MAX_FREQ;
+ dev->adapter.set_freq = diolan_set_freq;
i2c_set_adapdata(&dev->adapter, dev);
snprintf(dev->adapter.name, sizeof(dev->adapter.name),
DRIVER_NAME " at bus %03d device %03d",
@@ -485,6 +501,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
goto error_free;
}

+ /* set the current bus frequency */
+ dev->adapter.freq = frequency;
+
/* and finally attach to i2c layer */
ret = i2c_add_adapter(&dev->adapter);
if (ret < 0) {
--
1.9.1

2014-10-15 20:03:56

by Octavian Purdila

[permalink] [raw]
Subject: [PATH v3 2/4] i2c: document struct i2c_adapter

Document the i2c_adapter fields that must be initialized before
calling i2c_add_adapter().

Signed-off-by: Octavian Purdila <[email protected]>
---
include/linux/i2c.h | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a95efeb..36041e2 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -418,9 +418,19 @@ int i2c_recover_bus(struct i2c_adapter *adap);
int i2c_generic_gpio_recovery(struct i2c_adapter *adap);
int i2c_generic_scl_recovery(struct i2c_adapter *adap);

-/*
- * i2c_adapter is the structure used to identify a physical i2c bus along
- * with the access algorithms necessary to access it.
+/**
+ * struct i2c_adapter - represents an I2C physical bus
+ *
+ * The following members must be set by the adapter driver before
+ * calling i2c_add_adapter():
+ *
+ * @owner: the module owner, usually THIS_MODULE
+ * @class: bitmask of I2C_CLASS_*
+ * @algo: see struct i2c_algorithm
+ * @dev.parent: set this to the struct device of the driver (e.g. pci_dev->dev,
+ * usb->interface->dev, platform_device->dev etc.)
+ * @name: name of this i2c bus
+ * @bus_recovery_info: see struct i2c_bus_recovery_info. Optional.
*/
struct i2c_adapter {
struct module *owner;
--
1.9.1

2014-10-16 06:52:53

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATH v3 0/4] i2c: show and change bus frequency via sysfs

On Wed, Oct 15, 2014 at 11:03:27PM +0300, Octavian Purdila wrote:
> This patch series adds support to show and change the bus frequency
> via sysfs, by exposing files to show the minimum, maximum and current
> frequency as well as allowing the frequency to be changed. This allows
> the user to view or change the bus frequency on a per bus level.

To give you some early feedback.

Patches 1-2 are nice, will review them somewhen, no general issues here.
For patch 3, I am not convinced that sysfs is the right mechanism. Maybe
configfs, yet I have never dealt with it, so far. So, looking into this
might take more time since I have other core changes pending before
that.


Attachments:
(No filename) (676.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-10-16 13:24:52

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATH v3 4/4] i2c: i2c-diolan-u2c: sysfs bus frequency support

On 10/15/2014 01:03 PM, Octavian Purdila wrote:
> Add support for showing and changing the bus frequency via
> sysfs.
>
> Tested on a DLN2 adapter run in U2C-12 compatibility mode.
>
> Cc: Guenter Roeck <[email protected]>
> Signed-off-by: Octavian Purdila <[email protected]>
> ---
> drivers/i2c/busses/i2c-diolan-u2c.c | 49 +++++++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
> index b19a310..ff4e120 100644
> --- a/drivers/i2c/busses/i2c-diolan-u2c.c
> +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> @@ -71,6 +71,9 @@
> #define U2C_I2C_FREQ_STD 100000
> #define U2C_I2C_FREQ(s) (1000000 / (2 * (s - 1) + 10))
>
> +#define U2C_I2C_MIN_FREQ U2C_I2C_FREQ(U2C_I2C_SPEED_2KHZ)
> +#define U2C_I2C_MAX_FREQ U2C_I2C_FREQ_FAST
> +
> #define DIOLAN_USB_TIMEOUT 100 /* in ms */
> #define DIOLAN_SYNC_TIMEOUT 20 /* in ms */
>
> @@ -298,31 +301,24 @@ static void diolan_get_serial(struct i2c_diolan_u2c *dev)
> }
> }
>
> -static int diolan_init(struct i2c_diolan_u2c *dev)
> +static int diolan_set_freq(struct i2c_adapter *adapter, unsigned int *frequency)

I really dislike this kind of side-effect programming, where a function
named as _set changes one of its parameters. Not my call to make here,
though, so if the i2c maintainers are fine with it

Acked-by: Guenter Roeck <[email protected]>

> {
> + struct i2c_diolan_u2c *dev = i2c_get_adapdata(adapter);
> int speed, ret;
>
> - if (frequency >= 200000) {
> + if (*frequency >= 200000) {
> speed = U2C_I2C_SPEED_FAST;
> - frequency = U2C_I2C_FREQ_FAST;
> - } else if (frequency >= 100000 || frequency == 0) {
> + *frequency = U2C_I2C_FREQ_FAST;
> + } else if (*frequency >= 100000 || *frequency == 0) {
> speed = U2C_I2C_SPEED_STD;
> - frequency = U2C_I2C_FREQ_STD;
> + *frequency = U2C_I2C_FREQ_STD;
> } else {
> - speed = U2C_I2C_SPEED(frequency);
> + speed = U2C_I2C_SPEED(*frequency);
> if (speed > U2C_I2C_SPEED_2KHZ)
> speed = U2C_I2C_SPEED_2KHZ;
> - frequency = U2C_I2C_FREQ(speed);
> + *frequency = U2C_I2C_FREQ(speed);
> }
>
> - dev_info(&dev->interface->dev,
> - "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> - dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> -
> - diolan_flush_input(dev);
> - diolan_fw_version(dev);
> - diolan_get_serial(dev);
> -
> /* Set I2C speed */
> ret = diolan_set_speed(dev, speed);
> if (ret < 0)
> @@ -336,6 +332,23 @@ static int diolan_init(struct i2c_diolan_u2c *dev)
> if (speed != U2C_I2C_SPEED_FAST)
> ret = diolan_set_clock_synch_timeout(dev, DIOLAN_SYNC_TIMEOUT);
>
> + return 0;
> +}
> +
> +static int diolan_init(struct i2c_diolan_u2c *dev)
> +{
> + int ret;
> +
> + diolan_flush_input(dev);
> + diolan_fw_version(dev);
> + diolan_get_serial(dev);
> +
> + ret = diolan_set_freq(&dev->adapter, &frequency);
> +
> + dev_info(&dev->interface->dev,
> + "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> + dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> +
> return ret;
> }
>
> @@ -471,6 +484,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
> dev->adapter.owner = THIS_MODULE;
> dev->adapter.class = I2C_CLASS_HWMON;
> dev->adapter.algo = &diolan_usb_algorithm;
> + dev->adapter.min_freq = U2C_I2C_MIN_FREQ;
> + dev->adapter.max_freq = U2C_I2C_MAX_FREQ;
> + dev->adapter.set_freq = diolan_set_freq;
> i2c_set_adapdata(&dev->adapter, dev);
> snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> DRIVER_NAME " at bus %03d device %03d",
> @@ -485,6 +501,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
> goto error_free;
> }
>
> + /* set the current bus frequency */
> + dev->adapter.freq = frequency;
> +
> /* and finally attach to i2c layer */
> ret = i2c_add_adapter(&dev->adapter);
> if (ret < 0) {
>