Re: [RFC PATCH 05/14] iio/accel: add driver for apple SMS

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 29 Oct 2026 04:52:02 -0400
Atharva Tiwari <atharvatiwarilinuxdev@gmail.com> wrote:

> Add a driver for the Sudden Motion Sensor accelerometer,
> exposed through IIO. It reports the three axes and also
> drop, orientation and shock events.
> 
> This replaces the motion sensor support in applesmc.
> 
> Signed-off-by: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
A few specific comments inline.

> diff --git a/drivers/iio/accel/macsmc-accel.c b/drivers/iio/accel/macsmc-accel.c
> new file mode 100644
> index 000000000..0abf81e7b
> --- /dev/null
> +++ b/drivers/iio/accel/macsmc-accel.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC sudden motion sensor (SMS) driver
> + *
> + * Copyright (C) 2026 Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>
> + *
> + * Based on drivers/hwmon/applesmc.c:
> + * Copyright (C) 2007 Nicolas Boichat <nicolas@boichat.ch>
> + * Copyright (C) 2010 Henrik Rydberg <rydberg@euromail.se>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/macsmc.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +
> +struct macsmc_accel {
> +	struct apple_smc *smc;
> +	struct iio_dev *indio_dev;
> +	struct notifier_block nb;
> +};
> +
> +enum {
> +	SMS_DROP = 0,

Novel ABI, which means unused ABI given no tools will have
a clue.  Look at other drivers and the comments below for how
to do this.

> +	SMS_ORIENTATION,
> +	SMS_SHOCK,
> +};
> +
> +static int macsmc_accel_read(struct macsmc_accel *drv, smc_key key, s16 *p)
> +{
> +	int ret;
> +
> +	ret = apple_smc_read_s16(drv->smc, key, p);
> +	if (ret < 0)
> +		return ret;
> +
> +	*p = (s16)be16_to_cpu(*p);
> +	return 0;
> +}
> +
> +static int macsmc_accel_dev_init(struct macsmc_accel *drv)
> +{
> +	int ret, total;
> +	u16 buf;
> +
> +	for (total = 0; total < 100; total++) {
> +		ret = apple_smc_read_u16(drv->smc, SMC_KEY(MOCN), &buf);
> +		if (!ret && buf)
> +			return 0;
> +
> +		buf = 0xE0;

Some define in this file to give that magic value some meaning (if we
have any info) If not, add comment to say this was from the older
driver (and any info that had on where the magic value came from).

> +		apple_smc_write_u16(drv->smc, SMC_KEY(MOCN), buf);
> +		msleep(50);
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int macsmc_accel_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +		int *val, int *val2, long mask)
> +{
> +	struct macsmc_accel *drv = iio_priv(indio_dev);
> +	int ret;
> +	s16 buf;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		ret = macsmc_accel_read(drv, SMC_KEY(MO_X), &buf);

I'd just have the switch set a local variable for SMC_KEY(whatever)
and then unify the actual read after the switch statement.

> +		if (ret < 0)
> +			return ret;
> +
> +		*val = buf;
> +		return IIO_VAL_INT;
> +	case IIO_MOD_Y:
> +		ret = macsmc_accel_read(drv, SMC_KEY(MO_Y), &buf);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = buf;
> +		return IIO_VAL_INT;
> +	case IIO_MOD_Z:
> +		ret = macsmc_accel_read(drv, SMC_KEY(MO_Z), &buf);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = buf;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int macsmc_accel_event(struct notifier_block *nb, unsigned long event, void *data)
> +{
> +	struct macsmc_accel *drv = container_of(nb, struct macsmc_accel, nb);
> +	u64 code;
> +
> +	/* check if event is for us */
> +	if (event != 0x60 && event != 0x69 && event != 0x6F)

As in previous driver, these should be defines in some header.

> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case 0x60:
> +		code = IIO_UNMOD_EVENT_CODE(IIO_ACCEL, SMS_DROP,

These need to be more informative to be useful to userspace.
See below for discussion of the channels.

> +					    IIO_EV_TYPE_CHANGE,
> +					    IIO_EV_DIR_NONE);
> +		break;
> +	case 0x69:
> +		code = IIO_UNMOD_EVENT_CODE(IIO_ACCEL, SMS_ORIENTATION,
> +					    IIO_EV_TYPE_CHANGE,
> +					    IIO_EV_DIR_NONE);
> +		break;
> +	case 0x6F:
> +		code = IIO_UNMOD_EVENT_CODE(IIO_ACCEL, SMS_SHOCK,
> +					    IIO_EV_TYPE_CHANGE,
> +					    IIO_EV_DIR_NONE);
> +		break;
> +	}
> +
> +	iio_push_event(drv->indio_dev, code, iio_get_time_ns(drv->indio_dev));
> +	return NOTIFY_OK;
> +}

> +
> +static const struct iio_event_spec macsmc_accel_event_spec = {
> +	.type = IIO_EV_TYPE_CHANGE,
> +	.dir = IIO_EV_DIR_NONE,
> +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
> +static const struct iio_chan_spec macsmc_accel_channels[] = {
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_X,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Y,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Z,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_DROP,

Drop sounds like a typically falling threshold on all axis
event?  Lots of drivers support these via the slightly unusual
looking X&Y&Z modifier.

> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_ORIENTATION,

This one needs some more info.  What is it about the acceleration
values that leads to this event?  Do we have any info on that?

> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_SHOCK,

That's normally an over threshold on X|Y|Z. See how other
drivers do this.

> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +};