| Message ID | 20171214072928.6613-1-wens@csie.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2ff739b9bdd3199cd52e450097cb0f7fc4e1e9e8 |
| Headers | show |
On Thu, Dec 14, 2017 at 03:29:28PM +0800, Chen-Yu Tsai wrote: > When any of the DAI hardware configuration callbacks (.hw_param, > .set_fmt, .set_sysclk) fails, there is no explanation about why it > failed. This is particularly confusing for .hw_param, which covers > many parameters of the DAI. Telling the users what parameter isn't > supported, and what the requested value was goes a long way for > developers trying to combine sun4i-i2s with external codecs. > > This patch adds dev_err calls explaining what isn't supported or > failed, and what the value was. sun4i_i2s_set_clk_rate()'s first > parameter was changed to a struct snd_soc_dai *dai, so we can > get the underlying device. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
On 14 December 2017 at 08:29, Chen-Yu Tsai <wens@csie.org> wrote: > When any of the DAI hardware configuration callbacks (.hw_param, > .set_fmt, .set_sysclk) fails, there is no explanation about why it > failed. This is particularly confusing for .hw_param, which covers > many parameters of the DAI. Telling the users what parameter isn't > supported, and what the requested value was goes a long way for > developers trying to combine sun4i-i2s with external codecs. > > This patch adds dev_err calls explaining what isn't supported or > failed, and what the value was. sun4i_i2s_set_clk_rate()'s first > parameter was changed to a struct snd_soc_dai *dai, so we can > get the underlying device. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> Acked-by: Marcus Cooper <codekipper@gmail.com> Thanks...I can get rid of shit loads of my debugging now. > --- > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++++++++++++++++++------ > 1 file changed, 30 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 13d7ecabe1b6..dca1143c1150 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -269,10 +269,11 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample) > return false; > } > > -static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, > +static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, > unsigned int rate, > unsigned int word_size) > { > + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); > unsigned int oversample_rate, clk_rate; > int bclk_div, mclk_div; > int ret; > @@ -300,6 +301,7 @@ static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, > break; > > default: > + dev_err(dai->dev, "Unsupported sample rate: %u\n", rate); > return -EINVAL; > } > > @@ -308,18 +310,25 @@ static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, > return ret; > > oversample_rate = i2s->mclk_freq / rate; > - if (!sun4i_i2s_oversample_is_valid(oversample_rate)) > + if (!sun4i_i2s_oversample_is_valid(oversample_rate)) { > + dev_err(dai->dev, "Unsupported oversample rate: %d\n", > + oversample_rate); > return -EINVAL; > + } > > bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate, > word_size); > - if (bclk_div < 0) > + if (bclk_div < 0) { > + dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div); > return -EINVAL; > + } > > mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate, > clk_rate, rate); > - if (mclk_div < 0) > + if (mclk_div < 0) { > + dev_err(dai->dev, "Unsupported MCLK divider: %d\n", mclk_div); > return -EINVAL; > + } > > /* Adjust the clock division values if needed */ > bclk_div += i2s->variant->bclk_offset; > @@ -349,8 +358,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > u32 width; > > channels = params_channels(params); > - if (channels != 2) > + if (channels != 2) { > + dev_err(dai->dev, "Unsupported number of channels: %d\n", > + channels); > return -EINVAL; > + } > > if (i2s->variant->has_chcfg) { > regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, > @@ -382,6 +394,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > width = DMA_SLAVE_BUSWIDTH_2_BYTES; > break; > default: > + dev_err(dai->dev, "Unsupported physical sample width: %d\n", > + params_physical_width(params)); > return -EINVAL; > } > i2s->playback_dma_data.addr_width = width; > @@ -393,6 +407,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > break; > > default: > + dev_err(dai->dev, "Unsupported sample width: %d\n", > + params_width(params)); > return -EINVAL; > } > > @@ -401,7 +417,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > regmap_field_write(i2s->field_fmt_sr, > sr + i2s->variant->fmt_offset); > > - return sun4i_i2s_set_clk_rate(i2s, params_rate(params), > + return sun4i_i2s_set_clk_rate(dai, params_rate(params), > params_width(params)); > } > > @@ -426,6 +442,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) > val = SUN4I_I2S_FMT0_FMT_RIGHT_J; > break; > default: > + dev_err(dai->dev, "Unsupported format: %d\n", > + fmt & SND_SOC_DAIFMT_FORMAT_MASK); > return -EINVAL; > } > > @@ -464,6 +482,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) > case SND_SOC_DAIFMT_NB_NF: > break; > default: > + dev_err(dai->dev, "Unsupported clock polarity: %d\n", > + fmt & SND_SOC_DAIFMT_INV_MASK); > return -EINVAL; > } > > @@ -482,6 +502,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) > val = SUN4I_I2S_CTRL_MODE_SLAVE; > break; > default: > + dev_err(dai->dev, "Unsupported slave setting: %d\n", > + fmt & SND_SOC_DAIFMT_MASTER_MASK); > return -EINVAL; > } > regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, > @@ -504,6 +526,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) > val = 0; > break; > default: > + dev_err(dai->dev, "Unsupported slave setting: %d\n", > + fmt & SND_SOC_DAIFMT_MASTER_MASK); > return -EINVAL; > } > regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, > -- > 2.15.0 >
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 13d7ecabe1b6..dca1143c1150 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -269,10 +269,11 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample) return false; } -static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, +static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, unsigned int rate, unsigned int word_size) { + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int oversample_rate, clk_rate; int bclk_div, mclk_div; int ret; @@ -300,6 +301,7 @@ static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, break; default: + dev_err(dai->dev, "Unsupported sample rate: %u\n", rate); return -EINVAL; } @@ -308,18 +310,25 @@ static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s, return ret; oversample_rate = i2s->mclk_freq / rate; - if (!sun4i_i2s_oversample_is_valid(oversample_rate)) + if (!sun4i_i2s_oversample_is_valid(oversample_rate)) { + dev_err(dai->dev, "Unsupported oversample rate: %d\n", + oversample_rate); return -EINVAL; + } bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate, word_size); - if (bclk_div < 0) + if (bclk_div < 0) { + dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div); return -EINVAL; + } mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate, clk_rate, rate); - if (mclk_div < 0) + if (mclk_div < 0) { + dev_err(dai->dev, "Unsupported MCLK divider: %d\n", mclk_div); return -EINVAL; + } /* Adjust the clock division values if needed */ bclk_div += i2s->variant->bclk_offset; @@ -349,8 +358,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, u32 width; channels = params_channels(params); - if (channels != 2) + if (channels != 2) { + dev_err(dai->dev, "Unsupported number of channels: %d\n", + channels); return -EINVAL; + } if (i2s->variant->has_chcfg) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, @@ -382,6 +394,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, width = DMA_SLAVE_BUSWIDTH_2_BYTES; break; default: + dev_err(dai->dev, "Unsupported physical sample width: %d\n", + params_physical_width(params)); return -EINVAL; } i2s->playback_dma_data.addr_width = width; @@ -393,6 +407,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, break; default: + dev_err(dai->dev, "Unsupported sample width: %d\n", + params_width(params)); return -EINVAL; } @@ -401,7 +417,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, regmap_field_write(i2s->field_fmt_sr, sr + i2s->variant->fmt_offset); - return sun4i_i2s_set_clk_rate(i2s, params_rate(params), + return sun4i_i2s_set_clk_rate(dai, params_rate(params), params_width(params)); } @@ -426,6 +442,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) val = SUN4I_I2S_FMT0_FMT_RIGHT_J; break; default: + dev_err(dai->dev, "Unsupported format: %d\n", + fmt & SND_SOC_DAIFMT_FORMAT_MASK); return -EINVAL; } @@ -464,6 +482,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) case SND_SOC_DAIFMT_NB_NF: break; default: + dev_err(dai->dev, "Unsupported clock polarity: %d\n", + fmt & SND_SOC_DAIFMT_INV_MASK); return -EINVAL; } @@ -482,6 +502,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) val = SUN4I_I2S_CTRL_MODE_SLAVE; break; default: + dev_err(dai->dev, "Unsupported slave setting: %d\n", + fmt & SND_SOC_DAIFMT_MASTER_MASK); return -EINVAL; } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, @@ -504,6 +526,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) val = 0; break; default: + dev_err(dai->dev, "Unsupported slave setting: %d\n", + fmt & SND_SOC_DAIFMT_MASTER_MASK); return -EINVAL; } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
When any of the DAI hardware configuration callbacks (.hw_param, .set_fmt, .set_sysclk) fails, there is no explanation about why it failed. This is particularly confusing for .hw_param, which covers many parameters of the DAI. Telling the users what parameter isn't supported, and what the requested value was goes a long way for developers trying to combine sun4i-i2s with external codecs. This patch adds dev_err calls explaining what isn't supported or failed, and what the value was. sun4i_i2s_set_clk_rate()'s first parameter was changed to a struct snd_soc_dai *dai, so we can get the underlying device. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-)