Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                      |    2 +
>  drivers/input/misc/Kconfig       |   33 +
>  drivers/input/misc/Makefile      |    3 +
>  drivers/input/misc/cs40l50-i2c.c |   67 ++
>  drivers/input/misc/cs40l50-spi.c |   67 ++
>  drivers/input/misc/cs40l50.c     | 1008 ++++++++++++++++++++++++++++++
>  include/linux/input/cs40l50.h    |  321 ++++++++++

Is this part not going to support streaming at some point? Fine
if not, but it seems likely to me and as such we should probably
follow the l26 stuff and make this an MFD from the start, even if
we haven't implemented the audio bits.

> +
> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
> +{
> +
> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
> +{

These pseq and OWT bits, could they be shared with l26?
Definitely worth syncing with those guys, my assumption is the
wavetable/pseq won't have changed much and it might be nice to
factor these bits out into some library code that both drivers
can use.

> +err_free:
> +	if (is_new)
> +		error ? kfree(new_effect) : list_add(&new_effect->list, &cs40l50->effect_head);

This is a bit of an excessive use for a ternary, just make it an
if.

> +static int cs40l50_erase_effect(struct input_dev *dev, int effect_id)
> +{
> +	int error = 0;
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +	struct cs40l50_effect *effect, *effect_owt;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	effect = cs40l50_find_effect(cs40l50, dev->ff->effects[effect_id].id);
> +	if (!effect) {
> +		error = -EINVAL;
> +		goto err_mutex;
> +	}
> +
> +	if (effect->mapping != CS40L50_GPIO_MAPPING_INVALID) {
> +		error = cs40l50_dsp_write(cs40l50, effect->mapping, CS40L50_GPI_DISABLE);
> +		if (error)
> +			goto err_mutex;
> +	}
> +
> +	if (effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1,
> +				CS40L50_MBOX_CMD_OWT_DELETE | effect->trigger_index);
> +		if (error)
> +			goto err_mutex;
> +
> +		list_for_each_entry(effect_owt, &cs40l50->effect_head, list) {
> +			if (effect_owt->wvfrm_bank == CS40L50_WVFRM_BANK_OWT &&
> +					effect_owt->trigger_index > effect->trigger_index)
> +				effect_owt->trigger_index--;
> +		}
> +	}
> +
> +	list_del(&effect->list);
> +	kfree(effect);
> +err_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return error;
> +}

I seem to remember add/erase needed to get pushed to the work
queue too, not because the framework might call it in atomic
context, but in order to ensure ordering with the start/stops.

> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	int error, i, irq;
> +	u32 val;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> +			cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio)) {
> +		error = PTR_ERR(cs40l50->reset_gpio);
> +		goto err;
> +	}
> +
> +	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> +		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> +		goto err;
> +	}
> +
> +	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> +	if (!cs40l50->vibe_workqueue) {
> +		error = -ENOMEM;
> +		goto err;
> +	}
> +
> +	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> +	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> +	error = cs40l50_cs_dsp_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_input_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_patch_firmware(cs40l50);
> +	if (error)
> +		goto err;
> +

Doing this from probe, with a sync firmware load, can be a
little problematic as the firmware is often not available if
the driver is builtin rather than a module.

Fairly quick review as I am on holiday at the moment, but will
check back next week.

Thanks,
Charles