Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753819AbbHSMJx (ORCPT ); Wed, 19 Aug 2015 08:09:53 -0400 Received: from mga02.intel.com ([134.134.136.20]:48434 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753352AbbHSMJn (ORCPT ); Wed, 19 Aug 2015 08:09:43 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,709,1432623600"; d="scan'208";a="628334071" Date: Wed, 19 Aug 2015 15:09:28 +0300 From: Teodora Baluta To: Jonathan Cameron Cc: knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, daniel.baluta@intel.com, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/3] iio: accel: add support for mxc4005 accelerometer Message-ID: <20150819120927.GA27721@hard-bop> References: <1439487101-13085-1-git-send-email-teodora.baluta@intel.com> <1439487101-13085-2-git-send-email-teodora.baluta@intel.com> <55D03C6A.9020006@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55D03C6A.9020006@kernel.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13593 Lines: 468 On Sun, Aug 16, 2015 at 08:31:54AM +0100, Jonathan Cameron wrote: > On 13/08/15 18:31, Teodora Baluta wrote: > > This patch adds support for Memsic MXC4005XC 3-axis accelerometer. The > > current implementation is a minimal one as it adds raw readings for the > > three axes and setting scale from userspace. > > > > Signed-off-by: Teodora Baluta > Looking pretty good. Some real nitpicks and minor suggestions > inline. I was going to leave this on the list anyway to give > Peter and others time for another look so thought I might as > well be fussy. What can I say. It's Sunday morning and the > coffee hasn't kicked in yet :) Thanks, it's very nice to get some more feedback. :) I have missed a couple of things and I'll send another patch set asap. > > Jonathan > > --- > > drivers/iio/accel/Kconfig | 11 ++ > > drivers/iio/accel/Makefile | 2 + > > drivers/iio/accel/mxc4005.c | 358 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 371 insertions(+) > > create mode 100644 drivers/iio/accel/mxc4005.c > > > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > > index a59047d..69302be 100644 > > --- a/drivers/iio/accel/Kconfig > > +++ b/drivers/iio/accel/Kconfig > > @@ -137,6 +137,17 @@ config MMA9553 > > To compile this driver as a module, choose M here: the module > > will be called mma9553. > > > > +config MXC4005 > > + tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + Say yes here to build support for the Memsic MXC4005XC 3-axis > > + accelerometer. > > + > > + To compile this driver as a module, choose M. The module will be > > + called mxc4005. > > + > > config STK8312 > > tristate "Sensortek STK8312 3-Axis Accelerometer Driver" > > depends on I2C > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile > > index ebd2675..020dda0 100644 > > --- a/drivers/iio/accel/Makefile > > +++ b/drivers/iio/accel/Makefile > > @@ -14,6 +14,8 @@ obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o > > obj-$(CONFIG_MMA9551) += mma9551.o > > obj-$(CONFIG_MMA9553) += mma9553.o > > > > +obj-$(CONFIG_MXC4005) += mxc4005.o > > + > > obj-$(CONFIG_STK8312) += stk8312.o > > obj-$(CONFIG_STK8BA50) += stk8ba50.o > > > > diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c > > new file mode 100644 > > index 0000000..76a7954 > > --- /dev/null > > +++ b/drivers/iio/accel/mxc4005.c > > @@ -0,0 +1,358 @@ > > +/* > > + * 3-axis accelerometer driver for MXC4005XC Memsic sensor > > + * > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define MXC4005_DRV_NAME "mxc4005" > > +#define MXC4005_REGMAP_NAME "mxc4005_regmap" > You don't actually use the above... Guessing you did mean > to but used drv_name instead in the regmap configuration. That's right. :( > > + > > +#define MXC4005_REG_XOUT_UPPER 0x03 > > +#define MXC4005_REG_XOUT_LOWER 0x04 > > +#define MXC4005_REG_YOUT_UPPER 0x05 > > +#define MXC4005_REG_YOUT_LOWER 0x06 > > +#define MXC4005_REG_ZOUT_UPPER 0x07 > > +#define MXC4005_REG_ZOUT_LOWER 0x08 > > + > > +#define MXC4005_REG_CONTROL 0x0D > > +#define MXC4005_REG_CONTROL_MASK_FSR GENMASK(6, 5) > > +#define MXC4005_CONTROL_FSR_SHIFT 5 > > + > > +#define MXC4005_REG_DEVICE_ID 0x0E > > + > > +enum mxc4005_axis { > > + AXIS_X, > > + AXIS_Y, > > + AXIS_Z, > > +}; > > + > > +enum mxc4005_range { > > + MXC4005_RANGE_2G, > > + MXC4005_RANGE_4G, > > + MXC4005_RANGE_8G, > > +}; > > + > > +struct mxc4005_data { > > + struct i2c_client *client; > In a regmap cleanup series someone posted the other day, > they just put struct device *dev in here instead and > assigned data.dev = client->dev; > > Result is slightly cleaner code. Worth doing here? > Worth a change for this since I am always doing client->dev. > > + struct mutex mutex; > > + struct regmap *regmap; > > +}; > > + > > +/* > > + * MXC4005 can operate in the following ranges: > > + * +/- 2G, 4G, 8G (the default +/-2G) > > + * > > + * (2 + 2) * 9.81 / (2^12 - 1) = 0.009582 > > + * (4 + 4) * 9.81 / (2^12 - 1) = 0.019164 > > + * (8 + 8) * 9.81 / (2^12 - 1) = 0.038329 > > + */ > > +static const struct { > > + u8 range; > > + int scale; > > +} mxc4005_scale_table[] = { > > + {MXC4005_RANGE_2G, 9582}, > > + {MXC4005_RANGE_4G, 19164}, > > + {MXC4005_RANGE_8G, 38329}, > > +}; > > + > > + > > +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019164 0.038329"); > > + > > +static struct attribute *mxc4005_attributes[] = { > > + &iio_const_attr_in_accel_scale_available.dev_attr.attr, > > + NULL, > > +}; > > + > > +static const struct attribute_group mxc4005_attrs_group = { > > + .attrs = mxc4005_attributes, > > +}; > > + > > +static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case MXC4005_REG_XOUT_UPPER: > > + case MXC4005_REG_XOUT_LOWER: > > + case MXC4005_REG_YOUT_UPPER: > > + case MXC4005_REG_YOUT_LOWER: > > + case MXC4005_REG_ZOUT_UPPER: > > + case MXC4005_REG_ZOUT_LOWER: > > + case MXC4005_REG_DEVICE_ID: > > + case MXC4005_REG_CONTROL: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > +static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case MXC4005_REG_CONTROL: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > +static const struct regmap_config mxc4005_regmap_config = { > > + .name = MXC4005_DRV_NAME, > > + > > + .reg_bits = 8, > > + .val_bits = 8, > > + > > + .max_register = MXC4005_REG_DEVICE_ID, > > + .cache_type = REGCACHE_NONE, > REGCACHE_NONE is the default. No need to specify it here. > (enum value of 0) Ok. Will remove this. > > + > > + .readable_reg = mxc4005_is_readable_reg, > > + .writeable_reg = mxc4005_is_writeable_reg, > > +}; > > + > > +static int mxc4005_read_axis(struct mxc4005_data *data, > > + unsigned int addr) > > +{ > > + __be16 reg; > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, addr, (u8 *) ®, sizeof(reg)); > > + if (ret < 0) { > > + dev_err(&data->client->dev, "failed to read reg %02x\n", addr); > > + return ret; > > + } > > + > > + return be16_to_cpu(reg); > > +} > > + > > +static int mxc4005_read_scale(struct mxc4005_data *data) > > +{ > > + unsigned int reg; > > + int ret; > > + int i; > > + > > + ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, ®); > > + if (ret < 0) { > > + dev_err(&data->client->dev, "failed to read reg_control\n"); > > + return ret; > > + } > > + > > + i = reg >> MXC4005_CONTROL_FSR_SHIFT; > > + > > + if (i < 0 || i >= ARRAY_SIZE(mxc4005_scale_table)) > > + return -EINVAL; > > + > > + return mxc4005_scale_table[i].scale; > > +} > > + > > +static int mxc4005_set_scale(struct mxc4005_data *data, int val) > > +{ > > + unsigned int reg; > > + int i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(mxc4005_scale_table); i++) { > > + if (mxc4005_scale_table[i].scale == val) { > > + reg = i << MXC4005_CONTROL_FSR_SHIFT; > > + ret = regmap_update_bits(data->regmap, > > + MXC4005_REG_CONTROL, > > + MXC4005_REG_CONTROL_MASK_FSR, > > + reg); > > + if (ret < 0) > > + dev_err(&data->client->dev, > > + "failed to write reg_control\n"); > > + return ret; > > + } > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int mxc4005_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct mxc4005_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + switch (chan->type) { > > + case IIO_ACCEL: > > + if (iio_buffer_enabled(indio_dev)) > > + return -EBUSY; > > + > > + ret = mxc4005_read_axis(data, chan->address); > > + if (ret < 0) > > + return ret; > > + *val = sign_extend32(ret >> 4, 11); > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + case IIO_CHAN_INFO_SCALE: > > + ret = mxc4005_read_scale(data); > > + if (ret < 0) > > + return ret; > > + > > + *val = 0; > > + *val2 = ret; > > + return IIO_VAL_INT_PLUS_MICRO; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int mxc4005_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long mask) > > +{ > > + struct mxc4005_data *data = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_SCALE: > > + if (val != 0) > > + return -EINVAL; > > + > > + return mxc4005_set_scale(data, val2); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static const struct iio_info mxc4005_info = { > > + .driver_module = THIS_MODULE, > > + .read_raw = mxc4005_read_raw, > > + .write_raw = mxc4005_write_raw, > > + .attrs = &mxc4005_attrs_group, > > +}; > > + > > +#define MXC4005_CHANNEL(_axis, _addr) { \ > > + .type = IIO_ACCEL, \ > > + .modified = 1, \ > > + .channel2 = IIO_MOD_##_axis, \ > > + .address = _addr, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > +} > > + > > +static const struct iio_chan_spec mxc4005_channels[] = { > > + MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER), > > + MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER), > > + MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER), > > +}; > > + > > +static int mxc4005_chip_init(struct mxc4005_data *data) > > +{ > > + int ret; > > + unsigned int reg; > > + > > + ret = regmap_read(data->regmap, MXC4005_REG_DEVICE_ID, ®); > > + if (ret < 0) { > > + dev_err(&data->client->dev, "failed to read chip id\n"); > > + return ret; > > + } > > + > > + dev_dbg(&data->client->dev, "MXC4005 chip id %02x\n", reg); > > + > > + return 0; > > +} > > + > > +static int mxc4005_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct mxc4005_data *data; > > + struct iio_dev *indio_dev; > > + struct regmap *regmap; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + regmap = devm_regmap_init_i2c(client, &mxc4005_regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_err(&client->dev, "failed to initialize regmap\n"); > > + return PTR_ERR(regmap); > > + } > > + > > + data = iio_priv(indio_dev); > > + i2c_set_clientdata(client, indio_dev); > > + data->client = client; > > + data->regmap = regmap; > > + > > + ret = mxc4005_chip_init(data); > > + if (ret < 0) { > For consistency with previous error message (yeah I'm getting > picky ;) - initialize Yes, true. :) > > + dev_err(&client->dev, "failed to init chip\n"); > > + return ret; > > + } > > + > > + mutex_init(&data->mutex); > > + > > + indio_dev->dev.parent = &client->dev; > > + indio_dev->channels = mxc4005_channels; > > + indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels); > > + indio_dev->name = MXC4005_DRV_NAME; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->info = &mxc4005_info; > > + > > + ret = iio_device_register(indio_dev); > > + if (ret < 0) { > > + dev_err(&client->dev, > > + "unable to register iio device %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int mxc4005_remove(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > > + > Roll the line above and the one below into one. No point in the > separation. Ok. > > + iio_device_unregister(indio_dev); > > + > > + return 0; > > +} > > + > > +static const struct acpi_device_id mxc4005_acpi_match[] = { > > + {"MXC4005", 0}, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(acpi, mxc4005_acpi_match); > > + > > +static const struct i2c_device_id mxc4005_id[] = { > > + {"mxc4005", 0}, > > + { }, > > +}; > For consistency no blank line here. Ok. > > + > > +MODULE_DEVICE_TABLE(i2c, mxc4005_id); > > + > > +static struct i2c_driver mxc4005_driver = { > > + .driver = { > > + .name = MXC4005_DRV_NAME, > > + .acpi_match_table = ACPI_PTR(mxc4005_acpi_match), > > + }, > > + .probe = mxc4005_probe, > > + .remove = mxc4005_remove, > > + .id_table = mxc4005_id, > > +}; > > + > > +module_i2c_driver(mxc4005_driver); > > + > > +MODULE_AUTHOR("Teodora Baluta "); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("MXC4005 3-axis accelerometer driver"); > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/