| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Jun 2011 01:15:18 +0200
From: Lars-Peter Clausen <lars@...afoo.de>
To: Mark Brown <broonie@...nsource.wolfsonmicro.com>
CC: linux-kernel@...r.kernel.org,
Dimitris Papastamos <dp@...nsource.wolfsonmicro.com>,
Liam Girdwood <lrg@...com>,
Samuel Oritz <sameo@...ux.intel.com>,
Graeme Gregory <gg@...mlogic.co.uk>
Subject: Re: [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@...nsource.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@...nsource.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?
--
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