[v4,07/15] soundwire: slave: move uevent handling to slave
diff mbox series

Message ID 20191213050409.12776-8-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • soundwire: intel: implement new ASoC interfaces
Related show

Commit Message

Pierre-Louis Bossart Dec. 13, 2019, 5:04 a.m. UTC
Currently the code deals with uevents at the bus level, but we only care
for Slave events

Suggested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 3 +--
 drivers/soundwire/slave.c    | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

Comments

Greg KH Dec. 13, 2019, 7:22 a.m. UTC | #1
On Thu, Dec 12, 2019 at 11:04:01PM -0600, Pierre-Louis Bossart wrote:
> Currently the code deals with uevents at the bus level, but we only care
> for Slave events

What does this mean?  I can't understand it, can you please provide more
information on what you are doing here?

> 
> Suggested-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.h      | 2 ++
>  drivers/soundwire/bus_type.c | 3 +--
>  drivers/soundwire/slave.c    | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index cb482da914da..be01a5f3d00b 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -6,6 +6,8 @@
>  
>  #define DEFAULT_BANK_SWITCH_TIMEOUT 3000
>  
> +int sdw_uevent(struct device *dev, struct kobj_uevent_env *env);
> +
>  #if IS_ENABLED(CONFIG_ACPI)
>  int sdw_acpi_find_slaves(struct sdw_bus *bus);
>  #else
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index bbdedce5eb26..5c18c21545b5 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -47,7 +47,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
>  			slave->id.mfg_id, slave->id.part_id);
>  }
>  
> -static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> +int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct sdw_slave *slave;
>  	char modalias[32];
> @@ -71,7 +71,6 @@ static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  struct bus_type sdw_bus_type = {
>  	.name = "soundwire",
>  	.match = sdw_bus_match,
> -	.uevent = sdw_uevent,
>  };
>  EXPORT_SYMBOL_GPL(sdw_bus_type);
>  
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index c87267f12a3b..014c3ece1f17 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -17,6 +17,7 @@ static void sdw_slave_release(struct device *dev)
>  struct device_type sdw_slave_type = {
>  	.name =		"sdw_slave",
>  	.release =	sdw_slave_release,
> +	.uevent = sdw_uevent,

Align this with the other ones?

does this cause any different functionality?

thanks,

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 3:11 p.m. UTC | #2
On 12/13/19 1:22 AM, Greg KH wrote:
> On Thu, Dec 12, 2019 at 11:04:01PM -0600, Pierre-Louis Bossart wrote:
>> Currently the code deals with uevents at the bus level, but we only care
>> for Slave events
> 
> What does this mean?  I can't understand it, can you please provide more
> information on what you are doing here?

In the earlier versions of the patch, the code looks like this and there 
was an open on what to do with a master-specific event.

  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
  {
+	struct sdw_master_device *md;
  	struct sdw_slave *slave;
  	char modalias[32];

-	if (is_sdw_slave(dev)) {
+	if (is_sdw_md(dev)) {
+		md = to_sdw_master_device(dev);
+		/* TODO: do we need to call add_uevent_var() ? */
+	} else if (is_sdw_slave(dev)) {
  		slave = to_sdw_slave_device(dev);
+
+		sdw_slave_modalias(slave, modalias, sizeof(modalias));
+
+		if (add_uevent_var(env, "MODALIAS=%s", modalias))
+			return -ENOMEM;
  	} else {
  		dev_warn(dev, "uevent for unknown Soundwire type\n");
  		return -EINVAL;
  	}

Vinod suggested this was not needed and suggested the code for uevents 
be moved to be slave-specific, which is what this patch does.
>> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
>> index c87267f12a3b..014c3ece1f17 100644
>> --- a/drivers/soundwire/slave.c
>> +++ b/drivers/soundwire/slave.c
>> @@ -17,6 +17,7 @@ static void sdw_slave_release(struct device *dev)
>>   struct device_type sdw_slave_type = {
>>   	.name =		"sdw_slave",
>>   	.release =	sdw_slave_release,
>> +	.uevent = sdw_uevent,
> 
> Align this with the other ones?
> 
> does this cause any different functionality?

As mentioned above, this move was suggested by Vinod. I don't have a 
specific need for uevents for the master and there's no functionality 
limitation, that said this is way beyond my comfort zone so I will 
follow recommendations, if any.
Greg KH Dec. 13, 2019, 4:11 p.m. UTC | #3
On Fri, Dec 13, 2019 at 09:11:27AM -0600, Pierre-Louis Bossart wrote:
> On 12/13/19 1:22 AM, Greg KH wrote:
> > On Thu, Dec 12, 2019 at 11:04:01PM -0600, Pierre-Louis Bossart wrote:
> > > Currently the code deals with uevents at the bus level, but we only care
> > > for Slave events
> > 
> > What does this mean?  I can't understand it, can you please provide more
> > information on what you are doing here?
> 
> In the earlier versions of the patch, the code looks like this and there was
> an open on what to do with a master-specific event.
> 
>  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> +	struct sdw_master_device *md;
>  	struct sdw_slave *slave;
>  	char modalias[32];
> 
> -	if (is_sdw_slave(dev)) {
> +	if (is_sdw_md(dev)) {
> +		md = to_sdw_master_device(dev);
> +		/* TODO: do we need to call add_uevent_var() ? */
> +	} else if (is_sdw_slave(dev)) {
>  		slave = to_sdw_slave_device(dev);
> +
> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +
> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
> +			return -ENOMEM;
>  	} else {
>  		dev_warn(dev, "uevent for unknown Soundwire type\n");
>  		return -EINVAL;
>  	}
> 
> Vinod suggested this was not needed and suggested the code for uevents be
> moved to be slave-specific, which is what this patch does.

Then describe it really really well in the changelog text.  We have no
rememberance of prior conversations when looking at commits in the tree
in the future.

thaniks,

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 10:15 p.m. UTC | #4
On 12/13/19 10:11 AM, Greg KH wrote:
> On Fri, Dec 13, 2019 at 09:11:27AM -0600, Pierre-Louis Bossart wrote:
>> On 12/13/19 1:22 AM, Greg KH wrote:
>>> On Thu, Dec 12, 2019 at 11:04:01PM -0600, Pierre-Louis Bossart wrote:
>>>> Currently the code deals with uevents at the bus level, but we only care
>>>> for Slave events
>>>
>>> What does this mean?  I can't understand it, can you please provide more
>>> information on what you are doing here?
>>
>> In the earlier versions of the patch, the code looks like this and there was
>> an open on what to do with a master-specific event.
>>
>>   static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   {
>> +	struct sdw_master_device *md;
>>   	struct sdw_slave *slave;
>>   	char modalias[32];
>>
>> -	if (is_sdw_slave(dev)) {
>> +	if (is_sdw_md(dev)) {
>> +		md = to_sdw_master_device(dev);
>> +		/* TODO: do we need to call add_uevent_var() ? */
>> +	} else if (is_sdw_slave(dev)) {
>>   		slave = to_sdw_slave_device(dev);
>> +
>> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
>> +
>> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> +			return -ENOMEM;
>>   	} else {
>>   		dev_warn(dev, "uevent for unknown Soundwire type\n");
>>   		return -EINVAL;
>>   	}
>>
>> Vinod suggested this was not needed and suggested the code for uevents be
>> moved to be slave-specific, which is what this patch does.
> 
> Then describe it really really well in the changelog text.  We have no
> rememberance of prior conversations when looking at commits in the tree
> in the future.

ok, will do.

Patch
diff mbox series

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index cb482da914da..be01a5f3d00b 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -6,6 +6,8 @@ 
 
 #define DEFAULT_BANK_SWITCH_TIMEOUT 3000
 
+int sdw_uevent(struct device *dev, struct kobj_uevent_env *env);
+
 #if IS_ENABLED(CONFIG_ACPI)
 int sdw_acpi_find_slaves(struct sdw_bus *bus);
 #else
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index bbdedce5eb26..5c18c21545b5 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -47,7 +47,7 @@  int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 			slave->id.mfg_id, slave->id.part_id);
 }
 
-static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
+int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct sdw_slave *slave;
 	char modalias[32];
@@ -71,7 +71,6 @@  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 struct bus_type sdw_bus_type = {
 	.name = "soundwire",
 	.match = sdw_bus_match,
-	.uevent = sdw_uevent,
 };
 EXPORT_SYMBOL_GPL(sdw_bus_type);
 
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index c87267f12a3b..014c3ece1f17 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -17,6 +17,7 @@  static void sdw_slave_release(struct device *dev)
 struct device_type sdw_slave_type = {
 	.name =		"sdw_slave",
 	.release =	sdw_slave_release,
+	.uevent = sdw_uevent,
 };
 
 static int sdw_slave_add(struct sdw_bus *bus,