| Message ID | 20210122225443.186184-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: Add a ChromeOS EC MKBP proximity driver | expand |
On Fri, 22 Jan 2021 14:54:43 -0800 Stephen Boyd <swboyd@chromium.org> wrote: > Add support for a ChromeOS EC proximity driver that exposes a "front" > proximity sensor via the IIO subsystem. The EC decides when front > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to > notify the kernel of proximity. Similarly, when proximity detects > something far away it sets the switch bit to 0. For now this driver > exposes a single sensor, but it could be expanded in the future via more > MKBP bits if desired. > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Benson Leung <bleung@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Gwendal Grignou <gwendal@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Hi Stephen, Looks more or less fine to me. My main concern is potential confusion in naming with the cros_ec_prox_light driver that we already have. A few other minor bits and bobs inline. thanks, Jonathan > --- > drivers/iio/proximity/Kconfig | 11 + > drivers/iio/proximity/Makefile | 1 + > drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++ > 3 files changed, 264 insertions(+) > create mode 100644 drivers/iio/proximity/cros_ec_proximity.c > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig > index 12672a0e89ed..35a04e9ede7d 100644 > --- a/drivers/iio/proximity/Kconfig > +++ b/drivers/iio/proximity/Kconfig > @@ -21,6 +21,17 @@ endmenu > > menu "Proximity and distance sensors" > > +config CROS_EC_PROXIMITY > + tristate "ChromeOS EC MKBP Proximity sensor" > + depends on CROS_EC > + help > + Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP > + switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI) > + to use this. > + > + To compile this driver as a module, choose M here: the > + module will be called cros_ec_prox. > + > config ISL29501 > tristate "Intersil ISL29501 Time Of Flight sensor" > depends on I2C > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile > index 9c1aca1a8b79..b1330dd8e212 100644 > --- a/drivers/iio/proximity/Makefile > +++ b/drivers/iio/proximity/Makefile > @@ -5,6 +5,7 @@ > > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_AS3935) += as3935.o > +obj-$(CONFIG_CROS_EC_PROXIMITY) += cros_ec_proximity.o > obj-$(CONFIG_ISL29501) += isl29501.o > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o > obj-$(CONFIG_MB1232) += mb1232.o > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c > new file mode 100644 > index 000000000000..a3aef911e3cc > --- /dev/null > +++ b/drivers/iio/proximity/cros_ec_proximity.c > @@ -0,0 +1,252 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for cros-ec proximity sensor exposed through MKBP switch > + * > + * Copyright 2021 Google LLC. > + */ > + > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/kernel.h> Slight preference for alphabetical order though keeping specific sections separate as done here is fine. > +#include <linux/notifier.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +#include <linux/platform_data/cros_ec_commands.h> > +#include <linux/platform_data/cros_ec_proto.h> > + > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > +#include <asm/unaligned.h> > + > +struct cros_ec_proximity_data { > + struct cros_ec_device *ec; > + struct iio_dev *indio_dev; > + struct mutex lock; > + struct notifier_block notifier; > + bool enabled; > +}; > + > +static const struct iio_event_spec cros_ec_prox_events[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = { > + { > + .type = IIO_PROXIMITY, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .event_spec = cros_ec_prox_events, > + .num_event_specs = ARRAY_SIZE(cros_ec_prox_events), > + }, > +}; > + > +static int cros_ec_proximity_parse_state(const void *data) > +{ > + u32 switches = get_unaligned_le32(data); > + > + return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY)); > +} > + > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) > +{ > + struct ec_params_mkbp_info *params; > + struct cros_ec_command *msg; > + int ret; > + > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), > + GFP_KERNEL); Given this is known at build time, perhaps better to add it to the iio_priv() accessed structure and avoid having to handle allocations separately. > + if (!msg) > + return -ENOMEM; > + > + msg->command = EC_CMD_MKBP_INFO; > + msg->version = 1; > + msg->outsize = sizeof(*params); > + msg->insize = sizeof(u32); > + params = (struct ec_params_mkbp_info *)msg->data; > + params->info_type = EC_MKBP_INFO_CURRENT; > + params->event_type = EC_MKBP_EVENT_SWITCH; > + > + ret = cros_ec_cmd_xfer_status(ec_dev, msg); > + if (ret >= 0) { > + if (ret != sizeof(u32)) { > + dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n", > + ret, sizeof(u32)); > + ret = -EPROTO; > + } else { > + *state = cros_ec_proximity_parse_state(msg->data); > + ret = 0; If you move the allocation as suggested above, this can become if (ret < 0) return ret; if (ret != sizeof(u32)) { ... return -EPROTO; } *state = cros_.. return 0; Which will be neater and not as deeply nested. > + } > + } > + > + kfree(msg); > + > + return ret; > +} > + > +static int cros_ec_proximity_notify(struct notifier_block *nb, > + unsigned long queued_during_suspend, void *_ec) > +{ > + struct cros_ec_proximity_data *data; > + struct cros_ec_device *ec = _ec; > + u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK; > + void *switches = &ec->event_data.data.switches; > + struct iio_dev *indio_dev; > + s64 timestamp; > + int state, dir; > + u64 ev; > + > + if (event_type == EC_MKBP_EVENT_SWITCH) { > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > + indio_dev = data->indio_dev; > + > + mutex_lock(&data->lock); > + if (data->enabled) { > + timestamp = iio_get_time_ns(indio_dev); > + state = cros_ec_proximity_parse_state(switches); > + dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING; > + > + ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0, > + IIO_EV_TYPE_THRESH, dir); > + iio_push_event(indio_dev, ev, timestamp); > + } > + mutex_unlock(&data->lock); > + } > + > + return NOTIFY_OK; > +} > + > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, int *val, > + int *val2, long mask) > +{ > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > + struct cros_ec_device *ec = data->ec; > + int ret; > + > + if (chan->type != IIO_PROXIMITY) > + return -EINVAL; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = iio_device_claim_direct_mode(indio_dev); Normally we only introduce these protections when adding the ability to change the state from direct to buffered (which these prevent). This driver doesn't yet support any other modes so I don't think there is any benefit in having these. If the aim is more local protection then should use a local lock as the semantics fo these functions might change in future. > + if (ret) > + return ret; > + > + ret = cros_ec_proximity_query(ec, val); > + iio_device_release_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + } > + > + return -EINVAL; > +} > + > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > + > + return data->enabled; > +} > + > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int state) > +{ > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > + > + mutex_lock(&data->lock); > + data->enabled = state; > + mutex_unlock(&data->lock); > + > + return 0; > +} > + > +static const struct iio_info cros_ec_proximity_info = { > + .read_raw = cros_ec_proximity_read_raw, > + .read_event_config = cros_ec_proximity_read_event_config, > + .write_event_config = cros_ec_proximity_write_event_config, > +}; > + > +static int cros_ec_proximity_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); > + struct iio_dev *indio_dev; > + struct cros_ec_proximity_data *data; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + data->ec = ec; > + data->indio_dev = indio_dev; > + mutex_init(&data->lock); > + platform_set_drvdata(pdev, data); > + > + indio_dev->name = "cros_ec_proximity"; > + indio_dev->dev.parent = dev; > + indio_dev->info = &cros_ec_proximity_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = cros_ec_prox_chan_spec; > + indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return ret; > + > + data->notifier.notifier_call = cros_ec_proximity_notify; > + ret = blocking_notifier_chain_register(&ec->event_notifier, > + &data->notifier); > + if (ret) > + dev_err(dev, "cannot register notifier: %d\n", ret); > + > + return ret; > +} > + > +static int cros_ec_proximity_remove(struct platform_device *pdev) > +{ > + struct cros_ec_proximity_data *data = platform_get_drvdata(pdev); > + struct cros_ec_device *ec = data->ec; > + > + blocking_notifier_chain_unregister(&ec->event_notifier, > + &data->notifier); > + > + return 0; > +} > + > +#ifdef CONFIG_OF As a general rule, we are trying to clear out protections on CONFIG_OF etc and use of of_match_ptr() on the basis they don't really gain us anything and prevent use of some other firmware types. Here I guess you know what your firmware looks like, but I'm still keen to drop it in the interests of there being fewer places to copy it from. It may be a good idea to give this a more specific name as well given we already have cros-ec-prox as a platform device id from the cros_ec_light_prox driver. > +static const struct of_device_id cros_ec_proximity_of_match[] = { > + { .compatible = "google,cros-ec-proximity" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > +#endif > + > +static struct platform_driver cros_ec_proximity_driver = { > + .driver = { > + .name = "cros-ec-proximity", > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > + }, > + .probe = cros_ec_proximity_probe, > + .remove = cros_ec_proximity_remove, > +}; > +module_platform_driver(cros_ec_proximity_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");
On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 22 Jan 2021 14:54:43 -0800 > Stephen Boyd <swboyd@chromium.org> wrote: > > > Add support for a ChromeOS EC proximity driver that exposes a "front" > > proximity sensor via the IIO subsystem. The EC decides when front > > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to > > notify the kernel of proximity. Similarly, when proximity detects > > something far away it sets the switch bit to 0. For now this driver > > exposes a single sensor, but it could be expanded in the future via more > > MKBP bits if desired. > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Benson Leung <bleung@chromium.org> > > Cc: Guenter Roeck <groeck@chromium.org> > > Cc: Douglas Anderson <dianders@chromium.org> > > Cc: Gwendal Grignou <gwendal@chromium.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > Hi Stephen, > > Looks more or less fine to me. My main concern is potential confusion > in naming with the cros_ec_prox_light driver that we already have. > > A few other minor bits and bobs inline. > > thanks, > > Jonathan > > > > --- > > drivers/iio/proximity/Kconfig | 11 + > > drivers/iio/proximity/Makefile | 1 + > > drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++ > > 3 files changed, 264 insertions(+) > > create mode 100644 drivers/iio/proximity/cros_ec_proximity.c > > > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig > > index 12672a0e89ed..35a04e9ede7d 100644 > > --- a/drivers/iio/proximity/Kconfig > > +++ b/drivers/iio/proximity/Kconfig > > @@ -21,6 +21,17 @@ endmenu > > > > menu "Proximity and distance sensors" > > > > +config CROS_EC_PROXIMITY > > + tristate "ChromeOS EC MKBP Proximity sensor" > > + depends on CROS_EC > > + help > > + Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP > > + switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI) > > + to use this. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called cros_ec_prox. > > + > > config ISL29501 > > tristate "Intersil ISL29501 Time Of Flight sensor" > > depends on I2C > > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile > > index 9c1aca1a8b79..b1330dd8e212 100644 > > --- a/drivers/iio/proximity/Makefile > > +++ b/drivers/iio/proximity/Makefile > > @@ -5,6 +5,7 @@ > > > > # When adding new entries keep the list in alphabetical order > > obj-$(CONFIG_AS3935) += as3935.o > > +obj-$(CONFIG_CROS_EC_PROXIMITY) += cros_ec_proximity.o > > obj-$(CONFIG_ISL29501) += isl29501.o > > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o > > obj-$(CONFIG_MB1232) += mb1232.o > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c > > new file mode 100644 > > index 000000000000..a3aef911e3cc > > --- /dev/null > > +++ b/drivers/iio/proximity/cros_ec_proximity.c > > @@ -0,0 +1,252 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for cros-ec proximity sensor exposed through MKBP switch > > + * > > + * Copyright 2021 Google LLC. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/kernel.h> > > Slight preference for alphabetical order though keeping specific > sections separate as done here is fine. > > > +#include <linux/notifier.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/types.h> > > + > > +#include <linux/platform_data/cros_ec_commands.h> > > +#include <linux/platform_data/cros_ec_proto.h> > > + > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +#include <asm/unaligned.h> > > + > > +struct cros_ec_proximity_data { > > + struct cros_ec_device *ec; > > + struct iio_dev *indio_dev; > > + struct mutex lock; > > + struct notifier_block notifier; > > + bool enabled; > > +}; > > + > > +static const struct iio_event_spec cros_ec_prox_events[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_EITHER, > > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > > + }, > > +}; > > + > > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = { > > + { > > + .type = IIO_PROXIMITY, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > > + .event_spec = cros_ec_prox_events, > > + .num_event_specs = ARRAY_SIZE(cros_ec_prox_events), > > + }, > > +}; > > + > > +static int cros_ec_proximity_parse_state(const void *data) > > +{ > > + u32 switches = get_unaligned_le32(data); > > + > > + return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY)); > > +} > > + > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) > > +{ > > + struct ec_params_mkbp_info *params; > > + struct cros_ec_command *msg; > > + int ret; > > + > > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), > > + GFP_KERNEL); > > Given this is known at build time, perhaps better to add it to the > iio_priv() accessed structure and avoid having to handle allocations > separately. As Jonathan said, it can be preallocated in iio private structure. We can also use the stack, given the response size is known beforehand. See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example. > > > + if (!msg) > > + return -ENOMEM; > > + > > + msg->command = EC_CMD_MKBP_INFO; > > + msg->version = 1; > > + msg->outsize = sizeof(*params); > > + msg->insize = sizeof(u32); > > + params = (struct ec_params_mkbp_info *)msg->data; > > + params->info_type = EC_MKBP_INFO_CURRENT; > > + params->event_type = EC_MKBP_EVENT_SWITCH; > > + > > + ret = cros_ec_cmd_xfer_status(ec_dev, msg); > > + if (ret >= 0) { > > + if (ret != sizeof(u32)) { > > + dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n", > > + ret, sizeof(u32)); > > + ret = -EPROTO; > > + } else { > > + *state = cros_ec_proximity_parse_state(msg->data); > > + ret = 0; > If you move the allocation as suggested above, this can become > > if (ret < 0) > return ret; > > if (ret != sizeof(u32)) { > ... > return -EPROTO; > } > > *state = cros_.. > return 0; > > Which will be neater and not as deeply nested. > > > + } > > + } > > + > > + kfree(msg); > > + > > + return ret; > > +} > > + > > +static int cros_ec_proximity_notify(struct notifier_block *nb, > > + unsigned long queued_during_suspend, void *_ec) > > +{ > > + struct cros_ec_proximity_data *data; > > + struct cros_ec_device *ec = _ec; > > + u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK; > > + void *switches = &ec->event_data.data.switches; > > + struct iio_dev *indio_dev; > > + s64 timestamp; > > + int state, dir; > > + u64 ev; > > + > > + if (event_type == EC_MKBP_EVENT_SWITCH) { > > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > > + indio_dev = data->indio_dev; > > + > > + mutex_lock(&data->lock); > > + if (data->enabled) { > > + timestamp = iio_get_time_ns(indio_dev); For Android, given the timestamp must be time it happens, not reported [https://source.android.com/devices/sensors/sensors-hal2] """The timestamp must be accurate and correspond to the time at which the event physically happened, not the time it was reported.""", consider using ec_dev->last_event_time and apply a delta if the iio clock base is different from CLOCK_BOOTTIME. > > + state = cros_ec_proximity_parse_state(switches); > > + dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING; > > + > > + ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0, > > + IIO_EV_TYPE_THRESH, dir); > > + iio_push_event(indio_dev, ev, timestamp); > > + } > > + mutex_unlock(&data->lock); > > + } > > + > > + return NOTIFY_OK; > > +} > > + > > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, int *val, > > + int *val2, long mask) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + struct cros_ec_device *ec = data->ec; > > + int ret; > > + > > + if (chan->type != IIO_PROXIMITY) > > + return -EINVAL; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = iio_device_claim_direct_mode(indio_dev); > > Normally we only introduce these protections when adding the ability > to change the state from direct to buffered (which these prevent). > This driver doesn't yet support any other modes so I don't think > there is any benefit in having these. > > If the aim is more local protection then should use a local lock > as the semantics fo these functions might change in future. > > > > + if (ret) > > + return ret; > > + > > + ret = cros_ec_proximity_query(ec, val); > > + iio_device_release_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + > > + return IIO_VAL_INT; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + > > + return data->enabled; > > +} > > + > > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, int state) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + > > + mutex_lock(&data->lock); > > + data->enabled = state; > > + mutex_unlock(&data->lock); > > + > > + return 0; > > +} > > + > > +static const struct iio_info cros_ec_proximity_info = { > > + .read_raw = cros_ec_proximity_read_raw, > > + .read_event_config = cros_ec_proximity_read_event_config, > > + .write_event_config = cros_ec_proximity_write_event_config, > > +}; > > + > > +static int cros_ec_proximity_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); > > + struct iio_dev *indio_dev; > > + struct cros_ec_proximity_data *data; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + data = iio_priv(indio_dev); > > + data->ec = ec; > > + data->indio_dev = indio_dev; > > + mutex_init(&data->lock); > > + platform_set_drvdata(pdev, data); > > + > > + indio_dev->name = "cros_ec_proximity"; Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here and in struct platform_driver cros_ec_proximity_driver. > > + indio_dev->dev.parent = dev; Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc(). > > + indio_dev->info = &cros_ec_proximity_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = cros_ec_prox_chan_spec; > > + indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec); > > + > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret) > > + return ret; > > + > > + data->notifier.notifier_call = cros_ec_proximity_notify; > > + ret = blocking_notifier_chain_register(&ec->event_notifier, > > + &data->notifier); > > + if (ret) > > + dev_err(dev, "cannot register notifier: %d\n", ret); > > + > > + return ret; > > +} > > + > > +static int cros_ec_proximity_remove(struct platform_device *pdev) > > +{ > > + struct cros_ec_proximity_data *data = platform_get_drvdata(pdev); > > + struct cros_ec_device *ec = data->ec; > > + > > + blocking_notifier_chain_unregister(&ec->event_notifier, > > + &data->notifier); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_OF > > As a general rule, we are trying to clear out protections on CONFIG_OF etc > and use of of_match_ptr() on the basis they don't really gain us anything > and prevent use of some other firmware types. Here I guess you know what > your firmware looks like, but I'm still keen to drop it in the interests > of there being fewer places to copy it from. > > It may be a good idea to give this a more specific name as well given > we already have cros-ec-prox as a platform device id from > the cros_ec_light_prox driver. > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > + { .compatible = "google,cros-ec-proximity" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > +#endif > > + > > +static struct platform_driver cros_ec_proximity_driver = { > > + .driver = { > > + .name = "cros-ec-proximity", > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), Add a ACPI match table to match. > > + }, > > + .probe = cros_ec_proximity_probe, > > + .remove = cros_ec_proximity_remove, > > +}; > > +module_platform_driver(cros_ec_proximity_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver"); >
Quoting Jonathan Cameron (2021-01-24 09:38:20) > On Fri, 22 Jan 2021 14:54:43 -0800 > Stephen Boyd <swboyd@chromium.org> wrote: > > > Add support for a ChromeOS EC proximity driver that exposes a "front" > > proximity sensor via the IIO subsystem. The EC decides when front > > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to > > notify the kernel of proximity. Similarly, when proximity detects > > something far away it sets the switch bit to 0. For now this driver > > exposes a single sensor, but it could be expanded in the future via more > > MKBP bits if desired. > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Benson Leung <bleung@chromium.org> > > Cc: Guenter Roeck <groeck@chromium.org> > > Cc: Douglas Anderson <dianders@chromium.org> > > Cc: Gwendal Grignou <gwendal@chromium.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > Hi Stephen, > > Looks more or less fine to me. My main concern is potential confusion > in naming with the cros_ec_prox_light driver that we already have. > > A few other minor bits and bobs inline. > Cool thanks. > > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile > > index 9c1aca1a8b79..b1330dd8e212 100644 > > --- a/drivers/iio/proximity/Makefile > > +++ b/drivers/iio/proximity/Makefile > > @@ -5,6 +5,7 @@ > > > > # When adding new entries keep the list in alphabetical order > > obj-$(CONFIG_AS3935) += as3935.o > > +obj-$(CONFIG_CROS_EC_PROXIMITY) += cros_ec_proximity.o > > obj-$(CONFIG_ISL29501) += isl29501.o > > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o > > obj-$(CONFIG_MB1232) += mb1232.o > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c > > new file mode 100644 > > index 000000000000..a3aef911e3cc > > --- /dev/null > > +++ b/drivers/iio/proximity/cros_ec_proximity.c > > @@ -0,0 +1,252 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for cros-ec proximity sensor exposed through MKBP switch > > + * > > + * Copyright 2021 Google LLC. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/kernel.h> > > Slight preference for alphabetical order though keeping specific > sections separate as done here is fine. Will fix. > > > +#include <linux/notifier.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/types.h> > > + > > +#include <linux/platform_data/cros_ec_commands.h> > > +#include <linux/platform_data/cros_ec_proto.h> > > + > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +#include <asm/unaligned.h> > > + > > +struct cros_ec_proximity_data { > > + struct cros_ec_device *ec; > > + struct iio_dev *indio_dev; > > + struct mutex lock; > > + struct notifier_block notifier; > > + bool enabled; > > +}; > > + > > +static const struct iio_event_spec cros_ec_prox_events[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_EITHER, > > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > > + }, > > +}; > > + > > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = { > > + { > > + .type = IIO_PROXIMITY, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > > + .event_spec = cros_ec_prox_events, > > + .num_event_specs = ARRAY_SIZE(cros_ec_prox_events), > > + }, > > +}; > > + > > +static int cros_ec_proximity_parse_state(const void *data) > > +{ > > + u32 switches = get_unaligned_le32(data); > > + > > + return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY)); > > +} > > + > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) > > +{ > > + struct ec_params_mkbp_info *params; > > + struct cros_ec_command *msg; > > + int ret; > > + > > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), > > + GFP_KERNEL); > > Given this is known at build time, perhaps better to add it to the > iio_priv() accessed structure and avoid having to handle allocations > separately. Ok. > > > + if (!msg) > > + return -ENOMEM; [...] > > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, int *val, > > + int *val2, long mask) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + struct cros_ec_device *ec = data->ec; > > + int ret; > > + > > + if (chan->type != IIO_PROXIMITY) > > + return -EINVAL; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = iio_device_claim_direct_mode(indio_dev); > > Normally we only introduce these protections when adding the ability > to change the state from direct to buffered (which these prevent). > This driver doesn't yet support any other modes so I don't think > there is any benefit in having these. > > If the aim is more local protection then should use a local lock > as the semantics fo these functions might change in future. Alright I'll drop it. > > > > + if (ret) > > + return ret; > > + > > + ret = cros_ec_proximity_query(ec, val); > > + iio_device_release_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + > > + return IIO_VAL_INT; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + > > + return data->enabled; > > +} > > + > > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, int state) > > +{ > > + struct cros_ec_proximity_data *data = iio_priv(indio_dev); > > + > > + mutex_lock(&data->lock); > > + data->enabled = state; > > + mutex_unlock(&data->lock); > > + > > + return 0; > > +} > > + > > +static const struct iio_info cros_ec_proximity_info = { > > + .read_raw = cros_ec_proximity_read_raw, > > + .read_event_config = cros_ec_proximity_read_event_config, > > + .write_event_config = cros_ec_proximity_write_event_config, > > +}; > > + > > +static int cros_ec_proximity_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); > > + struct iio_dev *indio_dev; > > + struct cros_ec_proximity_data *data; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + data = iio_priv(indio_dev); > > + data->ec = ec; > > + data->indio_dev = indio_dev; > > + mutex_init(&data->lock); > > + platform_set_drvdata(pdev, data); > > + > > + indio_dev->name = "cros_ec_proximity"; > > + indio_dev->dev.parent = dev; > > + indio_dev->info = &cros_ec_proximity_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = cros_ec_prox_chan_spec; > > + indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec); > > + > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret) > > + return ret; > > + > > + data->notifier.notifier_call = cros_ec_proximity_notify; > > + ret = blocking_notifier_chain_register(&ec->event_notifier, > > + &data->notifier); > > + if (ret) > > + dev_err(dev, "cannot register notifier: %d\n", ret); > > + > > + return ret; > > +} > > + > > +static int cros_ec_proximity_remove(struct platform_device *pdev) > > +{ > > + struct cros_ec_proximity_data *data = platform_get_drvdata(pdev); > > + struct cros_ec_device *ec = data->ec; > > + > > + blocking_notifier_chain_unregister(&ec->event_notifier, > > + &data->notifier); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_OF > > As a general rule, we are trying to clear out protections on CONFIG_OF etc > and use of of_match_ptr() on the basis they don't really gain us anything > and prevent use of some other firmware types. Here I guess you know what > your firmware looks like, but I'm still keen to drop it in the interests > of there being fewer places to copy it from. > > It may be a good idea to give this a more specific name as well given > we already have cros-ec-prox as a platform device id from > the cros_ec_light_prox driver. Alright. I renamed it to cros_ec_mkbp_proximity throughout this driver. I'm concerned about dropping CONFIG_OF because of_match_ptr() and CONFIG_OF=n makes it unused but I suppose that will be OK as long as compilation passes. > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > + { .compatible = "google,cros-ec-proximity" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > +#endif > > + > > +static struct platform_driver cros_ec_proximity_driver = { > > + .driver = { > > + .name = "cros-ec-proximity", > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > > + }, > > + .probe = cros_ec_proximity_probe, > > + .remove = cros_ec_proximity_remove, > > +}; > > +module_platform_driver(cros_ec_proximity_driver); > > +
Quoting Gwendal Grignou (2021-01-24 13:41:44) > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 22 Jan 2021 14:54:43 -0800 > > Stephen Boyd <swboyd@chromium.org> wrote: > > > > > --- > > > drivers/iio/proximity/Kconfig | 11 + > > > drivers/iio/proximity/Makefile | 1 + > > > drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++ I suppose I'll change this to cros_ec_mkbp_proximity as well. > > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c > > > new file mode 100644 > > > index 000000000000..a3aef911e3cc > > > --- /dev/null > > > +++ b/drivers/iio/proximity/cros_ec_proximity.c > > > @@ -0,0 +1,252 @@ [...] > > > + > > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) > > > +{ > > > + struct ec_params_mkbp_info *params; > > > + struct cros_ec_command *msg; > > > + int ret; > > > + > > > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), > > > + GFP_KERNEL); > > > > Given this is known at build time, perhaps better to add it to the > > iio_priv() accessed structure and avoid having to handle allocations > > separately. > As Jonathan said, it can be preallocated in iio private structure. We > can also use the stack, given the response size is known beforehand. > See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example. I suppose stack is even simpler. I'll try that. > > > + > > > +static int cros_ec_proximity_notify(struct notifier_block *nb, > > > + unsigned long queued_during_suspend, void *_ec) > > > +{ > > > + struct cros_ec_proximity_data *data; > > > + struct cros_ec_device *ec = _ec; > > > + u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK; > > > + void *switches = &ec->event_data.data.switches; > > > + struct iio_dev *indio_dev; > > > + s64 timestamp; > > > + int state, dir; > > > + u64 ev; > > > + > > > + if (event_type == EC_MKBP_EVENT_SWITCH) { > > > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > > > + indio_dev = data->indio_dev; > > > + > > > + mutex_lock(&data->lock); > > > + if (data->enabled) { > > > + timestamp = iio_get_time_ns(indio_dev); > For Android, given the timestamp must be time it happens, not reported > [https://source.android.com/devices/sensors/sensors-hal2] """The > timestamp must be accurate and correspond to the time at which the > event physically happened, not the time it was reported.""", consider > using ec_dev->last_event_time and apply a delta if the iio clock base > is different from CLOCK_BOOTTIME. Ah alright. Is there a reason why cros_ec_get_time_ns() is using boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I suppose some sort of cros_ec API should be exposed to convert the last_event_time to whatever clock base is desired. Does that exist? > > > +static int cros_ec_proximity_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); > > > + struct iio_dev *indio_dev; > > > + struct cros_ec_proximity_data *data; > > > + int ret; > > > + > > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > > + if (!indio_dev) > > > + return -ENOMEM; > > > + > > > + data = iio_priv(indio_dev); > > > + data->ec = ec; > > > + data->indio_dev = indio_dev; > > > + mutex_init(&data->lock); > > > + platform_set_drvdata(pdev, data); > > > + > > > + indio_dev->name = "cros_ec_proximity"; > Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here > and in struct platform_driver cros_ec_proximity_driver. I used dev->driver->name instead. Yay for no define! > > > + indio_dev->dev.parent = dev; > Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc(). Ok. > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > > + { .compatible = "google,cros-ec-proximity" }, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > > +#endif > > > + > > > +static struct platform_driver cros_ec_proximity_driver = { > > > + .driver = { > > > + .name = "cros-ec-proximity", > > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > Add a ACPI match table to match. I don't have an ACPI system in hand. What should the ACPI table look like? Can ACPI use the of_match_table logic? > > > + }, > > > + .probe = cros_ec_proximity_probe, > > > + .remove = cros_ec_proximity_remove, > > > +};
On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Gwendal Grignou (2021-01-24 13:41:44) > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Fri, 22 Jan 2021 14:54:43 -0800 > > > Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > --- > > > > drivers/iio/proximity/Kconfig | 11 + > > > > drivers/iio/proximity/Makefile | 1 + > > > > drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++ > > I suppose I'll change this to cros_ec_mkbp_proximity as well. > > > > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c > > > > new file mode 100644 > > > > index 000000000000..a3aef911e3cc > > > > --- /dev/null > > > > +++ b/drivers/iio/proximity/cros_ec_proximity.c > > > > @@ -0,0 +1,252 @@ > [...] > > > > + > > > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) > > > > +{ > > > > + struct ec_params_mkbp_info *params; > > > > + struct cros_ec_command *msg; > > > > + int ret; > > > > + > > > > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), > > > > + GFP_KERNEL); > > > > > > Given this is known at build time, perhaps better to add it to the > > > iio_priv() accessed structure and avoid having to handle allocations > > > separately. > > As Jonathan said, it can be preallocated in iio private structure. We > > can also use the stack, given the response size is known beforehand. > > See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example. > > I suppose stack is even simpler. I'll try that. > > > > > + > > > > +static int cros_ec_proximity_notify(struct notifier_block *nb, > > > > + unsigned long queued_during_suspend, void *_ec) > > > > +{ > > > > + struct cros_ec_proximity_data *data; > > > > + struct cros_ec_device *ec = _ec; > > > > + u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK; > > > > + void *switches = &ec->event_data.data.switches; > > > > + struct iio_dev *indio_dev; > > > > + s64 timestamp; > > > > + int state, dir; > > > > + u64 ev; > > > > + > > > > + if (event_type == EC_MKBP_EVENT_SWITCH) { > > > > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > > > > + indio_dev = data->indio_dev; > > > > + > > > > + mutex_lock(&data->lock); > > > > + if (data->enabled) { > > > > + timestamp = iio_get_time_ns(indio_dev); > > For Android, given the timestamp must be time it happens, not reported > > [https://source.android.com/devices/sensors/sensors-hal2] """The > > timestamp must be accurate and correspond to the time at which the > > event physically happened, not the time it was reported.""", consider > > using ec_dev->last_event_time and apply a delta if the iio clock base > > is different from CLOCK_BOOTTIME. > > Ah alright. Is there a reason why cros_ec_get_time_ns() is using > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I > suppose some sort of cros_ec API should be exposed to convert the > last_event_time to whatever clock base is desired. Does that exist? CLOCK_BOOTTIME was chosen to be Android compliant, as it includes suspend time and match elapsedRealtime() [see https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()] Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127). In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we need to add a delta, like in cros_ec_sensors_push_data() [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210] > > > > > +static int cros_ec_proximity_probe(struct platform_device *pdev) > > > > +{ > > > > + struct device *dev = &pdev->dev; > > > > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); > > > > + struct iio_dev *indio_dev; > > > > + struct cros_ec_proximity_data *data; > > > > + int ret; > > > > + > > > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > > > + if (!indio_dev) > > > > + return -ENOMEM; > > > > + > > > > + data = iio_priv(indio_dev); > > > > + data->ec = ec; > > > > + data->indio_dev = indio_dev; > > > > + mutex_init(&data->lock); > > > > + platform_set_drvdata(pdev, data); > > > > + > > > > + indio_dev->name = "cros_ec_proximity"; > > Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here > > and in struct platform_driver cros_ec_proximity_driver. > > I used dev->driver->name instead. Yay for no define! > > > > > + indio_dev->dev.parent = dev; > > Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc(). > > Ok. > > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > > > + { .compatible = "google,cros-ec-proximity" }, > > > > + {} > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > > > +#endif > > > > + > > > > +static struct platform_driver cros_ec_proximity_driver = { > > > > + .driver = { > > > > + .name = "cros-ec-proximity", > > > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > > Add a ACPI match table to match. > > I don't have an ACPI system in hand. What should the ACPI table look > like? Can ACPI use the of_match_table logic? AFAIK, ACPI uses .acpi_match_table, see drivers/iio/magnetometer/ak8975.c for a simple example. > > > > > + }, > > > > + .probe = cros_ec_proximity_probe, > > > > + .remove = cros_ec_proximity_remove, > > > > +};
Quoting Gwendal Grignou (2021-01-25 14:28:46) > On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Gwendal Grignou (2021-01-24 13:41:44) > > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > On Fri, 22 Jan 2021 14:54:43 -0800 > > > > Stephen Boyd <swboyd@chromium.org> wrote: > > > > > + if (event_type == EC_MKBP_EVENT_SWITCH) { > > > > > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > > > > > + indio_dev = data->indio_dev; > > > > > + > > > > > + mutex_lock(&data->lock); > > > > > + if (data->enabled) { > > > > > + timestamp = iio_get_time_ns(indio_dev); > > > For Android, given the timestamp must be time it happens, not reported > > > [https://source.android.com/devices/sensors/sensors-hal2] """The > > > timestamp must be accurate and correspond to the time at which the > > > event physically happened, not the time it was reported.""", consider > > > using ec_dev->last_event_time and apply a delta if the iio clock base > > > is different from CLOCK_BOOTTIME. > > > > Ah alright. Is there a reason why cros_ec_get_time_ns() is using > > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I > > suppose some sort of cros_ec API should be exposed to convert the > > last_event_time to whatever clock base is desired. Does that exist? > CLOCK_BOOTTIME was chosen to be Android compliant, as it includes > suspend time and match elapsedRealtime() [see > https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()] > Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME > (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127). > In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we > need to add a delta, like in cros_ec_sensors_push_data() > [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210] The delta may help but what if the clock is adjusted in the time between the event is timestamped and this driver reading the timestamp? That could lead to some odd behavior where the timestamp is in the future because we don't know what sort of adjustment was made, e.g. the realtime clock being moved back in time. I'd rather have a way to request that cros_ec core timestamp the event with some clock base that this driver desires, instead of having to do an offset after the fact. For now I'll use ec_dev->last_event_time because this is only used on chromeos and that should work until userspace is changed, but in the future I think we'll need to have a way for this IIO device to be notified when the clock base changes in iio_device_set_clock() and then have this driver call into cros_ec to request that such a timestamp be made when this event is seen. Or even better have a way to request that cros_ec timestamp the event itself on the EC side, but I don't know if that's possible. > > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > > > > + { .compatible = "google,cros-ec-proximity" }, > > > > > + {} > > > > > +}; > > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > > > > +#endif > > > > > + > > > > > +static struct platform_driver cros_ec_proximity_driver = { > > > > > + .driver = { > > > > > + .name = "cros-ec-proximity", > > > > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > > > Add a ACPI match table to match. > > > > I don't have an ACPI system in hand. What should the ACPI table look > > like? Can ACPI use the of_match_table logic? > AFAIK, ACPI uses .acpi_match_table, see > drivers/iio/magnetometer/ak8975.c for a simple example. Ok. I'm leaning towards punting on this. I don't have an ACPI system to test and I don't know what the ACPI match table should have in it. If you can tell me what to put in the acpi_match_table then I can add it.
On Mon, Jan 25, 2021 at 3:44 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Gwendal Grignou (2021-01-25 14:28:46) > > On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Gwendal Grignou (2021-01-24 13:41:44) > > > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > > On Fri, 22 Jan 2021 14:54:43 -0800 > > > > > Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > + if (event_type == EC_MKBP_EVENT_SWITCH) { > > > > > > + data = container_of(nb, struct cros_ec_proximity_data, notifier); > > > > > > + indio_dev = data->indio_dev; > > > > > > + > > > > > > + mutex_lock(&data->lock); > > > > > > + if (data->enabled) { > > > > > > + timestamp = iio_get_time_ns(indio_dev); > > > > For Android, given the timestamp must be time it happens, not reported > > > > [https://source.android.com/devices/sensors/sensors-hal2] """The > > > > timestamp must be accurate and correspond to the time at which the > > > > event physically happened, not the time it was reported.""", consider > > > > using ec_dev->last_event_time and apply a delta if the iio clock base > > > > is different from CLOCK_BOOTTIME. > > > > > > Ah alright. Is there a reason why cros_ec_get_time_ns() is using > > > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I > > > suppose some sort of cros_ec API should be exposed to convert the > > > last_event_time to whatever clock base is desired. Does that exist? > > CLOCK_BOOTTIME was chosen to be Android compliant, as it includes > > suspend time and match elapsedRealtime() [see > > https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()] > > Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME > > (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127). > > In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we > > need to add a delta, like in cros_ec_sensors_push_data() > > [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210] > > The delta may help but what if the clock is adjusted in the time between > the event is timestamped and this driver reading the timestamp? That > could lead to some odd behavior where the timestamp is in the future > because we don't know what sort of adjustment was made, e.g. the > realtime clock being moved back in time. > > I'd rather have a way to request that cros_ec core timestamp the event > with some clock base that this driver desires, ec_dev->last_event_time is collected outside of the IIO stack, and can be used by multiple drivers. > instead of having to do > an offset after the fact. For now I'll use ec_dev->last_event_time > because this is only used on chromeos and that should work until > userspace is changed, but in the future I think we'll need to have a way > for this IIO device to be notified when the clock base changes in > iio_device_set_clock() and then have this driver call into cros_ec to > request that such a timestamp be made when this event is seen. Or even > better have a way to request that cros_ec timestamp the event itself on > the EC side, but I don't know if that's possible. One way would be use the EC sensor stack that collect such timestamp, but that would be more firmware changes. On second thought, to keep it simple and consistent with other IIO drivers, I suggest to keep using iio_get_time_ns() when the sensor clock is not CLOCK_BOOTTIME, ec_dev->last_event_time when it is. > > > > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = { > > > > > > + { .compatible = "google,cros-ec-proximity" }, > > > > > > + {} > > > > > > +}; > > > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); > > > > > > +#endif > > > > > > + > > > > > > +static struct platform_driver cros_ec_proximity_driver = { > > > > > > + .driver = { > > > > > > + .name = "cros-ec-proximity", > > > > > > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), > > > > Add a ACPI match table to match. > > > > > > I don't have an ACPI system in hand. What should the ACPI table look > > > like? Can ACPI use the of_match_table logic? > > AFAIK, ACPI uses .acpi_match_table, see > > drivers/iio/magnetometer/ak8975.c for a simple example. > > Ok. I'm leaning towards punting on this. I don't have an ACPI system to > test and I don't know what the ACPI match table should have in it. If > you can tell me what to put in the acpi_match_table then I can add it.
Quoting Gwendal Grignou (2021-01-26 12:59:50) > On Mon, Jan 25, 2021 at 3:44 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > instead of having to do > > an offset after the fact. For now I'll use ec_dev->last_event_time > > because this is only used on chromeos and that should work until > > userspace is changed, but in the future I think we'll need to have a way > > for this IIO device to be notified when the clock base changes in > > iio_device_set_clock() and then have this driver call into cros_ec to > > request that such a timestamp be made when this event is seen. Or even > > better have a way to request that cros_ec timestamp the event itself on > > the EC side, but I don't know if that's possible. > One way would be use the EC sensor stack that collect such timestamp, > but that would be more firmware changes. > > On second thought, to keep it simple and consistent with other IIO > drivers, I suggest to keep using iio_get_time_ns() when the sensor > clock is not CLOCK_BOOTTIME, ec_dev->last_event_time when it is. Ok I will make that change and send v3.
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig index 12672a0e89ed..35a04e9ede7d 100644 --- a/drivers/iio/proximity/Kconfig +++ b/drivers/iio/proximity/Kconfig @@ -21,6 +21,17 @@ endmenu menu "Proximity and distance sensors" +config CROS_EC_PROXIMITY + tristate "ChromeOS EC MKBP Proximity sensor" + depends on CROS_EC + help + Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP + switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI) + to use this. + + To compile this driver as a module, choose M here: the + module will be called cros_ec_prox. + config ISL29501 tristate "Intersil ISL29501 Time Of Flight sensor" depends on I2C diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile index 9c1aca1a8b79..b1330dd8e212 100644 --- a/drivers/iio/proximity/Makefile +++ b/drivers/iio/proximity/Makefile @@ -5,6 +5,7 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_AS3935) += as3935.o +obj-$(CONFIG_CROS_EC_PROXIMITY) += cros_ec_proximity.o obj-$(CONFIG_ISL29501) += isl29501.o obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o obj-$(CONFIG_MB1232) += mb1232.o diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c new file mode 100644 index 000000000000..a3aef911e3cc --- /dev/null +++ b/drivers/iio/proximity/cros_ec_proximity.c @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for cros-ec proximity sensor exposed through MKBP switch + * + * Copyright 2021 Google LLC. + */ + +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/kernel.h> +#include <linux/notifier.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include <linux/platform_data/cros_ec_commands.h> +#include <linux/platform_data/cros_ec_proto.h> + +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#include <asm/unaligned.h> + +struct cros_ec_proximity_data { + struct cros_ec_device *ec; + struct iio_dev *indio_dev; + struct mutex lock; + struct notifier_block notifier; + bool enabled; +}; + +static const struct iio_event_spec cros_ec_prox_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_separate = BIT(IIO_EV_INFO_ENABLE), + }, +}; + +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = { + { + .type = IIO_PROXIMITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .event_spec = cros_ec_prox_events, + .num_event_specs = ARRAY_SIZE(cros_ec_prox_events), + }, +}; + +static int cros_ec_proximity_parse_state(const void *data) +{ + u32 switches = get_unaligned_le32(data); + + return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY)); +} + +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state) +{ + struct ec_params_mkbp_info *params; + struct cros_ec_command *msg; + int ret; + + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)), + GFP_KERNEL); + if (!msg) + return -ENOMEM; + + msg->command = EC_CMD_MKBP_INFO; + msg->version = 1; + msg->outsize = sizeof(*params); + msg->insize = sizeof(u32); + params = (struct ec_params_mkbp_info *)msg->data; + params->info_type = EC_MKBP_INFO_CURRENT; + params->event_type = EC_MKBP_EVENT_SWITCH; + + ret = cros_ec_cmd_xfer_status(ec_dev, msg); + if (ret >= 0) { + if (ret != sizeof(u32)) { + dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n", + ret, sizeof(u32)); + ret = -EPROTO; + } else { + *state = cros_ec_proximity_parse_state(msg->data); + ret = 0; + } + } + + kfree(msg); + + return ret; +} + +static int cros_ec_proximity_notify(struct notifier_block *nb, + unsigned long queued_during_suspend, void *_ec) +{ + struct cros_ec_proximity_data *data; + struct cros_ec_device *ec = _ec; + u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK; + void *switches = &ec->event_data.data.switches; + struct iio_dev *indio_dev; + s64 timestamp; + int state, dir; + u64 ev; + + if (event_type == EC_MKBP_EVENT_SWITCH) { + data = container_of(nb, struct cros_ec_proximity_data, notifier); + indio_dev = data->indio_dev; + + mutex_lock(&data->lock); + if (data->enabled) { + timestamp = iio_get_time_ns(indio_dev); + state = cros_ec_proximity_parse_state(switches); + dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING; + + ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0, + IIO_EV_TYPE_THRESH, dir); + iio_push_event(indio_dev, ev, timestamp); + } + mutex_unlock(&data->lock); + } + + return NOTIFY_OK; +} + +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, int *val, + int *val2, long mask) +{ + struct cros_ec_proximity_data *data = iio_priv(indio_dev); + struct cros_ec_device *ec = data->ec; + int ret; + + if (chan->type != IIO_PROXIMITY) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; + + ret = cros_ec_proximity_query(ec, val); + iio_device_release_direct_mode(indio_dev); + if (ret) + return ret; + + return IIO_VAL_INT; + } + + return -EINVAL; +} + +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct cros_ec_proximity_data *data = iio_priv(indio_dev); + + return data->enabled; +} + +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int state) +{ + struct cros_ec_proximity_data *data = iio_priv(indio_dev); + + mutex_lock(&data->lock); + data->enabled = state; + mutex_unlock(&data->lock); + + return 0; +} + +static const struct iio_info cros_ec_proximity_info = { + .read_raw = cros_ec_proximity_read_raw, + .read_event_config = cros_ec_proximity_read_event_config, + .write_event_config = cros_ec_proximity_write_event_config, +}; + +static int cros_ec_proximity_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct cros_ec_device *ec = dev_get_drvdata(dev->parent); + struct iio_dev *indio_dev; + struct cros_ec_proximity_data *data; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + data->ec = ec; + data->indio_dev = indio_dev; + mutex_init(&data->lock); + platform_set_drvdata(pdev, data); + + indio_dev->name = "cros_ec_proximity"; + indio_dev->dev.parent = dev; + indio_dev->info = &cros_ec_proximity_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = cros_ec_prox_chan_spec; + indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + return ret; + + data->notifier.notifier_call = cros_ec_proximity_notify; + ret = blocking_notifier_chain_register(&ec->event_notifier, + &data->notifier); + if (ret) + dev_err(dev, "cannot register notifier: %d\n", ret); + + return ret; +} + +static int cros_ec_proximity_remove(struct platform_device *pdev) +{ + struct cros_ec_proximity_data *data = platform_get_drvdata(pdev); + struct cros_ec_device *ec = data->ec; + + blocking_notifier_chain_unregister(&ec->event_notifier, + &data->notifier); + + return 0; +} + +#ifdef CONFIG_OF +static const struct of_device_id cros_ec_proximity_of_match[] = { + { .compatible = "google,cros-ec-proximity" }, + {} +}; +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match); +#endif + +static struct platform_driver cros_ec_proximity_driver = { + .driver = { + .name = "cros-ec-proximity", + .of_match_table = of_match_ptr(cros_ec_proximity_of_match), + }, + .probe = cros_ec_proximity_probe, + .remove = cros_ec_proximity_remove, +}; +module_platform_driver(cros_ec_proximity_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");
Add support for a ChromeOS EC proximity driver that exposes a "front" proximity sensor via the IIO subsystem. The EC decides when front proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to notify the kernel of proximity. Similarly, when proximity detects something far away it sets the switch bit to 0. For now this driver exposes a single sensor, but it could be expanded in the future via more MKBP bits if desired. Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Benson Leung <bleung@chromium.org> Cc: Guenter Roeck <groeck@chromium.org> Cc: Douglas Anderson <dianders@chromium.org> Cc: Gwendal Grignou <gwendal@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/iio/proximity/Kconfig | 11 + drivers/iio/proximity/Makefile | 1 + drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++ 3 files changed, 264 insertions(+) create mode 100644 drivers/iio/proximity/cros_ec_proximity.c