Re: [PATCH V3 09/15] ASoC: samsung: i2s: Protect more registers with a spinlock
Hi Tuashar,
On 17/01/15 06:21, Tushar Behera wrote:
> On Thu, Jan 15, 2015 at 3:42 AM, Sylwester Nawrocki
> wrote:
>> Ensure the I2SMOD, I2SPSR registers, which are also exposed through
>> clk API are only accessed with the i2s->spinlock spinlock held.
>>
>> Signed-off-by: Sylwester Nawrocki
>> ---
>> sound/soc/samsung/i2s.c | 81
>> +--
>> 1 file changed, 51 insertions(+), 30 deletions(-)
>>
>> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
>> index 20cc51f..05fc2f0 100644
>> --- a/sound/soc/samsung/i2s.c
>> +++ b/sound/soc/samsung/i2s.c
>> @@ -472,17 +472,22 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
>> {
>> struct i2s_dai *i2s = to_info(dai);
>> struct i2s_dai *other = get_other_dai(i2s);
>> - u32 mod = readl(i2s->addr + I2SMOD);
>> const struct samsung_i2s_variant_regs *i2s_regs = i2s->variant_regs;
>> unsigned int cdcon_mask = 1 << i2s_regs->cdclkcon_off;
>> unsigned int rsrc_mask = 1 << i2s_regs->rclksrc_off;
>> + u32 mod, mask, val = 0;
>> +
>> + spin_lock(i2s->lock);
>> + mod = readl(i2s->addr + I2SMOD);
>> + spin_unlock(i2s->lock);
>>
>
> 'mod' is now updated only at the bottom of this function. The above
> readl can be omitted.
mod is used in some of the 'if' statements below, so we must read it
here beforehand.
>> switch (clk_id) {
>> case SAMSUNG_I2S_OPCLK:
>> - mod &= ~MOD_OPCLK_MASK;
>> - mod |= dir;
>> + mask = MOD_OPCLK_MASK;
>> + val = dir;
>> break;
>> case SAMSUNG_I2S_CDCLK:
>> + mask = 1 << i2s_regs->cdclkcon_off;
>
> Use BIT() macro instead?
Yes, it would be a good code cleanup, might be worth to include it in
some future patch series. I'll keep it in mind, since this patch is merged
already.
And the logical bit operations is one of things people make mistakes most
often, so any changes like these would need to be well tested and/or
carefully reviewed.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 09/15] ASoC: samsung: i2s: Protect more registers with a spinlock
On Thu, Jan 15, 2015 at 3:42 AM, Sylwester Nawrocki
wrote:
> Ensure the I2SMOD, I2SPSR registers, which are also exposed through
> clk API are only accessed with the i2s->spinlock spinlock held.
>
> Signed-off-by: Sylwester Nawrocki
> ---
> sound/soc/samsung/i2s.c | 81
> +--
> 1 file changed, 51 insertions(+), 30 deletions(-)
>
> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 20cc51f..05fc2f0 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -472,17 +472,22 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
> {
> struct i2s_dai *i2s = to_info(dai);
> struct i2s_dai *other = get_other_dai(i2s);
> - u32 mod = readl(i2s->addr + I2SMOD);
> const struct samsung_i2s_variant_regs *i2s_regs = i2s->variant_regs;
> unsigned int cdcon_mask = 1 << i2s_regs->cdclkcon_off;
> unsigned int rsrc_mask = 1 << i2s_regs->rclksrc_off;
> + u32 mod, mask, val = 0;
> +
> + spin_lock(i2s->lock);
> + mod = readl(i2s->addr + I2SMOD);
> + spin_unlock(i2s->lock);
>
'mod' is now updated only at the bottom of this function. The above
readl can be omitted.
> switch (clk_id) {
> case SAMSUNG_I2S_OPCLK:
> - mod &= ~MOD_OPCLK_MASK;
> - mod |= dir;
> + mask = MOD_OPCLK_MASK;
> + val = dir;
> break;
> case SAMSUNG_I2S_CDCLK:
> + mask = 1 << i2s_regs->cdclkcon_off;
Use BIT() macro instead?
> /* Shouldn't matter in GATING(CLOCK_IN) mode */
> if (dir == SND_SOC_CLOCK_IN)
> rfs = 0;
> @@ -499,15 +504,15 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
> }
>
> if (dir == SND_SOC_CLOCK_IN)
> - mod |= 1 << i2s_regs->cdclkcon_off;
> - else
> - mod &= ~(1 << i2s_regs->cdclkcon_off);
> + val = 1 << i2s_regs->cdclkcon_off;
>
Same as above.
> i2s->rfs = rfs;
> break;
>
> case SAMSUNG_I2S_RCLKSRC_0: /* clock corrsponding to IISMOD[10] := 0
> */
> case SAMSUNG_I2S_RCLKSRC_1: /* clock corrsponding to IISMOD[10] := 1
> */
> + mask = 1 << i2s_regs->rclksrc_off;
> +
Same as above.
> if ((i2s->quirks & QUIRK_NO_MUXPSR)
> || (clk_id == SAMSUNG_I2S_RCLKSRC_0))
> clk_id = 0;
> @@ -557,18 +562,19 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
> return 0;
> }
>
> - if (clk_id == 0)
> - mod &= ~(1 << i2s_regs->rclksrc_off);
> - else
> - mod |= 1 << i2s_regs->rclksrc_off;
> -
> + if (clk_id == 1)
> + val = 1 << i2s_regs->rclksrc_off;
Same as above.
--
Tushar Behera
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 09/15] ASoC: samsung: i2s: Protect more registers with a spinlock
Ensure the I2SMOD, I2SPSR registers, which are also exposed through
clk API are only accessed with the i2s->spinlock spinlock held.
Signed-off-by: Sylwester Nawrocki
---
sound/soc/samsung/i2s.c | 81 +--
1 file changed, 51 insertions(+), 30 deletions(-)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 20cc51f..05fc2f0 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -472,17 +472,22 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
{
struct i2s_dai *i2s = to_info(dai);
struct i2s_dai *other = get_other_dai(i2s);
- u32 mod = readl(i2s->addr + I2SMOD);
const struct samsung_i2s_variant_regs *i2s_regs = i2s->variant_regs;
unsigned int cdcon_mask = 1 << i2s_regs->cdclkcon_off;
unsigned int rsrc_mask = 1 << i2s_regs->rclksrc_off;
+ u32 mod, mask, val = 0;
+
+ spin_lock(i2s->lock);
+ mod = readl(i2s->addr + I2SMOD);
+ spin_unlock(i2s->lock);
switch (clk_id) {
case SAMSUNG_I2S_OPCLK:
- mod &= ~MOD_OPCLK_MASK;
- mod |= dir;
+ mask = MOD_OPCLK_MASK;
+ val = dir;
break;
case SAMSUNG_I2S_CDCLK:
+ mask = 1 << i2s_regs->cdclkcon_off;
/* Shouldn't matter in GATING(CLOCK_IN) mode */
if (dir == SND_SOC_CLOCK_IN)
rfs = 0;
@@ -499,15 +504,15 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
}
if (dir == SND_SOC_CLOCK_IN)
- mod |= 1 << i2s_regs->cdclkcon_off;
- else
- mod &= ~(1 << i2s_regs->cdclkcon_off);
+ val = 1 << i2s_regs->cdclkcon_off;
i2s->rfs = rfs;
break;
case SAMSUNG_I2S_RCLKSRC_0: /* clock corrsponding to IISMOD[10] := 0 */
case SAMSUNG_I2S_RCLKSRC_1: /* clock corrsponding to IISMOD[10] := 1 */
+ mask = 1 << i2s_regs->rclksrc_off;
+
if ((i2s->quirks & QUIRK_NO_MUXPSR)
|| (clk_id == SAMSUNG_I2S_RCLKSRC_0))
clk_id = 0;
@@ -557,18 +562,19 @@ static int i2s_set_sysclk(struct snd_soc_dai *dai,
return 0;
}
- if (clk_id == 0)
- mod &= ~(1 << i2s_regs->rclksrc_off);
- else
- mod |= 1 << i2s_regs->rclksrc_off;
-
+ if (clk_id == 1)
+ val = 1 << i2s_regs->rclksrc_off;
break;
default:
dev_err(&i2s->pdev->dev, "We don't serve that!\n");
return -EINVAL;
}
+ spin_lock(i2s->lock);
+ mod = readl(i2s->addr + I2SMOD);
+ mod = (mod & ~mask) | val;
writel(mod, i2s->addr + I2SMOD);
+ spin_unlock(i2s->lock);
return 0;
}
@@ -577,9 +583,8 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
unsigned int fmt)
{
struct i2s_dai *i2s = to_info(dai);
- u32 mod = readl(i2s->addr + I2SMOD);
int lrp_shift, sdf_shift, sdf_mask, lrp_rlow, mod_slave;
- u32 tmp = 0;
+ u32 mod, tmp = 0;
lrp_shift = i2s->variant_regs->lrp_off;
sdf_shift = i2s->variant_regs->sdf_off;
@@ -639,12 +644,15 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
return -EINVAL;
}
+ spin_lock(i2s->lock);
+ mod = readl(i2s->addr + I2SMOD);
/*
* Don't change the I2S mode if any controller is active on this
* channel.
*/
if (any_active(i2s) &&
((mod & (sdf_mask | lrp_rlow | mod_slave)) != tmp)) {
+ spin_unlock(i2s->lock);
dev_err(&i2s->pdev->dev,
"%s:%d Other DAI busy\n", __func__, __LINE__);
return -EAGAIN;
@@ -653,6 +661,7 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
mod &= ~(sdf_mask | lrp_rlow | mod_slave);
mod |= tmp;
writel(mod, i2s->addr + I2SMOD);
+ spin_unlock(i2s->lock);
return 0;
}
@@ -661,16 +670,16 @@ static int i2s_hw_params(struct snd_pcm_substream
*substream,
struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
{
struct i2s_dai *i2s = to_info(dai);
- u32 mod = readl(i2s->addr + I2SMOD);
+ u32 mod, mask = 0, val = 0;
if (!is_secondary(i2s))
- mod &= ~(MOD_DC2_EN | MOD_DC1_EN);
+ mask |= (MOD_DC2_EN | MOD_DC1_EN);
switch (params_channels(params)) {
case 6:
- mod |= MOD_DC2_EN;
+ val |= MOD_DC2_EN;
case 4:
- mod |= MOD_DC1_EN;
+ val |= MOD_DC1_EN;
break;
case 2:
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
@@ -692,44 +701,49 @@ static int i2s_hw_params(struct snd_p
