Date:	Thu, 30 Jun 2011 19:38:49 -0700
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Ben Hutchings <ben@...adent.org.uk>
Cc:	linux-kernel@...r.kernel.org,
	Dimitris Papastamos <dp@...nsource.wolfsonmicro.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Liam Girdwood <lrg@...com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Graeme Gregory <gg@...mlogic.co.uk>
Subject: Re: [PATCH 1/8] regmap: Add generic non-memory mapped register
 access API

On Fri, Jul 01, 2011 at 01:22:41AM +0100, Ben Hutchings wrote:
> On Wed, 2011-06-22 at 19:45 +0100, Mark Brown wrote:

> > +	b[0] = be16_to_cpu(b[0]);
> > +
> > +	return b[0];

> The assignment doesn't seem to be necessary and will irritate sparse,
> so:

> 	return be16_to_cpu(b[0]);

sparse is silent on whatever the issue is here and the assignment is
used (we both modify the buffer in place and return the result of the
conversion to make life more convenient for users).  Which isn't
urgently tasteful but felt less tasteless than anything else I thought
of.  Data manging doesn't lend itself to elegance.

> > + * dev: Device that will be interacted with
> > + * config: Configuration for register map

> Missing '@' before the parameter names.

You're looking at an old version of the code.

> > +	/* Otherwise fall back on linearising by hand. */
> > +	if (ret == -ENOTSUPP) {
> > +		len = map->format.reg_bytes + val_len;
> > +		buf = kmalloc(len, GFP_KERNEL);
> > +		if (!buf)
> > +			return -ENOMEM;
> > +		memcpy(buf, map->work_buf, map->format.reg_bytes);
> > +		memcpy(buf + map->format.reg_bytes, val, val_len);

> If I understand correctly, this is usually called by _regmap_write() and
> val is already at the appropriate offset in work_buf.  Is it not worth
> avoiding the allocation in that case?

It's marginal, and given that it's vanishingly rare to hit this case
(essentially all the interesting controllers for this API can do the
gather write) I didn't feel it was worth it for a first pass.  We can
always do the optimization later if someone finds it important.  I
suspect any controller that hits the issue is going to be slow enough to
dwarf the cost of the allocation with the I/O cost.

> General comments:

> - All the functions must be called in process context, but this isn't
> documented.

This should be obvious, none of the relevant buses can be interacted
with in anything other than process context anyway as they need to
sleep.

> - I think this could go in lib or drivers/base rather than in a new
> directory.  The specific support for I2C and SPI would go in their
> existing directories.

lib feels wrong, it's going to be peering into the drivers directory too
much.

drivers/base is a good point, but then the code will grow quite a bit
once we start layering cache support on top of it and until the code
settles down I don't really want to have to deal with cross tree issues
if we decide we need to update the interface to the bus code, that's
definitely a first pass thing right now.  The latter is the main thing
that made me do things this way rather than pushing things into the
buses, it's definitely not the finished product right now internally -
the client API should be reasonably stable so not have these issues.

My inclination is to start off isolating the code until we've reached
the point where things are more static.  I suppose we could put it in a
subdirectory of drivers/base in the meantime, though.  Will have a think
for the next repost.

Fixed everything else, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists