[PATCH 0/4] soundwire: few trival fixes and cleanups.

[PATCH 0/4] soundwire: few trival fixes and cleanups.

[Srinivas Kandagatla]

Hi Vinod,
Here are few trivial updates to soundwire which I found while
trying out Qualcomm WCD934x codecs.

Thanks,

Srinivas Kandagatla (4):
  soundwire: add module_sdw_driver helper macro
  soundwire: cdns: fix check for number of messages
  soundwire: bus: check if pm runtime is enabled before calling
  soundwire: remove unused struct sdw_bus_conf definition

 drivers/soundwire/bus.c            | 16 ++++++++++------
 drivers/soundwire/cadence_master.c |  2 +-
 include/linux/soundwire/sdw.h      | 15 ---------------
 include/linux/soundwire/sdw_type.h | 11 +++++++++++
 4 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.21.0

[PATCH 1/4] soundwire: add module_sdw_driver helper macro

[Srinivas Kandagatla]

This Helper macro is for Soundwire drivers which do not do anything special in
module init/exit. This eliminates a lot of boilerplate. Each module may only
use this macro once, and calling it replaces module_init() and module_exit()

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/linux/soundwire/sdw_type.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index 9fd553e553e9..e14843ed13a5 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -16,4 +16,15 @@ void sdw_unregister_driver(struct sdw_driver *drv);
 
 int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
 
+/**
+ * module_sdw_driver() - Helper macro for registering a Soundwire driver
+ * @__sdw_driver: soundwire slave driver struct
+ *
+ * Helper macro for Soundwire drivers which do not do anything special in
+ * module init/exit. This eliminates a lot of boilerplate. Each module may only
+ * use this macro once, and calling it replaces module_init() and module_exit()
+ */
+#define module_sdw_driver(__sdw_driver) \
+	module_driver(__sdw_driver, sdw_register_driver, \
+			sdw_unregister_driver)
 #endif /* __SOUNDWIRE_TYPES_H */
-- 
2.21.0

[PATCH 2/4] soundwire: cdns: fix check for number of messages

[Srinivas Kandagatla]

Looks like there is a typo while checking number of messages, which
should be checked in defer pointer rather than message length.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/cadence_master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index cb6a331f448a..d41adbc57918 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
 	int cmd = 0, ret;
 
 	/* for defer only 1 message is supported */
-	if (msg->len > 1)
+	if (defer->length > 1)
 		return -ENOTSUPP;
 
 	ret = cdns_prep_msg(cdns, msg, &cmd);
-- 
2.21.0

[PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Srinivas Kandagatla]

Check the device has pm runtime enabled before returning error.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/bus.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1cbfedfc20ef..101562a6fb0d 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
-		return ret;
+	if (pm_runtime_enabled(slave->bus->dev)) {
+		ret = pm_runtime_get_sync(slave->bus->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = sdw_transfer(slave->bus, &msg);
 	pm_runtime_put(slave->bus->dev);
@@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
-		return ret;
+	if (pm_runtime_enabled(slave->bus->dev)) {
+		ret = pm_runtime_get_sync(slave->bus->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = sdw_transfer(slave->bus, &msg);
 	pm_runtime_put(slave->bus->dev);
-- 
2.21.0

[PATCH 4/4] soundwire: remove unused struct sdw_bus_conf definition

[Srinivas Kandagatla]

struct sdw_bus_conf seems to be replaced with struct sdw_bus_params
so remove this unused data structure to avoid confusion to readers.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/linux/soundwire/sdw.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index df313913e856..a8129745b7ec 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -389,21 +389,6 @@ enum sdw_reg_bank {
 	SDW_BANK1,
 };
 
-/**
- * struct sdw_bus_conf: Bus configuration
- *
- * @clk_freq: Clock frequency, in Hz
- * @num_rows: Number of rows in frame
- * @num_cols: Number of columns in frame
- * @bank: Next register bank
- */
-struct sdw_bus_conf {
-	unsigned int clk_freq;
-	unsigned int num_rows;
-	unsigned int num_cols;
-	unsigned int bank;
-};
-
 /**
  * struct sdw_prepare_ch: Prepare/De-prepare Data Port channel
  *
-- 
2.21.0

Re: [PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Pierre-Louis Bossart]

On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> Check the device has pm runtime enabled before returning error.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/bus.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 1cbfedfc20ef..101562a6fb0d 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = pm_runtime_get_sync(slave->bus->dev);
> -	if (ret < 0)
> -		return ret;
> +	if (pm_runtime_enabled(slave->bus->dev)) {
> +		ret = pm_runtime_get_sync(slave->bus->dev);

Is this an recommended/accepted sequence in kernel circles? I did a 
quick git grep and don't see anyone using this sort of tests.


> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	ret = sdw_transfer(slave->bus, &msg);
>   	pm_runtime_put(slave->bus->dev);

and the weird thing is that you don't test for the put() case?

> @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = pm_runtime_get_sync(slave->bus->dev);
> -	if (ret < 0)
> -		return ret;
> +	if (pm_runtime_enabled(slave->bus->dev)) {
> +		ret = pm_runtime_get_sync(slave->bus->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	ret = sdw_transfer(slave->bus, &msg);
>   	pm_runtime_put(slave->bus->dev);
> 

Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Pierre-Louis Bossart]

On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> Looks like there is a typo while checking number of messages, which
> should be checked in defer pointer rather than message length.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/cadence_master.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index cb6a331f448a..d41adbc57918 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
>   	int cmd = 0, ret;
>   
>   	/* for defer only 1 message is supported */
> -	if (msg->len > 1)

I am not sure this is a typo.

IRRC the hardware only defer e.g. a single write that can perform bank 
switches and writes at specific times indicated with an SSP. What's more 
is that the defer structure is actually modified below this code (not 
shown in the diff) to set defer>length = msg->len, so testing before the 
value is set looks more suspicious than the current code.

Vinod, does this ring a bell?


> +	if (defer->length > 1)
>   		return -ENOTSUPP;
>   
>   	ret = cdns_prep_msg(cdns, msg, &cmd);
> 

Re: [PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Srinivas Kandagatla]

On 28/03/2019 13:55, Pierre-Louis Bossart wrote:
> On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
>> Check the device has pm runtime enabled before returning error.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/bus.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index 1cbfedfc20ef..101562a6fb0d 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, 
>> size_t count, u8 *val)
>>       if (ret < 0)
>>           return ret;
>> -    ret = pm_runtime_get_sync(slave->bus->dev);
>> -    if (ret < 0)
>> -        return ret;
>> +    if (pm_runtime_enabled(slave->bus->dev)) {
>> +        ret = pm_runtime_get_sync(slave->bus->dev);
> 
> Is this an recommended/accepted sequence in kernel circles? I did a 
> quick git grep and don't see anyone using this sort of tests.

There are lots of users in kernel with such sequences, ex:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279

> 
> 
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>>       ret = sdw_transfer(slave->bus, &msg);
>>       pm_runtime_put(slave->bus->dev);
> 
> and the weird thing is that you don't test for the put() case?
> 
>> @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, 
>> size_t count, u8 *val)
>>       if (ret < 0)
>>           return ret;
>> -    ret = pm_runtime_get_sync(slave->bus->dev);
>> -    if (ret < 0)
>> -        return ret;
>> +    if (pm_runtime_enabled(slave->bus->dev)) {
>> +        ret = pm_runtime_get_sync(slave->bus->dev);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>>       ret = sdw_transfer(slave->bus, &msg);
>>       pm_runtime_put(slave->bus->dev);

I should add a check in this path as well.

Thanks,
srini

>>
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Srinivas Kandagatla]

On 28/03/2019 14:03, Pierre-Louis Bossart wrote:
>> ---
>>   drivers/soundwire/cadence_master.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c 
>> b/drivers/soundwire/cadence_master.c
>> index cb6a331f448a..d41adbc57918 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
>>       int cmd = 0, ret;
>>       /* for defer only 1 message is supported */
>> -    if (msg->len > 1)
> 
> I am not sure this is a typo.
> 

Am bit confused with defer data-structure itself.

I was assuming that defer->length is number of sdw messages in the 
structure. If its same as message lenght then sdw_msg->len and 
sdw_defer->length are totally redundant. May be we should get rid of 
lenght field in defer to avoid confusion?

> IRRC the hardware only defer e.g. a single write that can perform bank 
> switches and writes at specific times indicated with an SSP. What's more 
okay got it.
Does that mean that candence controller can only support 1 byte defer 
transfers?

> is that the defer structure is actually modified below this code (not 
> shown in the diff) to set defer>length = msg->len, so testing before the 
> value is set looks more suspicious than the current code.

removing the length field in defer should get rid of such instances.

--srini

> 
> Vinod, does this ring a bell?
> 
> 
>> +    if (defer->length > 1)
>>           return -ENOTSUPP;
>>       ret = cdns_prep_msg(cdns, msg, &cmd);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Sanyog Kale]

On Thu, Mar 28, 2019 at 02:58:27PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 28/03/2019 14:03, Pierre-Louis Bossart wrote:
> > > ---
> > > ?? drivers/soundwire/cadence_master.c | 2 +-
> > > ?? 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c
> > > b/drivers/soundwire/cadence_master.c
> > > index cb6a331f448a..d41adbc57918 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
> > > ?????????? int cmd = 0, ret;
> > > ?????????? /* for defer only 1 message is supported */
> > > -?????? if (msg->len > 1)
> > 
> > I am not sure this is a typo.
> > 
> 
> Am bit confused with defer data-structure itself.
> 
> I was assuming that defer->length is number of sdw messages in the
> structure. If its same as message lenght then sdw_msg->len and
> sdw_defer->length are totally redundant. May be we should get rid of lenght
> field in defer to avoid confusion?
> 
> > IRRC the hardware only defer e.g. a single write that can perform bank
> > switches and writes at specific times indicated with an SSP. What's more
> okay got it.
> Does that mean that candence controller can only support 1 byte defer
> transfers?
> 
> > is that the defer structure is actually modified below this code (not
> > shown in the diff) to set defer>length = msg->len, so testing before the
> > value is set looks more suspicious than the current code.
> 
> removing the length field in defer should get rid of such instances.
>

defer->length is passed as count in cdns_fill_msg_resp function while
processing response of defer message. we can as well use msg->len and
remove length from sdw_defer. I dont see any other instance where
defer->length is used.

> --srini
> 
> > 
> > Vinod, does this ring a bell?
> > 
> > 
> > > +?????? if (defer->length > 1)
> > > ?????????????????? return -ENOTSUPP;
> > > ?????????? ret = cdns_prep_msg(cdns, msg, &cmd);

-- 

Re: [PATCH 1/4] soundwire: add module_sdw_driver helper macro

[Vinod Koul]

On 28-03-19, 13:41, Srinivas Kandagatla wrote:
> This Helper macro is for Soundwire drivers which do not do anything special in

s/Soundwire/SoundWire

Most of the 'documentation' uses SoundWire as that is spec name, unless
you are referring to subsystem in which case all will be lowercase :)

> module init/exit. This eliminates a lot of boilerplate. Each module may only
> use this macro once, and calling it replaces module_init() and module_exit()
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  include/linux/soundwire/sdw_type.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
> index 9fd553e553e9..e14843ed13a5 100644
> --- a/include/linux/soundwire/sdw_type.h
> +++ b/include/linux/soundwire/sdw_type.h
> @@ -16,4 +16,15 @@ void sdw_unregister_driver(struct sdw_driver *drv);
>  
>  int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
>  
> +/**
> + * module_sdw_driver() - Helper macro for registering a Soundwire driver
> + * @__sdw_driver: soundwire slave driver struct
> + *
> + * Helper macro for Soundwire drivers which do not do anything special in
> + * module init/exit. This eliminates a lot of boilerplate. Each module may only
> + * use this macro once, and calling it replaces module_init() and module_exit()
> + */
> +#define module_sdw_driver(__sdw_driver) \
> +	module_driver(__sdw_driver, sdw_register_driver, \
> +			sdw_unregister_driver)
>  #endif /* __SOUNDWIRE_TYPES_H */
> -- 
> 2.21.0

-- 
~Vinod

Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Vinod Koul]

On 28-03-19, 14:58, Srinivas Kandagatla wrote:
> 
> 
> On 28/03/2019 14:03, Pierre-Louis Bossart wrote:
> > > ---
> > >   drivers/soundwire/cadence_master.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c
> > > b/drivers/soundwire/cadence_master.c
> > > index cb6a331f448a..d41adbc57918 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
> > >       int cmd = 0, ret;
> > >       /* for defer only 1 message is supported */
> > > -    if (msg->len > 1)
> > 
> > I am not sure this is a typo.
> > 
> 
> Am bit confused with defer data-structure itself.
> 
> I was assuming that defer->length is number of sdw messages in the
> structure. If its same as message lenght then sdw_msg->len and
> sdw_defer->length are totally redundant. May be we should get rid of lenght
> field in defer to avoid confusion?

Sorry Srini, I didnt pay much attention to it last time we discussed. So
relooking at the code, we send msg as a message to trf (defered or not)
and then copy length few lines down, but I agree this is not most useful
and should be cleaned up.

> > IRRC the hardware only defer e.g. a single write that can perform bank
> > switches and writes at specific times indicated with an SSP. What's more
> okay got it.
> Does that mean that candence controller can only support 1 byte defer
> transfers?

I think so, Pierre can confirm.

> > is that the defer structure is actually modified below this code (not
> > shown in the diff) to set defer>length = msg->len, so testing before the
> > value is set looks more suspicious than the current code.
> 
> removing the length field in defer should get rid of such instances.

Right!

-- 
~Vinod

Re: [PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Vinod Koul]

On 28-03-19, 14:37, Srinivas Kandagatla wrote:
> 
> 
> On 28/03/2019 13:55, Pierre-Louis Bossart wrote:
> > On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> > > Check the device has pm runtime enabled before returning error.
> > > 
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > ---
> > >   drivers/soundwire/bus.c | 16 ++++++++++------
> > >   1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > > index 1cbfedfc20ef..101562a6fb0d 100644
> > > --- a/drivers/soundwire/bus.c
> > > +++ b/drivers/soundwire/bus.c
> > > @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32
> > > addr, size_t count, u8 *val)
> > >       if (ret < 0)
> > >           return ret;
> > > -    ret = pm_runtime_get_sync(slave->bus->dev);
> > > -    if (ret < 0)
> > > -        return ret;
> > > +    if (pm_runtime_enabled(slave->bus->dev)) {
> > > +        ret = pm_runtime_get_sync(slave->bus->dev);
> > 
> > Is this an recommended/accepted sequence in kernel circles? I did a
> > quick git grep and don't see anyone using this sort of tests.
> 
> There are lots of users in kernel with such sequences, ex:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279

Sorry not a fan of this :) I think PM core should do this :D

I guess you are trying to solve the case where PM can be disabled for a
device and pm_runtime_get_sync() returns error right?

That would be quite a common sequence in most frameworks..

Thanks


> 
> > 
> > 
> > > +        if (ret < 0)
> > > +            return ret;
> > > +    }
> > >       ret = sdw_transfer(slave->bus, &msg);
> > >       pm_runtime_put(slave->bus->dev);
> > 
> > and the weird thing is that you don't test for the put() case?
> > 
> > > @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32
> > > addr, size_t count, u8 *val)
> > >       if (ret < 0)
> > >           return ret;
> > > -    ret = pm_runtime_get_sync(slave->bus->dev);
> > > -    if (ret < 0)
> > > -        return ret;
> > > +    if (pm_runtime_enabled(slave->bus->dev)) {
> > > +        ret = pm_runtime_get_sync(slave->bus->dev);
> > > +        if (ret < 0)
> > > +            return ret;
> > > +    }
> > >       ret = sdw_transfer(slave->bus, &msg);
> > >       pm_runtime_put(slave->bus->dev);
> 
> I should add a check in this path as well.
> 
> Thanks,
> srini
> 
> > > 
> > 

-- 
~Vinod

Re: [PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Srinivas Kandagatla]

On 29/03/2019 05:58, Vinod Koul wrote:
>> There are lots of users in kernel with such sequences, ex:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279
> Sorry not a fan of this:)  I think PM core should do this :D
> 
Yes, I would prefer that too, There might be a reason why its not done 
in the pm core, I will catchup with Ulf next week and see if we can add 
checks to return -ENOSUPPORT  or something more sensible error code to 
users.


Thanks,
srini
> I guess you are trying to solve the case where PM can be disabled for a
> device and pm_runtime_get_sync() returns error right?
> 
> That would be quite a common sequence in most frameworks..
> 
> Thanks
> 
> 

Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Srinivas Kandagatla]

On 29/03/2019 05:54, Vinod Koul wrote:
>>> is that the defer structure is actually modified below this code (not
>>> shown in the diff) to set defer>length = msg->len, so testing before the
>>> value is set looks more suspicious than the current code.
>> removing the length field in defer should get rid of such instances.
> Right!
I will go ahead and post the cleanup patch to get rid of 
confusing/redundant length field in defer.

Thanks,
srini

Re: [PATCH 3/4] soundwire: bus: check if pm runtime is enabled before calling

[Ranjani Sridharan]

On Thu, 2019-03-28 at 09:55 -0400, Pierre-Louis Bossart wrote:
> On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> > Check the device has pm runtime enabled before returning error.
> > 
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > ---
> >   drivers/soundwire/bus.c | 16 ++++++++++------
> >   1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > index 1cbfedfc20ef..101562a6fb0d 100644
> > --- a/drivers/soundwire/bus.c
> > +++ b/drivers/soundwire/bus.c
> > @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32
> > addr, size_t count, u8 *val)
> >   	if (ret < 0)
> >   		return ret;
> >   
> > -	ret = pm_runtime_get_sync(slave->bus->dev);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (pm_runtime_enabled(slave->bus->dev)) {
> > +		ret = pm_runtime_get_sync(slave->bus->dev);
> 
> Is this an recommended/accepted sequence in kernel circles? I did a 
> quick git grep and don't see anyone using this sort of tests.
Hi Srinivas/Pierre,

Sorry for the delayed reply.

The only instance where I have seen the pm_runtime_get_sync() fail is
not because pm_runtime was disabled. But it is because the power is
powered off when trying to do a pm_runtime_get_sync().

I'm not very familiar with the code in soundwire yet, but is it
possible that the pm_domain supplier has powered off the soundwire
device and would cause a failure in pm_runtime_get_sync()?

Thanks,
Ranjani

> 
> 
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >   
> >   	ret = sdw_transfer(slave->bus, &msg);
> >   	pm_runtime_put(slave->bus->dev);
> 
> and the weird thing is that you don't test for the put() case?
> 
> > @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32
> > addr, size_t count, u8 *val)
> >   	if (ret < 0)
> >   		return ret;
> >   
> > -	ret = pm_runtime_get_sync(slave->bus->dev);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (pm_runtime_enabled(slave->bus->dev)) {
> > +		ret = pm_runtime_get_sync(slave->bus->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >   
> >   	ret = sdw_transfer(slave->bus, &msg);
> >   	pm_runtime_put(slave->bus->dev);
> > 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel