Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

On Thu, Jan 12, 2012 at 05:54:21PM +0530, Ashish Chavan wrote:

> Hi,

Remember this is a changelog...

> +/**
> + * da7210_set_dai_clkdiv: Set codec input clock divider
> + * @param codec_dai	: pointer to codec DAI
> + * @param div_id	: DA7210 has only one clk_div, so div_id is always zero
> + * @param div		: Divider value (configured via control reg MCLK_RANGE)
> + * @return int		: Zero for success, negative error code for error
> + *
> + */
> +static int da7210_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
> +				 int div_id, int div)

Why does the driver need the machine driver to manually configure clock
dividers?

> +	switch (fref) {
> +	case 12000000:
> +		mclk_offset = 0;
> +		break;

> +	switch (fout) {
> +	case 2822400:
> +		fsdm_offset = 0;
> +		break;

> +	if (da7210->master) {
> +		/* In PLL master mode, use Master mode PLL dividers */
> +		pll_div_offset = (mclk_offset * 2 + fsdm_offset) * 3;
> +		pll_div1 = da7210_pll_master_div[pll_div_offset];
> +		pll_div2 = da7210_pll_master_div[pll_div_offset + 1];
> +		pll_div3 = da7210_pll_master_div[pll_div_offset + 2];

This isn't great, you're indexing into a table of divisors using raw
numeric constants in a totally separate part of the code.  Worse, these
constants aren't even directly used but have a calculation applied to 
them apparently because this is really a multi dimensional array.  This
isn't great for either legibility or robustness.

Fix this to remove the use of magic numbers, for example by putting
fout and fref into the table and searching for them.

Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

On Wed, Jan 18, 2012 at 06:56:18PM +0530, Ashish Chavan wrote:

> > > +static int da7210_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
> > > +				 int div_id, int div)

> > Why does the driver need the machine driver to manually configure clock
> > dividers?

> Do you mean that the input mclk value should be passed via platform data
> and driver should use it from there during initialization?

No, it should be configured using set_sysclk() like for all the other
CODEC drivers.  The problem is that everyone using the driver needs to
know all the dividers in the chip.

Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On Wed, Apr 11, 2012 at 10:58:40AM +0530, Ashish Chavan wrote:

> +/* PLL dividers table */
> +static const struct pll_div da7210_pll_div[] = {
> +	/* for MASTER mode, fs = 44.1Khz */
> +	{ 12000000, 0xE8, 0x6C, 0x2, },		/* MCLK=12Mhz */
> +	{ 13000000, 0xDF, 0x28, 0xC, },		/* MCLK=13Mhz */
> +	{ 13500000, 0xDB, 0x0A, 0xD, },		/* MCLK=13.5Mhz */
> +	{ 14400000, 0xD4, 0x5A, 0x2, },		/* MCLK=14.4Mhz */
> +	{ 19200000, 0xBB, 0x43, 0x9, },		/* MCLK=19.2Mhz */
> +	{ 19680000, 0xB9, 0x6D, 0xA, },		/* MCLK=19.68Mhz */
> +	{ 19800000, 0xB8, 0xFB, 0xB, },		/* MCLK=19.8Mhz */
> +	/* for MASTER mode, fs = 48Khz */
> +	{ 12000000, 0xF3, 0x12, 0x7, },		/* MCLK=12Mhz */

This *still* has magic number problems.

> +	if (da7210->master) {
> +		/* In PLL master mode, use master mode PLL dividers */
> +		switch (fout) {
> +		case 2822400:
> +			row_idx = MASTER_2822400_DIV_OFFSET;
> +			break;
> +		case 3072000:
> +			row_idx = MASTER_3072000_DIV_OFFSET;
> +			break;

These defines now need to be kept in sync with the table and are going
to be *very* painful to review.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Mon, Apr 16, 2012 at 05:53:13PM +0530, Ashish Chavan wrote:
> On Fri, 2012-04-13 at 10:30 +0100, Mark Brown wrote:
> > On Wed, Apr 11, 2012 at 10:58:40AM +0530, Ashish Chavan wrote:

> > > +	/* for MASTER mode, fs = 48Khz */
> > > +	{ 12000000, 0xF3, 0x12, 0x7, },		/* MCLK=12Mhz */

> > This *still* has magic number problems.

> OK, will replace frequency values with defines. That is what you are
> pointing, right?

No!  As with *all* the other times you've submitted this you're relying
on magic array indexes to find stuff in the table which I've *repeatedly* 
pointed out is terrible for readability and maintainability.  It's very
disappointing to see the same problem coming back repeatedly.

> > These defines now need to be kept in sync with the table and are going
> > to be *very* painful to review.

> Yes, these defines need to be kept in sync with the table. Can you
> suggest any other, preferred way to do this?

Yes.  For example, you could use the same technique you're using for the
frequencies.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Mon, Apr 16, 2012 at 07:12:30PM +0530, Ashish Chavan wrote:

> Thanks for bearing with me Mark. But the only other way I can think is
> to make the *extra* info like master/slave and fs a part of array itself
> and then search through the array to get required divisors. In this
> case, every time I need to search through entire array to pick up
> correct divisors. Wouldn't that be inefficient, especially when the
> array indexes are fixed? Or the readability/maintainability of that
> would outweigh slight performance overhead?

If there is a genuine performance concern there's plenty of other ways
to do it which would make it run faster.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]