Hi James,
On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
+static int vibra_add(struct input_dev *dev, struct ff_effect *effect,
struct ff_effect *old)+{
- struct vibra_info *info = input_get_drvdata(dev);
- u32 len = effect->u.periodic.custom_len;
- if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
dev_err(info->dev, "Type (%#X) or waveform (%#X) unsupported\n",effect->type, effect->u.periodic.waveform);return -EINVAL;- }
- memcpy(&info->add_effect, effect, sizeof(struct ff_effect));
structures can be assigned, no need for memcpy.
- info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
- if (!info->add_effect.u.periodic.custom_data)
return -ENOMEM;- if (copy_from_user(info->add_effect.u.periodic.custom_data,
effect->u.periodic.custom_data, sizeof(s16) * len)) {info->add_error = -EFAULT;goto out_free;- }
- queue_work(info->vibe_wq, &info->add_work);
- flush_work(&info->add_work);
I do not understand the need of scheduling a work here. You are obviously in a sleeping context (otherwise you would not be able to execute flush_work()) so you should be able to upload the effect right here.
...
+static int vibra_playback(struct input_dev *dev, int effect_id, int val) +{
- struct vibra_info *info = input_get_drvdata(dev);
- if (val > 0) {
value is supposed to signal how many times an effect should be repeated. It looks like you are not handling this at all.
info->start_effect = &dev->ff->effects[effect_id];queue_work(info->vibe_wq, &info->vibe_start_work);
The API allows playback of several effects at once, the way you have it done here if multiple requests come at same time only one will be handled.
- } else {
queue_work(info->vibe_wq, &info->vibe_stop_work);
Which effect are you stopping? All of them? You need to stop a particular one.
- }
Essentially you need a queue of requests and a single work handling all of them...
...
+static int cs40l50_vibra_probe(struct platform_device *pdev) +{
- struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
- struct vibra_info *info;
- int error;
- info = devm_kzalloc(pdev->dev.parent, sizeof(*info), GFP_KERNEL);
- if (!info)
return -ENOMEM;- info->dev = cs40l50->dev;
- info->regmap = cs40l50->regmap;
- info->input = devm_input_allocate_device(info->dev);
- if (!info->input)
return -ENOMEM;- info->input->id.product = cs40l50->devid & 0xFFFF;
- info->input->id.version = cs40l50->revid;
- info->input->name = "cs40l50_vibra";
- input_set_drvdata(info->input, info);
- input_set_capability(info->input, EV_FF, FF_PERIODIC);
- input_set_capability(info->input, EV_FF, FF_CUSTOM);
- error = input_ff_create(info->input, FF_MAX_EFFECTS);
- if (error) {
dev_err(info->dev, "Failed to create input device\n");return error;- }
- info->input->ff->upload = vibra_add;
- info->input->ff->playback = vibra_playback;
- info->input->ff->erase = vibra_erase;
- INIT_LIST_HEAD(&info->effect_head);
- info->dsp = cs40l50_dsp;
- info->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0);
- if (!info->vibe_wq)
return -ENOMEM;- error = devm_add_action_or_reset(info->dev, vibra_remove_wq, info);
- if (error)
return error;
Why do you need a dedicated workqueue? So you can flush works?
- mutex_init(&info->lock);
- INIT_WORK(&info->vibe_start_work, vibra_start_worker);
- INIT_WORK(&info->vibe_stop_work, vibra_stop_worker);
- INIT_WORK(&info->erase_work, vibra_erase_worker);
- INIT_WORK(&info->add_work, vibra_add_worker);
- error = input_register_device(info->input);
- if (error) {
dev_err(info->dev, "Failed to register input device\n");input_free_device(info->input);
Not needed, you are using devm_input_allocate_device().
return error;- }
- return devm_add_action_or_reset(info->dev, vibra_input_unregister,
info->input);
Not needed, managed input devices will be unregistered automatically by devm.
Thanks.