diff mbox series

[4.19.y-cip,19/57] ASoC: rsnd: use 32bit TDM width as default

Message ID 1571295929-47286-20-git-send-email-biju.das@bp.renesas.com (mailing list archive)
State Changes Requested
Headers show
Series Audio improvements/SSIU BUSIF/ | expand

Commit Message

Biju Das Oct. 17, 2019, 7:04 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

commit 82ab7e9a4d3fcec27f745be04063e17da1881dda upstream.

commit fb2815f44a9e ("ASoC: rsnd: add support for 16/24 bit slot widths")
added TDM width check, and return error if it was not 16/24/32 bit.
But it is too strict. This patch uses 32bit same as default.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 sound/soc/sh/rcar/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Pavel Machek Oct. 20, 2019, 9:49 a.m. UTC | #1 
On Thu 2019-10-17 08:04:51, Biju Das wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> commit 82ab7e9a4d3fcec27f745be04063e17da1881dda upstream.
> 
> commit fb2815f44a9e ("ASoC: rsnd: add support for 16/24 bit slot widths")
> added TDM width check, and return error if it was not 16/24/32 bit.
> But it is too strict. This patch uses 32bit same as default.

> @@ -738,8 +738,8 @@ static int rsnd_soc_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  	case 32:
>  		break;
>  	default:
> -		dev_err(dev, "unsupported slot width value: %d\n", slot_width);
> -		return -EINVAL;
> +		/* use default */
> +		slot_width = 32;
>  	}
>  
>  	switch (slots) {

I don't think I like this. People should not be passing strange values
to this function. Can the caller be fixed, instead?

Thanks,
								Pavel
Biju Das Oct. 21, 2019, 1:54 p.m. UTC | #2 
+ Morimoto-San,

Hi Pavel,

Thanks for the feedback. 

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: Sunday, October 20, 2019 10:49 AM
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek <pavel@denx.de>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>
> Subject: Re: [PATCH 4.19.y-cip 19/57] ASoC: rsnd: use 32bit TDM width as
> default
> 
> On Thu 2019-10-17 08:04:51, Biju Das wrote:
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > commit 82ab7e9a4d3fcec27f745be04063e17da1881dda upstream.
> >
> > commit fb2815f44a9e ("ASoC: rsnd: add support for 16/24 bit slot
> > widths") added TDM width check, and return error if it was not 16/24/32
> bit.
> > But it is too strict. This patch uses 32bit same as default.
> 
> > @@ -738,8 +738,8 @@ static int rsnd_soc_set_dai_tdm_slot(struct
> snd_soc_dai *dai,
> >  	case 32:
> >  		break;
> >  	default:
> > -		dev_err(dev, "unsupported slot width value: %d\n",
> slot_width);
> > -		return -EINVAL;
> > +		/* use default */
> > +		slot_width = 32;
> >  	}
> >
> >  	switch (slots) {
> 
> I don't think I like this. People should not be passing strange values to this
> function. Can the caller be fixed, instead?

Morimoto San, Do you agree to Pavel's comment?

Regards,
Biju
Kuninori Morimoto Oct. 23, 2019, 1:45 a.m. UTC | #3 
Hi

> > > @@ -738,8 +738,8 @@ static int rsnd_soc_set_dai_tdm_slot(struct snd_soc_dai *dai,
> > >  	case 32:
> > >  		break;
> > >  	default:
> > > -		dev_err(dev, "unsupported slot width value: %d\n", slot_width);
> > > -		return -EINVAL;
> > > +		/* use default */
> > > +		slot_width = 32;
> > >  	}
> > >
> > >  	switch (slots) {
> > 
> > I don't think I like this. People should not be passing strange values to this
> > function. Can the caller be fixed, instead?
> 
> Morimoto San, Do you agree to Pavel's comment?

As git-log said, it is default values, not strange value.
People don't set all settings, especially it was default.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Pavel Machek Nov. 18, 2019, 3:32 p.m. UTC | #4 
On Wed 2019-10-23 10:45:18, Kuninori Morimoto wrote:
> 
> Hi
> 
> > > > @@ -738,8 +738,8 @@ static int rsnd_soc_set_dai_tdm_slot(struct snd_soc_dai *dai,
> > > >  	case 32:
> > > >  		break;
> > > >  	default:
> > > > -		dev_err(dev, "unsupported slot width value: %d\n", slot_width);
> > > > -		return -EINVAL;
> > > > +		/* use default */
> > > > +		slot_width = 32;
> > > >  	}
> > > >
> > > >  	switch (slots) {
> > > 
> > > I don't think I like this. People should not be passing strange values to this
> > > function. Can the caller be fixed, instead?
> > 
> > Morimoto San, Do you agree to Pavel's comment?
> 
> As git-log said, it is default values, not strange value.
> People don't set all settings, especially it was default.

I'd understand turning 0 to 32 (that is if someone forgot to set
it?). But turning 19 into 32 seems wrong.

Best regards,
								Pavel
Kuninori Morimoto Nov. 19, 2019, 12:53 a.m. UTC | #5 
Hi Pavel

> > > > > @@ -738,8 +738,8 @@ static int rsnd_soc_set_dai_tdm_slot(struct snd_soc_dai *dai,
> > > > >  	case 32:
> > > > >  		break;
> > > > >  	default:
> > > > > -		dev_err(dev, "unsupported slot width value: %d\n", slot_width);
> > > > > -		return -EINVAL;
> > > > > +		/* use default */
> > > > > +		slot_width = 32;
> > > > >  	}
> > > > >
> > > > >  	switch (slots) {
> > > > 
> > > > I don't think I like this. People should not be passing strange values to this
> > > > function. Can the caller be fixed, instead?
> > > 
> > > Morimoto San, Do you agree to Pavel's comment?
> > 
> > As git-log said, it is default values, not strange value.
> > People don't set all settings, especially it was default.
> 
> I'd understand turning 0 to 32 (that is if someone forgot to set
> it?). But turning 19 into 32 seems wrong.

Hmm... indeed.
But 19 (in this case) is wrong settings.
So, can you accept like this ?

	switch (slot_width) {
	...
	default:
		/* use default */
=>		if (slot_width)
=>			dev_warn(dev, "unsupported slot width(%d). use default\n", slot_width);
		slot_width = 32;
	}


Thank you for your help !!
Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index fd41da7..4a2a48e 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -738,8 +738,8 @@  static int rsnd_soc_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	case 32:
 		break;
 	default:
-		dev_err(dev, "unsupported slot width value: %d\n", slot_width);
-		return -EINVAL;
+		/* use default */
+		slot_width = 32;
 	}
 
 	switch (slots) {