[RFC PATCH 0/3] i2c: show and change bus frequency via sysfs

[RFC PATCH 0/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

The first two patches are just general cleanup the actual
implementation is in patch 3. If the general direction looks
acceptable, I will send a follow-up series that adds sysfs bus
frequency support to the existing drivers.

Octavian Purdila (3):
  i2c: document the existing i2c sysfs ABI
  i2c: document struct i2c_adapter
  i2c: show and change bus frequency via sysfs

 Documentation/ABI/testing/sysfs-bus-i2c | 69 +++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c                  | 77 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 26 +++++++++--
 3 files changed, 169 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

-- 
1.9.1

[RFC PATCH 1/3] i2c: document the existing i2c sysfs ABI

[Octavian Purdila]

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

Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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..22c621a
--- /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:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:
+		Write only entry that allows the instantiation of a
+		new i2c device to 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:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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

[RFC PATCH 2/3] i2c: document struct i2c_adapter

[Octavian Purdila]

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

Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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

[RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

This patch adds two new sysfs files, bus_frequency and
bus_supported_frequencies which allows the user to view or change the
bus frequency on a per bus level.

This is required for e.g. USB I2C buses where we can have multiple
device plugged in a system and where a module parameter does not allow
controlling the bus speed individually.

Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-bus-i2c | 24 ++++++++++
 drivers/i2c/i2c-core.c                  | 77 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 10 +++++
 3 files changed, 111 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
index 22c621a..058ac68 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -43,3 +43,27 @@ Contact:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 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/bus_frequency
+KernelVersion:	3.19
+Contact:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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 showing/changing the
+		frequency then reading/writting to this entry will
+		fail with -EOPNOTSUPP.
+		When updating the bus frequency that value must be one
+		of the values in bus_supported_frequencies otherwise
+		writting will fail with -EINVAL.
+
+What:		/sys/bus/i2c/devices/i2c-X/bus_supported_frequencies
+KernelVersion:	3.19
+Contact:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:
+		Supported frequencies for bus X. The unit is HZ and
+		format is "%d %d %d ... %d\n".
+		If the bus does not support showing/changing the
+		frequency then reading to this entry will fail with
+		-EOPNOTSUPP.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 632057a..32b918a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -940,10 +940,87 @@ 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);
+
+	if (!adap->freq)
+		return -EOPNOTSUPP;
+
+	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;
+	int i;
+
+	if (!adap->set_freq)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &freq))
+		return -EINVAL;
+
+	if (freq == 0)
+		return -EINVAL;
+
+	for (i = 0; i < I2C_MAX_FREQS && adap->supported_freqs[i]; i++)
+		if (adap->supported_freqs[i] >= freq)
+			break;
+
+	if (adap->supported_freqs[i] != 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(bus_frequency, S_IWUSR | S_IRUGO, i2c_sysfs_freq_show,
+		   i2c_sysfs_freq_store);
+
+
+static ssize_t
+i2c_sysfs_supp_freqs_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+	ssize_t count = 0;
+	int i;
+
+	if (!adap->supported_freqs[0])
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < I2C_MAX_FREQS && adap->supported_freqs[i]; i++)
+		count += snprintf(buf + count, PAGE_SIZE - count, "%d ",
+				  adap->supported_freqs[i]);
+	buf[count - 1] = '\n';
+
+	return count;
+}
+
+static DEVICE_ATTR(bus_supported_frequencies, S_IRUGO,
+		   i2c_sysfs_supp_freqs_show, NULL);
+
 static struct attribute *i2c_adapter_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_new_device.attr,
 	&dev_attr_delete_device.attr,
+	&dev_attr_bus_frequency.attr,
+	&dev_attr_bus_supported_frequencies.attr,
 	NULL
 };
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 36041e2..e410637 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -418,6 +418,8 @@ 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);
 
+#define I2C_MAX_FREQS 16
+
 /**
  * struct i2c_adapter - represents an I2C physical bus
  *
@@ -431,6 +433,10 @@ 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.
+ * @supported_freqs: supported bus frequencies (in HZ). Must be sorted
+ *	in ascending order. Optional.
+ * @freq: initial bus frequency. Optional.
+ * @set_bus_freq: set the bus frequency (in HZ). Optional.
  */
 struct i2c_adapter {
 	struct module *owner;
@@ -438,6 +444,10 @@ struct i2c_adapter {
 	const struct i2c_algorithm *algo; /* the algorithm to access the bus */
 	void *algo_data;
 
+	unsigned int supported_freqs[I2C_MAX_FREQS];
+	unsigned int 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

Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

On Sat, Oct 11, 2014 at 11:14 PM, Mark Roszko <mark.roszko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This seems limiting to arches with peripherals that can support a range of
> frequencies rather than fixed numbers.
> Also it creates some portability quirkiness between platforms when all the
> i2c bus drivers have different supported freq lists and you have to match
> exactly the right frequency. I.e. one guy does 60khz but another only has
> 80khz.

Sorry, I don't understand your points here. If this limitations exists
they are not introduced by this patch. This patch just exposes the
frequency so that it can be read or changed in userspace.

> Another issue is in systems where you have i2c devices on the same bus as
> the sysfs user space driver. User space could set a bus frequency that
> prevents operation with a system i2c device.

Changing the frequency is limited to root. Also, bus drivers do not
have to implement set_freq if it is thought not to be safe.

On a different not, I have noticed that a fixed set of frequencies
might not be the best API, since multiple drivers rather support a
rather large set of frequencies in a range. A better API might be to
expose a min-max range and let the bus driver adjust the requested
frequency. I will follow up with a second version that does that.

Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Guenter Roeck]

On Sun, Oct 12, 2014 at 12:32:56PM +0300, Octavian Purdila wrote:
> On Sat, Oct 11, 2014 at 11:14 PM, Mark Roszko <mark.roszko@gmail.com> wrote:
> 
> > This seems limiting to arches with peripherals that can support a range of
> > frequencies rather than fixed numbers.
> > Also it creates some portability quirkiness between platforms when all the
> > i2c bus drivers have different supported freq lists and you have to match
> > exactly the right frequency. I.e. one guy does 60khz but another only has
> > 80khz.
> 
> Sorry, I don't understand your points here. If this limitations exists
> they are not introduced by this patch. This patch just exposes the
> frequency so that it can be read or changed in userspace.
> 
> > Another issue is in systems where you have i2c devices on the same bus as
> > the sysfs user space driver. User space could set a bus frequency that
> > prevents operation with a system i2c device.
> 
> Changing the frequency is limited to root. Also, bus drivers do not
> have to implement set_freq if it is thought not to be safe.
> 
> On a different not, I have noticed that a fixed set of frequencies
> might not be the best API, since multiple drivers rather support a
> rather large set of frequencies in a range. A better API might be to
> expose a min-max range and let the bus driver adjust the requested
> frequency. I will follow up with a second version that does that.

As two separate sysfs attributes, maybe ? sysfs is supposed to provide
one value per attribute.

For the patch itself, I would find it better if you used is_visible to
determine if the new attributes should be visible (and/or writable) instead
of returning -EOPNOTSUPP.

Guenter

Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

On Sun, Oct 12, 2014 at 8:06 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On Sun, Oct 12, 2014 at 12:32:56PM +0300, Octavian Purdila wrote:
>> On Sat, Oct 11, 2014 at 11:14 PM, Mark Roszko <mark.roszko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > This seems limiting to arches with peripherals that can support a range of
>> > frequencies rather than fixed numbers.
>> > Also it creates some portability quirkiness between platforms when all the
>> > i2c bus drivers have different supported freq lists and you have to match
>> > exactly the right frequency. I.e. one guy does 60khz but another only has
>> > 80khz.
>>
>> Sorry, I don't understand your points here. If this limitations exists
>> they are not introduced by this patch. This patch just exposes the
>> frequency so that it can be read or changed in userspace.
>>
>> > Another issue is in systems where you have i2c devices on the same bus as
>> > the sysfs user space driver. User space could set a bus frequency that
>> > prevents operation with a system i2c device.
>>
>> Changing the frequency is limited to root. Also, bus drivers do not
>> have to implement set_freq if it is thought not to be safe.
>>
>> On a different not, I have noticed that a fixed set of frequencies
>> might not be the best API, since multiple drivers rather support a
>> rather large set of frequencies in a range. A better API might be to
>> expose a min-max range and let the bus driver adjust the requested
>> frequency. I will follow up with a second version that does that.
>
> As two separate sysfs attributes, maybe ? sysfs is supposed to provide
> one value per attribute.
>

Yes, I was thinking of using three attributes: bus_frequency,
bus_min_frequency and bus_max_frequency.

> For the patch itself, I would find it better if you used is_visible to
> determine if the new attributes should be visible (and/or writable) instead
> of returning -EOPNOTSUPP.
>

OK, that sounds better. I will switch to that model in v2.

Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Mark Roszko]

> If this limitations exists
>they are not introduced by this patch. This patch just exposes the
>frequency so that it can be read or changed in userspace.

Ah, well right now you can have an i2c bus with driver 1 and 2. Say
the i2c bus is configured for 60khz in kernel space which normally
can't be changed. Driver 1 talks to a slave that cannot go above
100khz. Now the userspace interface is added to the i2c bus. Some
userspace application decides to reconfigure the bus for 400khz and do
its communication to some slave device. Now the kernel tries to do
some background talking to the drive 1 slave and suddenly finds it can
no longer communicate with it. Right now with the kernel space only
configuration, the system is safe from being messed up easily. It's
more of a sanity of configuration issue.

>On a different not, I have noticed that a fixed set of frequencies
>might not be the best API, since multiple drivers rather support a
>rather large set of frequencies in a range. A better API might be to
>expose a min-max range and let the bus driver adjust the requested
>frequency. I will follow up with a second version that does that.

I was actually thinking you could eliminate the table of supported
frequencies and just have the bus driver handle the set frequency
decision itself and just return an error code if it's invalid. There
are legitiamate drivers that cannot do more than a list of frequencies
already as well. One example is here:
http://lxr.free-electrons.com/source/drivers/i2c/busses/i2c-bcm-kona.c#L717

Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

On Tue, Oct 14, 2014 at 4:53 AM, Mark Roszko <mark.roszko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > If this limitations exists
> >they are not introduced by this patch. This patch just exposes the
> >frequency so that it can be read or changed in userspace.
>
> Ah, well right now you can have an i2c bus with driver 1 and 2. Say
> the i2c bus is configured for 60khz in kernel space which normally
> can't be changed. Driver 1 talks to a slave that cannot go above
> 100khz. Now the userspace interface is added to the i2c bus. Some
> userspace application decides to reconfigure the bus for 400khz and do
> its communication to some slave device. Now the kernel tries to do
> some background talking to the drive 1 slave and suddenly finds it can
> no longer communicate with it. Right now with the kernel space only
> configuration, the system is safe from being messed up easily. It's
> more of a sanity of configuration issue.

You need privileges to change the bus frequency, so this is a
configuration issue. Which you still have today, since can still set
the i2c frequency on some busses via a module parameter.

>
>
> >On a different not, I have noticed that a fixed set of frequencies
> >might not be the best API, since multiple drivers rather support a
> >rather large set of frequencies in a range. A better API might be to
> >expose a min-max range and let the bus driver adjust the requested
> >frequency. I will follow up with a second version that does that.
>
> I was actually thinking you could eliminate the table of supported
> frequencies and just have the bus driver handle the set frequency
> decision itself and just return an error code if it's invalid. There
> are legitiamate drivers that cannot do more than a list of frequencies
> already as well. One example is here:
> http://lxr.free-electrons.com/source/drivers/i2c/busses/i2c-bcm-kona.c#L717

Yes, I will do that for v2 and in addition I will also add min and max
attributes to make it easier to determine if a bus supports fast mode,
fast plus, high, etc.