[alsa-devel] [PATCH 04/14] ASoC: Allow codecs to override register display

[Takashi Iwai]

At Fri, 1 Aug 2008 10:50:37 -0400,
Jon Smirl wrote:
> 
> On 8/1/08, Mark Brown <broonie at opensource.wolfsonmicro.com> wrote:
> > On Fri, Aug 01, 2008 at 11:42:28AM +0200, Takashi Iwai wrote:
> >  > Jon Smirl wrote:
> >
> >  > > Mark, I see this patch is already in your tree. I have rewritten it a
> >  > > little differently to ensure that too many results can't be returned.
> >  > > This is in my system and tested. I had a bug too so I know the
> >  > > limiting it to pagesize code works.
> >
> >  > Mark's patch was already applied.  Could you re-generated your change
> >  > based on that?
> >
> >
> > In general it's easiest if ASoC patches are generated against Takashi's
> >  master branch - that's where ASoC gets merged and usually the diff
> >  between the two trees is very small.
> >
> 
> This is against Takashi's tree. I handled PAGE_LIMIT differently,
> instead of breaking from the loop I just let the loop finished and
> told snprintf it had zero bytes.
> 
> Allow external display_register()'s to skip sparse registers.
> 
> Signed-off-by: Jon Smirl <jonsmirl at gmail.com>
> 
> ---
> 
>  include/sound/soc.h  |    3 +--
>  sound/soc/soc-core.c |   33 ++++++++++-----------------------
>  2 files changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index a1e0357..1f4a9a2 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -416,8 +416,7 @@ struct snd_soc_codec {
>  	void *control_data; /* codec control (i2c/3wire) data */
>  	unsigned int (*read)(struct snd_soc_codec *, unsigned int);
>  	int (*write)(struct snd_soc_codec *, unsigned int, unsigned int);
> -	int (*display_register)(struct snd_soc_codec *, char *,
> -				size_t, unsigned int);
> +	int (*display_register)(struct snd_soc_codec *, char *, unsigned
> int, unsigned int);

This change is backward.

> @@ -969,31 +969,18 @@ static ssize_t codec_reg_show(struct device *dev,
>  	if (codec->reg_cache_step)
>  		step = codec->reg_cache_step;
> 
> -	count += sprintf(buf, "%s registers\n", codec->name);
> +	count = sprintf(buf, "%s registers\n", codec->name);
>  	for (i = 0; i < codec->reg_cache_size; i += step) {
> -		count += sprintf(buf + count, "%2x: ", i);
> -		if (count >= PAGE_SIZE - 1)
> -			break;
> -
>  		if (codec->display_register)
> -			count += codec->display_register(codec, buf + count,
> -							 PAGE_SIZE - count, i);
> -		else
> -			count += snprintf(buf + count, PAGE_SIZE - count,
> -					  "%4x", codec->read(codec, i));
> -
> -		if (count >= PAGE_SIZE - 1)
> -			break;
> -
> -		count += snprintf(buf + count, PAGE_SIZE - count, "\n");
> -		if (count >= PAGE_SIZE - 1)
> -			break;
> +			count += codec->display_register(codec,
> +							 buf + count, PAGE_SIZE - count, i);

Please make it fit in 80 chars like the original code.

> +		else {
> +			count += snprintf(buf + count, PAGE_SIZE - count, "%2x: ", i);
> +			count += snprintf(buf + count, PAGE_SIZE - count, "%4x",
> +					 codec->read(codec, i));
> +			count += snprintf(buf + count, PAGE_SIZE - count, "\n");

These can be a single snprintf().

You should add the loop break condition (count >= PAGE_SIZE - 1)
somewhere, too.

> +		}
>  	}
> -
> -	/* Truncate count; min() would cause a warning */
> -	if (count >= PAGE_SIZE)
> -		count = PAGE_SIZE - 1;
> -

Really safe to remove this?


thanks,

Takashi