Just some minor tidy ups and preparation for starting to upstream some
Cirrus SoundWire devices. The first three patches are pretty trivial,
the last patch which moves the remaining core over to using the no_pm
functions could probably use some careful review.
Thanks,
Charles
Charles Keepax (3):
soundwire: Provide build stubs for common functions
soundwire: debugfs: Switch to sdw_read_no_pm
soundwire: stream: Move remaining register accesses over to no_pm
Simon Trimmer (1):
soundwire: bus: export sdw_nwrite_no_pm and sdw_nread_no_pm functions
drivers/soundwire/bus.c | 10 ++--
drivers/soundwire/debugfs.c | 11 +++-
drivers/soundwire/stream.c | 30 +++++------
include/linux/soundwire/sdw.h | 94 +++++++++++++++++++++++++++++++----
4 files changed, 114 insertions(+), 31 deletions(-)
--
2.30.2
Provide stub functions when CONFIG_SOUNDWIRE is not set for functions
that are quite likely to be used from common code on devices supporting
multiple control buses.
Signed-off-by: Charles Keepax <[email protected]>
---
include/linux/soundwire/sdw.h | 92 +++++++++++++++++++++++++++++++----
1 file changed, 82 insertions(+), 10 deletions(-)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 902ed46f76c80..4f80cba898f11 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -1021,15 +1021,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
struct sdw_port_config *port_config,
unsigned int num_ports,
struct sdw_stream_runtime *stream);
-int sdw_stream_add_slave(struct sdw_slave *slave,
- struct sdw_stream_config *stream_config,
- struct sdw_port_config *port_config,
- unsigned int num_ports,
- struct sdw_stream_runtime *stream);
int sdw_stream_remove_master(struct sdw_bus *bus,
struct sdw_stream_runtime *stream);
-int sdw_stream_remove_slave(struct sdw_slave *slave,
- struct sdw_stream_runtime *stream);
int sdw_startup_stream(void *sdw_substream);
int sdw_prepare_stream(struct sdw_stream_runtime *stream);
int sdw_enable_stream(struct sdw_stream_runtime *stream);
@@ -1040,8 +1033,20 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus);
int sdw_bus_clk_stop(struct sdw_bus *bus);
int sdw_bus_exit_clk_stop(struct sdw_bus *bus);
-/* messaging and data APIs */
+int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id);
+void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);
+
+#if IS_ENABLED(CONFIG_SOUNDWIRE)
+int sdw_stream_add_slave(struct sdw_slave *slave,
+ struct sdw_stream_config *stream_config,
+ struct sdw_port_config *port_config,
+ unsigned int num_ports,
+ struct sdw_stream_runtime *stream);
+int sdw_stream_remove_slave(struct sdw_slave *slave,
+ struct sdw_stream_runtime *stream);
+
+/* messaging and data APIs */
int sdw_read(struct sdw_slave *slave, u32 addr);
int sdw_write(struct sdw_slave *slave, u32 addr, u8 value);
int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value);
@@ -1053,7 +1058,74 @@ int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *
int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
-int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id);
-void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);
+#else
+
+static inline int sdw_stream_add_slave(struct sdw_slave *slave,
+ struct sdw_stream_config *stream_config,
+ struct sdw_port_config *port_config,
+ unsigned int num_ports,
+ struct sdw_stream_runtime *stream)
+{
+ return 0;
+}
+
+static inline int sdw_stream_remove_slave(struct sdw_slave *slave,
+ struct sdw_stream_runtime *stream)
+{
+ return 0;
+}
+
+/* messaging and data APIs */
+static inline int sdw_read(struct sdw_slave *slave, u32 addr)
+{
+ return 0;
+}
+
+static inline int sdw_write(struct sdw_slave *slave, u32 addr, u8 value)
+{
+ return 0;
+}
+
+static inline int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value)
+{
+ return 0;
+}
+
+static inline int sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
+{
+ return 0;
+}
+
+static inline int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+ return 0;
+}
+
+static inline int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+ return 0;
+}
+
+static inline int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
+{
+ return 0;
+}
+
+static inline int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
+{
+ return 0;
+}
+
+static inline int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
+{
+ return 0;
+}
+
+static inline int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
+{
+ return 0;
+}
+
+#endif /* CONFIG_SOUNDWIRE */
#endif /* __SOUNDWIRE_H */
--
2.30.2
There is no need to play with the runtime reference everytime a register
is accessed. All the remaining "pm" style register accesses trace back
to 4 functions:
sdw_prepare_stream
sdw_deprepare_stream
sdw_enable_stream
sdw_disable_stream
Any sensible implementation will need to hold a runtime reference
across all those functions, it makes no sense to be allowing the
device/bus to suspend whilst streams are being prepared/enabled. And
certainly in the case of the all existing users, they all call these
functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
which will have already runtime resumed all the audio devices
associated during the open callback.
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/soundwire/bus.c | 2 +-
drivers/soundwire/stream.c | 30 +++++++++++++++---------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index ef4878258afad..d87a188fcce1e 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1214,7 +1214,7 @@ int sdw_configure_dpn_intr(struct sdw_slave *slave,
val &= ~SDW_DPN_INT_PORT_READY;
}
- ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
+ ret = sdw_update_no_pm(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
if (ret < 0)
dev_err(&slave->dev,
"SDW_DPN_INTMASK write failed:%d\n", val);
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index bd502368339e5..df3b36670df4c 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -81,14 +81,14 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
}
/* Program DPN_OffsetCtrl2 registers */
- ret = sdw_write(slave, addr1, t_params->offset2);
+ ret = sdw_write_no_pm(slave, addr1, t_params->offset2);
if (ret < 0) {
dev_err(bus->dev, "DPN_OffsetCtrl2 register write failed\n");
return ret;
}
/* Program DPN_BlockCtrl3 register */
- ret = sdw_write(slave, addr2, t_params->blk_pkg_mode);
+ ret = sdw_write_no_pm(slave, addr2, t_params->blk_pkg_mode);
if (ret < 0) {
dev_err(bus->dev, "DPN_BlockCtrl3 register write failed\n");
return ret;
@@ -105,7 +105,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
/* Program DPN_SampleCtrl2 register */
wbuf = FIELD_GET(SDW_DPN_SAMPLECTRL_HIGH, t_params->sample_interval - 1);
- ret = sdw_write(slave, addr3, wbuf);
+ ret = sdw_write_no_pm(slave, addr3, wbuf);
if (ret < 0) {
dev_err(bus->dev, "DPN_SampleCtrl2 register write failed\n");
return ret;
@@ -115,7 +115,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
wbuf = FIELD_PREP(SDW_DPN_HCTRL_HSTART, t_params->hstart);
wbuf |= FIELD_PREP(SDW_DPN_HCTRL_HSTOP, t_params->hstop);
- ret = sdw_write(slave, addr4, wbuf);
+ ret = sdw_write_no_pm(slave, addr4, wbuf);
if (ret < 0)
dev_err(bus->dev, "DPN_HCtrl register write failed\n");
@@ -163,7 +163,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
wbuf = FIELD_PREP(SDW_DPN_PORTCTRL_DATAMODE, p_params->data_mode);
wbuf |= FIELD_PREP(SDW_DPN_PORTCTRL_FLOWMODE, p_params->flow_mode);
- ret = sdw_update(s_rt->slave, addr1, 0xF, wbuf);
+ ret = sdw_update_no_pm(s_rt->slave, addr1, 0xF, wbuf);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_PortCtrl register write failed for port %d\n",
@@ -173,7 +173,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
if (!dpn_prop->read_only_wordlength) {
/* Program DPN_BlockCtrl1 register */
- ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
+ ret = sdw_write_no_pm(s_rt->slave, addr2, (p_params->bps - 1));
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_BlockCtrl1 register write failed for port %d\n",
@@ -184,7 +184,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
/* Program DPN_SampleCtrl1 register */
wbuf = (t_params->sample_interval - 1) & SDW_DPN_SAMPLECTRL_LOW;
- ret = sdw_write(s_rt->slave, addr3, wbuf);
+ ret = sdw_write_no_pm(s_rt->slave, addr3, wbuf);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_SampleCtrl1 register write failed for port %d\n",
@@ -193,7 +193,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
}
/* Program DPN_OffsetCtrl1 registers */
- ret = sdw_write(s_rt->slave, addr4, t_params->offset1);
+ ret = sdw_write_no_pm(s_rt->slave, addr4, t_params->offset1);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_OffsetCtrl1 register write failed for port %d\n",
@@ -203,7 +203,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
/* Program DPN_BlockCtrl2 register*/
if (t_params->blk_grp_ctrl_valid) {
- ret = sdw_write(s_rt->slave, addr5, t_params->blk_grp_ctrl);
+ ret = sdw_write_no_pm(s_rt->slave, addr5, t_params->blk_grp_ctrl);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_BlockCtrl2 reg write failed for port %d\n",
@@ -214,7 +214,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
/* program DPN_LaneCtrl register */
if (slave_prop->lane_control_support) {
- ret = sdw_write(s_rt->slave, addr6, t_params->lane_ctrl);
+ ret = sdw_write_no_pm(s_rt->slave, addr6, t_params->lane_ctrl);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
"DPN_LaneCtrl register write failed for port %d\n",
@@ -319,9 +319,9 @@ static int sdw_enable_disable_slave_ports(struct sdw_bus *bus,
* it is safe to reset this register
*/
if (en)
- ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
+ ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
else
- ret = sdw_write(s_rt->slave, addr, 0x0);
+ ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
if (ret < 0)
dev_err(&s_rt->slave->dev,
@@ -476,9 +476,9 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
addr = SDW_DPN_PREPARECTRL(p_rt->num);
if (prep)
- ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
+ ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
else
- ret = sdw_write(s_rt->slave, addr, 0x0);
+ ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
if (ret < 0) {
dev_err(&s_rt->slave->dev,
@@ -491,7 +491,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
wait_for_completion_timeout(port_ready,
msecs_to_jiffies(dpn_prop->ch_prep_timeout));
- val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
+ val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
if ((val < 0) || (val & p_rt->ch_mask)) {
ret = (val < 0) ? val : -ETIMEDOUT;
dev_err(&s_rt->slave->dev,
--
2.30.2
It is rather inefficient to be constantly enabling/disabling the PM
runtime as we print out each individual register, switch to holding a PM
runtime reference across the whole register output.
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/soundwire/debugfs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 49900cd207bc7..0718e9cda138a 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -4,6 +4,7 @@
#include <linux/device.h>
#include <linux/debugfs.h>
#include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_registers.h>
@@ -35,7 +36,7 @@ static ssize_t sdw_sprintf(struct sdw_slave *slave,
{
int value;
- value = sdw_read(slave, reg);
+ value = sdw_read_no_pm(slave, reg);
if (value < 0)
return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
@@ -55,6 +56,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
if (!buf)
return -ENOMEM;
+ ret = pm_runtime_resume_and_get(&slave->dev);
+ if (ret < 0 && ret != -EACCES)
+ return ret;
+
ret = scnprintf(buf, RD_BUF, "Register Value\n");
/* DP0 non-banked registers */
@@ -112,6 +117,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
}
seq_printf(s_file, "%s", buf);
+
+ pm_runtime_mark_last_busy(&slave->dev);
+ pm_runtime_put(&slave->dev);
+
kfree(buf);
return 0;
--
2.30.2
From: Simon Trimmer <[email protected]>
The commit 167790abb90f ("soundwire: export sdw_write/read_no_pm
functions") exposed the single byte no_pm versions of the IO functions
that can be used without touching PM, export the multi byte no_pm
versions for the same reason.
Signed-off-by: Simon Trimmer <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/soundwire/bus.c | 8 ++++----
include/linux/soundwire/sdw.h | 2 ++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 76515c33e639e..ef4878258afad 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -414,8 +414,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
* all clients need to use the pm versions
*/
-static int
-sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
{
struct sdw_msg msg;
int ret;
@@ -430,9 +429,9 @@ sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
ret = 0;
return ret;
}
+EXPORT_SYMBOL(sdw_nread_no_pm);
-static int
-sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
+int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
{
struct sdw_msg msg;
int ret;
@@ -447,6 +446,7 @@ sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
ret = 0;
return ret;
}
+EXPORT_SYMBOL(sdw_nwrite_no_pm);
int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value)
{
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 9e4537f409c29..902ed46f76c80 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -1047,7 +1047,9 @@ int sdw_write(struct sdw_slave *slave, u32 addr, u8 value);
int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value);
int sdw_read_no_pm(struct sdw_slave *slave, u32 addr);
int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);
+int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);
int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val);
+int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val);
int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
--
2.30.2
On 11/14/22 04:29, Charles Keepax wrote:
> There is no need to play with the runtime reference everytime a register
> is accessed. All the remaining "pm" style register accesses trace back
> to 4 functions:
>
> sdw_prepare_stream
> sdw_deprepare_stream
> sdw_enable_stream
> sdw_disable_stream
>
> Any sensible implementation will need to hold a runtime reference
> across all those functions, it makes no sense to be allowing the
> device/bus to suspend whilst streams are being prepared/enabled. And
> certainly in the case of the all existing users, they all call these
> functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
> which will have already runtime resumed all the audio devices
> associated during the open callback.
>
> Signed-off-by: Charles Keepax <[email protected]>
I tend to agree with this one, and if this ever fails that would point
to a miss at a higher-level we'd need to address.
Reviewed-by: Pierre-Louis Bossart <[email protected]>
> ---
> drivers/soundwire/bus.c | 2 +-
> drivers/soundwire/stream.c | 30 +++++++++++++++---------------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index ef4878258afad..d87a188fcce1e 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1214,7 +1214,7 @@ int sdw_configure_dpn_intr(struct sdw_slave *slave,
> val &= ~SDW_DPN_INT_PORT_READY;
> }
>
> - ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
> + ret = sdw_update_no_pm(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
> if (ret < 0)
> dev_err(&slave->dev,
> "SDW_DPN_INTMASK write failed:%d\n", val);
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index bd502368339e5..df3b36670df4c 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -81,14 +81,14 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
> }
>
> /* Program DPN_OffsetCtrl2 registers */
> - ret = sdw_write(slave, addr1, t_params->offset2);
> + ret = sdw_write_no_pm(slave, addr1, t_params->offset2);
> if (ret < 0) {
> dev_err(bus->dev, "DPN_OffsetCtrl2 register write failed\n");
> return ret;
> }
>
> /* Program DPN_BlockCtrl3 register */
> - ret = sdw_write(slave, addr2, t_params->blk_pkg_mode);
> + ret = sdw_write_no_pm(slave, addr2, t_params->blk_pkg_mode);
> if (ret < 0) {
> dev_err(bus->dev, "DPN_BlockCtrl3 register write failed\n");
> return ret;
> @@ -105,7 +105,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
> /* Program DPN_SampleCtrl2 register */
> wbuf = FIELD_GET(SDW_DPN_SAMPLECTRL_HIGH, t_params->sample_interval - 1);
>
> - ret = sdw_write(slave, addr3, wbuf);
> + ret = sdw_write_no_pm(slave, addr3, wbuf);
> if (ret < 0) {
> dev_err(bus->dev, "DPN_SampleCtrl2 register write failed\n");
> return ret;
> @@ -115,7 +115,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
> wbuf = FIELD_PREP(SDW_DPN_HCTRL_HSTART, t_params->hstart);
> wbuf |= FIELD_PREP(SDW_DPN_HCTRL_HSTOP, t_params->hstop);
>
> - ret = sdw_write(slave, addr4, wbuf);
> + ret = sdw_write_no_pm(slave, addr4, wbuf);
> if (ret < 0)
> dev_err(bus->dev, "DPN_HCtrl register write failed\n");
>
> @@ -163,7 +163,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
> wbuf = FIELD_PREP(SDW_DPN_PORTCTRL_DATAMODE, p_params->data_mode);
> wbuf |= FIELD_PREP(SDW_DPN_PORTCTRL_FLOWMODE, p_params->flow_mode);
>
> - ret = sdw_update(s_rt->slave, addr1, 0xF, wbuf);
> + ret = sdw_update_no_pm(s_rt->slave, addr1, 0xF, wbuf);
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_PortCtrl register write failed for port %d\n",
> @@ -173,7 +173,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>
> if (!dpn_prop->read_only_wordlength) {
> /* Program DPN_BlockCtrl1 register */
> - ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
> + ret = sdw_write_no_pm(s_rt->slave, addr2, (p_params->bps - 1));
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_BlockCtrl1 register write failed for port %d\n",
> @@ -184,7 +184,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>
> /* Program DPN_SampleCtrl1 register */
> wbuf = (t_params->sample_interval - 1) & SDW_DPN_SAMPLECTRL_LOW;
> - ret = sdw_write(s_rt->slave, addr3, wbuf);
> + ret = sdw_write_no_pm(s_rt->slave, addr3, wbuf);
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_SampleCtrl1 register write failed for port %d\n",
> @@ -193,7 +193,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
> }
>
> /* Program DPN_OffsetCtrl1 registers */
> - ret = sdw_write(s_rt->slave, addr4, t_params->offset1);
> + ret = sdw_write_no_pm(s_rt->slave, addr4, t_params->offset1);
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_OffsetCtrl1 register write failed for port %d\n",
> @@ -203,7 +203,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>
> /* Program DPN_BlockCtrl2 register*/
> if (t_params->blk_grp_ctrl_valid) {
> - ret = sdw_write(s_rt->slave, addr5, t_params->blk_grp_ctrl);
> + ret = sdw_write_no_pm(s_rt->slave, addr5, t_params->blk_grp_ctrl);
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_BlockCtrl2 reg write failed for port %d\n",
> @@ -214,7 +214,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>
> /* program DPN_LaneCtrl register */
> if (slave_prop->lane_control_support) {
> - ret = sdw_write(s_rt->slave, addr6, t_params->lane_ctrl);
> + ret = sdw_write_no_pm(s_rt->slave, addr6, t_params->lane_ctrl);
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> "DPN_LaneCtrl register write failed for port %d\n",
> @@ -319,9 +319,9 @@ static int sdw_enable_disable_slave_ports(struct sdw_bus *bus,
> * it is safe to reset this register
> */
> if (en)
> - ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
> + ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
> else
> - ret = sdw_write(s_rt->slave, addr, 0x0);
> + ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
>
> if (ret < 0)
> dev_err(&s_rt->slave->dev,
> @@ -476,9 +476,9 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
> addr = SDW_DPN_PREPARECTRL(p_rt->num);
>
> if (prep)
> - ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
> + ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
> else
> - ret = sdw_write(s_rt->slave, addr, 0x0);
> + ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
>
> if (ret < 0) {
> dev_err(&s_rt->slave->dev,
> @@ -491,7 +491,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
> wait_for_completion_timeout(port_ready,
> msecs_to_jiffies(dpn_prop->ch_prep_timeout));
>
> - val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
> + val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
> if ((val < 0) || (val & p_rt->ch_mask)) {
> ret = (val < 0) ? val : -ETIMEDOUT;
> dev_err(&s_rt->slave->dev,
On 11/14/22 04:29, Charles Keepax wrote:
> It is rather inefficient to be constantly enabling/disabling the PM
> runtime as we print out each individual register, switch to holding a PM
> runtime reference across the whole register output.
the change is good, but technically the pm_runtime resume happens for
the first read and suspend with a delay if use_autosuspend() is enabled,
so presumably we'll see the same number of resume/suspend with the
existing code and the suggested change.
Maybe update the commit message to mention that we constantly change
reference counts, as you did in the next patch?
>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> drivers/soundwire/debugfs.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 49900cd207bc7..0718e9cda138a 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -4,6 +4,7 @@
> #include <linux/device.h>
> #include <linux/debugfs.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/pm_runtime.h>
> #include <linux/slab.h>
> #include <linux/soundwire/sdw.h>
> #include <linux/soundwire/sdw_registers.h>
> @@ -35,7 +36,7 @@ static ssize_t sdw_sprintf(struct sdw_slave *slave,
> {
> int value;
>
> - value = sdw_read(slave, reg);
> + value = sdw_read_no_pm(slave, reg);
>
> if (value < 0)
> return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
> @@ -55,6 +56,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
> if (!buf)
> return -ENOMEM;
>
> + ret = pm_runtime_resume_and_get(&slave->dev);
> + if (ret < 0 && ret != -EACCES)
> + return ret;
> +
> ret = scnprintf(buf, RD_BUF, "Register Value\n");
>
> /* DP0 non-banked registers */
> @@ -112,6 +117,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
> }
>
> seq_printf(s_file, "%s", buf);
> +
> + pm_runtime_mark_last_busy(&slave->dev);
> + pm_runtime_put(&slave->dev);
> +
> kfree(buf);
>
> return 0;
On 11/14/22 04:29, Charles Keepax wrote:
> Provide stub functions when CONFIG_SOUNDWIRE is not set for functions
> that are quite likely to be used from common code on devices supporting
> multiple control buses.
So far this case has been covered by splitting SoundWire related code
away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the
case for rt5682, max98373, etc.
Is this not good enough?
I am not against this patch, just wondering if allowing code for
different interfaces to be part of the same file will lead to confusions
with e.g. register offsets or functionality exposed with different
registers.
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> include/linux/soundwire/sdw.h | 92 +++++++++++++++++++++++++++++++----
> 1 file changed, 82 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 902ed46f76c80..4f80cba898f11 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -1021,15 +1021,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
> struct sdw_port_config *port_config,
> unsigned int num_ports,
> struct sdw_stream_runtime *stream);
> -int sdw_stream_add_slave(struct sdw_slave *slave,
> - struct sdw_stream_config *stream_config,
> - struct sdw_port_config *port_config,
> - unsigned int num_ports,
> - struct sdw_stream_runtime *stream);
> int sdw_stream_remove_master(struct sdw_bus *bus,
> struct sdw_stream_runtime *stream);
> -int sdw_stream_remove_slave(struct sdw_slave *slave,
> - struct sdw_stream_runtime *stream);
> int sdw_startup_stream(void *sdw_substream);
> int sdw_prepare_stream(struct sdw_stream_runtime *stream);
> int sdw_enable_stream(struct sdw_stream_runtime *stream);
> @@ -1040,8 +1033,20 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus);
> int sdw_bus_clk_stop(struct sdw_bus *bus);
> int sdw_bus_exit_clk_stop(struct sdw_bus *bus);
>
> -/* messaging and data APIs */
> +int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id);
> +void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);
> +
> +#if IS_ENABLED(CONFIG_SOUNDWIRE)
>
> +int sdw_stream_add_slave(struct sdw_slave *slave,
> + struct sdw_stream_config *stream_config,
> + struct sdw_port_config *port_config,
> + unsigned int num_ports,
> + struct sdw_stream_runtime *stream);
> +int sdw_stream_remove_slave(struct sdw_slave *slave,
> + struct sdw_stream_runtime *stream);
> +
> +/* messaging and data APIs */
> int sdw_read(struct sdw_slave *slave, u32 addr);
> int sdw_write(struct sdw_slave *slave, u32 addr, u8 value);
> int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value);
> @@ -1053,7 +1058,74 @@ int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *
> int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
> int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val);
>
> -int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id);
> -void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);
> +#else
> +
> +static inline int sdw_stream_add_slave(struct sdw_slave *slave,
> + struct sdw_stream_config *stream_config,
> + struct sdw_port_config *port_config,
> + unsigned int num_ports,
> + struct sdw_stream_runtime *stream)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_stream_remove_slave(struct sdw_slave *slave,
> + struct sdw_stream_runtime *stream)
> +{
> + return 0;
> +}
> +
> +/* messaging and data APIs */
> +static inline int sdw_read(struct sdw_slave *slave, u32 addr)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_write(struct sdw_slave *slave, u32 addr, u8 value)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
> +{
> + return 0;
> +}
> +
> +static inline int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
> +{
> + return 0;
> +}
> +
> +#endif /* CONFIG_SOUNDWIRE */
>
> #endif /* __SOUNDWIRE_H */
On Mon, Nov 14, 2022 at 10:14:19AM -0600, Pierre-Louis Bossart wrote:
>
>
> On 11/14/22 04:29, Charles Keepax wrote:
> > It is rather inefficient to be constantly enabling/disabling the PM
> > runtime as we print out each individual register, switch to holding a PM
> > runtime reference across the whole register output.
>
> the change is good, but technically the pm_runtime resume happens for
> the first read and suspend with a delay if use_autosuspend() is enabled,
> so presumably we'll see the same number of resume/suspend with the
> existing code and the suggested change.
>
> Maybe update the commit message to mention that we constantly change
> reference counts, as you did in the next patch?
Yeah agree, I will respin the commit message.
Thanks,
Charles
On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote:
>
>
> On 11/14/22 04:29, Charles Keepax wrote:
> > Provide stub functions when CONFIG_SOUNDWIRE is not set for functions
> > that are quite likely to be used from common code on devices supporting
> > multiple control buses.
>
> So far this case has been covered by splitting SoundWire related code
> away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the
> case for rt5682, max98373, etc.
>
> Is this not good enough?
>
> I am not against this patch, just wondering if allowing code for
> different interfaces to be part of the same file will lead to confusions
> with e.g. register offsets or functionality exposed with different
> registers.
>
I guess this is a bit of a grey area this one. Both work, I guess
the reason I was leaning this way is that in order to avoid a
circular dependency if I put all the soundwire DAI handling into
the soundwire code then I have to duplicate the snd_soc_dai_driver
structure into both the sdw and i2c specific code (worth noting
the I2S DAIs are still usable when the part is sdw to the host). But
there are also downsides to this approach in that it will likely have
some small impact on driver size when soundwire is not built in.
Thanks,
Charles
On Mon, Nov 14, 2022 at 10:04:55AM -0600, Pierre-Louis Bossart wrote:
>
>
> On 11/14/22 04:29, Charles Keepax wrote:
> > There is no need to play with the runtime reference everytime a register
> > is accessed. All the remaining "pm" style register accesses trace back
> > to 4 functions:
> >
> > sdw_prepare_stream
> > sdw_deprepare_stream
> > sdw_enable_stream
> > sdw_disable_stream
> >
> > Any sensible implementation will need to hold a runtime reference
> > across all those functions, it makes no sense to be allowing the
> > device/bus to suspend whilst streams are being prepared/enabled. And
> > certainly in the case of the all existing users, they all call these
> > functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
> > which will have already runtime resumed all the audio devices
> > associated during the open callback.
> >
> > Signed-off-by: Charles Keepax <[email protected]>
>
> I tend to agree with this one, and if this ever fails that would point
> to a miss at a higher-level we'd need to address.
Exactly my concern here is the core is trying to be helpful, but
really it is just going to be hiding bugs in the callers.
Thanks,
Charles
On 15/11/2022 11:03, Charles Keepax wrote:
> On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote:
>>
>>
>> On 11/14/22 04:29, Charles Keepax wrote:
>>> Provide stub functions when CONFIG_SOUNDWIRE is not set for functions
>>> that are quite likely to be used from common code on devices supporting
>>> multiple control buses.
>>
>> So far this case has been covered by splitting SoundWire related code
>> away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the
>> case for rt5682, max98373, etc.
>>
>> Is this not good enough?
>>
>> I am not against this patch, just wondering if allowing code for
>> different interfaces to be part of the same file will lead to confusions
>> with e.g. register offsets or functionality exposed with different
>> registers.
>>
>
> I guess this is a bit of a grey area this one. Both work, I guess
> the reason I was leaning this way is that in order to avoid a
> circular dependency if I put all the soundwire DAI handling into
> the soundwire code then I have to duplicate the snd_soc_dai_driver
> structure into both the sdw and i2c specific code (worth noting
> the I2S DAIs are still usable when the part is sdw to the host). But
> there are also downsides to this approach in that it will likely have
> some small impact on driver size when soundwire is not built in.
>
I think we should just add the stubs. Other subsystems use stubs to help
with code that references stuff that might not be available.
Splitting all the soundwire-specifics out into a separate module works
for simple chips that are either I2S or soundwire. but can get messy for
a complex codec. I used the separate file method for CS42L42, but for a
driver I'm working on I abandoned that and put both DAIs in the core
code. I didn't notice the missing stubs because my defconfig that was
intended to omit soundwire apparently has something that is selecting
it anyway.
On 11/15/22 05:41, Richard Fitzgerald wrote:
> On 15/11/2022 11:03, Charles Keepax wrote:
>> On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 11/14/22 04:29, Charles Keepax wrote:
>>>> Provide stub functions when CONFIG_SOUNDWIRE is not set for functions
>>>> that are quite likely to be used from common code on devices supporting
>>>> multiple control buses.
>>>
>>> So far this case has been covered by splitting SoundWire related code
>>> away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the
>>> case for rt5682, max98373, etc.
>>>
>>> Is this not good enough?
>>>
>>> I am not against this patch, just wondering if allowing code for
>>> different interfaces to be part of the same file will lead to confusions
>>> with e.g. register offsets or functionality exposed with different
>>> registers.
>>>
>>
>> I guess this is a bit of a grey area this one. Both work, I guess
>> the reason I was leaning this way is that in order to avoid a
>> circular dependency if I put all the soundwire DAI handling into
>> the soundwire code then I have to duplicate the snd_soc_dai_driver
>> structure into both the sdw and i2c specific code (worth noting
>> the I2S DAIs are still usable when the part is sdw to the host). But
>> there are also downsides to this approach in that it will likely have
>> some small impact on driver size when soundwire is not built in.
>>
>
> I think we should just add the stubs. Other subsystems use stubs to help
> with code that references stuff that might not be available.
>
> Splitting all the soundwire-specifics out into a separate module works
> for simple chips that are either I2S or soundwire. but can get messy for
> a complex codec. I used the separate file method for CS42L42, but for a
> driver I'm working on I abandoned that and put both DAIs in the core
> code. I didn't notice the missing stubs because my defconfig that was
> intended to omit soundwire apparently has something that is selecting
> it anyway.
It would be good if you could look into this, I don't see any 'select
SOUNDWIRE'.
I agree the premise of the split was that the device is used in one mode
of the other, I am not sure however what the a 'complex codec' would
change. It's likely that we will see a second level within a SoundWire
device to deal with independent 'functions', but I don't quite see how
this would matter.
That said, I don't write codec drivers so I am not going to lay on the
tracks over 2 stubs. We can revisit the sdw.h split as well later.
Reviewed-by: Pierre-Louis Bossart <[email protected]>
Hi Charles,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/Minor-SoundWire-clean-ups/20221114-183333
patch link: https://lore.kernel.org/r/20221114102956.914468-4-ckeepax%40opensource.cirrus.com
patch subject: [PATCH 3/4] soundwire: debugfs: Switch to sdw_read_no_pm
config: loongarch-randconfig-m041-20221114
compiler: loongarch64-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/soundwire/debugfs.c:61 sdw_slave_reg_show() warn: possible memory leak of 'buf'
vim +/buf +61 drivers/soundwire/debugfs.c
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 48 static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 49 {
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 50 struct sdw_slave *slave = s_file->private;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 51 char *buf;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 52 ssize_t ret;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 53 int i, j;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 54
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 55 buf = kzalloc(RD_BUF, GFP_KERNEL);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 56 if (!buf)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 57 return -ENOMEM;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 58
f3345eda607ecc Charles Keepax 2022-11-14 59 ret = pm_runtime_resume_and_get(&slave->dev);
f3345eda607ecc Charles Keepax 2022-11-14 60 if (ret < 0 && ret != -EACCES)
f3345eda607ecc Charles Keepax 2022-11-14 @61 return ret;
kfree(buf);
f3345eda607ecc Charles Keepax 2022-11-14 62
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 63 ret = scnprintf(buf, RD_BUF, "Register Value\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 64
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 65 /* DP0 non-banked registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 66 ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 67 for (i = SDW_DP0_INT; i <= SDW_DP0_PREPARECTRL; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 68 ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 69
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 70 /* DP0 Bank 0 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 71 ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 72 ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 73 for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 74 ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 75
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 76 /* DP0 Bank 1 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 77 ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 78 ret += sdw_sprintf(slave, buf, ret,
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 79 SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 80 for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 81 i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 82 ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 83
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 84 /* SCP registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 85 ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 86 for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 87 ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 88 for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 89 ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 90
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 91 /*
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 92 * SCP Bank 0/1 registers are read-only and cannot be
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 93 * retrieved from the Slave. The Master typically keeps track
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 94 * of the current frame size so the information can be found
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 95 * in other places
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 96 */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 97
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 98 /* DP1..14 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 99 for (i = 1; SDW_VALID_PORT_RANGE(i); i++) {
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 100
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 101 /* DPi registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 102 ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 103 for (j = SDW_DPN_INT(i); j <= SDW_DPN_PREPARECTRL(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 104 ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 105
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 106 /* DPi Bank0 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 107 ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 108 for (j = SDW_DPN_CHANNELEN_B0(i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 109 j <= SDW_DPN_LANECTRL_B0(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 110 ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 111
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 112 /* DPi Bank1 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 113 ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 114 for (j = SDW_DPN_CHANNELEN_B1(i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 115 j <= SDW_DPN_LANECTRL_B1(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 116 ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 117 }
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 118
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 119 seq_printf(s_file, "%s", buf);
f3345eda607ecc Charles Keepax 2022-11-14 120
f3345eda607ecc Charles Keepax 2022-11-14 121 pm_runtime_mark_last_busy(&slave->dev);
f3345eda607ecc Charles Keepax 2022-11-14 122 pm_runtime_put(&slave->dev);
f3345eda607ecc Charles Keepax 2022-11-14 123
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 124 kfree(buf);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 125
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 126 return 0;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 127 }
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21 128 DEFINE_SHOW_ATTRIBUTE(sdw_slave_reg);
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Mon, Nov 21, 2022 at 01:50:55PM +0300, Dan Carpenter wrote:
> Hi Charles,
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/Minor-SoundWire-clean-ups/20221114-183333
> patch link: https://lore.kernel.org/r/20221114102956.914468-4-ckeepax%40opensource.cirrus.com
> patch subject: [PATCH 3/4] soundwire: debugfs: Switch to sdw_read_no_pm
> config: loongarch-randconfig-m041-20221114
> compiler: loongarch64-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
>
Thanks for the spot I will fix this up and do a v3.
Thanks,
Charles