by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 0/8] Generic I2C and SPI register map library

On Thu, Jun 23, 2011 at 09:54:25AM +0100, Jonathan Cameron wrote:
> On 06/23/11 02:25, Mark Brown wrote:

> > If you're only using SMBus then the I2C interface includes SMBus based
> > register support which should do what you want. With the regmap API it
> > ought to work as a device with 8 bit registers and 8 bit values.

> The kicker here is that quite a few hosts only support the smbus subset.
> How should that be handled here?

For 8x8 registers we *should* end up only using the SMBus subset so it
should just work.

2011-07-01 00:22:50

by Ben Hutchings

[permalink] [raw]

Subject: Re: [PATCH 1/8] regmap: Add generic non-memory mapped register access API

On Wed, 2011-06-22 at 19:45 +0100, Mark Brown wrote:
[...]
> --- /dev/null
> +++ b/drivers/regmap/regmap.c
[...]
> +static void regmap_format_4_12_write(struct regmap *map,
> + unsigned int reg, unsigned int val)
> +{
> + u16 *out = map->work_buf;

__be16 *out

> + *out = cpu_to_be16((reg << 12) | val);
> +}
> +
> +static void regmap_format_7_9_write(struct regmap *map,
> + unsigned int reg, unsigned int val)
> +{
> + u16 *out = map->work_buf;

__be16 *out

> + *out = cpu_to_be16((reg << 9) | val);
> +}
> +
> +static void regmap_format_8(void *buf, unsigned int val)
> +{
> + u8 *b = buf;
> +
> + b[0] = val;
> +}
> +
> +static void regmap_format_16(void *buf, unsigned int val)
> +{
> + u16 *b = buf;

__be16 *b

> + b[0] = cpu_to_be16(val);
> +}
> +
> +static unsigned int regmap_parse_8(void *buf)
> +{
> + u8 *b = buf;
> +
> + return b[0];
> +}
> +
> +static unsigned int regmap_parse_16(void *buf)
> +{
> + u16 *b = buf;

__be16 *b

> + 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]);

> +}
> +
> +/**
> + * remap_init: Initialise register map

Missing 'g' in 'regmap_init'. And the first line's format must be:

regmap_init() - Initialise register map

(The other functions all have this problem as well.)

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

Missing '@' before the parameter names.

[...]
> +static int _regmap_raw_write(struct regmap *map, unsigned int reg,
> + const void *val, size_t val_len)
> +{
> + void *buf;
> + int ret = -ENOTSUPP;
> + size_t len;
> +
> + map->format.format_reg(map->work_buf, reg);
> +
> + /* Try to do a gather write if we can */
> + if (map->bus->gather_write)
> + ret = map->bus->gather_write(map->dev, map->work_buf,
> + map->format.reg_bytes,
> + val, val_len);
> +
> + /* 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?

General comments:

- All the functions must be called in process context, but this isn't
documented.
- 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.

Ben.

--
Ben Hutchings
If the facts do not conform to your theory, they must be disposed of.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2011-07-01 02:38:53

by Mark Brown

[permalink] [raw]

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.