Date:	Sat, 5 Sep 2015 18:05:49 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Martin Kepplinger <martink@...teo.de>, knaack.h@....de,
	lars@...afoo.de, pmeerw@...erw.net, mfuzzey@...keon.com,
	roberta.dobrescu@...il.com, robh+dt@...nel.org, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org
Cc:	devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	christoph.muellner@...obroma-systems.com,
	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
Subject: Re: [PATCH 4/6] iio: mma8452: add support for MMA8652FC and MMA8653FC

On 01/09/15 12:45, Martin Kepplinger wrote:
> MMA8652FC and MMA8653FC don't provide the transient interrupt source, so
> the motion interrupt source is used by providing a new iio_chan_spec
> definition, so that other supported devices are not affected by this.
> 
> Datasheets for the newly supported devices are available at Freescale's
> website:
> 
> http://cache.freescale.com/files/sensors/doc/data_sheet/MMA8652FC.pdf
> http://cache.freescale.com/files/sensors/doc/data_sheet/MMA8653FC.pdf
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@...obroma-systems.com>
One comment inline, but I'm arguing in favour of the approach you have
taken so don't worry about that!

Another nice clean focused patch.

(note I'm marking all of them as to be applied in my email archive so
if no one else chips in they'll probably get picked up next weekend.)

Jonathan
> ---
>  .../devicetree/bindings/iio/accel/mma8452.txt      |  4 +-
>  drivers/iio/accel/Kconfig                          |  2 +-
>  drivers/iio/accel/mma8452.c                        | 98 ++++++++++++++++++++--
>  3 files changed, 95 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/mma8452.txt b/Documentation/devicetree/bindings/iio/accel/mma8452.txt
> index c3bc272..e3c3746 100644
> --- a/Documentation/devicetree/bindings/iio/accel/mma8452.txt
> +++ b/Documentation/devicetree/bindings/iio/accel/mma8452.txt
> @@ -1,10 +1,12 @@
> -Freescale MMA8452Q or MMA8453Q triaxial accelerometer
> +Freescale MMA8452Q, MMA8453Q, MMA8652FC or MMA8653FC triaxial accelerometer
>  
>  Required properties:
>  
>    - compatible: should contain one of
>      * "fsl,mma8452"
>      * "fsl,mma8453"
> +    * "fsl,mma8652"
> +    * "fsl,mma8653"
>    - reg: the I2C address of the chip
>  
>  Optional properties:
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index e4075a0..a37155d 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -106,7 +106,7 @@ config MMA8452
>  	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for the following Freescale 3-axis
> -	  accelerometers: MMA8452Q, MMA8453Q.
> +	  accelerometers: MMA8452Q, MMA8453Q, MMA8652FC, MMA8653FC.
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mma8452.
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 6b1a862..59b4455 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -3,6 +3,8 @@
>   *
>   * MMA8452Q (12 bit)
>   * MMA8453Q (10 bit)
> + * MMA8652FC (12 bit)
> + * MMA8653FC (10 bit)
>   *
>   * Copyright 2014 Peter Meerwald <pmeerw@...erw.net>
>   *
> @@ -84,6 +86,8 @@
>  
>  #define  MMA8452_DEVICE_ID			0x2a
>  #define  MMA8453_DEVICE_ID			0x3a
> +#define MMA8652_DEVICE_ID			0x4a
> +#define MMA8653_DEVICE_ID			0x5a
>  
>  struct mma8452_data {
>  	struct i2c_client *client;
> @@ -791,6 +795,26 @@ static struct attribute_group mma8452_event_attribute_group = {
>  	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>  }
>  
> +#define MMA8652_CHANNEL(axis, idx, bits) { \
> +	.type = IIO_ACCEL, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_##axis, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +		BIT(IIO_CHAN_INFO_CALIBBIAS), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +		BIT(IIO_CHAN_INFO_SCALE), \
> +	.scan_index = idx, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = (bits), \
> +		.storagebits = 16, \
> +		.shift = 16 - (bits), \
> +		.endianness = IIO_BE, \
> +	}, \
> +	.event_spec = mma8452_motion_event, \
I suppose you could add another macro def that does everything other than
the event_spec. However I'd argue that it is better to keep it clear like
you have done here.
> +	.num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
> +}
> +
>  static const struct iio_chan_spec mma8452_channels[] = {
>  	MMA8452_CHANNEL(X, 0, 12),
>  	MMA8452_CHANNEL(Y, 1, 12),
> @@ -805,9 +829,25 @@ static const struct iio_chan_spec mma8453_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> +static const struct iio_chan_spec mma8652_channels[] = {
> +	MMA8652_CHANNEL(X, 0, 12),
> +	MMA8652_CHANNEL(Y, 1, 12),
> +	MMA8652_CHANNEL(Z, 2, 12),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static const struct iio_chan_spec mma8653_channels[] = {
> +	MMA8652_CHANNEL(X, 0, 10),
> +	MMA8652_CHANNEL(Y, 1, 10),
> +	MMA8652_CHANNEL(Z, 2, 10),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
>  enum {
>  	mma8452,
>  	mma8453,
> +	mma8652,
> +	mma8653,
>  };
>  
>  static const struct mma_chip_info mma_chip_info_table[] = {
> @@ -850,6 +890,38 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
>  		.ev_count = MMA8452_TRANSIENT_COUNT,
>  	},
> +	[mma8652] = {
> +		.chip_id = MMA8652_DEVICE_ID,
> +		.channels = mma8652_channels,
> +		.num_channels = ARRAY_SIZE(mma8652_channels),
> +		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
> +		.ev_cfg = MMA8452_FF_MT_CFG,
> +		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> +		.ev_cfg_chan_shift = 3,
> +		.ev_src = MMA8452_FF_MT_SRC,
> +		.ev_src_xe = MMA8452_FF_MT_SRC_XHE,
> +		.ev_src_ye = MMA8452_FF_MT_SRC_YHE,
> +		.ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
> +		.ev_ths = MMA8452_FF_MT_THS,
> +		.ev_ths_mask = MMA8452_FF_MT_THS_MASK,
> +		.ev_count = MMA8452_FF_MT_COUNT,
> +	},
> +	[mma8653] = {
> +		.chip_id = MMA8653_DEVICE_ID,
> +		.channels = mma8653_channels,
> +		.num_channels = ARRAY_SIZE(mma8653_channels),
> +		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
> +		.ev_cfg = MMA8452_FF_MT_CFG,
> +		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> +		.ev_cfg_chan_shift = 3,
> +		.ev_src = MMA8452_FF_MT_SRC,
> +		.ev_src_xe = MMA8452_FF_MT_SRC_XHE,
> +		.ev_src_ye = MMA8452_FF_MT_SRC_YHE,
> +		.ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
> +		.ev_ths = MMA8452_FF_MT_THS,
> +		.ev_ths_mask = MMA8452_FF_MT_THS_MASK,
> +		.ev_count = MMA8452_FF_MT_COUNT,
> +	},
>  };
>  
>  static struct attribute *mma8452_attributes[] = {
> @@ -972,6 +1044,8 @@ static int mma8452_reset(struct i2c_client *client)
>  static const struct of_device_id mma8452_dt_ids[] = {
>  	{ .compatible = "fsl,mma8452", .data = &mma_chip_info_table[mma8452] },
>  	{ .compatible = "fsl,mma8453", .data = &mma_chip_info_table[mma8453] },
> +	{ .compatible = "fsl,mma8652", .data = &mma_chip_info_table[mma8652] },
> +	{ .compatible = "fsl,mma8653", .data = &mma_chip_info_table[mma8653] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, mma8452_dt_ids);
> @@ -984,13 +1058,6 @@ static int mma8452_probe(struct i2c_client *client,
>  	int ret;
>  	const struct of_device_id *match;
>  
> -	ret = i2c_smbus_read_byte_data(client, MMA8452_WHO_AM_I);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (ret != MMA8452_DEVICE_ID && ret != MMA8453_DEVICE_ID)
> -		return -ENODEV;
> -
>  	match = of_match_device(mma8452_dt_ids, &client->dev);
>  	if (!match) {
>  		dev_err(&client->dev, "unknown device model\n");
> @@ -1006,6 +1073,21 @@ static int mma8452_probe(struct i2c_client *client,
>  	mutex_init(&data->lock);
>  	data->chip_info = match->data;
>  
> +	ret = i2c_smbus_read_byte_data(client, MMA8452_WHO_AM_I);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case MMA8452_DEVICE_ID:
> +	case MMA8453_DEVICE_ID:
> +	case MMA8652_DEVICE_ID:
> +	case MMA8653_DEVICE_ID:
> +		if (ret == data->chip_info->chip_id)
> +			break;
> +	default:
> +		return -ENODEV;
> +	}
> +
>  	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
>  		 match->compatible, data->chip_info->chip_id);
>  
> @@ -1138,6 +1220,8 @@ static SIMPLE_DEV_PM_OPS(mma8452_pm_ops, mma8452_suspend, mma8452_resume);
>  static const struct i2c_device_id mma8452_id[] = {
>  	{ "mma8452", mma8452 },
>  	{ "mma8453", mma8453 },
> +	{ "mma8652", mma8652 },
> +	{ "mma8653", mma8653 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, mma8452_id);
> 

--
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