Message-ID: <6DECA9DD-B464-446A-B6A1-5EECD4FF5E0B@cirrus.com>
Date:   Wed, 1 Nov 2023 20:46:12 +0000
From:   James Ogletree <James.Ogletree@...rus.com>
To:     Jeff LaBundy <jeff@...undy.com>
CC:     James Ogletree <james.ogletree@...nsource.cirrus.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Lee Jones <lee@...nel.org>,
        Fred Treven <Fred.Treven@...rus.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] Input: cs40l50 - Add cirrus haptics base support

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@...undy.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

Powered by blists - more mailing lists