| Message ID | 20231018175726.3879955-3-james.ogletree@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for CS40L50 | expand |
Hi James, Excellent work as always! On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote: > From: James Ogletree <james.ogletree@cirrus.com> > > Introduce the cirrus haptics library which factors out > common haptics operations used by Cirrus Logic Input > drivers. > > Signed-off-by: James Ogletree <james.ogletree@cirrus.com> > --- > MAINTAINERS | 2 + > drivers/input/misc/cirrus_haptics.c | 586 +++++++++++++++++++++++++++ > include/linux/input/cirrus_haptics.h | 121 ++++++ > 3 files changed, 709 insertions(+) > create mode 100644 drivers/input/misc/cirrus_haptics.c > create mode 100644 include/linux/input/cirrus_haptics.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 28f0ca9324b3..57daf77bf550 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4970,6 +4970,8 @@ M: Ben Bright <ben.bright@cirrus.com> > L: patches@opensource.cirrus.com > S: Supported > F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml > +F: drivers/input/misc/cirrus* > +F: include/linux/input/cirrus* > > CIRRUS LOGIC DSP FIRMWARE DRIVER > M: Simon Trimmer <simont@opensource.cirrus.com> > diff --git a/drivers/input/misc/cirrus_haptics.c b/drivers/input/misc/cirrus_haptics.c > new file mode 100644 > index 000000000000..7e539cd45167 > --- /dev/null > +++ b/drivers/input/misc/cirrus_haptics.c > @@ -0,0 +1,586 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Helper functions for dealing with wavetable > + * formats and DSP interfaces used by Cirrus > + * haptic drivers. > + * > + * Copyright 2023 Cirrus Logic, Inc. > + */ > + > +#include <linux/firmware/cirrus/cs_dsp.h> > +#include <linux/input.h> > +#include <linux/input/cirrus_haptics.h> > +#include <linux/pm_runtime.h> > + > +static int cs_hap_pseq_init(struct cs_hap *haptics) > +{ > + struct cs_hap_pseq_op *op; > + int error, i, num_words; > + u8 operation; > + u32 *words; > + > + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg) > + return 0; > + > + INIT_LIST_HEAD(&haptics->pseq_head); Anything that allocates or initializes an element that is normally held in a driver's private data, like a list head or mutex, belongs in probe() in my opinion. It's less of an issue here, but for more complex cases where we may set something up in probe() and tear it down in remove(), the driver is easier to maintain if helper functions such as cs_hap_pseq_init() only manipulate or organize the data, rather than absorb additional work. > + > + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); > + if (!words) > + return -ENOMEM; > + > + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, > + words, haptics->dsp.pseq_size); > + if (error) > + goto err_free; > + > + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { > + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); > + switch (operation) { > + case PSEQ_OP_END: > + case PSEQ_OP_WRITE_UNLOCK: > + num_words = PSEQ_OP_END_WORDS; > + break; > + case PSEQ_OP_WRITE_ADDR8: > + case PSEQ_OP_WRITE_H16: > + case PSEQ_OP_WRITE_L16: > + num_words = PSEQ_OP_WRITE_X16_WORDS; > + break; > + case PSEQ_OP_WRITE_FULL: > + num_words = PSEQ_OP_WRITE_FULL_WORDS; > + break; > + default: > + error = -EINVAL; > + dev_err(haptics->dev, "Unsupported op: %u\n", operation); > + goto err_free; > + } > + > + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); > + if (!op) { > + error = -ENOMEM; > + goto err_free; > + } > + > + op->size = num_words * sizeof(u32); > + memcpy(op->words, &words[i], op->size); > + op->offset = i * sizeof(u32); > + op->operation = operation; > + list_add(&op->list, &haptics->pseq_head); > + > + if (operation == PSEQ_OP_END) > + break; > + } > + > + if (operation != PSEQ_OP_END) > + error = -ENOENT; > + > +err_free: > + kfree(words); > + > + return error; > +} My first thought as I reviewed this patch was that this and the lot of pseq-related functions are not necessarily related to haptics, but rather CS40L50 register access and housekeeping in general. I seem to recall on L25 and friends that the the power-on sequencer, i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory that can play back a series of address/data pairs when the device comes out of hibernation, and any registers written during runtime must also be mirrored to the PSEQ for "playback" later. Is that still the case here? Assuming so, these functions seem like they belong in the MFD, not an input-specific library, because they will presumably be used by the codec driver as well, since that driver will write registers to set BCLK/LRCK ratio, etc. Therefore, I think it makes more sense for these functions to move to the MFD, which can then export them for use by the input/FF and codec children. This leaves cirrus_haptics.* with only a few functions related to starting and stopping work, which seem specific enough to just live in cs40l50-vibra.c. Presumably many of those could be re-used by the L30 down the road, but in that case I think we'd be looking to actually re-use the L50 driver and simply add a compatible string for L30. I recall L30 has some overhead that L50 does not, which may make it hairy for cs40l50-vibra.c to support both. But in that case, you could always fork a cs40l30-vibra.c with its own compatible string, then have the L50 MFD selectively load the correct child based on device ID. To summarize, we should have: * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery, IRQ handling, exported PSEQ functions, etc. * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from the MFD. * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop work, also uses PSEQ library from the MFD. And down the road, depending on complexity, maybe we also have: * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that includes other functionality that didn't really fit in cs40l50-vibra.c; also uses PSEQ library from, and is loaded by, the original L50 MFD. If this driver duplicates small bits of cs40l50-vibra.c, it's not the end of the world. All of these files would #include include/linux/mfd/cs40l50.h. And finally, cirrus_haptics.* simply go away. Same idea, just slightly more scalable, and closer to common design patterns. > + > +static int cs_hap_pseq_find_end(struct cs_hap *haptics, > + struct cs_hap_pseq_op **op_end) > +{ > + u8 operation = PSEQ_OP_WRITE_FULL; > + struct cs_hap_pseq_op *op; > + > + list_for_each_entry(op, &haptics->pseq_head, list) { > + operation = op->operation; > + if (operation == PSEQ_OP_END) > + break; > + } > + > + if (operation != PSEQ_OP_END) { > + dev_err(haptics->dev, "Missing PSEQ list terminator\n"); > + return -ENOENT; > + } > + > + *op_end = op; > + > + return 0; > +} > + > +static struct cs_hap_pseq_op *cs_hap_pseq_find_op(struct cs_hap_pseq_op *match_op, > + struct list_head *pseq_head) > +{ > + struct cs_hap_pseq_op *op; > + > + list_for_each_entry(op, pseq_head, list) { > + if (op->operation == PSEQ_OP_END) > + break; Nit: a line break here makes this easier to read IMO. > + if (op->operation != match_op->operation || > + op->words[0] != match_op->words[0]) > + continue; And here. > + switch (op->operation) { > + case PSEQ_OP_WRITE_FULL: > + if (FIELD_GET(GENMASK(23, 8), op->words[1]) == > + FIELD_GET(GENMASK(23, 8), match_op->words[1])) > + return op; > + break; > + case PSEQ_OP_WRITE_H16: > + case PSEQ_OP_WRITE_L16: > + if (FIELD_GET(GENMASK(23, 16), op->words[1]) == > + FIELD_GET(GENMASK(23, 16), match_op->words[1])) > + return op; > + break; > + default: > + break; > + } > + } > + > + return NULL; > +} > + > +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, > + u32 data, bool update, u8 op_code) > +{ > + struct cs_hap_pseq_op *op, *op_end, *op_new; > + struct cs_dsp_chunk ch; > + u32 pseq_bytes; > + int error; > + > + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL); > + if (!op_new) > + return -ENOMEM; > + > + op_new->operation = op_code; > + > + ch = cs_dsp_chunk((void *) op_new->words, > + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32)); > + cs_dsp_chunk_write(&ch, 8, op_code); > + switch (op_code) { > + case PSEQ_OP_WRITE_FULL: > + cs_dsp_chunk_write(&ch, 32, addr); > + cs_dsp_chunk_write(&ch, 32, data); > + break; > + case PSEQ_OP_WRITE_L16: > + case PSEQ_OP_WRITE_H16: > + cs_dsp_chunk_write(&ch, 24, addr); > + cs_dsp_chunk_write(&ch, 16, data); > + break; > + default: > + error = -EINVAL; > + goto op_new_free; > + } > + > + op_new->size = cs_dsp_chunk_bytes(&ch); > + > + if (update) { > + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head); > + if (!op) { > + error = -EINVAL; > + goto op_new_free; > + } > + } It seems we are relying on the developer to remember if he or she has already written 'addr' using a previous call to cs_hap_pseq_write(), then set the update flag accordingly; is that accurate? If so, that is a high risk for bugs to be introduced as the driver is maintained. Can we not search for an existing address/data entry upon any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add or replace a new address/data entry automatically? > + > + error = cs_hap_pseq_find_end(haptics, &op_end); > + if (error) > + goto op_new_free; > + > + pseq_bytes = haptics->dsp.pseq_size * sizeof(u32); > + > + if (pseq_bytes - op_end->offset < op_new->size) { > + error = -ENOMEM; > + goto op_new_free; > + } > + > + if (update) { > + op_new->offset = op->offset; > + } else { > + op_new->offset = op_end->offset; > + op_end->offset += op_new->size; > + } > + > + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg + > + op_new->offset, op_new->words, op_new->size); > + if (error) > + goto op_new_free; > + > + if (update) { > + list_replace(&op->list, &op_new->list); > + } else { > + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg + > + op_end->offset, op_end->words, > + op_end->size); > + if (error) > + goto op_new_free; > + > + list_add(&op_new->list, &haptics->pseq_head); > + } > + > + return 0; > + > +op_new_free: > + devm_kfree(haptics->dev, op_new); > + > + return error; > +} > +EXPORT_SYMBOL_GPL(cs_hap_pseq_write); > + > +int cs_hap_pseq_multi_write(struct cs_hap *haptics, > + const struct reg_sequence *reg_seq, > + int num_regs, bool update, u8 op_code) > +{ > + int error, i; > + > + for (i = 0; i < num_regs; i++) { > + error = cs_hap_pseq_write(haptics, reg_seq[i].reg, > + reg_seq[i].def, update, op_code); > + if (error) > + return error; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cs_hap_pseq_multi_write); > + > +static struct cs_hap_effect *cs_hap_find_effect(int id, > + struct list_head *effect_head) > +{ > + struct cs_hap_effect *effect; > + > + list_for_each_entry(effect, effect_head, list) > + if (effect->id == id) > + return effect; > + > + return NULL; > +} > + > +static int cs_hap_effect_bank_set(struct cs_hap *haptics, > + struct cs_hap_effect *effect, > + struct ff_periodic_effect add_effect) > +{ > + s16 bank = add_effect.custom_data[0] & 0xffffu; > + unsigned int len = add_effect.custom_len; > + > + if (bank >= WVFRM_BANK_NUM) { > + dev_err(haptics->dev, "Invalid waveform bank: %d\n", bank); > + return -EINVAL; > + } > + > + effect->bank = len > CUSTOM_DATA_SIZE ? WVFRM_BANK_OWT : bank; > + > + return 0; > +} > + > +static int cs_hap_effect_mapping_set(struct cs_hap *haptics, u16 button, > + struct cs_hap_effect *effect) > +{ > + u32 gpio_num, gpio_edge; > + > + if (button) { > + gpio_num = FIELD_GET(BTN_NUM_MASK, button); > + gpio_edge = FIELD_GET(BTN_EDGE_MASK, button); > + effect->mapping = haptics->dsp.gpio_base_reg + > + (gpio_num * 8) - gpio_edge; > + > + return regmap_write(haptics->regmap, effect->mapping, button); > + } > + > + effect->mapping = GPIO_MAPPING_INVALID; > + > + return 0; > +} > + > +static int cs_hap_effect_index_set(struct cs_hap *haptics, > + struct cs_hap_effect *effect, > + struct ff_periodic_effect add_effect) > +{ > + struct cs_hap_effect *owt_effect; > + u32 base_index, max_index; > + > + base_index = haptics->banks[effect->bank].base_index; > + max_index = haptics->banks[effect->bank].max_index; > + > + effect->index = base_index; > + > + switch (effect->bank) { > + case WVFRM_BANK_OWT: > + list_for_each_entry(owt_effect, &haptics->effect_head, list) > + if (owt_effect->bank == WVFRM_BANK_OWT) > + effect->index++; > + break; > + case WVFRM_BANK_ROM: > + case WVFRM_BANK_RAM: > + effect->index += add_effect.custom_data[1] & 0xffffu; > + break; > + default: > + dev_err(haptics->dev, "Bank not supported: %d\n", effect->bank); > + return -EINVAL; > + } > + > + if (effect->index > max_index || effect->index < base_index) { > + dev_err(haptics->dev, "Index out of bounds: %u\n", effect->index); > + return -ENOSPC; > + } > + > + return 0; > +} > + > +static int cs_hap_upload_pwle(struct cs_hap *haptics, > + struct cs_hap_effect *effect, > + struct ff_periodic_effect add_effect) > +{ > + u32 len, wt_offset, wt_size_words; > + struct cs_hap_pwle_header header; > + u8 *out_data; > + int error; > + > + error = regmap_read(haptics->regmap, haptics->dsp.owt_offset_reg, > + &wt_offset); > + if (error) > + return error; > + > + error = regmap_read(haptics->regmap, haptics->dsp.owt_size_reg, > + &wt_size_words); > + if (error) > + return error; > + > + len = 2 * add_effect.custom_len; > + > + if ((wt_size_words * sizeof(u32)) < OWT_HEADER_SIZE + len) > + return -ENOSPC; > + > + out_data = kzalloc(OWT_HEADER_SIZE + len, GFP_KERNEL); > + if (!out_data) > + return -ENOMEM; > + > + header.type = add_effect.custom_data[0] == PCM_ID ? OWT_TYPE_PCM : > + OWT_TYPE_PWLE; > + header.offset = OWT_HEADER_SIZE / sizeof(u32); > + header.data_words = len / sizeof(u32); > + > + memcpy(out_data, &header, sizeof(header)); > + memcpy(out_data + OWT_HEADER_SIZE, add_effect.custom_data, len); > + > + error = regmap_bulk_write(haptics->regmap, haptics->dsp.owt_base_reg + > + (wt_offset * sizeof(u32)), out_data, > + OWT_HEADER_SIZE + len); > + if (error) > + goto err_free; > + > + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > + haptics->dsp.push_owt_cmd); > + > +err_free: > + kfree(out_data); > + > + return error; > +} > + > +static void cs_hap_add_worker(struct work_struct *work) > +{ > + struct cs_hap *haptics = container_of(work, struct cs_hap, > + add_work); > + struct ff_effect add_effect = haptics->add_effect; > + bool is_new = false; > + struct cs_hap_effect *effect; > + int error; > + > + if (haptics->runtime_pm) { > + error = pm_runtime_resume_and_get(haptics->dev); > + if (error < 0) { > + haptics->add_error = error; > + return; > + } > + } > + > + mutex_lock(&haptics->lock); > + > + effect = cs_hap_find_effect(add_effect.id, &haptics->effect_head); > + if (!effect) { > + effect = kzalloc(sizeof(*effect), GFP_KERNEL); > + if (!effect) { > + error = -ENOMEM; > + goto err_mutex; > + } > + effect->id = add_effect.id; > + is_new = true; > + } > + > + error = cs_hap_effect_bank_set(haptics, effect, add_effect.u.periodic); > + if (error) > + goto err_free; > + > + error = cs_hap_effect_index_set(haptics, effect, add_effect.u.periodic); > + if (error) > + goto err_free; > + > + error = cs_hap_effect_mapping_set(haptics, add_effect.trigger.button, > + effect); > + if (error) > + goto err_free; > + > + if (effect->bank == WVFRM_BANK_OWT) > + error = cs_hap_upload_pwle(haptics, effect, > + add_effect.u.periodic); > + > +err_free: > + if (is_new) { > + if (error) > + kfree(effect); > + else > + list_add(&effect->list, &haptics->effect_head); > + } > + > +err_mutex: > + mutex_unlock(&haptics->lock); > + > + if (haptics->runtime_pm) { > + pm_runtime_mark_last_busy(haptics->dev); > + pm_runtime_put_autosuspend(haptics->dev); > + } > + > + haptics->add_error = error; > +} > + > +static void cs_hap_erase_worker(struct work_struct *work) > +{ > + struct cs_hap *haptics = container_of(work, struct cs_hap, > + erase_work); > + int error = 0; > + struct cs_hap_effect *owt_effect, *erase_effect; > + > + if (haptics->runtime_pm) { > + error = pm_runtime_resume_and_get(haptics->dev); > + if (error < 0) { > + haptics->erase_error = error; > + return; > + } > + } > + > + mutex_lock(&haptics->lock); > + > + erase_effect = cs_hap_find_effect(haptics->erase_effect->id, > + &haptics->effect_head); > + if (!erase_effect) { > + dev_err(haptics->dev, "Effect to erase does not exist\n"); > + error = -EINVAL; > + goto err_mutex; > + } > + > + if (erase_effect->mapping != GPIO_MAPPING_INVALID) { > + error = regmap_write(haptics->regmap, erase_effect->mapping, > + GPIO_DISABLE); > + if (error) > + goto err_mutex; > + } > + > + if (erase_effect->bank == WVFRM_BANK_OWT) { > + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > + haptics->dsp.delete_owt_cmd | > + erase_effect->index); > + if (error) > + goto err_mutex; > + > + list_for_each_entry(owt_effect, &haptics->effect_head, list) > + if (owt_effect->bank == WVFRM_BANK_OWT && > + owt_effect->index > erase_effect->index) > + owt_effect->index--; > + } > + > + list_del(&erase_effect->list); > + kfree(erase_effect); > + > +err_mutex: > + mutex_unlock(&haptics->lock); > + > + if (haptics->runtime_pm) { > + pm_runtime_mark_last_busy(haptics->dev); > + pm_runtime_put_autosuspend(haptics->dev); > + } > + > + haptics->erase_error = error; > +} > + > +static void cs_hap_vibe_start_worker(struct work_struct *work) > +{ > + struct cs_hap *haptics = container_of(work, struct cs_hap, > + vibe_start_work); > + struct cs_hap_effect *effect; > + int error; > + > + if (haptics->runtime_pm) { > + error = pm_runtime_resume_and_get(haptics->dev); > + if (error < 0) { > + haptics->start_error = error; > + return; > + } > + } > + > + mutex_lock(&haptics->lock); > + > + effect = cs_hap_find_effect(haptics->start_effect->id, > + &haptics->effect_head); > + if (effect) { > + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > + effect->index); > + } else { > + dev_err(haptics->dev, "Effect to start does not exist\n"); > + error = -EINVAL; > + } > + > + mutex_unlock(&haptics->lock); > + > + if (haptics->runtime_pm) { > + pm_runtime_mark_last_busy(haptics->dev); > + pm_runtime_put_autosuspend(haptics->dev); > + } > + > + haptics->start_error = error; > +} > + > +static void cs_hap_vibe_stop_worker(struct work_struct *work) > +{ > + struct cs_hap *haptics = container_of(work, struct cs_hap, > + vibe_stop_work); > + int error; > + > + if (haptics->runtime_pm) { > + error = pm_runtime_resume_and_get(haptics->dev); > + if (error < 0) { > + haptics->stop_error = error; > + return; > + } > + } > + > + mutex_lock(&haptics->lock); > + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > + haptics->dsp.stop_cmd); > + mutex_unlock(&haptics->lock); > + > + if (haptics->runtime_pm) { > + pm_runtime_mark_last_busy(haptics->dev); > + pm_runtime_put_autosuspend(haptics->dev); > + } > + > + haptics->stop_error = error; This seems like another argument for not separating the input/FF child from the meat of the driver; it just seems messy to pass around error codes within a driver's private data like this. That being said, where are start_error and stop_error used? I didn't see them in the input/FF child. We should only introduce code that has at least one user. > +} > + > +int cs_hap_init(struct cs_hap *haptics) > +{ > + haptics->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0); > + if (!haptics->vibe_wq) > + return -ENOMEM; > + > + mutex_init(&haptics->lock); > + > + INIT_WORK(&haptics->vibe_start_work, cs_hap_vibe_start_worker); > + INIT_WORK(&haptics->vibe_stop_work, cs_hap_vibe_stop_worker); > + INIT_WORK(&haptics->erase_work, cs_hap_erase_worker); > + INIT_WORK(&haptics->add_work, cs_hap_add_worker); > + > + return cs_hap_pseq_init(haptics); > +} > +EXPORT_SYMBOL_GPL(cs_hap_init); > + > +void cs_hap_remove(struct cs_hap *haptics) > +{ > + flush_workqueue(haptics->vibe_wq); > + destroy_workqueue(haptics->vibe_wq); > +} > +EXPORT_SYMBOL_GPL(cs_hap_remove); > + > +MODULE_DESCRIPTION("Cirrus Logic Haptics Support"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/input/cirrus_haptics.h b/include/linux/input/cirrus_haptics.h > new file mode 100644 > index 000000000000..42f6afed7944 > --- /dev/null > +++ b/include/linux/input/cirrus_haptics.h > @@ -0,0 +1,121 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Helper functions for dealing with wavetable > + * formats and DSP interfaces used by Cirrus > + * haptic drivers. > + * > + * Copyright 2023 Cirrus Logic, Inc. > + */ > + > +#ifndef __CIRRUS_HAPTICS_H > +#define __CIRRUS_HAPTICS_H > + > +#include <linux/property.h> > +#include <linux/regmap.h> > + > +/* Power-on write sequencer */ > +#define PSEQ_OP_MASK GENMASK(23, 16) > +#define PSEQ_OP_SHIFT 16 > +#define PSEQ_OP_WRITE_FULL_WORDS 3 > +#define PSEQ_OP_WRITE_X16_WORDS 2 > +#define PSEQ_OP_END_WORDS 1 > +#define PSEQ_OP_WRITE_FULL 0x00 > +#define PSEQ_OP_WRITE_ADDR8 0x02 > +#define PSEQ_OP_WRITE_L16 0x04 > +#define PSEQ_OP_WRITE_H16 0x05 > +#define PSEQ_OP_WRITE_UNLOCK 0xFD > +#define PSEQ_OP_END 0xFF > + > +/* Open wavetable */ > +#define OWT_HEADER_SIZE 12 > +#define OWT_TYPE_PCM 8 > +#define OWT_TYPE_PWLE 12 > +#define PCM_ID 0x0 > +#define CUSTOM_DATA_SIZE 2 > + > +/* GPIO */ > +#define BTN_NUM_MASK GENMASK(14, 12) > +#define BTN_EDGE_MASK BIT(15) > +#define GPIO_MAPPING_INVALID 0 > +#define GPIO_DISABLE 0x1FF > + > +enum cs_hap_bank_type { > + WVFRM_BANK_RAM, > + WVFRM_BANK_ROM, > + WVFRM_BANK_OWT, > + WVFRM_BANK_NUM, > +}; > + > +struct cs_hap_pseq_op { > + struct list_head list; > + u32 words[3]; > + u16 offset; > + u8 operation; > + u8 size; > +}; > + > +struct cs_hap_effect { > + enum cs_hap_bank_type bank; > + struct list_head list; > + u32 mapping; > + u32 index; > + int id; > +}; > + > +struct cs_hap_pwle_header { > + u32 type; > + u32 data_words; > + u32 offset; > +} __packed; > + > +struct cs_hap_bank { > + enum cs_hap_bank_type bank; > + u32 base_index; > + u32 max_index; > +}; > + > +struct cs_hap_dsp { > + u32 gpio_base_reg; > + u32 owt_offset_reg; > + u32 owt_size_reg; > + u32 owt_base_reg; > + u32 mailbox_reg; > + u32 pseq_reg; > + u32 push_owt_cmd; > + u32 delete_owt_cmd; > + u32 stop_cmd; > + u32 pseq_size; > +}; > + > +struct cs_hap { > + struct regmap *regmap; > + struct mutex lock; > + struct device *dev; > + struct list_head pseq_head; > + struct cs_hap_bank *banks; > + struct cs_hap_dsp dsp; > + struct workqueue_struct *vibe_wq; > + struct work_struct vibe_start_work; > + struct work_struct vibe_stop_work; > + struct work_struct erase_work; > + struct work_struct add_work; > + struct ff_effect *start_effect; > + struct ff_effect *erase_effect; > + struct ff_effect add_effect; > + struct list_head effect_head; > + int erase_error; > + int start_error; > + int stop_error; > + int add_error; > + bool runtime_pm; > +}; > + > +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, > + u32 data, bool update, u8 op_code); > +int cs_hap_pseq_multi_write(struct cs_hap *haptics, > + const struct reg_sequence *reg_seq, > + int num_regs, bool update, u8 op_code); > +int cs_hap_init(struct cs_hap *haptics); > +void cs_hap_remove(struct cs_hap *haptics); > + > +#endif > -- > 2.25.1 > Kind regards, Jeff LaBundy
Hi Jeff, You’ve given such great feedback covering many aspects of the driver, on this patch and the whole series, which has required very careful consideration, so thank you for your patience. > On Oct 24, 2023, at 9:04 PM, Jeff LaBundy <jeff@labundy.com> wrote: > > On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote: >> >> +static int cs_hap_pseq_init(struct cs_hap *haptics) >> +{ >> + struct cs_hap_pseq_op *op; >> + int error, i, num_words; >> + u8 operation; >> + u32 *words; >> + >> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg) >> + return 0; >> + >> + INIT_LIST_HEAD(&haptics->pseq_head); > > Anything that allocates or initializes an element that is normally held > in a driver's private data, like a list head or mutex, belongs in probe() > in my opinion. It's less of an issue here, but for more complex cases > where we may set something up in probe() and tear it down in remove(), > the driver is easier to maintain if helper functions such as cs_hap_pseq_init() > only manipulate or organize the data, rather than absorb additional work. I agree with your reasoning, however, doesn’t this then turn on the question of who the rightful owner of the write sequencer code is? If the pseq code belongs in the MFD driver, it ought to be moved to the driver’s private data structure there, however if it fits here, then not so. A counter example would be cs_dsp.c, a library which contains within it some INIT_LIST_HEAD calls. Perhaps the dust should settle on that dispute, and in regards to that, please read my comments below. >> + >> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); >> + if (!words) >> + return -ENOMEM; >> + >> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, >> + words, haptics->dsp.pseq_size); >> + if (error) >> + goto err_free; >> + >> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { >> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); >> + switch (operation) { >> + case PSEQ_OP_END: >> + case PSEQ_OP_WRITE_UNLOCK: >> + num_words = PSEQ_OP_END_WORDS; >> + break; >> + case PSEQ_OP_WRITE_ADDR8: >> + case PSEQ_OP_WRITE_H16: >> + case PSEQ_OP_WRITE_L16: >> + num_words = PSEQ_OP_WRITE_X16_WORDS; >> + break; >> + case PSEQ_OP_WRITE_FULL: >> + num_words = PSEQ_OP_WRITE_FULL_WORDS; >> + break; >> + default: >> + error = -EINVAL; >> + dev_err(haptics->dev, "Unsupported op: %u\n", operation); >> + goto err_free; >> + } >> + >> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); >> + if (!op) { >> + error = -ENOMEM; >> + goto err_free; >> + } >> + >> + op->size = num_words * sizeof(u32); >> + memcpy(op->words, &words[i], op->size); >> + op->offset = i * sizeof(u32); >> + op->operation = operation; >> + list_add(&op->list, &haptics->pseq_head); >> + >> + if (operation == PSEQ_OP_END) >> + break; >> + } >> + >> + if (operation != PSEQ_OP_END) >> + error = -ENOENT; >> + >> +err_free: >> + kfree(words); >> + >> + return error; >> +} > > My first thought as I reviewed this patch was that this and the lot > of pseq-related functions are not necessarily related to haptics, but > rather CS40L50 register access and housekeeping in general. > > I seem to recall on L25 and friends that the the power-on sequencer, > i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory > that can play back a series of address/data pairs when the device > comes out of hibernation, and any registers written during runtime > must also be mirrored to the PSEQ for "playback" later. Is that still > the case here? > > Assuming so, these functions seem like they belong in the MFD, not > an input-specific library, because they will presumably be used by > the codec driver as well, since that driver will write registers to > set BCLK/LRCK ratio, etc. > > Therefore, I think it makes more sense for these functions to move to > the MFD, which can then export them for use by the input/FF and codec > children. I think the problem with moving the write sequencer code to the MFD driver is that it would go unused in a codec-only environment. We only need to write to the PSEQ when we want to maintain register settings throughout hibernation cycles, and it isn’t possible to hibernate when there is data streaming to the device. So the PSEQ will never be used in the codec driver. This leaves either the input driver or the library, and it makes more sense to be in the library since it is shared code with L26. This was my reasoning, let me know whether you think it is sound. > This leaves cirrus_haptics.* with only a few functions related to > starting and stopping work, which seem specific enough to just live > in cs40l50-vibra.c. Presumably many of those could be re-used by > the L30 down the road, but in that case I think we'd be looking to > actually re-use the L50 driver and simply add a compatible string > for L30. > > I recall L30 has some overhead that L50 does not, which may make > it hairy for cs40l50-vibra.c to support both. But in that case, > you could always fork a cs40l30-vibra.c with its own compatible > string, then have the L50 MFD selectively load the correct child > based on device ID. To summarize, we should have: > > * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery, > IRQ handling, exported PSEQ functions, etc. > * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from > the MFD. > * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop > work, also uses PSEQ library from the MFD. > > And down the road, depending on complexity, maybe we also have: > > * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that > includes other functionality that didn't really fit in cs40l50-vibra.c; > also uses PSEQ library from, and is loaded by, the original L50 MFD. > If this driver duplicates small bits of cs40l50-vibra.c, it's not the > end of the world. > > All of these files would #include include/linux/mfd/cs40l50.h. And > finally, cirrus_haptics.* simply go away. Same idea, just slightly > more scalable, and closer to common design patterns. I understand that it is a common design pattern to selectively load devices from the MFD driver. If I could summarize my thoughts on why that would not be fitting here, it’s that the L26 and L50 share a ton of input FF related work, and not enough “MFD core” related work. Between errata differences, power supply configurations, regmap configurations, interrupt register differences, it would seem to make for a very awkward, scrambled MFD driver. Moreover, I think I will be moving firmware download to the MFD driver, and that alone constitutes a significant incompatibility because firmware downloading is compulsory on L26, not so on L50. On the other hand, I want to emphasize the amount that L26 and L50 share when it comes to the Input FF callbacks. The worker functions in cirrus_haptics.c are bare-bones for this first submission, but were designed to be totally generic and scalable to the needs of L26 and all future Cirrus input drivers. While it might appear too specific for L26, everything currently in cirrus_haptics is usable by L26 as-is. For the above reasons I favor the current approach. >> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, >> + u32 data, bool update, u8 op_code) >> +{ >> + struct cs_hap_pseq_op *op, *op_end, *op_new; >> + struct cs_dsp_chunk ch; >> + u32 pseq_bytes; >> + int error; >> + >> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL); >> + if (!op_new) >> + return -ENOMEM; >> + >> + op_new->operation = op_code; >> + >> + ch = cs_dsp_chunk((void *) op_new->words, >> + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32)); >> + cs_dsp_chunk_write(&ch, 8, op_code); >> + switch (op_code) { >> + case PSEQ_OP_WRITE_FULL: >> + cs_dsp_chunk_write(&ch, 32, addr); >> + cs_dsp_chunk_write(&ch, 32, data); >> + break; >> + case PSEQ_OP_WRITE_L16: >> + case PSEQ_OP_WRITE_H16: >> + cs_dsp_chunk_write(&ch, 24, addr); >> + cs_dsp_chunk_write(&ch, 16, data); >> + break; >> + default: >> + error = -EINVAL; >> + goto op_new_free; >> + } >> + >> + op_new->size = cs_dsp_chunk_bytes(&ch); >> + >> + if (update) { >> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head); >> + if (!op) { >> + error = -EINVAL; >> + goto op_new_free; >> + } >> + } > > It seems we are relying on the developer to remember if he or she has > already written 'addr' using a previous call to cs_hap_pseq_write(), > then set the update flag accordingly; is that accurate? > > If so, that is a high risk for bugs to be introduced as the driver is > maintained. Can we not search for an existing address/data entry upon > any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add > or replace a new address/data entry automatically? There are currently sequences on L26 where we want to write to the same address twice. In such cases we wouldn't want the driver to automatically look up and update the first entry during the second write call, hence the need for the update flag. I think with this use case in mind, updating the entries automatically wouldn't be feasible. >> +static void cs_hap_vibe_stop_worker(struct work_struct *work) >> +{ >> + struct cs_hap *haptics = container_of(work, struct cs_hap, >> + vibe_stop_work); >> + int error; >> + >> + if (haptics->runtime_pm) { >> + error = pm_runtime_resume_and_get(haptics->dev); >> + if (error < 0) { >> + haptics->stop_error = error; >> + return; >> + } >> + } >> + >> + mutex_lock(&haptics->lock); >> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, >> + haptics->dsp.stop_cmd); >> + mutex_unlock(&haptics->lock); >> + >> + if (haptics->runtime_pm) { >> + pm_runtime_mark_last_busy(haptics->dev); >> + pm_runtime_put_autosuspend(haptics->dev); >> + } >> + >> + haptics->stop_error = error; > > This seems like another argument for not separating the input/FF child > from the meat of the driver; it just seems messy to pass around error > codes within a driver's private data like this. I removed the start_error and stop_error members. However I think the erase_error and add_error need to stay. I think this is more of a symptom of the Input FF layer requiring error reporting and having to use workqueues for those Input FF callbacks, than it is to do with the separation of these functions from the driver. Point being, even if these were moved, we would still need these *_error members. Let me know if I understood you right here. Best, James
Hi James, On Wed, Nov 01, 2023 at 08:46:12PM +0000, James Ogletree wrote: > Hi Jeff, > > You’ve given such great feedback covering many aspects of the > driver, on this patch and the whole series, which has required very > careful consideration, so thank you for your patience. Your high-quality submissions make the review process efficient and productive :) Please excuse my delayed response. > > > On Oct 24, 2023, at 9:04 PM, Jeff LaBundy <jeff@labundy.com> wrote: > > > > On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote: > >> > >> +static int cs_hap_pseq_init(struct cs_hap *haptics) > >> +{ > >> + struct cs_hap_pseq_op *op; > >> + int error, i, num_words; > >> + u8 operation; > >> + u32 *words; > >> + > >> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg) > >> + return 0; > >> + > >> + INIT_LIST_HEAD(&haptics->pseq_head); > > > > Anything that allocates or initializes an element that is normally held > > in a driver's private data, like a list head or mutex, belongs in probe() > > in my opinion. It's less of an issue here, but for more complex cases > > where we may set something up in probe() and tear it down in remove(), > > the driver is easier to maintain if helper functions such as cs_hap_pseq_init() > > only manipulate or organize the data, rather than absorb additional work. > > I agree with your reasoning, however, doesn’t this then turn on the question of > who the rightful owner of the write sequencer code is? If the pseq code > belongs in the MFD driver, it ought to be moved to the driver’s private data > structure there, however if it fits here, then not so. A counter example would > be cs_dsp.c, a library which contains within it some INIT_LIST_HEAD calls. > Perhaps the dust should settle on that dispute, and in regards to that, please > read my comments below. I think the list head belongs in the MFD's private data (cs40l50_private) because the pseq code belongs in the MFD; more details as to why below. > > >> + > >> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); > >> + if (!words) > >> + return -ENOMEM; > >> + > >> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, > >> + words, haptics->dsp.pseq_size); > >> + if (error) > >> + goto err_free; > >> + > >> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { > >> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); > >> + switch (operation) { > >> + case PSEQ_OP_END: > >> + case PSEQ_OP_WRITE_UNLOCK: > >> + num_words = PSEQ_OP_END_WORDS; > >> + break; > >> + case PSEQ_OP_WRITE_ADDR8: > >> + case PSEQ_OP_WRITE_H16: > >> + case PSEQ_OP_WRITE_L16: > >> + num_words = PSEQ_OP_WRITE_X16_WORDS; > >> + break; > >> + case PSEQ_OP_WRITE_FULL: > >> + num_words = PSEQ_OP_WRITE_FULL_WORDS; > >> + break; > >> + default: > >> + error = -EINVAL; > >> + dev_err(haptics->dev, "Unsupported op: %u\n", operation); > >> + goto err_free; > >> + } > >> + > >> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); > >> + if (!op) { > >> + error = -ENOMEM; > >> + goto err_free; > >> + } > >> + > >> + op->size = num_words * sizeof(u32); > >> + memcpy(op->words, &words[i], op->size); > >> + op->offset = i * sizeof(u32); > >> + op->operation = operation; > >> + list_add(&op->list, &haptics->pseq_head); > >> + > >> + if (operation == PSEQ_OP_END) > >> + break; > >> + } > >> + > >> + if (operation != PSEQ_OP_END) > >> + error = -ENOENT; > >> + > >> +err_free: > >> + kfree(words); > >> + > >> + return error; > >> +} > > > > My first thought as I reviewed this patch was that this and the lot > > of pseq-related functions are not necessarily related to haptics, but > > rather CS40L50 register access and housekeeping in general. > > > > I seem to recall on L25 and friends that the the power-on sequencer, > > i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory > > that can play back a series of address/data pairs when the device > > comes out of hibernation, and any registers written during runtime > > must also be mirrored to the PSEQ for "playback" later. Is that still > > the case here? > > > > Assuming so, these functions seem like they belong in the MFD, not > > an input-specific library, because they will presumably be used by > > the codec driver as well, since that driver will write registers to > > set BCLK/LRCK ratio, etc. > > > > Therefore, I think it makes more sense for these functions to move to > > the MFD, which can then export them for use by the input/FF and codec > > children. > > I think the problem with moving the write sequencer code to the MFD > driver is that it would go unused in a codec-only environment. We only > need to write to the PSEQ when we want to maintain register settings > throughout hibernation cycles, and it isn’t possible to hibernate when > there is data streaming to the device. So the PSEQ will never be used > in the codec driver. I agree that in practice, the write sequencer would technically go unused in a codec-only implementation. However, that is because the ASoC core happens to write all pertinent registers ahead-of-time each time a stream starts. That is a property of the ASoC core and not L50; my feeling is that the driver should not be structured based on what one of the subsystems associated with it happens to do, but rather the nature of the hardware. Some specific reasons I think the MFD is a better home for the pseq code: 1. The write sequencer is a housekeeping function derived from the way the hardware implements its power management; it doesn't have anything to do with haptics. My opinion is that facilities supporting application- agnostic functions belong in the MFD, for all children to access, even if only one happens to do so today. 2. Over the course of the IC's life, you may be required to add errata writes after the IC is taken out of reset; these presumably would need to be "scheduled" in the write sequencer as well. These wouldn't make logical sense to add in the input driver, and they would be missed in the theoretical codec-only case. 3. This device has a programmable DSP; it wouldn't be unheard of for a customer to ask for a new function down the road that begets a third child device. Perhaps a customer asks for a special .wmfw file that repurposes the SDOUT pin as a PWM output for an LED, and now you must add a pwm child. That's a made-up example of course, but in general we want to structure things in such a way that rip-up is minimal in case requirements change in the future. > This leaves either the input driver or the library, and it makes more > sense to be in the library since it is shared code with L26. This was > my reasoning, let me know whether you think it is sound. I'm not sold on the input driver and the methods in what is now an FF library living in separate files; more on that below. > > > This leaves cirrus_haptics.* with only a few functions related to > > starting and stopping work, which seem specific enough to just live > > in cs40l50-vibra.c. Presumably many of those could be re-used by > > the L30 down the road, but in that case I think we'd be looking to > > actually re-use the L50 driver and simply add a compatible string > > for L30. > > > > I recall L30 has some overhead that L50 does not, which may make > > it hairy for cs40l50-vibra.c to support both. But in that case, > > you could always fork a cs40l30-vibra.c with its own compatible > > string, then have the L50 MFD selectively load the correct child > > based on device ID. To summarize, we should have: > > > > * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery, > > IRQ handling, exported PSEQ functions, etc. > > * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from > > the MFD. > > * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop > > work, also uses PSEQ library from the MFD. > > > > And down the road, depending on complexity, maybe we also have: > > > > * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that > > includes other functionality that didn't really fit in cs40l50-vibra.c; > > also uses PSEQ library from, and is loaded by, the original L50 MFD. > > If this driver duplicates small bits of cs40l50-vibra.c, it's not the > > end of the world. > > > > All of these files would #include include/linux/mfd/cs40l50.h. And > > finally, cirrus_haptics.* simply go away. Same idea, just slightly > > more scalable, and closer to common design patterns. > > > I understand that it is a common design pattern to selectively load > devices from the MFD driver. If I could summarize my thoughts on why > that would not be fitting here, it’s that the L26 and L50 share a ton of > input FF related work, and not enough “MFD core” related work. > Between errata differences, power supply configurations, regmap > configurations, interrupt register differences, it would seem to make for > a very awkward, scrambled MFD driver. Moreover, I think I will be moving > firmware download to the MFD driver, and that alone constitutes a > significant incompatibility because firmware downloading is compulsory > on L26, not so on L50. > > On the other hand, I want to emphasize the amount that L26 and > L50 share when it comes to the Input FF callbacks. The worker > functions in cirrus_haptics.c are bare-bones for this first > submission, but were designed to be totally generic and scalable to > the needs of L26 and all future Cirrus input drivers. While it might appear > too specific for L26, everything currently in cirrus_haptics is usable by > L26 as-is. > > For the above reasons I favor the current approach. > If the core functions (e.g. firmware loading and interrupt handling) of L26 and L50 are drastically different, then it's perfectly acceptable to have different MFD drivers for the two. There are plenty of examples of devices throughout MFD who are close cousins to one another. Likewise, if the input-related functions of L26 and L50 are nearly identical, then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which supports both L26 and L50 haptics-related functions. You're already doing something similar, but I disagree on the following: 1. Rather than have a library referenced by two drivers that support children which are largely the same in a logcial sense, just have a single driver that supports two children. 2. That common code encapsulates too many things (e.g. pseq facilities). A library may make sense, but not restricted to input children. For code that is common to either MFD (e.g. pseq), it is largely a judgement call and maintainer preference as to where that code lives. One approach could be to simply include all common code in drivers/mfd/cs40l50.c today. When drivers/mfd/cs40l26.c lands, it can simply #include <linux/mfd/cs40l50.h>, and its Kconfig can select MFD_CS40L50. Both modules would get built, but only one would bind and probe. Another approach could be to move the pseq code from the L50 MFD into a library (e.g. drivers/mfs/cs40l50-common.c) selected by both L50 and L26 Kconfigs. That is of course some rip-up, but at least the code is not jumping subsystems at this point, which it otherwise might if you leave it in an input library for now. That being said, you are the one who is responsible for supporting the driver in the future, so the decision is ultimately yours. You have a much better sense of the hardware variants and the relationships among them. It's a delicate balance of preparing for what customers may request, and any decision will inevitably be wrong in some way :) I think the important thing is to not restrict yourself too much up front, and my humble opinion is that slightly redrawing the dotted lines in your "block diagram" may ease some pain down the road. > > >> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, > >> + u32 data, bool update, u8 op_code) > >> +{ > >> + struct cs_hap_pseq_op *op, *op_end, *op_new; > >> + struct cs_dsp_chunk ch; > >> + u32 pseq_bytes; > >> + int error; > >> + > >> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL); > >> + if (!op_new) > >> + return -ENOMEM; > >> + > >> + op_new->operation = op_code; > >> + > >> + ch = cs_dsp_chunk((void *) op_new->words, > >> + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32)); > >> + cs_dsp_chunk_write(&ch, 8, op_code); > >> + switch (op_code) { > >> + case PSEQ_OP_WRITE_FULL: > >> + cs_dsp_chunk_write(&ch, 32, addr); > >> + cs_dsp_chunk_write(&ch, 32, data); > >> + break; > >> + case PSEQ_OP_WRITE_L16: > >> + case PSEQ_OP_WRITE_H16: > >> + cs_dsp_chunk_write(&ch, 24, addr); > >> + cs_dsp_chunk_write(&ch, 16, data); > >> + break; > >> + default: > >> + error = -EINVAL; > >> + goto op_new_free; > >> + } > >> + > >> + op_new->size = cs_dsp_chunk_bytes(&ch); > >> + > >> + if (update) { > >> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head); > >> + if (!op) { > >> + error = -EINVAL; > >> + goto op_new_free; > >> + } > >> + } > > > > It seems we are relying on the developer to remember if he or she has > > already written 'addr' using a previous call to cs_hap_pseq_write(), > > then set the update flag accordingly; is that accurate? > > > > If so, that is a high risk for bugs to be introduced as the driver is > > maintained. Can we not search for an existing address/data entry upon > > any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add > > or replace a new address/data entry automatically? > > There are currently sequences on L26 where we want to write to the same > address twice. In such cases we wouldn't want the driver to automatically > look up and update the first entry during the second write call, hence the > need for the update flag. I think with this use case in mind, updating > the entries automatically wouldn't be feasible. ACK. Good call on returning -EINVAL if update is true yet the entry was never added; that seems like a reasonable level of protection. > > >> +static void cs_hap_vibe_stop_worker(struct work_struct *work) > >> +{ > >> + struct cs_hap *haptics = container_of(work, struct cs_hap, > >> + vibe_stop_work); > >> + int error; > >> + > >> + if (haptics->runtime_pm) { > >> + error = pm_runtime_resume_and_get(haptics->dev); > >> + if (error < 0) { > >> + haptics->stop_error = error; > >> + return; > >> + } > >> + } > >> + > >> + mutex_lock(&haptics->lock); > >> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > >> + haptics->dsp.stop_cmd); > >> + mutex_unlock(&haptics->lock); > >> + > >> + if (haptics->runtime_pm) { > >> + pm_runtime_mark_last_busy(haptics->dev); > >> + pm_runtime_put_autosuspend(haptics->dev); > >> + } > >> + > >> + haptics->stop_error = error; > > > > This seems like another argument for not separating the input/FF child > > from the meat of the driver; it just seems messy to pass around error > > codes within a driver's private data like this. > > I removed the start_error and stop_error members. However I think the > erase_error and add_error need to stay. I think this is more of a symptom > of the Input FF layer requiring error reporting and having to use workqueues > for those Input FF callbacks, than it is to do with the separation of these > functions from the driver. Point being, even if these were moved, we would still > need these *_error members. Let me know if I understood you right here. Sure, but why do adding and removing waveforms require workqueues? The DA7280 driver doesn't do this; what is different in this case? (That's a genuine question, not an assertion that what you have is wrong, although it seems unique based on my limited search). > > Best, > James > > Kind regards, Jeff LaBundy
>>>> + >>>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); >>>> + if (!words) >>>> + return -ENOMEM; >>>> + >>>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, >>>> + words, haptics->dsp.pseq_size); >>>> + if (error) >>>> + goto err_free; >>>> + >>>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { >>>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); >>>> + switch (operation) { >>>> + case PSEQ_OP_END: >>>> + case PSEQ_OP_WRITE_UNLOCK: >>>> + num_words = PSEQ_OP_END_WORDS; >>>> + break; >>>> + case PSEQ_OP_WRITE_ADDR8: >>>> + case PSEQ_OP_WRITE_H16: >>>> + case PSEQ_OP_WRITE_L16: >>>> + num_words = PSEQ_OP_WRITE_X16_WORDS; >>>> + break; >>>> + case PSEQ_OP_WRITE_FULL: >>>> + num_words = PSEQ_OP_WRITE_FULL_WORDS; >>>> + break; >>>> + default: >>>> + error = -EINVAL; >>>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation); >>>> + goto err_free; >>>> + } >>>> + >>>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); >>>> + if (!op) { >>>> + error = -ENOMEM; >>>> + goto err_free; >>>> + } >>>> + >>>> + op->size = num_words * sizeof(u32); >>>> + memcpy(op->words, &words[i], op->size); >>>> + op->offset = i * sizeof(u32); >>>> + op->operation = operation; >>>> + list_add(&op->list, &haptics->pseq_head); >>>> + >>>> + if (operation == PSEQ_OP_END) >>>> + break; >>>> + } >>>> + >>>> + if (operation != PSEQ_OP_END) >>>> + error = -ENOENT; >>>> + >>>> +err_free: >>>> + kfree(words); >>>> + >>>> + return error; >>>> +} >>> >>> My first thought as I reviewed this patch was that this and the lot >>> of pseq-related functions are not necessarily related to haptics, but >>> rather CS40L50 register access and housekeeping in general. >>> >>> I seem to recall on L25 and friends that the the power-on sequencer, >>> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory >>> that can play back a series of address/data pairs when the device >>> comes out of hibernation, and any registers written during runtime >>> must also be mirrored to the PSEQ for "playback" later. Is that still >>> the case here? >>> >>> Assuming so, these functions seem like they belong in the MFD, not >>> an input-specific library, because they will presumably be used by >>> the codec driver as well, since that driver will write registers to >>> set BCLK/LRCK ratio, etc. >>> >>> Therefore, I think it makes more sense for these functions to move to >>> the MFD, which can then export them for use by the input/FF and codec >>> children. >> >> I think the problem with moving the write sequencer code to the MFD >> driver is that it would go unused in a codec-only environment. We only >> need to write to the PSEQ when we want to maintain register settings >> throughout hibernation cycles, and it isn’t possible to hibernate when >> there is data streaming to the device. So the PSEQ will never be used >> in the codec driver. > > I agree that in practice, the write sequencer would technically go unused > in a codec-only implementation. However, that is because the ASoC core > happens to write all pertinent registers ahead-of-time each time a stream > starts. That is a property of the ASoC core and not L50; my feeling is that > the driver should not be structured based on what one of the subsystems > associated with it happens to do, but rather the nature of the hardware. > > Some specific reasons I think the MFD is a better home for the pseq code: > > 1. The write sequencer is a housekeeping function derived from the way > the hardware implements its power management; it doesn't have anything > to do with haptics. My opinion is that facilities supporting application- > agnostic functions belong in the MFD, for all children to access, even > if only one happens to do so today. > > 2. Over the course of the IC's life, you may be required to add errata > writes after the IC is taken out of reset; these presumably would need > to be "scheduled" in the write sequencer as well. These wouldn't make > logical sense to add in the input driver, and they would be missed in > the theoretical codec-only case. > > 3. This device has a programmable DSP; it wouldn't be unheard of for a > customer to ask for a new function down the road that begets a third > child device. Perhaps a customer asks for a special .wmfw file that > repurposes the SDOUT pin as a PWM output for an LED, and now you must > add a pwm child. That's a made-up example of course, but in general we > want to structure things in such a way that rip-up is minimal in case > requirements change in the future. Great points. I agree now that the write sequencer code ought not to go in cirrus_haptics.c. After talking it over with the internal team, I am considering moving the write sequencer interface to cs_dsp.c. It’s an already existing library with both Cirrus haptics and audio users. It seems to dodge your concerns above and avoids a new common library as you suggested below. Do you have any concerns on this approach over putting it in MFD? >>> This leaves cirrus_haptics.* with only a few functions related to >>> starting and stopping work, which seem specific enough to just live >>> in cs40l50-vibra.c. Presumably many of those could be re-used by >>> the L30 down the road, but in that case I think we'd be looking to >>> actually re-use the L50 driver and simply add a compatible string >>> for L30. >>> >>> I recall L30 has some overhead that L50 does not, which may make >>> it hairy for cs40l50-vibra.c to support both. But in that case, >>> you could always fork a cs40l30-vibra.c with its own compatible >>> string, then have the L50 MFD selectively load the correct child >>> based on device ID. To summarize, we should have: >>> >>> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery, >>> IRQ handling, exported PSEQ functions, etc. >>> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from >>> the MFD. >>> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop >>> work, also uses PSEQ library from the MFD. >>> >>> And down the road, depending on complexity, maybe we also have: >>> >>> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that >>> includes other functionality that didn't really fit in cs40l50-vibra.c; >>> also uses PSEQ library from, and is loaded by, the original L50 MFD. >>> If this driver duplicates small bits of cs40l50-vibra.c, it's not the >>> end of the world. >>> >>> All of these files would #include include/linux/mfd/cs40l50.h. And >>> finally, cirrus_haptics.* simply go away. Same idea, just slightly >>> more scalable, and closer to common design patterns. >> >> >> I understand that it is a common design pattern to selectively load >> devices from the MFD driver. If I could summarize my thoughts on why >> that would not be fitting here, it’s that the L26 and L50 share a ton of >> input FF related work, and not enough “MFD core” related work. >> Between errata differences, power supply configurations, regmap >> configurations, interrupt register differences, it would seem to make for >> a very awkward, scrambled MFD driver. Moreover, I think I will be moving >> firmware download to the MFD driver, and that alone constitutes a >> significant incompatibility because firmware downloading is compulsory >> on L26, not so on L50. >> >> On the other hand, I want to emphasize the amount that L26 and >> L50 share when it comes to the Input FF callbacks. The worker >> functions in cirrus_haptics.c are bare-bones for this first >> submission, but were designed to be totally generic and scalable to >> the needs of L26 and all future Cirrus input drivers. While it might appear >> too specific for L26, everything currently in cirrus_haptics is usable by >> L26 as-is. >> >> For the above reasons I favor the current approach. >> > > Likewise, if the input-related functions of L26 and L50 are nearly identical, > then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and > drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which > supports both L26 and L50 haptics-related functions. You're already doing > something similar, but I disagree on the following: > > 1. Rather than have a library referenced by two drivers that support children > which are largely the same in a logcial sense, just have a single driver that > supports two children. Your point here is clear and makes sense to me, especially now with the write sequencer interface moving out. After considering the similarities and differences closer, I am still a little wary. Maybe you can help me with these concerns: 1. In the current implementation, drivers would be able to configure their own input FF capabilities, and selectively register to input FF callbacks. L50 does not register to the set_gain callback, whereas L26 does. I anticipate future divergences, such as one driver supporting different effect types (see the L50-specific error checking in cs40l50_add()). This would be exacerbated by any future additional children. 2. This may be my lack of imagination, but in the current implementation it seems much easier to develop new haptic features that don’t apply to all the users of the library. One would simply wrap the feature in a boolean in cirrus_haptics, which clients can take or leave. In the one driver implementation, it seems you would have to find some clever, generalized way of determining whether or not a feature can be used. This would also seem to be exacerbated by any future additional children. 3. The current implementation provides for the individual drivers to setup the haptics interface in whatever way peculiar to that device, whether that interface be static (L50) or dependent on the loaded firmware (L26). Since I am moving around a lot of code in and out of both -vibra.c and the library for the next version, I think it would be helpful for me to wait until the next version is submitted to decide on this. Would that be acceptable? > > >> >>>> +static void cs_hap_vibe_stop_worker(struct work_struct *work) >>>> +{ >>>> + struct cs_hap *haptics = container_of(work, struct cs_hap, >>>> + vibe_stop_work); >>>> + int error; >>>> + >>>> + if (haptics->runtime_pm) { >>>> + error = pm_runtime_resume_and_get(haptics->dev); >>>> + if (error < 0) { >>>> + haptics->stop_error = error; >>>> + return; >>>> + } >>>> + } >>>> + >>>> + mutex_lock(&haptics->lock); >>>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, >>>> + haptics->dsp.stop_cmd); >>>> + mutex_unlock(&haptics->lock); >>>> + >>>> + if (haptics->runtime_pm) { >>>> + pm_runtime_mark_last_busy(haptics->dev); >>>> + pm_runtime_put_autosuspend(haptics->dev); >>>> + } >>>> + >>>> + haptics->stop_error = error; >>> >>> This seems like another argument for not separating the input/FF child >>> from the meat of the driver; it just seems messy to pass around error >>> codes within a driver's private data like this. >> >> I removed the start_error and stop_error members. However I think the >> erase_error and add_error need to stay. I think this is more of a symptom >> of the Input FF layer requiring error reporting and having to use workqueues >> for those Input FF callbacks, than it is to do with the separation of these >> functions from the driver. Point being, even if these were moved, we would still >> need these *_error members. Let me know if I understood you right here. > > Sure, but why do adding and removing waveforms require workqueues? The DA7280 > driver doesn't do this; what is different in this case? (That's a genuine > question, not an assertion that what you have is wrong, although it seems > unique based on my limited search). The reason why we have worker items for upload and erase input FF callbacks is because we need to ensure their ordering with playback worker items, and we need those worker items because the Input FF layer calls playbacks in atomic context. Best, James
Hi James, Apologies for the delayed response. On Wed, Nov 29, 2023 at 10:22:16PM +0000, James Ogletree wrote: > > >>>> + > >>>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); > >>>> + if (!words) > >>>> + return -ENOMEM; > >>>> + > >>>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, > >>>> + words, haptics->dsp.pseq_size); > >>>> + if (error) > >>>> + goto err_free; > >>>> + > >>>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { > >>>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); > >>>> + switch (operation) { > >>>> + case PSEQ_OP_END: > >>>> + case PSEQ_OP_WRITE_UNLOCK: > >>>> + num_words = PSEQ_OP_END_WORDS; > >>>> + break; > >>>> + case PSEQ_OP_WRITE_ADDR8: > >>>> + case PSEQ_OP_WRITE_H16: > >>>> + case PSEQ_OP_WRITE_L16: > >>>> + num_words = PSEQ_OP_WRITE_X16_WORDS; > >>>> + break; > >>>> + case PSEQ_OP_WRITE_FULL: > >>>> + num_words = PSEQ_OP_WRITE_FULL_WORDS; > >>>> + break; > >>>> + default: > >>>> + error = -EINVAL; > >>>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation); > >>>> + goto err_free; > >>>> + } > >>>> + > >>>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); > >>>> + if (!op) { > >>>> + error = -ENOMEM; > >>>> + goto err_free; > >>>> + } > >>>> + > >>>> + op->size = num_words * sizeof(u32); > >>>> + memcpy(op->words, &words[i], op->size); > >>>> + op->offset = i * sizeof(u32); > >>>> + op->operation = operation; > >>>> + list_add(&op->list, &haptics->pseq_head); > >>>> + > >>>> + if (operation == PSEQ_OP_END) > >>>> + break; > >>>> + } > >>>> + > >>>> + if (operation != PSEQ_OP_END) > >>>> + error = -ENOENT; > >>>> + > >>>> +err_free: > >>>> + kfree(words); > >>>> + > >>>> + return error; > >>>> +} > >>> > >>> My first thought as I reviewed this patch was that this and the lot > >>> of pseq-related functions are not necessarily related to haptics, but > >>> rather CS40L50 register access and housekeeping in general. > >>> > >>> I seem to recall on L25 and friends that the the power-on sequencer, > >>> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory > >>> that can play back a series of address/data pairs when the device > >>> comes out of hibernation, and any registers written during runtime > >>> must also be mirrored to the PSEQ for "playback" later. Is that still > >>> the case here? > >>> > >>> Assuming so, these functions seem like they belong in the MFD, not > >>> an input-specific library, because they will presumably be used by > >>> the codec driver as well, since that driver will write registers to > >>> set BCLK/LRCK ratio, etc. > >>> > >>> Therefore, I think it makes more sense for these functions to move to > >>> the MFD, which can then export them for use by the input/FF and codec > >>> children. > >> > >> I think the problem with moving the write sequencer code to the MFD > >> driver is that it would go unused in a codec-only environment. We only > >> need to write to the PSEQ when we want to maintain register settings > >> throughout hibernation cycles, and it isn’t possible to hibernate when > >> there is data streaming to the device. So the PSEQ will never be used > >> in the codec driver. > > > > I agree that in practice, the write sequencer would technically go unused > > in a codec-only implementation. However, that is because the ASoC core > > happens to write all pertinent registers ahead-of-time each time a stream > > starts. That is a property of the ASoC core and not L50; my feeling is that > > the driver should not be structured based on what one of the subsystems > > associated with it happens to do, but rather the nature of the hardware. > > > > Some specific reasons I think the MFD is a better home for the pseq code: > > > > 1. The write sequencer is a housekeeping function derived from the way > > the hardware implements its power management; it doesn't have anything > > to do with haptics. My opinion is that facilities supporting application- > > agnostic functions belong in the MFD, for all children to access, even > > if only one happens to do so today. > > > > 2. Over the course of the IC's life, you may be required to add errata > > writes after the IC is taken out of reset; these presumably would need > > to be "scheduled" in the write sequencer as well. These wouldn't make > > logical sense to add in the input driver, and they would be missed in > > the theoretical codec-only case. > > > > 3. This device has a programmable DSP; it wouldn't be unheard of for a > > customer to ask for a new function down the road that begets a third > > child device. Perhaps a customer asks for a special .wmfw file that > > repurposes the SDOUT pin as a PWM output for an LED, and now you must > > add a pwm child. That's a made-up example of course, but in general we > > want to structure things in such a way that rip-up is minimal in case > > requirements change in the future. > > Great points. I agree now that the write sequencer code ought not to go in > cirrus_haptics.c. After talking it over with the internal team, I am considering > moving the write sequencer interface to cs_dsp.c. It’s an already existing > library with both Cirrus haptics and audio users. It seems to dodge your > concerns above and avoids a new common library as you suggested > below. Do you have any concerns on this approach over putting it in MFD? I think that's a great idea. Not every DSP-equipped device has a write sequencer, but most, if not all write-sequencer-equipped devices have a DSP. Adding the write sequencer code to cs_dsp.c seems like a natural union of the two facilities. > > > >>> This leaves cirrus_haptics.* with only a few functions related to > >>> starting and stopping work, which seem specific enough to just live > >>> in cs40l50-vibra.c. Presumably many of those could be re-used by > >>> the L30 down the road, but in that case I think we'd be looking to > >>> actually re-use the L50 driver and simply add a compatible string > >>> for L30. > >>> > >>> I recall L30 has some overhead that L50 does not, which may make > >>> it hairy for cs40l50-vibra.c to support both. But in that case, > >>> you could always fork a cs40l30-vibra.c with its own compatible > >>> string, then have the L50 MFD selectively load the correct child > >>> based on device ID. To summarize, we should have: > >>> > >>> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery, > >>> IRQ handling, exported PSEQ functions, etc. > >>> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from > >>> the MFD. > >>> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop > >>> work, also uses PSEQ library from the MFD. > >>> > >>> And down the road, depending on complexity, maybe we also have: > >>> > >>> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that > >>> includes other functionality that didn't really fit in cs40l50-vibra.c; > >>> also uses PSEQ library from, and is loaded by, the original L50 MFD. > >>> If this driver duplicates small bits of cs40l50-vibra.c, it's not the > >>> end of the world. > >>> > >>> All of these files would #include include/linux/mfd/cs40l50.h. And > >>> finally, cirrus_haptics.* simply go away. Same idea, just slightly > >>> more scalable, and closer to common design patterns. > >> > >> > >> I understand that it is a common design pattern to selectively load > >> devices from the MFD driver. If I could summarize my thoughts on why > >> that would not be fitting here, it’s that the L26 and L50 share a ton of > >> input FF related work, and not enough “MFD core” related work. > >> Between errata differences, power supply configurations, regmap > >> configurations, interrupt register differences, it would seem to make for > >> a very awkward, scrambled MFD driver. Moreover, I think I will be moving > >> firmware download to the MFD driver, and that alone constitutes a > >> significant incompatibility because firmware downloading is compulsory > >> on L26, not so on L50. > >> > >> On the other hand, I want to emphasize the amount that L26 and > >> L50 share when it comes to the Input FF callbacks. The worker > >> functions in cirrus_haptics.c are bare-bones for this first > >> submission, but were designed to be totally generic and scalable to > >> the needs of L26 and all future Cirrus input drivers. While it might appear > >> too specific for L26, everything currently in cirrus_haptics is usable by > >> L26 as-is. > >> > >> For the above reasons I favor the current approach. > >> > > > > Likewise, if the input-related functions of L26 and L50 are nearly identical, > > then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and > > drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which > > supports both L26 and L50 haptics-related functions. You're already doing > > something similar, but I disagree on the following: > > > > 1. Rather than have a library referenced by two drivers that support children > > which are largely the same in a logcial sense, just have a single driver that > > supports two children. > > Your point here is clear and makes sense to me, especially now with the write > sequencer interface moving out. After considering the similarities and > differences closer, I am still a little wary. Maybe you can help me with these > concerns: > > 1. In the current implementation, drivers would be able to configure their own > input FF capabilities, and selectively register to input FF callbacks. L50 does > not register to the set_gain callback, whereas L26 does. I anticipate future > divergences, such as one driver supporting different effect types (see > the L50-specific error checking in cs40l50_add()). This would be exacerbated > by any future additional children. > > 2. This may be my lack of imagination, but in the current implementation it > seems much easier to develop new haptic features that don’t apply to all the > users of the library. One would simply wrap the feature in a boolean in > cirrus_haptics, which clients can take or leave. In the one driver > implementation, it seems you would have to find some clever, generalized > way of determining whether or not a feature can be used. This would also > seem to be exacerbated by any future additional children. > > 3. The current implementation provides for the individual drivers to setup > the haptics interface in whatever way peculiar to that device, whether that > interface be static (L50) or dependent on the loaded firmware (L26). > Maybe the solution here is to define a "descriptor", that is a struct which reserves members for all things that can vary based on device (FF callback pointers, input device capabilities, etc.). You then define an array of such structs, with L50 being the only member, and L26 and others to come. Then, cs40l50_vibra_probe() would simply parse the array index based on the device ID hinted to it by the parent MFD. As the person not doing the actual work, my naive opinion is that you face the same problems whether two different drivers reference a library, or a common driver supports two devices. The problem is therefore cosmetic, and if so, we should tend toward a design pattern that seems to be most common in the input subsystem, which is to support as many devices as possible with a single driver. > Since I am moving around a lot of code in and out of both -vibra.c and the > library for the next version, I think it would be helpful for me to wait until the > next version is submitted to decide on this. Would that be acceptable? I very much support moving in incremental steps; if you still feel strongly against my suggestion, let us by all means evaluate your approach in the context of the other changes. > > > > > > >> > >>>> +static void cs_hap_vibe_stop_worker(struct work_struct *work) > >>>> +{ > >>>> + struct cs_hap *haptics = container_of(work, struct cs_hap, > >>>> + vibe_stop_work); > >>>> + int error; > >>>> + > >>>> + if (haptics->runtime_pm) { > >>>> + error = pm_runtime_resume_and_get(haptics->dev); > >>>> + if (error < 0) { > >>>> + haptics->stop_error = error; > >>>> + return; > >>>> + } > >>>> + } > >>>> + > >>>> + mutex_lock(&haptics->lock); > >>>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, > >>>> + haptics->dsp.stop_cmd); > >>>> + mutex_unlock(&haptics->lock); > >>>> + > >>>> + if (haptics->runtime_pm) { > >>>> + pm_runtime_mark_last_busy(haptics->dev); > >>>> + pm_runtime_put_autosuspend(haptics->dev); > >>>> + } > >>>> + > >>>> + haptics->stop_error = error; > >>> > >>> This seems like another argument for not separating the input/FF child > >>> from the meat of the driver; it just seems messy to pass around error > >>> codes within a driver's private data like this. > >> > >> I removed the start_error and stop_error members. However I think the > >> erase_error and add_error need to stay. I think this is more of a symptom > >> of the Input FF layer requiring error reporting and having to use workqueues > >> for those Input FF callbacks, than it is to do with the separation of these > >> functions from the driver. Point being, even if these were moved, we would still > >> need these *_error members. Let me know if I understood you right here. > > > > Sure, but why do adding and removing waveforms require workqueues? The DA7280 > > driver doesn't do this; what is different in this case? (That's a genuine > > question, not an assertion that what you have is wrong, although it seems > > unique based on my limited search). > > The reason why we have worker items for upload and erase input FF callbacks is > because we need to ensure their ordering with playback worker items, and we need > those worker items because the Input FF layer calls playbacks in atomic context. Got it, that makes sense. Presumably, the act of adding or deleting waveforms directly manipulates XMEM and YMEM regions of the DSP, and we cannot do so while they are actively being read during playback. Acknowledged on all counts. > > Best, > James > > Kind regards, Jeff LaBundy
diff --git a/MAINTAINERS b/MAINTAINERS index 28f0ca9324b3..57daf77bf550 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4970,6 +4970,8 @@ M: Ben Bright <ben.bright@cirrus.com> L: patches@opensource.cirrus.com S: Supported F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml +F: drivers/input/misc/cirrus* +F: include/linux/input/cirrus* CIRRUS LOGIC DSP FIRMWARE DRIVER M: Simon Trimmer <simont@opensource.cirrus.com> diff --git a/drivers/input/misc/cirrus_haptics.c b/drivers/input/misc/cirrus_haptics.c new file mode 100644 index 000000000000..7e539cd45167 --- /dev/null +++ b/drivers/input/misc/cirrus_haptics.c @@ -0,0 +1,586 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Helper functions for dealing with wavetable + * formats and DSP interfaces used by Cirrus + * haptic drivers. + * + * Copyright 2023 Cirrus Logic, Inc. + */ + +#include <linux/firmware/cirrus/cs_dsp.h> +#include <linux/input.h> +#include <linux/input/cirrus_haptics.h> +#include <linux/pm_runtime.h> + +static int cs_hap_pseq_init(struct cs_hap *haptics) +{ + struct cs_hap_pseq_op *op; + int error, i, num_words; + u8 operation; + u32 *words; + + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg) + return 0; + + INIT_LIST_HEAD(&haptics->pseq_head); + + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL); + if (!words) + return -ENOMEM; + + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg, + words, haptics->dsp.pseq_size); + if (error) + goto err_free; + + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) { + operation = FIELD_GET(PSEQ_OP_MASK, words[i]); + switch (operation) { + case PSEQ_OP_END: + case PSEQ_OP_WRITE_UNLOCK: + num_words = PSEQ_OP_END_WORDS; + break; + case PSEQ_OP_WRITE_ADDR8: + case PSEQ_OP_WRITE_H16: + case PSEQ_OP_WRITE_L16: + num_words = PSEQ_OP_WRITE_X16_WORDS; + break; + case PSEQ_OP_WRITE_FULL: + num_words = PSEQ_OP_WRITE_FULL_WORDS; + break; + default: + error = -EINVAL; + dev_err(haptics->dev, "Unsupported op: %u\n", operation); + goto err_free; + } + + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL); + if (!op) { + error = -ENOMEM; + goto err_free; + } + + op->size = num_words * sizeof(u32); + memcpy(op->words, &words[i], op->size); + op->offset = i * sizeof(u32); + op->operation = operation; + list_add(&op->list, &haptics->pseq_head); + + if (operation == PSEQ_OP_END) + break; + } + + if (operation != PSEQ_OP_END) + error = -ENOENT; + +err_free: + kfree(words); + + return error; +} + +static int cs_hap_pseq_find_end(struct cs_hap *haptics, + struct cs_hap_pseq_op **op_end) +{ + u8 operation = PSEQ_OP_WRITE_FULL; + struct cs_hap_pseq_op *op; + + list_for_each_entry(op, &haptics->pseq_head, list) { + operation = op->operation; + if (operation == PSEQ_OP_END) + break; + } + + if (operation != PSEQ_OP_END) { + dev_err(haptics->dev, "Missing PSEQ list terminator\n"); + return -ENOENT; + } + + *op_end = op; + + return 0; +} + +static struct cs_hap_pseq_op *cs_hap_pseq_find_op(struct cs_hap_pseq_op *match_op, + struct list_head *pseq_head) +{ + struct cs_hap_pseq_op *op; + + list_for_each_entry(op, pseq_head, list) { + if (op->operation == PSEQ_OP_END) + break; + if (op->operation != match_op->operation || + op->words[0] != match_op->words[0]) + continue; + switch (op->operation) { + case PSEQ_OP_WRITE_FULL: + if (FIELD_GET(GENMASK(23, 8), op->words[1]) == + FIELD_GET(GENMASK(23, 8), match_op->words[1])) + return op; + break; + case PSEQ_OP_WRITE_H16: + case PSEQ_OP_WRITE_L16: + if (FIELD_GET(GENMASK(23, 16), op->words[1]) == + FIELD_GET(GENMASK(23, 16), match_op->words[1])) + return op; + break; + default: + break; + } + } + + return NULL; +} + +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, + u32 data, bool update, u8 op_code) +{ + struct cs_hap_pseq_op *op, *op_end, *op_new; + struct cs_dsp_chunk ch; + u32 pseq_bytes; + int error; + + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL); + if (!op_new) + return -ENOMEM; + + op_new->operation = op_code; + + ch = cs_dsp_chunk((void *) op_new->words, + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32)); + cs_dsp_chunk_write(&ch, 8, op_code); + switch (op_code) { + case PSEQ_OP_WRITE_FULL: + cs_dsp_chunk_write(&ch, 32, addr); + cs_dsp_chunk_write(&ch, 32, data); + break; + case PSEQ_OP_WRITE_L16: + case PSEQ_OP_WRITE_H16: + cs_dsp_chunk_write(&ch, 24, addr); + cs_dsp_chunk_write(&ch, 16, data); + break; + default: + error = -EINVAL; + goto op_new_free; + } + + op_new->size = cs_dsp_chunk_bytes(&ch); + + if (update) { + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head); + if (!op) { + error = -EINVAL; + goto op_new_free; + } + } + + error = cs_hap_pseq_find_end(haptics, &op_end); + if (error) + goto op_new_free; + + pseq_bytes = haptics->dsp.pseq_size * sizeof(u32); + + if (pseq_bytes - op_end->offset < op_new->size) { + error = -ENOMEM; + goto op_new_free; + } + + if (update) { + op_new->offset = op->offset; + } else { + op_new->offset = op_end->offset; + op_end->offset += op_new->size; + } + + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg + + op_new->offset, op_new->words, op_new->size); + if (error) + goto op_new_free; + + if (update) { + list_replace(&op->list, &op_new->list); + } else { + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg + + op_end->offset, op_end->words, + op_end->size); + if (error) + goto op_new_free; + + list_add(&op_new->list, &haptics->pseq_head); + } + + return 0; + +op_new_free: + devm_kfree(haptics->dev, op_new); + + return error; +} +EXPORT_SYMBOL_GPL(cs_hap_pseq_write); + +int cs_hap_pseq_multi_write(struct cs_hap *haptics, + const struct reg_sequence *reg_seq, + int num_regs, bool update, u8 op_code) +{ + int error, i; + + for (i = 0; i < num_regs; i++) { + error = cs_hap_pseq_write(haptics, reg_seq[i].reg, + reg_seq[i].def, update, op_code); + if (error) + return error; + } + + return 0; +} +EXPORT_SYMBOL_GPL(cs_hap_pseq_multi_write); + +static struct cs_hap_effect *cs_hap_find_effect(int id, + struct list_head *effect_head) +{ + struct cs_hap_effect *effect; + + list_for_each_entry(effect, effect_head, list) + if (effect->id == id) + return effect; + + return NULL; +} + +static int cs_hap_effect_bank_set(struct cs_hap *haptics, + struct cs_hap_effect *effect, + struct ff_periodic_effect add_effect) +{ + s16 bank = add_effect.custom_data[0] & 0xffffu; + unsigned int len = add_effect.custom_len; + + if (bank >= WVFRM_BANK_NUM) { + dev_err(haptics->dev, "Invalid waveform bank: %d\n", bank); + return -EINVAL; + } + + effect->bank = len > CUSTOM_DATA_SIZE ? WVFRM_BANK_OWT : bank; + + return 0; +} + +static int cs_hap_effect_mapping_set(struct cs_hap *haptics, u16 button, + struct cs_hap_effect *effect) +{ + u32 gpio_num, gpio_edge; + + if (button) { + gpio_num = FIELD_GET(BTN_NUM_MASK, button); + gpio_edge = FIELD_GET(BTN_EDGE_MASK, button); + effect->mapping = haptics->dsp.gpio_base_reg + + (gpio_num * 8) - gpio_edge; + + return regmap_write(haptics->regmap, effect->mapping, button); + } + + effect->mapping = GPIO_MAPPING_INVALID; + + return 0; +} + +static int cs_hap_effect_index_set(struct cs_hap *haptics, + struct cs_hap_effect *effect, + struct ff_periodic_effect add_effect) +{ + struct cs_hap_effect *owt_effect; + u32 base_index, max_index; + + base_index = haptics->banks[effect->bank].base_index; + max_index = haptics->banks[effect->bank].max_index; + + effect->index = base_index; + + switch (effect->bank) { + case WVFRM_BANK_OWT: + list_for_each_entry(owt_effect, &haptics->effect_head, list) + if (owt_effect->bank == WVFRM_BANK_OWT) + effect->index++; + break; + case WVFRM_BANK_ROM: + case WVFRM_BANK_RAM: + effect->index += add_effect.custom_data[1] & 0xffffu; + break; + default: + dev_err(haptics->dev, "Bank not supported: %d\n", effect->bank); + return -EINVAL; + } + + if (effect->index > max_index || effect->index < base_index) { + dev_err(haptics->dev, "Index out of bounds: %u\n", effect->index); + return -ENOSPC; + } + + return 0; +} + +static int cs_hap_upload_pwle(struct cs_hap *haptics, + struct cs_hap_effect *effect, + struct ff_periodic_effect add_effect) +{ + u32 len, wt_offset, wt_size_words; + struct cs_hap_pwle_header header; + u8 *out_data; + int error; + + error = regmap_read(haptics->regmap, haptics->dsp.owt_offset_reg, + &wt_offset); + if (error) + return error; + + error = regmap_read(haptics->regmap, haptics->dsp.owt_size_reg, + &wt_size_words); + if (error) + return error; + + len = 2 * add_effect.custom_len; + + if ((wt_size_words * sizeof(u32)) < OWT_HEADER_SIZE + len) + return -ENOSPC; + + out_data = kzalloc(OWT_HEADER_SIZE + len, GFP_KERNEL); + if (!out_data) + return -ENOMEM; + + header.type = add_effect.custom_data[0] == PCM_ID ? OWT_TYPE_PCM : + OWT_TYPE_PWLE; + header.offset = OWT_HEADER_SIZE / sizeof(u32); + header.data_words = len / sizeof(u32); + + memcpy(out_data, &header, sizeof(header)); + memcpy(out_data + OWT_HEADER_SIZE, add_effect.custom_data, len); + + error = regmap_bulk_write(haptics->regmap, haptics->dsp.owt_base_reg + + (wt_offset * sizeof(u32)), out_data, + OWT_HEADER_SIZE + len); + if (error) + goto err_free; + + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, + haptics->dsp.push_owt_cmd); + +err_free: + kfree(out_data); + + return error; +} + +static void cs_hap_add_worker(struct work_struct *work) +{ + struct cs_hap *haptics = container_of(work, struct cs_hap, + add_work); + struct ff_effect add_effect = haptics->add_effect; + bool is_new = false; + struct cs_hap_effect *effect; + int error; + + if (haptics->runtime_pm) { + error = pm_runtime_resume_and_get(haptics->dev); + if (error < 0) { + haptics->add_error = error; + return; + } + } + + mutex_lock(&haptics->lock); + + effect = cs_hap_find_effect(add_effect.id, &haptics->effect_head); + if (!effect) { + effect = kzalloc(sizeof(*effect), GFP_KERNEL); + if (!effect) { + error = -ENOMEM; + goto err_mutex; + } + effect->id = add_effect.id; + is_new = true; + } + + error = cs_hap_effect_bank_set(haptics, effect, add_effect.u.periodic); + if (error) + goto err_free; + + error = cs_hap_effect_index_set(haptics, effect, add_effect.u.periodic); + if (error) + goto err_free; + + error = cs_hap_effect_mapping_set(haptics, add_effect.trigger.button, + effect); + if (error) + goto err_free; + + if (effect->bank == WVFRM_BANK_OWT) + error = cs_hap_upload_pwle(haptics, effect, + add_effect.u.periodic); + +err_free: + if (is_new) { + if (error) + kfree(effect); + else + list_add(&effect->list, &haptics->effect_head); + } + +err_mutex: + mutex_unlock(&haptics->lock); + + if (haptics->runtime_pm) { + pm_runtime_mark_last_busy(haptics->dev); + pm_runtime_put_autosuspend(haptics->dev); + } + + haptics->add_error = error; +} + +static void cs_hap_erase_worker(struct work_struct *work) +{ + struct cs_hap *haptics = container_of(work, struct cs_hap, + erase_work); + int error = 0; + struct cs_hap_effect *owt_effect, *erase_effect; + + if (haptics->runtime_pm) { + error = pm_runtime_resume_and_get(haptics->dev); + if (error < 0) { + haptics->erase_error = error; + return; + } + } + + mutex_lock(&haptics->lock); + + erase_effect = cs_hap_find_effect(haptics->erase_effect->id, + &haptics->effect_head); + if (!erase_effect) { + dev_err(haptics->dev, "Effect to erase does not exist\n"); + error = -EINVAL; + goto err_mutex; + } + + if (erase_effect->mapping != GPIO_MAPPING_INVALID) { + error = regmap_write(haptics->regmap, erase_effect->mapping, + GPIO_DISABLE); + if (error) + goto err_mutex; + } + + if (erase_effect->bank == WVFRM_BANK_OWT) { + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, + haptics->dsp.delete_owt_cmd | + erase_effect->index); + if (error) + goto err_mutex; + + list_for_each_entry(owt_effect, &haptics->effect_head, list) + if (owt_effect->bank == WVFRM_BANK_OWT && + owt_effect->index > erase_effect->index) + owt_effect->index--; + } + + list_del(&erase_effect->list); + kfree(erase_effect); + +err_mutex: + mutex_unlock(&haptics->lock); + + if (haptics->runtime_pm) { + pm_runtime_mark_last_busy(haptics->dev); + pm_runtime_put_autosuspend(haptics->dev); + } + + haptics->erase_error = error; +} + +static void cs_hap_vibe_start_worker(struct work_struct *work) +{ + struct cs_hap *haptics = container_of(work, struct cs_hap, + vibe_start_work); + struct cs_hap_effect *effect; + int error; + + if (haptics->runtime_pm) { + error = pm_runtime_resume_and_get(haptics->dev); + if (error < 0) { + haptics->start_error = error; + return; + } + } + + mutex_lock(&haptics->lock); + + effect = cs_hap_find_effect(haptics->start_effect->id, + &haptics->effect_head); + if (effect) { + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, + effect->index); + } else { + dev_err(haptics->dev, "Effect to start does not exist\n"); + error = -EINVAL; + } + + mutex_unlock(&haptics->lock); + + if (haptics->runtime_pm) { + pm_runtime_mark_last_busy(haptics->dev); + pm_runtime_put_autosuspend(haptics->dev); + } + + haptics->start_error = error; +} + +static void cs_hap_vibe_stop_worker(struct work_struct *work) +{ + struct cs_hap *haptics = container_of(work, struct cs_hap, + vibe_stop_work); + int error; + + if (haptics->runtime_pm) { + error = pm_runtime_resume_and_get(haptics->dev); + if (error < 0) { + haptics->stop_error = error; + return; + } + } + + mutex_lock(&haptics->lock); + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg, + haptics->dsp.stop_cmd); + mutex_unlock(&haptics->lock); + + if (haptics->runtime_pm) { + pm_runtime_mark_last_busy(haptics->dev); + pm_runtime_put_autosuspend(haptics->dev); + } + + haptics->stop_error = error; +} + +int cs_hap_init(struct cs_hap *haptics) +{ + haptics->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0); + if (!haptics->vibe_wq) + return -ENOMEM; + + mutex_init(&haptics->lock); + + INIT_WORK(&haptics->vibe_start_work, cs_hap_vibe_start_worker); + INIT_WORK(&haptics->vibe_stop_work, cs_hap_vibe_stop_worker); + INIT_WORK(&haptics->erase_work, cs_hap_erase_worker); + INIT_WORK(&haptics->add_work, cs_hap_add_worker); + + return cs_hap_pseq_init(haptics); +} +EXPORT_SYMBOL_GPL(cs_hap_init); + +void cs_hap_remove(struct cs_hap *haptics) +{ + flush_workqueue(haptics->vibe_wq); + destroy_workqueue(haptics->vibe_wq); +} +EXPORT_SYMBOL_GPL(cs_hap_remove); + +MODULE_DESCRIPTION("Cirrus Logic Haptics Support"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/input/cirrus_haptics.h b/include/linux/input/cirrus_haptics.h new file mode 100644 index 000000000000..42f6afed7944 --- /dev/null +++ b/include/linux/input/cirrus_haptics.h @@ -0,0 +1,121 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Helper functions for dealing with wavetable + * formats and DSP interfaces used by Cirrus + * haptic drivers. + * + * Copyright 2023 Cirrus Logic, Inc. + */ + +#ifndef __CIRRUS_HAPTICS_H +#define __CIRRUS_HAPTICS_H + +#include <linux/property.h> +#include <linux/regmap.h> + +/* Power-on write sequencer */ +#define PSEQ_OP_MASK GENMASK(23, 16) +#define PSEQ_OP_SHIFT 16 +#define PSEQ_OP_WRITE_FULL_WORDS 3 +#define PSEQ_OP_WRITE_X16_WORDS 2 +#define PSEQ_OP_END_WORDS 1 +#define PSEQ_OP_WRITE_FULL 0x00 +#define PSEQ_OP_WRITE_ADDR8 0x02 +#define PSEQ_OP_WRITE_L16 0x04 +#define PSEQ_OP_WRITE_H16 0x05 +#define PSEQ_OP_WRITE_UNLOCK 0xFD +#define PSEQ_OP_END 0xFF + +/* Open wavetable */ +#define OWT_HEADER_SIZE 12 +#define OWT_TYPE_PCM 8 +#define OWT_TYPE_PWLE 12 +#define PCM_ID 0x0 +#define CUSTOM_DATA_SIZE 2 + +/* GPIO */ +#define BTN_NUM_MASK GENMASK(14, 12) +#define BTN_EDGE_MASK BIT(15) +#define GPIO_MAPPING_INVALID 0 +#define GPIO_DISABLE 0x1FF + +enum cs_hap_bank_type { + WVFRM_BANK_RAM, + WVFRM_BANK_ROM, + WVFRM_BANK_OWT, + WVFRM_BANK_NUM, +}; + +struct cs_hap_pseq_op { + struct list_head list; + u32 words[3]; + u16 offset; + u8 operation; + u8 size; +}; + +struct cs_hap_effect { + enum cs_hap_bank_type bank; + struct list_head list; + u32 mapping; + u32 index; + int id; +}; + +struct cs_hap_pwle_header { + u32 type; + u32 data_words; + u32 offset; +} __packed; + +struct cs_hap_bank { + enum cs_hap_bank_type bank; + u32 base_index; + u32 max_index; +}; + +struct cs_hap_dsp { + u32 gpio_base_reg; + u32 owt_offset_reg; + u32 owt_size_reg; + u32 owt_base_reg; + u32 mailbox_reg; + u32 pseq_reg; + u32 push_owt_cmd; + u32 delete_owt_cmd; + u32 stop_cmd; + u32 pseq_size; +}; + +struct cs_hap { + struct regmap *regmap; + struct mutex lock; + struct device *dev; + struct list_head pseq_head; + struct cs_hap_bank *banks; + struct cs_hap_dsp dsp; + struct workqueue_struct *vibe_wq; + struct work_struct vibe_start_work; + struct work_struct vibe_stop_work; + struct work_struct erase_work; + struct work_struct add_work; + struct ff_effect *start_effect; + struct ff_effect *erase_effect; + struct ff_effect add_effect; + struct list_head effect_head; + int erase_error; + int start_error; + int stop_error; + int add_error; + bool runtime_pm; +}; + +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr, + u32 data, bool update, u8 op_code); +int cs_hap_pseq_multi_write(struct cs_hap *haptics, + const struct reg_sequence *reg_seq, + int num_regs, bool update, u8 op_code); +int cs_hap_init(struct cs_hap *haptics); +void cs_hap_remove(struct cs_hap *haptics); + +#endif