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
next prev parent 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.