alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
From: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
To: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: alsa-devel@alsa-project.org, patches@opensource.wolfsonmicro.com,
	Timur Tabi <timur@freescale.com>,
	Liam Girdwood <lrg@slimlogic.co.uk>
Subject: Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching
Date: Mon, 25 Oct 2010 09:09:40 +0100	[thread overview]
Message-ID: <1287994180.9010.13.camel@dplaptop> (raw)
In-Reply-To: <20101024213525.GA14256@opensource.wolfsonmicro.com>

On Sun, 2010-10-24 at 14:35 -0700, Mark Brown wrote:
> On Sun, Oct 24, 2010 at 08:18:59AM -0500, Timur Tabi wrote:
> > On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos
> 
> > > The only problem I see with the above code, is when
> > > codec_drv->reg_word_size > sizeof (unsigned int) but that can't really
> > > happen in practice.
> 
> Plus if it did happen the rest of the code would fall over fairly badly
> since we've got that assumption embedded in the register read and write
> APIs.
> 
> > I'm going to have to agree with Mark that this code is suspect.  I
> > understand everything you said, but it makes me nervous.  Unless this
> > code is in some kind of fast-path, I would prefer to see it rewritten
> > to avoid any assumption about the sizes of the types involved.
> 
> I think the important thing here is that the code is clear - from a
> maintainability so long as it's clear how and why the code works things
> should be fine, otherwise we'll have people scratching their heads over
> it every time someone looks at the code which is going to be painful.
> This could be done with documentation as well as with code changes,
> though code changes should definitely be considered.

Yes that makes sense.  The reason why I did this in the first place was
to make it work with 8/16-byte reads/writes without using branching.  I
will change this to explicitly dereference a u8/u16 pointer.

Thanks,
Dimitrios

  reply	other threads:[~2010-10-25  8:09 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-10-22 14:28 [PATCH 0/4] ASoC: Implement new caching API Dimitris Papastamos
2010-10-22 14:28 ` [PATCH 1/4] ASoC: soc.h: Add new caching API prototypes and hooks Dimitris Papastamos
2010-10-22 15:44   ` Mark Brown
2010-10-22 14:28 ` [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching Dimitris Papastamos
2010-10-22 16:03   ` Mark Brown
2010-10-23 18:24     ` Dimitris Papastamos
2010-10-24 13:18       ` Timur Tabi
2010-10-24 21:35         ` Mark Brown
2010-10-25  8:09           ` Dimitris Papastamos [this message]
2010-10-22 14:28 ` [PATCH 3/4] ASoC: soc-core: Adapt soc-core to fit the new caching API Dimitris Papastamos
2010-10-22 14:28 ` [PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching Dimitris Papastamos
2010-10-22 16:18   ` Mark Brown
2010-10-23 18:28     ` Dimitris Papastamos
2010-10-25 18:13       ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1287994180.9010.13.camel@dplaptop \
    --to=dp@opensource.wolfsonmicro.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=lrg@slimlogic.co.uk \
    --cc=patches@opensource.wolfsonmicro.com \
    --cc=timur@freescale.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).