diff mbox series

[RFC,1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom

Message ID 20210316140707.RFC.1.I3a21995726282f1e9fcb70da5eb96f19ed96634f@changeid
State New
Headers show
Series [RFC,1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom | expand

Commit Message

Doug Anderson March 16, 2021, 9:08 p.m. UTC
The sc7180-trogdor-pompom board might be attached to any number of a
pile of eDP panels. At the moment I'm told that the list might include:
- KD KD116N21-30NV-A010
- KD KD116N09-30NH-A016
- Starry 2081116HHD028001-51D
- Sharp LQ116M1JW10

It should be noted that while the EDID programmed in the first 3
panels indicates that they should run with exactly the same timing (to
keep things simple), the 4th panel not only needs different timing but
has a different resolution.

As is true in general with eDP panels, we can figure out which panel
we have and all the info needed to drive its pixel clock by reading
the EDID. However, we can do this only after we've powered the panel
on. Powering on the panels requires following the timing diagram in
each panel's datasheet which specifies delays between certain
actions. This means that, while we can be quite dynamic about handling
things we can't just totally skip out on describing the panel like we
could do if it was connected to an external-facing DP port.

While the different panels have slightly different delays, it's
possible to come up with a set of unified delays that will work on all
the panels. From reading the datasheets:
* KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
  - HPD absent delay: 200 ms
  - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
* Starry 2081116HHD028001-51D
  - HPD absent delay: 100 ms
  - Enable delay: (link training done till enable BL): 200 ms
  - Unprepare delay: 500 ms
* Sharp LQ116M1JW10
  - HPD absent delay: 200 ms
  - Unprepare delay: 500 ms
  - Prepare to enable delay (power on till backlight): 100 ms

Unified:
- HPD absent delay: 200 ms
- Unprepare delay: 500 ms
- Enable delay: 200 ms

NOTE: in theory the only thing that we _really_ need unity on is the
"HPD absent delay" since once the panel asserts HPD we can read the
EDID and could make per-panel decisions if we wanted.

Let's create a definition of "a panel that can be attached to pompom"
as a panel that provides a valid EDID and can work with the standard
pompom power sequencing. If more panels are later attached to pompom
then it's fine as long as they work in a compatible way.

One might ask why we can't just use a generic string here and provide
the timings directly in the device tree file. As I understand it,
trying to describe generic power sequencing in the device tree is
frowned upon and the one instance (SD/MMC) is regarded as a mistake
that shouldn't be repeated. Specifying a power sequence per board (or
per board class) feels like a reasonable compromise. We're not trying
to define fully generic power sequence bindings but we can also take
advantage of the semi-probable properties of the attached device.

NOTE: I believe that past instances of supporting this type of thing
have used the "white lie" approach. One representative panel was
listed in the device tree. The power sequencings of this
representative panel were OK to use across all panels that might be
attached and other differences were handled by EDID. This patch
attempts to set a new precedent and avoid the need for the white lie.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../devicetree/bindings/display/panel/panel-simple.yaml       | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rob Clark March 18, 2021, 1:53 a.m. UTC | #1
On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > The sc7180-trogdor-pompom board might be attached to any number of a
> > pile of eDP panels. At the moment I'm told that the list might include:
> > - KD KD116N21-30NV-A010
> > - KD KD116N09-30NH-A016
> > - Starry 2081116HHD028001-51D
> > - Sharp LQ116M1JW10
> >
> > It should be noted that while the EDID programmed in the first 3
> > panels indicates that they should run with exactly the same timing (to
> > keep things simple), the 4th panel not only needs different timing but
> > has a different resolution.
> >
> > As is true in general with eDP panels, we can figure out which panel
> > we have and all the info needed to drive its pixel clock by reading
> > the EDID. However, we can do this only after we've powered the panel
> > on. Powering on the panels requires following the timing diagram in
> > each panel's datasheet which specifies delays between certain
> > actions. This means that, while we can be quite dynamic about handling
> > things we can't just totally skip out on describing the panel like we
> > could do if it was connected to an external-facing DP port.
> >
> > While the different panels have slightly different delays, it's
> > possible to come up with a set of unified delays that will work on all
> > the panels. From reading the datasheets:
> > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> >   - HPD absent delay: 200 ms
> >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > * Starry 2081116HHD028001-51D
> >   - HPD absent delay: 100 ms
> >   - Enable delay: (link training done till enable BL): 200 ms
> >   - Unprepare delay: 500 ms
> > * Sharp LQ116M1JW10
> >   - HPD absent delay: 200 ms
> >   - Unprepare delay: 500 ms
> >   - Prepare to enable delay (power on till backlight): 100 ms
> >
> > Unified:
> > - HPD absent delay: 200 ms
> > - Unprepare delay: 500 ms
> > - Enable delay: 200 ms
> >
> > NOTE: in theory the only thing that we _really_ need unity on is the
> > "HPD absent delay" since once the panel asserts HPD we can read the
> > EDID and could make per-panel decisions if we wanted.
> >
> > Let's create a definition of "a panel that can be attached to pompom"
> > as a panel that provides a valid EDID and can work with the standard
> > pompom power sequencing. If more panels are later attached to pompom
> > then it's fine as long as they work in a compatible way.
> >
> > One might ask why we can't just use a generic string here and provide
> > the timings directly in the device tree file. As I understand it,
> > trying to describe generic power sequencing in the device tree is
> > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > that shouldn't be repeated. Specifying a power sequence per board (or
> > per board class) feels like a reasonable compromise. We're not trying
> > to define fully generic power sequence bindings but we can also take
> > advantage of the semi-probable properties of the attached device.
> >
> > NOTE: I believe that past instances of supporting this type of thing
> > have used the "white lie" approach. One representative panel was
> > listed in the device tree. The power sequencings of this
> > representative panel were OK to use across all panels that might be
> > attached and other differences were handled by EDID. This patch
> > attempts to set a new precedent and avoid the need for the white lie.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
>
> Sounds reasonable to me if DT maintainers can live with this abstract
> hardware definition. It's clearer than the 'white lie' approach.

Yeah, it is a weird grey area between "discoverable" and "not
discoverable".. but I favor DT reflecting reality as much as
possible/feasible, so I think this is definity cleaner than "white
lies"

Reviewed-by: Rob Clark <robdclark@gmail.com>

> It's then up to the vendor/manufacturer to ensure to only ship devices
> with panels that have compatible timings.
>
> >  .../devicetree/bindings/display/panel/panel-simple.yaml       | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> > index 62b0d54d87b7..9807dbc1cceb 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> > @@ -140,6 +140,10 @@ properties:
> >        - giantplus,gpg48273qs5
> >          # GiantPlus GPM940B0 3.0" QVGA TFT LCD panel
> >        - giantplus,gpm940b0
> > +        # A panel connected to a google,pompom board. Panel is guaranteed to
> > +        # confirm to google,pompom-panel power sequencing requirements and then
>
> s/confirm/conform/ ?
>
> > +        # the specific panel will be probed via EDID.
> > +      - google,pompom-panel
> >          # HannStar Display Corp. HSD070PWW1 7.0" WXGA TFT LCD panel
> >        - hannstar,hsd070pww1
> >          # HannStar Display Corp. HSD100PXN1 10.1" XGA LVDS panel
>
> FWIW:
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thierry Reding March 26, 2021, 12:39 p.m. UTC | #2
On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > pile of eDP panels. At the moment I'm told that the list might include:
> > > - KD KD116N21-30NV-A010
> > > - KD KD116N09-30NH-A016
> > > - Starry 2081116HHD028001-51D
> > > - Sharp LQ116M1JW10
> > >
> > > It should be noted that while the EDID programmed in the first 3
> > > panels indicates that they should run with exactly the same timing (to
> > > keep things simple), the 4th panel not only needs different timing but
> > > has a different resolution.
> > >
> > > As is true in general with eDP panels, we can figure out which panel
> > > we have and all the info needed to drive its pixel clock by reading
> > > the EDID. However, we can do this only after we've powered the panel
> > > on. Powering on the panels requires following the timing diagram in
> > > each panel's datasheet which specifies delays between certain
> > > actions. This means that, while we can be quite dynamic about handling
> > > things we can't just totally skip out on describing the panel like we
> > > could do if it was connected to an external-facing DP port.
> > >
> > > While the different panels have slightly different delays, it's
> > > possible to come up with a set of unified delays that will work on all
> > > the panels. From reading the datasheets:
> > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > >   - HPD absent delay: 200 ms
> > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > * Starry 2081116HHD028001-51D
> > >   - HPD absent delay: 100 ms
> > >   - Enable delay: (link training done till enable BL): 200 ms
> > >   - Unprepare delay: 500 ms
> > > * Sharp LQ116M1JW10
> > >   - HPD absent delay: 200 ms
> > >   - Unprepare delay: 500 ms
> > >   - Prepare to enable delay (power on till backlight): 100 ms
> > >
> > > Unified:
> > > - HPD absent delay: 200 ms
> > > - Unprepare delay: 500 ms
> > > - Enable delay: 200 ms
> > >
> > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > EDID and could make per-panel decisions if we wanted.
> > >
> > > Let's create a definition of "a panel that can be attached to pompom"
> > > as a panel that provides a valid EDID and can work with the standard
> > > pompom power sequencing. If more panels are later attached to pompom
> > > then it's fine as long as they work in a compatible way.
> > >
> > > One might ask why we can't just use a generic string here and provide
> > > the timings directly in the device tree file. As I understand it,
> > > trying to describe generic power sequencing in the device tree is
> > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > per board class) feels like a reasonable compromise. We're not trying
> > > to define fully generic power sequence bindings but we can also take
> > > advantage of the semi-probable properties of the attached device.
> > >
> > > NOTE: I believe that past instances of supporting this type of thing
> > > have used the "white lie" approach. One representative panel was
> > > listed in the device tree. The power sequencings of this
> > > representative panel were OK to use across all panels that might be
> > > attached and other differences were handled by EDID. This patch
> > > attempts to set a new precedent and avoid the need for the white lie.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> >
> > Sounds reasonable to me if DT maintainers can live with this abstract
> > hardware definition. It's clearer than the 'white lie' approach.
> 
> Yeah, it is a weird grey area between "discoverable" and "not
> discoverable".. but I favor DT reflecting reality as much as
> possible/feasible, so I think this is definity cleaner than "white
> lies"

This is practically no different than the "white lie". I suppose you
could perhaps call it "more honest", if you want.

The point remains that unless we describe exactly which panel we're
dealing with, we ultimately have no way of properly quirking anything if
we ever have to. Also, once we allow this kind of wildcard we can
suddenly get into a situation where people might want to reuse this on
something that's not at all a google-pompom board because the same
particular power sequence happens to work on on some other board.

Similarly I can imagine a situation where we could now have the same
panel supported by multiple different wildcard compatible strings. How
is that supposed to be any cleaner than what we have now?

And I still keep wondering why bootloaders can't be taught about these
kinds of things. We have in the past discussed various solutions where
the bootloader could detect the type of panel connected and set the
proper compatible string.

If that's too complicated and these really are standardized interfaces
that work across a wide range of devices with perhaps a couple of
standard parameter, then introducing a standard connector type like
Rob Herring is suggesting makes more sense because that more properly
describes where exactly the standardization is going on (i.e. at the
interface level rather than the panel level).

Thierry
Rob Clark March 26, 2021, 3:24 p.m. UTC | #3
On Fri, Mar 26, 2021 at 8:18 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > > > pile of eDP panels. At the moment I'm told that the list might include:
> > > > > - KD KD116N21-30NV-A010
> > > > > - KD KD116N09-30NH-A016
> > > > > - Starry 2081116HHD028001-51D
> > > > > - Sharp LQ116M1JW10
> > > > >
> > > > > It should be noted that while the EDID programmed in the first 3
> > > > > panels indicates that they should run with exactly the same timing (to
> > > > > keep things simple), the 4th panel not only needs different timing but
> > > > > has a different resolution.
> > > > >
> > > > > As is true in general with eDP panels, we can figure out which panel
> > > > > we have and all the info needed to drive its pixel clock by reading
> > > > > the EDID. However, we can do this only after we've powered the panel
> > > > > on. Powering on the panels requires following the timing diagram in
> > > > > each panel's datasheet which specifies delays between certain
> > > > > actions. This means that, while we can be quite dynamic about handling
> > > > > things we can't just totally skip out on describing the panel like we
> > > > > could do if it was connected to an external-facing DP port.
> > > > >
> > > > > While the different panels have slightly different delays, it's
> > > > > possible to come up with a set of unified delays that will work on all
> > > > > the panels. From reading the datasheets:
> > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > >   - HPD absent delay: 200 ms
> > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > > > * Starry 2081116HHD028001-51D
> > > > >   - HPD absent delay: 100 ms
> > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > >   - Unprepare delay: 500 ms
> > > > > * Sharp LQ116M1JW10
> > > > >   - HPD absent delay: 200 ms
> > > > >   - Unprepare delay: 500 ms
> > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > >
> > > > > Unified:
> > > > > - HPD absent delay: 200 ms
> > > > > - Unprepare delay: 500 ms
> > > > > - Enable delay: 200 ms
> > > > >
> > > > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > > > EDID and could make per-panel decisions if we wanted.
> > > > >
> > > > > Let's create a definition of "a panel that can be attached to pompom"
> > > > > as a panel that provides a valid EDID and can work with the standard
> > > > > pompom power sequencing. If more panels are later attached to pompom
> > > > > then it's fine as long as they work in a compatible way.
> > > > >
> > > > > One might ask why we can't just use a generic string here and provide
> > > > > the timings directly in the device tree file. As I understand it,
> > > > > trying to describe generic power sequencing in the device tree is
> > > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > > > per board class) feels like a reasonable compromise. We're not trying
> > > > > to define fully generic power sequence bindings but we can also take
> > > > > advantage of the semi-probable properties of the attached device.
> > > > >
> > > > > NOTE: I believe that past instances of supporting this type of thing
> > > > > have used the "white lie" approach. One representative panel was
> > > > > listed in the device tree. The power sequencings of this
> > > > > representative panel were OK to use across all panels that might be
> > > > > attached and other differences were handled by EDID. This patch
> > > > > attempts to set a new precedent and avoid the need for the white lie.
> > > > >
> > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > ---
> > > >
> > > > Sounds reasonable to me if DT maintainers can live with this abstract
> > > > hardware definition. It's clearer than the 'white lie' approach.
> > >
> > > Yeah, it is a weird grey area between "discoverable" and "not
> > > discoverable".. but I favor DT reflecting reality as much as
> > > possible/feasible, so I think this is definity cleaner than "white
> > > lies"
> >
> > This is practically no different than the "white lie". I suppose you
> > could perhaps call it "more honest", if you want.
> >
> > The point remains that unless we describe exactly which panel we're
> > dealing with, we ultimately have no way of properly quirking anything if
> > we ever have to. Also, once we allow this kind of wildcard we can
> > suddenly get into a situation where people might want to reuse this on
> > something that's not at all a google-pompom board because the same
> > particular power sequence happens to work on on some other board.
> >
> > Similarly I can imagine a situation where we could now have the same
> > panel supported by multiple different wildcard compatible strings. How
> > is that supposed to be any cleaner than what we have now?
> >
> > And I still keep wondering why bootloaders can't be taught about these
> > kinds of things. We have in the past discussed various solutions where
> > the bootloader could detect the type of panel connected and set the
> > proper compatible string.
>
> The bootloader cannot detect the panel without powering up the panel,
> which it normally does not do if you are not in dev-mode (it would add
> a significant amount of time to bootup, which is why we can't do this)

what if we had a sort of multi-choice panel node:

   panel: panel {
     compatible = "panel,one-of";
     compatible-one-of = "vendor1,panel-a", "vendor2,panel-b",
"vendor3,panel-c";
  };

The kernel could construct power sequence timings that are the
superset of all the possible panels.  That seems about as explicit as
we could get in this sort of case.

BR,
-R


> BR,
> -R
>
> > If that's too complicated and these really are standardized interfaces
> > that work across a wide range of devices with perhaps a couple of
> > standard parameter, then introducing a standard connector type like
> > Rob Herring is suggesting makes more sense because that more properly
> > describes where exactly the standardization is going on (i.e. at the
> > interface level rather than the panel level).
> >
> > Thierry
Bjorn Andersson March 26, 2021, 5 p.m. UTC | #4
On Fri 26 Mar 10:24 CDT 2021, Rob Clark wrote:

> On Fri, Mar 26, 2021 at 8:18 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > >
> > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > > > > pile of eDP panels. At the moment I'm told that the list might include:
> > > > > > - KD KD116N21-30NV-A010
> > > > > > - KD KD116N09-30NH-A016
> > > > > > - Starry 2081116HHD028001-51D
> > > > > > - Sharp LQ116M1JW10
> > > > > >
> > > > > > It should be noted that while the EDID programmed in the first 3
> > > > > > panels indicates that they should run with exactly the same timing (to
> > > > > > keep things simple), the 4th panel not only needs different timing but
> > > > > > has a different resolution.
> > > > > >
> > > > > > As is true in general with eDP panels, we can figure out which panel
> > > > > > we have and all the info needed to drive its pixel clock by reading
> > > > > > the EDID. However, we can do this only after we've powered the panel
> > > > > > on. Powering on the panels requires following the timing diagram in
> > > > > > each panel's datasheet which specifies delays between certain
> > > > > > actions. This means that, while we can be quite dynamic about handling
> > > > > > things we can't just totally skip out on describing the panel like we
> > > > > > could do if it was connected to an external-facing DP port.
> > > > > >
> > > > > > While the different panels have slightly different delays, it's
> > > > > > possible to come up with a set of unified delays that will work on all
> > > > > > the panels. From reading the datasheets:
> > > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > > >   - HPD absent delay: 200 ms
> > > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > > > > * Starry 2081116HHD028001-51D
> > > > > >   - HPD absent delay: 100 ms
> > > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > > >   - Unprepare delay: 500 ms
> > > > > > * Sharp LQ116M1JW10
> > > > > >   - HPD absent delay: 200 ms
> > > > > >   - Unprepare delay: 500 ms
> > > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > > >
> > > > > > Unified:
> > > > > > - HPD absent delay: 200 ms
> > > > > > - Unprepare delay: 500 ms
> > > > > > - Enable delay: 200 ms
> > > > > >
> > > > > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > > > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > > > > EDID and could make per-panel decisions if we wanted.
> > > > > >
> > > > > > Let's create a definition of "a panel that can be attached to pompom"
> > > > > > as a panel that provides a valid EDID and can work with the standard
> > > > > > pompom power sequencing. If more panels are later attached to pompom
> > > > > > then it's fine as long as they work in a compatible way.
> > > > > >
> > > > > > One might ask why we can't just use a generic string here and provide
> > > > > > the timings directly in the device tree file. As I understand it,
> > > > > > trying to describe generic power sequencing in the device tree is
> > > > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > > > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > > > > per board class) feels like a reasonable compromise. We're not trying
> > > > > > to define fully generic power sequence bindings but we can also take
> > > > > > advantage of the semi-probable properties of the attached device.
> > > > > >
> > > > > > NOTE: I believe that past instances of supporting this type of thing
> > > > > > have used the "white lie" approach. One representative panel was
> > > > > > listed in the device tree. The power sequencings of this
> > > > > > representative panel were OK to use across all panels that might be
> > > > > > attached and other differences were handled by EDID. This patch
> > > > > > attempts to set a new precedent and avoid the need for the white lie.
> > > > > >
> > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > ---
> > > > >
> > > > > Sounds reasonable to me if DT maintainers can live with this abstract
> > > > > hardware definition. It's clearer than the 'white lie' approach.
> > > >
> > > > Yeah, it is a weird grey area between "discoverable" and "not
> > > > discoverable".. but I favor DT reflecting reality as much as
> > > > possible/feasible, so I think this is definity cleaner than "white
> > > > lies"
> > >
> > > This is practically no different than the "white lie". I suppose you
> > > could perhaps call it "more honest", if you want.
> > >
> > > The point remains that unless we describe exactly which panel we're
> > > dealing with, we ultimately have no way of properly quirking anything if
> > > we ever have to. Also, once we allow this kind of wildcard we can
> > > suddenly get into a situation where people might want to reuse this on
> > > something that's not at all a google-pompom board because the same
> > > particular power sequence happens to work on on some other board.
> > >
> > > Similarly I can imagine a situation where we could now have the same
> > > panel supported by multiple different wildcard compatible strings. How
> > > is that supposed to be any cleaner than what we have now?
> > >
> > > And I still keep wondering why bootloaders can't be taught about these
> > > kinds of things. We have in the past discussed various solutions where
> > > the bootloader could detect the type of panel connected and set the
> > > proper compatible string.
> >
> > The bootloader cannot detect the panel without powering up the panel,
> > which it normally does not do if you are not in dev-mode (it would add
> > a significant amount of time to bootup, which is why we can't do this)
> 
> what if we had a sort of multi-choice panel node:
> 
>    panel: panel {
>      compatible = "panel,one-of";
>      compatible-one-of = "vendor1,panel-a", "vendor2,panel-b",
> "vendor3,panel-c";
>   };
> 
> The kernel could construct power sequence timings that are the
> superset of all the possible panels.  That seems about as explicit as
> we could get in this sort of case.
> 

Being able to express a "panel selector" like this would certainly be
helpful in a number of phones, where a set of gpios or adc values are
read to determine which panel is actually mounted.

This is easier to do in the bootloader than your case, but the
bootloaders I've seen doing this have a tendency to come with a
dependency on the DT structure - which wouldn't match the upstream
approved DT bindings...

Regards,
Bjorn
Rob Herring March 26, 2021, 7:48 p.m. UTC | #5
On Fri, Mar 26, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 8:18 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > >
> > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > > > > pile of eDP panels. At the moment I'm told that the list might include:
> > > > > > - KD KD116N21-30NV-A010
> > > > > > - KD KD116N09-30NH-A016
> > > > > > - Starry 2081116HHD028001-51D
> > > > > > - Sharp LQ116M1JW10
> > > > > >
> > > > > > It should be noted that while the EDID programmed in the first 3
> > > > > > panels indicates that they should run with exactly the same timing (to
> > > > > > keep things simple), the 4th panel not only needs different timing but
> > > > > > has a different resolution.
> > > > > >
> > > > > > As is true in general with eDP panels, we can figure out which panel
> > > > > > we have and all the info needed to drive its pixel clock by reading
> > > > > > the EDID. However, we can do this only after we've powered the panel
> > > > > > on. Powering on the panels requires following the timing diagram in
> > > > > > each panel's datasheet which specifies delays between certain
> > > > > > actions. This means that, while we can be quite dynamic about handling
> > > > > > things we can't just totally skip out on describing the panel like we
> > > > > > could do if it was connected to an external-facing DP port.
> > > > > >
> > > > > > While the different panels have slightly different delays, it's
> > > > > > possible to come up with a set of unified delays that will work on all
> > > > > > the panels. From reading the datasheets:
> > > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > > >   - HPD absent delay: 200 ms
> > > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > > > > * Starry 2081116HHD028001-51D
> > > > > >   - HPD absent delay: 100 ms
> > > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > > >   - Unprepare delay: 500 ms
> > > > > > * Sharp LQ116M1JW10
> > > > > >   - HPD absent delay: 200 ms
> > > > > >   - Unprepare delay: 500 ms
> > > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > > >
> > > > > > Unified:
> > > > > > - HPD absent delay: 200 ms
> > > > > > - Unprepare delay: 500 ms
> > > > > > - Enable delay: 200 ms
> > > > > >
> > > > > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > > > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > > > > EDID and could make per-panel decisions if we wanted.
> > > > > >
> > > > > > Let's create a definition of "a panel that can be attached to pompom"
> > > > > > as a panel that provides a valid EDID and can work with the standard
> > > > > > pompom power sequencing. If more panels are later attached to pompom
> > > > > > then it's fine as long as they work in a compatible way.
> > > > > >
> > > > > > One might ask why we can't just use a generic string here and provide
> > > > > > the timings directly in the device tree file. As I understand it,
> > > > > > trying to describe generic power sequencing in the device tree is
> > > > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > > > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > > > > per board class) feels like a reasonable compromise. We're not trying
> > > > > > to define fully generic power sequence bindings but we can also take
> > > > > > advantage of the semi-probable properties of the attached device.
> > > > > >
> > > > > > NOTE: I believe that past instances of supporting this type of thing
> > > > > > have used the "white lie" approach. One representative panel was
> > > > > > listed in the device tree. The power sequencings of this
> > > > > > representative panel were OK to use across all panels that might be
> > > > > > attached and other differences were handled by EDID. This patch
> > > > > > attempts to set a new precedent and avoid the need for the white lie.
> > > > > >
> > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > ---
> > > > >
> > > > > Sounds reasonable to me if DT maintainers can live with this abstract
> > > > > hardware definition. It's clearer than the 'white lie' approach.
> > > >
> > > > Yeah, it is a weird grey area between "discoverable" and "not
> > > > discoverable".. but I favor DT reflecting reality as much as
> > > > possible/feasible, so I think this is definity cleaner than "white
> > > > lies"
> > >
> > > This is practically no different than the "white lie". I suppose you
> > > could perhaps call it "more honest", if you want.
> > >
> > > The point remains that unless we describe exactly which panel we're
> > > dealing with, we ultimately have no way of properly quirking anything if
> > > we ever have to. Also, once we allow this kind of wildcard we can
> > > suddenly get into a situation where people might want to reuse this on
> > > something that's not at all a google-pompom board because the same
> > > particular power sequence happens to work on on some other board.
> > >
> > > Similarly I can imagine a situation where we could now have the same
> > > panel supported by multiple different wildcard compatible strings. How
> > > is that supposed to be any cleaner than what we have now?
> > >
> > > And I still keep wondering why bootloaders can't be taught about these
> > > kinds of things. We have in the past discussed various solutions where
> > > the bootloader could detect the type of panel connected and set the
> > > proper compatible string.
> >
> > The bootloader cannot detect the panel without powering up the panel,
> > which it normally does not do if you are not in dev-mode (it would add
> > a significant amount of time to bootup, which is why we can't do this)
>
> what if we had a sort of multi-choice panel node:
>
>    panel: panel {
>      compatible = "panel,one-of";
>      compatible-one-of = "vendor1,panel-a", "vendor2,panel-b",
> "vendor3,panel-c";
>   };
>
> The kernel could construct power sequence timings that are the
> superset of all the possible panels.  That seems about as explicit as
> we could get in this sort of case.

If we were to go this route, I'm inclined to say just shove all the
possible panel compatibles into 'compatible'. That kind of breaks the
notion of most specific to least specific. OTOH, it is saying the set
of panels are somehow 'compatible' with each other.

If there's not some level of compatibility between the panels, then
it's still the bootloader's problem.

Rob
Rob Herring March 26, 2021, 11:48 p.m. UTC | #6
On Fri, Mar 26, 2021 at 4:13 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 12:48 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Mar 26, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Fri, Mar 26, 2021 at 8:18 AM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > > > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > >
> > > > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > > > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > > > > > > pile of eDP panels. At the moment I'm told that the list might include:
> > > > > > > > - KD KD116N21-30NV-A010
> > > > > > > > - KD KD116N09-30NH-A016
> > > > > > > > - Starry 2081116HHD028001-51D
> > > > > > > > - Sharp LQ116M1JW10
> > > > > > > >
> > > > > > > > It should be noted that while the EDID programmed in the first 3
> > > > > > > > panels indicates that they should run with exactly the same timing (to
> > > > > > > > keep things simple), the 4th panel not only needs different timing but
> > > > > > > > has a different resolution.
> > > > > > > >
> > > > > > > > As is true in general with eDP panels, we can figure out which panel
> > > > > > > > we have and all the info needed to drive its pixel clock by reading
> > > > > > > > the EDID. However, we can do this only after we've powered the panel
> > > > > > > > on. Powering on the panels requires following the timing diagram in
> > > > > > > > each panel's datasheet which specifies delays between certain
> > > > > > > > actions. This means that, while we can be quite dynamic about handling
> > > > > > > > things we can't just totally skip out on describing the panel like we
> > > > > > > > could do if it was connected to an external-facing DP port.
> > > > > > > >
> > > > > > > > While the different panels have slightly different delays, it's
> > > > > > > > possible to come up with a set of unified delays that will work on all
> > > > > > > > the panels. From reading the datasheets:
> > > > > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > > > > >   - HPD absent delay: 200 ms
> > > > > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > > > > > > * Starry 2081116HHD028001-51D
> > > > > > > >   - HPD absent delay: 100 ms
> > > > > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > > > > >   - Unprepare delay: 500 ms
> > > > > > > > * Sharp LQ116M1JW10
> > > > > > > >   - HPD absent delay: 200 ms
> > > > > > > >   - Unprepare delay: 500 ms
> > > > > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > > > > >
> > > > > > > > Unified:
> > > > > > > > - HPD absent delay: 200 ms
> > > > > > > > - Unprepare delay: 500 ms
> > > > > > > > - Enable delay: 200 ms
> > > > > > > >
> > > > > > > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > > > > > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > > > > > > EDID and could make per-panel decisions if we wanted.
> > > > > > > >
> > > > > > > > Let's create a definition of "a panel that can be attached to pompom"
> > > > > > > > as a panel that provides a valid EDID and can work with the standard
> > > > > > > > pompom power sequencing. If more panels are later attached to pompom
> > > > > > > > then it's fine as long as they work in a compatible way.
> > > > > > > >
> > > > > > > > One might ask why we can't just use a generic string here and provide
> > > > > > > > the timings directly in the device tree file. As I understand it,
> > > > > > > > trying to describe generic power sequencing in the device tree is
> > > > > > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > > > > > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > > > > > > per board class) feels like a reasonable compromise. We're not trying
> > > > > > > > to define fully generic power sequence bindings but we can also take
> > > > > > > > advantage of the semi-probable properties of the attached device.
> > > > > > > >
> > > > > > > > NOTE: I believe that past instances of supporting this type of thing
> > > > > > > > have used the "white lie" approach. One representative panel was
> > > > > > > > listed in the device tree. The power sequencings of this
> > > > > > > > representative panel were OK to use across all panels that might be
> > > > > > > > attached and other differences were handled by EDID. This patch
> > > > > > > > attempts to set a new precedent and avoid the need for the white lie.
> > > > > > > >
> > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > > ---
> > > > > > >
> > > > > > > Sounds reasonable to me if DT maintainers can live with this abstract
> > > > > > > hardware definition. It's clearer than the 'white lie' approach.
> > > > > >
> > > > > > Yeah, it is a weird grey area between "discoverable" and "not
> > > > > > discoverable".. but I favor DT reflecting reality as much as
> > > > > > possible/feasible, so I think this is definity cleaner than "white
> > > > > > lies"
> > > > >
> > > > > This is practically no different than the "white lie". I suppose you
> > > > > could perhaps call it "more honest", if you want.
> > > > >
> > > > > The point remains that unless we describe exactly which panel we're
> > > > > dealing with, we ultimately have no way of properly quirking anything if
> > > > > we ever have to. Also, once we allow this kind of wildcard we can
> > > > > suddenly get into a situation where people might want to reuse this on
> > > > > something that's not at all a google-pompom board because the same
> > > > > particular power sequence happens to work on on some other board.
> > > > >
> > > > > Similarly I can imagine a situation where we could now have the same
> > > > > panel supported by multiple different wildcard compatible strings. How
> > > > > is that supposed to be any cleaner than what we have now?
> > > > >
> > > > > And I still keep wondering why bootloaders can't be taught about these
> > > > > kinds of things. We have in the past discussed various solutions where
> > > > > the bootloader could detect the type of panel connected and set the
> > > > > proper compatible string.
> > > >
> > > > The bootloader cannot detect the panel without powering up the panel,
> > > > which it normally does not do if you are not in dev-mode (it would add
> > > > a significant amount of time to bootup, which is why we can't do this)
> > >
> > > what if we had a sort of multi-choice panel node:
> > >
> > >    panel: panel {
> > >      compatible = "panel,one-of";
> > >      compatible-one-of = "vendor1,panel-a", "vendor2,panel-b",
> > > "vendor3,panel-c";
> > >   };
> > >
> > > The kernel could construct power sequence timings that are the
> > > superset of all the possible panels.  That seems about as explicit as
> > > we could get in this sort of case.
> >
> > If we were to go this route, I'm inclined to say just shove all the
> > possible panel compatibles into 'compatible'. That kind of breaks the
> > notion of most specific to least specific. OTOH, it is saying the set
> > of panels are somehow 'compatible' with each other.
> >
> > If there's not some level of compatibility between the panels, then
> > it's still the bootloader's problem.
> >
>
> I'm not sure about this.. since there could be slight differences in
> various delay params between the possible panels.  I'd prefer that in
> panel-simple.c, we listed exact delay params "vendorFoo,panelBar", but
> it could mean that for a device that had three possible panels the
> worst case (max of all possible delays) could be higher than any
> individual choice.. and I don't think we should encourage the "white
> lie" approach (which will be the obvious outcome of not handling this
> directly in dt IME, based on prior art).  OTOH pushing it to the
> bootloader, when the bootloader actually has to power up the panel
> (and abide by the necessary delays) to figure out what choice we have
> isn't a viable option either.

I was only saying if the panels are different enough and there's not a
worse case setting, then it's back to a bootloader problem. If we have
multiple distinct compatibles, then it means the kernel should be able
to figure out settings that work on all the possible panels listed.

> It is better to be explicit about what we know and at the same time
> about what we don't know.

Can you be explicit about what we know and don't know here? With what
you proposed and my alternative, at the end of the day we just have a
list of compatibles. The only implicit part is the expectation that
the set is somehow compatible with each other.

Rob
Rob Clark March 27, 2021, 12:49 a.m. UTC | #7
On Fri, Mar 26, 2021 at 5:33 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 4:48 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Mar 26, 2021 at 4:13 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Fri, Mar 26, 2021 at 12:48 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Fri, Mar 26, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > >
> > > > > On Fri, Mar 26, 2021 at 8:18 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > > > > > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > > > > > > The sc7180-trogdor-pompom board might be attached to any number of a
> > > > > > > > > > pile of eDP panels. At the moment I'm told that the list might include:
> > > > > > > > > > - KD KD116N21-30NV-A010
> > > > > > > > > > - KD KD116N09-30NH-A016
> > > > > > > > > > - Starry 2081116HHD028001-51D
> > > > > > > > > > - Sharp LQ116M1JW10
> > > > > > > > > >
> > > > > > > > > > It should be noted that while the EDID programmed in the first 3
> > > > > > > > > > panels indicates that they should run with exactly the same timing (to
> > > > > > > > > > keep things simple), the 4th panel not only needs different timing but
> > > > > > > > > > has a different resolution.
> > > > > > > > > >
> > > > > > > > > > As is true in general with eDP panels, we can figure out which panel
> > > > > > > > > > we have and all the info needed to drive its pixel clock by reading
> > > > > > > > > > the EDID. However, we can do this only after we've powered the panel
> > > > > > > > > > on. Powering on the panels requires following the timing diagram in
> > > > > > > > > > each panel's datasheet which specifies delays between certain
> > > > > > > > > > actions. This means that, while we can be quite dynamic about handling
> > > > > > > > > > things we can't just totally skip out on describing the panel like we
> > > > > > > > > > could do if it was connected to an external-facing DP port.
> > > > > > > > > >
> > > > > > > > > > While the different panels have slightly different delays, it's
> > > > > > > > > > possible to come up with a set of unified delays that will work on all
> > > > > > > > > > the panels. From reading the datasheets:
> > > > > > > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > > > > > > >   - HPD absent delay: 200 ms
> > > > > > > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 ms)
> > > > > > > > > > * Starry 2081116HHD028001-51D
> > > > > > > > > >   - HPD absent delay: 100 ms
> > > > > > > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > > > > > > >   - Unprepare delay: 500 ms
> > > > > > > > > > * Sharp LQ116M1JW10
> > > > > > > > > >   - HPD absent delay: 200 ms
> > > > > > > > > >   - Unprepare delay: 500 ms
> > > > > > > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > > > > > > >
> > > > > > > > > > Unified:
> > > > > > > > > > - HPD absent delay: 200 ms
> > > > > > > > > > - Unprepare delay: 500 ms
> > > > > > > > > > - Enable delay: 200 ms
> > > > > > > > > >
> > > > > > > > > > NOTE: in theory the only thing that we _really_ need unity on is the
> > > > > > > > > > "HPD absent delay" since once the panel asserts HPD we can read the
> > > > > > > > > > EDID and could make per-panel decisions if we wanted.
> > > > > > > > > >
> > > > > > > > > > Let's create a definition of "a panel that can be attached to pompom"
> > > > > > > > > > as a panel that provides a valid EDID and can work with the standard
> > > > > > > > > > pompom power sequencing. If more panels are later attached to pompom
> > > > > > > > > > then it's fine as long as they work in a compatible way.
> > > > > > > > > >
> > > > > > > > > > One might ask why we can't just use a generic string here and provide
> > > > > > > > > > the timings directly in the device tree file. As I understand it,
> > > > > > > > > > trying to describe generic power sequencing in the device tree is
> > > > > > > > > > frowned upon and the one instance (SD/MMC) is regarded as a mistake
> > > > > > > > > > that shouldn't be repeated. Specifying a power sequence per board (or
> > > > > > > > > > per board class) feels like a reasonable compromise. We're not trying
> > > > > > > > > > to define fully generic power sequence bindings but we can also take
> > > > > > > > > > advantage of the semi-probable properties of the attached device.
> > > > > > > > > >
> > > > > > > > > > NOTE: I believe that past instances of supporting this type of thing
> > > > > > > > > > have used the "white lie" approach. One representative panel was
> > > > > > > > > > listed in the device tree. The power sequencings of this
> > > > > > > > > > representative panel were OK to use across all panels that might be
> > > > > > > > > > attached and other differences were handled by EDID. This patch
> > > > > > > > > > attempts to set a new precedent and avoid the need for the white lie.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Sounds reasonable to me if DT maintainers can live with this abstract
> > > > > > > > > hardware definition. It's clearer than the 'white lie' approach.
> > > > > > > >
> > > > > > > > Yeah, it is a weird grey area between "discoverable" and "not
> > > > > > > > discoverable".. but I favor DT reflecting reality as much as
> > > > > > > > possible/feasible, so I think this is definity cleaner than "white
> > > > > > > > lies"
> > > > > > >
> > > > > > > This is practically no different than the "white lie". I suppose you
> > > > > > > could perhaps call it "more honest", if you want.
> > > > > > >
> > > > > > > The point remains that unless we describe exactly which panel we're
> > > > > > > dealing with, we ultimately have no way of properly quirking anything if
> > > > > > > we ever have to. Also, once we allow this kind of wildcard we can
> > > > > > > suddenly get into a situation where people might want to reuse this on
> > > > > > > something that's not at all a google-pompom board because the same
> > > > > > > particular power sequence happens to work on on some other board.
> > > > > > >
> > > > > > > Similarly I can imagine a situation where we could now have the same
> > > > > > > panel supported by multiple different wildcard compatible strings. How
> > > > > > > is that supposed to be any cleaner than what we have now?
> > > > > > >
> > > > > > > And I still keep wondering why bootloaders can't be taught about these
> > > > > > > kinds of things. We have in the past discussed various solutions where
> > > > > > > the bootloader could detect the type of panel connected and set the
> > > > > > > proper compatible string.
> > > > > >
> > > > > > The bootloader cannot detect the panel without powering up the panel,
> > > > > > which it normally does not do if you are not in dev-mode (it would add
> > > > > > a significant amount of time to bootup, which is why we can't do this)
> > > > >
> > > > > what if we had a sort of multi-choice panel node:
> > > > >
> > > > >    panel: panel {
> > > > >      compatible = "panel,one-of";
> > > > >      compatible-one-of = "vendor1,panel-a", "vendor2,panel-b",
> > > > > "vendor3,panel-c";
> > > > >   };
> > > > >
> > > > > The kernel could construct power sequence timings that are the
> > > > > superset of all the possible panels.  That seems about as explicit as
> > > > > we could get in this sort of case.
> > > >
> > > > If we were to go this route, I'm inclined to say just shove all the
> > > > possible panel compatibles into 'compatible'. That kind of breaks the
> > > > notion of most specific to least specific. OTOH, it is saying the set
> > > > of panels are somehow 'compatible' with each other.
> > > >
> > > > If there's not some level of compatibility between the panels, then
> > > > it's still the bootloader's problem.
> > > >
> > >
> > > I'm not sure about this.. since there could be slight differences in
> > > various delay params between the possible panels.  I'd prefer that in
> > > panel-simple.c, we listed exact delay params "vendorFoo,panelBar", but
> > > it could mean that for a device that had three possible panels the
> > > worst case (max of all possible delays) could be higher than any
> > > individual choice.. and I don't think we should encourage the "white
> > > lie" approach (which will be the obvious outcome of not handling this
> > > directly in dt IME, based on prior art).  OTOH pushing it to the
> > > bootloader, when the bootloader actually has to power up the panel
> > > (and abide by the necessary delays) to figure out what choice we have
> > > isn't a viable option either.
> >
> > I was only saying if the panels are different enough and there's not a
> > worse case setting, then it's back to a bootloader problem. If we have
> > multiple distinct compatibles, then it means the kernel should be able
> > to figure out settings that work on all the possible panels listed.
> >
> > > It is better to be explicit about what we know and at the same time
> > > about what we don't know.
> >
> > Can you be explicit about what we know and don't know here? With what
> > you proposed and my alternative, at the end of the day we just have a
> > list of compatibles. The only implicit part is the expectation that
> > the set is somehow compatible with each other.
> >
>
> Ok, I think I was being incompatible with my definition of "compatible" ;-)
>
> To make sure we are on the same page, this is what I have in mind:
>
> 1) the panels are compatible enough that if a user breaks their panel
> and takes device in for repair, they might end up with a different
> panel
> 2) the different possible panels may have different power-on delay,
> etc, but max of all possible power on delays is fine and enough to get
> the kernel to the point that it can probe EDID and figure out the rest
>

(by which I mean, I think that is what you are saying.. earlier I was
considering panel A, B, and C as not being compatible if they had
different power on delays)

BR,
-R
Doug Anderson March 29, 2021, 4:07 p.m. UTC | #8
Hi,

On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> The point remains that unless we describe exactly which panel we're
> dealing with, we ultimately have no way of properly quirking anything if
> we ever have to.

Just to clarify here: with my initial proposal we actually could still
quirk things if we had to. If the quirk needed to be applied before
power on we'd just have to apply the quirk to the whole board (which
we'd have to do anyway). After the panel was powered on then we could
read the EDID and apply a quirk based on what the EDID tells us,
right?


> Also, once we allow this kind of wildcard we can
> suddenly get into a situation where people might want to reuse this on
> something that's not at all a google-pompom board because the same
> particular power sequence happens to work on on some other board.

That's a legit concern. Of course, people could already do that with
existing panels right? One would also hope that if they reused this
they also used the "more specific to least specific" rule, so someone
could reuse (without any problems) with:

compatible = "some-other-company,some-other-board-panel", "google,pompom-panel"

That doesn't seem like it would be terrible.


> Similarly I can imagine a situation where we could now have the same
> panel supported by multiple different wildcard compatible strings. How
> is that supposed to be any cleaner than what we have now?

I'm tempted to call this (same panel supported by multiple different
compatible strings) a feature, actually. Specifically:

Even if the exact same hardware is shipped with more than one board,
it may have a different EDID programmed into it. From what I've seen
the timings used on a panel may need to be adjusted based on the SoC
used (and what clock rates it can provide / features of the underlying
display driver), EMI concerns, and power consumption concerns. Once a
different EDID is programmed in it then it sorta becomes a "different"
panel, right? I think sometimes (?) panel vendors assign a slightly
different model number per board, but I'm not sure.


-Doug
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
index 62b0d54d87b7..9807dbc1cceb 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
@@ -140,6 +140,10 @@  properties:
       - giantplus,gpg48273qs5
         # GiantPlus GPM940B0 3.0" QVGA TFT LCD panel
       - giantplus,gpm940b0
+        # A panel connected to a google,pompom board. Panel is guaranteed to
+        # confirm to google,pompom-panel power sequencing requirements and then
+        # the specific panel will be probed via EDID.
+      - google,pompom-panel
         # HannStar Display Corp. HSD070PWW1 7.0" WXGA TFT LCD panel
       - hannstar,hsd070pww1
         # HannStar Display Corp. HSD100PXN1 10.1" XGA LVDS panel