Add bindings for DisplayPort aux backlight driver.
Changes in v2:
- New
Signed-off-by: Rajeev Nandan <[email protected]>
---
.../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 <[email protected]>
+
+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>;
+ max-brightness = <2047>;
+ };
+
+...
--
2.7.4
On Mon, 26 Apr 2021 11:29:15 +0530, Rajeev Nandan wrote:
> Add bindings for DisplayPort aux backlight driver.
>
> Changes in v2:
> - New
>
> Signed-off-by: Rajeev Nandan <[email protected]>
> ---
> .../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
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.example.dts:22.38-39 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:377: Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1414: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1470203
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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/[email protected]/
Rob
Hi,
On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +
> > +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/[email protected]/
I think Rajeev needs to rework everything anyway as per:
https://lore.kernel.org/r/[email protected]
...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
On 30-04-2021 02:33, Doug Anderson wrote:
> Hi,
>
> On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
>> > ---
>> > .../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 <[email protected]>
>> > +
>> > +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/[email protected]/
>
> I think Rajeev needs to rework everything anyway as per:
>
> https://lore.kernel.org/r/[email protected]
>
> ...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/[email protected]/
[2] https://www.spinics.net/lists/dri-devel/msg295429.html
[3]
https://lore.kernel.org/dri-devel/[email protected]/
Thanks,
Rajeev
Hi,
On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
>
> On 30-04-2021 02:33, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
> >> > ---
> >> > .../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 <[email protected]>
> >> > +
> >> > +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/[email protected]/
> >
> > I think Rajeev needs to rework everything anyway as per:
> >
> > https://lore.kernel.org/r/[email protected]
> >
> > ...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/[email protected]/
> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> [3]
> https://lore.kernel.org/dri-devel/[email protected]/
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.
I guess an alternate way to solve this (I'm not totally sure whether
it's better or worse) would be to add a function that would walk up
the chain of parent bridges and ask them for a pointer to the aux bus.
I definitely haven't thought it all the way through, but I'd imagine
something like drm_bridge_chain_get_ddc_aux(). This is _probably_
better than adding the "ddc-aux-bus" property but it assumes that the
aux bus is provided by one of our parents. Hrm, looking at this
briefly, though, I'm not sure how to do it. It doesn't seem possible
to get the parent bridges from the panel structure. Even if you assume
that your parent is wrapping you with a panel_bridge it still doesn't
seem possible?
This probably needs more drm-expertise.
-Doug
On 01-05-2021 03:08, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
>>
>> On 30-04-2021 02:33, Doug Anderson wrote:
>> > Hi,
>> >
>> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
>> >> > ---
>> >> > .../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 <[email protected]>
>> >> > +
>> >> > +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/[email protected]/
>> >
>> > I think Rajeev needs to rework everything anyway as per:
>> >
>> > https://lore.kernel.org/r/[email protected]
>> >
>> > ...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/[email protected]/
>> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
>> [3]
>> https://lore.kernel.org/dri-devel/[email protected]/
>
> 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.
Below is the summary of the changes in drm dp helper:
---
// drm_dp_helper.h
struct drm_dp_aux {
...
struct list_head list;
...
}
// drm_dp_helper.c
static DEFINE_MUTEX(dp_aux_lock);
static LIST_HEAD(dp_aux_list);
static void drm_dp_aux_add(struct drm_dp_aux *aux)
{
mutex_lock(&dp_aux_lock);
list_add_tail(&aux->list, &dp_aux_list);
mutex_unlock(&dp_aux_lock);
}
static void drm_dp_aux_remove(struct drm_dp_aux *aux)
{
mutex_lock(&dp_aux_lock);
list_del_init(&aux->list);
mutex_unlock(&dp_aux_lock);
}
#ifdef CONFIG_OF
struct drm_dp_aux *of_drm_find_dp_aux_by_node(struct device_node *np)
{
struct drm_dp_aux *aux;
mutex_lock(&dp_aux_lock);
list_for_each_entry(aux, &dp_aux_list, list) {
if (aux->dev->of_node == np) {
mutex_unlock(&dp_aux_lock);
return aux;
}
}
mutex_unlock(&dp_aux_lock);
return NULL;
}
EXPORT_SYMBOL(of_drm_find_dp_aux_by_node);
#endif
int drm_dp_aux_init(struct drm_dp_aux *aux)
{
INIT_LIST_HEAD(&aux->list);
...
}
int drm_dp_aux_register(struct drm_dp_aux *aux)
{
...
drm_dp_aux_add(aux);
return 0;
}
void drm_dp_aux_unregister(struct drm_dp_aux *aux)
{
drm_dp_aux_remove(aux);
...
}
---
Thanks,
Rajeev
> I guess an alternate way to solve this (I'm not totally sure whether
> it's better or worse) would be to add a function that would walk up
> the chain of parent bridges and ask them for a pointer to the aux bus.
> I definitely haven't thought it all the way through, but I'd imagine
> something like drm_bridge_chain_get_ddc_aux(). This is _probably_
> better than adding the "ddc-aux-bus" property but it assumes that the
> aux bus is provided by one of our parents. Hrm, looking at this
> briefly, though, I'm not sure how to do it. It doesn't seem possible
> to get the parent bridges from the panel structure. Even if you assume
> that your parent is wrapping you with a panel_bridge it still doesn't
> seem possible?
>
> This probably needs more drm-expertise.
>
> -Doug
Hi Rajeevny,
On Tue, May 11, 2021 at 11:41:57PM +0530, [email protected] wrote:
> On 01-05-2021 03:08, Doug Anderson wrote:
> > On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
> >> On 30-04-2021 02:33, Doug Anderson wrote:
> >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
> >> >> > ---
> >> >> > .../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 <[email protected]>
> >> >> > +
> >> >> > +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/[email protected]/
> >> >
> >> > I think Rajeev needs to rework everything anyway as per:
> >> >
> >> > https://lore.kernel.org/r/[email protected]
> >> >
> >> > ...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.
> >>
> >> 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/[email protected]/
> >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> >> [3]
> >> https://lore.kernel.org/dri-devel/[email protected]/
> >
> > 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.
>
> Below is the summary of the changes in drm dp helper:
>
> ---
>
> // drm_dp_helper.h
>
> struct drm_dp_aux {
> ...
> struct list_head list;
> ...
> }
>
> // drm_dp_helper.c
>
> static DEFINE_MUTEX(dp_aux_lock);
> static LIST_HEAD(dp_aux_list);
>
> static void drm_dp_aux_add(struct drm_dp_aux *aux)
> {
> mutex_lock(&dp_aux_lock);
> list_add_tail(&aux->list, &dp_aux_list);
> mutex_unlock(&dp_aux_lock);
> }
>
> static void drm_dp_aux_remove(struct drm_dp_aux *aux)
> {
> mutex_lock(&dp_aux_lock);
> list_del_init(&aux->list);
> mutex_unlock(&dp_aux_lock);
> }
>
> #ifdef CONFIG_OF
> struct drm_dp_aux *of_drm_find_dp_aux_by_node(struct device_node *np)
> {
> struct drm_dp_aux *aux;
> mutex_lock(&dp_aux_lock);
>
> list_for_each_entry(aux, &dp_aux_list, list) {
> if (aux->dev->of_node == np) {
> mutex_unlock(&dp_aux_lock);
> return aux;
> }
> }
>
> mutex_unlock(&dp_aux_lock);
> return NULL;
> }
> EXPORT_SYMBOL(of_drm_find_dp_aux_by_node);
> #endif
>
>
> int drm_dp_aux_init(struct drm_dp_aux *aux)
> {
> INIT_LIST_HEAD(&aux->list);
> ...
> }
>
> int drm_dp_aux_register(struct drm_dp_aux *aux)
> {
> ...
> drm_dp_aux_add(aux);
>
> return 0;
> }
>
> void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> {
> drm_dp_aux_remove(aux);
> ...
> }
Overall this seems like a good approach, but there's one unanswered
question: what happens if drm_dp_aux_unregister() is called while a
panel holds a reference to it ? The drm_dp_aux instances likely need to
be reference-counted.
> > I guess an alternate way to solve this (I'm not totally sure whether
> > it's better or worse) would be to add a function that would walk up
> > the chain of parent bridges and ask them for a pointer to the aux bus.
> > I definitely haven't thought it all the way through, but I'd imagine
> > something like drm_bridge_chain_get_ddc_aux(). This is _probably_
> > better than adding the "ddc-aux-bus" property but it assumes that the
> > aux bus is provided by one of our parents. Hrm, looking at this
> > briefly, though, I'm not sure how to do it. It doesn't seem possible
> > to get the parent bridges from the panel structure. Even if you assume
> > that your parent is wrapping you with a panel_bridge it still doesn't
> > seem possible?
> >
> > This probably needs more drm-expertise.
--
Regards,
Laurent Pinchart
Hi,
On Tue, May 11, 2021 at 11:12 AM <[email protected]> wrote:
>
> On 01-05-2021 03:08, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
> >>
> >> On 30-04-2021 02:33, Doug Anderson wrote:
> >> > Hi,
> >> >
> >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
> >> >> > ---
> >> >> > .../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 <[email protected]>
> >> >> > +
> >> >> > +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/[email protected]/
> >> >
> >> > I think Rajeev needs to rework everything anyway as per:
> >> >
> >> > https://lore.kernel.org/r/[email protected]
> >> >
> >> > ...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/[email protected]/
> >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> >> [3]
> >> https://lore.kernel.org/dri-devel/[email protected]/
> >
> > 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
-Doug
Hi,
On Tue, May 11, 2021 at 4:17 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, May 11, 2021 at 11:12 AM <[email protected]> wrote:
> >
> > On 01-05-2021 03:08, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
> > >>
> > >> On 30-04-2021 02:33, Doug Anderson wrote:
> > >> > Hi,
> > >> >
> > >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
> > >> >> > ---
> > >> >> > .../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 <[email protected]>
> > >> >> > +
> > >> >> > +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/[email protected]/
> > >> >
> > >> > I think Rajeev needs to rework everything anyway as per:
> > >> >
> > >> > https://lore.kernel.org/r/[email protected]
> > >> >
> > >> > ...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/[email protected]/
> > >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
> > >> [3]
> > >> https://lore.kernel.org/dri-devel/[email protected]/
> > >
> > > 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/[email protected]/
Assuming that people don't hate that solution it may be a good things
for you to build your patches atop. :-)
-Doug
Hi,
On 18-05-2021 01:51, Doug Anderson wrote:
> Hi,
>
> On Tue, May 11, 2021 at 4:17 PM Doug Anderson <[email protected]>
> wrote:
>>
>> Hi,
>>
>> On Tue, May 11, 2021 at 11:12 AM <[email protected]> wrote:
>> >
>> > On 01-05-2021 03:08, Doug Anderson wrote:
>> > > Hi,
>> > >
>> > > On Fri, Apr 30, 2021 at 8:10 AM <[email protected]> wrote:
>> > >>
>> > >> On 30-04-2021 02:33, Doug Anderson wrote:
>> > >> > Hi,
>> > >> >
>> > >> > On Thu, Apr 29, 2021 at 11:04 AM Rob Herring <[email protected]> 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 <[email protected]>
>> > >> >> > ---
>> > >> >> > .../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 <[email protected]>
>> > >> >> > +
>> > >> >> > +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/[email protected]/
>> > >> >
>> > >> > I think Rajeev needs to rework everything anyway as per:
>> > >> >
>> > >> > https://lore.kernel.org/r/[email protected]
>> > >> >
>> > >> > ...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/[email protected]/
>> > >> [2] https://www.spinics.net/lists/dri-devel/msg295429.html
>> > >> [3]
>> > >> https://lore.kernel.org/dri-devel/[email protected]/
>> > >
>> > > 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/[email protected]/
>
> Assuming that people don't hate that solution it may be a good things
> for you to build your patches atop. :-)
>
> -Doug
Thank you, for all your suggestions.
I have posted the new patch series (v4) on top of the above series.
Please check:
https://lore.kernel.org/dri-devel/[email protected]/
Thanks,
Rajeev