Quoting Gwendal Grignou (2021-01-24 13:41:44)

On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron [off-list ref] wrote:

quoted

On Fri, 22 Jan 2021 14:54:43 -0800
Stephen Boyd [off-list ref] wrote:

quoted

---
 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.

quoted

quoted

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 @@

[...]

quoted

quoted

+
+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.

quoted

quoted

+
+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?

quoted

quoted

+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!

quoted

quoted

+     indio_dev->dev.parent = dev;

Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc().

Ok.

quoted

quoted

+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?

quoted

quoted

+     },
+     .probe = cros_ec_proximity_probe,
+     .remove = cros_ec_proximity_remove,
+};