[alsa-devel] [PATCH 0/4] soundwire: few trival fixes and cleanups.
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(-)
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 */
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
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);
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);
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);
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);
--
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!
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
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);
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);
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/driv...
+ 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
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/driv...
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
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/driv...
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
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
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 *
participants (5)
-
Pierre-Louis Bossart -
Ranjani Sridharan -
Sanyog Kale -
Srinivas Kandagatla -
Vinod Koul