* [v3 0/2] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge @ 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 ` [v3 2/2] backlight: Add DisplayPort aux backlight driver Rajeev Nandan 0 siblings, 2 replies; 16+ messages in thread From: Rajeev Nandan @ 2021-04-26 5:59 UTC (permalink / raw) To: dri-devel, linux-arm-msm, freedreno, devicetree Cc: mkrishn, Rajeev Nandan, linux-kernel, abhinavk, dianders, seanpaul, kalyan_t, hoegsberg The backlight level of an eDP panel can be controlled through the AUX channel using DPCD registers of the panel. The capability for the Source device to adjust backlight characteristics within the panel, using the Sink device DPCD registers is indicated by the TCON_BACKLIGHT_ADJUSTMENT_CAPABLE bit in the EDP_GENERAL_CAPABILITY_1 register (DPCD Address 701h, bit0). In this configuration, the eDP TCON receives the backlight level information from the host, through the AUX channel. Anderson's patch series [1] exposed the DDC bus from ti-sn65dsi86 bridge, that gives an option to move the backlight control out of the bridge and to create a separate backlight driver. Changes in v2: - Created a new DisplayPort aux backlight driver and moved the code from drm_dp_aux_backlight.c (v1) to the new driver. - Removed the changes done in ti-sn65dsi86 bridge. (Rob Herring) Changes in v3: - Add missing ';' to fix module compilation (kernel test bot) [1] https://lore.kernel.org/dri-devel/20210416223950.3586967-1-dianders@chromium.org/ Rajeev Nandan (2): dt-bindings: backlight: add DisplayPort aux backlight backlight: Add DisplayPort aux backlight driver .../bindings/leds/backlight/dp-aux-backlight.yaml | 49 +++++ drivers/video/backlight/Kconfig | 7 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/dp_aux_backlight.c | 245 +++++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/dp-aux-backlight.yaml create mode 100644 drivers/video/backlight/dp_aux_backlight.c -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 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 21:04 ` Rob Herring 2021-04-29 18:04 ` Rob Herring 2021-04-26 5:59 ` [v3 2/2] backlight: Add DisplayPort aux backlight driver Rajeev Nandan 1 sibling, 2 replies; 16+ messages in thread From: Rajeev Nandan @ 2021-04-26 5:59 UTC (permalink / raw) To: dri-devel, linux-arm-msm, freedreno, devicetree Cc: mkrishn, Rajeev Nandan, linux-kernel, abhinavk, dianders, seanpaul, kalyan_t, hoegsberg 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>; + max-brightness = <2047>; + }; + +... -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-26 5:59 ` [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight Rajeev Nandan @ 2021-04-26 21:04 ` Rob Herring 2021-04-29 18:04 ` Rob Herring 1 sibling, 0 replies; 16+ messages in thread From: Rob Herring @ 2021-04-26 21:04 UTC (permalink / raw) To: Rajeev Nandan Cc: mkrishn, devicetree, linux-arm-msm, dianders, dri-devel, linux-kernel, seanpaul, abhinavk, kalyan_t, hoegsberg, freedreno 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 <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 > 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-26 5:59 ` [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight Rajeev Nandan 2021-04-26 21:04 ` Rob Herring @ 2021-04-29 18:04 ` Rob Herring 2021-04-29 21:03 ` Doug Anderson 1 sibling, 1 reply; 16+ messages in thread From: Rob Herring @ 2021-04-29 18:04 UTC (permalink / raw) To: Rajeev Nandan Cc: devicetree, mkrishn, linux-arm-msm, linux-kernel, dri-devel, dianders, seanpaul, abhinavk, kalyan_t, hoegsberg, freedreno 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/ Rob _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-29 18:04 ` Rob Herring @ 2021-04-29 21:03 ` Doug Anderson 2021-04-30 15:10 ` rajeevny 0 siblings, 1 reply; 16+ messages in thread From: Doug Anderson @ 2021-04-29 21:03 UTC (permalink / raw) To: Rob Herring Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajeev Nandan, mkrishn, linux-arm-msm, LKML, dri-devel, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-29 21:03 ` Doug Anderson @ 2021-04-30 15:10 ` rajeevny 2021-04-30 21:38 ` Doug Anderson 0 siblings, 1 reply; 16+ messages in thread From: rajeevny @ 2021-04-30 15:10 UTC (permalink / raw) To: Doug Anderson, Rob Herring Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, mkrishn, linux-arm-msm, LKML, dri-devel, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten 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/ Thanks, Rajeev _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-30 15:10 ` rajeevny @ 2021-04-30 21:38 ` Doug Anderson 2021-05-11 18:11 ` rajeevny 0 siblings, 1 reply; 16+ messages in thread From: Doug Anderson @ 2021-04-30 21:38 UTC (permalink / raw) To: Rajeev Nandan Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, LKML, dri-devel, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten, Laurent Pinchart 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. 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-04-30 21:38 ` Doug Anderson @ 2021-05-11 18:11 ` rajeevny 2021-05-11 22:33 ` Laurent Pinchart 2021-05-11 23:17 ` Doug Anderson 0 siblings, 2 replies; 16+ messages in thread From: rajeevny @ 2021-05-11 18:11 UTC (permalink / raw) To: Doug Anderson Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, LKML, dri-devel, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten, Laurent Pinchart 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. 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-05-11 18:11 ` rajeevny @ 2021-05-11 22:33 ` Laurent Pinchart 2021-05-11 23:17 ` Doug Anderson 1 sibling, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2021-05-11 22:33 UTC (permalink / raw) To: rajeevny Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, Doug Anderson, dri-devel, LKML, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten Hi Rajeevny, On Tue, May 11, 2021 at 11:41:57PM +0530, rajeevny@codeaurora.org wrote: > On 01-05-2021 03:08, Doug Anderson wrote: > > On Fri, Apr 30, 2021 at 8:10 AM <rajeevny@codeaurora.org> wrote: > >> On 30-04-2021 02:33, Doug Anderson wrote: > >> > 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. > >> > >> 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. > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-05-11 18:11 ` rajeevny 2021-05-11 22:33 ` Laurent Pinchart @ 2021-05-11 23:17 ` Doug Anderson 2021-05-17 20:21 ` Doug Anderson 1 sibling, 1 reply; 16+ messages in thread From: Doug Anderson @ 2021-05-11 23:17 UTC (permalink / raw) To: Rajeev Nandan Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, LKML, dri-devel, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten, Laurent Pinchart 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 -Doug ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-05-11 23:17 ` Doug Anderson @ 2021-05-17 20:21 ` Doug Anderson 2021-05-25 8:03 ` rajeevny 0 siblings, 1 reply; 16+ messages in thread From: Doug Anderson @ 2021-05-17 20:21 UTC (permalink / raw) To: Rajeev Nandan Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, LKML, dri-devel, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight 2021-05-17 20:21 ` Doug Anderson @ 2021-05-25 8:03 ` rajeevny 0 siblings, 0 replies; 16+ messages in thread From: rajeevny @ 2021-05-25 8:03 UTC (permalink / raw) To: Doug Anderson Cc: mkrishn, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-msm, LKML, dri-devel, Andrzej Hajda, Sean Paul, Abhinav Kumar, Kalyan Thota, Kristian H. Kristensen, freedreno, Lankhorst, Maarten, Laurent Pinchart Hi, On 18-05-2021 01:51, Doug Anderson wrote: > 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 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/1621927831-29471-1-git-send-email-rajeevny@codeaurora.org/ Thanks, Rajeev ^ permalink raw reply [flat|nested] 16+ messages in thread
* [v3 2/2] backlight: Add DisplayPort aux backlight driver 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 ` [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight Rajeev Nandan @ 2021-04-26 5:59 ` Rajeev Nandan 2021-04-26 9:49 ` Jani Nikula 1 sibling, 1 reply; 16+ messages in thread From: Rajeev Nandan @ 2021-04-26 5:59 UTC (permalink / raw) To: dri-devel, linux-arm-msm, freedreno, devicetree Cc: mkrishn, Rajeev Nandan, linux-kernel, abhinavk, dianders, seanpaul, kalyan_t, hoegsberg Add backlight driver for the panels supporting backlight control using DPCD registers on the DisplayPort aux channel. Changes in v2: - New (most of the code reused from drm_dp_aux_backlight.c of v1) Changes in v3: - Add missing ';' to fix module compilation (kernel test bot) Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org> --- drivers/video/backlight/Kconfig | 7 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/dp_aux_backlight.c | 245 +++++++++++++++++++++++++++++ 3 files changed, 253 insertions(+) create mode 100644 drivers/video/backlight/dp_aux_backlight.c diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index d83c87b..82c88f0 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -456,6 +456,13 @@ config BACKLIGHT_LED If you have a LCD backlight adjustable by LED class driver, say Y to enable this driver. +config BACKLIGHT_DP_AUX + tristate "DisplayPort aux backlight driver" + depends on DRM && DRM_KMS_HELPER + help + If you have a panel backlight controlled by DPCD registers + on the DisplayPort aux channel, say Y to enable this driver. + endif # BACKLIGHT_CLASS_DEVICE endmenu diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index 685f3f1..ba23c7c 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X) += wm831x_bl.o obj-$(CONFIG_BACKLIGHT_ARCXCNN) += arcxcnn_bl.o obj-$(CONFIG_BACKLIGHT_RAVE_SP) += rave-sp-backlight.o obj-$(CONFIG_BACKLIGHT_LED) += led_bl.o +obj-$(CONFIG_BACKLIGHT_DP_AUX) += dp_aux_backlight.o diff --git a/drivers/video/backlight/dp_aux_backlight.c b/drivers/video/backlight/dp_aux_backlight.c new file mode 100644 index 00000000..3398383 --- /dev/null +++ b/drivers/video/backlight/dp_aux_backlight.c @@ -0,0 +1,245 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Backlight driver to control the brightness over DisplayPort aux channel. + */ + +#include <linux/backlight.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <drm/drm_dp_helper.h> + +#define DP_AUX_MAX_BRIGHTNESS 0xffff + +/** + * struct dp_aux_backlight - DisplayPort aux backlight data + * @dev: pointer to our device. + * @aux: the DisplayPort aux channel. + * @enable_gpio: the backlight enable gpio. + * @enabled: true if backlight is enabled else false. + */ +struct dp_aux_backlight { + struct device *dev; + struct drm_dp_aux *aux; + struct gpio_desc *enable_gpio; + bool enabled; +}; + +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c) +{ + return container_of(i2c, struct drm_dp_aux, ddc); +} + +static int dp_aux_backlight_enable(struct dp_aux_backlight *aux_bl) +{ + u8 val = 0; + int ret; + + if (aux_bl->enabled) + return 0; + + /* Set backlight control mode */ + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, + &val); + if (ret < 0) + return ret; + + val &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; + val |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, + val); + if (ret < 0) + return ret; + + /* Enable backlight */ + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, + &val); + if (ret < 0) + return ret; + + val |= DP_EDP_BACKLIGHT_ENABLE; + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, + val); + if (ret < 0) + return ret; + + if (aux_bl->enable_gpio) + gpiod_set_value(aux_bl->enable_gpio, 1); + + aux_bl->enabled = true; + + return 0; +} + +static int dp_aux_backlight_disable(struct dp_aux_backlight *aux_bl) +{ + u8 val = 0; + int ret; + + if (!aux_bl->enabled) + return 0; + + if (aux_bl->enable_gpio) + gpiod_set_value(aux_bl->enable_gpio, 0); + + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, + &val); + if (ret < 0) + return ret; + + val &= ~DP_EDP_BACKLIGHT_ENABLE; + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, + val); + if (ret < 0) + return ret; + + aux_bl->enabled = false; + + return 0; +} + +static int dp_aux_backlight_update_status(struct backlight_device *bd) +{ + struct dp_aux_backlight *aux_bl = bl_get_data(bd); + u16 brightness = backlight_get_brightness(bd); + u8 val[2] = { 0x0 }; + int ret = 0; + + if (brightness > 0) { + val[0] = brightness >> 8; + val[1] = brightness & 0xff; + ret = drm_dp_dpcd_write(aux_bl->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, + val, sizeof(val)); + if (ret < 0) + return ret; + + dp_aux_backlight_enable(aux_bl); + } else { + dp_aux_backlight_disable(aux_bl); + } + + return 0; +} + +static int dp_aux_backlight_get_brightness(struct backlight_device *bd) +{ + struct dp_aux_backlight *aux_bl = bl_get_data(bd); + u8 val[2] = { 0x0 }; + int ret = 0; + + if (backlight_is_blank(bd)) + return 0; + + ret = drm_dp_dpcd_read(aux_bl->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, + &val, sizeof(val)); + if (ret < 0) + return ret; + + return (val[0] << 8 | val[1]); +} + +static const struct backlight_ops aux_bl_ops = { + .update_status = dp_aux_backlight_update_status, + .get_brightness = dp_aux_backlight_get_brightness, +}; + + +static int dp_aux_backlight_probe(struct platform_device *pdev) +{ + struct dp_aux_backlight *aux_bl; + struct backlight_device *bd; + struct backlight_properties bl_props = { 0 }; + struct device_node *np; + struct i2c_adapter *ddc; + int ret = 0; + u32 val; + + aux_bl = devm_kzalloc(&pdev->dev, sizeof(*aux_bl), GFP_KERNEL); + if (!aux_bl) + return -ENOMEM; + + aux_bl->dev = &pdev->dev; + + np = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0); + if (!np) { + dev_err(&pdev->dev, "failed to get aux ddc I2C bus\n"); + return -ENODEV; + } + + ddc = of_find_i2c_adapter_by_node(np); + of_node_put(np); + if (!ddc) + return -EPROBE_DEFER; + + aux_bl->aux = i2c_to_aux(ddc); + dev_dbg(&pdev->dev, "using dp aux %s\n", aux_bl->aux->name); + + aux_bl->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", + GPIOD_OUT_LOW); + if (IS_ERR(aux_bl->enable_gpio)) { + ret = PTR_ERR(aux_bl->enable_gpio); + goto free_ddc; + } + + val = DP_AUX_MAX_BRIGHTNESS; + of_property_read_u32(pdev->dev.of_node, "max-brightness", &val); + if (val > DP_AUX_MAX_BRIGHTNESS) + val = DP_AUX_MAX_BRIGHTNESS; + + bl_props.max_brightness = val; + bl_props.brightness = val; + bl_props.type = BACKLIGHT_RAW; + bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev), + &pdev->dev, aux_bl, + &aux_bl_ops, &bl_props); + if (IS_ERR(bd)) { + ret = PTR_ERR(bd); + dev_err(&pdev->dev, + "failed to register backlight (%d)\n", ret); + goto free_ddc; + } + + platform_set_drvdata(pdev, bd); + + return 0; + +free_ddc: + if (ddc) + put_device(&ddc->dev); + + return ret; +} + +static int dp_aux_backlight_remove(struct platform_device *pdev) +{ + struct backlight_device *bd = platform_get_drvdata(pdev); + struct dp_aux_backlight *aux_bl = bl_get_data(bd); + struct i2c_adapter *ddc = &aux_bl->aux->ddc; + + if (ddc) + put_device(&ddc->dev); + + return 0; +} + +static const struct of_device_id dp_aux_bl_of_match_table[] = { + { .compatible = "dp-aux-backlight"}, + {}, +}; +MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table); + +static struct platform_driver dp_aux_backlight_driver = { + .driver = { + .name = "dp-aux-backlight", + .of_match_table = dp_aux_bl_of_match_table, + }, + .probe = dp_aux_backlight_probe, + .remove = dp_aux_backlight_remove, + +}; +module_platform_driver(dp_aux_backlight_driver); + +MODULE_DESCRIPTION("DisplayPort aux backlight driver"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [v3 2/2] backlight: Add DisplayPort aux backlight driver 2021-04-26 5:59 ` [v3 2/2] backlight: Add DisplayPort aux backlight driver Rajeev Nandan @ 2021-04-26 9:49 ` Jani Nikula 2021-04-26 14:57 ` rajeevny 2021-04-30 18:48 ` Lyude Paul 0 siblings, 2 replies; 16+ messages in thread From: Jani Nikula @ 2021-04-26 9:49 UTC (permalink / raw) To: Rajeev Nandan, dri-devel, linux-arm-msm, freedreno, devicetree Cc: mkrishn, Rajeev Nandan, Daniel Vetter, intel-gfx, linux-kernel, abhinavk, dianders, seanpaul, Thomas Zimmermann, kalyan_t, hoegsberg, Lankhorst, Maarten On Mon, 26 Apr 2021, Rajeev Nandan <rajeevny@codeaurora.org> wrote: > Add backlight driver for the panels supporting backlight control > using DPCD registers on the DisplayPort aux channel. No, please don't do this. I wrote you last week in reply to v1 why I thought merging this would not be a good idea [1]. Why have you sent two versions since then without replying to me, or Cc'ing me or Lyude? I think it's an even worse idea to merge this to drivers/video/backlight. With DP AUX backlight you can't pretend it's just an independent aux interface for backlight without everything else around it. It's not independent of eDP, and exposing it as a direct backlight sysfs interface bypasses the encoder. And it still remains that there is existing DP AUX backlight code in use, in the tree, with more features than this, with plans and previously submitted patches to lift from one driver to drm core, and with patches to add support to another driver. I don't say this lightly, or very often at all, but, NAK. BR, Jani. [1] https://lore.kernel.org/dri-devel/871rb5bcf9.fsf@intel.com/ > > Changes in v2: > - New (most of the code reused from drm_dp_aux_backlight.c of v1) > > Changes in v3: > - Add missing ';' to fix module compilation (kernel test bot) > > Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org> > --- > drivers/video/backlight/Kconfig | 7 + > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/dp_aux_backlight.c | 245 +++++++++++++++++++++++++++++ > 3 files changed, 253 insertions(+) > create mode 100644 drivers/video/backlight/dp_aux_backlight.c > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index d83c87b..82c88f0 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -456,6 +456,13 @@ config BACKLIGHT_LED > If you have a LCD backlight adjustable by LED class driver, say Y > to enable this driver. > > +config BACKLIGHT_DP_AUX > + tristate "DisplayPort aux backlight driver" > + depends on DRM && DRM_KMS_HELPER > + help > + If you have a panel backlight controlled by DPCD registers > + on the DisplayPort aux channel, say Y to enable this driver. > + > endif # BACKLIGHT_CLASS_DEVICE > > endmenu > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile > index 685f3f1..ba23c7c 100644 > --- a/drivers/video/backlight/Makefile > +++ b/drivers/video/backlight/Makefile > @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X) += wm831x_bl.o > obj-$(CONFIG_BACKLIGHT_ARCXCNN) += arcxcnn_bl.o > obj-$(CONFIG_BACKLIGHT_RAVE_SP) += rave-sp-backlight.o > obj-$(CONFIG_BACKLIGHT_LED) += led_bl.o > +obj-$(CONFIG_BACKLIGHT_DP_AUX) += dp_aux_backlight.o > diff --git a/drivers/video/backlight/dp_aux_backlight.c b/drivers/video/backlight/dp_aux_backlight.c > new file mode 100644 > index 00000000..3398383 > --- /dev/null > +++ b/drivers/video/backlight/dp_aux_backlight.c > @@ -0,0 +1,245 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Backlight driver to control the brightness over DisplayPort aux channel. > + */ > + > +#include <linux/backlight.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <drm/drm_dp_helper.h> > + > +#define DP_AUX_MAX_BRIGHTNESS 0xffff > + > +/** > + * struct dp_aux_backlight - DisplayPort aux backlight data > + * @dev: pointer to our device. > + * @aux: the DisplayPort aux channel. > + * @enable_gpio: the backlight enable gpio. > + * @enabled: true if backlight is enabled else false. > + */ > +struct dp_aux_backlight { > + struct device *dev; > + struct drm_dp_aux *aux; > + struct gpio_desc *enable_gpio; > + bool enabled; > +}; > + > +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c) > +{ > + return container_of(i2c, struct drm_dp_aux, ddc); > +} > + > +static int dp_aux_backlight_enable(struct dp_aux_backlight *aux_bl) > +{ > + u8 val = 0; > + int ret; > + > + if (aux_bl->enabled) > + return 0; > + > + /* Set backlight control mode */ > + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > + &val); > + if (ret < 0) > + return ret; > + > + val &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; > + val |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > + val); > + if (ret < 0) > + return ret; > + > + /* Enable backlight */ > + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, > + &val); > + if (ret < 0) > + return ret; > + > + val |= DP_EDP_BACKLIGHT_ENABLE; > + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, > + val); > + if (ret < 0) > + return ret; > + > + if (aux_bl->enable_gpio) > + gpiod_set_value(aux_bl->enable_gpio, 1); > + > + aux_bl->enabled = true; > + > + return 0; > +} > + > +static int dp_aux_backlight_disable(struct dp_aux_backlight *aux_bl) > +{ > + u8 val = 0; > + int ret; > + > + if (!aux_bl->enabled) > + return 0; > + > + if (aux_bl->enable_gpio) > + gpiod_set_value(aux_bl->enable_gpio, 0); > + > + ret = drm_dp_dpcd_readb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, > + &val); > + if (ret < 0) > + return ret; > + > + val &= ~DP_EDP_BACKLIGHT_ENABLE; > + ret = drm_dp_dpcd_writeb(aux_bl->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, > + val); > + if (ret < 0) > + return ret; > + > + aux_bl->enabled = false; > + > + return 0; > +} > + > +static int dp_aux_backlight_update_status(struct backlight_device *bd) > +{ > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > + u16 brightness = backlight_get_brightness(bd); > + u8 val[2] = { 0x0 }; > + int ret = 0; > + > + if (brightness > 0) { > + val[0] = brightness >> 8; > + val[1] = brightness & 0xff; > + ret = drm_dp_dpcd_write(aux_bl->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > + val, sizeof(val)); > + if (ret < 0) > + return ret; > + > + dp_aux_backlight_enable(aux_bl); > + } else { > + dp_aux_backlight_disable(aux_bl); > + } > + > + return 0; > +} > + > +static int dp_aux_backlight_get_brightness(struct backlight_device *bd) > +{ > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > + u8 val[2] = { 0x0 }; > + int ret = 0; > + > + if (backlight_is_blank(bd)) > + return 0; > + > + ret = drm_dp_dpcd_read(aux_bl->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > + &val, sizeof(val)); > + if (ret < 0) > + return ret; > + > + return (val[0] << 8 | val[1]); > +} > + > +static const struct backlight_ops aux_bl_ops = { > + .update_status = dp_aux_backlight_update_status, > + .get_brightness = dp_aux_backlight_get_brightness, > +}; > + > + > +static int dp_aux_backlight_probe(struct platform_device *pdev) > +{ > + struct dp_aux_backlight *aux_bl; > + struct backlight_device *bd; > + struct backlight_properties bl_props = { 0 }; > + struct device_node *np; > + struct i2c_adapter *ddc; > + int ret = 0; > + u32 val; > + > + aux_bl = devm_kzalloc(&pdev->dev, sizeof(*aux_bl), GFP_KERNEL); > + if (!aux_bl) > + return -ENOMEM; > + > + aux_bl->dev = &pdev->dev; > + > + np = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0); > + if (!np) { > + dev_err(&pdev->dev, "failed to get aux ddc I2C bus\n"); > + return -ENODEV; > + } > + > + ddc = of_find_i2c_adapter_by_node(np); > + of_node_put(np); > + if (!ddc) > + return -EPROBE_DEFER; > + > + aux_bl->aux = i2c_to_aux(ddc); > + dev_dbg(&pdev->dev, "using dp aux %s\n", aux_bl->aux->name); > + > + aux_bl->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(aux_bl->enable_gpio)) { > + ret = PTR_ERR(aux_bl->enable_gpio); > + goto free_ddc; > + } > + > + val = DP_AUX_MAX_BRIGHTNESS; > + of_property_read_u32(pdev->dev.of_node, "max-brightness", &val); > + if (val > DP_AUX_MAX_BRIGHTNESS) > + val = DP_AUX_MAX_BRIGHTNESS; > + > + bl_props.max_brightness = val; > + bl_props.brightness = val; > + bl_props.type = BACKLIGHT_RAW; > + bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev), > + &pdev->dev, aux_bl, > + &aux_bl_ops, &bl_props); > + if (IS_ERR(bd)) { > + ret = PTR_ERR(bd); > + dev_err(&pdev->dev, > + "failed to register backlight (%d)\n", ret); > + goto free_ddc; > + } > + > + platform_set_drvdata(pdev, bd); > + > + return 0; > + > +free_ddc: > + if (ddc) > + put_device(&ddc->dev); > + > + return ret; > +} > + > +static int dp_aux_backlight_remove(struct platform_device *pdev) > +{ > + struct backlight_device *bd = platform_get_drvdata(pdev); > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > + struct i2c_adapter *ddc = &aux_bl->aux->ddc; > + > + if (ddc) > + put_device(&ddc->dev); > + > + return 0; > +} > + > +static const struct of_device_id dp_aux_bl_of_match_table[] = { > + { .compatible = "dp-aux-backlight"}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table); > + > +static struct platform_driver dp_aux_backlight_driver = { > + .driver = { > + .name = "dp-aux-backlight", > + .of_match_table = dp_aux_bl_of_match_table, > + }, > + .probe = dp_aux_backlight_probe, > + .remove = dp_aux_backlight_remove, > + > +}; > +module_platform_driver(dp_aux_backlight_driver); > + > +MODULE_DESCRIPTION("DisplayPort aux backlight driver"); > +MODULE_LICENSE("GPL v2"); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 2/2] backlight: Add DisplayPort aux backlight driver 2021-04-26 9:49 ` Jani Nikula @ 2021-04-26 14:57 ` rajeevny 2021-04-30 18:48 ` Lyude Paul 1 sibling, 0 replies; 16+ messages in thread From: rajeevny @ 2021-04-26 14:57 UTC (permalink / raw) To: Jani Nikula Cc: mkrishn, Daniel Thompson, devicetree, linux-arm-msm, intel-gfx, linux-kernel, dri-devel, dianders, Daniel Vetter, seanpaul, abhinavk, Thomas Zimmermann, kalyan_t, hoegsberg, freedreno, Lankhorst, Maarten On 26-04-2021 15:19, Jani Nikula wrote: > On Mon, 26 Apr 2021, Rajeev Nandan <rajeevny@codeaurora.org> wrote: >> Add backlight driver for the panels supporting backlight control >> using DPCD registers on the DisplayPort aux channel. > > No, please don't do this. > > I wrote you last week in reply to v1 why I thought merging this would > not be a good idea [1]. Why have you sent two versions since then > without replying to me, or Cc'ing me or Lyude? > > I think it's an even worse idea to merge this to > drivers/video/backlight. With DP AUX backlight you can't pretend it's > just an independent aux interface for backlight without everything else > around it. It's not independent of eDP, and exposing it as a direct > backlight sysfs interface bypasses the encoder. > > And it still remains that there is existing DP AUX backlight code in > use, in the tree, with more features than this, with plans and > previously submitted patches to lift from one driver to drm core, and > with patches to add support to another driver. > > I don't say this lightly, or very often at all, but, > > NAK. > > > BR, > Jani. > > > [1] https://lore.kernel.org/dri-devel/871rb5bcf9.fsf@intel.com/ > Hi Jani, Apologies for not acknowledging your comment on v1. I was looking here [1] for all the comments I would be receiving on my patches. I was not aware that the comments on the cover letter don't appear on this page. Also, I completely missed your mail. I will ensure such omissions don't happen again. Keeping your comments in mind, I will look into Lyude's series and work upon my code. [1] https://patchwork.freedesktop.org/series/89085/ Yours sincerely, Rajeev >> >> Changes in v2: >> - New (most of the code reused from drm_dp_aux_backlight.c of v1) >> >> Changes in v3: >> - Add missing ';' to fix module compilation (kernel test bot) >> >> Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org> >> --- >> drivers/video/backlight/Kconfig | 7 + >> drivers/video/backlight/Makefile | 1 + >> drivers/video/backlight/dp_aux_backlight.c | 245 >> +++++++++++++++++++++++++++++ >> 3 files changed, 253 insertions(+) >> create mode 100644 drivers/video/backlight/dp_aux_backlight.c >> >> diff --git a/drivers/video/backlight/Kconfig >> b/drivers/video/backlight/Kconfig >> index d83c87b..82c88f0 100644 >> --- a/drivers/video/backlight/Kconfig >> +++ b/drivers/video/backlight/Kconfig >> @@ -456,6 +456,13 @@ config BACKLIGHT_LED >> If you have a LCD backlight adjustable by LED class driver, say Y >> to enable this driver. >> >> +config BACKLIGHT_DP_AUX >> + tristate "DisplayPort aux backlight driver" >> + depends on DRM && DRM_KMS_HELPER >> + help >> + If you have a panel backlight controlled by DPCD registers >> + on the DisplayPort aux channel, say Y to enable this driver. >> + >> endif # BACKLIGHT_CLASS_DEVICE >> >> endmenu >> diff --git a/drivers/video/backlight/Makefile >> b/drivers/video/backlight/Makefile >> index 685f3f1..ba23c7c 100644 >> --- a/drivers/video/backlight/Makefile >> +++ b/drivers/video/backlight/Makefile >> @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X) += wm831x_bl.o >> obj-$(CONFIG_BACKLIGHT_ARCXCNN) += arcxcnn_bl.o >> obj-$(CONFIG_BACKLIGHT_RAVE_SP) += rave-sp-backlight.o >> obj-$(CONFIG_BACKLIGHT_LED) += led_bl.o >> +obj-$(CONFIG_BACKLIGHT_DP_AUX) += dp_aux_backlight.o >> diff --git a/drivers/video/backlight/dp_aux_backlight.c >> b/drivers/video/backlight/dp_aux_backlight.c >> new file mode 100644 >> index 00000000..3398383 >> --- /dev/null >> +++ b/drivers/video/backlight/dp_aux_backlight.c >> @@ -0,0 +1,245 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Backlight driver to control the brightness over DisplayPort aux >> channel. >> + */ >> + >> +#include <linux/backlight.h> >> +#include <linux/err.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <drm/drm_dp_helper.h> >> + >> +#define DP_AUX_MAX_BRIGHTNESS 0xffff >> + >> +/** >> + * struct dp_aux_backlight - DisplayPort aux backlight data >> + * @dev: pointer to our device. >> + * @aux: the DisplayPort aux channel. >> + * @enable_gpio: the backlight enable gpio. >> + * @enabled: true if backlight is enabled else false. >> + */ >> +struct dp_aux_backlight { >> + struct device *dev; >> + struct drm_dp_aux *aux; >> + struct gpio_desc *enable_gpio; >> + bool enabled; >> +}; >> + >> +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c) >> +{ >> + return container_of(i2c, struct drm_dp_aux, ddc); >> +} >> + >> +static int dp_aux_backlight_enable(struct dp_aux_backlight *aux_bl) >> +{ >> + u8 val = 0; >> + int ret; >> + >> + if (aux_bl->enabled) >> + return 0; >> + >> + /* Set backlight control mode */ >> + ret = drm_dp_dpcd_readb(aux_bl->aux, >> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, >> + &val); >> + if (ret < 0) >> + return ret; >> + >> + val &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; >> + val |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; >> + ret = drm_dp_dpcd_writeb(aux_bl->aux, >> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, >> + val); >> + if (ret < 0) >> + return ret; >> + >> + /* Enable backlight */ >> + ret = drm_dp_dpcd_readb(aux_bl->aux, >> DP_EDP_DISPLAY_CONTROL_REGISTER, >> + &val); >> + if (ret < 0) >> + return ret; >> + >> + val |= DP_EDP_BACKLIGHT_ENABLE; >> + ret = drm_dp_dpcd_writeb(aux_bl->aux, >> DP_EDP_DISPLAY_CONTROL_REGISTER, >> + val); >> + if (ret < 0) >> + return ret; >> + >> + if (aux_bl->enable_gpio) >> + gpiod_set_value(aux_bl->enable_gpio, 1); >> + >> + aux_bl->enabled = true; >> + >> + return 0; >> +} >> + >> +static int dp_aux_backlight_disable(struct dp_aux_backlight *aux_bl) >> +{ >> + u8 val = 0; >> + int ret; >> + >> + if (!aux_bl->enabled) >> + return 0; >> + >> + if (aux_bl->enable_gpio) >> + gpiod_set_value(aux_bl->enable_gpio, 0); >> + >> + ret = drm_dp_dpcd_readb(aux_bl->aux, >> DP_EDP_DISPLAY_CONTROL_REGISTER, >> + &val); >> + if (ret < 0) >> + return ret; >> + >> + val &= ~DP_EDP_BACKLIGHT_ENABLE; >> + ret = drm_dp_dpcd_writeb(aux_bl->aux, >> DP_EDP_DISPLAY_CONTROL_REGISTER, >> + val); >> + if (ret < 0) >> + return ret; >> + >> + aux_bl->enabled = false; >> + >> + return 0; >> +} >> + >> +static int dp_aux_backlight_update_status(struct backlight_device >> *bd) >> +{ >> + struct dp_aux_backlight *aux_bl = bl_get_data(bd); >> + u16 brightness = backlight_get_brightness(bd); >> + u8 val[2] = { 0x0 }; >> + int ret = 0; >> + >> + if (brightness > 0) { >> + val[0] = brightness >> 8; >> + val[1] = brightness & 0xff; >> + ret = drm_dp_dpcd_write(aux_bl->aux, >> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, >> + val, sizeof(val)); >> + if (ret < 0) >> + return ret; >> + >> + dp_aux_backlight_enable(aux_bl); >> + } else { >> + dp_aux_backlight_disable(aux_bl); >> + } >> + >> + return 0; >> +} >> + >> +static int dp_aux_backlight_get_brightness(struct backlight_device >> *bd) >> +{ >> + struct dp_aux_backlight *aux_bl = bl_get_data(bd); >> + u8 val[2] = { 0x0 }; >> + int ret = 0; >> + >> + if (backlight_is_blank(bd)) >> + return 0; >> + >> + ret = drm_dp_dpcd_read(aux_bl->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, >> + &val, sizeof(val)); >> + if (ret < 0) >> + return ret; >> + >> + return (val[0] << 8 | val[1]); >> +} >> + >> +static const struct backlight_ops aux_bl_ops = { >> + .update_status = dp_aux_backlight_update_status, >> + .get_brightness = dp_aux_backlight_get_brightness, >> +}; >> + >> + >> +static int dp_aux_backlight_probe(struct platform_device *pdev) >> +{ >> + struct dp_aux_backlight *aux_bl; >> + struct backlight_device *bd; >> + struct backlight_properties bl_props = { 0 }; >> + struct device_node *np; >> + struct i2c_adapter *ddc; >> + int ret = 0; >> + u32 val; >> + >> + aux_bl = devm_kzalloc(&pdev->dev, sizeof(*aux_bl), GFP_KERNEL); >> + if (!aux_bl) >> + return -ENOMEM; >> + >> + aux_bl->dev = &pdev->dev; >> + >> + np = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0); >> + if (!np) { >> + dev_err(&pdev->dev, "failed to get aux ddc I2C bus\n"); >> + return -ENODEV; >> + } >> + >> + ddc = of_find_i2c_adapter_by_node(np); >> + of_node_put(np); >> + if (!ddc) >> + return -EPROBE_DEFER; >> + >> + aux_bl->aux = i2c_to_aux(ddc); >> + dev_dbg(&pdev->dev, "using dp aux %s\n", aux_bl->aux->name); >> + >> + aux_bl->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(aux_bl->enable_gpio)) { >> + ret = PTR_ERR(aux_bl->enable_gpio); >> + goto free_ddc; >> + } >> + >> + val = DP_AUX_MAX_BRIGHTNESS; >> + of_property_read_u32(pdev->dev.of_node, "max-brightness", &val); >> + if (val > DP_AUX_MAX_BRIGHTNESS) >> + val = DP_AUX_MAX_BRIGHTNESS; >> + >> + bl_props.max_brightness = val; >> + bl_props.brightness = val; >> + bl_props.type = BACKLIGHT_RAW; >> + bd = devm_backlight_device_register(&pdev->dev, >> dev_name(&pdev->dev), >> + &pdev->dev, aux_bl, >> + &aux_bl_ops, &bl_props); >> + if (IS_ERR(bd)) { >> + ret = PTR_ERR(bd); >> + dev_err(&pdev->dev, >> + "failed to register backlight (%d)\n", ret); >> + goto free_ddc; >> + } >> + >> + platform_set_drvdata(pdev, bd); >> + >> + return 0; >> + >> +free_ddc: >> + if (ddc) >> + put_device(&ddc->dev); >> + >> + return ret; >> +} >> + >> +static int dp_aux_backlight_remove(struct platform_device *pdev) >> +{ >> + struct backlight_device *bd = platform_get_drvdata(pdev); >> + struct dp_aux_backlight *aux_bl = bl_get_data(bd); >> + struct i2c_adapter *ddc = &aux_bl->aux->ddc; >> + >> + if (ddc) >> + put_device(&ddc->dev); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id dp_aux_bl_of_match_table[] = { >> + { .compatible = "dp-aux-backlight"}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table); >> + >> +static struct platform_driver dp_aux_backlight_driver = { >> + .driver = { >> + .name = "dp-aux-backlight", >> + .of_match_table = dp_aux_bl_of_match_table, >> + }, >> + .probe = dp_aux_backlight_probe, >> + .remove = dp_aux_backlight_remove, >> + >> +}; >> +module_platform_driver(dp_aux_backlight_driver); >> + >> +MODULE_DESCRIPTION("DisplayPort aux backlight driver"); >> +MODULE_LICENSE("GPL v2"); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v3 2/2] backlight: Add DisplayPort aux backlight driver 2021-04-26 9:49 ` Jani Nikula 2021-04-26 14:57 ` rajeevny @ 2021-04-30 18:48 ` Lyude Paul 1 sibling, 0 replies; 16+ messages in thread From: Lyude Paul @ 2021-04-30 18:48 UTC (permalink / raw) To: Jani Nikula, Rajeev Nandan, dri-devel, linux-arm-msm, freedreno, devicetree Cc: mkrishn, Daniel Vetter, intel-gfx, linux-kernel, abhinavk, dianders, seanpaul, Thomas Zimmermann, kalyan_t, hoegsberg, Lankhorst, Maarten JFYI for anyone who is interested, I will be respinning my patches for adding backlight helpers very soon since we've got pretty much all of the prep work for it upstream now On Mon, 2021-04-26 at 12:49 +0300, Jani Nikula wrote: > On Mon, 26 Apr 2021, Rajeev Nandan <rajeevny@codeaurora.org> wrote: > > Add backlight driver for the panels supporting backlight control > > using DPCD registers on the DisplayPort aux channel. > > No, please don't do this. > > I wrote you last week in reply to v1 why I thought merging this would > not be a good idea [1]. Why have you sent two versions since then > without replying to me, or Cc'ing me or Lyude? > > I think it's an even worse idea to merge this to > drivers/video/backlight. With DP AUX backlight you can't pretend it's > just an independent aux interface for backlight without everything else > around it. It's not independent of eDP, and exposing it as a direct > backlight sysfs interface bypasses the encoder. > > And it still remains that there is existing DP AUX backlight code in > use, in the tree, with more features than this, with plans and > previously submitted patches to lift from one driver to drm core, and > with patches to add support to another driver. > > I don't say this lightly, or very often at all, but, > > NAK. > > > BR, > Jani. > > > [1] https://lore.kernel.org/dri-devel/871rb5bcf9.fsf@intel.com/ > > > > > Changes in v2: > > - New (most of the code reused from drm_dp_aux_backlight.c of v1) > > > > Changes in v3: > > - Add missing ';' to fix module compilation (kernel test bot) > > > > Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org> > > --- > > drivers/video/backlight/Kconfig | 7 + > > drivers/video/backlight/Makefile | 1 + > > drivers/video/backlight/dp_aux_backlight.c | 245 > > +++++++++++++++++++++++++++++ > > 3 files changed, 253 insertions(+) > > create mode 100644 drivers/video/backlight/dp_aux_backlight.c > > > > diff --git a/drivers/video/backlight/Kconfig > > b/drivers/video/backlight/Kconfig > > index d83c87b..82c88f0 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -456,6 +456,13 @@ config BACKLIGHT_LED > > If you have a LCD backlight adjustable by LED class driver, say > > Y > > to enable this driver. > > > > +config BACKLIGHT_DP_AUX > > + tristate "DisplayPort aux backlight driver" > > + depends on DRM && DRM_KMS_HELPER > > + help > > + If you have a panel backlight controlled by DPCD registers > > + on the DisplayPort aux channel, say Y to enable this driver. > > + > > endif # BACKLIGHT_CLASS_DEVICE > > > > endmenu > > diff --git a/drivers/video/backlight/Makefile > > b/drivers/video/backlight/Makefile > > index 685f3f1..ba23c7c 100644 > > --- a/drivers/video/backlight/Makefile > > +++ b/drivers/video/backlight/Makefile > > @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X) += > > wm831x_bl.o > > obj-$(CONFIG_BACKLIGHT_ARCXCNN) += arcxcnn_bl.o > > obj-$(CONFIG_BACKLIGHT_RAVE_SP) += rave-sp-backlight.o > > obj-$(CONFIG_BACKLIGHT_LED) += led_bl.o > > +obj-$(CONFIG_BACKLIGHT_DP_AUX) += dp_aux_backlight.o > > diff --git a/drivers/video/backlight/dp_aux_backlight.c > > b/drivers/video/backlight/dp_aux_backlight.c > > new file mode 100644 > > index 00000000..3398383 > > --- /dev/null > > +++ b/drivers/video/backlight/dp_aux_backlight.c > > @@ -0,0 +1,245 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Backlight driver to control the brightness over DisplayPort aux > > channel. > > + */ > > + > > +#include <linux/backlight.h> > > +#include <linux/err.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <drm/drm_dp_helper.h> > > + > > +#define DP_AUX_MAX_BRIGHTNESS 0xffff > > + > > +/** > > + * struct dp_aux_backlight - DisplayPort aux backlight data > > + * @dev: pointer to our device. > > + * @aux: the DisplayPort aux channel. > > + * @enable_gpio: the backlight enable gpio. > > + * @enabled: true if backlight is enabled else false. > > + */ > > +struct dp_aux_backlight { > > + struct device *dev; > > + struct drm_dp_aux *aux; > > + struct gpio_desc *enable_gpio; > > + bool enabled; > > +}; > > + > > +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c) > > +{ > > + return container_of(i2c, struct drm_dp_aux, ddc); > > +} > > + > > +static int dp_aux_backlight_enable(struct dp_aux_backlight *aux_bl) > > +{ > > + u8 val = 0; > > + int ret; > > + > > + if (aux_bl->enabled) > > + return 0; > > + > > + /* Set backlight control mode */ > > + ret = drm_dp_dpcd_readb(aux_bl->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > + &val); > > + if (ret < 0) > > + return ret; > > + > > + val &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; > > + val |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > + ret = drm_dp_dpcd_writeb(aux_bl->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > + val); > > + if (ret < 0) > > + return ret; > > + > > + /* Enable backlight */ > > + ret = drm_dp_dpcd_readb(aux_bl->aux, > > DP_EDP_DISPLAY_CONTROL_REGISTER, > > + &val); > > + if (ret < 0) > > + return ret; > > + > > + val |= DP_EDP_BACKLIGHT_ENABLE; > > + ret = drm_dp_dpcd_writeb(aux_bl->aux, > > DP_EDP_DISPLAY_CONTROL_REGISTER, > > + val); > > + if (ret < 0) > > + return ret; > > + > > + if (aux_bl->enable_gpio) > > + gpiod_set_value(aux_bl->enable_gpio, 1); > > + > > + aux_bl->enabled = true; > > + > > + return 0; > > +} > > + > > +static int dp_aux_backlight_disable(struct dp_aux_backlight *aux_bl) > > +{ > > + u8 val = 0; > > + int ret; > > + > > + if (!aux_bl->enabled) > > + return 0; > > + > > + if (aux_bl->enable_gpio) > > + gpiod_set_value(aux_bl->enable_gpio, 0); > > + > > + ret = drm_dp_dpcd_readb(aux_bl->aux, > > DP_EDP_DISPLAY_CONTROL_REGISTER, > > + &val); > > + if (ret < 0) > > + return ret; > > + > > + val &= ~DP_EDP_BACKLIGHT_ENABLE; > > + ret = drm_dp_dpcd_writeb(aux_bl->aux, > > DP_EDP_DISPLAY_CONTROL_REGISTER, > > + val); > > + if (ret < 0) > > + return ret; > > + > > + aux_bl->enabled = false; > > + > > + return 0; > > +} > > + > > +static int dp_aux_backlight_update_status(struct backlight_device *bd) > > +{ > > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > > + u16 brightness = backlight_get_brightness(bd); > > + u8 val[2] = { 0x0 }; > > + int ret = 0; > > + > > + if (brightness > 0) { > > + val[0] = brightness >> 8; > > + val[1] = brightness & 0xff; > > + ret = drm_dp_dpcd_write(aux_bl->aux, > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > + val, sizeof(val)); > > + if (ret < 0) > > + return ret; > > + > > + dp_aux_backlight_enable(aux_bl); > > + } else { > > + dp_aux_backlight_disable(aux_bl); > > + } > > + > > + return 0; > > +} > > + > > +static int dp_aux_backlight_get_brightness(struct backlight_device *bd) > > +{ > > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > > + u8 val[2] = { 0x0 }; > > + int ret = 0; > > + > > + if (backlight_is_blank(bd)) > > + return 0; > > + > > + ret = drm_dp_dpcd_read(aux_bl->aux, > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > + &val, sizeof(val)); > > + if (ret < 0) > > + return ret; > > + > > + return (val[0] << 8 | val[1]); > > +} > > + > > +static const struct backlight_ops aux_bl_ops = { > > + .update_status = dp_aux_backlight_update_status, > > + .get_brightness = dp_aux_backlight_get_brightness, > > +}; > > + > > + > > +static int dp_aux_backlight_probe(struct platform_device *pdev) > > +{ > > + struct dp_aux_backlight *aux_bl; > > + struct backlight_device *bd; > > + struct backlight_properties bl_props = { 0 }; > > + struct device_node *np; > > + struct i2c_adapter *ddc; > > + int ret = 0; > > + u32 val; > > + > > + aux_bl = devm_kzalloc(&pdev->dev, sizeof(*aux_bl), GFP_KERNEL); > > + if (!aux_bl) > > + return -ENOMEM; > > + > > + aux_bl->dev = &pdev->dev; > > + > > + np = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0); > > + if (!np) { > > + dev_err(&pdev->dev, "failed to get aux ddc I2C bus\n"); > > + return -ENODEV; > > + } > > + > > + ddc = of_find_i2c_adapter_by_node(np); > > + of_node_put(np); > > + if (!ddc) > > + return -EPROBE_DEFER; > > + > > + aux_bl->aux = i2c_to_aux(ddc); > > + dev_dbg(&pdev->dev, "using dp aux %s\n", aux_bl->aux->name); > > + > > + aux_bl->enable_gpio = devm_gpiod_get_optional(&pdev->dev, > > "enable", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(aux_bl->enable_gpio)) { > > + ret = PTR_ERR(aux_bl->enable_gpio); > > + goto free_ddc; > > + } > > + > > + val = DP_AUX_MAX_BRIGHTNESS; > > + of_property_read_u32(pdev->dev.of_node, "max-brightness", &val); > > + if (val > DP_AUX_MAX_BRIGHTNESS) > > + val = DP_AUX_MAX_BRIGHTNESS; > > + > > + bl_props.max_brightness = val; > > + bl_props.brightness = val; > > + bl_props.type = BACKLIGHT_RAW; > > + bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev- > > >dev), > > + &pdev->dev, aux_bl, > > + &aux_bl_ops, &bl_props); > > + if (IS_ERR(bd)) { > > + ret = PTR_ERR(bd); > > + dev_err(&pdev->dev, > > + "failed to register backlight (%d)\n", ret); > > + goto free_ddc; > > + } > > + > > + platform_set_drvdata(pdev, bd); > > + > > + return 0; > > + > > +free_ddc: > > + if (ddc) > > + put_device(&ddc->dev); > > + > > + return ret; > > +} > > + > > +static int dp_aux_backlight_remove(struct platform_device *pdev) > > +{ > > + struct backlight_device *bd = platform_get_drvdata(pdev); > > + struct dp_aux_backlight *aux_bl = bl_get_data(bd); > > + struct i2c_adapter *ddc = &aux_bl->aux->ddc; > > + > > + if (ddc) > > + put_device(&ddc->dev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id dp_aux_bl_of_match_table[] = { > > + { .compatible = "dp-aux-backlight"}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table); > > + > > +static struct platform_driver dp_aux_backlight_driver = { > > + .driver = { > > + .name = "dp-aux-backlight", > > + .of_match_table = dp_aux_bl_of_match_table, > > + }, > > + .probe = dp_aux_backlight_probe, > > + .remove = dp_aux_backlight_remove, > > + > > +}; > > +module_platform_driver(dp_aux_backlight_driver); > > + > > +MODULE_DESCRIPTION("DisplayPort aux backlight driver"); > > +MODULE_LICENSE("GPL v2"); > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-05-25 8:03 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [v3 1/2] dt-bindings: backlight: add DisplayPort aux backlight Rajeev Nandan 2021-04-26 21:04 ` Rob Herring 2021-04-29 18:04 ` Rob Herring 2021-04-29 21:03 ` Doug Anderson 2021-04-30 15:10 ` rajeevny 2021-04-30 21:38 ` Doug Anderson 2021-05-11 18:11 ` rajeevny 2021-05-11 22:33 ` Laurent Pinchart 2021-05-11 23:17 ` Doug Anderson 2021-05-17 20:21 ` Doug Anderson 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 9:49 ` Jani Nikula 2021-04-26 14:57 ` rajeevny 2021-04-30 18:48 ` Lyude Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).