diff mbox series

[2/6] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups

Message ID 2024013029-afternoon-suitably-cb59@gregkh
State New
Headers
Series Soundwire: clean up sysfs group creation |

Commit Message

Greg KH Jan. 30, 2024, 6:46 p.m. UTC 
  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(-)

Comments

Dan Williams Jan. 31, 2024, 5:20 a.m. UTC | #1 
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.
> 
> 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;
> -

Yeah, open-coding the groups, much better than dynamically adding one.

>  	if (slave->prop.dp0_prop) {
>  		ret = devm_device_add_group(&slave->dev, &dp0_group);
>  		if (ret < 0)

Makes sense. I won't say "looks good" as this file has "slave" all over
the place, but I checked and it entered the kernel just before the
CodingStyle changed.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Pierre-Louis Bossart Jan. 31, 2024, 7:12 a.m. UTC | #2 
> Makes sense. I won't say "looks good" as this file has "slave" all over
> the place, but I checked and it entered the kernel just before the
> CodingStyle changed.

SoundWire 1.2.1 introduced the terms "Manager" and "Peripheral", I had a 
patchset to rename everything maybe two years ago already but it's been 
difficult to add without getting in the way of development and 
backports. Maybe a gradual replacement makes more sense, not sure how to 
go about this.
Greg KH Feb. 1, 2024, 2:56 p.m. UTC | #3 
On Wed, Jan 31, 2024 at 08:12:10AM +0100, Pierre-Louis Bossart wrote:
> 
> > Makes sense. I won't say "looks good" as this file has "slave" all over
> > the place, but I checked and it entered the kernel just before the
> > CodingStyle changed.
> 
> SoundWire 1.2.1 introduced the terms "Manager" and "Peripheral", I had a
> patchset to rename everything maybe two years ago already but it's been
> difficult to add without getting in the way of development and backports.

Don't worry about backports for stable, we can handle that.  Development
for fixes or changes should NEVER worry about stable kernels, worst
case, we can just take all of the same changes into them, no problem.

> Maybe a gradual replacement makes more sense, not sure how to go about this.

Just rename it all.

thanks,

greg k-h
diff mbox series

Patch

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)