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

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

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.

What I don't like too much is that there's no way to write correct DTS
without fixing these drivers somehow. :-(

-- 
Kind regards,

Sakari Ailus