| Message ID | 20220729135041.2285908-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups | expand |
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 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
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
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)
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(-)