Re: [PATCH 5/8] Input: cap11xx - add reset gpio support

[Jun Yan <jerrysteve1101@gmail.com>]

> Thank you for your contribution! Sashiko AI review found 2 potential issue(=
> s) to consider:
> - [Low] Inefficient delay using usleep_range() for a large duration (400-50=
> 0ms).
> - [Low] Missing explicit inclusion of <linux/delay.h>.
> --
> 
> commit 83de47341df3454fdb8216dc0a95948c3f158a40
> Author: Jun Yan <jerrysteve1101@gmail.com>
> 
> Input: cap11xx - add reset gpio support
> 
> This commit adds hardware reset support for CAP11xx devices by toggling a
> dedicated reset GPIO during the i2c probe phase. It incorporates specific
> timing requirements for the reset filter and active reset duration.
> 
> > diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/ca=
> p11xx.c
> > index 686174722204c..7015aa7654ce7 100644
> > --- a/drivers/input/keyboard/cap11xx.c
> > +++ b/drivers/input/keyboard/cap11xx.c
> 
> [ ... ]
> 
> > @@ -452,6 +457,17 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_=
> client)
> >  	if (IS_ERR(priv->regmap))
> >  		return PTR_ERR(priv->regmap);
> > =20
> > +	priv->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LO=
> W);
> > +	if (IS_ERR(priv->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
> > +							"Failed to get 'reset' GPIO\n");
> > +	if (priv->reset_gpio) {
> > +		gpiod_set_value_cansleep(priv->reset_gpio, 1);
> > +		usleep_range(CAP11XX_T_RST_FILT_MIN, CAP11XX_T_RST_FILT_MIN * 2);
> > +		gpiod_set_value_cansleep(priv->reset_gpio, 0);
> > +		usleep_range(CAP11XX_T_RST_ON_MIN, CAP11XX_T_RST_ON_MAX);
> 
> [Severity: Low]
> Does this regression introduce unnecessary overhead by using usleep_range()
> for a 400-500ms delay in cap11xx_i2c_probe()?
> 
> According to kernel timer guidelines, usleep_range() is designed for short
> delays (10us to 20ms) and sets up high-resolution timers. For delays longer
> than 20ms, using msleep() or fsleep() avoids unnecessary overhead and
> expensive CPU wakeups.
> 

Agreed. Will replace usleep_range() with msleep() in V2.

>
> [Severity: Low]
> Could this regression in header dependencies be avoided by explicitly
> including <linux/delay.h> since we are now calling usleep_range() here?
> 
> While the code might compile due to implicit inclusion via other headers,
> relying on implicit includes can sometimes lead to build failures on other
> architectures or if intermediate headers are refactored in the future.
> 

OK. Will add #include <linux/delay.h> in v2,

> > +	}
> > +
> >  	error =3D regmap_read(priv->regmap, CAP11XX_REG_PRODUCT_ID, &val);
> >  	if (error)
> >  		return dev_err_probe(dev, error, "Failed to read product ID\n");
> 
> --=20
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260606150458.2506=
> 06-1-jerrysteve1101@gmail.com?part=3D5