Re: [PATCH 6/7] ASoC: max9867: Fix BSEL value in master mode.

[Ladislav Michl <ladis@linux-mips.org>]

On Tue, Feb 27, 2018 at 06:23:09PM +0100, Alexandre Belloni wrote:
> This also needs a proper commit message.

Meanwhile more patches for this driver accumulated, so I'll send simple ones
first and those I'm uncertain of later.

Now few questions:
- should hw_params emit any error message when asked to set for example
  unsupported sample rate? Many drivers do so, but it seems strange.
- table used in this driver is overkill, frequencies can be calculated
  directly (patch ready), but that brings question about
  SNDRV_PCM_RATE_CONTINUOUS: what does it exactly mean? What if codec is
  able to set "any" rate, but there are rounding errors?

> On 30/01/2018 at 12:10:33 +0100, Ladislav Michl wrote:
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  Note: It should be reconsidered where BSEL should be set.
> > 
> >  sound/soc/codecs/max9867.c | 43 +++----------------------------------------
> >  1 file changed, 3 insertions(+), 40 deletions(-)
> > 
> > diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> > index 026b7cf94910..6272cf5df4a9 100644
> > --- a/sound/soc/codecs/max9867.c
> > +++ b/sound/soc/codecs/max9867.c
> > @@ -148,46 +148,6 @@ static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
> >  			MAX9867_RAPID_LOCK, MAX9867_RAPID_LOCK);
> >  		regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
> >  			MAX9867_PLL, MAX9867_PLL);
> > -	} else {
> > -		unsigned long int bclk_rate, pclk_bclk_ratio;
> > -		int bclk_value;
> > -
> > -		bclk_rate = params_rate(params) * 2 * params_width(params);
> > -		pclk_bclk_ratio = max9867->pclk/bclk_rate;
> > -		switch (params_width(params)) {
> > -		case 8:
> > -		case 16:
> > -			switch (pclk_bclk_ratio) {
> > -			case 2:
> > -				bclk_value = MAX9867_IFC1B_PCLK_2;
> > -				break;
> > -			case 4:
> > -				bclk_value = MAX9867_IFC1B_PCLK_4;
> > -				break;
> > -			case 8:
> > -				bclk_value = MAX9867_IFC1B_PCLK_8;
> > -				break;
> > -			case 16:
> > -				bclk_value = MAX9867_IFC1B_PCLK_16;
> > -				break;
> > -			default:
> > -				dev_err(codec->dev,
> > -					"unsupported sampling rate\n");
> > -				return -EINVAL;
> > -			}
> > -			break;
> > -		case 24:
> > -			bclk_value = MAX9867_IFC1B_24BIT;
> > -			break;
> > -		case 32:
> > -			bclk_value = MAX9867_IFC1B_32BIT;
> > -			break;
> > -		default:
> > -			dev_err(codec->dev, "unsupported sampling rate\n");
> > -			return -EINVAL;
> > -		}
> > -		regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > -			MAX9867_IFC1B_BCLK_MASK, bclk_value);
> >  	}
> >  	return 0;
> >  }
> > @@ -244,6 +204,9 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> >  	value &= ~MAX9867_FREQ_MASK;
> >  	regmap_update_bits(max9867->regmap, MAX9867_SYSCLK,
> >  			MAX9867_PSCLK_MASK, value);
> > +	regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > +			MAX9867_IFC1B_BCLK_MASK, 0x010);
> 
> This magic value has to be defined somewhere.
> 
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com