Messages in this thread | | | Subject | Re: [PATCH 1/8] regmap: Add generic non-memory mapped register access API | From | Ben Hutchings <> | Date | Fri, 01 Jul 2011 01:22:41 +0100 |
| |
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. [unhandled content-type:application/pgp-signature] | |