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 <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Sanyog Kale <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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)
--
2.37.1
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 <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Sanyog Kale <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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,
--
2.37.1
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 <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Sanyog Kale <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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)
--
2.37.1
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 <[email protected]>
> > Cc: Bard Liao <[email protected]>
> > Cc: Pierre-Louis Bossart <[email protected]>
> > Cc: Sanyog Kale <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > 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 <[email protected]>
>>> Cc: Bard Liao <[email protected]>
>>> Cc: Pierre-Louis Bossart <[email protected]>
>>> Cc: Sanyog Kale <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>>> ---
>>> 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 <[email protected]>
> >>> Cc: Bard Liao <[email protected]>
> >>> Cc: Pierre-Louis Bossart <[email protected]>
> >>> Cc: Sanyog Kale <[email protected]>
> >>> Cc: [email protected]
> >>> Cc: [email protected]
> >>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >>> ---
> >>> 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
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 <[email protected]>
> Cc: Bard Liao <[email protected]>
> Cc: Pierre-Louis Bossart <[email protected]>
> Cc: Sanyog Kale <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> 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)
>>>>> 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 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 <[email protected]>
> > >>> Cc: Bard Liao <[email protected]>
> > >>> Cc: Pierre-Louis Bossart <[email protected]>
> > >>> Cc: Sanyog Kale <[email protected]>
> > >>> Cc: [email protected]
> > >>> Cc: [email protected]
> > >>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > >>> ---
> > >>> 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
--
~Vinod
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...
--
~Vinod
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 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 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/[email protected]