* [PATCH] media: i2c: Add note to prevent buggy code re-use
@ 2025-12-30 8:34 Krzysztof Kozlowski
2025-12-30 9:05 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Kozlowski @ 2025-12-30 8:34 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Pavel Machek, Sakari Ailus,
linux-media, linux-kernel
Cc: Krzysztof Kozlowski
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 */
--
2.51.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] media: i2c: Add note to prevent buggy code re-use
2025-12-30 8:34 [PATCH] media: i2c: Add note to prevent buggy code re-use Krzysztof Kozlowski
@ 2025-12-30 9:05 ` Sakari Ailus
2025-12-30 9:35 ` Krzysztof Kozlowski
0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2025-12-30 9:05 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hans Verkuil, Mauro Carvalho Chehab, Pavel Machek, linux-media,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] media: i2c: Add note to prevent buggy code re-use
2025-12-30 9:05 ` Sakari Ailus
@ 2025-12-30 9:35 ` Krzysztof Kozlowski
2025-12-30 13:00 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Kozlowski @ 2025-12-30 9:35 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, Mauro Carvalho Chehab, Pavel Machek, linux-media,
linux-kernel
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).
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] media: i2c: Add note to prevent buggy code re-use
2025-12-30 9:35 ` Krzysztof Kozlowski
@ 2025-12-30 13:00 ` Sakari Ailus
0 siblings, 0 replies; 4+ messages in thread
From: Sakari Ailus @ 2025-12-30 13:00 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hans Verkuil, Mauro Carvalho Chehab, Pavel Machek, linux-media,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-12-30 13:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-30 8:34 [PATCH] media: i2c: Add note to prevent buggy code re-use Krzysztof Kozlowski
2025-12-30 9:05 ` Sakari Ailus
2025-12-30 9:35 ` Krzysztof Kozlowski
2025-12-30 13:00 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.