lkml.org 
[lkml]   [2011]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/8] regmap: Add generic non-memory mapped register access API
    Very nice!

    > [...]
    > diff --git a/drivers/regmap/regmap.c b/drivers/regmap/regmap.c
    > new file mode 100644
    > index 0000000..a8610c7
    > --- /dev/null
    > +++ b/drivers/regmap/regmap.c
    > @@ -0,0 +1,478 @@
    > +/*
    > + * Register map access API
    > + *
    > + * Copyright 2011 Wolfson Microelectronics plc
    > + *
    > + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License version 2 as
    > + * published by the Free Software Foundation.
    > + */
    > +
    > +#include <linux/slab.h>
    > +#include <linux/module.h>
    > +#include <linux/mutex.h>
    > +#include <linux/err.h>
    > +
    > +#include <linux/regmap.h>
    > +
    > +struct regmap;
    > +
    > +static DEFINE_MUTEX(regmap_bus_lock);
    > +static LIST_HEAD(regmap_bus_list);
    > +
    > +struct regmap_format {
    > + size_t buf_size;
    > + size_t reg_bytes;
    > + size_t val_bytes;
    > + void (*format_write)(struct regmap *map,
    > + unsigned int reg, unsigned int val);
    > + void (*format_reg)(void *buf, unsigned int reg);
    > + void (*format_val)(void *buf, unsigned int val);
    > + unsigned int (*parse_val)(void *buf);

    const void *buf. It probably also makes sense to pass the regmap to all
    callbacks, so we can write generic format/parser functions which use
    information for example from the regmap_format.

    > +};
    > +
    > +struct regmap {
    > + struct mutex lock;
    > +
    > + struct device *dev; /* Device we do I/O on */
    > + void *work_buf; /* Scratch buffer used to format I/O */
    > + struct regmap_format format; /* Buffer format */
    > + struct regmap_bus *bus;

    const struct regmap_bus

    > +};
    > +
    > +static void regmap_format_4_12_write(struct regmap *map,
    > + unsigned int reg, unsigned int val)
    > +{
    > + u16 *out = map->work_buf;
    > + *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;
    > + *out = cpu_to_be16((reg << 9) | val);
    > +}

    It would make sense to keep val_bits around and implement these two generically:
    *out = cpu_to_be16((reg << map->format.val_bits) | val);

    > [...]
    > +/**
    > + * remap_init: Initialise register map
    > + *
    > + * dev: Device that will be interacted with
    > + * config: Configuration for register map
    > + *
    > + * The return value will be an ERR_PTR() on error or a valid pointer
    > + * to a struct regmap.
    > + */
    > +struct regmap *regmap_init(struct device *dev, struct regmap_config *config)

    regmap_alloc would in my opinion be a better name.

    > [...]
    > +static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
    > + unsigned int val_len)
    > +{
    > + u8 *u8 = map->work_buf;
    > + int ret;
    > +
    > + map->format.format_reg(map->work_buf, reg);
    > +
    > + /*
    > + * Some buses flag reads by setting the high bit in the
    > + * register addresss; since it's always the high bit for all
    > + * current formats we can do this here rather than in
    > + * formatting. This may break if we get interesting formats.
    > + */
    > + if (map->bus->read_flag_in_reg)
    > + u8[0] |= 0x80;

    This is rather specific. Making this a per device mask or bit offset, would
    make more sense in my opinion. I have for example one SPI codec device which
    uses the 7th bit to indicate a read. And I also have devices on a custom bus,
    which needs to set the upper bit to indicate a write, so a mask for both write
    and read would be good.

    > +
    > + ret = map->bus->read(map->dev, map->work_buf, map->format.reg_bytes,
    > + val, map->format.val_bytes);
    > + if (ret != 0)
    > + return ret;
    > +
    > + return 0;
    > +}
    > +
    > [...]
    > +/**
    > + * regmap_bulk_read: Read multiple registers from the device
    > + *
    > + * @map: Register map to write to
    > + * @reg: First register to be read from
    > + * @val: Pointer to store read value, in native register size for device
    > + * @val_count: Number of registers to read
    > + *
    > + * A value of zero will be returned on success, a negative errno will
    > + * be returned in error cases.
    > + */
    > +int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
    > + size_t val_count)
    > +{
    > + int ret, i;
    > + size_t val_bytes = map->format.val_bytes;
    > +
    > + if (!map->format.parse_val)
    > + return -EINVAL;
    > +
    > + ret = regmap_raw_read(map, reg, val, val_bytes);

    val_bytes * val_count

    > + if (ret != 0)
    > + return ret;
    > +
    > + for (i = 0; i < val_count * val_bytes; i += val_bytes)
    > + map->format.parse_val(val + i);

    This doesn't make sense. parse_val returns the parsed value, but doesn't modify
    the parsed data. It would make sense to make the interface similar to
    regmap_read and make the returned values a unsigned integer array and use a
    bounce buffer for reading them from the device.

    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(regmap_bulk_read);
    > +
    > [...]
    > diff --git a/include/linux/regmap.h b/include/linux/regmap.h
    > new file mode 100644
    > index 0000000..0c2e402
    > --- /dev/null
    > +++ b/include/linux/regmap.h
    > @@ -0,0 +1,75 @@
    > +#ifndef __LINUX_REGMAP_H
    > +#define __LINUX_REGMAP_H
    > +
    > +/*
    > + * Register map access API
    > + *
    > + * Copyright 2011 Wolfson Microelectronics plc
    > + *
    > + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License version 2 as
    > + * published by the Free Software Foundation.
    > + */
    > +
    > +#include <linux/device.h>
    > +#include <linux/list.h>
    #include <linux/module.h>

    > +
    > +struct regmap_config {
    > + int reg_bits;
    > + int val_bits;

    size_t for both?

    > +};
    > +
    > ...
    > +
    > +struct regmap *regmap_init(struct device *dev, struct regmap_config *config);

    const struct regmap_config

    > +void regmap_free(struct regmap *map);
    > +int regmap_write(struct regmap *map, unsigned int reg, unsigned int val);
    > +int regmap_raw_write(struct regmap *map, unsigned int reg,
    > + const void *val, size_t val_len);
    > +int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val);
    > +int regmap_raw_read(struct regmap *map, unsigned int reg,
    > + void *val, size_t val_len);
    > +int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
    > + size_t val_count);

    What about bulk_write?


    \
     
     \ /
      Last update: 2011-06-21 01:19    [W:0.040 / U:0.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site