[PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups
The sysfs logic already creates a list of groups for the device, so add the sdw_slave_dev_attr_group group to that list instead of having to do a two-step process of adding a group list and then an individual group.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/soundwire/sysfs_slave.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 3210359cd944..83e3f6cc3250 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -105,7 +105,10 @@ static struct attribute *slave_attrs[] = { &dev_attr_modalias.attr, NULL, }; -ATTRIBUTE_GROUPS(slave); + +static const struct attribute_group slave_attr_group = { + .attrs = slave_attrs, +};
static struct attribute *slave_dev_attrs[] = { &dev_attr_mipi_revision.attr, @@ -190,6 +193,12 @@ static const struct attribute_group dp0_group = { .name = "dp0", };
+static const struct attribute_group *slave_groups[] = { + &slave_attr_group, + &sdw_slave_dev_attr_group, + NULL, +}; + int sdw_slave_sysfs_init(struct sdw_slave *slave) { int ret; @@ -198,10 +207,6 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave) if (ret < 0) return ret;
- ret = devm_device_add_group(&slave->dev, &sdw_slave_dev_attr_group); - if (ret < 0) - return ret; - if (slave->prop.dp0_prop) { ret = devm_device_add_group(&slave->dev, &dp0_group); if (ret < 0)
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr, + int n) +{ + struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj)); + + if (slave->prop.dp0_prop) + return attr->mode; + return 0; +} + static struct attribute *dp0_attrs[] = { &dev_attr_max_word.attr, &dev_attr_min_word.attr, @@ -190,12 +200,14 @@ static struct attribute *dp0_attrs[] = { */ static const struct attribute_group dp0_group = { .attrs = dp0_attrs, + .is_visible = dp0_is_visible, .name = "dp0", };
static const struct attribute_group *slave_groups[] = { &slave_attr_group, &sdw_slave_dev_attr_group, + &dp0_group, NULL, };
@@ -207,12 +219,6 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave) if (ret < 0) return ret;
- if (slave->prop.dp0_prop) { - ret = devm_device_add_group(&slave->dev, &dp0_group); - if (ret < 0) - return ret; - } - if (slave->prop.source_ports || slave->prop.sink_ports) { ret = sdw_slave_sysfs_dpn_init(slave); if (ret < 0)
On 7/29/22 08:50, Greg Kroah-Hartman wrote:
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
static struct attribute *dp0_attrs[] = { &dev_attr_max_word.attr, &dev_attr_min_word.attr, @@ -190,12 +200,14 @@ static struct attribute *dp0_attrs[] = { */ static const struct attribute_group dp0_group = { .attrs = dp0_attrs,
- .is_visible = dp0_is_visible, .name = "dp0",
};
static const struct attribute_group *slave_groups[] = { &slave_attr_group, &sdw_slave_dev_attr_group,
- &dp0_group, NULL,
};
@@ -207,12 +219,6 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave) if (ret < 0) return ret;
- if (slave->prop.dp0_prop) {
ret = devm_device_add_group(&slave->dev, &dp0_group);if (ret < 0)return ret;- }
- if (slave->prop.source_ports || slave->prop.sink_ports) { ret = sdw_slave_sysfs_dpn_init(slave); if (ret < 0)
On Fri, Jul 29, 2022 at 09:46:26AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 08:50, Greg Kroah-Hartman wrote:
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
thanks,
greg k-h
On 7/29/22 09:52, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 09:46:26AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 08:50, Greg Kroah-Hartman wrote:
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
The attributes are really details that few people/applications would understand, and unfortunately the information reported in DSDT is more often than not complete garbage.
On Fri, Jul 29, 2022 at 09:57:52AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 09:52, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 09:46:26AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 08:50, Greg Kroah-Hartman wrote:
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The attributes are really details that few people/applications would understand, and unfortunately the information reported in DSDT is more often than not complete garbage.
I don't understand what DSDT is, or how it is relevant here :(
thanks,
greg k-h
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
The attributes are really details that few people/applications would understand, and unfortunately the information reported in DSDT is more often than not complete garbage.
I don't understand what DSDT is, or how it is relevant here :(
Platform firmware typically exposes the presence of ports and the details since there are no descriptors in hardware. The DSDT in ACPI exposes _DSD properties under the SoundWire device scope, which are compatible with DT properties. In other words, what the driver exposes in sysfs is just a mirror of what was reported by platform firmware - unless it was overridden by a driver.
On Fri, Jul 29, 2022 at 10:52:28AM -0500, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
Why not make them a real device like we do for USB endpoints?
What uses these sysfs files today that would be confused about an empty directory?
thanks,
greg k-h
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
Why not make them a real device like we do for USB endpoints?
I don't see what adding another layer of hierarchy would bring. In their simplest configuration, there are 6 registers 8-bit exposed. And the port registers, when present, are accessed with a plain vanilla offset.
What uses these sysfs files today that would be confused about an empty directory?
That's a good question. I am not aware of any tools making use of those attributes. To a large degree, they are helpful only for debug and support, all these read-only attributes could be moved to debugfs. That could be a way to simplify everyone's life....
On Fri, Jul 29, 2022 at 11:46:32AM -0500, Pierre-Louis Bossart wrote:
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
Why not make them a real device like we do for USB endpoints?
I don't see what adding another layer of hierarchy would bring. In their simplest configuration, there are 6 registers 8-bit exposed. And the port registers, when present, are accessed with a plain vanilla offset.
Who uses these registers?
What uses these sysfs files today that would be confused about an empty directory?
That's a good question. I am not aware of any tools making use of those attributes. To a large degree, they are helpful only for debug and support, all these read-only attributes could be moved to debugfs. That could be a way to simplify everyone's life....
That would be much nicer, put it all in a single debugfs file and it would be so simple.
What attributes could we do that for?
thanks,
greg k-h
On 7/29/22 12:15, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 11:46:32AM -0500, Pierre-Louis Bossart wrote:
> That should be fine, tools should just be looking for the attributes, > not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
Why not make them a real device like we do for USB endpoints?
I don't see what adding another layer of hierarchy would bring. In their simplest configuration, there are 6 registers 8-bit exposed. And the port registers, when present, are accessed with a plain vanilla offset.
Who uses these registers?
The bus layer. When a 'stream' is created, the 'bit allocation' will define who owns which bitSlots in the frame and the registers will be programmed. The bit allocation may be dynamic or fixed depending on the host.
What uses these sysfs files today that would be confused about an empty directory?
That's a good question. I am not aware of any tools making use of those attributes. To a large degree, they are helpful only for debug and support, all these read-only attributes could be moved to debugfs. That could be a way to simplify everyone's life....
That would be much nicer, put it all in a single debugfs file and it would be so simple.
What attributes could we do that for?
All of them really - except maybe the device number which could be used to figure what the device is when looking at power status and other 'standard' sysfs attributes. sdw:3:025d:0714:01 is not really user-friendly, device_number 1 is.
On Fri, Jul 29, 2022 at 11:46:32AM -0500, Pierre-Louis Bossart wrote:
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
The SoundWire spec defines the concept of 'data port'. The valid ranges are 1..14, but in all existing devices the number of data ports is way smaller, typically 2 to 4. Data ports (DPn) are source or sink, and there's no firm rule that data ports needs to be contiguous.
DP0 is a 'special case' where the data transport is used for control information, e.g. programming large set of registers or firmware download. DP0 is completely optional in hardware, and not handled in Linux for now.
DP0 and DPn expose low-level transport registers, which define how the contents of a FIFO will be written or read from the bus. Think of it as a generalization of the concept of TDM slots, where instead of having a fixed slot per frame the slot position/repetition/runlength can be programmed.
The data ports could be as simple as 1-bit PDM, or support 8ch PCM 24-bits. That's the sort of information reported in attributes.
Why not make them a real device like we do for USB endpoints?
I don't see what adding another layer of hierarchy would bring. In their simplest configuration, there are 6 registers 8-bit exposed. And the port registers, when present, are accessed with a plain vanilla offset.
What uses these sysfs files today that would be confused about an empty directory?
That's a good question. I am not aware of any tools making use of those attributes. To a large degree, they are helpful only for debug and support, all these read-only attributes could be moved to debugfs. That could be a way to simplify everyone's life....
Ok, this is why I didn't just rebase and resend. I've now worked on sysfs to NOT create the directory if no attributes were present. I'll send out this series rebased along with that commit as well which should help with this issue.
thanks,
greg k-h
On 29-07-22, 17:03, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 09:57:52AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 09:52, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 09:46:26AM -0500, Pierre-Louis Bossart wrote:
On 7/29/22 08:50, Greg Kroah-Hartman wrote:
There's no need to special-case the dp0 sysfs attributes, the is_visible() callback in the attribute group can handle that for us, so add that and add it to the attribute group list making the logic simpler overall.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/soundwire/sysfs_slave.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 83e3f6cc3250..3723333a5c2b 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -174,6 +174,16 @@ static ssize_t words_show(struct device *dev, } static DEVICE_ATTR_RO(words);
+static umode_t dp0_is_visible(struct kobject *kobj, struct attribute *attr,
int n)+{
- struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));
- if (slave->prop.dp0_prop)
return attr->mode;- return 0;
+}
This changes the results slightly by creating an empty 'dp0' directory with no attributes inside.
Before:
[root@fedora ~]# cd /sys/bus/soundwire/devices/sdw:3:025d:0714:01 [root@fedora sdw:3:025d:0714:01]# ls dp0 ls: cannot access 'dp0': No such file or directory
After: [root@fedora sdw:3:025d:0714:01]# ls dp0
That should be fine, tools should just be looking for the attributes, not the existance of a directory, right?
The idea what that we would only expose ports that actually exist. That's helpful information anyone with a basic knowledge of the SoundWire specification would understand.
Is "dp0" a port? If so, why isn't it a real device?
No they are not. It is a logical channel to send data to the device. The device can have one or many data ports...
So the device logic or usb-endpoint style maynot look good here...
The change looks good though, dp0 maybe present and empty, we should looks for attributes...
The attributes are really details that few people/applications would understand, and unfortunately the information reported in DSDT is more often than not complete garbage.
I don't understand what DSDT is, or how it is relevant here :(
thanks,
greg k-h
The driver core supports the ability to handle the creation and removal of device-specific sysfs files in a race-free manner. Take advantage of that by converting this driver to use this by moving the sysfs attributes into a group and assigning the dev_groups pointer to it.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/soundwire/bus_type.c | 1 + drivers/soundwire/sysfs_local.h | 3 +++ drivers/soundwire/sysfs_slave.c | 6 +----- 3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 893296f3fe39..81c77e6ddbad 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -193,6 +193,7 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
drv->driver.owner = owner; drv->driver.probe = sdw_drv_probe; + drv->driver.dev_groups = sdw_attr_groups;
if (drv->remove) drv->driver.remove = sdw_drv_remove; diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h index 7268bc24c538..3ab8658a7782 100644 --- a/drivers/soundwire/sysfs_local.h +++ b/drivers/soundwire/sysfs_local.h @@ -11,6 +11,9 @@ /* basic attributes to report status of Slave (attachment, dev_num) */ extern const struct attribute_group *sdw_slave_status_attr_groups[];
+/* attributes for all soundwire devices */ +extern const struct attribute_group *sdw_attr_groups[]; + /* additional device-managed properties reported after driver probe */ int sdw_slave_sysfs_init(struct sdw_slave *slave); int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave); diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 3723333a5c2b..4c716c167493 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -204,7 +204,7 @@ static const struct attribute_group dp0_group = { .name = "dp0", };
-static const struct attribute_group *slave_groups[] = { +const struct attribute_group *sdw_attr_groups[] = { &slave_attr_group, &sdw_slave_dev_attr_group, &dp0_group, @@ -215,10 +215,6 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave) { int ret;
- ret = devm_device_add_groups(&slave->dev, slave_groups); - if (ret < 0) - return ret; - if (slave->prop.source_ports || slave->prop.sink_ports) { ret = sdw_slave_sysfs_dpn_init(slave); if (ret < 0)
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 893296f3fe39..81c77e6ddbad 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -193,6 +193,7 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
drv->driver.owner = owner; drv->driver.probe = sdw_drv_probe;
drv->driver.dev_groups = sdw_attr_groups;
if (drv->remove) drv->driver.remove = sdw_drv_remove;
Minor rebase issue: this version of the bus_type.c code is no longer up-to-date, this patch creates a conflict with "soundwire: bus_type: fix remove and shutdown support" merged by Vinod - should be in your char-misc tree as well for 5.20.
++<<<<<<< HEAD
+ drv->driver.remove = sdw_drv_remove;
+ drv->driver.shutdown = sdw_drv_shutdown;
++=======
+ drv->driver.dev_groups = sdw_attr_groups;
+
+ if (drv->remove)
+ drv->driver.remove = sdw_drv_remove;
+
+ if (drv->shutdown)
+ drv->driver.shutdown = sdw_drv_shutdown;
++>>>>>>> soundwire: sysfs: have the driver core handle the creation of the device groups
Fixing it locally and testing.
On Fri, Jul 29, 2022 at 09:12:17AM -0500, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 893296f3fe39..81c77e6ddbad 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -193,6 +193,7 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
drv->driver.owner = owner; drv->driver.probe = sdw_drv_probe;
drv->driver.dev_groups = sdw_attr_groups;
if (drv->remove) drv->driver.remove = sdw_drv_remove;
Minor rebase issue: this version of the bus_type.c code is no longer up-to-date, this patch creates a conflict with "soundwire: bus_type: fix remove and shutdown support" merged by Vinod - should be in your char-misc tree as well for 5.20.
Sorry, did this against Linus's tree, I will rebase it against 5.20-rc1 when that comes out and resend it then.
thanks,
greg k-h
Now that sdw_slave_sysfs_init() only calls sdw_slave_sysfs_dpn_init(), just do that instead and remove sdw_slave_sysfs_init() to get it out of the way to save a bit of logic and code size.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/soundwire/bus_type.c | 4 ++-- drivers/soundwire/sysfs_local.h | 1 - drivers/soundwire/sysfs_slave.c | 13 ------------- drivers/soundwire/sysfs_slave_dpn.c | 3 +++ 4 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 81c77e6ddbad..4e4e62d1e475 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -121,8 +121,8 @@ static int sdw_drv_probe(struct device *dev) if (slave->ops && slave->ops->read_prop) slave->ops->read_prop(slave);
- /* init the sysfs as we have properties now */ - ret = sdw_slave_sysfs_init(slave); + /* init the dynamic sysfs attributes we need */ + ret = sdw_slave_sysfs_dpn_init(slave); if (ret < 0) dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h index 3ab8658a7782..fa048e112629 100644 --- a/drivers/soundwire/sysfs_local.h +++ b/drivers/soundwire/sysfs_local.h @@ -15,7 +15,6 @@ extern const struct attribute_group *sdw_slave_status_attr_groups[]; extern const struct attribute_group *sdw_attr_groups[];
/* additional device-managed properties reported after driver probe */ -int sdw_slave_sysfs_init(struct sdw_slave *slave); int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
#endif /* __SDW_SYSFS_LOCAL_H */ diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 4c716c167493..070e0d84be94 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -211,19 +211,6 @@ const struct attribute_group *sdw_attr_groups[] = { NULL, };
-int sdw_slave_sysfs_init(struct sdw_slave *slave) -{ - int ret; - - if (slave->prop.source_ports || slave->prop.sink_ports) { - ret = sdw_slave_sysfs_dpn_init(slave); - if (ret < 0) - return ret; - } - - return 0; -} - /* * the status is shown in capital letters for UNATTACHED and RESERVED * on purpose, to highligh users to the fact that these status values diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c index c4b6543c09fd..a3fb380ee519 100644 --- a/drivers/soundwire/sysfs_slave_dpn.c +++ b/drivers/soundwire/sysfs_slave_dpn.c @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) int ret; int i;
+ if (!slave->prop.source_ports && !slave->prop.sink_ports) + return 0; + mask = slave->prop.source_ports; for_each_set_bit(i, &mask, 32) { ret = add_all_attributes(&slave->dev, i, 1);
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c index c4b6543c09fd..a3fb380ee519 100644 --- a/drivers/soundwire/sysfs_slave_dpn.c +++ b/drivers/soundwire/sysfs_slave_dpn.c @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) int ret; int i;
- if (!slave->prop.source_ports && !slave->prop.sink_ports)
return 0;- mask = slave->prop.source_ports; for_each_set_bit(i, &mask, 32) { ret = add_all_attributes(&slave->dev, i, 1);
I am struggling with this one since the driver is still adding attributes manually. You mentioned in the other thread that
" That's what the is_visible() callback is for in the groups structure, you determine if the attribute is visable or not at runtime, you don't rely on the driver itself to add/remove attributes, that does not scale and again, is racy. "
I interpret that as "there's still a race here", no?
On Fri, Jul 29, 2022 at 10:00:42AM -0500, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c index c4b6543c09fd..a3fb380ee519 100644 --- a/drivers/soundwire/sysfs_slave_dpn.c +++ b/drivers/soundwire/sysfs_slave_dpn.c @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) int ret; int i;
- if (!slave->prop.source_ports && !slave->prop.sink_ports)
return 0;- mask = slave->prop.source_ports; for_each_set_bit(i, &mask, 32) { ret = add_all_attributes(&slave->dev, i, 1);
I am struggling with this one since the driver is still adding attributes manually. You mentioned in the other thread that
" That's what the is_visible() callback is for in the groups structure, you determine if the attribute is visable or not at runtime, you don't rely on the driver itself to add/remove attributes, that does not scale and again, is racy. "
I interpret that as "there's still a race here", no?
Yes, there is, BUT as you are creating all of these attributes "on the fly" for now, I don't see a simple conversion to fix that up. Let me do these, the easy ones first. Your dynamic attribute allocations are the harder things to do, let me think about those after I've fixed the rest of the tree up with the trivial ones :)
thanks,
greg k-h
On 29-07-22, 17:13, Greg Kroah-Hartman wrote:
On Fri, Jul 29, 2022 at 10:00:42AM -0500, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c index c4b6543c09fd..a3fb380ee519 100644 --- a/drivers/soundwire/sysfs_slave_dpn.c +++ b/drivers/soundwire/sysfs_slave_dpn.c @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) int ret; int i;
- if (!slave->prop.source_ports && !slave->prop.sink_ports)
return 0;- mask = slave->prop.source_ports; for_each_set_bit(i, &mask, 32) { ret = add_all_attributes(&slave->dev, i, 1);
I am struggling with this one since the driver is still adding attributes manually. You mentioned in the other thread that
" That's what the is_visible() callback is for in the groups structure, you determine if the attribute is visable or not at runtime, you don't rely on the driver itself to add/remove attributes, that does not scale and again, is racy. "
I interpret that as "there's still a race here", no?
Yes, there is, BUT as you are creating all of these attributes "on the fly" for now, I don't see a simple conversion to fix that up. Let me do these, the easy ones first. Your dynamic attribute allocations are the harder things to do, let me think about those after I've fixed the rest of the tree up with the trivial ones :)
Sounds good to me.. Yes the dynamic ones are the one that need attention. How do you propose to handle these?
Now that we manually created our own attribute group list, the outdated ATTRIBUTE_GROUPS() comments can be removed as they are not needed at all.
Cc: Vinod Koul vkoul@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Sanyog Kale sanyog.r.kale@intel.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/soundwire/sysfs_slave.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 070e0d84be94..5b7666d27722 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -129,10 +129,6 @@ static struct attribute *slave_dev_attrs[] = { NULL, };
-/* - * we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory - * for device-level properties - */ static const struct attribute_group sdw_slave_dev_attr_group = { .attrs = slave_dev_attrs, .name = "dev-properties", @@ -194,10 +190,6 @@ static struct attribute *dp0_attrs[] = { NULL, };
-/* - * we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory - * for dp0-level properties - */ static const struct attribute_group dp0_group = { .attrs = dp0_attrs, .is_visible = dp0_is_visible,
On 29-07-22, 15:50, Greg Kroah-Hartman wrote:
The sysfs logic already creates a list of groups for the device, so add the sdw_slave_dev_attr_group group to that list instead of having to do a two-step process of adding a group list and then an individual group.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Hey Greg,
DO you have a v2 for this, or if you are too busy I can update the patchset...
On Tue, Aug 23, 2022 at 09:30:02PM +0530, Vinod Koul wrote:
On 29-07-22, 15:50, Greg Kroah-Hartman wrote:
The sysfs logic already creates a list of groups for the device, so add the sdw_slave_dev_attr_group group to that list instead of having to do a two-step process of adding a group list and then an individual group.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Hey Greg,
DO you have a v2 for this, or if you are too busy I can update the patchset...
Oh wait, I did rebase it, I'll send it out this afternoon, thanks for the reminder...
greg k-h
On Tue, Aug 23, 2022 at 09:30:02PM +0530, Vinod Koul wrote:
On 29-07-22, 15:50, Greg Kroah-Hartman wrote:
The sysfs logic already creates a list of groups for the device, so add the sdw_slave_dev_attr_group group to that list instead of having to do a two-step process of adding a group list and then an individual group.
This is a step on the way to moving all of the sysfs attribute handling into the default driver core attribute group logic so that the soundwire core does not have to do any of it manually.
Hey Greg,
DO you have a v2 for this, or if you are too busy I can update the patchset...
v2 is now here: https://lore.kernel.org/r/20220824135951.3604059-1-gregkh@linuxfoundation.or...
participants (3)
-
Greg Kroah-Hartman -
Pierre-Louis Bossart -
Vinod Koul