| Message ID | 1619416756-3533-3-git-send-email-rajeevny@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge | expand |
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");
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");
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"); >
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");
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