On Mon, 11 Dec 2017 10:19:31 +0100 Crt Mori <cmo@xxxxxxxxxxx> wrote: > Melexis has just released Infra Red temperature sensor MLX90632 used > for contact-less temperature measurement. Driver provides basic > functionality for reporting object (and ambient) temperature with > support for object emissivity. > > Signed-off-by: Crt Mori <cmo@xxxxxxxxxxx> Out of curiosity, how are you testing runtime pm? Normally I'd expect this driver to be deliberately putting the hardware to sleep but it isn't doing so that I can see. A few minor comments inline. Jonathan > --- > MAINTAINERS | 7 + > drivers/iio/temperature/Kconfig | 12 + > drivers/iio/temperature/Makefile | 1 + > drivers/iio/temperature/mlx90632.c | 774 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 794 insertions(+) > create mode 100644 drivers/iio/temperature/mlx90632.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2d3d750b19c0..81aec02b08b8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8690,6 +8690,13 @@ W: http://www.melexis.com > S: Supported > F: drivers/iio/temperature/mlx90614.c > > +MELEXIS MLX90632 DRIVER > +M: Crt Mori <cmo@xxxxxxxxxxx> > +L: linux-iio@xxxxxxxxxxxxxxx > +W: http://www.melexis.com > +S: Supported > +F: drivers/iio/temperature/mlx90632.c > + > MELFAS MIP4 TOUCHSCREEN DRIVER > M: Sangwon Jee <jeesw@xxxxxxxxxx> > W: http://www.melfas.com > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig > index 5378976d6d27..82e4a62745e2 100644 > --- a/drivers/iio/temperature/Kconfig > +++ b/drivers/iio/temperature/Kconfig > @@ -43,6 +43,18 @@ config MLX90614 > This driver can also be built as a module. If so, the module will > be called mlx90614. > > +config MLX90632 > + tristate "MLX90632 contact-less infrared sensor with medical accuracy" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get support for the Melexis > + MLX90632 contact-less infrared sensor with medical accuracy > + connected with I2C. > + > + This driver can also be built as a module. If so, the module will > + be called mlx90632. > + <snip> > +static int mlx90632_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct iio_dev *indio_dev; > + struct mlx90632_data *mlx90632; > + struct regmap *regmap; > + int ret; > + unsigned int read; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*mlx90632)); > + if (!indio_dev) { > + dev_err(&client->dev, "Failed to allocate device"); > + return -ENOMEM; > + } > + > + regmap = devm_regmap_init_i2c(client, &mlx90632_regmap); > + if (IS_ERR(regmap)) { > + ret = PTR_ERR(regmap); > + dev_err(&client->dev, "Failed to allocate regmap: %d\n", ret); > + return ret; > + } > + > + mlx90632 = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + mlx90632->client = client; > + mlx90632->regmap = regmap; > + > + mutex_init(&mlx90632->lock); > + mlx90632_wakeup(mlx90632); > + > + indio_dev->dev.parent = &client->dev; > + indio_dev->name = id->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &mlx90632_info; > + indio_dev->channels = mlx90632_channels; > + indio_dev->num_channels = ARRAY_SIZE(mlx90632_channels); > + > + ret = regmap_read(mlx90632->regmap, MLX90632_EE_VERSION, &read); > + if (ret < 0) { > + dev_err(&client->dev, "read of version failed: %d\n", ret); > + return ret; > + } > + if (read == MLX90632_ID_MEDICAL) { > + dev_dbg(&client->dev, > + "Detected Medical EEPROM calibration %x", read); > + } else if (read == MLX90632_ID_CONSUMER) { > + dev_dbg(&client->dev, > + "Detected Consumer EEPROM calibration %x", read); > + } else { > + dev_err(&client->dev, > + "Chip EEPROM version mismatch %x (expected %x or %x)", > + read, MLX90632_ID_CONSUMER, MLX90632_ID_MEDICAL); > + return -EPROTONOSUPPORT; > + } > + > + mlx90632->emissivity = 1000; > + > + return iio_device_register(indio_dev); > +} > + > +static int mlx90632_remove(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct mlx90632_data *data = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + mlx90632_sleep(data); > + pm_runtime_set_suspended(&client->dev); > + > + return 0; > +} > + > +static const struct i2c_device_id mlx90632_id[] = { > + { "mlx90632", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, mlx90632_id); > + > +static const struct of_device_id mlx90632_of_match[] = { > + { .compatible = "melexis,mlx90632" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mlx90632_of_match); > + > +static int __maybe_unused mlx90632_pm_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct mlx90632_data *data = iio_priv(indio_dev); > + > + regcache_sync(data->regmap); This could be syncing to a runtime sleeping device. Also not needed as the cache should be right anyway I think. > + > + if (pm_runtime_active(dev)) > + return mlx90632_sleep(data); This prevents the device from sleeping twice, but nothing stops it from coming up in resume if it was already in runtime pm. > + > + return 0; > +} > + > +static int __maybe_unused mlx90632_pm_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct mlx90632_data *data = iio_priv(indio_dev); > + int err; > + > + regcache_mark_dirty(data->regmap); > + regcache_cache_only(data->regmap, false); > + err = regcache_sync(data->regmap); > + if (err < 0) { > + dev_err(dev, "Failed to sync regmap registers: %d\n", err); > + return err; > + } > + > + err = mlx90632_wakeup(data); > + if (err < 0) > + return err; > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > + return 0; > +} > + > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct mlx90632_data *mlx90632 = iio_priv(indio_dev); > + > + regcache_sync(mlx90632->regmap); Why sync here? The cache should be in sync already. > + > + return mlx90632_sleep(mlx90632); > +} > + > +static int __maybe_unused mlx90632_pm_runtime_resume(struct device *dev) > +{ > + s32 ret; > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct mlx90632_data *mlx90632 = iio_priv(indio_dev); > + > + regcache_mark_dirty(mlx90632->regmap); Technically it doesn't matter much, but I think the convention is to normally mark the cache dirty when it becomes possible for it to be dirty - that is during suspend. > + regcache_cache_only(mlx90632->regmap, false); > + ret = regcache_sync(mlx90632->regmap); > + if (ret < 0) { > + dev_err(dev, "Failed to sync regmap registers: %d\n", ret); > + return ret; > + } > + > + return mlx90632_wakeup(mlx90632); This doesn't look quite right. What if the device was in runtime suspend before full suspend? Would expect it to return to that state on resume but not runtime_resume. > +} > + > +static const struct dev_pm_ops mlx90632_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume) > + SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend, > + mlx90632_pm_runtime_resume, NULL) > +}; > + > +static struct i2c_driver mlx90632_driver = { > + .driver = { > + .name = "mlx90632", > + .of_match_table = mlx90632_of_match, > + .pm = &mlx90632_pm_ops, > + }, > + .probe = mlx90632_probe, > + .remove = mlx90632_remove, > + .id_table = mlx90632_id, > +}; > +module_i2c_driver(mlx90632_driver); > + > +MODULE_AUTHOR("Crt Mori <cmo@xxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Melexis MLX90632 contactless Infra Red temperature sensor driver"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
