Received-SPF: pass (google.com: domain of linux-kernel+bounces-18761-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 6 Jan 2024 17:58:43 -0800
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: James Ogletree <jogletre@opensource.cirrus.com>
Cc: James Ogletree <james.ogletree@cirrus.com>,
	Fred Treven <fred.treven@cirrus.com>,
	Ben Bright <ben.bright@cirrus.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Simon Trimmer <simont@opensource.cirrus.com>,
	Charles Keepax <ckeepax@opensource.cirrus.com>,
	Richard Fitzgerald <rf@opensource.cirrus.com>,
	Lee Jones <lee@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	James Schulman <james.schulman@cirrus.com>,
	David Rhodes <david.rhodes@cirrus.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Peng Fan <peng.fan@nxp.com>, Jeff LaBundy <jeff@labundy.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	Jacky Bai <ping.bai@nxp.com>,
	Weidong Wang <wangweidong.a@awinic.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Herve Codina <herve.codina@bootlin.com>,
	Shuming Fan <shumingf@realtek.com>,
	Shenghao Ding <13916275206@139.com>,
	Ryan Lee <ryans.lee@analog.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	"open list:CIRRUS LOGIC HAPTIC DRIVERS" <patches@opensource.cirrus.com>,
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." <linux-input@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <linux-sound@vger.kernel.org>,
	"moderated list:CIRRUS LOGIC AUDIO CODEC DRIVERS" <alsa-devel@alsa-project.org>
Subject: Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50
 haptic driver
Message-ID: <ZZoFUwOEF6NByIp2@google.com>
References: <20240104223643.876292-1-jogletre@opensource.cirrus.com>
 <20240104223643.876292-5-jogletre@opensource.cirrus.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240104223643.876292-5-jogletre@opensource.cirrus.com>

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.

-- 
Dmitry