Message-ID: <6ABC85BA-AF2D-4D2E-8CA8-32E372570DA0@cirrus.com>
Date:   Fri, 14 Apr 2023 20:51:48 +0000
From:   Fred Treven <Fred.Treven@...rus.com>
To:     Jeff LaBundy <jeff@...undy.com>
CC:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        James Ogletree <James.Ogletree@...rus.com>,
        "lee@...nel.org" <lee@...nel.org>,
        "jdelvare@...e.de" <jdelvare@...e.de>,
        "joel@....id.au" <joel@....id.au>,
        "cy_huang@...htek.com" <cy_huang@...htek.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "eajames@...ux.ibm.com" <eajames@...ux.ibm.com>,
        "ping.bai@....com" <ping.bai@....com>,
        "msp@...libre.com" <msp@...libre.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "bartosz.golaszewski@...aro.org" <bartosz.golaszewski@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "patches@...nsource.cirrus.com" <patches@...nsource.cirrus.com>
Subject: Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic
 Amplifier


>> +
>> + return cs40l26_probe(cs40l26, pdata);
>> +}
>> +
>> +static void cs40l26_i2c_remove(struct i2c_client *client)
>> +{
>> + struct cs40l26_private *cs40l26 = i2c_get_clientdata(client);
>> +
>> + cs40l26_remove(cs40l26);
>> +}
>> +
>> +static struct i2c_driver cs40l26_i2c_driver = {
>> + .driver = {
>> + .name = "cs40l26",
>> + .of_match_table = cs40l26_of_match,
>> + .pm = &cs40l26_pm_ops,
> 
> Please guard this with the new pm_sleep_ptr(), as not all platforms would
> define CONFIG_PM. More comments in the core driver.

Understood, and I certainly agree with this change. One thing I’m unsure of is what the effect would be on the driver if CONFIG_PM is not set in the .config. Surely, this driver would not work as expected right? Should I add a dependency in the kconfig to avoid building the driver without CONFIG_PM?
> 
>> +{
>> + size_t len_words = len_bytes / sizeof(__be32);
>> + struct cs_dsp_coeff_ctl *ctl;
>> + __be32 *val;
>> + int i, ret;
>> +
>> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
>> + if (IS_ERR_OR_NULL(ctl)) {
>> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
>> + return -ENOENT;
>> + }
>> +
>> + val = kzalloc(len_bytes, GFP_KERNEL);
>> + if (!val)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < len_words; i++)
>> + val[i] = cpu_to_be32(buf[i]);
>> +
>> + ret = cs_dsp_coeff_write_ctrl(ctl, off_words, val, len_bytes);
>> + if (ret)
>> + dev_err(dsp->dev, "Failed to write fw ctl %s: %d\n", name, ret);
>> +
>> + kfree(val);
>> +
>> + return ret;
>> +}
>> +
>> +static inline int cs40l26_fw_ctl_write(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, u32 val)
>> +{
>> + return cs40l26_fw_ctl_write_raw(dsp, name, algo_id, 0, sizeof(u32), &val);
>> +}
>> +
>> +static int cs40l26_fw_ctl_read_raw(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, unsigned int off_words, size_t len_bytes, u32 *buf)
>> +{
>> + size_t len_words = len_bytes / sizeof(u32);
>> + struct cs_dsp_coeff_ctl *ctl;
>> + int i, ret;
>> +
>> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
>> + if (IS_ERR_OR_NULL(ctl)) {
>> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
>> + return -ENOENT;
>> + }
>> +
>> + ret = cs_dsp_coeff_read_ctrl(ctl, off_words, buf, len_bytes);
>> + if (ret) {
>> + dev_err(dsp->dev, "Failed to read fw ctl %s: %d\n", name, ret);
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < len_words; i++)
>> + buf[i] = be32_to_cpu(buf[i]);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int cs40l26_fw_ctl_read(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, u32 *buf)
>> +{
>> + return cs40l26_fw_ctl_read_raw(dsp, name, algo_id, 0, sizeof(u32), buf);
>> +}
> 
> None of these four functions seem particularly specific to L26; is there any
> reason they don't belong in cs_dsp or wm_adsp? In fact, some of the functions
> throughout those drivers seem to be doing similar work.
> 
> Maybe out of scope for this particular submission, but is there not any room
> for re-use here?
> 
I wanted to avoid making too many changes to the firmware drivers in this initial submission, and I think I’d like to keep it as-is for now, but I think moving this combination to cs_dsp is the right move. Let me know if you feel that it would be better to make the change now rather than later.

> On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
>> On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
>>> On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
>>>> On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
>>>> I would far rather not have every single attempt to communicate
>>>> with the device wrapped in a retry if the communication failed
>>>> incase the device is hibernating. It seems much cleaner, and less
>>>> likely to risk odd behaviour, to know we have brought the device
>>>> out of hibernation.
>> 
>>> A common way to deal with this is that of [1], where the bus calls
>>> are simply wrapped with all retry logic limited to two places (read
>>> and write). These functions could also print the register address
>>> in case of failure, solving the problem of having dozens of custom
>>> error messages thorughout the driver.
>> 
>> I suspect this really comes down to a matter of taste, but my
>> thoughts would be that the code is shorter that way, but not
>> necessarily simpler. This feels far more error prone and likely
>> to encounter issues where the device hibernates at a time someone
>> hadn't properly thought through. I am far more comfortable with
>> the device is blocked from hibernating whilst the driver is
>> actively engaged with it and it keeps any special handling for
>> exiting hibernate in one place.
> 
> Fair enough. I do concede that having this control in the driver as
> opposed to DSP FW is more nimble and makes it easier to respond to
> customer issues; I'm sure your battle scars will agree :)

I concur with Charles here, and it seems like you’re also ok with this so I will leave it as-is.


>>> +static int cs40l26_irq_update_mask(struct cs40l26_private *cs40l26, u32 reg, u32 val, u32 bit_mask)
>>> +{
>>> + u32 eint_reg, cur_mask, new_mask;
>>> + int ret;
>>> +
>>> + if (reg == CS40L26_IRQ1_MASK_1) {
>>> + eint_reg = CS40L26_IRQ1_EINT_1;
>>> + } else if (reg == CS40L26_IRQ1_MASK_2) {
>>> + eint_reg = CS40L26_IRQ1_EINT_2;
>>> + } else {
>>> + dev_err(cs40l26->dev, "Invalid IRQ mask reg: 0x%08X\n", reg);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = regmap_read(cs40l26->regmap, reg, &cur_mask);
>>> + if (ret) {
>>> + dev_err(cs40l26->dev, "Failed to get IRQ mask\n");
>> 
>> Having a custom error message for every possible failed register read
>> does not ultimately aid in debugging and unnecessarily grows the size
>> of the driver.
>> 
>> If a specific message is absolutely necessary, then add wrappers around
>> regmap_read/write and print the failed address. However, this does not
>> seem necessary either. Simply propagate the error code all the way up
>> to the caller, whether it is probe or a sysfs attribute.
>> 
>> Stated another way:
>> 
>> error = regmap_...(...);
>> if (error)
>> return error;
>> 
> 
> Not sure I feel super strongly on this one, but I do find when
> debugging issues on drivers that do this that I usually end up
> adding the printks back in.

I went ahead and implemented the change Jeff suggested here; it does streamline the driver to a good degree, and I think it’s worth making the adjustment.

>>> +static int cs40l26_dsp_state_get(struct cs40l26_private *cs40l26, u8 *state)
>>> +{
>>> + bool mutex_available = !mutex_is_locked(&cs40l26->dsp.pwr_lock);
>> 
>> This is dangerous and a sign that locks are not properly managed. What would
>> be a case where you do not know the state of the lock upon entering this function?
>> If you do not know whether the mutex is locked inside this function, it is not the
>> proper place to grab it.
>> 
> 
> Yes I whole heartedly agree here this should not be done this
> way.

I certainly understand the concern here, but I wanted to provide some context. Since cs40l26_dsp_state_get() is called both in the cs_dsp_pre_run() callback as well as outside of this, there are instances where pwr_lock needs to be grabbed and times when it is already taken (in the instance of the callback invocation). Because of this, attempting to take the lock during pre_run causes a deadlock. I felt that it was quite clear when the lock would be available and when it wouldn’t be and to avoid the deadlock, I checked whether or not the lock was available. What do y’all feel is the best way to handle this? Some separate variable to determine if we are in the pre_run sequence or not? A separate function for dsp_state_get() for when it is not invoked from the callback?

> 
>> +
>> + cs40l26_pm_runtime_teardown(cs40l26);
>> +
>> + if (cs40l26->dsp.running)
>> + cs_dsp_stop(&cs40l26->dsp);
>> + if (cs40l26->dsp.booted)
>> + cs_dsp_power_down(&cs40l26->dsp);
>> + if (&cs40l26->dsp)
>> + cs_dsp_remove(&cs40l26->dsp);
>> +
>> + if (cs40l26->vibe_workqueue) {
>> + cancel_work_sync(&cs40l26->erase_work);
>> + cancel_work_sync(&cs40l26->set_gain_work);
>> + cancel_work_sync(&cs40l26->upload_work);
>> + cancel_work_sync(&cs40l26->vibe_start_work);
>> + cancel_work_sync(&cs40l26->vibe_stop_work);
>> + destroy_workqueue(cs40l26->vibe_workqueue);
>> + }
>> +
>> + mutex_destroy(&cs40l26->lock);
> 
> This ultimately does nothing.

Could you please clarify a bit what you mean here? Does the mutex not need to be destroyed explicitly? What about the work queue? Is it canceled/destroyed automatically when the module is removed? Are the cs_dsp functions not necessary to gracefully stop the DSP etc.?

In v2 of this patch, I plan to implement all of the suggestions in your comments that I didn’t question or disagree with here. Thanks for your in-depth analysis.

Best regards,
Fred

Powered by blists - more mailing lists