On 11/01/15 15:10, Tirdea, Irina wrote: > > >> -----Original Message----- >> From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] >> Sent: 01 January, 2015 13:58 >> To: Tirdea, Irina; linux-iio@xxxxxxxxxxxxxxx >> Cc: linux-kernel@xxxxxxxxxxxxxxx; Dogaru, Vlad; Baluta, Daniel; Hartmut Knaack; Lars-Peter Clausen; Peter Meerwald >> Subject: Re: [PATCH 8/8] iio: add driver for Freescale MMA9553 >> >> On 19/12/14 22:57, Irina Tirdea wrote: >>> Add support for Freescale MMA9553L Intelligent Pedometer Platform. >>> >>> The following functionalities are supported: >>> - step counter (counts the number of steps using a HW register) >>> - step detector (generates an iio event at every step the user takes) >>> - activity recognition (rest, walking, jogging, running) >>> - speed >>> - calories >>> - distance >>> >>> To get accurate pedometer results, the user's height, weight and gender >>> need to be configured. >>> >>> The specifications can be downloaded from: >>> http://www.freescale.com/files/sensors/doc/ref_manual/MMA955xLSWRM.pdf >>> http://www.freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf >>> >>> Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx> >> A few bits an bobs beyond the interface discussions in the earlier patches >> in the series. >> >> A nice looking driver on the whole, for a complex little device ;) > > Thanks for the detailed review, Jonathan! > <snip> >>> + mutex_unlock(&data->mutex); >>> + return ret; >>> + default: >>> + return -EINVAL; >>> + } >>> + case IIO_EV_INFO_PERIOD: >>> + switch (chan->type) { >>> + case IIO_STEPS: >>> + if (val < 0 || val > 255) >>> + return -EINVAL; >>> + mutex_lock(&data->mutex); >>> + ret = mma9553_set_config(data, MMA9553_CONF_SPEED_STEP, >>> + &data->conf.speed_step, val, >>> + MMA9553_CONF_STEPCOALESCE); >> So this makes it fire every N steps? Kind of nice, but not quite what period >> is usually used for. We have talked about having an absolute change >> (rather than ROC) event type before - (requires a change of N and doesn't care >> how long it took to happen). Perhaps that makes sense here rather than >> an instance event and a period? > Considering the stepcoalesce option, a change event does make more > sense. I will add IIO_CHANGE and use it instead. > > Adding IIO_CHANGE event type will make IIO_INSTANCE redundant (since > instance is change with a value of 1). I know once an interface is > introduced it cannot be changed, but since nobody is using > IIO_INSTANCE could we remove it? Drop it and don't worry about. ABI changes are only a problem if anyone notices :) >> >>> + mutex_unlock(&data->mutex); >>> + return ret; >>> + default: >>> + return -EINVAL; >>> + } >>> + default: >>> + return -EINVAL; >>> + } >>> +} >>> + >>> +static const struct iio_event_spec mma9553_step_event = { >>> + .type = IIO_EV_TYPE_INSTANCE, >>> + .dir = IIO_EV_DIR_NONE, >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | BIT(IIO_EV_INFO_PERIOD), >>> +}; >>> + >>> +static const struct iio_event_spec mma9553_activity_events[] = { >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_RISING, >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | >>> + BIT(IIO_EV_INFO_VALUE), >>> + }, >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | >>> + BIT(IIO_EV_INFO_VALUE), >>> + }, >>> +}; >>> + >>> +#define MMA9553_PEDOMETER_CHANNEL(_type, _mask) { \ >>> + .type = _type, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) | \ >>> + BIT(IIO_CHAN_INFO_CALIBHEIGHT) | \ >>> + BIT(IIO_CHAN_INFO_CALIBGENDER) | \ >>> + _mask, \ >>> +} >>> + >>> +#define MMA9553_ACTIVITY_CHANNEL(_chan2) { \ >>> + .type = IIO_ACTIVITY, \ >>> + .modified = 1, \ >>> + .channel2 = _chan2, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ >>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT) | \ >>> + BIT(IIO_CHAN_INFO_CALIBGENDER), \ >>> + .event_spec = mma9553_activity_events, \ >>> + .num_event_specs = ARRAY_SIZE(mma9553_activity_events), \ >>> +} >>> + >>> +static const struct iio_chan_spec mma9553_channels[] = { >>> + MMA9551_ACCEL_CHANNEL(IIO_MOD_X), >>> + MMA9551_ACCEL_CHANNEL(IIO_MOD_Y), >>> + MMA9551_ACCEL_CHANNEL(IIO_MOD_Z), >>> + >>> + { >>> + .type = IIO_STEPS, >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >>> + BIT(IIO_CHAN_INFO_ENABLE) | >>> + BIT(IIO_CHAN_INFO_OFFSET) | >>> + BIT(IIO_CHAN_INFO_INT_TIME), >>> + .event_spec = &mma9553_step_event, >>> + .num_event_specs = 1, >>> + }, >>> + >>> + MMA9553_PEDOMETER_CHANNEL(IIO_DISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)), >>> + MMA9553_PEDOMETER_CHANNEL(IIO_SPEED, BIT(IIO_CHAN_INFO_RAW) | >>> + BIT(IIO_CHAN_INFO_SCALE) | >>> + BIT(IIO_CHAN_INFO_INT_TIME)), >>> + MMA9553_PEDOMETER_CHANNEL(IIO_CALORIES, BIT(IIO_CHAN_INFO_RAW) | >>> + BIT(IIO_CHAN_INFO_SCALE) | >>> + BIT(IIO_CHAN_INFO_CALIBWEIGHT)), >>> + >>> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_RUNNING), >>> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_JOGGING), >>> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_WALKING), >>> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_STILL), >>> +}; >>> + >>> +static const struct iio_info mma9553_info = { >>> + .driver_module = THIS_MODULE, >>> + .read_raw = mma9553_read_raw, >>> + .write_raw = mma9553_write_raw, >>> + .read_event_config = mma9553_read_event_config, >>> + .write_event_config = mma9553_write_event_config, >>> + .read_event_value = mma9553_read_event_value, >>> + .write_event_value = mma9553_write_event_value, >>> +}; >>> + >> Only one copy of this? We don't want to prevent people having more >> than one of these devices attached to a given machine. >> Hence by all means have a default version of this but then >> clone it into the mma9553_data structure for manipulation. >> Or have a mached array of booleans in there (same indexes) >> to use for the "enabled"s. >> > Will fix this. > >>> +static struct mma9553_event mma9553_events[] = { >>> + { >>> + .type = IIO_STEPS, >>> + .mod = IIO_NO_MOD, >>> + .dir = IIO_EV_DIR_NONE, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_STILL, >>> + .dir = IIO_EV_DIR_RISING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_STILL, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_WALKING, >>> + .dir = IIO_EV_DIR_RISING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_WALKING, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_JOGGING, >>> + .dir = IIO_EV_DIR_RISING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_JOGGING, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_RUNNING, >>> + .dir = IIO_EV_DIR_RISING, >>> + .enabled = false, >>> + }, >>> + { >>> + .type = IIO_ACTIVITY, >>> + .mod = IIO_MOD_RUNNING, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .enabled = false, >>> + }, >>> +}; >>> + >>> +static irqreturn_t mma9553_irq_handler(int irq, void *private) >>> +{ >>> + /* >>> + * Since we only configure the interrupt pin when an >>> + * event is enabled, we are sure we have at least >>> + * one event enabled at this point. >>> + */ >> IIRC simply not supplyling the top half handler has the same effect as >> this (though then you don't have anywhere to put the comment!) >> Actually - I'd be tempted to grab and stash a timestamp in here to >> increase the precision of you step timing etc. > Initially I used a NULL pointer instead of mma9553_irq_handler in devm_request_threaded_irq, but the registration failed with "Threaded irq requested with handler=NULL and !ONESHOT". > However, it is better to grab the timestamp anyway so I will do that here. > >>> + return IRQ_WAKE_THREAD; >>> +} >>> + >>> +static irqreturn_t mma9553_event_handler(int irq, void *private) >>> +{ >>> + struct iio_dev *indio_dev = private; >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + u16 stepcnt; >>> + u8 activity; >>> + struct mma9553_event *ev_activity, *ev_prev_activity, *ev_step_detect; >>> + int ret; >>> + >>> + mutex_lock(&data->mutex); >>> + ret = mma9553_read_activity_stepcnt(data, &activity, &stepcnt); >>> + if (ret < 0) { >>> + mutex_unlock(&data->mutex); >>> + return IRQ_HANDLED; >>> + } >>> + >>> + ev_prev_activity = >>> + mma9553_get_event(data, IIO_ACTIVITY, >>> + mma9553_activity_to_mod(data->activity), >>> + IIO_EV_DIR_FALLING); >>> + ev_activity = >>> + mma9553_get_event(data, IIO_ACTIVITY, >>> + mma9553_activity_to_mod(activity), >>> + IIO_EV_DIR_RISING); >>> + ev_step_detect = >>> + mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE); >>> + >>> + if (ev_step_detect->enabled && (stepcnt != data->stepcnt)) { >>> + data->stepcnt = stepcnt; >>> + iio_push_event(indio_dev, >>> + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD, >>> + IIO_EV_DIR_NONE, IIO_EV_TYPE_INSTANCE, 0, 0, 0), >>> + iio_get_time_ns()); >> Worth grabbing the timestamp earlier than this for precision reasons? >>> + } >>> + >>> + if (activity != data->activity) { >>> + data->activity = activity; >>> + /* ev_activity can be NULL if activity == ACTIVITY_UNKNOWN */ >>> + if (ev_prev_activity && ev_prev_activity->enabled) >>> + iio_push_event(indio_dev, >>> + IIO_EVENT_CODE(IIO_ACTIVITY, 0, >>> + ev_prev_activity->mod, >>> + IIO_EV_DIR_FALLING, >>> + IIO_EV_TYPE_THRESH, 0, 0, 0), >>> + iio_get_time_ns()); >>> + >>> + if (ev_activity && ev_activity->enabled) >>> + iio_push_event(indio_dev, >>> + IIO_EVENT_CODE(IIO_ACTIVITY, 0, >>> + ev_activity->mod, >>> + IIO_EV_DIR_RISING, >>> + IIO_EV_TYPE_THRESH, 0, 0, 0), >>> + iio_get_time_ns()); >>> + } >>> + mutex_unlock(&data->mutex); >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int mma9553_gpio_probe(struct i2c_client *client) >>> +{ >>> + struct device *dev; >>> + struct gpio_desc *gpio; >>> + int ret; >>> + >>> + if (!client) >>> + return -EINVAL; >>> + >>> + dev = &client->dev; >>> + >>> + /* data ready gpio interrupt pin */ >>> + gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0); >>> + if (IS_ERR(gpio)) { >>> + dev_err(dev, "acpi gpio get index failed\n"); >>> + return PTR_ERR(gpio); >>> + } >>> + >>> + ret = gpiod_direction_input(gpio); >>> + if (ret) >>> + return ret; >>> + >>> + ret = gpiod_to_irq(gpio); >>> + >>> + dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret); >>> + >>> + return ret; >>> +} >>> + >>> +static const char *mma9553_match_acpi_device(struct device *dev) >>> +{ >>> + const struct acpi_device_id *id; >>> + >>> + id = acpi_match_device(dev->driver->acpi_match_table, dev); >>> + if (!id) >>> + return NULL; >>> + >>> + return dev_name(dev); >>> +} >>> + >>> +static int mma9553_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + struct mma9553_data *data; >>> + struct iio_dev *indio_dev; >>> + const char *name = NULL; >>> + int ret; >>> + >>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>> + if (!indio_dev) >>> + return -ENOMEM; >>> + >>> + data = iio_priv(indio_dev); >>> + i2c_set_clientdata(client, indio_dev); >>> + data->client = client; >>> + >>> + if (id) >>> + name = id->name; >>> + else if (ACPI_HANDLE(&client->dev)) >>> + name = mma9553_match_acpi_device(&client->dev); >>> + else >> Interesting to note the original driver doesn't have this else. Worth >> checking? > There was a similar check in the initial driver (it checked for name == NULL instead) that somehow got removed in a later version. > I could add the check in mma9551 as well, but I am not sure on what is the proper way to handle this. > There is an ongoing discussion on this: http://www.spinics.net/lists/linux-iio/msg16231.html. > I would rather for this to be cleared out and send a separate patch to fix both drivers if needed. > >>> + return -ENOSYS; >>> + >>> + mutex_init(&data->mutex); >>> + data->events = mma9553_events; >>> + data->num_events = ARRAY_SIZE(mma9553_events); >>> + >>> + ret = mma9553_init(data); >>> + if (ret < 0) >>> + return ret; >>> + >>> + indio_dev->dev.parent = &client->dev; >>> + indio_dev->channels = mma9553_channels; >>> + indio_dev->num_channels = ARRAY_SIZE(mma9553_channels); >>> + indio_dev->name = name; >>> + indio_dev->modes = INDIO_DIRECT_MODE; >>> + indio_dev->info = &mma9553_info; >>> + >>> + if (client->irq < 0) >>> + client->irq = mma9553_gpio_probe(client); >>> + >> So this time we just have a single irq. That makes life simpler! > Yes, mma9553 provides the option to combine all its interrupts in one status bit (while mma9551 does not). > >>> + if (client->irq >= 0) { >>> + ret = devm_request_threaded_irq(&client->dev, client->irq, >>> + mma9553_irq_handler, >>> + mma9553_event_handler, >>> + IRQF_TRIGGER_RISING, >>> + MMA9553_IRQ_NAME, indio_dev); >>> + if (ret < 0) { >>> + dev_err(&client->dev, "request irq %d failed\n", >>> + client->irq); >>> + goto out_poweroff; >>> + } >>> + >>> + } >>> + >>> + ret = iio_device_register(indio_dev); >>> + if (ret < 0) { >>> + dev_err(&client->dev, "unable to register iio device\n"); >>> + goto out_poweroff; >>> + } >>> + >>> + ret = pm_runtime_set_active(&client->dev); >>> + if (ret < 0) >>> + goto out_iio_unregister; >>> + >>> + pm_runtime_enable(&client->dev); >>> + pm_runtime_set_autosuspend_delay(&client->dev, >>> + MMA9551_AUTO_SUSPEND_DELAY_MS); >>> + pm_runtime_use_autosuspend(&client->dev); >>> + >>> + dev_dbg(&indio_dev->dev, "Registered device %s\n", name); >>> + return 0; >>> + >>> +out_iio_unregister: >>> + iio_device_unregister(indio_dev); >>> +out_poweroff: >>> + mma9551_set_device_state(client, false); >>> + return ret; >>> +} >>> + >>> +static int mma9553_remove(struct i2c_client *client) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + >>> + pm_runtime_disable(&client->dev); >>> + pm_runtime_set_suspended(&client->dev); >>> + pm_runtime_put_noidle(&client->dev); >>> + >>> + iio_device_unregister(indio_dev); >>> + mutex_lock(&data->mutex); >>> + mma9551_set_device_state(data->client, false); >>> + mutex_unlock(&data->mutex); >>> + >>> + return 0; >>> +} >>> + >>> +#ifdef CONFIG_PM >>> +static int mma9553_runtime_suspend(struct device *dev) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + mutex_lock(&data->mutex); >>> + ret = mma9551_set_device_state(data->client, false); >>> + mutex_unlock(&data->mutex); >>> + if (ret < 0) { >>> + dev_err(&data->client->dev, "powering off device failed\n"); >>> + return -EAGAIN; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int mma9553_runtime_resume(struct device *dev) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + ret = mma9551_set_device_state(data->client, true); >>> + if (ret < 0) >>> + return ret; >>> + >>> + mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE); >>> + return 0; >>> +} >>> +#endif >>> + >>> +#ifdef CONFIG_PM_SLEEP >>> +static int mma9553_suspend(struct device *dev) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + mutex_lock(&data->mutex); >>> + ret = mma9551_set_device_state(data->client, false); >>> + mutex_unlock(&data->mutex); >>> + >>> + return ret; >>> +} >>> + >>> +static int mma9553_resume(struct device *dev) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >>> + struct mma9553_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + mutex_lock(&data->mutex); >>> + ret = mma9551_set_device_state(data->client, true); >>> + mutex_unlock(&data->mutex); >>> + >>> + return ret; >>> +} >>> +#endif >>> + >>> +static const struct dev_pm_ops mma9553_pm_ops = { >>> + SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume) >>> + SET_RUNTIME_PM_OPS(mma9553_runtime_suspend, >>> + mma9553_runtime_resume, NULL) >>> +}; >>> + >>> +static const struct acpi_device_id mma9553_acpi_match[] = { >>> + {"MMA9553", 0}, >>> + {}, >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match); >>> + >>> +static const struct i2c_device_id mma9553_id[] = { >>> + {"mma9553", 0}, >>> + {}, >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(i2c, mma9553_id); >>> + >>> +static struct i2c_driver mma9553_driver = { >>> + .driver = { >>> + .name = MMA9553_DRV_NAME, >>> + .acpi_match_table = ACPI_PTR(mma9553_acpi_match), >>> + .pm = &mma9553_pm_ops, >>> + }, >>> + .probe = mma9553_probe, >>> + .remove = mma9553_remove, >>> + .id_table = mma9553_id, >>> +}; >>> + >>> +module_i2c_driver(mma9553_driver); >>> + >>> +MODULE_AUTHOR("Irina Tirdea <irina.tirdea@xxxxxxxxx>"); >>> +MODULE_LICENSE("GPL v2"); >>> +MODULE_DESCRIPTION("MMA9553L pedometer platform driver"); >>> > -- 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
