> -----Original Message-----
> From: Jonathan Cameron [mailto:ji...@kernel.org]
> Sent: 01 January, 2015 12:59
> To: Tirdea, Irina; linux-...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Dogaru, Vlad; Baluta, Daniel; Hartmut
> Knaack; Lars-Peter Clausen; Peter Meerwald
> Subject: Re: [PATCH 7/8] iio: accel: mma9551: split driver to expose mma955x
> api
>
> On 19/12/14 22:57, Irina Tirdea wrote:
> > Freescale has the MMA955xL family of devices that use the
> > same communication protocol (based on i2c messages):
> > http://www.freescale.com/files/sensors/doc/data_sheet/MMA955xL.pdf.
> >
> > To support more devices from this family, we need to split the
> > mma9551 driver so we can export the common functions that will
> > be used by other mma955x drivers.
> >
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > Reviewed-by: Vlad Dogaru <vlad.dog...@intel.com>
> Sorry, didn't get this far the other day.
>
> Anyhow, other than a query on the depends and a suggestion that some
> documentation of locking semantics might be good (perhaps in some
> general kernel doc for the exported functions) - this looks good to
> me.
>
> Jonathan
> > ---
> > drivers/iio/accel/Kconfig | 6 +
> > drivers/iio/accel/Makefile | 4 +-
> > drivers/iio/accel/mma9551.c | 443
> > +-------------------------------------
> > drivers/iio/accel/mma9551_core.c | 443
> > ++++++++++++++++++++++++++++++++++++++
> > drivers/iio/accel/mma9551_core.h | 74 +++++++
> > 5 files changed, 530 insertions(+), 440 deletions(-)
> > create mode 100644 drivers/iio/accel/mma9551_core.c
> > create mode 100644 drivers/iio/accel/mma9551_core.h
> >
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index d80616d..0600798 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -105,9 +105,15 @@ config KXCJK1013
> > To compile this driver as a module, choose M here: the module will
> > be called kxcjk-1013.
> >
> > +config MMA9551_CORE
> > + tristate
> > + depends on MMA9551
> Why depend here? THis isn't visible (due to lack of help) and in theory
> an out of tree driver might want to use it. Hence don't think you want this.
Right. Will remove this.
> > +
> > config MMA9551
> > tristate "Freescale MMA9551L Intelligent Motion-Sensing Platform Driver"
> > depends on I2C
> > + select MMA9551_CORE
> > +
> > help
> > Say yes here to build support for the Freescale MMA9551L
> > Intelligent Motion-Sensing Platform Driver.
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index de5b9cb..8105316 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -9,7 +9,9 @@ obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
> > obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
> > obj-$(CONFIG_KXSD9) += kxsd9.o
> > obj-$(CONFIG_MMA8452) += mma8452.o
> > -obj-$(CONFIG_MMA9551) += mma9551.o
> > +
> > +obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o
> > +obj-$(CONFIG_MMA9551) += mma9551.o
> >
> > obj-$(CONFIG_IIO_ST_ACCEL_3AXIS) += st_accel.o
> > st_accel-y := st_accel_core.o
> > diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> > index 34ee9d6..6031b5a 100644
> > --- a/drivers/iio/accel/mma9551.c
> > +++ b/drivers/iio/accel/mma9551.c
> > @@ -23,63 +23,13 @@
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/events.h>
> > #include <linux/pm_runtime.h>
> > +#include "mma9551_core.h"
> >
> > #define MMA9551_DRV_NAME "mma9551"
> > #define MMA9551_IRQ_NAME "mma9551_event"
> > #define MMA9551_GPIO_NAME "mma9551_int"
> > #define MMA9551_GPIO_COUNT 4
> >
> > -/* Applications IDs */
> > -#define MMA9551_APPID_VERSION 0x00
> > -#define MMA9551_APPID_GPIO 0x03
> > -#define MMA9551_APPID_AFE 0x06
> > -#define MMA9551_APPID_TILT 0x0B
> > -#define MMA9551_APPID_SLEEP_WAKE 0x12
> > -#define MMA9551_APPID_RESET 0x17
> > -#define MMA9551_APPID_NONE 0xff
> > -
> > -/* Command masks for mailbox write command */
> > -#define MMA9551_CMD_READ_VERSION_INFO 0x00
> > -#define MMA9551_CMD_READ_CONFIG 0x10
> > -#define MMA9551_CMD_WRITE_CONFIG 0x20
> > -#define MMA9551_CMD_READ_STATUS 0x30
> > -
[...]
> > diff --git a/drivers/iio/accel/mma9551_core.c
> > b/drivers/iio/accel/mma9551_core.c
> > new file mode 100644
> > index 0000000..cab481b
> > --- /dev/null
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -0,0 +1,443 @@
> > +/*
> > + * Common code for Freescale MMA955x Intelligent Sensor Platform drivers
> > + * Copyright (c) 2014, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > for
> > + * more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/pm_runtime.h>
> > +#include "mma9551_core.h"
> > +
> > +/* Command masks for mailbox write command */
> > +#define MMA9551_CMD_READ_VERSION_INFO 0x00
> > +#define MMA9551_CMD_READ_CONFIG 0x10
> > +#define MMA9551_CMD_WRITE_CONFIG 0x20
> > +#define MMA9551_CMD_READ_STATUS 0x30
> > +
> > +/* Mailbox read command */
> > +#define MMA9551_RESPONSE_COCO BIT(7)
> > +
> > +/* Error-Status codes returned in mailbox read command */
> > +#define MMA9551_MCI_ERROR_NONE 0x00
> > +#define MMA9551_MCI_ERROR_PARAM 0x04
> > +#define MMA9551_MCI_INVALID_COUNT 0x19
> > +#define MMA9551_MCI_ERROR_COMMAND 0x1C
> > +#define MMA9551_MCI_ERROR_INVALID_LENGTH 0x21
> > +#define MMA9551_MCI_ERROR_FIFO_BUSY 0x22
> > +#define MMA9551_MCI_ERROR_FIFO_ALLOCATED 0x23
> > +#define MMA9551_MCI_ERROR_FIFO_OVERSIZE 0x24
> > +
> > +/* GPIO Application */
> > +#define MMA9551_GPIO_POL_MSB 0x08
> > +#define MMA9551_GPIO_POL_LSB 0x09
> > +
> > +/* Sleep/Wake application */
> > +#define MMA9551_SLEEP_CFG 0x06
> > +#define MMA9551_SLEEP_CFG_SNCEN BIT(0)
> > +#define MMA9551_SLEEP_CFG_FLEEN BIT(1)
> > +#define MMA9551_SLEEP_CFG_SCHEN BIT(2)
> > +
> > +/* AFE application */
> > +#define MMA9551_AFE_X_ACCEL_REG 0x00
> > +#define MMA9551_AFE_Y_ACCEL_REG 0x02
> > +#define MMA9551_AFE_Z_ACCEL_REG 0x04
> > +
> > +/*
> > + * A response is composed of:
> > + * - control registers: MB0-3
> > + * - data registers: MB4-31
> > + *
> > + * A request is composed of:
> > + * - mbox to write to (always 0)
> > + * - control registers: MB1-4
> > + * - data registers: MB5-31
> > + */
> > +#define MMA9551_MAILBOX_CTRL_REGS 4
> > +#define MMA9551_MAX_MAILBOX_DATA_REGS 28
> > +#define MMA9551_MAILBOX_REGS 32
> > +
> > +#define MMA9551_I2C_READ_RETRIES 5
> > +#define MMA9551_I2C_READ_DELAY 50 /* us */
> > +
> > +struct mma9551_mbox_request {
> > + u8 start_mbox; /* Always 0. */
> > + u8 app_id;
> > + /*
> > + * See Section 5.3.1 of the MMA955xL Software Reference Manual.
> > + *
> > + * Bit 7: reserved, always 0
> > + * Bits 6-4: command
> > + * Bits 3-0: upper bits of register offset
> > + */
> > + u8 cmd_off;
> > + u8 lower_off;
> > + u8 nbytes;
> > + u8 buf[MMA9551_MAX_MAILBOX_DATA_REGS - 1];
> > +} __packed;
> > +
> > +struct mma9551_mbox_response {
> > + u8 app_id;
> > + /*
> > + * See Section 5.3.3 of the MMA955xL Software Reference Manual.
> > + *
> > + * Bit 7: COCO
> > + * Bits 6-0: Error code.
> > + */
> > + u8 coco_err;
> > + u8 nbytes;
> > + u8 req_bytes;
> > + u8 buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +} __packed;
> > +
> > +static int mma9551_transfer(struct i2c_client *client,
> > + u8 app_id, u8 command, u16 offset,
> > + u8 *inbytes, int num_inbytes,
> > + u8 *outbytes, int num_outbytes)
> > +{
> > + struct mma9551_mbox_request req;
> > + struct mma9551_mbox_response rsp;
> > + struct i2c_msg in, out;
> > + u8 req_len, err_code;
> > + int ret, retries;
> > +
> > + if (offset >= 1 << 12) {
> > + dev_err(&client->dev, "register offset too large\n");
> > + return -EINVAL;
> > + }
> > +
> > + req_len = 1 + MMA9551_MAILBOX_CTRL_REGS + num_inbytes;
> > + req.start_mbox = 0;
> > + req.app_id = app_id;
> > + req.cmd_off = command | (offset >> 8);
> > + req.lower_off = offset;
> > +
> > + if (command == MMA9551_CMD_WRITE_CONFIG)
> > + req.nbytes = num_inbytes;
> > + else
> > + req.nbytes = num_outbytes;
> > + if (num_inbytes)
> > + memcpy(req.buf, inbytes, num_inbytes);
> > +
> > + out.addr = client->addr;
> > + out.flags = 0;
> > + out.len = req_len;
> > + out.buf = (u8 *)&req;
> > +
> > + ret = i2c_transfer(client->adapter, &out, 1);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "i2c write failed\n");
> > + return ret;
> > + }
> > +
> > + retries = MMA9551_I2C_READ_RETRIES;
> > + do {
> > + udelay(MMA9551_I2C_READ_DELAY);
> > +
> > + in.addr = client->addr;
> > + in.flags = I2C_M_RD;
> > + in.len = sizeof(rsp);
> > + in.buf = (u8 *)&rsp;
> > +
> > + ret = i2c_transfer(client->adapter, &in, 1);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "i2c read failed\n");
> > + return ret;
> > + }
> > +
> > + if (rsp.coco_err & MMA9551_RESPONSE_COCO)
> > + break;
> > + } while (--retries > 0);
> > +
> > + if (retries == 0) {
> > + dev_err(&client->dev,
> > + "timed out while waiting for command response\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + if (rsp.app_id != app_id) {
> > + dev_err(&client->dev,
> > + "app_id mismatch in response got %02x expected %02x\n",
> > + rsp.app_id, app_id);
> > + return -EINVAL;
> > + }
> > +
> > + err_code = rsp.coco_err & ~MMA9551_RESPONSE_COCO;
> > + if (err_code != MMA9551_MCI_ERROR_NONE) {
> > + dev_err(&client->dev, "read returned error %x\n", err_code);
> > + return -EINVAL;
> > + }
> > +
> > + if (rsp.nbytes != rsp.req_bytes) {
> > + dev_err(&client->dev,
> > + "output length mismatch got %d expected %d\n",
> > + rsp.nbytes, rsp.req_bytes);
> > + return -EINVAL;
> > + }
> > +
> > + if (num_outbytes)
> > + memcpy(outbytes, rsp.buf, num_outbytes);
> > +
> > + return 0;
> > +}
> > +
> > +int mma9551_read_config_byte(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 *val)
> > +{
> > + return mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > + reg, NULL, 0, val, 1);
> > +}
> > +EXPORT_SYMBOL(mma9551_read_config_byte);
> > +
> > +int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 val)
> > +{
> > + return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG, reg,
> > + &val, 1, NULL, 0);
> > +}
> > +EXPORT_SYMBOL(mma9551_write_config_byte);
> > +
> > +int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 *val)
> > +{
> > + return mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > + reg, NULL, 0, val, 1);
> > +}
> > +EXPORT_SYMBOL(mma9551_read_status_byte);
> > +
> > +int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> > + u16 reg, u16 *val)
> > +{
> > + int ret;
> > + __be16 v;
> > +
> > + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > + reg, NULL, 0, (u8 *)&v, 2);
> > + *val = be16_to_cpu(v);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_status_word);
> > +
> I wonder if it's worth adding documentation to these functions - to point
> out that they should only be used under a lock. Might get forgotten as
> it's not readily apparent from the naming that locking is done externally
> rather than internally.
>
Good point. Will add some kernel-doc comments to these functions.
> > +int mma9551_update_config_bits(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 mask, u8 val)
> > +{
> > + int ret;
> > + u8 tmp, orig;
> > +
> > + ret = mma9551_read_config_byte(client, app_id, reg, &orig);
> > + if (ret < 0)
> > + return ret;
> > +
> > + tmp = orig & ~mask;
> > + tmp |= val & mask;
> > +
> > + if (tmp == orig)
> > + return 0;
> > +
> > + return mma9551_write_config_byte(client, app_id, reg, tmp);
> > +}
> > +EXPORT_SYMBOL(mma9551_update_config_bits);
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
