| Message ID | 20190326101651.25056-1-rui.silva@linaro.org |
|---|---|
| Headers | show |
| Series | iio: gyro: add fxas21002c driver | expand |
On Tue, 26 Mar 2019 10:16:47 +0000 Rui Miguel Silva <rui.silva@linaro.org> wrote: > Add core support for the NXP fxas21002c Tri-axis gyroscope, > using the iio subsystem. It supports PM operations, axis reading, > temperature, scale factor of the axis, high pass and low pass > filtering, and sampling frequency selection. > > It will have extras modules to support the communication over i2c and > spi. > > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> Ah, slightly missunderstanding around what I suggested for probe / remove ordering. I've marked it inline and would have just fixed it whilst applying but there is another issue inline that I had missed before. > --- > drivers/iio/gyro/Kconfig | 11 + > drivers/iio/gyro/Makefile | 1 + > drivers/iio/gyro/fxas21002c.h | 151 +++++ > drivers/iio/gyro/fxas21002c_core.c | 1006 ++++++++++++++++++++++++++++ > 4 files changed, 1169 insertions(+) > create mode 100644 drivers/iio/gyro/fxas21002c.h > create mode 100644 drivers/iio/gyro/fxas21002c_core.c > > diff --git a/drivers/iio/gyro/fxas21002c.h b/drivers/iio/gyro/fxas21002c.h > new file mode 100644 > index 000000000000..e21fd410950c > --- /dev/null > +++ b/drivers/iio/gyro/fxas21002c.h > @@ -0,0 +1,151 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Driver for NXP FXAS21002C Gyroscope - Header > + * > + * Copyright (C) 2019 Linaro Ltd. > + * I'm a stickler for hanging blank lines. So drop this one. > + */ > + > +#ifndef FXAS21002C_H_ > +#define FXAS21002C_H_ ... > + > +static irqreturn_t fxas21002c_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct fxas21002c_data *data = iio_priv(indio_dev); > + int ret; > + > + mutex_lock(&data->lock); > + ret = regmap_bulk_read(data->regmap, FXAS21002C_REG_OUT_X_MSB, > + data->buffer, CHANNEL_SCAN_MAX * sizeof(s16)); > + mutex_unlock(&data->lock); A side note here. Right now it's not possible to use data->buffer concurrently so I'm not sure this lock is actually needed. In most drivers we also end up using the same buffer for other accesses, but here it it is only this one which is in a thread which can only be running once at a time IIRC. Ah well, paranoid never hurt anyone ;) I only got suspicious when I noticed it didn't protect the buffer when it is still be used in the push_to_buffers call below. > + if (ret < 0) > + goto notify_done; > + > + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer, > + pf->timestamp); > + > +notify_done: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static int fxas21002c_chip_init(struct fxas21002c_data *data) > +{ > + struct device *dev = regmap_get_device(data->regmap); > + unsigned int chip_id; > + int ret; > + > + ret = regmap_field_read(data->regmap_fields[F_WHO_AM_I], &chip_id); > + if (ret < 0) > + return ret; > + > + if (chip_id != FXAS21002C_CHIP_ID_1 && > + chip_id != FXAS21002C_CHIP_ID_2) { > + dev_err(dev, "chip id 0x%02x is not supported\n", chip_id); > + return -EINVAL; > + } > + > + data->chip_id = chip_id; > + > + ret = fxas21002c_mode_set(data, FXAS21002C_MODE_STANDBY); > + if (ret < 0) > + return ret; > + > + /* Set ODR to 200HZ as default */ > + ret = fxas21002c_odr_set(data, 200); > + if (ret < 0) > + dev_err(dev, "failed to set ODR: %d\n", ret); > + > + return ret; > +} > + > +static int fxas21002c_data_rdy_trigger_set_state(struct iio_trigger *trig, > + bool state) > +{ > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct fxas21002c_data *data = iio_priv(indio_dev); > + > + return regmap_field_write(data->regmap_fields[F_INT_EN_DRDY], state); > +} > + > +static const struct iio_trigger_ops fxas21002c_trigger_ops = { > + .set_trigger_state = &fxas21002c_data_rdy_trigger_set_state, > +}; > + > +static irqreturn_t fxas21002c_data_rdy_trig_poll(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct fxas21002c_data *data = iio_priv(indio_dev); > + unsigned int data_ready; > + int ret; > + > + ret = regmap_field_read(data->regmap_fields[F_SRC_DRDY], &data_ready); > + if (ret < 0) > + return IRQ_NONE; > + > + if (!data_ready) > + return IRQ_NONE; > + > + iio_trigger_poll(data->dready_trig); This isn't right. The function sleeps in the regmap_read above adn iio_trigger_poll is to be called from an irq context. You want iio_trigger_poll_chained (which is missnamed and I should fix that one day - it should probably be _nested) which is the version that only calls the thread parts. https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-trigger.c#L189 The problem is that you are relying on the iio_pollfunc_store_time top half which won't ever get called. Easy fix though is to just get the timestamp in your interrupt thread function directly. > + > + return IRQ_HANDLED; > +} > + > +static int fxas21002c_trigger_probe(struct fxas21002c_data *data) > +{ > + struct device *dev = regmap_get_device(data->regmap); > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct device_node *np = indio_dev->dev.of_node; > + unsigned long irq_trig; > + bool irq_open_drain; > + int irq1; > + int ret; > + > + if (!data->irq) > + return 0; > + > + irq1 = of_irq_get_byname(np, "INT1"); > + > + if (irq1 == data->irq) { > + dev_info(dev, "using interrupt line INT1\n"); > + ret = regmap_field_write(data->regmap_fields[F_INT_CFG_DRDY], > + 1); > + if (ret < 0) > + return ret; > + } > + > + dev_info(dev, "using interrupt line INT2\n"); > + > + irq_open_drain = of_property_read_bool(np, "drive-open-drain"); > + > + data->dready_trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > + indio_dev->name, > + indio_dev->id); > + if (!data->dready_trig) > + return -ENOMEM; > + > + irq_trig = irqd_get_trigger_type(irq_get_irq_data(data->irq)); > + > + if (irq_trig == IRQF_TRIGGER_RISING) { > + ret = regmap_field_write(data->regmap_fields[F_IPOL], 1); > + if (ret < 0) > + return ret; > + } > + > + if (irq_open_drain) > + irq_trig |= IRQF_SHARED; > + > + ret = devm_request_irq(dev, data->irq, fxas21002c_data_rdy_trig_poll, > + irq_trig, "fxas21002c_data_ready", > + data->dready_trig); > + if (ret < 0) > + return ret; > + > + data->dready_trig->dev.parent = dev; > + data->dready_trig->ops = &fxas21002c_trigger_ops; > + iio_trigger_set_drvdata(data->dready_trig, indio_dev); > + > + return iio_trigger_register(data->dready_trig); You are fine using the devm version of this . See below. > +} > + > +static int fxas21002c_power_enable(struct fxas21002c_data *data) > +{ > + int ret; > + > + ret = regulator_enable(data->vdd); > + if (ret < 0) > + return ret; > + > + ret = regulator_enable(data->vddio); > + if (ret < 0) { > + regulator_disable(data->vdd); > + return ret; > + } > + > + return 0; > +} > + > +static void fxas21002c_power_disable(struct fxas21002c_data *data) > +{ > + regulator_disable(data->vdd); > + regulator_disable(data->vddio); > +} > + > +static void fxas21002c_power_disable_action(void *_data) > +{ > + struct fxas21002c_data *data = _data; > + > + fxas21002c_power_disable(data); > +} > + > +static int fxas21002c_regulators_get(struct fxas21002c_data *data) > +{ > + struct device *dev = regmap_get_device(data->regmap); > + > + data->vdd = devm_regulator_get(dev->parent, "vdd"); > + if (IS_ERR(data->vdd)) > + return PTR_ERR(data->vdd); > + > + data->vddio = devm_regulator_get(dev->parent, "vddio"); > + if (IS_ERR(data->vddio)) > + return PTR_ERR(data->vddio); > + > + return 0; > +} > + > +int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq, > + const char *name) > +{ > + struct fxas21002c_data *data; > + struct iio_dev *indio_dev; > + struct regmap_field *f; > + int i; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + dev_set_drvdata(dev, indio_dev); > + data->irq = irq; > + data->regmap = regmap; > + > + for (i = 0; i < F_MAX_FIELDS; i++) { > + f = devm_regmap_field_alloc(dev, data->regmap, > + fxas21002c_reg_fields[i]); > + if (IS_ERR(f)) > + return PTR_ERR(f); > + > + data->regmap_fields[i] = f; > + } > + > + mutex_init(&data->lock); > + > + ret = fxas21002c_regulators_get(data); > + if (ret < 0) > + return ret; > + > + ret = fxas21002c_power_enable(data); > + if (ret < 0) > + return ret; > + > + ret = devm_add_action_or_reset(dev, fxas21002c_power_disable_action, > + data); Good. This effectively makes up for us not having a devm_regulator_enable so it will be automatically cleared up in remove or error paths. > + if (ret < 0) > + return ret; > + > + ret = fxas21002c_chip_init(data); > + if (ret < 0) > + return ret; > + > + indio_dev->dev.parent = dev; > + indio_dev->channels = fxas21002c_channels; > + indio_dev->num_channels = ARRAY_SIZE(fxas21002c_channels); > + indio_dev->name = name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &fxas21002c_info; > + > + ret = fxas21002c_trigger_probe(data); > + if (ret < 0) > + return ret; This can still use the devm_iio_trigger_register internally so doesn't need clearing up by hand. > + > + ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time, > + fxas21002c_trigger_handler, NULL); > + if (ret < 0) devm_iio_trigger_buffer_setup is fine. Then return ret; > + goto trigger_unregister; > + > + ret = pm_runtime_set_active(dev); > + if (ret) > + goto buffer_cleanup; So this is the point where we now have to stop using devm_ based cleanup if we want to maintain ordering. So all correct from here, but before here you are fine using devm. > + > + pm_runtime_enable(dev); > + pm_runtime_set_autosuspend_delay(dev, 2000); > + pm_runtime_use_autosuspend(dev); > + > + ret = iio_device_register(indio_dev); > + if (ret < 0) > + goto pm_disable; > + > + return 0; > + > +pm_disable: > + pm_runtime_disable(dev); > + pm_runtime_set_suspended(dev); > + pm_runtime_put_noidle(dev); > + > +buffer_cleanup: > + iio_triggered_buffer_cleanup(indio_dev); > + > +trigger_unregister: > + if (data->irq) > + iio_trigger_unregister(data->dready_trig); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(fxas21002c_core_probe); > + > +void fxas21002c_core_remove(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct fxas21002c_data *data = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + > + pm_runtime_disable(dev); > + pm_runtime_set_suspended(dev); > + pm_runtime_put_noidle(dev); > + > + iio_triggered_buffer_cleanup(indio_dev); > + > + if (data->irq) > + iio_trigger_unregister(data->dready_trig); > + fxas21002c_power_disable(data); This is handled by the devm_action_or_reset in probe. That will ensure that remove automatically calls this. As a result there is no need to use non devm_ functions for the next few. I'll clean it up when applying but please check I didn't mess it up! > +} > +EXPORT_SYMBOL_GPL(fxas21002c_core_remove); > + > +static int __maybe_unused fxas21002c_suspend(struct device *dev) > +{ > + struct fxas21002c_data *data = iio_priv(dev_get_drvdata(dev)); > + > + fxas21002c_mode_set(data, FXAS21002C_MODE_STANDBY); > + fxas21002c_power_disable(data); > + > + return 0; > +} > + > +static int __maybe_unused fxas21002c_resume(struct device *dev) > +{ > + struct fxas21002c_data *data = iio_priv(dev_get_drvdata(dev)); > + int ret; > + > + ret = fxas21002c_power_enable(data); > + if (ret < 0) > + return ret; > + > + return fxas21002c_mode_set(data, data->prev_mode); > +} > + > +static int __maybe_unused fxas21002c_runtime_suspend(struct device *dev) > +{ > + struct fxas21002c_data *data = iio_priv(dev_get_drvdata(dev)); > + > + return fxas21002c_mode_set(data, FXAS21002C_MODE_READY); > +} > + > +static int __maybe_unused fxas21002c_runtime_resume(struct device *dev) > +{ > + struct fxas21002c_data *data = iio_priv(dev_get_drvdata(dev)); > + > + return fxas21002c_mode_set(data, FXAS21002C_MODE_ACTIVE); > +} > + > +const struct dev_pm_ops fxas21002c_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(fxas21002c_suspend, fxas21002c_resume) > + SET_RUNTIME_PM_OPS(fxas21002c_runtime_suspend, > + fxas21002c_runtime_resume, NULL) > +}; > +EXPORT_SYMBOL_GPL(fxas21002c_pm_ops); > + > +MODULE_AUTHOR("Rui Miguel Silva <rui.silva@linaro.org>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("FXAS21002C Gyro driver");
On Tue, 26 Mar 2019 10:16:45 +0000 Rui Miguel Silva <rui.silva@linaro.org> wrote: > Hi, > This series introduce a NXP fxas21002c tri axis gyroscope driver [0] > It add a core implementaiton plus an i2c and spi. > > This device can be found in the warp7 board [1], where it was tested. > > --- > Cheers, > Rui > > v4->v5: > Jonathan Cameron: > - remove init ret > - change handle of iio register, buffer and trigger in probe and > remove (I think this is what you mean, Jonathan) You kind of ended up with both versions when it was an either or. I'll have a go at tidying it up whilst applying. Please check the result. > > v3->v4: > Tomasz Duszynski: > - mention irq flags in bindings > - add reference to drive-open-drain in bindings > - fix headers files > - drop comas in of_device_id sentinel > - drop of_match_ptr > - use probe_new > > Jonathan Cameron: > - disable vdd regulator in case of vddio fail > - use devm_add_action_or_reset > - simpler functions returns > - remove noisy dev_info > - remove EAGAIN from runtime_suspend > > v2->v3: > Jonathan Cameron: > - cacheline aligned (DMA safe buffer) > Great presentation [2] and links in the presentation, thanks > - global renaming, including filenames, fxas2100x->fxas21002c > - provide spi info in dts bidings > - Remove SPI_MASTER in Kconfig i2c patch and move it to right patch > - remove extra blank line in comment > - add break range_value_from_fs after found > - in range_value_from_fs use local variable > - remove mode check at mode_set > - combine two if statements in mode_set > - in scale_get return 0 and let caller to set IIO_VAL_FRACTIONAL > - remove dev_err from vdd_io regulator get > - handle regulator error path > - devm_add_action > - check unwind order > - simplify data in _suspend and alike > - disable regulators at suspend > - error handling at _resume > - return -EAGAIN at runtime_resume > > Rob Herring: > - Set label as gyroscope > > - add interrupt to bindings > - add entry to maintainers > > > v1->v2: > Peter Meerwal-Stadler: > - changed (c) to current year > - add regmap include file in .h > - fix comments s/cuttof/cutoff/ > - add more info in mutex comment > - check value in range_fs_from_value > - ret not checked in range_value_from_fs > - move mode to enum type > - remove line between value get and validation of value in all file > - pre-write, regmap_field_write, post_write refactoring > - check val2 and val == 0 in write raw > - check in_anglvel_scale: 7.8125? > - trigger_handler: 2 => sizeof(s16) > - check buffer size > - print %02% to output chip id > - remove !! as state is bool > - trigger probe return devm_iio_trigger_register > - remove error msg in case of devm_iio_device_register > Fabio Estebam: > - rename FXAS2100X to FXAS21002 > - change compatible nxp,fxas2100x to the exact support > - add VDD and VDDIO regulators in bindings and driver > > [0]: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf > [1]: https://www.element14.com/community/community/designcenter/single-board-computers/warp7/overview > [2]: https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf > > Rui Miguel Silva (6): > iio: gyro: add DT bindings to fxas21002c > iio: gyro: add core driver for fxas21002c > iio: gyro: fxas21002c: add i2c driver > iio: gyro: fxas21002c: add spi driver > ARM: dts: imx7s-warp: add fxas21002c gyroscope > MAINTAINERS: add entry for fxas21002c gyro driver > > .../bindings/iio/gyroscope/nxp,fxas21002c.txt | 31 + > MAINTAINERS | 10 + > arch/arm/boot/dts/imx7s-warp.dts | 7 + > drivers/iio/gyro/Kconfig | 22 + > drivers/iio/gyro/Makefile | 3 + > drivers/iio/gyro/fxas21002c.h | 151 +++ > drivers/iio/gyro/fxas21002c_core.c | 1006 +++++++++++++++++ > drivers/iio/gyro/fxas21002c_i2c.c | 69 ++ > drivers/iio/gyro/fxas21002c_spi.c | 70 ++ > 9 files changed, 1369 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/gyroscope/nxp,fxas21002c.txt > create mode 100644 drivers/iio/gyro/fxas21002c.h > create mode 100644 drivers/iio/gyro/fxas21002c_core.c > create mode 100644 drivers/iio/gyro/fxas21002c_i2c.c > create mode 100644 drivers/iio/gyro/fxas21002c_spi.c >
Hi, This series introduce a NXP fxas21002c tri axis gyroscope driver [0] It add a core implementaiton plus an i2c and spi. This device can be found in the warp7 board [1], where it was tested. --- Cheers, Rui v4->v5: Jonathan Cameron: - remove init ret - change handle of iio register, buffer and trigger in probe and remove (I think this is what you mean, Jonathan) v3->v4: Tomasz Duszynski: - mention irq flags in bindings - add reference to drive-open-drain in bindings - fix headers files - drop comas in of_device_id sentinel - drop of_match_ptr - use probe_new Jonathan Cameron: - disable vdd regulator in case of vddio fail - use devm_add_action_or_reset - simpler functions returns - remove noisy dev_info - remove EAGAIN from runtime_suspend v2->v3: Jonathan Cameron: - cacheline aligned (DMA safe buffer) Great presentation [2] and links in the presentation, thanks - global renaming, including filenames, fxas2100x->fxas21002c - provide spi info in dts bidings - Remove SPI_MASTER in Kconfig i2c patch and move it to right patch - remove extra blank line in comment - add break range_value_from_fs after found - in range_value_from_fs use local variable - remove mode check at mode_set - combine two if statements in mode_set - in scale_get return 0 and let caller to set IIO_VAL_FRACTIONAL - remove dev_err from vdd_io regulator get - handle regulator error path - devm_add_action - check unwind order - simplify data in _suspend and alike - disable regulators at suspend - error handling at _resume - return -EAGAIN at runtime_resume Rob Herring: - Set label as gyroscope - add interrupt to bindings - add entry to maintainers v1->v2: Peter Meerwal-Stadler: - changed (c) to current year - add regmap include file in .h - fix comments s/cuttof/cutoff/ - add more info in mutex comment - check value in range_fs_from_value - ret not checked in range_value_from_fs - move mode to enum type - remove line between value get and validation of value in all file - pre-write, regmap_field_write, post_write refactoring - check val2 and val == 0 in write raw - check in_anglvel_scale: 7.8125? - trigger_handler: 2 => sizeof(s16) - check buffer size - print %02% to output chip id - remove !! as state is bool - trigger probe return devm_iio_trigger_register - remove error msg in case of devm_iio_device_register Fabio Estebam: - rename FXAS2100X to FXAS21002 - change compatible nxp,fxas2100x to the exact support - add VDD and VDDIO regulators in bindings and driver [0]: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf [1]: https://www.element14.com/community/community/designcenter/single-board-computers/warp7/overview [2]: https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf Rui Miguel Silva (6): iio: gyro: add DT bindings to fxas21002c iio: gyro: add core driver for fxas21002c iio: gyro: fxas21002c: add i2c driver iio: gyro: fxas21002c: add spi driver ARM: dts: imx7s-warp: add fxas21002c gyroscope MAINTAINERS: add entry for fxas21002c gyro driver .../bindings/iio/gyroscope/nxp,fxas21002c.txt | 31 + MAINTAINERS | 10 + arch/arm/boot/dts/imx7s-warp.dts | 7 + drivers/iio/gyro/Kconfig | 22 + drivers/iio/gyro/Makefile | 3 + drivers/iio/gyro/fxas21002c.h | 151 +++ drivers/iio/gyro/fxas21002c_core.c | 1006 +++++++++++++++++ drivers/iio/gyro/fxas21002c_i2c.c | 69 ++ drivers/iio/gyro/fxas21002c_spi.c | 70 ++ 9 files changed, 1369 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/gyroscope/nxp,fxas21002c.txt create mode 100644 drivers/iio/gyro/fxas21002c.h create mode 100644 drivers/iio/gyro/fxas21002c_core.c create mode 100644 drivers/iio/gyro/fxas21002c_i2c.c create mode 100644 drivers/iio/gyro/fxas21002c_spi.c