Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU
- From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
- Date: Wed, 17 Sep 2008 19:28:36 -0700
On Tue, 16 Sep 2008 10:43:13 +0100 Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
The WM8400 is a highly integrated audio CODEC and power management unit
optimised for use in mobile multimedia applications. This patch adds
core support for the WM8400 to the MFD subsystem.
Both I2C and SPI access are supported by the hardware but currently only
I2C access is implemented. The code is structured to allow SPI support
to be slotted in later.
Various ankle-biting comments, just to show I read it:
...
+#include <linux/bug.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mfd/wm8400-private.h>
+#include <linux/mfd/wm8400-audio.h>
+
+static struct
+{
hm, checkpatch chews a couple minutes CPU time then misses this error.
static struct {
please.
+ u16 readable; /* Mask of readable bits */
+ u16 writable; /* Mask of writable bits */
+ u16 vol; /* Mask of volatile bits */
+ int is_codec; /* Register controlled by codec reset */
+ u16 default_val; /* Value on reset */
+} reg_data[] =
+{
} reg_data[] = {
would be conventional, too.
+ { 0xFFFF, 0xFFFF, 0x0000, 0, 0x6172 }, /* R0 */
+ { 0x7000, 0x0000, 0x8000, 0, 0x0000 }, /* R1 */
+ { 0xFF17, 0xFF17, 0x0000, 0, 0x0000 }, /* R2 */
+ { 0xEBF3, 0xEBF3, 0x0000, 1, 0x6000 }, /* R3 */
+ { 0x3CF3, 0x3CF3, 0x0000, 1, 0x0000 }, /* R4 */
...
+/**
+ * wm8400_reg_read - Single register read
+ *
+ * @wm8400: Pointer to wm8400 control structure
+ * @reg: Register to read
+ *
+ * @return Read value
+ */
+u16 wm8400_reg_read(struct wm8400 *wm8400, u8 reg)
+{
+ u16 val;
+
+ mutex_lock(&wm8400->io_lock);
+
+ wm8400_read(wm8400, reg, 1, &val);
+
+ mutex_unlock(&wm8400->io_lock);
+
+ return val;
+}
+EXPORT_SYMBOL_GPL(wm8400_reg_read);
Is it just me, or do all the useless newlines there look silly? sigh.
+int wm8400_block_read(struct wm8400 *wm8400, u8 reg, int count, u16 *data)
+{
+ int ret;
+
+ mutex_lock(&wm8400->io_lock);
+
+ ret = wm8400_read(wm8400, reg, count, data);
+
+ mutex_unlock(&wm8400->io_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(wm8400_block_read);
+
...
+static void wm8400_release(struct wm8400 *wm8400)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(wm8400->regulators); i++)
+ if (wm8400->regulators[i].name)
+ platform_device_unregister(&wm8400->regulators[i]);
+}
+
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
Is CONFIG_I2C=n worth supporting? Is anyone likely to test and
maintain that combination?
+static int wm8400_i2c_read(void *io_data, char reg, int count, u16 *dest)
+{
+ struct i2c_client *i2c = io_data;
+ struct i2c_msg xfer[2];
+ int ret;
+
+ /* Write register */
+ xfer[0].addr = i2c->addr;
+ xfer[0].flags = 0;
+ xfer[0].len = 1;
+ xfer[0].buf = ®
+
+ /* Read data */
+ xfer[1].addr = i2c->addr;
+ xfer[1].flags = I2C_M_RD;
+ xfer[1].len = count * sizeof(u16);
+ xfer[1].buf = (u8 *)dest;
+
+ ret = i2c_transfer(i2c->adapter, xfer, 2);
+ if (ret == 2)
+ ret = 0;
+ else if (ret >= 0)
+ ret = -EIO;
+
+ return ret;
+}
+
+static int wm8400_i2c_write(void *io_data, char reg, int count, const u16 *src)
+{
+ struct i2c_client *i2c = io_data;
+ u8 *msg;
+ int ret;
+
+ /* We add 1 byte for device register - ideally I2C would gather. */
+ msg = kmalloc((count * sizeof(u16)) + 1, GFP_KERNEL);
+ if (msg == NULL)
+ return -ENOMEM;
+
+ msg[0] = reg;
+ memcpy(&msg[1], src, count * sizeof(u16));
+
+ ret = i2c_master_send(i2c, msg, (count * sizeof(u16)) + 1);
+
+ if (ret == (count * 2) + 1)
+ ret = 0;
+ else if (ret >= 0)
+ ret = -EIO;
+
+ kfree(msg);
+
+ return ret;
+}
Always freaks me out to see read/write methods returning `int' instead
of `ssize_t'. i2c weirdness, iirc.
...
+struct wm8400 {
+ struct device *dev;
+
+ int (*read_dev)(void *data, char reg, int count, u16 *dst);
+ int (*write_dev)(void *data, char reg, int count, const u16 *src);
+
+ struct mutex io_lock;
+ void *io_data;
+
+ u16 reg_cache[WM8400_REGISTER_COUNT];
Should this be __be16?
+ struct platform_device regulators[6];
+};
+
...
+#define WM8400_LINE_CMP_VTHD_MASK 0x000F /* LINE_CMP_VTHD - [3:0] */
+#define WM8400_LINE_CMP_VTHD_SHIFT 0 /* LINE_CMP_VTHD - [3:0] */
+#define WM8400_LINE_CMP_VTHD_WIDTH 4 /* LINE_CMP_VTHD - [3:0] */
+
This patch adds an amazing 1733 #defines, of which only a few
percent are used. Oh well.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- References:
- [PATCH 0/2] WM8400 support v2
- From: Mark Brown
- [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU
- From: Mark Brown
- [PATCH 0/2] WM8400 support v2
- Prev by Date: Re: [PATCH -mm] memcg: fix handling of shmem migration(v2)
- Next by Date: Re: [PATCH -mm] memcg: fix handling of shmem migration(v2)
- Previous by thread: [PATCH 2/2] regulator: Add WM8400 regulator support
- Next by thread: Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU
- Index(es):
Relevant Pages
|
