lkml.org 
[lkml]   [2011]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/8] regmap: Add generic non-memory mapped register access API
    From
    Date
    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]
    \
     
     \ /
      Last update: 2011-07-01 02:25    [W:0.026 / U:31.436 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site