commit cfb7b8bf1e ("ASoC: rsnd: tidyup
rsnd_ssiu_busif_err_status_clear()") merged duplicate code, but it didn't
care about default case, and causes smatch warnings.
smatch warnings:
sound/soc/sh/rcar/ssiu.c:112 rsnd_ssiu_busif_err_status_clear() \
error: uninitialized symbol 'offset'.
sound/soc/sh/rcar/ssiu.c:114 rsnd_ssiu_busif_err_status_clear() \
error: uninitialized symbol 'shift'.
This patch cares it.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87r15rgn6p.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
A copy of substream pointer is stored in priv structure during
rz_ssi_dai_trigger() callback ie in SNDRV_PCM_TRIGGER_START case
and the pointer is assigned to NULL in case of SNDRV_PCM_TRIGGER_STOP.
The driver used the locks only in rz_ssi_stream_is_valid() and assigned
the local substream pointer to NULL in rz_ssi_dai_trigger() callback but
never locked it while making a local copy.
This patch adds the rz_ssi_set_substream() helper function to set the
substream pointer with locks acquired and replaces the instances of
setting the local substream pointer with the rz_ssi_set_substream()
function.
Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Link: https://lore.kernel.org/r/20220125132457.14984-3-prabhakar.mahadev-lad.rj@bp.renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This driver is assuming that all adg->clk[i] is not NULL.
Because of this prerequisites, for_each_rsnd_clk() is possible to work
for all clk without checking NULL. In other words, all adg->clk[i]
should not NULL.
Some SoC might doesn't have clk_a/b/c/i. devm_clk_get() returns error in
such case. This driver calls rsnd_adg_null_clk_get() and use null_clk
instead of NULL in such cases.
But devm_clk_get() might returns NULL even though such clocks exist, but
it doesn't mean error (user deliberately chose to disable the feature).
NULL clk itself is not error from clk point of view, but is error from
this driver point of view because it is not assuming such case.
But current code is using IS_ERR() which doesn't care NULL.
This driver uses IS_ERR_OR_NULL() instead of IS_ERR() for clk check.
And it uses ERR_CAST() to clarify null_clk error.
One concern here is that it unconditionally uses null_clk if clk_a/b/c/i
was error. It is correct if it doesn't exist, but is not correct if it
returns error even though it exist.
It needs to check "clock-names" from DT before calling devm_clk_get() to
handling such case. But let's assume it is overkill so far.
Link: https://lore.kernel.org/r/YMCmhfQUimHCSH/n@mwanda
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87v940wyf9.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Don't populate arrays on the stack but instead them static const.
Makes the object code smaller by 48 bytes.
Before:
text data bss dec hex filename
20938 916 104 21958 55c6 ./sound/soc/sh/rcar/core.o
After:
text data bss dec hex filename
20890 916 104 21910 5596 ./sound/soc/sh/rcar/core.o
gcc version 11.1.0)
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87tujkwydx.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Don't populate arrays on the stack but instead them static const.
Makes the object code smaller by 242 bytes.
Before:
text data bss dec hex filename
23827 8764 0 32591 7f4f ./sound/soc/sh/rcar/ssi.o
After:
text data bss dec hex filename
23361 8988 0 32349 7e5d ./sound/soc/sh/rcar/ssi.o
gcc version 10.2.0)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/r/20210801063237.137998-1-colin.king@canonical.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Hi Mark
I noticed that adg can be more clean code.
And rsnd.h header comment was not so good
because patch has been randomly added.
This patch tidyup these.
Kuninori Morimoto (5):
ASoC: rsnd: adg: supply __printf(x, y) formatting for dbg_msg()
ASoC: rsnd: adg: tidyup rsnd_adg_get_clkin/out() parameter
ASoC: rsnd: adg: use more simple method for null_clk
ASoC: rsnd: adg: check return value for rsnd_adg_get_clkin/out()
ASoC: rsnd: tidyup __rsnd_mod_xxx macro comments
sound/soc/sh/rcar/adg.c | 139 +++++++++++++++++++++++++--------------
sound/soc/sh/rcar/rsnd.h | 21 ++----
2 files changed, 97 insertions(+), 63 deletions(-)
--
2.25.1
Current rsnd_adg_get_clkin/out() are void function,
thus adg->clk/clkout[i] might be NULL.
But, for_each_rsnd_clk/clkout() macros are assuming
all clks are non NULL.
Because of this mismatch, code can be complex and/or buggy.
These functions return error by this patch,
and make sure all clks are non NULL.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87pmx5i20m.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
commit 965386c976 ("ASoC: rsnd: call unregister for null_hw when
removed") tried unregister null_clk, but it has some issues.
1st issue is kernel will indicate below message when unregistering,
because of its timing. unregistering should be happen after clk_disable().
clk_unregister: unregistering prepared clock: rsnd_adg_null
2nd issue is, it is using priv->null_clk, but it should be adg->null_clk.
3rd issue is it is using very complex clk registering method.
more simple clk_register/unregister_fixed_rate() should be OK.
This patch fixes these.
Fixes: 965386c976 ("ASoC: rsnd: call unregister for null_hw when removed")
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87r1hli215.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In general Renesas SoC's SSI/SRC are all enabled, but some SoC is not.
H2 E2
SRC0 <=
SRC1 SRC1
SRC2 SRC2
... ...
Renesas Sound driver is assuming that *all* modules are
enabled, and thus it is using *data array* to access each modules.
Because of it, we have been using "status = disabled" at DT,
and using *full size* array but avoiding disabled module.
ex)
rcar_sound,src {
src-0 {
=> status = "disabled";
};
src1: src-1 {
...
};
...
But R-Car D3 have many disabled modules (It has SSI3/SSI4, SRC5/SRC6),
and Renesas SoC maintainer don't want above style on DT.
ex)
rcar_sound,src {
=> src0: src-0 { status = "disabled"; };
=> src1: src-1 { status = "disabled"; };
=> src2: src-2 { status = "disabled"; };
=> src3: src-3 { status = "disabled"; };
=> src4: src-4 { status = "disabled"; };
src5: src-5 {
...
};
src6: src-6 {
...
};
};
rcar_sound,ssi {
=> ssi0: ssi-0 { status = "disabled"; };
=> ssi1: ssi-1 { status = "disabled"; };
=> ssi2: ssi-2 { status = "disabled"; };
ssi3: ssi-3 {
...
};
ssi4: ssi-4 {
...
};
};
To adjust it, it needs to care about related for_each_child_of_node()
loop on rsnd driver, and it is used from...
> grep -l for_each_child_of_node sound/soc/sh/rcar/*
sound/soc/sh/rcar/core.c
sound/soc/sh/rcar/ctu.c
sound/soc/sh/rcar/dma.c
sound/soc/sh/rcar/dvc.c
sound/soc/sh/rcar/mix.c
sound/soc/sh/rcar/src.c
sound/soc/sh/rcar/ssi.c
sound/soc/sh/rcar/ssiu.c
This patch adjust to this situation.
By this patch, we can avoid disabled modules on DT
rcar_sound,src {
src5: src-5 {
...
};
src6: src-6 {
...
};
};
rcar_sound,ssi {
ssi3: ssi-3 {
...
};
ssi4: ssi-4 {
...
};
};
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/875yyzk017.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Renesas Sound uses many modules (SSI/SSIU/SRC/CTU/MIX/DVC/DMA),
and supports complex connections/path.
Thus each modules needs to save its status to correctly control it.
This status is updated when by .trigger, and .hw_params/.hw_free.
Renesas Sound is protecting modules by using lock when .trigger,
but it was not enough to protecting each modules "status" if it was
used from many paths.
1) .hw_params/.hw_free update status
2) another doesn't update status, but overwrites by same value
This patch do
1) protects .hw_params/.hw_free by lock
2) do nothing if no status update
Without this patch, protected mod->status (= .trigger) might be
overwrote by non protected mod->status (= .hw_params / .hw_free),
and in such case, CTU/MIX/DVC/SSIU/SSI which are used from
many paths might get damage.
If above issue happens, Renesas Sound will be hung (= silence)
and never be recoverd.
I could reproduce this issue by continue playing very short sound
with loop very long term (3-4 hours) through 2 inputs (= MIXer).
For updating rsnd_status_update(), this patch removes rsnd_dai_call()
debug message. Because we already have debugfs support, and is not
good match to new code.
Reported-by: Linh Phung T. Y <linh.phung.jy@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Message-Id: <87wnrklwyh.wl-kuninori.morimoto.gx@renesas.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
