Re: [PATCH] media: i2c: Add note to prevent buggy code re-use

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Tue, Dec 30, 2025 at 10:35:28AM +0100, Krzysztof Kozlowski wrote:
> On 30/12/2025 10:05, Sakari Ailus wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the patch.
> > 
> > On Tue, Dec 30, 2025 at 09:34:36AM +0100, Krzysztof Kozlowski wrote:
> >> adv7604 and et8ek8 sensor drivers have mixed up logical and line level
> >> for reset/powerdown signal.  They call it a reset signal (it indeed
> >> behaves like that), but drivers assert the reset to operate which is
> >> clearly incorrect and relies on wrong ACTIVE_HIGH flag in the DTS.
> >>
> >> People in discussions copy existing poor code and claim they can repeat
> >> same mistake, so add a note to prevent that.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> >>
> >> ---
> >>
> >> Similar to my commit 9d108d226224 ("media: i2c: imx: Add note to prevent
> >> buggy code re-use"). I went through rest of i2c drivers and found only
> >> these two doing it incorrectly.
> >> ---
> >>  drivers/media/i2c/adv7604.c              | 8 +++++++-
> >>  drivers/media/i2c/et8ek8/et8ek8_driver.c | 4 ++++
> >>  2 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> >> index 516553fb17e9..67116a4ef134 100644
> >> --- a/drivers/media/i2c/adv7604.c
> >> +++ b/drivers/media/i2c/adv7604.c
> >> @@ -3453,7 +3453,13 @@ static int configure_regmaps(struct adv76xx_state *state)
> >>  static void adv76xx_reset(struct adv76xx_state *state)
> >>  {
> >>  	if (state->reset_gpio) {
> >> -		/* ADV76XX can be reset by a low reset pulse of minimum 5 ms. */
> >> +		/*
> >> +		 * Note: Misinterpretation of reset assertion - do not re-use
> >> +		 * this code.  The reset pin is using incorrect (for a reset
> >> +		 * signal) logical level.
> >> +		 *
> >> +		 * ADV76XX can be reset by a low reset pulse of minimum 5 ms.
> >> +		 */
> >>  		gpiod_set_value_cansleep(state->reset_gpio, 0);
> >>  		usleep_range(5000, 10000);
> >>  		gpiod_set_value_cansleep(state->reset_gpio, 1);
> >> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> >> index 2cb7b718782b..50121c3e5b48 100644
> >> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> >> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> >> @@ -835,6 +835,10 @@ static int et8ek8_power_on(struct et8ek8_sensor *sensor)
> >>  
> >>  	udelay(10); /* I wish this is a good value */
> >>  
> >> +	/*
> >> +	 * Note: Misinterpretation of reset assertion - do not re-use this code.
> >> +	 * The reset pin is using incorrect (for a reset signal) logical level.
> >> +	 */
> >>  	gpiod_set_value(sensor->reset, 1);
> >>  
> >>  	msleep(5000 * 1000 / sensor->xclk_freq + 1); /* Wait 5000 cycles */
> > 
> > Related to the topic, would you be able to comment on the discussion
> > related to <20251114133822.434171-2-loic.poulain@oss.qualcomm.com>? I
> > believe you're cc'd, with your @kernel.org address.
> 
> I don't have it in my inbox (it's limited to ~10k of messages, so the
> roll out of inbox after 1 month usually).

I just bounced the thread to you...

-- 
Sakari Ailus