Message-ID: <ZDSqfHemG8pKj1k7@nixie71>
Date:   Mon, 10 Apr 2023 19:31:56 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc:     Fred Treven <fred.treven@...rus.com>, dmitry.torokhov@...il.com,
        ben.bright@...rus.com, james.ogletree@...rus.com, lee@...nel.org,
        jdelvare@...e.de, joel@....id.au, cy_huang@...htek.com,
        rdunlap@...radead.org, eajames@...ux.ibm.com, ping.bai@....com,
        msp@...libre.com, arnd@...db.de, bartosz.golaszewski@...aro.org,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic
 Amplifier

Hi Charles,

Thank you for the additional follow-up.

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:
> 
> Hi Jeff,
> 
> Thanks for the detailed review, very much appreciated. I agree
> with most of your comments, save a couple of exceptions below,
> well and one that I just felt the need to agree with you
> explictly:
> 
> > > +static bool cs40l26_readable_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +	switch (reg) {
> > > +	case CS40L26_DEVID ... CS40L26_REVID:
> > > +	case CS40L26_TEST_KEY_CTRL:
> > > +	case CS40L26_GLOBAL_ENABLES:
> > > +	case CS40L26_ERROR_RELEASE:
> > > +	case CS40L26_PWRMGT_CTL ... CS40L26_PWRMGT_STS:
> > > +	case CS40L26_REFCLK_INPUT:
> > > +	case CS40L26_PLL_REFCLK_DETECT_0:
> > > +	case CS40L26_VBST_CTL_1 ... CS40L26_BST_IPK_CTL:
> > > +	case CS40L26_TEST_LBST:
> > > +	case CS40L26_NGATE1_INPUT:
> > > +	case CS40L26_DAC_MSM_CONFIG ... CS40L26_TST_DAC_MSM_CONFIG:
> > > +	case CS40L26_IRQ1_STATUS:
> > > +	case CS40L26_IRQ1_EINT_1 ... CS40L26_IRQ1_EINT_5:
> > > +	case CS40L26_IRQ1_STS_1 ... CS40L26_IRQ1_STS_5:
> > > +	case CS40L26_IRQ1_MASK_1 ... CS40L26_IRQ1_MASK_5:
> > > +	case CS40L26_MIXER_NGATE_CH1_CFG:
> > > +	case CS40L26_DSP_MBOX_1 ... CS40L26_DSP_VIRTUAL2_MBOX_8:
> > > +	case CS40L26_OTP_MEM0 ... CS40L26_OTP_MEM31:
> > > +	case CS40L26_DSP1_XMEM_PACKED_0 ... CS40L26_DSP1_XMEM_PACKED_6143:
> > > +	case CS40L26_DSP1_XROM_PACKED_0 ... CS40L26_DSP1_XROM_PACKED_4604:
> > > +	case CS40L26_DSP1_XMEM_UNPACKED32_0 ... CS40L26_DSP1_XROM_UNPACKED32_3070:
> > > +	case CS40L26_DSP1_SYS_INFO_ID:
> > > +	case CS40L26_DSP1_XMEM_UNPACKED24_0 ... CS40L26_DSP1_XMEM_UNPACKED24_8191:
> > > +	case CS40L26_DSP1_XROM_UNPACKED24_0 ... CS40L26_DSP1_XROM_UNPACKED24_6141:
> > > +	case CS40L26_DSP1_CCM_CORE_CONTROL:
> > > +	case CS40L26_DSP1_YMEM_PACKED_0 ... CS40L26_DSP1_YMEM_PACKED_1532:
> > > +	case CS40L26_DSP1_YMEM_UNPACKED32_0 ... CS40L26_DSP1_YMEM_UNPACKED32_1022:
> > > +	case CS40L26_DSP1_YMEM_UNPACKED24_0 ... CS40L26_DSP1_YMEM_UNPACKED24_2045:
> > > +	case CS40L26_DSP1_PMEM_0 ... CS40L26_DSP1_PMEM_5114:
> > > +	case CS40L26_DSP1_PROM_0 ... CS40L26_DSP1_PROM_30714:
> > > +		return true;
> > > +	default:
> > > +		return false;
> > > +	}
> > 
> > Is this function necessary? Are there cases throughout the driver that may
> > attempt to read an illegal register, and you depend on the regmap call to
> > fail? If not, I think you can just drop this function.
> > 
> 
> I would much rather we kept this one, at the very least such
> that the debugfs representation of the regmap shows the correct
> registers.

Ah, that's a good point; with that in mind, I agree.

> 
> > > +static inline void cs40l26_pm_runtime_setup(struct cs40l26_private *cs40l26)
> > > +{
> > > +	pm_runtime_mark_last_busy(cs40l26->dev);
> > > +	pm_runtime_use_autosuspend(cs40l26->dev);
> > > +	pm_runtime_set_autosuspend_delay(cs40l26->dev, CS40L26_AUTOSUSPEND_DELAY_MS);
> > > +	pm_runtime_enable(cs40l26->dev);
> > 
> > My first impression was that this driver is doing an uncommonly large
> > amount of PM-related housekeeping. In fact, not a single haptic driver
> > in mainline is calling any of these. What is unique about this device
> > that calls for this much overhead?
> > 
> > Can the device wake up from hibernation (AoH?) from both control port
> > and GPIO triggers? If so, why not to simply allow the device to hibernate
> > at its own discretion and avoid all PM-related housekeeping? Stated
> > another way, it seems some of the DSP's job is unnecessarily duplicated
> > in the kernel.
> > 
> > In case I have misunderstood, please let me know.
> > 
> 
> 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.

In my mind that is simpler, reduces the number of instances that
keep track of the device's state from two (kernel and DSP FW) to
one (DSP FW), and removes the onslaught of PM-related housekeeping
that doesn't seem to be required by other input/misc devices.

Does the current implementation at least allow the device to hibernate
while the system is otherwise active, as opposed to _only_ during
runtime suspend? If so, that's still a marked improvement from L25
era where customers rightfully pointed out that the downstream driver
was not making efficient use of hibernation. ;)

I don't feel particularly strongly about it, so if the current
implementation will stay, perhaps consider a few comments in this
area to describe how the device's state is managed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/iqs5xx.c#n158

> 
> > > +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.
> 
> > > +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.
> 
> > > +	cs40l26->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > +	if (IS_ERR_OR_NULL(cs40l26->reset_gpio)) {
> > > +		dev_err(dev, "Failed to get reset GPIO: %d\n", (int) PTR_ERR(cs40l26->reset_gpio));
> > > +		ret = -ENOENT;
> > > +		goto err;
> > > +	}
> > > +
> > > +	usleep_range(CS40L26_MIN_RESET_PULSE_WIDTH_US, CS40L26_MIN_RESET_PULSE_WIDTH_US + 100);
> > > +
> > > +	gpiod_set_value_cansleep(cs40l26->reset_gpio, 1);
> > 
> > This is backwards, which means your dts must be backwards as well. gpiod is
> > a logical API, so you should actually declare the GPIO as GPIOD_OUT_HIGH, i.e.
> > asserted, and then call gpiod_set_value_cansleep() with 0, i.e. de-asserted.
> > The definition of "asserted" then comes from the polarity defined in dts.
> > 
> > By the way, did you test this driver without a reset GPIO defined in dts? It's
> > an optional GPIO, rightfully so, but you need to check whether reset_gpio is
> > NULL prior to acting upon it.
> > 
> 
> gpiod does the NULL check for you no need to duplicate it in the
> driver.

Great catch; you're right.

> 
> Thanks,
> Charles

Kind regards,
Jeff LaBundy

Powered by blists - more mailing lists