| Message ID | 87zigenerc.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello! On 3/22/2017 7:02 AM, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > R-Car Datasheet is indicating "SSICR.CKDV = 000 is invalid when > SSIWSR.WS_MODE = 1 or SSIWSR.CONT = 1". > Current driver will set CONT, thus, we shouldn't use CKDV = 000. > This patch fixup it. > > Reported-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Tested-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com> > --- > sound/soc/sh/rcar/ssi.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c > index 9594d44..d0602c1 100644 > --- a/sound/soc/sh/rcar/ssi.c > +++ b/sound/soc/sh/rcar/ssi.c > @@ -245,6 +245,15 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, > for (j = 0; j < ARRAY_SIZE(ssi_clk_mul_table); j++) { > > /* > + * It will set SSIWSR.CONT here, but SSICR.CKDV = 000 > + * with it is not allowed. (SSIWSR.WS_MODE with > + * SSICR.CKDV = 000 is not allowed either). > + * Skip it. See SSICR.CKDV > + */ > + if (j == 0) > + continue; Why not change the *for* statement itself to start with j = 1? [...] MBR, Sergei
Hi Sergei > > /* > > + * It will set SSIWSR.CONT here, but SSICR.CKDV = 000 > > + * with it is not allowed. (SSIWSR.WS_MODE with > > + * SSICR.CKDV = 000 is not allowed either). > > + * Skip it. See SSICR.CKDV > > + */ > > + if (j == 0) > > + continue; > > Why not change the *for* statement itself to start with j = 1? It can be one solution. Actually my local 1st patch was such style. But I thought that it is difficult to notice such magical operation from code. Thus, I used this style Best regards --- Kuninori Morimoto
On Wed, Mar 22, 2017 at 08:56:07AM +0000, Kuninori Morimoto wrote: > > Hi Sergei > > > > /* > > > + * It will set SSIWSR.CONT here, but SSICR.CKDV = 000 > > > + * with it is not allowed. (SSIWSR.WS_MODE with > > > + * SSICR.CKDV = 000 is not allowed either). > > > + * Skip it. See SSICR.CKDV > > > + */ > > > + if (j == 0) > > > + continue; > > > > Why not change the *for* statement itself to start with j = 1? > > It can be one solution. Actually my local 1st patch was such style. > But I thought that it is difficult to notice such magical operation from code. > Thus, I used this style FWIIW, I think if you have a comment, like the one above, then j = 1 should be obvious enough.
Hi Simon > > > > /* > > > > + * It will set SSIWSR.CONT here, but SSICR.CKDV = 000 > > > > + * with it is not allowed. (SSIWSR.WS_MODE with > > > > + * SSICR.CKDV = 000 is not allowed either). > > > > + * Skip it. See SSICR.CKDV > > > > + */ > > > > + if (j == 0) > > > > + continue; > > > > > > Why not change the *for* statement itself to start with j = 1? > > > > It can be one solution. Actually my local 1st patch was such style. > > But I thought that it is difficult to notice such magical operation from code. > > Thus, I used this style > > FWIIW, I think if you have a comment, like the one above, then j = 1 should > be obvious enough. I'm OK for both, and have no special opinion about this. Let's follow to ALSA SoC maintainer's opinion Best regards --- Kuninori Morimoto
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 9594d44..d0602c1 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -245,6 +245,15 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, for (j = 0; j < ARRAY_SIZE(ssi_clk_mul_table); j++) { /* + * It will set SSIWSR.CONT here, but SSICR.CKDV = 000 + * with it is not allowed. (SSIWSR.WS_MODE with + * SSICR.CKDV = 000 is not allowed either). + * Skip it. See SSICR.CKDV + */ + if (j == 0) + continue; + + /* * this driver is assuming that * system word is 32bit x chan * see rsnd_ssi_init()