devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Loic Poulain <loic.poulain@oss.qualcomm.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	krzk+dt@kernel.org, andersson@kernel.org, konradybcio@kernel.org,
	dave.stevenson@raspberrypi.com, robh@kernel.org,
	conor+dt@kernel.org, mchehab@kernel.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v3 1/4] media: i2c: ov9282: Fix reset-gpio logical state
Date: Tue, 30 Dec 2025 17:40:48 +0200	[thread overview]
Message-ID: <20251230154048.GA15048@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAFEp6-26NiAcoP-nTaFZrG6AT3QimZsNLfPU07Fj2TwqimBbRg@mail.gmail.com>

On Tue, Dec 30, 2025 at 04:03:58PM +0100, Loic Poulain wrote:
> On Tue, Dec 30, 2025 at 2:54 PM Krzysztof Kozlowski wrote:
> > On 15/12/2025 11:19, Loic Poulain wrote:
> > > On Mon, Dec 15, 2025 at 10:40 AM Sakari Ailus wrote:
> > >> On Mon, Dec 15, 2025 at 10:35:15AM +0100, Loic Poulain wrote:
> > >>> On Mon, Nov 17, 2025 at 6:30 PM Sakari Ailus wrote:
> > >>>> On Fri, Nov 14, 2025 at 02:38:19PM +0100, Loic Poulain wrote:
> > >>>>> Ensure reset state is low in the power-on state and high in the
> > >>>>> power-off state (assert reset). Note that the polarity is abstracted
> > >>>>> by the GPIO subsystem, so the logic level reflects the intended reset
> > >>>>> behavior.
> > >>>>
> > >>>> That's an interesting approach to fix DTS gone systematically wrong.
> > >>>>
> > >>>> I was thinking of the drivers that have this issue, too, but I would have
> > >>>> introduced a new GPIO under a different name (many sensors use "enable",
> > >>>> too). Any thoughts?
> > >>>
> > >>> Apologies for missing your point earlier. We can’t really name it
> > >>> enable, as it performs the opposite function and that would be
> > >>> confusing in the device tree description. A property like reset2 would
> > >>> be more accurate, but I suspect such a binding wouldn’t be acceptable
> > >>> from a device tree/bindings perspective.
> > >>
> > >> Many sensor datasheets document a pin called "xshutdown" or alike. That's
> > >> not exactly "reset" or "enable" but it can be mapped to either and this can
> > >> be seen in the existing bindings. The polarity is effectively the opposite,
> > >> yes, but does that matter?
> > >
> > > I assume naming a pin 'xshutdown' or 'xreset' indicates that its
> > > polarity is inverted at the driver level, the driver interprets the
> > > shutdown or reset function as being active when the logical level is 0
> > > (low), as they actually incorrectly do for the 'reset' gpio.
> > >
> > > From the driver’s perspective, this naming convention is acceptable;
> > > however, it causes the devicetree description to slightly diverge from
> > > the datasheet and leaves the reset property effectively inverted (and
> > > therefore incorrect).
> > >
> > > Honestly, in this specific case, the simplest solution would be to fix
> > > the driver, since there is currently no upstream devicetree using this
> > > sensor. That would technically break backward compatibility for any
> > > out-of-tree DTS (if they exist), but those would have been incorrect
> > > in the first place.
> > >
> > > But yes, this seems like a good opportunity to discuss and define a
> > > more general approach that can be applied to other drivers with
> > > similar polarity or naming issues.
> > >
> > > Krzysztof, any thoughts?
> >
> > You need to first CC me. You sent it to the special bulk email
> > address... Anyway, please be specific about the question.
> 
> Ultimately, I’d like to reach a consensus before moving forward with
> V4, as several approaches have been discussed so far:
> 
> 1. Keep the current (incorrect) driver logic: This was the approach I
> used in V1 of this series, explicitly noting in the DTS that the
> polarity was incorrect. However, this workaround was fairly rejected
> as not being an acceptable solution.
> 
> 2. Fix the driver logic: This was the approach in V2. It ensures
> correct behavior going forward, especially since there is currently no
> upstream DTB using this binding yet. The downside is that it would
> consistently break any out-of-tree DTBs that *incorrectly* describe
> the GPIO polarity.
> 
> 3. Follow the wsa881x approach: this is V3, aiming for best-effort
> backward compatibility. That said, it’s true that this approach does
> not handle all cases.
> 
> There have also been discussions about introducing an additional
> property for the same pin, with polarity described correctly... From a
> DTS perspective, I believe this would likely be rejected.
> 
> Based on Laurent’s reply, he seems more inclined toward solutions 1
> and 2. Would either of these approaches be acceptable from a DTS
> standpoint?

Do you know of DTs in the wild that use the ov9282 reset-gpios ? Based
on the git history, I see the driver was initially upstreamed by Intel,
and there's been lots of activity on the driver from Dave Stevenson from
Raspberry Pi.

Raspberry Pi modules don't wire the reset pin to a GPIO (the GPIO on the
connector controls the on-module regulators), so there should be no
regression if we changed the driver behaviour.

As the driver was upstreamed by Intel, I assume it may be used on
ACPI-based systems. Sakari, do you know what those machines are, and if
they expose the reset GPIO through ACPI ?

> > I responded to earlier message that your claims in your comment in this
> > patch are clearly wrong, but what it is surprising me, it's second
> > approach this month people completely ignore existing and new DTS. Other
> > was MT7530 where author also claim all is fine, but actually both old
> > and new DTS were broken. Same here.
> 
> Yes, the comment is oversimplified, which makes it incorrect in
> certain cases. I’ll ensure the comment is accurate in the next version
> if we decide to stick with this solution.

-- 
Regards,

Laurent Pinchart

  reply	other threads:[~2025-12-30 15:41 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-14 13:38 [PATCH v3 0/4] Add QRB2210 RB1 vision mezzanine/kit support Loic Poulain
2025-11-14 13:38 ` [PATCH v3 1/4] media: i2c: ov9282: Fix reset-gpio logical state Loic Poulain
2025-11-17 17:30   ` Sakari Ailus
2025-12-03 10:00     ` Loic Poulain
2025-12-30 13:49       ` Krzysztof Kozlowski
2025-12-30 13:56       ` Krzysztof Kozlowski
2025-12-15  9:35     ` Loic Poulain
2025-12-15  9:40       ` Sakari Ailus
2025-12-15 10:19         ` Loic Poulain
2025-12-30 13:33           ` Laurent Pinchart
2025-12-30 13:54           ` Krzysztof Kozlowski
2025-12-30 15:03             ` Loic Poulain
2025-12-30 15:40               ` Laurent Pinchart [this message]
2025-12-30 15:41               ` Krzysztof Kozlowski
2026-01-06 19:53                 ` Loic Poulain
2025-11-14 13:38 ` [PATCH v3 2/4] arm64: dts: qcom: qcm2290: Add pin configuration for mclks Loic Poulain
2025-11-17 13:07   ` Konrad Dybcio
2025-11-14 13:38 ` [PATCH v3 3/4] arm64: dts: qcom: qrb2210-rb1: Add PM8008 node Loic Poulain
2025-11-14 13:38 ` [PATCH v3 4/4] arm64: dts: qcom: qrb2210-rb1: Add overlay for vision mezzanine Loic Poulain
2025-11-18  4:25   ` Dmitry Baryshkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20251230154048.GA15048@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=andersson@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=devicetree@vger.kernel.org \
    --cc=konradybcio@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=krzk@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=loic.poulain@oss.qualcomm.com \
    --cc=mchehab@kernel.org \
    --cc=robh@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).