[v2 0/2] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge

[v2 0/2] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge

[Rajeev Nandan]

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)

[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

[v2 1/2] dt-bindings: backlight: add DisplayPort aux backlight

[Rajeev Nandan]

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

[v2 2/2] backlight: Add DisplayPort aux backlight driver

[Rajeev Nandan]

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)

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..235fb42
--- /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

Re: [v2 2/2] backlight: Add DisplayPort aux backlight driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

Hi Rajeev,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.12-rc8 next-20210423]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rajeev-Nandan/drm-Add-support-for-backlight-control-of-eDP-panel-on-ti-sn65dsi86-bridge/20210424-055607
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8ecbdba09ae9869fe35ca8a17a76ef414eeb7edd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rajeev-Nandan/drm-Add-support-for-backlight-control-of-eDP-panel-on-ti-sn65dsi86-bridge/20210424-055607
        git checkout 8ecbdba09ae9869fe35ca8a17a76ef414eeb7edd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kallsyms.h:13,
                    from include/linux/ftrace.h:12,
                    from include/linux/kprobes.h:29,
                    from include/linux/kgdb.h:19,
                    from include/linux/fb.h:5,
                    from include/linux/backlight.h:13,
                    from drivers/video/backlight/dp_aux_backlight.c:6:
>> include/linux/module.h:241:1: error: expected ',' or ';' before 'extern'
     241 | extern typeof(name) __mod_##type##__##name##_device_table  \
         | ^~~~~~
   drivers/video/backlight/dp_aux_backlight.c:231:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
     231 | MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table);
         | ^~~~~~~~~~~~~~~~~~~


vim +241 include/linux/module.h

^1da177e4c3f41 Linus Torvalds    2005-04-16  237  
cff26a51da5d20 Rusty Russell     2014-02-03  238  #ifdef MODULE
cff26a51da5d20 Rusty Russell     2014-02-03  239  /* Creates an alias so file2alias.c can find device table. */
^1da177e4c3f41 Linus Torvalds    2005-04-16  240  #define MODULE_DEVICE_TABLE(type, name)					\
0bf8bf50eddc75 Matthias Kaehlcke 2017-07-24 @241  extern typeof(name) __mod_##type##__##name##_device_table		\
cff26a51da5d20 Rusty Russell     2014-02-03  242    __attribute__ ((unused, alias(__stringify(name))))
cff26a51da5d20 Rusty Russell     2014-02-03  243  #else  /* !MODULE */
cff26a51da5d20 Rusty Russell     2014-02-03  244  #define MODULE_DEVICE_TABLE(type, name)
cff26a51da5d20 Rusty Russell     2014-02-03  245  #endif
^1da177e4c3f41 Linus Torvalds    2005-04-16  246  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70130 bytes --]

Re: [v2 2/2] backlight: Add DisplayPort aux backlight driver

[Daniel Thompson]

On Sat, Apr 24, 2021 at 03:25:04AM +0530, Rajeev Nandan wrote:
> 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)

Did you respond to Jani's feedback on the v1 posting (asking you to
coordinate with Lyude's work on refactoring the i915 eDP helpers[1])?
I can't find anything showing the outcome of that.

[1]: https://www.spinics.net/lists/dri-devel/msg295602.html


> +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
> +{
> +	return container_of(i2c, struct drm_dp_aux, ddc);
> +}

[...]

> +	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);

It looks like this code "just knows" that the I2C controller it has
looked up is a special one. That seems a little odd to me.

If we "just know" this then I'd hope that is could either be modelled
in the devicetree or that it would be possible for the backlight driver
to be registered (possibly using helpers) by whatever it is that is
setting up struct drm_dp_aux in the first place.


Daniel.