Re: [PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching

[Dimitris Papastamos <dp@opensource.wolfsonmicro.com>]

On Fri, 2010-10-22 at 09:18 -0700, Mark Brown wrote:
> On Fri, Oct 22, 2010 at 03:28:22PM +0100, Dimitris Papastamos wrote:
> 
> > This patch adds support for LZO compression when storing the register
> > cache.  The initial register defaults cache is marked as __devinitconst
> > and the only change required for a driver to use LZO compression is
> 
> This looks good, though the changelog could use a bit more discussion as
> to the tradeoffs involved - clearly we're trading CPU consumption for
> memory consumption here, but things like numbers about the sorts of
> compression you can get and the amount of CPU time burned relative to
> the actual I/O operations would help people understand when and how to
> use this.

Yes, that makes sense.

> Actually, now that I think about it debug data either via debugfs or via
> dev_dbg() showing the before and after memory sizes would be very useful
> for people trying to decide if their register map compresses down well
> enough to really benefit from compression.

I'll send an incremental patch for that.

> It looks like this patch also needs to add selects for LZO_COMPRESS and
> LZO_DECOMPRESS to Kconfig, otherwise we'll fail to build if nothing else
> has enabled them.
> 
> > to set the compress_type member in codec->driver to
> > SND_SOC_LZO_COMPRESSION.
> 
> It would be good if machine drivers were able to override this, 

I'll think about this to see how it can be overriden.

> > +static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
> > +					struct snd_soc_lzo_ctx *lzo_ctx)
> > +{
> 
> This is all rather assuming that LZO is the only compression method we
> can use?  It doesn't really matter, though, as this is all internal to
> the cache code so we can deal with adding new compression types as and
> when we want them.

The function naming is wrong.  This function is a helper function for
the LZO compression type.  I've prefixed this function with LZO to avoid
misunderstanding.  Ideally this would live in a separate file.

Thanks,
Dimitrios