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

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

[Octavian Purdila]

This second version gets rid of the frequency table and introduces a
min and max range. It also changes the ABI to allow the driver to
round the given frequency to one that is supported, since many drivers
seems to work this way (i2c-stu300, i2c-s3c2410, i2c-kempld).

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 | 75 ++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c                  | 94 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 32 +++++++++--
 3 files changed, 198 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

-- 
1.9.1

[RFC PATCH v2 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..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:	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 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:	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 v2 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@intel.com>
---
 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 v2 3/3] i2c: show and change bus frequency via sysfs

[Octavian Purdila]

This patch adds three new sysfs files: bus_frequency,
bus_min_frequency and bus_max_frequency 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 and where a module parameter does not allow controlling
the bus speed individually (e.g. USB I2C adapters).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-i2c | 30 +++++++++++
 drivers/i2c/i2c-core.c                  | 94 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 16 ++++++
 3 files changed, 140 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
index 8075585..4a7f8e7 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -43,3 +43,33 @@ Contact:	linux-i2c@vger.kernel.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@vger.kernel.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 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/bus_min_frequency
+What:		/sys/bus/i2c/devices/i2c-X/bus_max_frequency
+KernelVersion:	3.19
+Contact:	linux-i2c@vger.kernel.org
+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..ab77f7f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -940,15 +940,101 @@ 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(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
+		   i2c_sysfs_freq_store);
+
+
+static ssize_t
+i2c_sysfs_min_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->min_freq);
+}
+
+static DEVICE_ATTR(bus_min_frequency, S_IRUGO, i2c_sysfs_min_freq_show, NULL);
+
+static ssize_t
+i2c_sysfs_max_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->max_freq);
+}
+
+static DEVICE_ATTR(bus_max_frequency, S_IRUGO, i2c_sysfs_max_freq_show, NULL);
+
+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_bus_min_frequency.attr)
+		return adap->min_freq ? mode : 0;
+
+	if (attr == &dev_attr_bus_max_frequency.attr)
+		return adap->max_freq ? mode : 0;
+
+	if (attr == &dev_attr_bus_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_bus_frequency.attr,
+	&dev_attr_bus_min_frequency.attr,
+	&dev_attr_bus_max_frequency.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 +1348,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..3482b09 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 freqeuncies 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

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

[Guenter Roeck]

On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
> This patch adds three new sysfs files: bus_frequency,
> bus_min_frequency and bus_max_frequency 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 and where a module parameter does not allow controlling
> the bus speed individually (e.g. USB I2C adapters).
> 
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-i2c | 30 +++++++++++
>  drivers/i2c/i2c-core.c                  | 94 +++++++++++++++++++++++++++++++++
>  include/linux/i2c.h                     | 16 ++++++
>  3 files changed, 140 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
> index 8075585..4a7f8e7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-i2c
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c
> @@ -43,3 +43,33 @@ 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 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/bus_min_frequency
> +What:		/sys/bus/i2c/devices/i2c-X/bus_max_frequency
> +KernelVersion:	3.19
> +Contact:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +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..ab77f7f 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -940,15 +940,101 @@ 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(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
> +		   i2c_sysfs_freq_store);

Consider using DEVICE_ATTR_RO here. Also, extra empty line.

> +
> +
> +static ssize_t
> +i2c_sysfs_min_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->min_freq);
> +}
> +
> +static DEVICE_ATTR(bus_min_frequency, S_IRUGO, i2c_sysfs_min_freq_show, NULL);
> +

Consider using DEVICE_ATTR_RO.

> +static ssize_t
> +i2c_sysfs_max_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->max_freq);
> +}
> +
> +static DEVICE_ATTR(bus_max_frequency, S_IRUGO, i2c_sysfs_max_freq_show, NULL);
> +
Consider using DEVICE_ATTR_RO.

Overall, it seems to me that the bus_ in front of the attrribute names
is really not necessary. The attributes are attached to the adapter, so it
should be obvious that the attributes describe the adapter (=bus) frequency and
not some other frequency.

> +umode_t i2c_adapter_visible_attr(struct kobject *kobj,
> +				 struct attribute *attr, int idx)

static umode_t

> +{
> +	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_bus_min_frequency.attr)
> +		return adap->min_freq ? mode : 0;
> +
> +	if (attr == &dev_attr_bus_max_frequency.attr)
> +		return adap->max_freq ? mode : 0;
> +
> +	if (attr == &dev_attr_bus_frequency.attr) {
> +		if (adap->set_freq)
> +			mode |= S_IWUSR;
> +		return adap->freq ? mode : 0;

Personally, I would make all those attributes only visible if the adapter
supports setting the frquency, and not bother with other conditions,
to keep things simple. Not a strong call, though.

> +	}
> +
> +	return mode;
> +}
> +
> +
extra empty line

>  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_min_frequency.attr,
> +	&dev_attr_bus_max_frequency.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 +1348,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..3482b09 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 freqeuncies 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
> 

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

[Octavian Purdila]

On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>
> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
> > This patch adds three new sysfs files: bus_frequency,
> > bus_min_frequency and bus_max_frequency which allows the user to view
> > or change the bus frequency on a per bus level.
> >

<snip>

> > +
> > +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
> > +                i2c_sysfs_freq_store);
>
> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>

Unfortunately that won't work because we must transform bus_frequency
to a RW entry (via is_visible) if the bus can change the frequency. We
can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
RO entry with is visible is not possible:

fs/sysfs/group.c:

static int create_files(struct kernfs_node *parent, struct kobject *kobj,
...
                        if (grp->is_visible) {
                                mode = grp->is_visible(kobj, *attr, i);
                                if (!mode)
                                        continue;
                        }
                        error = sysfs_add_file_mode_ns(parent, *attr, false,
                                                       (*attr)->mode | mode,
                                                       NULL);

Of course if we only allow a RW frequency entry as you suggest below,
then we can use DEVICE_ATTR_RW.

> > +
> > +
> > +static ssize_t
> > +i2c_sysfs_min_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->min_freq);
> > +}
> > +
> > +static DEVICE_ATTR(bus_min_frequency, S_IRUGO, i2c_sysfs_min_freq_show, NULL);
> > +
>
> Consider using DEVICE_ATTR_RO.
>

OK.

> > +static ssize_t
> > +i2c_sysfs_max_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->max_freq);
> > +}
> > +
> > +static DEVICE_ATTR(bus_max_frequency, S_IRUGO, i2c_sysfs_max_freq_show, NULL);
> > +
> Consider using DEVICE_ATTR_RO.
>

OK.

> Overall, it seems to me that the bus_ in front of the attrribute names
> is really not necessary. The attributes are attached to the adapter, so it
> should be obvious that the attributes describe the adapter (=bus) frequency and
> not some other frequency.
>
> > +umode_t i2c_adapter_visible_attr(struct kobject *kobj,
> > +                              struct attribute *attr, int idx)
>
> static umode_t
>

Oops :)

> > +{
> > +     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_bus_min_frequency.attr)
> > +             return adap->min_freq ? mode : 0;
> > +
> > +     if (attr == &dev_attr_bus_max_frequency.attr)
> > +             return adap->max_freq ? mode : 0;
> > +
> > +     if (attr == &dev_attr_bus_frequency.attr) {
> > +             if (adap->set_freq)
> > +                     mode |= S_IWUSR;
> > +             return adap->freq ? mode : 0;
>
> Personally, I would make all those attributes only visible if the adapter
> supports setting the frquency, and not bother with other conditions,
> to keep things simple. Not a strong call, though.
>

I don't have a strong opinion either. I think a RO frequency entry
would help with debugging, but I am not sure how useful it is in
practice.

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

[Guenter Roeck]

On 10/15/2014 04:49 AM, Octavian Purdila wrote:
> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>
>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>> This patch adds three new sysfs files: bus_frequency,
>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>> or change the bus frequency on a per bus level.
>>>
>
> <snip>
>
>>> +
>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>> +                i2c_sysfs_freq_store);
>>
>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>
>
> Unfortunately that won't work because we must transform bus_frequency
> to a RW entry (via is_visible) if the bus can change the frequency. We

Ah yes, you are right.

> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
> RO entry with is visible is not possible:
>

Why not ?

is_visible returns the desired mode. Just like you can return mode | S_IWUSR,
you can return mode & ~S_IWUSR.

Am I missing something ?

Guenter

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

[Octavian Purdila]

On Wed, Oct 15, 2014 at 4:13 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 10/15/2014 04:49 AM, Octavian Purdila wrote:
>>
>> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>
>>>
>>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>>>
>>>> This patch adds three new sysfs files: bus_frequency,
>>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>>> or change the bus frequency on a per bus level.
>>>>
>>
>> <snip>
>>
>>>> +
>>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>>> +                i2c_sysfs_freq_store);
>>>
>>>
>>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>>
>>
>> Unfortunately that won't work because we must transform bus_frequency
>> to a RW entry (via is_visible) if the bus can change the frequency. We
>
>
> Ah yes, you are right.
>
>> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
>> RO entry with is visible is not possible:
>>
>
> Why not ?
>
> is_visible returns the desired mode. Just like you can return mode |
> S_IWUSR,
> you can return mode & ~S_IWUSR.
>
> Am I missing something ?
>

Here is how sysfs uses is_visible:

static int create_files(struct kernfs_node *parent, struct kobject *kobj,
...
                        if (grp->is_visible) {
                                mode = grp->is_visible(kobj, *attr, i);
                                if (!mode)
                                        continue;
                        }
                        error = sysfs_add_file_mode_ns(parent, *attr, false,
                                                       (*attr)->mode | mode,
                                                       NULL);

so basically is the mode set is the original mode from the attributed
"or-ed" with the mode return by is_visible.

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

[Guenter Roeck]

On 10/15/2014 06:32 AM, Octavian Purdila wrote:
> On Wed, Oct 15, 2014 at 4:13 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 10/15/2014 04:49 AM, Octavian Purdila wrote:
>>>
>>> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>>
>>>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>>>>
>>>>> This patch adds three new sysfs files: bus_frequency,
>>>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>>>> or change the bus frequency on a per bus level.
>>>>>
>>>
>>> <snip>
>>>
>>>>> +
>>>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>>>> +                i2c_sysfs_freq_store);
>>>>
>>>>
>>>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>>>
>>>
>>> Unfortunately that won't work because we must transform bus_frequency
>>> to a RW entry (via is_visible) if the bus can change the frequency. We
>>
>>
>> Ah yes, you are right.
>>
>>> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
>>> RO entry with is visible is not possible:
>>>
>>
>> Why not ?
>>
>> is_visible returns the desired mode. Just like you can return mode |
>> S_IWUSR,
>> you can return mode & ~S_IWUSR.
>>
>> Am I missing something ?
>>
>
> Here is how sysfs uses is_visible:
>
> static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> ...
>                          if (grp->is_visible) {
>                                  mode = grp->is_visible(kobj, *attr, i);
>                                  if (!mode)
>                                          continue;
>                          }
>                          error = sysfs_add_file_mode_ns(parent, *attr, false,
>                                                         (*attr)->mode | mode,
>                                                         NULL);
>
> so basically is the mode set is the original mode from the attributed
> "or-ed" with the mode return by is_visible.
>
Ah, you are right. That's a new one for me. Thanks, I didn't notice earlier.
Good to know ;-).

Guenter