From: Doug Anderson <dianders@chromium.org>
To: Rajeev Nandan <rajeevny@codeaurora.org>
Cc: Rob Herring <robh@kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
freedreno <freedreno@lists.freedesktop.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
Rob Clark <robdclark@gmail.com>,
mkrishn@codeaurora.org, Kalyan Thota <kalyan_t@codeaurora.org>,
"Kristian H. Kristensen" <hoegsberg@chromium.org>,
Abhinav Kumar <abhinavk@codeaurora.org>,
Sean Paul <seanpaul@chromium.org>,
Jani Nikula <jani.nikula@linux.intel.com>,
Lyude Paul <lyude@redhat.com>,
"Lankhorst, Maarten" <maarten.lankhorst@intel.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight
Date: Mon, 17 May 2021 13:21:13 -0700 [thread overview]
Message-ID: <CAD=FV=XPDLn187AR06xzChwT1Y0pCeQmRvhO_MjEoMoabXwybg@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=XBb9zs=ZGG-Ky8=_is20L1O6pJ-xBV1k5cF6-vL78pgA@mail.gmail.com>
Hi,
On Tue, May 11, 2021 at 4:17 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, May 11, 2021 at 11:12 AM <rajeevny@codeaurora.org> wrote:
> >
> > On 01-05-2021 03:08, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Apr 30, 2021 at 8:10 AM <rajeevny@codeaurora.org> wrote:
> > >>
> > >> On 30-04-2021 02:33, Doug Anderson wrote:
> > >> > Hi,
> > >> >
> > >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <robh@kernel.org> wrote:
> > >> >>
> > >> >> On Mon, Apr 26, 2021 at 11:29:15AM +0530, Rajeev Nandan wrote:
> > >> >> > Add bindings for DisplayPort aux backlight driver.
> > >> >> >
> > >> >> > Changes in v2:
> > >> >> > - New
> > >> >> >
> > >> >> > Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org>
> > >> >> > ---
> > >> >> > .../bindings/leds/backlight/dp-aux-backlight.yaml | 49 ++++++++++++++++++++++
> > >> >> > 1 file changed, 49 insertions(+)
> > >> >> > create mode 100644 Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> >
> > >> >> > diff --git a/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> > new file mode 100644
> > >> >> > index 00000000..0fa8bf0
> > >> >> > --- /dev/null
> > >> >> > +++ b/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> > @@ -0,0 +1,49 @@
> > >> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >> >> > +%YAML 1.2
> > >> >> > +---
> > >> >> > +$id: http://devicetree.org/schemas/leds/backlight/dp-aux-backlight.yaml#
> > >> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> >> > +
> > >> >> > +title: DisplayPort aux backlight driver bindings
> > >> >> > +
> > >> >> > +maintainers:
> > >> >> > + - Rajeev Nandan <rajeevny@codeaurora.org>
> > >> >> > +
> > >> >> > +description:
> > >> >> > + Backlight driver to control the brightness over DisplayPort aux channel.
> > >> >> > +
> > >> >> > +allOf:
> > >> >> > + - $ref: common.yaml#
> > >> >> > +
> > >> >> > +properties:
> > >> >> > + compatible:
> > >> >> > + const: dp-aux-backlight
> > >> >> > +
> > >> >> > + ddc-i2c-bus:
> > >> >> > + $ref: /schemas/types.yaml#/definitions/phandle
> > >> >> > + description:
> > >> >> > + A phandle to the system I2C controller connected to the DDC bus used
> > >> >> > + for the DisplayPort AUX channel.
> > >> >> > +
> > >> >> > + enable-gpios:
> > >> >> > + maxItems: 1
> > >> >> > + description: GPIO specifier for backlight enable pin.
> > >> >> > +
> > >> >> > + max-brightness: true
> > >> >> > +
> > >> >> > +required:
> > >> >> > + - compatible
> > >> >> > + - ddc-i2c-bus
> > >> >> > +
> > >> >> > +additionalProperties: false
> > >> >> > +
> > >> >> > +examples:
> > >> >> > + - |
> > >> >> > + backlight {
> > >> >> > + compatible = "dp-aux-backlight";
> > >> >> > + ddc-i2c-bus = <&sn65dsi86_bridge>;
> > >> >> > + enable-gpios = <&tlmm 12 GPIO_ACTIVE_HIGH>;
> > >> >>
> > >> >> So the DDC bus is connected to a backlight and also a panel? This
> > >> >> binding is not reflecting the h/w, but rather what you want for some
> > >> >> driver.
> > >> >>
> > >> >> There's only one thing here and that's an eDP panel which supports
> > >> >> backlight control via DP aux channel. You can figure all that out from
> > >> >> the panel's compatible and/or reading the EDID.
> > >> >>
> > >> >> You might also be interested in this thread:
> > >> >>
> > >> >> https://lore.kernel.org/lkml/YIKsDtjcIHGNvW0u@orome.fritz.box/
> > >> >
> > >> > I think Rajeev needs to rework everything anyway as per:
> > >> >
> > >> > https://lore.kernel.org/r/87zgxl5qar.fsf@intel.com
> > >> >
> > >> > ...but you're right that it makes sense not to model the backlight as
> > >> > a separate node in the device tree. The panel driver can handle
> > >> > setting up the backlight.
> > >> >
> > >> > -Doug
> > >>
> > >> It was not a good idea to create a separate backlight driver and use
> > >> ddc-i2c-bus to get access to DP aux. I am working to move the code
> > >> to the panel driver and to utilize the new DRM helper functions
> > >> (drm_edp_backlight_*) Lyude has added [1].
> > >>
> > >> To use these helper functions, the panel driver should have access to
> > >> the
> > >> "struct drm_dp_aux *". The simple-panel has a "ddc-i2c-bus" property
> > >> to give the panel access to the DDC bus and is currently being used to
> > >> get the EDID from the panel. Can I use the same ddc bus i2c_adapter to
> > >> get
> > >> the "struct drm_dp_aux *"?
> > >>
> > >> As per the suggestion [2], I get the "struct drm_dp_aux *" from the
> > >> i2c_adapter of ddc bus (maybe I didn't understand the suggestion
> > >> correctly),
> > >> and, it turned out, the way I have implemented is not the right way
> > >> [3].
> > >> So, I am afraid to use the same method in the panel driver.
> > >>
> > >>
> > >> [1] https://lore.kernel.org/dri-devel/871rb5bcf9.fsf@intel.com/
> > >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> > >> [3]
> > >> https://lore.kernel.org/dri-devel/20210426111116.4lc3ekxjugjr3oho@maple.lan/
> > >
> > > So it's definitely up to maintainers, not me. ...but I guess I would
> > > have expected something like a new property called "ddc-aux-bus". Then
> > > you'd have to create a new API call called something like
> > > "of_find_ddc_aux_adapter_by_node()" that would allow you to find it.
> > >
> >
> > To implement the first suggestion, I can think of the following way
> > to get the "struct drm_dp_aux" in the panel_simple_probe function:
> >
> > - Create a new panel-simple DT property "ddc-aux-bus", a phandle to the
> > platform device that implements the AUX channel.
> >
> > - Create a global list of drm_dp_aux in drm_dp_helper.c. Initialize list
> > head
> > in drm_dp_aux_init(), add the drm_dp_aux onto the list in
> > drm_dp_aux_register().
> > Similarly, remove the drm_dp_aux from list in drm_dp_aux_unregister().
> >
> > - Create a new function of_drm_find_dp_aux_by_node() to get the expected
> > drm_dp_aux from this global list.
> >
> > Please let me know your views on this implementation.
>
> BTW: a bunch of discussion today on IRC:
>
> https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2021-05-11&show_html=true
To finish adding breadcrumbs to those playing at home, v7 of my patch
series should get the AUX channel to the panel device:
https://lore.kernel.org/r/20210517200907.1459182-1-dianders@chromium.org/
Assuming that people don't hate that solution it may be a good things
for you to build your patches atop. :-)
-Doug
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org>
To: Rajeev Nandan <rajeevny@codeaurora.org>
Cc: mkrishn@codeaurora.org,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Andrzej Hajda <a.hajda@samsung.com>,
Sean Paul <seanpaul@chromium.org>,
Abhinav Kumar <abhinavk@codeaurora.org>,
Kalyan Thota <kalyan_t@codeaurora.org>,
"Kristian H. Kristensen" <hoegsberg@chromium.org>,
freedreno <freedreno@lists.freedesktop.org>,
"Lankhorst, Maarten" <maarten.lankhorst@intel.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight
Date: Mon, 17 May 2021 13:21:13 -0700 [thread overview]
Message-ID: <CAD=FV=XPDLn187AR06xzChwT1Y0pCeQmRvhO_MjEoMoabXwybg@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=XBb9zs=ZGG-Ky8=_is20L1O6pJ-xBV1k5cF6-vL78pgA@mail.gmail.com>
Hi,
On Tue, May 11, 2021 at 4:17 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, May 11, 2021 at 11:12 AM <rajeevny@codeaurora.org> wrote:
> >
> > On 01-05-2021 03:08, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Apr 30, 2021 at 8:10 AM <rajeevny@codeaurora.org> wrote:
> > >>
> > >> On 30-04-2021 02:33, Doug Anderson wrote:
> > >> > Hi,
> > >> >
> > >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <robh@kernel.org> wrote:
> > >> >>
> > >> >> On Mon, Apr 26, 2021 at 11:29:15AM +0530, Rajeev Nandan wrote:
> > >> >> > Add bindings for DisplayPort aux backlight driver.
> > >> >> >
> > >> >> > Changes in v2:
> > >> >> > - New
> > >> >> >
> > >> >> > Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org>
> > >> >> > ---
> > >> >> > .../bindings/leds/backlight/dp-aux-backlight.yaml | 49 ++++++++++++++++++++++
> > >> >> > 1 file changed, 49 insertions(+)
> > >> >> > create mode 100644 Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> >
> > >> >> > diff --git a/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> > new file mode 100644
> > >> >> > index 00000000..0fa8bf0
> > >> >> > --- /dev/null
> > >> >> > +++ b/Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml
> > >> >> > @@ -0,0 +1,49 @@
> > >> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >> >> > +%YAML 1.2
> > >> >> > +---
> > >> >> > +$id: http://devicetree.org/schemas/leds/backlight/dp-aux-backlight.yaml#
> > >> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> >> > +
> > >> >> > +title: DisplayPort aux backlight driver bindings
> > >> >> > +
> > >> >> > +maintainers:
> > >> >> > + - Rajeev Nandan <rajeevny@codeaurora.org>
> > >> >> > +
> > >> >> > +description:
> > >> >> > + Backlight driver to control the brightness over DisplayPort aux channel.
> > >> >> > +
> > >> >> > +allOf:
> > >> >> > + - $ref: common.yaml#
> > >> >> > +
> > >> >> > +properties:
> > >> >> > + compatible:
> > >> >> > + const: dp-aux-backlight
> > >> >> > +
> > >> >> > + ddc-i2c-bus:
> > >> >> > + $ref: /schemas/types.yaml#/definitions/phandle
> > >> >> > + description:
> > >> >> > + A phandle to the system I2C controller connected to the DDC bus used
> > >> >> > + for the DisplayPort AUX channel.
> > >> >> > +
> > >> >> > + enable-gpios:
> > >> >> > + maxItems: 1
> > >> >> > + description: GPIO specifier for backlight enable pin.
> > >> >> > +
> > >> >> > + max-brightness: true
> > >> >> > +
> > >> >> > +required:
> > >> >> > + - compatible
> > >> >> > + - ddc-i2c-bus
> > >> >> > +
> > >> >> > +additionalProperties: false
> > >> >> > +
> > >> >> > +examples:
> > >> >> > + - |
> > >> >> > + backlight {
> > >> >> > + compatible = "dp-aux-backlight";
> > >> >> > + ddc-i2c-bus = <&sn65dsi86_bridge>;
> > >> >> > + enable-gpios = <&tlmm 12 GPIO_ACTIVE_HIGH>;
> > >> >>
> > >> >> So the DDC bus is connected to a backlight and also a panel? This
> > >> >> binding is not reflecting the h/w, but rather what you want for some
> > >> >> driver.
> > >> >>
> > >> >> There's only one thing here and that's an eDP panel which supports
> > >> >> backlight control via DP aux channel. You can figure all that out from
> > >> >> the panel's compatible and/or reading the EDID.
> > >> >>
> > >> >> You might also be interested in this thread:
> > >> >>
> > >> >> https://lore.kernel.org/lkml/YIKsDtjcIHGNvW0u@orome.fritz.box/
> > >> >
> > >> > I think Rajeev needs to rework everything anyway as per:
> > >> >
> > >> > https://lore.kernel.org/r/87zgxl5qar.fsf@intel.com
> > >> >
> > >> > ...but you're right that it makes sense not to model the backlight as
> > >> > a separate node in the device tree. The panel driver can handle
> > >> > setting up the backlight.
> > >> >
> > >> > -Doug
> > >>
> > >> It was not a good idea to create a separate backlight driver and use
> > >> ddc-i2c-bus to get access to DP aux. I am working to move the code
> > >> to the panel driver and to utilize the new DRM helper functions
> > >> (drm_edp_backlight_*) Lyude has added [1].
> > >>
> > >> To use these helper functions, the panel driver should have access to
> > >> the
> > >> "struct drm_dp_aux *". The simple-panel has a "ddc-i2c-bus" property
> > >> to give the panel access to the DDC bus and is currently being used to
> > >> get the EDID from the panel. Can I use the same ddc bus i2c_adapter to
> > >> get
> > >> the "struct drm_dp_aux *"?
> > >>
> > >> As per the suggestion [2], I get the "struct drm_dp_aux *" from the
> > >> i2c_adapter of ddc bus (maybe I didn't understand the suggestion
> > >> correctly),
> > >> and, it turned out, the way I have implemented is not the right way
> > >> [3].
> > >> So, I am afraid to use the same method in the panel driver.
> > >>
> > >>
> > >> [1] https://lore.kernel.org/dri-devel/871rb5bcf9.fsf@intel.com/
> > >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> > >> [3]
> > >> https://lore.kernel.org/dri-devel/20210426111116.4lc3ekxjugjr3oho@maple.lan/
> > >
> > > So it's definitely up to maintainers, not me. ...but I guess I would
> > > have expected something like a new property called "ddc-aux-bus". Then
> > > you'd have to create a new API call called something like
> > > "of_find_ddc_aux_adapter_by_node()" that would allow you to find it.
> > >
> >
> > To implement the first suggestion, I can think of the following way
> > to get the "struct drm_dp_aux" in the panel_simple_probe function:
> >
> > - Create a new panel-simple DT property "ddc-aux-bus", a phandle to the
> > platform device that implements the AUX channel.
> >
> > - Create a global list of drm_dp_aux in drm_dp_helper.c. Initialize list
> > head
> > in drm_dp_aux_init(), add the drm_dp_aux onto the list in
> > drm_dp_aux_register().
> > Similarly, remove the drm_dp_aux from list in drm_dp_aux_unregister().
> >
> > - Create a new function of_drm_find_dp_aux_by_node() to get the expected
> > drm_dp_aux from this global list.
> >
> > Please let me know your views on this implementation.
>
> BTW: a bunch of discussion today on IRC:
>
> https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2021-05-11&show_html=true
To finish adding breadcrumbs to those playing at home, v7 of my patch
series should get the AUX channel to the panel device:
https://lore.kernel.org/r/20210517200907.1459182-1-dianders@chromium.org/
Assuming that people don't hate that solution it may be a good things
for you to build your patches atop. :-)
-Doug
next prev parent reply other threads:[~2021-05-17 20:21 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-26 5:59 [v3 0/2] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge Rajeev Nandan
2021-04-26 5:59 ` Rajeev Nandan
2021-04-26 5:59 ` [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight Rajeev Nandan
2021-04-26 5:59 ` Rajeev Nandan
2021-04-26 21:04 ` Rob Herring
2021-04-26 21:04 ` Rob Herring
2021-04-29 18:04 ` Rob Herring
2021-04-29 18:04 ` Rob Herring
2021-04-29 21:03 ` Doug Anderson
2021-04-29 21:03 ` Doug Anderson
2021-04-30 15:10 ` rajeevny
2021-04-30 15:10 ` rajeevny
2021-04-30 21:38 ` Doug Anderson
2021-04-30 21:38 ` Doug Anderson
2021-05-11 18:11 ` rajeevny
2021-05-11 18:11 ` rajeevny
2021-05-11 22:33 ` Laurent Pinchart
2021-05-11 22:33 ` Laurent Pinchart
2021-05-11 23:17 ` Doug Anderson
2021-05-11 23:17 ` Doug Anderson
2021-05-17 20:21 ` Doug Anderson [this message]
2021-05-17 20:21 ` Doug Anderson
2021-05-25 8:03 ` rajeevny
2021-05-25 8:03 ` rajeevny
2021-04-26 5:59 ` [v3 2/2] backlight: Add DisplayPort aux backlight driver Rajeev Nandan
2021-04-26 5:59 ` Rajeev Nandan
2021-04-26 9:49 ` Jani Nikula
2021-04-26 9:49 ` [Intel-gfx] " Jani Nikula
2021-04-26 9:49 ` Jani Nikula
2021-04-26 14:57 ` rajeevny
2021-04-26 14:57 ` [Intel-gfx] " rajeevny
2021-04-26 14:57 ` rajeevny
2021-04-30 18:48 ` Lyude Paul
2021-04-30 18:48 ` Lyude Paul
2021-04-30 18:48 ` [Intel-gfx] " Lyude Paul
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='CAD=FV=XPDLn187AR06xzChwT1Y0pCeQmRvhO_MjEoMoabXwybg@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=a.hajda@samsung.com \
--cc=abhinavk@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=hoegsberg@chromium.org \
--cc=jani.nikula@linux.intel.com \
--cc=kalyan_t@codeaurora.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lyude@redhat.com \
--cc=maarten.lankhorst@intel.com \
--cc=mkrishn@codeaurora.org \
--cc=rajeevny@codeaurora.org \
--cc=robdclark@gmail.com \
--cc=robh@kernel.org \
--cc=seanpaul@chromium.org \
/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 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.