Re: [PATCH v4 2/4] Input: cs40l50 - Add cirrus haptics base support

[James Ogletree]

>>>> +
>>>> + 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