by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 02/20] regmap: Remove regmap_bulk_write 64bit support

On Wed, Aug 12, 2015 at 12:12:27PM +0200, Markus Pargmann wrote:
> Regmap does not support 64bit. The ival that is used to write the 64bit
> data to, is unsigned int and can't hold 64bit. _regmap_write also just
> supports unsigend int.

What makes you say that unsigned int can't hold 64 bit? An architecture
can have 64 bit ints if it likes.

Attachments:

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 17/20] iio: bmc150: Use i2c regmap

On 12/08/15 11:12, Markus Pargmann wrote:
> This replaces all usage of direct i2c accesses with regmap accesses.
>
> Signed-off-by: Markus Pargmann <[email protected]>
Clearly there is some work needed on the earlier patches and this
might change as a result (particularly the fifo read). I'll review
as is however..

Few bits on top of what other reviews have highlighted...

Jonathan
> ---
> drivers/iio/accel/Kconfig | 2 +
> drivers/iio/accel/bmc150-accel.c | 225 +++++++++++++++++----------------------
> 2 files changed, 101 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 00e7bcbdbe24..01dd03d194d1 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -22,6 +22,8 @@ config BMC150_ACCEL
> depends on I2C
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> + select REGMAP
> + select REGMAP_I2C
> help
> Say yes here to build support for the following Bosch accelerometers:
> BMC150, BMI055, BMA250E, BMA222E, BMA255, BMA280.
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index fe2d2316158f..1484e956482e 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -35,6 +35,7 @@
> #include <linux/iio/trigger.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
> +#include <linux/regmap.h>
>
> #define BMC150_ACCEL_DRV_NAME "bmc150_accel"
> #define BMC150_ACCEL_IRQ_NAME "bmc150_accel_event"
> @@ -185,6 +186,8 @@ enum bmc150_accel_trigger_id {
>
> struct bmc150_accel_data {
> struct i2c_client *client;
> + struct regmap *regmap;
> + struct device *dev;
> struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> atomic_t active_intr;
> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
> @@ -241,6 +244,14 @@ static const struct {
> {500000, BMC150_ACCEL_SLEEP_500_MS},
> {1000000, BMC150_ACCEL_SLEEP_1_SEC} };
>
> +static const struct regmap_config bmc150_i2c_regmap_conf = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x3f,
> +
> + .use_single_rw = false,
> + .cache_type = REGCACHE_NONE,
> +};
>
> static int bmc150_accel_set_mode(struct bmc150_accel_data *data,
> enum bmc150_power_modes mode,
> @@ -270,8 +281,7 @@ static int bmc150_accel_set_mode(struct bmc150_accel_data *data,
>
> dev_dbg(&data->client->dev, "Set Mode bits %x\n", lpw_bits);
>
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_PMU_LPW, lpw_bits);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_PMU_LPW, lpw_bits);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_pmu_lpw\n");
> return ret;
> @@ -289,8 +299,7 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
> for (i = 0; i < ARRAY_SIZE(bmc150_accel_samp_freq_table); ++i) {
> if (bmc150_accel_samp_freq_table[i].val == val &&
> bmc150_accel_samp_freq_table[i].val2 == val2) {
> - ret = i2c_smbus_write_byte_data(
> - data->client,
> + ret = regmap_write(data->regmap,
> BMC150_ACCEL_REG_PMU_BW,
> bmc150_accel_samp_freq_table[i].bw_bits);
> if (ret < 0)
> @@ -307,26 +316,19 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
>
> static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
> {
> - int ret, val;
> + int ret;
>
> - ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_6,
> data->slope_thres);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_int_6\n");
> return ret;
> }
>
> - ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> + ret = regmap_update_bits(data->regmap, BMC150_ACCEL_REG_INT_5,
> + BMC150_ACCEL_SLOPE_DUR_MASK, data->slope_dur);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_int_5\n");
> - return ret;
> - }
> -
> - val = (ret & ~BMC150_ACCEL_SLOPE_DUR_MASK) | data->slope_dur;
> - ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
> - val);
> - if (ret < 0) {
> - dev_err(&data->client->dev, "Error write reg_int_5\n");
> + dev_err(&data->client->dev, "Error updating reg_int_5\n");
> return ret;
> }
>
> @@ -348,17 +350,18 @@ static int bmc150_accel_any_motion_setup(struct bmc150_accel_trigger *t,
> static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> {
> int ret;
> + unsigned int val;
>
> - ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_CHIP_ID);
> + ret = regmap_read(data->regmap, BMC150_ACCEL_REG_CHIP_ID, &val);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "Error: Reading chip id\n");
> return ret;
> }
>
> - dev_dbg(&data->client->dev, "Chip Id %x\n", ret);
> - if (ret != data->chip_info->chip_id) {
> - dev_err(&data->client->dev, "Invalid chip %x\n", ret);
> + dev_dbg(&data->client->dev, "Chip Id %x\n", val);
> + if (val != data->chip_info->chip_id) {
> + dev_err(&data->client->dev, "Invalid chip %x\n", val);
> return -ENODEV;
> }
>
> @@ -372,9 +375,8 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> return ret;
>
> /* Set Default Range */
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_PMU_RANGE,
> - BMC150_ACCEL_DEF_RANGE_4G);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_PMU_RANGE,
> + BMC150_ACCEL_DEF_RANGE_4G);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "Error writing reg_pmu_range\n");
> @@ -391,10 +393,9 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> return ret;
>
> /* Set default as latched interrupts */
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_INT_RST_LATCH,
> - BMC150_ACCEL_INT_MODE_LATCH_INT |
> - BMC150_ACCEL_INT_MODE_LATCH_RESET);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_RST_LATCH,
> + BMC150_ACCEL_INT_MODE_LATCH_INT |
> + BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "Error writing reg_int_rst_latch\n");
> @@ -527,38 +528,18 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data, int i,
> return ret;
>
> /* map the interrupt to the appropriate pins */
> - ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
> - if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_int_map\n");
> - goto out_fix_power_state;
> - }
> - if (state)
> - ret |= info->map_bitmask;
> - else
> - ret &= ~info->map_bitmask;
> -
> - ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
> - ret);
> + ret = regmap_update_bits(data->regmap, info->map_reg, info->map_bitmask,
> + (state ? info->map_bitmask : 0));
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_int_map\n");
> + dev_err(&data->client->dev, "Error updating reg_int_map\n");
> goto out_fix_power_state;
> }
>
> /* enable/disable the interrupt */
> - ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
> - if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_int_en\n");
> - goto out_fix_power_state;
> - }
> -
> - if (state)
> - ret |= info->en_bitmask;
> - else
> - ret &= ~info->en_bitmask;
> -
> - ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
> + ret = regmap_update_bits(data->regmap, info->en_reg, info->en_bitmask,
> + (state ? info->en_bitmask : 0));
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_int_en\n");
> + dev_err(&data->client->dev, "Error updating reg_int_en\n");
> goto out_fix_power_state;
> }
>
> @@ -581,8 +562,7 @@ static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>
> for (i = 0; i < ARRAY_SIZE(data->chip_info->scale_table); ++i) {
> if (data->chip_info->scale_table[i].scale == val) {
> - ret = i2c_smbus_write_byte_data(
> - data->client,
> + ret = regmap_write(data->regmap,
> BMC150_ACCEL_REG_PMU_RANGE,
> data->chip_info->scale_table[i].reg_range);
> if (ret < 0) {
> @@ -602,16 +582,17 @@ static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
> static int bmc150_accel_get_temp(struct bmc150_accel_data *data, int *val)
> {
> int ret;
> + unsigned int value;
>
> mutex_lock(&data->mutex);
>
> - ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_TEMP);
> + ret = regmap_read(data->regmap, BMC150_ACCEL_REG_TEMP, &value);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_temp\n");
> mutex_unlock(&data->mutex);
> return ret;
> }
> - *val = sign_extend32(ret, 7);
> + *val = sign_extend32(value, 7);
>
> mutex_unlock(&data->mutex);
>
> @@ -624,6 +605,7 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
> {
> int ret;
> int axis = chan->scan_index;
> + unsigned int raw_val;
>
> mutex_lock(&data->mutex);
> ret = bmc150_accel_set_power_state(data, true);
> @@ -632,15 +614,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
> return ret;
> }
>
> - ret = i2c_smbus_read_word_data(data->client,
> - BMC150_ACCEL_AXIS_TO_REG(axis));
> + ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
> + &raw_val, 2);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading axis %d\n", axis);
> bmc150_accel_set_power_state(data, false);
> mutex_unlock(&data->mutex);
> return ret;
> }
> - *val = sign_extend32(ret >> chan->scan_type.shift,
> + *val = sign_extend32(raw_val >> chan->scan_type.shift,
> chan->scan_type.realbits - 1);
> ret = bmc150_accel_set_power_state(data, false);
> mutex_unlock(&data->mutex);
> @@ -904,52 +886,37 @@ static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
> * We must read at least one full frame in one burst, otherwise the rest of the
> * frame data is discarded.
> */
> -static int bmc150_accel_fifo_transfer(const struct i2c_client *client,
> +static int bmc150_accel_fifo_transfer(struct bmc150_accel_data *data,
> char *buffer, int samples)
> {
> int sample_length = 3 * 2;
> - u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
> - int ret = -EIO;
> -
> - if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> - struct i2c_msg msg[2] = {
> - {
> - .addr = client->addr,
> - .flags = 0,
> - .buf = &reg_fifo_data,
> - .len = sizeof(reg_fifo_data),
> - },
> - {
> - .addr = client->addr,
> - .flags = I2C_M_RD,
> - .buf = (u8 *)buffer,
> - .len = samples * sample_length,
> - }
> - };
> + int ret;
> + int total_length = samples * sample_length;
> + int i, step;
>
> - ret = i2c_transfer(client->adapter, msg, 2);
> - if (ret != 2)
> - ret = -EIO;
> - else
> - ret = 0;
> - } else {
> - int i, step = I2C_SMBUS_BLOCK_MAX / sample_length;
> -
> - for (i = 0; i < samples * sample_length; i += step) {
> - ret = i2c_smbus_read_i2c_block_data(client,
> - reg_fifo_data, step,
> - &buffer[i]);
> - if (ret != step) {
> - ret = -EIO;
> - break;
> - }
> + ret = regmap_raw_read(data->regmap, BMC150_ACCEL_REG_FIFO_DATA, buffer,
> + total_length);
> + if (ret != -E2BIG) {
> + if (ret)
I'd invert the logic for more readability.

if (ret == -E2BIT) {
...
} else if (ret) {
...
} else {
return ret;
}
> + dev_err(data->dev, "Error transferring data from fifo\n");
> + return ret;
> + }
>
> - ret = 0;
> - }
> + /*
> + * Seems we have a bus with size limitation so we have to execute
> + * multiple reads
> + */
Can we not just query this in advance before going through the previous
failed call? THat would be cleaner to my mind.

> + step = regmap_get_raw_io_max(data->regmap) / sample_length;
> + for (i = -1; i < samples * sample_length; i += step) {
> + ret = regmap_raw_read(data->regmap, BMC150_ACCEL_REG_FIFO_DATA,
> + &buffer[i], step);
umm. Can't say I like the negative index into buffer. Why is it
necessary?
> + if (ret)
> + break;
> }
>
> if (ret)
> - dev_err(&client->dev, "Error transferring data from fifo\n");
> + dev_err(data->dev, "Error transferring data from fifo in single steps of %zu\n",

multiple steps of %zu perhaps?
> + step);
>
> return ret;
> }
> @@ -963,14 +930,15 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
> u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> int64_t tstamp;
> uint64_t sample_period;
> - ret = i2c_smbus_read_byte_data(data->client,
> - BMC150_ACCEL_REG_FIFO_STATUS);
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, BMC150_ACCEL_REG_FIFO_STATUS, &val);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_fifo_status\n");
> return ret;
> }
>
> - count = ret & 0x7F;
> + count = val & 0x7F;
>
> if (!count)
> return 0;
> @@ -1009,7 +977,7 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
> if (samples && count > samples)
> count = samples;
>
> - ret = bmc150_accel_fifo_transfer(data->client, (u8 *)buffer, count);
> + ret = bmc150_accel_fifo_transfer(data, (u8 *)buffer, count);
> if (ret)
> return ret;
>
> @@ -1206,17 +1174,19 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
> struct iio_dev *indio_dev = pf->indio_dev;
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> int bit, ret, i = 0;
> + unsigned int raw_val;
>
> mutex_lock(&data->mutex);
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> indio_dev->masklength) {
> - ret = i2c_smbus_read_word_data(data->client,
> - BMC150_ACCEL_AXIS_TO_REG(bit));
> + ret = regmap_bulk_read(data->regmap,
> + BMC150_ACCEL_AXIS_TO_REG(bit), &raw_val,
> + 2);
Is using a variable on the stack not going to cause issues when we add
SPI? (cacheline dma requirements).
> if (ret < 0) {
> mutex_unlock(&data->mutex);
> goto err_read;
> }
> - data->buffer[i++] = ret;
> + data->buffer[i++] = raw_val;
> }
> mutex_unlock(&data->mutex);
>
> @@ -1240,10 +1210,9 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
>
> mutex_lock(&data->mutex);
> /* clear any latched interrupt */
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_INT_RST_LATCH,
> - BMC150_ACCEL_INT_MODE_LATCH_INT |
> - BMC150_ACCEL_INT_MODE_LATCH_RESET);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_RST_LATCH,
> + BMC150_ACCEL_INT_MODE_LATCH_INT |
> + BMC150_ACCEL_INT_MODE_LATCH_RESET);
> mutex_unlock(&data->mutex);
> if (ret < 0) {
> dev_err(&data->client->dev,
> @@ -1300,34 +1269,34 @@ static int bmc150_accel_handle_roc_event(struct iio_dev *indio_dev)
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> int dir;
> int ret;
> + unsigned int val;
>
> - ret = i2c_smbus_read_byte_data(data->client,
> - BMC150_ACCEL_REG_INT_STATUS_2);
> + ret = regmap_read(data->regmap, BMC150_ACCEL_REG_INT_STATUS_2, &val);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_int_status_2\n");
> return ret;
> }
>
> - if (ret & BMC150_ACCEL_ANY_MOTION_BIT_SIGN)
> + if (val & BMC150_ACCEL_ANY_MOTION_BIT_SIGN)
> dir = IIO_EV_DIR_FALLING;
> else
> dir = IIO_EV_DIR_RISING;
>
> - if (ret & BMC150_ACCEL_ANY_MOTION_BIT_X)
> + if (val & BMC150_ACCEL_ANY_MOTION_BIT_X)
> iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> IIO_MOD_X,
> IIO_EV_TYPE_ROC,
> dir),
> data->timestamp);
> - if (ret & BMC150_ACCEL_ANY_MOTION_BIT_Y)
> + if (val & BMC150_ACCEL_ANY_MOTION_BIT_Y)
> iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> IIO_MOD_Y,
> IIO_EV_TYPE_ROC,
> dir),
> data->timestamp);
> - if (ret & BMC150_ACCEL_ANY_MOTION_BIT_Z)
> + if (val & BMC150_ACCEL_ANY_MOTION_BIT_Z)
> iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> IIO_MOD_Z,
> @@ -1360,10 +1329,9 @@ static irqreturn_t bmc150_accel_irq_thread_handler(int irq, void *private)
> }
>
> if (ack) {
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_INT_RST_LATCH,
> - BMC150_ACCEL_INT_MODE_LATCH_INT |
> - BMC150_ACCEL_INT_MODE_LATCH_RESET);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_RST_LATCH,
> + BMC150_ACCEL_INT_MODE_LATCH_INT |
> + BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret)
> dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> ret = IRQ_HANDLED;
> @@ -1516,7 +1484,7 @@ static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
> u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
> int ret;
>
> - ret = i2c_smbus_write_byte_data(data->client, reg, data->fifo_mode);
> + ret = regmap_write(data->regmap, reg, data->fifo_mode);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_fifo_config1\n");
> return ret;
> @@ -1525,9 +1493,8 @@ static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
> if (!data->fifo_mode)
> return 0;
>
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_FIFO_CONFIG0,
> - data->watermark);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_FIFO_CONFIG0,
> + data->watermark);
> if (ret < 0)
> dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
>
> @@ -1627,6 +1594,13 @@ static int bmc150_accel_probe(struct i2c_client *client,
> data = iio_priv(indio_dev);
> i2c_set_clientdata(client, indio_dev);
> data->client = client;
> + data->dev = &client->dev;
> +
> + data->regmap = devm_regmap_init_i2c(client, &bmc150_i2c_regmap_conf);
> + if (IS_ERR(data->regmap)) {
> + dev_err(&client->dev, "Failed to initialize i2c regmap\n");
> + return PTR_ERR(data->regmap);
> + }
>
> if (id) {
> name = id->name;
> @@ -1680,9 +1654,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
> * want to use latch mode when we can to prevent interrupt
> * flooding.
> */
> - ret = i2c_smbus_write_byte_data(data->client,
> - BMC150_ACCEL_REG_INT_RST_LATCH,
> - BMC150_ACCEL_INT_MODE_LATCH_RESET);
> + ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_RST_LATCH,
> + BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> goto err_buffer_cleanup;
>

2015-08-15 13:33:55

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 18/20] iio: bcm150: Remove i2c_client from private data

On 12/08/15 11:12, Markus Pargmann wrote:
> i2c_client struct is now only used for debugging output. We can use the
> device struct as well so we can remove all struct i2c_client usage.
>
> Signed-off-by: Markus Pargmann <[email protected]>
This one looks fine to me.

Acked-by: Jonathan Cameron <[email protected]>

Ideally these bmc150 patches should also get a look from Srinivas
before merging however (and testing given I'm guessing you don't have
all the parts!)

Jonathan
> ---
> drivers/iio/accel/bmc150-accel.c | 120 +++++++++++++++++++--------------------
> 1 file changed, 58 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 1484e956482e..d75e1b0aa7e9 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -185,9 +185,9 @@ enum bmc150_accel_trigger_id {
> };
>
> struct bmc150_accel_data {
> - struct i2c_client *client;
> struct regmap *regmap;
> struct device *dev;
> + int irq;
> struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> atomic_t active_intr;
> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
> @@ -279,11 +279,11 @@ static int bmc150_accel_set_mode(struct bmc150_accel_data *data,
> lpw_bits = mode << BMC150_ACCEL_PMU_MODE_SHIFT;
> lpw_bits |= (dur_val << BMC150_ACCEL_PMU_BIT_SLEEP_DUR_SHIFT);
>
> - dev_dbg(&data->client->dev, "Set Mode bits %x\n", lpw_bits);
> + dev_dbg(data->dev, "Set Mode bits %x\n", lpw_bits);
>
> ret = regmap_write(data->regmap, BMC150_ACCEL_REG_PMU_LPW, lpw_bits);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_pmu_lpw\n");
> + dev_err(data->dev, "Error writing reg_pmu_lpw\n");
> return ret;
> }
>
> @@ -321,18 +321,18 @@ static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
> ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_6,
> data->slope_thres);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_int_6\n");
> + dev_err(data->dev, "Error writing reg_int_6\n");
> return ret;
> }
>
> ret = regmap_update_bits(data->regmap, BMC150_ACCEL_REG_INT_5,
> BMC150_ACCEL_SLOPE_DUR_MASK, data->slope_dur);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error updating reg_int_5\n");
> + dev_err(data->dev, "Error updating reg_int_5\n");
> return ret;
> }
>
> - dev_dbg(&data->client->dev, "%s: %x %x\n", __func__, data->slope_thres,
> + dev_dbg(data->dev, "%s: %x %x\n", __func__, data->slope_thres,
> data->slope_dur);
>
> return ret;
> @@ -354,14 +354,14 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>
> ret = regmap_read(data->regmap, BMC150_ACCEL_REG_CHIP_ID, &val);
> if (ret < 0) {
> - dev_err(&data->client->dev,
> + dev_err(data->dev,
> "Error: Reading chip id\n");
> return ret;
> }
>
> - dev_dbg(&data->client->dev, "Chip Id %x\n", val);
> + dev_dbg(data->dev, "Chip Id %x\n", val);
> if (val != data->chip_info->chip_id) {
> - dev_err(&data->client->dev, "Invalid chip %x\n", val);
> + dev_err(data->dev, "Invalid chip %x\n", val);
> return -ENODEV;
> }
>
> @@ -378,8 +378,7 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> ret = regmap_write(data->regmap, BMC150_ACCEL_REG_PMU_RANGE,
> BMC150_ACCEL_DEF_RANGE_4G);
> if (ret < 0) {
> - dev_err(&data->client->dev,
> - "Error writing reg_pmu_range\n");
> + dev_err(data->dev, "Error writing reg_pmu_range\n");
> return ret;
> }
>
> @@ -397,7 +396,7 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> BMC150_ACCEL_INT_MODE_LATCH_INT |
> BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret < 0) {
> - dev_err(&data->client->dev,
> + dev_err(data->dev,
> "Error writing reg_int_rst_latch\n");
> return ret;
> }
> @@ -439,16 +438,16 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
> int ret;
>
> if (on)
> - ret = pm_runtime_get_sync(&data->client->dev);
> + ret = pm_runtime_get_sync(data->dev);
> else {
> - pm_runtime_mark_last_busy(&data->client->dev);
> - ret = pm_runtime_put_autosuspend(&data->client->dev);
> + pm_runtime_mark_last_busy(data->dev);
> + ret = pm_runtime_put_autosuspend(data->dev);
> }
> if (ret < 0) {
> - dev_err(&data->client->dev,
> + dev_err(data->dev,
> "Failed: bmc150_accel_set_power_state for %d\n", on);
> if (on)
> - pm_runtime_put_noidle(&data->client->dev);
> + pm_runtime_put_noidle(data->dev);
>
> return ret;
> }
> @@ -531,7 +530,7 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data, int i,
> ret = regmap_update_bits(data->regmap, info->map_reg, info->map_bitmask,
> (state ? info->map_bitmask : 0));
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error updating reg_int_map\n");
> + dev_err(data->dev, "Error updating reg_int_map\n");
> goto out_fix_power_state;
> }
>
> @@ -539,7 +538,7 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data, int i,
> ret = regmap_update_bits(data->regmap, info->en_reg, info->en_bitmask,
> (state ? info->en_bitmask : 0));
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error updating reg_int_en\n");
> + dev_err(data->dev, "Error updating reg_int_en\n");
> goto out_fix_power_state;
> }
>
> @@ -566,7 +565,7 @@ static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
> BMC150_ACCEL_REG_PMU_RANGE,
> data->chip_info->scale_table[i].reg_range);
> if (ret < 0) {
> - dev_err(&data->client->dev,
> + dev_err(data->dev,
> "Error writing pmu_range\n");
> return ret;
> }
> @@ -588,7 +587,7 @@ static int bmc150_accel_get_temp(struct bmc150_accel_data *data, int *val)
>
> ret = regmap_read(data->regmap, BMC150_ACCEL_REG_TEMP, &value);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_temp\n");
> + dev_err(data->dev, "Error reading reg_temp\n");
> mutex_unlock(&data->mutex);
> return ret;
> }
> @@ -617,7 +616,7 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
> ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
> &raw_val, 2);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading axis %d\n", axis);
> + dev_err(data->dev, "Error reading axis %d\n", axis);
> bmc150_accel_set_power_state(data, false);
> mutex_unlock(&data->mutex);
> return ret;
> @@ -934,7 +933,7 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
>
> ret = regmap_read(data->regmap, BMC150_ACCEL_REG_FIFO_STATUS, &val);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_fifo_status\n");
> + dev_err(data->dev, "Error reading reg_fifo_status\n");
> return ret;
> }
>
> @@ -1215,7 +1214,7 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
> BMC150_ACCEL_INT_MODE_LATCH_RESET);
> mutex_unlock(&data->mutex);
> if (ret < 0) {
> - dev_err(&data->client->dev,
> + dev_err(data->dev,
> "Error writing reg_int_rst_latch\n");
> return ret;
> }
> @@ -1273,7 +1272,7 @@ static int bmc150_accel_handle_roc_event(struct iio_dev *indio_dev)
>
> ret = regmap_read(data->regmap, BMC150_ACCEL_REG_INT_STATUS_2, &val);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error reading reg_int_status_2\n");
> + dev_err(data->dev, "Error reading reg_int_status_2\n");
> return ret;
> }
>
> @@ -1333,7 +1332,7 @@ static irqreturn_t bmc150_accel_irq_thread_handler(int irq, void *private)
> BMC150_ACCEL_INT_MODE_LATCH_INT |
> BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret)
> - dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> + dev_err(data->dev, "Error writing reg_int_rst_latch\n");
> ret = IRQ_HANDLED;
> } else {
> ret = IRQ_NONE;
> @@ -1385,17 +1384,13 @@ static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
> return dev_name(dev);
> }
>
> -static int bmc150_accel_gpio_probe(struct i2c_client *client,
> - struct bmc150_accel_data *data)
> +static int bmc150_accel_gpio_probe(struct bmc150_accel_data *data)
> {
> struct device *dev;
> struct gpio_desc *gpio;
> int ret;
>
> - if (!client)
> - return -EINVAL;
> -
> - dev = &client->dev;
> + dev = data->dev;
>
> /* data ready gpio interrupt pin */
> gpio = devm_gpiod_get_index(dev, BMC150_ACCEL_GPIO_NAME, 0, GPIOD_IN);
> @@ -1448,7 +1443,7 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> struct bmc150_accel_trigger *t = &data->triggers[i];
>
> - t->indio_trig = devm_iio_trigger_alloc(&data->client->dev,
> + t->indio_trig = devm_iio_trigger_alloc(data->dev,
> bmc150_accel_triggers[i].name,
> indio_dev->name,
> indio_dev->id);
> @@ -1457,7 +1452,7 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> break;
> }
>
> - t->indio_trig->dev.parent = &data->client->dev;
> + t->indio_trig->dev.parent = data->dev;
> t->indio_trig->ops = &bmc150_accel_trigger_ops;
> t->intr = bmc150_accel_triggers[i].intr;
> t->data = data;
> @@ -1486,7 +1481,7 @@ static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
>
> ret = regmap_write(data->regmap, reg, data->fifo_mode);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_fifo_config1\n");
> + dev_err(data->dev, "Error writing reg_fifo_config1\n");
> return ret;
> }
>
> @@ -1496,7 +1491,7 @@ static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
> ret = regmap_write(data->regmap, BMC150_ACCEL_REG_FIFO_CONFIG0,
> data->watermark);
> if (ret < 0)
> - dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
> + dev_err(data->dev, "Error writing reg_fifo_config0\n");
>
> return ret;
> }
> @@ -1586,6 +1581,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> int ret;
> const char *name = NULL;
> int chip_id = 0;
> + struct device *dev;
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (!indio_dev)
> @@ -1593,12 +1589,13 @@ static int bmc150_accel_probe(struct i2c_client *client,
>
> data = iio_priv(indio_dev);
> i2c_set_clientdata(client, indio_dev);
> - data->client = client;
> data->dev = &client->dev;
> + dev = &client->dev;
> + data->irq = client->irq;
>
> data->regmap = devm_regmap_init_i2c(client, &bmc150_i2c_regmap_conf);
> if (IS_ERR(data->regmap)) {
> - dev_err(&client->dev, "Failed to initialize i2c regmap\n");
> + dev_err(dev, "Failed to initialize i2c regmap\n");
> return PTR_ERR(data->regmap);
> }
>
> @@ -1607,8 +1604,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
> chip_id = id->driver_data;
> }
>
> - if (ACPI_HANDLE(&client->dev))
> - name = bmc150_accel_match_acpi_device(&client->dev, &chip_id);
> + if (ACPI_HANDLE(dev))
> + name = bmc150_accel_match_acpi_device(dev, &chip_id);
>
> data->chip_info = &bmc150_accel_chip_info_tbl[chip_id];
>
> @@ -1618,7 +1615,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>
> mutex_init(&data->mutex);
>
> - indio_dev->dev.parent = &client->dev;
> + indio_dev->dev.parent = dev;
> indio_dev->channels = data->chip_info->channels;
> indio_dev->num_channels = data->chip_info->num_channels;
> indio_dev->name = name;
> @@ -1630,16 +1627,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
> bmc150_accel_trigger_handler,
> &bmc150_accel_buffer_ops);
> if (ret < 0) {
> - dev_err(&client->dev, "Failed: iio triggered buffer setup\n");
> + dev_err(data->dev, "Failed: iio triggered buffer setup\n");
> return ret;
> }
>
> - if (client->irq <= 0)
> - client->irq = bmc150_accel_gpio_probe(client, data);
> + if (data->irq <= 0)
> + data->irq = bmc150_accel_gpio_probe(data);
>
> - if (client->irq > 0) {
> + if (data->irq > 0) {
> ret = devm_request_threaded_irq(
> - &client->dev, client->irq,
> + data->dev, data->irq,
> bmc150_accel_irq_handler,
> bmc150_accel_irq_thread_handler,
> IRQF_TRIGGER_RISING,
> @@ -1657,7 +1654,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> ret = regmap_write(data->regmap, BMC150_ACCEL_REG_INT_RST_LATCH,
> BMC150_ACCEL_INT_MODE_LATCH_RESET);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> + dev_err(data->dev, "Error writing reg_int_rst_latch\n");
> goto err_buffer_cleanup;
> }
>
> @@ -1678,18 +1675,17 @@ static int bmc150_accel_probe(struct i2c_client *client,
>
> ret = iio_device_register(indio_dev);
> if (ret < 0) {
> - dev_err(&client->dev, "Unable to register iio device\n");
> + dev_err(data->dev, "Unable to register iio device\n");
> goto err_trigger_unregister;
> }
>
> - ret = pm_runtime_set_active(&client->dev);
> + ret = pm_runtime_set_active(dev);
> if (ret)
> goto err_iio_unregister;
>
> - pm_runtime_enable(&client->dev);
> - pm_runtime_set_autosuspend_delay(&client->dev,
> - BMC150_AUTO_SUSPEND_DELAY_MS);
> - pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, BMC150_AUTO_SUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
>
> return 0;
>
> @@ -1708,9 +1704,9 @@ static int bmc150_accel_remove(struct i2c_client *client)
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
>
> - pm_runtime_disable(&client->dev);
> - pm_runtime_set_suspended(&client->dev);
> - pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(data->dev);
> + pm_runtime_set_suspended(data->dev);
> + pm_runtime_put_noidle(data->dev);
>
> iio_device_unregister(indio_dev);
>
> @@ -1728,7 +1724,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
> #ifdef CONFIG_PM_SLEEP
> static int bmc150_accel_suspend(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
>
> mutex_lock(&data->mutex);
> @@ -1740,7 +1736,7 @@ static int bmc150_accel_suspend(struct device *dev)
>
> static int bmc150_accel_resume(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
>
> mutex_lock(&data->mutex);
> @@ -1756,11 +1752,11 @@ static int bmc150_accel_resume(struct device *dev)
> #ifdef CONFIG_PM
> static int bmc150_accel_runtime_suspend(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> int ret;
>
> - dev_dbg(&data->client->dev, __func__);
> + dev_dbg(data->dev, __func__);
> ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_SUSPEND, 0);
> if (ret < 0)
> return -EAGAIN;
> @@ -1770,12 +1766,12 @@ static int bmc150_accel_runtime_suspend(struct device *dev)
>
> static int bmc150_accel_runtime_resume(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> int ret;
> int sleep_val;
>
> - dev_dbg(&data->client->dev, __func__);
> + dev_dbg(data->dev, __func__);
>
> ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
> if (ret < 0)
>

2015-08-15 13:41:33

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 19/20] iio: bmc150: Split the driver into core and i2c

On 12/08/15 11:12, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <[email protected]>
There are a few bits in here following through from earlier
patches that I haven't bothered mentioning as you'll have fixed
them already before this split patch).

Two more bits inline. Nothing major though.

Jonathan
> ---
> drivers/iio/accel/Kconfig | 22 +++--
> drivers/iio/accel/Makefile | 3 +-
> .../accel/{bmc150-accel.c => bmc150-accel-core.c} | 95 ++++---------------
> drivers/iio/accel/bmc150-accel-i2c.c | 101 +++++++++++++++++++++
> drivers/iio/accel/bmc150-accel.h | 21 +++++
> 5 files changed, 156 insertions(+), 86 deletions(-)
> rename drivers/iio/accel/{bmc150-accel.c => bmc150-accel-core.c} (95%)
> create mode 100644 drivers/iio/accel/bmc150-accel-i2c.c
> create mode 100644 drivers/iio/accel/bmc150-accel.h
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 01dd03d194d1..c63e981c38ff 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -18,22 +18,30 @@ config BMA180
> module will be called bma180.
>
> config BMC150_ACCEL
> - tristate "Bosch BMC150 Accelerometer Driver"
> - depends on I2C
> - select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> + bool "Bosch BMC150 Accelerometer Driver"
> select REGMAP
> - select REGMAP_I2C
> help
> Say yes here to build support for the following Bosch accelerometers:
> BMC150, BMI055, BMA250E, BMA222E, BMA255, BMA280.
>
> - Currently this only supports the device via an i2c interface.
> -
> This is a combo module with both accelerometer and magnetometer.
> This driver is only implementing accelerometer part, which has
> its own address and register map.
This approach does lead to a proliferation of complexity (to the
person configuring the kernel build). I prefer the approach the
ST accel driver for example takes, in which the SPI or I2C drivers
are built depending on whether the relevant dependencies are present
and the core driver is selected.
>
> +if BMC150_ACCEL
> +
> +config BMC150_ACCEL_I2C
> + tristate "I2C support"
> + depends on I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select REGMAP_I2C
> + help
> + Say yes here to build support for I2C communication with the
> + mentioned accelerometer.
> +
> +endif
> +
> config HID_SENSOR_ACCEL_3D
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index ebd2675b2a02..5ef8bdbad092 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -4,7 +4,8 @@
>
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_BMA180) += bma180.o
> -obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel.o
> +obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
> +obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
> obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
> obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
> obj-$(CONFIG_KXSD9) += kxsd9.o
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel-core.c
> similarity index 95%
> rename from drivers/iio/accel/bmc150-accel.c
> rename to drivers/iio/accel/bmc150-accel-core.c
> index d75e1b0aa7e9..8cf2786dd433 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -37,6 +37,8 @@
> #include <linux/iio/triggered_buffer.h>
> #include <linux/regmap.h>
>
> +#include "bmc150-accel.h"
> +
> #define BMC150_ACCEL_DRV_NAME "bmc150_accel"
> #define BMC150_ACCEL_IRQ_NAME "bmc150_accel_event"
> #define BMC150_ACCEL_GPIO_NAME "bmc150_accel_int"
> @@ -187,7 +189,6 @@ enum bmc150_accel_trigger_id {
> struct bmc150_accel_data {
> struct regmap *regmap;
> struct device *dev;
At least from how diff is listing it, looks like you've just
moved this down the struct? Why?
> - int irq;
> struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> atomic_t active_intr;
> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
> @@ -201,6 +202,7 @@ struct bmc150_accel_data {
> int ev_enable_state;
> int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
> const struct bmc150_accel_chip_info *chip_info;
> + int irq;
> };
>
> static const struct {
> @@ -1076,15 +1078,6 @@ static const struct iio_chan_spec bmc150_accel_channels[] =
> static const struct iio_chan_spec bma280_accel_channels[] =
> BMC150_ACCEL_CHANNELS(14);
>
> -enum {
> - bmc150,
> - bmi055,
> - bma255,
> - bma250e,
> - bma222e,
> - bma280,
> -};
> -
> static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> [bmc150] = {
> .chip_id = 0xFA,
> @@ -1573,36 +1566,22 @@ static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
> .postdisable = bmc150_accel_buffer_postdisable,
> };
>
> -static int bmc150_accel_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name, int chip_id, bool block_supported)
> {
> struct bmc150_accel_data *data;
> struct iio_dev *indio_dev;
> int ret;
> - const char *name = NULL;
> - int chip_id = 0;
> - struct device *dev;
>
> - indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> data = iio_priv(indio_dev);
> - i2c_set_clientdata(client, indio_dev);
> - data->dev = &client->dev;
> - dev = &client->dev;
> - data->irq = client->irq;
> -
> - data->regmap = devm_regmap_init_i2c(client, &bmc150_i2c_regmap_conf);
> - if (IS_ERR(data->regmap)) {
> - dev_err(dev, "Failed to initialize i2c regmap\n");
> - return PTR_ERR(data->regmap);
> - }
> -
> - if (id) {
> - name = id->name;
> - chip_id = id->driver_data;
> - }
> + dev_set_drvdata(dev, indio_dev);
> + data->dev = dev;
> + data->irq = irq;
> + data->regmap = regmap;
>
> if (ACPI_HANDLE(dev))
> name = bmc150_accel_match_acpi_device(dev, &chip_id);
> @@ -1664,9 +1643,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> if (ret)
> goto err_buffer_cleanup;
>
> - if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> - i2c_check_functionality(client->adapter,
> - I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> + if (block_supported) {
> indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> indio_dev->info = &bmc150_accel_info_fifo;
> indio_dev->buffer->attrs = bmc150_accel_fifo_attributes;
> @@ -1675,7 +1652,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>
> ret = iio_device_register(indio_dev);
> if (ret < 0) {
> - dev_err(data->dev, "Unable to register iio device\n");
> + dev_err(dev, "Unable to register iio device\n");
> goto err_trigger_unregister;
> }
>
> @@ -1698,10 +1675,11 @@ err_buffer_cleanup:
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(bmc150_accel_core_probe);
>
> -static int bmc150_accel_remove(struct i2c_client *client)
> +int bmc150_accel_core_remove(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
>
> pm_runtime_disable(data->dev);
> @@ -1720,6 +1698,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(bmc150_accel_core_remove);
>
> #ifdef CONFIG_PM_SLEEP
> static int bmc150_accel_suspend(struct device *dev)
> @@ -1790,48 +1769,8 @@ static int bmc150_accel_runtime_resume(struct device *dev)
> }
> #endif
>
> -static const struct dev_pm_ops bmc150_accel_pm_ops = {
> +const struct dev_pm_ops bmc150_accel_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(bmc150_accel_suspend, bmc150_accel_resume)
> SET_RUNTIME_PM_OPS(bmc150_accel_runtime_suspend,
> bmc150_accel_runtime_resume, NULL)
> };
> -
> -static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> - {"BSBA0150", bmc150},
> - {"BMC150A", bmc150},
> - {"BMI055A", bmi055},
> - {"BMA0255", bma255},
> - {"BMA250E", bma250e},
> - {"BMA222E", bma222e},
> - {"BMA0280", bma280},
> - { },
> -};
> -MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
> -
> -static const struct i2c_device_id bmc150_accel_id[] = {
> - {"bmc150_accel", bmc150},
> - {"bmi055_accel", bmi055},
> - {"bma255", bma255},
> - {"bma250e", bma250e},
> - {"bma222e", bma222e},
> - {"bma280", bma280},
> - {}
> -};
> -
> -MODULE_DEVICE_TABLE(i2c, bmc150_accel_id);
> -
> -static struct i2c_driver bmc150_accel_driver = {
> - .driver = {
> - .name = BMC150_ACCEL_DRV_NAME,
> - .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match),
> - .pm = &bmc150_accel_pm_ops,
> - },
> - .probe = bmc150_accel_probe,
> - .remove = bmc150_accel_remove,
> - .id_table = bmc150_accel_id,
> -};
> -module_i2c_driver(bmc150_accel_driver);
> -
> -MODULE_AUTHOR("Srinivas Pandruvada <[email protected]>");
> -MODULE_LICENSE("GPL v2");
> -MODULE_DESCRIPTION("BMC150 accelerometer driver");
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> new file mode 100644
> index 000000000000..7e036e2eb077
> --- /dev/null
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -0,0 +1,101 @@
> +/*
> + * 3-axis accelerometer driver supporting following I2C Bosch-Sensortec chips:
> + * - BMC150
> + * - BMI055
> + * - BMA255
> + * - BMA250E
> + * - BMA222E
> + * - BMA280
> + *
> + * 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/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/regmap.h>
> +
> +#include "bmc150-accel.h"
> +
> +static const struct regmap_config bmc150_i2c_regmap_conf = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .use_single_rw = true,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static int bmc150_accel_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct regmap *regmap;
> + bool block_supported =
> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> + i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK);
> +
> + regmap = devm_regmap_init_i2c(client, &bmc150_i2c_regmap_conf);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Failed to initialize i2c regmap\n");
> + return PTR_ERR(regmap);
> + }
> +
> + return bmc150_accel_core_probe(&client->dev, regmap, client->irq,
> + id->name, id->driver_data,
> + block_supported);
> +}
> +
> +static int bmc150_accel_remove(struct i2c_client *client)
> +{
> + return bmc150_accel_core_remove(&client->dev);
> +}
> +
> +static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> + {"BSBA0150", bmc150},
> + {"BMC150A", bmc150},
> + {"BMI055A", bmi055},
> + {"BMA0255", bma255},
> + {"BMA250E", bma250e},
> + {"BMA222E", bma222e},
> + {"BMA0280", bma280},
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
> +
> +static const struct i2c_device_id bmc150_accel_id[] = {
> + {"bmc150_accel", bmc150},
> + {"bmi055_accel", bmi055},
> + {"bma255", bma255},
> + {"bma250e", bma250e},
> + {"bma222e", bma222e},
> + {"bma280", bma280},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, bmc150_accel_id);
> +
> +static struct i2c_driver bmc150_accel_driver = {
> + .driver = {
> + .name = "bmc150_accel_i2c",
> + .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match),
> + .pm = &bmc150_accel_pm_ops,
> + },
> + .probe = bmc150_accel_probe,
> + .remove = bmc150_accel_remove,
> + .id_table = bmc150_accel_id,
> +};
> +module_i2c_driver(bmc150_accel_driver);
> +
> +MODULE_AUTHOR("Srinivas Pandruvada <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("BMC150 I2C accelerometer driver");
> diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> new file mode 100644
> index 000000000000..988b57573d0c
> --- /dev/null
> +++ b/drivers/iio/accel/bmc150-accel.h
> @@ -0,0 +1,21 @@
> +#ifndef _BMC150_ACCEL_H_
> +#define _BMC150_ACCEL_H_
> +
> +struct regmap;
> +
> +enum {
> + bmc150,
> + bmi055,
> + bma255,
> + bma250e,
> + bma222e,
> + bma280,
> +};
> +
> +int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name, int chip_id,
> + bool block_supported);
> +int bmc150_accel_core_remove(struct device *dev);
> +extern const struct dev_pm_ops bmc150_accel_pm_ops;
> +
> +#endif /* _BMC150_ACCEL_H_ */
>

2015-08-15 13:47:25

by Jonathan Cameron

On Sat, Aug 15, 2015 at 02:47:20PM +0100, Jonathan Cameron wrote:
> On 12/08/15 11:12, Markus Pargmann wrote:
> > Add a simple SPI driver which initializes the spi regmap for the bmc150
> > core driver.
> >
> > Signed-off-by: Markus Pargmann <[email protected]>
> 1 thing inline, plus the change of kconfig approach suggested for the
> previous patch.
>
> Otherwise, looks fine to me.
>
> Srinivas, over to you for the next version :) (unless you are feeling
> enthusiastic and want to take a look at this one)
> > ---
> > drivers/iio/accel/Kconfig | 10 +++++
> > drivers/iio/accel/Makefile | 1 +
> > drivers/iio/accel/bmc150-accel-spi.c | 86 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 97 insertions(+)
> > create mode 100644 drivers/iio/accel/bmc150-accel-spi.c
> >
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index c63e981c38ff..bdb42069a767 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -40,6 +40,16 @@ config BMC150_ACCEL_I2C
> > Say yes here to build support for I2C communication with the
> > mentioned accelerometer.
> >
> > +config BMC150_ACCEL_SPI
> > + tristate "SPI support"
> > + depends on SPI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + select REGMAP_SPI
> > + help
> > + Say yes here to build support for SPI communication with the
> > + mentioned accelerometer.
> If you move to the config approach I suggested earlier, these options
> are hidden away avoiding the need for a help message ;)

Yes.

> > +
> > endif
> >
> > config HID_SENSOR_ACCEL_3D
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 5ef8bdbad092..e579e93bf022 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -6,6 +6,7 @@
> > obj-$(CONFIG_BMA180) += bma180.o
> > obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
> > obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
> > +obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
> > obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
> > obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
> > obj-$(CONFIG_KXSD9) += kxsd9.o
> > diff --git a/drivers/iio/accel/bmc150-accel-spi.c b/drivers/iio/accel/bmc150-accel-spi.c
> > new file mode 100644
> > index 000000000000..c13fd2aa5f34
> > --- /dev/null
> > +++ b/drivers/iio/accel/bmc150-accel-spi.c
> > @@ -0,0 +1,86 @@
> > +/*
> > + * 3-axis accelerometer driver supporting following I2C Bosch-Sensortec chips:
> Umm. Read the line above? Spot the obvious minor point ;)

Oh yes. Thanks.

Best regards,

Markus

>
> > + * - BMC150
> > + * - BMI055
> > + * - BMA255
> > + * - BMA250E
> > + * - BMA222E
> > + * - BMA280
> > + *
> > + * 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/device.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include "bmc150-accel.h"
> > +
> > +static const struct regmap_config bmc150_spi_regmap_conf = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = 0x3f,
> > +
> > + .use_single_rw = false,
> > + .cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int bmc150_accel_probe(struct spi_device *spi)
> > +{
> > + struct regmap *regmap;
> > + const struct spi_device_id *id = spi_get_device_id(spi);
> > +
> > + regmap = devm_regmap_init_spi(spi, &bmc150_spi_regmap_conf);
> > + if (IS_ERR(regmap)) {
> > + dev_err(&spi->dev, "Failed to initialize spi regmap\n");
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return bmc150_accel_core_probe(&spi->dev, regmap, spi->irq,
> > + id->name, id->driver_data, true);
> > +}
> > +
> > +static int bmc150_accel_remove(struct spi_device *spi)
> > +{
> > + return bmc150_accel_core_remove(&spi->dev);
> > +}
> > +
> > +static const struct spi_device_id bmc150_accel_id[] = {
> > + {"bmc150_accel", bmc150},
> > + {"bmi055_accel", bmi055},
> > + {"bma255", bma255},
> > + {"bma250e", bma250e},
> > + {"bma222e", bma222e},
> > + {"bma280", bma280},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(spi, bmc150_accel_id);
> > +
> > +static struct spi_driver bmc150_accel_driver = {
> > + .driver = {
> > + .name = "bmc150_accel_spi",
> > + .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match),
> > + .pm = &bmc150_accel_pm_ops,
> > + },
> > + .probe = bmc150_accel_probe,
> > + .remove = bmc150_accel_remove,
> > + .id_table = bmc150_accel_id,
> > +};
> > +module_spi_driver(bmc150_accel_driver);
> > +
> > +MODULE_AUTHOR("Markus Pargmann <[email protected]>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("BMC150 SPI accelerometer driver");
> >
>
>

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (5.42 kB)
signature.asc (819.00 B)
Digital signature Download all attachments