Message-ID: <20230410085634.GV68926@ediswmail.ad.cirrus.com>
Date:   Mon, 10 Apr 2023 08:56:34 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Jeff LaBundy <jeff@...undy.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

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.

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

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

Thanks,
Charles

Powered by blists - more mailing lists