[PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

[PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

[Greg Kroah-Hartman]

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)
-- 
2.37.1

[PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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)
-- 
2.37.1

[PATCH 3/5] soundwire: sysfs: have the driver core handle the creation of the device groups

[Greg Kroah-Hartman]

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)
-- 
2.37.1

[PATCH 4/5] soundwire: sysfs: remove sdw_slave_sysfs_init()

[Greg Kroah-Hartman]

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);
-- 
2.37.1

[PATCH 5/5] soundwire: sysfs: remove unneeded ATTRIBUTE_GROUPS() comments

[Greg Kroah-Hartman]

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,
-- 
2.37.1

Re: [PATCH 3/5] soundwire: sysfs: have the driver core handle the creation of the device groups

[Pierre-Louis Bossart]

> 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.

Re: [PATCH 3/5] soundwire: sysfs: have the driver core handle the creation of the device groups

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Pierre-Louis Bossart]

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)

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Pierre-Louis Bossart]

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.

Re: [PATCH 4/5] soundwire: sysfs: remove sdw_slave_sysfs_init()

[Pierre-Louis Bossart]

> 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?

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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

Re: [PATCH 4/5] soundwire: sysfs: remove sdw_slave_sysfs_init()

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Pierre-Louis Bossart]

>>>>> 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.

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Pierre-Louis Bossart]

>>>>> 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....

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Pierre-Louis Bossart]

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.

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH 4/5] soundwire: sysfs: remove sdw_slave_sysfs_init()

[Vinod Koul]

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?

-- 
~Vinod

Re: [PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

[Vinod Koul]

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

Re: [PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

[Greg Kroah-Hartman]

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

Re: [PATCH 2/5] soundwire: sysfs: cleanup the logic for creating the dp0 sysfs attributes

[Greg Kroah-Hartman]

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

Re: [PATCH 1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

[Greg Kroah-Hartman]

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.org