From: Ondrej Jirman <[email protected]>
This series adds support for AF8133J magnetometer sensor. It's a simple
3-axis sensor with two sensitivity options and not much else to it.
This sensor is used on both Pinephone and Pinephone Pro. DT patches
adding it will come later, once this driver is merged.
Please take a look. :)
Thank you very much,
Ondřej Jirman
v2:
- move maintainers patch to the end of series
- bindings:
- fix compatible definition in bindings file
- require power supplies
- fix descriptions
- driver:
- sort includes
- rework RPM, the driver should now work with RPM disabled
among other improvements
- I've tested RPM left and right doing device bind/unbind under
various conditions, system suspend under various conditions,
etc.
- use scoped_guard for mutexes
- use devm for power down and handle power down correctly with both
RPM enabled/disabled without tracking power state in data->powered
- fix issue with changing scale while RPM suspended
- various code formatting issues resolved
- as for sign-offs, I've added co-developed-by for people I know for
sure worked on the driver, and left other tags as they were when
I picked up the patch 2 years ago to my Linux branch
Icenowy Zheng (3):
dt-bindings: vendor-prefix: Add prefix for Voltafield
dt-bindings: iio: magnetometer: Add Voltafield AF8133J
iio: magnetometer: add a driver for Voltafield AF8133J magnetometer
Ondrej Jirman (1):
MAINTAINERS: Add an entry for AF8133J driver
.../iio/magnetometer/voltafield,af8133j.yaml | 60 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 6 +
drivers/iio/magnetometer/Kconfig | 12 +
drivers/iio/magnetometer/Makefile | 1 +
drivers/iio/magnetometer/af8133j.c | 528 ++++++++++++++++++
6 files changed, 609 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
create mode 100644 drivers/iio/magnetometer/af8133j.c
--
2.43.0
From: Icenowy Zheng <[email protected]>
Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
Technology Corp, with dual power supplies (one for core and one for I/O)
and active-low reset pin.
The sensor has configurable range 1.2 - 2.2 mT and a software controlled
standby mode.
Add a device tree binding for it.
Signed-off-by: Icenowy Zheng <[email protected]>
Signed-off-by: Ondřej Jirman <[email protected]>
---
.../iio/magnetometer/voltafield,af8133j.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
new file mode 100644
index 000000000000..b6ab01a6914a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Voltafield AF8133J magnetometer sensor
+
+maintainers:
+ - Ondřej Jirman <[email protected]>
+
+properties:
+ compatible:
+ const: voltafield,af8133j
+
+ reg:
+ maxItems: 1
+
+ reset-gpios:
+ description:
+ A signal for active low reset input of the sensor. (optional; if not
+ used, software reset over I2C will be used instead)
+
+ avdd-supply:
+ description:
+ A regulator that provides AVDD power (Working power, usually 3.3V) to
+ the sensor.
+
+ dvdd-supply:
+ description:
+ A regulator that provides DVDD power (Digital IO power, 1.8V - AVDD)
+ to the sensor.
+
+ mount-matrix:
+ description: An optional 3x3 mounting rotation matrix.
+
+required:
+ - compatible
+ - reg
+ - avdd-supply
+ - dvdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ magnetometer@1c {
+ compatible = "voltafield,af8133j";
+ reg = <0x1c>;
+ avdd-supply = <®_dldo1>;
+ dvdd-supply = <®_dldo1>;
+ reset-gpios = <&pio 1 1 GPIO_ACTIVE_LOW>;
+ };
+ };
--
2.43.0
From: Icenowy Zheng <[email protected]>
AF8133J is a simple I2C-connected magnetometer, without interrupts.
Add a simple IIO driver for it.
Co-developed-by: Icenowy Zheng <[email protected]>
Signed-off-by: Icenowy Zheng <[email protected]>
Signed-off-by: Dalton Durst <[email protected]>
Signed-off-by: Shoji Keita <[email protected]>
Co-developed-by: Ondrej Jirman <[email protected]>
Signed-off-by: Ondrej Jirman <[email protected]>
---
drivers/iio/magnetometer/Kconfig | 12 +
drivers/iio/magnetometer/Makefile | 1 +
drivers/iio/magnetometer/af8133j.c | 528 +++++++++++++++++++++++++++++
3 files changed, 541 insertions(+)
create mode 100644 drivers/iio/magnetometer/af8133j.c
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 38532d840f2a..cd2917d71904 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -6,6 +6,18 @@
menu "Magnetometer sensors"
+config AF8133J
+ tristate "Voltafield AF8133J 3-Axis Magnetometer"
+ depends on I2C
+ depends on OF
+ select REGMAP_I2C
+ help
+ Say yes here to build support for Voltafield AF8133J I2C-based
+ 3-axis magnetometer chip.
+
+ To compile this driver as a module, choose M here: the module
+ will be called af8133j.
+
config AK8974
tristate "Asahi Kasei AK8974 3-Axis Magnetometer"
depends on I2C
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index b1c784ea71c8..ec5c46fbf999 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -4,6 +4,7 @@
#
# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AF8133J) += af8133j.o
obj-$(CONFIG_AK8974) += ak8974.o
obj-$(CONFIG_AK8975) += ak8975.o
obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
new file mode 100644
index 000000000000..1f64a2337f6e
--- /dev/null
+++ b/drivers/iio/magnetometer/af8133j.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * af8133j.c - Voltafield AF8133J magnetometer driver
+ *
+ * Copyright 2021 Icenowy Zheng <[email protected]>
+ * Copyright 2024 Ondřej Jirman <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AF8133J_REG_OUT 0x03
+#define AF8133J_REG_PCODE 0x00
+#define AF8133J_REG_PCODE_VAL 0x5e
+#define AF8133J_REG_STATUS 0x02
+#define AF8133J_REG_STATUS_ACQ BIT(0)
+#define AF8133J_REG_STATE 0x0a
+#define AF8133J_REG_STATE_STBY 0x00
+#define AF8133J_REG_STATE_WORK 0x01
+#define AF8133J_REG_RANGE 0x0b
+#define AF8133J_REG_RANGE_22G 0x12
+#define AF8133J_REG_RANGE_12G 0x34
+#define AF8133J_REG_SWR 0x11
+#define AF8133J_REG_SWR_PERFORM 0x81
+
+static const char * const af8133j_supply_names[] = {
+ "avdd",
+ "dvdd",
+};
+
+struct af8133j_data {
+ struct i2c_client *client;
+ struct regmap *regmap;
+ struct mutex mutex;
+ struct iio_mount_matrix orientation;
+
+ struct gpio_desc *reset_gpiod;
+ struct regulator_bulk_data supplies[ARRAY_SIZE(af8133j_supply_names)];
+
+ u8 range;
+};
+
+enum af8133j_axis {
+ AXIS_X = 0,
+ AXIS_Y,
+ AXIS_Z,
+};
+
+static struct iio_mount_matrix *
+af8133j_get_mount_matrix(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct af8133j_data *data = iio_priv(indio_dev);
+
+ return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info af8133j_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, af8133j_get_mount_matrix),
+ { }
+};
+
+#define AF8133J_CHANNEL(_si, _axis) { \
+ .type = IIO_MAGN, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_ ## _axis, \
+ .address = AXIS_ ## _axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
+ .ext_info = af8133j_ext_info, \
+ .scan_index = _si, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
+}
+
+static const struct iio_chan_spec af8133j_channels[] = {
+ AF8133J_CHANNEL(0, X),
+ AF8133J_CHANNEL(1, Y),
+ AF8133J_CHANNEL(2, Z),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static int af8133j_product_check(struct af8133j_data *data)
+{
+ struct device *dev = &data->client->dev;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
+ if (ret < 0) {
+ dev_err(dev, "Error reading product code (%d)\n", ret);
+ return ret;
+ }
+
+ if (val != AF8133J_REG_PCODE_VAL) {
+ dev_err(dev, "Invalid product code (0x%02x)\n", val);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int af8133j_reset(struct af8133j_data *data)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ if (data->reset_gpiod) {
+ /* If we have GPIO reset line, use it */
+ gpiod_set_value_cansleep(data->reset_gpiod, 1);
+ udelay(10);
+ gpiod_set_value_cansleep(data->reset_gpiod, 0);
+ } else {
+ /* Otherwise use software reset */
+ ret = regmap_write(data->regmap, AF8133J_REG_SWR,
+ AF8133J_REG_SWR_PERFORM);
+ if (ret < 0) {
+ dev_err(dev, "Failed to reset the chip\n");
+ return ret;
+ }
+ }
+
+ /* Wait for reset to finish */
+ usleep_range(1000, 1100);
+
+ /* Restore range setting */
+ if (data->range == AF8133J_REG_RANGE_22G) {
+ ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static void af8133j_power_down(struct af8133j_data *data)
+{
+ gpiod_set_value_cansleep(data->reset_gpiod, 1);
+ regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+}
+
+static int af8133j_power_up(struct af8133j_data *data)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+ if (ret) {
+ dev_err(dev, "Could not enable regulators\n");
+ return ret;
+ }
+
+ gpiod_set_value_cansleep(data->reset_gpiod, 0);
+
+ /* Wait for power on reset */
+ usleep_range(15000, 16000);
+
+ ret = af8133j_reset(data);
+ if (ret) {
+ af8133j_power_down(data);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int af8133j_take_measurement(struct af8133j_data *data)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_write(data->regmap,
+ AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
+ if (ret < 0)
+ return ret;
+
+ /* The datasheet says "Mesaure Time <1.5ms" */
+ ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
+ val & AF8133J_REG_STATUS_ACQ,
+ 500, 1500);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(data->regmap,
+ AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret) {
+ /*
+ * Ignore EACCES because that happens when RPM is disabled
+ * during system sleep, while userspace leave eg. hrtimer
+ * trigger attached and IIO core keeps trying to do measurements.
+ */
+ if (ret != -EACCES)
+ dev_err(dev, "Failed to power on (%d)\n", ret);
+ return ret;
+ }
+
+ scoped_guard(mutex, &data->mutex) {
+ ret = af8133j_take_measurement(data);
+ if (ret)
+ goto out_rpm_put;
+
+ ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
+ buf, sizeof(__le16) * 3);
+ }
+
+out_rpm_put:
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return ret;
+}
+
+static const int af8133j_scales[][2] = {
+ [0] = { 0, 366210 }, // 12 gauss
+ [1] = { 0, 671386 }, // 22 gauss
+};
+
+static int af8133j_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct af8133j_data *data = iio_priv(indio_dev);
+ __le16 buf[3];
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = af8133j_read_measurement(data, buf);
+ if (ret < 0)
+ return ret;
+
+ *val = sign_extend32(le16_to_cpu(buf[chan->address]),
+ chan->scan_type.realbits - 1);
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = 0;
+
+ if (data->range == AF8133J_REG_RANGE_12G)
+ *val2 = af8133j_scales[0][1];
+ else
+ *val2 = af8133j_scales[1][1];
+
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int af8133j_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (const int *)af8133j_scales;
+ *length = ARRAY_SIZE(af8133j_scales) * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int af8133j_set_scale(struct af8133j_data *data,
+ unsigned int val, unsigned int val2)
+{
+ struct device *dev = &data->client->dev;
+ u8 range;
+ int ret = 0;
+
+ if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
+ range = AF8133J_REG_RANGE_12G;
+ else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
+ range = AF8133J_REG_RANGE_22G;
+ else
+ return -EINVAL;
+
+ pm_runtime_disable(dev);
+
+ /*
+ * When suspended, just store the new range to data->range to be
+ * applied later during power up.
+ */
+ if (!pm_runtime_status_suspended(dev))
+ ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
+
+ pm_runtime_enable(dev);
+
+ data->range = range;
+ return ret;
+}
+
+static int af8133j_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct af8133j_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ scoped_guard(mutex, &data->mutex)
+ ret = af8133j_set_scale(data, val, val2);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const struct iio_info af8133j_info = {
+ .read_raw = af8133j_read_raw,
+ .read_avail = af8133j_read_avail,
+ .write_raw = af8133j_write_raw,
+ .write_raw_get_fmt = af8133j_write_raw_get_fmt,
+};
+
+static irqreturn_t af8133j_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct af8133j_data *data = iio_priv(indio_dev);
+ s64 timestamp = iio_get_time_ns(indio_dev);
+ struct {
+ __le16 values[3];
+ s64 timestamp __aligned(8);
+ } sample;
+ int ret;
+
+ memset(&sample, 0, sizeof(sample));
+
+ ret = af8133j_read_measurement(data, sample.values);
+ if (ret)
+ goto out_done;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
+
+out_done:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static const struct regmap_config af8133j_regmap_config = {
+ .name = "af8133j_regmap",
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = AF8133J_REG_SWR,
+ .cache_type = REGCACHE_NONE,
+};
+
+static void af8133j_power_down_action(void *ptr)
+{
+ struct af8133j_data *data = ptr;
+ struct device *dev = &data->client->dev;
+
+ pm_runtime_disable(dev);
+ if (!pm_runtime_status_suspended(dev))
+ af8133j_power_down(data);
+ pm_runtime_enable(dev);
+}
+
+static int af8133j_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct af8133j_data *data;
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ int ret, i;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "regmap initialization failed\n");
+
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+ data->regmap = regmap;
+ data->range = AF8133J_REG_RANGE_12G;
+ mutex_init(&data->mutex);
+
+ data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(data->reset_gpiod))
+ return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
+ "Failed to get reset gpio\n");
+
+ for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
+ data->supplies[i].supply = af8133j_supply_names[i];
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+ data->supplies);
+ if (ret)
+ return ret;
+
+ ret = iio_read_mount_matrix(dev, &data->orientation);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
+
+ ret = af8133j_power_up(data);
+ if (ret)
+ return ret;
+
+ pm_runtime_set_active(dev);
+
+ ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
+ if (ret)
+ return ret;
+
+ ret = af8133j_product_check(data);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &af8133j_info;
+ indio_dev->name = "af8133j";
+ indio_dev->channels = af8133j_channels;
+ indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ &af8133j_trigger_handler, NULL);
+ if (ret < 0)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to setup iio triggered buffer\n");
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register iio device");
+
+ pm_runtime_get_noresume(dev);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_set_autosuspend_delay(dev, 500);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
+
+ pm_runtime_put_autosuspend(dev);
+
+ return 0;
+}
+
+static int af8133j_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct af8133j_data *data = iio_priv(indio_dev);
+
+ af8133j_power_down(data);
+
+ return 0;
+}
+
+static int af8133j_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct af8133j_data *data = iio_priv(indio_dev);
+
+ return af8133j_power_up(data);
+}
+
+const struct dev_pm_ops af8133j_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+ RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
+};
+
+static const struct of_device_id af8133j_of_match[] = {
+ { .compatible = "voltafield,af8133j", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, af8133j_of_match);
+
+static const struct i2c_device_id af8133j_id[] = {
+ { "af8133j", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, af8133j_id);
+
+static struct i2c_driver af8133j_driver = {
+ .driver = {
+ .name = "af8133j",
+ .of_match_table = af8133j_of_match,
+ .pm = pm_ptr(&af8133j_pm_ops),
+ },
+ .probe = af8133j_probe,
+ .id_table = af8133j_id,
+};
+
+module_i2c_driver(af8133j_driver);
+
+MODULE_AUTHOR("Icenowy Zheng <[email protected]>");
+MODULE_AUTHOR("Ondřej Jirman <[email protected]>");
+MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
+MODULE_LICENSE("GPL");
--
2.43.0
From: Ondrej Jirman <[email protected]>
As I am submitting the driver and have the device to test. I'll maintain
the driver.
Signed-off-by: Ondrej Jirman <[email protected]>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index dc5ca7a042b5..cc691f61a77e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -579,6 +579,12 @@ F: drivers/iio/accel/adxl372.c
F: drivers/iio/accel/adxl372_i2c.c
F: drivers/iio/accel/adxl372_spi.c
+AF8133J THREE-AXIS MAGNETOMETER DRIVER
+M: Ondřej Jirman <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
+F: drivers/iio/magnetometer/af8133j.c
+
AF9013 MEDIA DRIVER
L: [email protected]
S: Orphan
--
2.43.0
On Mon, Feb 12, 2024 at 06:53:54PM +0100, Ondřej Jirman wrote:
> From: Icenowy Zheng <[email protected]>
>
> Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
> Technology Corp, with dual power supplies (one for core and one for I/O)
> and active-low reset pin.
>
> The sensor has configurable range 1.2 - 2.2 mT and a software controlled
> standby mode.
>
> Add a device tree binding for it.
>
> Signed-off-by: Icenowy Zheng <[email protected]>
> Signed-off-by: Ondřej Jirman <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Cheers,
Conor.
Hi Ondřej,
thank you for submitting the driver.
On 24-02-12 18:53, Ondřej Jirman wrote:
> From: Icenowy Zheng <[email protected]>
>
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
>
> Add a simple IIO driver for it.
>
> Co-developed-by: Icenowy Zheng <[email protected]>
> Signed-off-by: Icenowy Zheng <[email protected]>
> Signed-off-by: Dalton Durst <[email protected]>
> Signed-off-by: Shoji Keita <[email protected]>
> Co-developed-by: Ondrej Jirman <[email protected]>
> Signed-off-by: Ondrej Jirman <[email protected]>
> ---
> drivers/iio/magnetometer/Kconfig | 12 +
> drivers/iio/magnetometer/Makefile | 1 +
> drivers/iio/magnetometer/af8133j.c | 528 +++++++++++++++++++++++++++++
> 3 files changed, 541 insertions(+)
> create mode 100644 drivers/iio/magnetometer/af8133j.c
>
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 38532d840f2a..cd2917d71904 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -6,6 +6,18 @@
>
Reviewed-by: Andrey Skvortsov <[email protected]>
I've successfully tested driver from v2 on 6.8-rc3.
--
Best regards,
Andrey Skvortsov
Hi Jonathan,
On Wed, Feb 14, 2024 at 05:01:36PM +0000, Jonathan Cameron wrote:
> On Mon, 12 Feb 2024 18:53:55 +0100
> Ondřej Jirman <[email protected]> wrote:
>
> > From: Icenowy Zheng <[email protected]>
> >
> > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> >
> > Add a simple IIO driver for it.
> >
> > Co-developed-by: Icenowy Zheng <[email protected]>
> > Signed-off-by: Icenowy Zheng <[email protected]>
> > Signed-off-by: Dalton Durst <[email protected]>
> > Signed-off-by: Shoji Keita <[email protected]>
> > Co-developed-by: Ondrej Jirman <[email protected]>
> > Signed-off-by: Ondrej Jirman <[email protected]>
>
>
> Hi a few comments (mostly on changes)
>
> The runtime_pm handling can be simplified somewhat if you
> rearrange probe a little.
>
> > diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> > new file mode 100644
> > index 000000000000..1f64a2337f6e
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/af8133j.c
> > @@ -0,0 +1,528 @@
>
>
> > +static int af8133j_take_measurement(struct af8133j_data *data)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap,
> > + AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* The datasheet says "Mesaure Time <1.5ms" */
> > + ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
> > + val & AF8133J_REG_STATUS_ACQ,
> > + 500, 1500);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = regmap_write(data->regmap,
> > + AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);
>
> return regmap_write()
>
> regmap accesses return 0 or a negative error code enabling little code
> reductions like this.
Yeah, some reviewers dislike this, because modifying the code in the future
creates a more unpleasant diff. But if you like this style, I don't mind
changing it.
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
> > +{
> > + struct device *dev = &data->client->dev;
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret) {
> > + /*
> > + * Ignore EACCES because that happens when RPM is disabled
> > + * during system sleep, while userspace leave eg. hrtimer
> > + * trigger attached and IIO core keeps trying to do measurements.
>
> Yeah. We still need to fix that more elegantly :(
>
> > + */
> > + if (ret != -EACCES)
> > + dev_err(dev, "Failed to power on (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + scoped_guard(mutex, &data->mutex) {
> > + ret = af8133j_take_measurement(data);
> > + if (ret)
> > + goto out_rpm_put;
> > +
> > + ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> > + buf, sizeof(__le16) * 3);
> > + }
> > +
> > +out_rpm_put:
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + return ret;
> > +}
> > +
>
>
> > +
> > +static int af8133j_set_scale(struct af8133j_data *data,
> > + unsigned int val, unsigned int val2)
> > +{
> > + struct device *dev = &data->client->dev;
> > + u8 range;
> > + int ret = 0;
> > +
> > + if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
> > + range = AF8133J_REG_RANGE_12G;
> > + else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
> > + range = AF8133J_REG_RANGE_22G;
> > + else
> > + return -EINVAL;
> > +
> > + pm_runtime_disable(dev);
> > +
> > + /*
> > + * When suspended, just store the new range to data->range to be
> > + * applied later during power up.
> Better to just do
> pm_runtime_resume_and_get() here
>
> > + */
> > + if (!pm_runtime_status_suspended(dev))
> > + ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> > +
> > + pm_runtime_enable(dev);
> and
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> here.
>
> The userspace interface is only way this function is called so rearrange
> probe a little so that you don't need extra complexity in these functions.
It doesn't make sense to wakeup the device for range change, because it will
forget the range the moment it's powered off again, after changing the range.
Also this function has nothing to do with probe. data->range is authoritative
value, not cache. It gets applied to HW on each power up.
>
> > +
> > + data->range = range;
>
> If the write failed, generally don't update the cached value.
Right.
> > + return ret;
> > +}
> > +
> > +static int af8133j_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct af8133j_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + scoped_guard(mutex, &data->mutex)
> > + ret = af8133j_set_scale(data, val, val2);
>
> Look more closely at what scoped_guard() does.
> return af8133j_set_scale(data, val, val2);
> is fine and simpler as no local variable needed.
I did, it will not work as you suggest. It's implemented as for loop with
condition, and the compiler will complain about fallthrough.
I can do:
scoped_guard(mutex, &data->mutex)
return af8133j_set_scale(data, val, val2);
return 0;
but it looks weirder at first glance, at least to my eye.
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +static void af8133j_power_down_action(void *ptr)
> > +{
> > + struct af8133j_data *data = ptr;
> > + struct device *dev = &data->client->dev;
> > +
> > + pm_runtime_disable(dev);
> You group together unwinding of calls that occur in very
> different places in probe. Don't do that as it leas
> to disabling runtime pm having never enabled it
> in some error paths. That may be safe but if fails the
> obviously correct test.
This whole disable/enable dance is here so that pm_runtime_status_suspended can
be trusted. Not for disabling PM during device remove or in error paths.
There's no imbalance here or problem with disabling PM when it's already
disabled. Disable/enable is reference counted, and this function keeps the
balance.
>
> Instead, have multiple callbacks registered.
> Disable will happen anyway due to
> > + if (!pm_runtime_status_suspended(dev))
> This works as the stub for no runtime pm support returns
> false.
Output can't be trusted as long as RPM is enabled.
> So this is a good solution to the normal dance of turning power on
> just to turn it off shortly afterwards.
>
> > + af8133j_power_down(data);
> > + pm_runtime_enable(dev);
> Why?
See above. To keep the disable ref count balanced.
Looks like actual RPM disable already happened at this point a bit earlier in
another callback registered via devm_pm_runtime_enable(). I guess this
pm_runtime_enable()/pm_runtime_disable() guard can just be skipped, because RPM
is already disabled thanks to reverse ordering of devm callbacks during device
remove. So while this is safe, it's redundant at this point and call to
pm_runtime_status_suspended() is safe without this.
> > +}
> > +
> > +static int af8133j_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct af8133j_data *data;
> > + struct iio_dev *indio_dev;
> > + struct regmap *regmap;
> > + int ret, i;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > + if (IS_ERR(regmap))
> > + return dev_err_probe(dev, PTR_ERR(regmap),
> > + "regmap initialization failed\n");
> > +
> > + data = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + data->client = client;
> > + data->regmap = regmap;
> > + data->range = AF8133J_REG_RANGE_12G;
> > + mutex_init(&data->mutex);
> > +
> > + data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(data->reset_gpiod))
> > + return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > + "Failed to get reset gpio\n");
> > +
> > + for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
> > + data->supplies[i].supply = af8133j_supply_names[i];
> > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> > + data->supplies);
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_read_mount_matrix(dev, &data->orientation);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > +
> > + ret = af8133j_power_up(data);
> > + if (ret)
> > + return ret;
> > +
> > + pm_runtime_set_active(dev);
> > +
> > + ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
>
> As mentioned above, this should only undo things done before this point.
> So just the af8133j_power_down() I think.
The callback doesn't do anything else but power down. It leaves everything
else as is after it exits.
> > + if (ret)
> > + return ret;
> > +
> > + ret = af8133j_product_check(data);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->info = &af8133j_info;
> > + indio_dev->name = "af8133j";
> > + indio_dev->channels = af8133j_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > + &af8133j_trigger_handler, NULL);
> > + if (ret < 0)
> > + return dev_err_probe(&client->dev, ret,
> > + "Failed to setup iio triggered buffer\n");
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to register iio device");
> > +
> > + pm_runtime_get_noresume(dev);
>
> > + pm_runtime_use_autosuspend(dev);
> > + pm_runtime_set_autosuspend_delay(dev, 500);
> > + ret = devm_pm_runtime_enable(dev);
>
> This already deals with pm_runtime_disable() so you shouldn't need do it manually.
I'm not disabling RPM manually, it was just used as temporary guard to be able
to read pm_runtime_status_suspended() safely.
> Also you want to enable that before the devm_iio_device_register() to avoid
> problems with it not being available as the userspace interfaces are used.
>
> So just move this up a few lines.
Good idea, thanks.
kind regards,
o.
>
>
> > + if (ret)
> > + return ret;
> > +
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + return 0;
> > +}
>
On 24-02-14 16:38, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 00:21:31 +0300
> Andrey Skvortsov <[email protected]> wrote:
>
> > Hi Ondřej,
> >
> > thank you for submitting the driver.
> >
> > On 24-02-12 18:53, Ondřej Jirman wrote:
> > > From: Icenowy Zheng <[email protected]>
> > >
> > > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > >
> > > Add a simple IIO driver for it.
> > >
> > > Co-developed-by: Icenowy Zheng <[email protected]>
> > > Signed-off-by: Icenowy Zheng <[email protected]>
> > > Signed-off-by: Dalton Durst <[email protected]>
> > > Signed-off-by: Shoji Keita <[email protected]>
> > > Co-developed-by: Ondrej Jirman <[email protected]>
> > > Signed-off-by: Ondrej Jirman <[email protected]>
> > > ---
> > > drivers/iio/magnetometer/Kconfig | 12 +
> > > drivers/iio/magnetometer/Makefile | 1 +
> > > drivers/iio/magnetometer/af8133j.c | 528 +++++++++++++++++++++++++++++
> > > 3 files changed, 541 insertions(+)
> > > create mode 100644 drivers/iio/magnetometer/af8133j.c
> > >
> > > diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> > > index 38532d840f2a..cd2917d71904 100644
> > > --- a/drivers/iio/magnetometer/Kconfig
> > > +++ b/drivers/iio/magnetometer/Kconfig
> > > @@ -6,6 +6,18 @@
> > >
> >
> > Reviewed-by: Andrey Skvortsov <[email protected]>
> >
> > I've successfully tested driver from v2 on 6.8-rc3.
> >
> How about a Tested-by tag so we can keep that for ever?
I have nothing against that.
Tested-by: Andrey Skvortsov <[email protected]>
--
Best regards,
Andrey Skvortsov
On Wed, 14 Feb 2024 18:43:10 +0100
Ondřej Jirman <[email protected]> wrote:
> Hi Jonathan,
Gah. Runtime pm always gives me a headache. I'd indeed misunderstood some
of what you are doing.
>
> On Wed, Feb 14, 2024 at 05:01:36PM +0000, Jonathan Cameron wrote:
> > On Mon, 12 Feb 2024 18:53:55 +0100
> > Ondřej Jirman <[email protected]> wrote:
> >
> > > From: Icenowy Zheng <[email protected]>
> > >
> > > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > >
> > > Add a simple IIO driver for it.
> > >
> > > Co-developed-by: Icenowy Zheng <[email protected]>
> > > Signed-off-by: Icenowy Zheng <[email protected]>
> > > Signed-off-by: Dalton Durst <[email protected]>
> > > Signed-off-by: Shoji Keita <[email protected]>
> > > Co-developed-by: Ondrej Jirman <[email protected]>
> > > Signed-off-by: Ondrej Jirman <[email protected]>
> >
> >
> > Hi a few comments (mostly on changes)
> >
> > The runtime_pm handling can be simplified somewhat if you
> > rearrange probe a little.
> >
> > > diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> > > new file mode 100644
> > > index 000000000000..1f64a2337f6e
> > > --- /dev/null
> > > +++ b/drivers/iio/magnetometer/af8133j.c
> > > @@ -0,0 +1,528 @@
> >
> >
> > > +static int af8133j_take_measurement(struct af8133j_data *data)
> > > +{
> > > + unsigned int val;
> > > + int ret;
> > > +
> > > + ret = regmap_write(data->regmap,
> > > + AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* The datasheet says "Mesaure Time <1.5ms" */
> > > + ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
> > > + val & AF8133J_REG_STATUS_ACQ,
> > > + 500, 1500);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = regmap_write(data->regmap,
> > > + AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);
> >
> > return regmap_write()
> >
> > regmap accesses return 0 or a negative error code enabling little code
> > reductions like this.
>
> Yeah, some reviewers dislike this, because modifying the code in the future
> creates a more unpleasant diff. But if you like this style, I don't mind
> changing it.
Always a gamble on chance of a modification coming.
In general I'd check regmap calls with if (ret) but don't feel that strongly
about that either.
So not really important either way.
>
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
> > > +{
> > > + struct device *dev = &data->client->dev;
> > > + int ret;
> > > +
> > > + ret = pm_runtime_resume_and_get(dev);
> > > + if (ret) {
> > > + /*
> > > + * Ignore EACCES because that happens when RPM is disabled
> > > + * during system sleep, while userspace leave eg. hrtimer
> > > + * trigger attached and IIO core keeps trying to do measurements.
> >
> > Yeah. We still need to fix that more elegantly :(
> >
> > > + */
> > > + if (ret != -EACCES)
> > > + dev_err(dev, "Failed to power on (%d)\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + scoped_guard(mutex, &data->mutex) {
> > > + ret = af8133j_take_measurement(data);
> > > + if (ret)
> > > + goto out_rpm_put;
> > > +
> > > + ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> > > + buf, sizeof(__le16) * 3);
> > > + }
> > > +
> > > +out_rpm_put:
> > > + pm_runtime_mark_last_busy(dev);
> > > + pm_runtime_put_autosuspend(dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> >
> >
> > > +
> > > +static int af8133j_set_scale(struct af8133j_data *data,
> > > + unsigned int val, unsigned int val2)
> > > +{
> > > + struct device *dev = &data->client->dev;
> > > + u8 range;
> > > + int ret = 0;
> > > +
> > > + if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
> > > + range = AF8133J_REG_RANGE_12G;
> > > + else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
> > > + range = AF8133J_REG_RANGE_22G;
> > > + else
> > > + return -EINVAL;
> > > +
> > > + pm_runtime_disable(dev);
> > > +
> > > + /*
> > > + * When suspended, just store the new range to data->range to be
> > > + * applied later during power up.
> > Better to just do
> > pm_runtime_resume_and_get() here
> >
> > > + */
> > > + if (!pm_runtime_status_suspended(dev))
> > > + ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> > > +
> > > + pm_runtime_enable(dev);
> > and
> > pm_runtime_mark_last_busy(dev);
> > pm_runtime_put_autosuspend(dev);
> > here.
> >
> > The userspace interface is only way this function is called so rearrange
> > probe a little so that you don't need extra complexity in these functions.
>
> It doesn't make sense to wakeup the device for range change, because it will
> forget the range the moment it's powered off again, after changing the range.
Ah. I'd missed understood that. Thanks for extra explanation.
I'm not keen on the enable / disable dance but anything else is probably worse
(delaying update until we actually using it etc).
>
> Also this function has nothing to do with probe. data->range is authoritative
> value, not cache. It gets applied to HW on each power up.
>
> >
> > > +
> > > + data->range = range;
> >
> > If the write failed, generally don't update the cached value.
>
> Right.
>
> > > + return ret;
> > > +}
> > > +
> > > +static int af8133j_write_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int val, int val2, long mask)
> > > +{
> > > + struct af8133j_data *data = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_SCALE:
> > > + scoped_guard(mutex, &data->mutex)
> > > + ret = af8133j_set_scale(data, val, val2);
> >
> > Look more closely at what scoped_guard() does.
> > return af8133j_set_scale(data, val, val2);
> > is fine and simpler as no local variable needed.
>
> I did, it will not work as you suggest. It's implemented as for loop with
> condition, and the compiler will complain about fallthrough.
>
> I can do:
>
> scoped_guard(mutex, &data->mutex)
> return af8133j_set_scale(data, val, val2);
> return 0;
>
> but it looks weirder at first glance, at least to my eye.
I agree that bit is less than ideal, but with your code it should also
get confused about whether ret is ever set.
scoped_guard(mutex, &data->mutex)
return ...
unreachable();
perhaps? or just use a guard and add scope manually
case IIO_CHAN_INFO_SCALE: {
guard(mutex)(&data->mutex);
return af8133j_set_scale(...);
}
I'd go with this as the cleanest solution in this case.
>
> > > + return ret;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> >
> > > +static void af8133j_power_down_action(void *ptr)
> > > +{
> > > + struct af8133j_data *data = ptr;
> > > + struct device *dev = &data->client->dev;
> > > +
> > > + pm_runtime_disable(dev);
> > You group together unwinding of calls that occur in very
> > different places in probe. Don't do that as it leas
> > to disabling runtime pm having never enabled it
> > in some error paths. That may be safe but if fails the
> > obviously correct test.
>
> This whole disable/enable dance is here so that pm_runtime_status_suspended can
> be trusted. Not for disabling PM during device remove or in error paths.
>
> There's no imbalance here or problem with disabling PM when it's already
> disabled. Disable/enable is reference counted, and this function keeps the
> balance.
Whilst not buggy but I still want to be able to cleanly associate a given
bit of cleanup with what is being cleaned up. That is the path to
maintainable code longer term. Runtime PM does make a mess of doing this
but tends to have somewhat logical sets of calls that go together.
As long as we hold a reference, doesn't matter when we turn it on in probe()
Only the put_autosuspend has to come after we done talking to it.
>
> > So this is a good solution to the normal dance of turning power on
> > just to turn it off shortly afterwards.
> >
> > > + af8133j_power_down(data);
> > > + pm_runtime_enable(dev);
> > Why?
>
> See above. To keep the disable ref count balanced.
>
> Looks like actual RPM disable already happened at this point a bit earlier in
> another callback registered via devm_pm_runtime_enable(). I guess this
> pm_runtime_enable()/pm_runtime_disable() guard can just be skipped, because RPM
> is already disabled thanks to reverse ordering of devm callbacks during device
> remove. So while this is safe, it's redundant at this point and call to
> pm_runtime_status_suspended() is safe without this.
Yes, That's a side effect of only enabling it right at the end.
>
> > > +}
> > > +
> > > +static int af8133j_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct af8133j_data *data;
> > > + struct iio_dev *indio_dev;
> > > + struct regmap *regmap;
> > > + int ret, i;
> > > +
> > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > > + if (IS_ERR(regmap))
> > > + return dev_err_probe(dev, PTR_ERR(regmap),
> > > + "regmap initialization failed\n");
> > > +
> > > + data = iio_priv(indio_dev);
> > > + i2c_set_clientdata(client, indio_dev);
> > > + data->client = client;
> > > + data->regmap = regmap;
> > > + data->range = AF8133J_REG_RANGE_12G;
> > > + mutex_init(&data->mutex);
> > > +
> > > + data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(data->reset_gpiod))
> > > + return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > > + "Failed to get reset gpio\n");
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
> > > + data->supplies[i].supply = af8133j_supply_names[i];
> > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> > > + data->supplies);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = iio_read_mount_matrix(dev, &data->orientation);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > > +
> > > + ret = af8133j_power_up(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + pm_runtime_set_active(dev);
> > > +
> > > + ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
> >
> > As mentioned above, this should only undo things done before this point.
> > So just the af8133j_power_down() I think.
>
> The callback doesn't do anything else but power down. It leaves everything
> else as is after it exits.
>
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = af8133j_product_check(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + indio_dev->info = &af8133j_info;
> > > + indio_dev->name = "af8133j";
> > > + indio_dev->channels = af8133j_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > + &af8133j_trigger_handler, NULL);
> > > + if (ret < 0)
> > > + return dev_err_probe(&client->dev, ret,
> > > + "Failed to setup iio triggered buffer\n");
> > > +
> > > + ret = devm_iio_device_register(dev, indio_dev);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to register iio device");
> > > +
> > > + pm_runtime_get_noresume(dev);
> >
> > > + pm_runtime_use_autosuspend(dev);
> > > + pm_runtime_set_autosuspend_delay(dev, 500);
> > > + ret = devm_pm_runtime_enable(dev);
> >
> > This already deals with pm_runtime_disable() so you shouldn't need do it manually.
>
> I'm not disabling RPM manually, it was just used as temporary guard to be able
> to read pm_runtime_status_suspended() safely.
I'd indeed misunderstood that. I forgot the oddity that runtime pm is effectively
reference counting in only one direction for enable / disable and the opposite
one for get and put.
pm_runtime_disable()
pm_runtime_disable()
pm_runtime_enable()
pm_runtime_enable()
pm_runtime_enable()
is fine, but
pm_runtime_enable()
pm_runtime_enable()
pm_runtime_disable()
pm_runtime_disable()
pm_runtime_enable()
is not.
Which makes sense when you realise it's all about ensuring it is off, but
never ensuring that it is turned on.
>
> > Also you want to enable that before the devm_iio_device_register() to avoid
> > problems with it not being available as the userspace interfaces are used.
> >
> > So just move this up a few lines.
>
> Good idea, thanks.
>
> kind regards,
> o.
>
> >
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + pm_runtime_put_autosuspend(dev);
> > > +
> > > + return 0;
> > > +}
> >
On Fri, Feb 16, 2024 at 03:39:25PM +0000, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 18:43:10 +0100
> Ondřej Jirman <[email protected]> wrote:
>
> > Hi Jonathan,
>
> Gah. Runtime pm always gives me a headache. I'd indeed misunderstood some
> of what you are doing.
> >
> > [...]
> >
> > I did, it will not work as you suggest. It's implemented as for loop with
> > condition, and the compiler will complain about fallthrough.
> >
> > I can do:
> >
> > scoped_guard(mutex, &data->mutex)
> > return af8133j_set_scale(data, val, val2);
> > return 0;
> >
> > but it looks weirder at first glance, at least to my eye.
>
> I agree that bit is less than ideal, but with your code it should also
> get confused about whether ret is ever set.
>
> scoped_guard(mutex, &data->mutex)
> return ...
> unreachable();
>
> perhaps? or just use a guard and add scope manually
>
> case IIO_CHAN_INFO_SCALE: {
> guard(mutex)(&data->mutex);
>
> return af8133j_set_scale(...);
> }
>
> I'd go with this as the cleanest solution in this case.
Yes, that looks much nicer. Thanks. :)
Though in the end I'll go with pushing the locking down to actual register
access in the af8133j_set_scale() function, so that I don't lock around
RPM disable/enable for no reason.
>
> >
> > > > + return ret;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > >
> > > > +static void af8133j_power_down_action(void *ptr)
> > > > +{
> > > > + struct af8133j_data *data = ptr;
> > > > + struct device *dev = &data->client->dev;
> > > > +
> > > > + pm_runtime_disable(dev);
> > > You group together unwinding of calls that occur in very
> > > different places in probe. Don't do that as it leas
> > > to disabling runtime pm having never enabled it
> > > in some error paths. That may be safe but if fails the
> > > obviously correct test.
> >
> > This whole disable/enable dance is here so that pm_runtime_status_suspended can
> > be trusted. Not for disabling PM during device remove or in error paths.
> >
> > There's no imbalance here or problem with disabling PM when it's already
> > disabled. Disable/enable is reference counted, and this function keeps the
> > balance.
>
> Whilst not buggy but I still want to be able to cleanly associate a given
> bit of cleanup with what is being cleaned up. That is the path to
> maintainable code longer term. Runtime PM does make a mess of doing this
> but tends to have somewhat logical sets of calls that go together.
>
> As long as we hold a reference, doesn't matter when we turn it on in probe()
> Only the put_autosuspend has to come after we done talking to it.
>
> >
> > > So this is a good solution to the normal dance of turning power on
> > > just to turn it off shortly afterwards.
> > >
> > > > + af8133j_power_down(data);
> > > > + pm_runtime_enable(dev);
> > > Why?
> >
> > See above. To keep the disable ref count balanced.
> >
> > Looks like actual RPM disable already happened at this point a bit earlier in
> > another callback registered via devm_pm_runtime_enable(). I guess this
> > pm_runtime_enable()/pm_runtime_disable() guard can just be skipped, because RPM
> > is already disabled thanks to reverse ordering of devm callbacks during device
> > remove. So while this is safe, it's redundant at this point and call to
> > pm_runtime_status_suspended() is safe without this.
>
> Yes, That's a side effect of only enabling it right at the end.
>
> >
> > > > +}
> > > > +
> > > > +static int af8133j_probe(struct i2c_client *client)
> > > > +{
> > > > + struct device *dev = &client->dev;
> > > > + struct af8133j_data *data;
> > > > + struct iio_dev *indio_dev;
> > > > + struct regmap *regmap;
> > > > + int ret, i;
> > > > +
> > > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > > + if (!indio_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > > > + if (IS_ERR(regmap))
> > > > + return dev_err_probe(dev, PTR_ERR(regmap),
> > > > + "regmap initialization failed\n");
> > > > +
> > > > + data = iio_priv(indio_dev);
> > > > + i2c_set_clientdata(client, indio_dev);
> > > > + data->client = client;
> > > > + data->regmap = regmap;
> > > > + data->range = AF8133J_REG_RANGE_12G;
> > > > + mutex_init(&data->mutex);
> > > > +
> > > > + data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > + if (IS_ERR(data->reset_gpiod))
> > > > + return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > > > + "Failed to get reset gpio\n");
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
> > > > + data->supplies[i].supply = af8133j_supply_names[i];
> > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> > > > + data->supplies);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = iio_read_mount_matrix(dev, &data->orientation);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > > > +
> > > > + ret = af8133j_power_up(data);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pm_runtime_set_active(dev);
> > > > +
> > > > + ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
> > >
> > > As mentioned above, this should only undo things done before this point.
> > > So just the af8133j_power_down() I think.
> >
> > The callback doesn't do anything else but power down. It leaves everything
> > else as is after it exits.
> >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = af8133j_product_check(data);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + indio_dev->info = &af8133j_info;
> > > > + indio_dev->name = "af8133j";
> > > > + indio_dev->channels = af8133j_channels;
> > > > + indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +
> > > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > > + &af8133j_trigger_handler, NULL);
> > > > + if (ret < 0)
> > > > + return dev_err_probe(&client->dev, ret,
> > > > + "Failed to setup iio triggered buffer\n");
> > > > +
> > > > + ret = devm_iio_device_register(dev, indio_dev);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret, "Failed to register iio device");
> > > > +
> > > > + pm_runtime_get_noresume(dev);
> > >
> > > > + pm_runtime_use_autosuspend(dev);
> > > > + pm_runtime_set_autosuspend_delay(dev, 500);
> > > > + ret = devm_pm_runtime_enable(dev);
> > >
> > > This already deals with pm_runtime_disable() so you shouldn't need do it manually.
> >
> > I'm not disabling RPM manually, it was just used as temporary guard to be able
> > to read pm_runtime_status_suspended() safely.
>
> I'd indeed misunderstood that. I forgot the oddity that runtime pm is effectively
> reference counting in only one direction for enable / disable and the opposite
> one for get and put.
>
> pm_runtime_disable()
> pm_runtime_disable()
> pm_runtime_enable()
> pm_runtime_enable()
> pm_runtime_enable()
> is fine, but
>
> pm_runtime_enable()
> pm_runtime_enable()
> pm_runtime_disable()
> pm_runtime_disable()
> pm_runtime_enable()
> is not.
>
> Which makes sense when you realise it's all about ensuring it is off, but
> never ensuring that it is turned on.
Yeah. Enabling already enabled RPM is thankfully easier to catch though, due to
a kernel warning:
https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L1494
Unbalanced disable is annoying though. Not sure why, but disable_depth even
persists device unbind, so next rebinding will leave RPM disabled after probe.
That is when doing this after intentionally make the driver disable RPM twice
in remove callback:
echo 4-001c > /sys/bus/i2c/drivers/af8133j/unbind
echo 4-001c > /sys/bus/i2c/drivers/af8133j/bind
(driver remove/probe gets called)
Maybe it's due to RPM dependencies on parent device. Dunno. But it's a silent
problem in this case.
regards,
o.
>
>
>
> >
> > > Also you want to enable that before the devm_iio_device_register() to avoid
> > > problems with it not being available as the userspace interfaces are used.
> > >
> > > So just move this up a few lines.
> >
> > Good idea, thanks.
> >
> > kind regards,
> > o.
> >
> > >
> > >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pm_runtime_put_autosuspend(dev);
> > > > +
> > > > + return 0;
> > > > +}
> > >
>