* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-01-21 18:31 [PATCH] backlight: add DDC/CI brightness driver Miłosz Rachwał
@ 2017-02-08 12:52 ` Lee Jones
2017-02-08 12:53 ` Lee Jones
2017-02-13 12:12 ` Daniel Thompson
2 siblings, 0 replies; 7+ messages in thread
From: Lee Jones @ 2017-02-08 12:52 UTC (permalink / raw)
To: Miłosz Rachwał; +Cc: Jingoo Han, linux-kernel
Cc'ing Daniel Thompson, the new Maintainer.
On Sat, 21 Jan 2017, Miłosz Rachwał wrote:
> This patch contains driver for exposing VESA DDC/CI MCCS brightness
> settings via kernel backlight API. This allows to control monitor
> brightness using standard software which uses /sys/class/backlight/
> interface, same as for laptop backlight drivers.
> Because some monitors don't correctly implement DDC/CI standard module
> parameter allows to override maximum brightness value reported by
> monitor.
> This module doesn't support autodetection, so driver must be manually
> instantiated on monitor I²C bus under 0x37 address.
>
> Signed-off-by: Miłosz Rachwał <me@milek7.pl>
> ---
> drivers/video/backlight/Kconfig | 7 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/ddcci_bl.c | 128 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/video/backlight/ddcci_bl.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5ffa4b4e..cce62832 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -460,6 +460,13 @@ config BACKLIGHT_BD6107
> help
> If you have a Rohm BD6107 say Y to enable the backlight driver.
>
> +config BACKLIGHT_DDCCI
> + tristate "VESA DDC/CI MCCS brightness driver"
> + depends on BACKLIGHT_CLASS_DEVICE && I2C
> + default m
> + help
> + This supports brightness control via VESA DDC/CI MCCS.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 16ec534c..2e5352aa 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
> obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
> obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
> +obj-$(CONFIG_BACKLIGHT_DDCCI) += ddcci_bl.o
> obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
> obj-$(CONFIG_BACKLIGHT_GENERIC) += generic_bl.o
> obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o
> diff --git a/drivers/video/backlight/ddcci_bl.c b/drivers/video/backlight/ddcci_bl.c
> new file mode 100644
> index 00000000..c314cbc8
> --- /dev/null
> +++ b/drivers/video/backlight/ddcci_bl.c
> @@ -0,0 +1,128 @@
> +/*
> + * VESA DDC/CI MCCS brightness driver
> + *
> + * Copyright (C) 2017 Miłosz Rachwał <me@milek7.pl>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +
> +static int maxbr;
> +module_param(maxbr, int, 0644);
> +MODULE_PARM_DESC(maxbr, "Override maximum brightness value specified by monitor");
> +
> +static int ddcci_update_status(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + char buf[] = { 0x51, 0x84, 0x03, 0x10, 0x00, 0x00, 0x00 };
> + buf[4] = bl->props.brightness >> 8;
> + buf[5] = bl->props.brightness;
> + buf[6] = (client->addr << 1) ^ 0xC6 ^ buf[4] ^ buf[5];
> +
> + i2c_master_send(client, buf, 7);
> + msleep(50);
> +
> + return 0;
> +}
> +
> +static int ddcci_read(struct i2c_client *client, struct backlight_properties *props, int init)
> +{
> + int i;
> + char xor;
> +
> + char buf[11] = { 0x51, 0x82, 0x01, 0x10, 0x00 };
> + buf[4] = (client->addr << 1) ^ 0xC2;
> +
> + i2c_master_send(client, buf, 5);
> + msleep(40);
> + i2c_master_recv(client, buf, 11);
> +
> + if (buf[3] != 0)
> + goto fail;
> +
> + xor = 0x50;
> + for (i = 0; i < 11; i++)
> + xor ^= buf[i];
> +
> + if (xor)
> + goto fail;
> +
> + if (init) {
> + if (maxbr)
> + props->max_brightness = maxbr;
> + else
> + props->max_brightness = (buf[6] << 8) | buf[7];
> + }
> + props->brightness = (buf[8] << 8) | buf[9];
> +
> + return 0;
> +
> +fail:
> + dev_err(&client->dev, "failed to read brightness");
> + return -1;
> +}
> +
> +static int ddcci_get_brightness(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + ddcci_read(client, &bl->props, 0);
> + return bl->props.brightness;
> +}
> +
> +static const struct backlight_ops ddcci_ops = {
> + .update_status = ddcci_update_status,
> + .get_brightness = ddcci_get_brightness
> +};
> +
> +static int ddcci_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct backlight_properties props;
> + int retry;
> + char name[20];
> +
> + memset(&props, 0, sizeof(struct backlight_properties));
> + props.type = BACKLIGHT_FIRMWARE;
> + snprintf(name, 20, "ddcci_%s", dev_name(&client->dev));
> +
> + for (retry = 0; retry < 3; retry++) {
> + if (ddcci_read(client, &props, 1) >= 0) {
> + devm_backlight_device_register(&client->dev, name, &client->dev, client, &ddcci_ops, &props);
> + return 0;
> + }
> + }
> +
> + return -1;
> +}
> +
> +static int ddcci_remove(struct i2c_client *client)
> +{
> + return 0;
> +}
> +
> +static struct i2c_device_id ddcci_idtable[] = {
> + { "ddcci_bl", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ddcci_idtable);
> +
> +static struct i2c_driver ddcci_driver = {
> + .driver = { .name = "ddcci_bl" },
> + .id_table = ddcci_idtable,
> + .probe = ddcci_probe,
> + .remove = ddcci_remove
> +};
> +module_i2c_driver(ddcci_driver);
> +
> +MODULE_AUTHOR("Miłosz Rachwał <me@milek7.pl>");
> +MODULE_DESCRIPTION("VESA DDC/CI MCCS brightness driver");
> +MODULE_LICENSE("GPL");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-01-21 18:31 [PATCH] backlight: add DDC/CI brightness driver Miłosz Rachwał
2017-02-08 12:52 ` Lee Jones
@ 2017-02-08 12:53 ` Lee Jones
2017-02-13 12:12 ` Daniel Thompson
2 siblings, 0 replies; 7+ messages in thread
From: Lee Jones @ 2017-02-08 12:53 UTC (permalink / raw)
To: Miłosz Rachwał; +Cc: Jingoo Han, linux-kernel, daniel.thompson
Cc'ing Daniel Thompson, the new Maintainer.
On Sat, 21 Jan 2017, Miłosz Rachwał wrote:
> This patch contains driver for exposing VESA DDC/CI MCCS brightness
> settings via kernel backlight API. This allows to control monitor
> brightness using standard software which uses /sys/class/backlight/
> interface, same as for laptop backlight drivers.
> Because some monitors don't correctly implement DDC/CI standard module
> parameter allows to override maximum brightness value reported by
> monitor.
> This module doesn't support autodetection, so driver must be manually
> instantiated on monitor I²C bus under 0x37 address.
>
> Signed-off-by: Miłosz Rachwał <me@milek7.pl>
> ---
> drivers/video/backlight/Kconfig | 7 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/ddcci_bl.c | 128 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/video/backlight/ddcci_bl.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5ffa4b4e..cce62832 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -460,6 +460,13 @@ config BACKLIGHT_BD6107
> help
> If you have a Rohm BD6107 say Y to enable the backlight driver.
>
> +config BACKLIGHT_DDCCI
> + tristate "VESA DDC/CI MCCS brightness driver"
> + depends on BACKLIGHT_CLASS_DEVICE && I2C
> + default m
> + help
> + This supports brightness control via VESA DDC/CI MCCS.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 16ec534c..2e5352aa 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
> obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
> obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
> +obj-$(CONFIG_BACKLIGHT_DDCCI) += ddcci_bl.o
> obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
> obj-$(CONFIG_BACKLIGHT_GENERIC) += generic_bl.o
> obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o
> diff --git a/drivers/video/backlight/ddcci_bl.c b/drivers/video/backlight/ddcci_bl.c
> new file mode 100644
> index 00000000..c314cbc8
> --- /dev/null
> +++ b/drivers/video/backlight/ddcci_bl.c
> @@ -0,0 +1,128 @@
> +/*
> + * VESA DDC/CI MCCS brightness driver
> + *
> + * Copyright (C) 2017 Miłosz Rachwał <me@milek7.pl>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +
> +static int maxbr;
> +module_param(maxbr, int, 0644);
> +MODULE_PARM_DESC(maxbr, "Override maximum brightness value specified by monitor");
> +
> +static int ddcci_update_status(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + char buf[] = { 0x51, 0x84, 0x03, 0x10, 0x00, 0x00, 0x00 };
> + buf[4] = bl->props.brightness >> 8;
> + buf[5] = bl->props.brightness;
> + buf[6] = (client->addr << 1) ^ 0xC6 ^ buf[4] ^ buf[5];
> +
> + i2c_master_send(client, buf, 7);
> + msleep(50);
> +
> + return 0;
> +}
> +
> +static int ddcci_read(struct i2c_client *client, struct backlight_properties *props, int init)
> +{
> + int i;
> + char xor;
> +
> + char buf[11] = { 0x51, 0x82, 0x01, 0x10, 0x00 };
> + buf[4] = (client->addr << 1) ^ 0xC2;
> +
> + i2c_master_send(client, buf, 5);
> + msleep(40);
> + i2c_master_recv(client, buf, 11);
> +
> + if (buf[3] != 0)
> + goto fail;
> +
> + xor = 0x50;
> + for (i = 0; i < 11; i++)
> + xor ^= buf[i];
> +
> + if (xor)
> + goto fail;
> +
> + if (init) {
> + if (maxbr)
> + props->max_brightness = maxbr;
> + else
> + props->max_brightness = (buf[6] << 8) | buf[7];
> + }
> + props->brightness = (buf[8] << 8) | buf[9];
> +
> + return 0;
> +
> +fail:
> + dev_err(&client->dev, "failed to read brightness");
> + return -1;
> +}
> +
> +static int ddcci_get_brightness(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + ddcci_read(client, &bl->props, 0);
> + return bl->props.brightness;
> +}
> +
> +static const struct backlight_ops ddcci_ops = {
> + .update_status = ddcci_update_status,
> + .get_brightness = ddcci_get_brightness
> +};
> +
> +static int ddcci_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct backlight_properties props;
> + int retry;
> + char name[20];
> +
> + memset(&props, 0, sizeof(struct backlight_properties));
> + props.type = BACKLIGHT_FIRMWARE;
> + snprintf(name, 20, "ddcci_%s", dev_name(&client->dev));
> +
> + for (retry = 0; retry < 3; retry++) {
> + if (ddcci_read(client, &props, 1) >= 0) {
> + devm_backlight_device_register(&client->dev, name, &client->dev, client, &ddcci_ops, &props);
> + return 0;
> + }
> + }
> +
> + return -1;
> +}
> +
> +static int ddcci_remove(struct i2c_client *client)
> +{
> + return 0;
> +}
> +
> +static struct i2c_device_id ddcci_idtable[] = {
> + { "ddcci_bl", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ddcci_idtable);
> +
> +static struct i2c_driver ddcci_driver = {
> + .driver = { .name = "ddcci_bl" },
> + .id_table = ddcci_idtable,
> + .probe = ddcci_probe,
> + .remove = ddcci_remove
> +};
> +module_i2c_driver(ddcci_driver);
> +
> +MODULE_AUTHOR("Miłosz Rachwał <me@milek7.pl>");
> +MODULE_DESCRIPTION("VESA DDC/CI MCCS brightness driver");
> +MODULE_LICENSE("GPL");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-01-21 18:31 [PATCH] backlight: add DDC/CI brightness driver Miłosz Rachwał
2017-02-08 12:52 ` Lee Jones
2017-02-08 12:53 ` Lee Jones
@ 2017-02-13 12:12 ` Daniel Thompson
2017-08-10 12:56 ` Miłosz Rachwał
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Thompson @ 2017-02-13 12:12 UTC (permalink / raw)
To: Miłosz Rachwał, Jingoo Han, Lee Jones; +Cc: linux-kernel
On 21/01/17 18:31, Miłosz Rachwał wrote:
> This patch contains driver for exposing VESA DDC/CI MCCS brightness
> settings via kernel backlight API. This allows to control monitor
> brightness using standard software which uses /sys/class/backlight/
> interface, same as for laptop backlight drivers.
> Because some monitors don't correctly implement DDC/CI standard module
> parameter allows to override maximum brightness value reported by
> monitor.
> This module doesn't support autodetection, so driver must be manually
> instantiated on monitor I²C bus under 0x37 address.
This driver is well written and very clear but I'm afraid I have a few
high level concerns about this driver.
Firstly, have you discussed this driver with the DRM developers? DDC/CI
is closely related to EDID (monitor usually implemented both, same I2C
bus, etc). It seems odd for this driver to be standalone rather than
integrated into the DRM connector model (allowing it to probe
automatically).
Secondly, it assumes the Luminance VCD is the best way to control the
backlight (ignoring both the current and legacy backlight controls). I'm
not totally sure that falling back to luminance is sensible for a
backlight control but it is certainly wrong to go directly to it,
ignoring the other VCPs.
Finally, I'm also not sure about they way the maxbr module parameter
works because it makes a per-device configuration setting and applies it
system-wide.
Daniel.
> Signed-off-by: Miłosz Rachwał <me@milek7.pl>
> ---
> drivers/video/backlight/Kconfig | 7 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/ddcci_bl.c | 128 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/video/backlight/ddcci_bl.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5ffa4b4e..cce62832 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -460,6 +460,13 @@ config BACKLIGHT_BD6107
> help
> If you have a Rohm BD6107 say Y to enable the backlight driver.
>
> +config BACKLIGHT_DDCCI
> + tristate "VESA DDC/CI MCCS brightness driver"
> + depends on BACKLIGHT_CLASS_DEVICE && I2C
> + default m
> + help
> + This supports brightness control via VESA DDC/CI MCCS.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 16ec534c..2e5352aa 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
> obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
> obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
> +obj-$(CONFIG_BACKLIGHT_DDCCI) += ddcci_bl.o
> obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
> obj-$(CONFIG_BACKLIGHT_GENERIC) += generic_bl.o
> obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o
> diff --git a/drivers/video/backlight/ddcci_bl.c b/drivers/video/backlight/ddcci_bl.c
> new file mode 100644
> index 00000000..c314cbc8
> --- /dev/null
> +++ b/drivers/video/backlight/ddcci_bl.c
> @@ -0,0 +1,128 @@
> +/*
> + * VESA DDC/CI MCCS brightness driver
> + *
> + * Copyright (C) 2017 Miłosz Rachwał <me@milek7.pl>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +
> +static int maxbr;
> +module_param(maxbr, int, 0644);
> +MODULE_PARM_DESC(maxbr, "Override maximum brightness value specified by monitor");
Hmnnn... module parameters are
> +
> +static int ddcci_update_status(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + char buf[] = { 0x51, 0x84, 0x03, 0x10, 0x00, 0x00, 0x00 };
> + buf[4] = bl->props.brightness >> 8;
> + buf[5] = bl->props.brightness;
> + buf[6] = (client->addr << 1) ^ 0xC6 ^ buf[4] ^ buf[5];
> +
> + i2c_master_send(client, buf, 7);
> + msleep(50);
> +
> + return 0;
> +}
> +
> +static int ddcci_read(struct i2c_client *client, struct backlight_properties *props, int init)
> +{
> + int i;
> + char xor;
> +
> + char buf[11] = { 0x51, 0x82, 0x01, 0x10, 0x00 };
> + buf[4] = (client->addr << 1) ^ 0xC2;
> +
> + i2c_master_send(client, buf, 5);
> + msleep(40);
> + i2c_master_recv(client, buf, 11);
> +
> + if (buf[3] != 0)
> + goto fail;
> +
> + xor = 0x50;
> + for (i = 0; i < 11; i++)
> + xor ^= buf[i];
> +
> + if (xor)
> + goto fail;
> +
> + if (init) {
> + if (maxbr)
> + props->max_brightness = maxbr;
> + else
> + props->max_brightness = (buf[6] << 8) | buf[7];
> + }
> + props->brightness = (buf[8] << 8) | buf[9];
> +
> + return 0;
> +
> +fail:
> + dev_err(&client->dev, "failed to read brightness");
> + return -1;
> +}
> +
> +static int ddcci_get_brightness(struct backlight_device *bl)
> +{
> + struct i2c_client *client = bl_get_data(bl);
> +
> + ddcci_read(client, &bl->props, 0);
> + return bl->props.brightness;
> +}
> +
> +static const struct backlight_ops ddcci_ops = {
> + .update_status = ddcci_update_status,
> + .get_brightness = ddcci_get_brightness
> +};
> +
> +static int ddcci_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct backlight_properties props;
> + int retry;
> + char name[20];
> +
> + memset(&props, 0, sizeof(struct backlight_properties));
> + props.type = BACKLIGHT_FIRMWARE;
> + snprintf(name, 20, "ddcci_%s", dev_name(&client->dev));
> +
> + for (retry = 0; retry < 3; retry++) {
> + if (ddcci_read(client, &props, 1) >= 0) {
> + devm_backlight_device_register(&client->dev, name, &client->dev, client, &ddcci_ops, &props);
> + return 0;
> + }
> + }
> +
> + return -1;
> +}
> +
> +static int ddcci_remove(struct i2c_client *client)
> +{
> + return 0;
> +}
> +
> +static struct i2c_device_id ddcci_idtable[] = {
> + { "ddcci_bl", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ddcci_idtable);
> +
> +static struct i2c_driver ddcci_driver = {
> + .driver = { .name = "ddcci_bl" },
> + .id_table = ddcci_idtable,
> + .probe = ddcci_probe,
> + .remove = ddcci_remove
> +};
> +module_i2c_driver(ddcci_driver);
> +
> +MODULE_AUTHOR("Miłosz Rachwał <me@milek7.pl>");
> +MODULE_DESCRIPTION("VESA DDC/CI MCCS brightness driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-02-13 12:12 ` Daniel Thompson
@ 2017-08-10 12:56 ` Miłosz Rachwał
2017-08-10 17:05 ` Linus Torvalds
2017-08-14 9:48 ` Daniel Thompson
0 siblings, 2 replies; 7+ messages in thread
From: Miłosz Rachwał @ 2017-08-10 12:56 UTC (permalink / raw)
To: Daniel Thompson, Jingoo Han, Lee Jones; +Cc: linux-kernel
On 13.02.2017 13:12, Daniel Thompson wrote:
> On 21/01/17 18:31, Miłosz Rachwał wrote:
>> This patch contains driver for exposing VESA DDC/CI MCCS brightness
>> settings via kernel backlight API. This allows to control monitor
>> brightness using standard software which uses /sys/class/backlight/
>> interface, same as for laptop backlight drivers.
>> Because some monitors don't correctly implement DDC/CI standard module
>> parameter allows to override maximum brightness value reported by
>> monitor.
>> This module doesn't support autodetection, so driver must be manually
>> instantiated on monitor I²C bus under 0x37 address.
>
> This driver is well written and very clear but I'm afraid I have a few high level concerns about this driver.
>
> Firstly, have you discussed this driver with the DRM developers? DDC/CI is closely related to EDID (monitor usually implemented both, same I2C bus, etc). It seems odd for this driver to be standalone rather than integrated into the DRM connector model (allowing it to probe automatically).
Yes, it should be somehow integrated into DRM EDID probing, but I haven't
looked into that code yet. But I think that this driver should stay mostly
unchanged, and DRM code should only do probing of this driver and pass
fb_info* so that it is possible to check relation of framebuffer and
backlight device via backlight_ops.check_fb, yes?
Or whole driver should be moved into DRM code?
> Secondly, it assumes the Luminance VCD is the best way to control the backlight (ignoring both the current and legacy backlight controls). I'm not totally sure that falling back to luminance is sensible for a backlight control but it is certainly wrong to go directly to it, ignoring the other VCPs.
Oops, I used VCD that worked on my monitor and forgotten about the others.
It is probably necessary to query and parse capability string to get
supported VCP codes.
But anyway in MCCS specifiaction the only other code that seems revelant
is 0x6B "backlight level: white", what are other "current and legacy backlight controls"?
> Finally, I'm also not sure about they way the maxbr module parameter works because it makes a per-device configuration setting and applies it system-wide.
maxbr is written to backlight_properties.max_brightness during probing,
so it could be changed between device instantiations and each instance
would get their own max_brightness value. Ugly solution, but I don't had
any other idea as i2c probe function doesn't have any extra arguments.
Though while that approach was doable with driver instantiatiated by
userspace sysfs interface it won't be with automatic probing by DRM.
I think alternative solution is passing quirk list with values for specific
monitor models, or maybe just hardcoded quirk list.
>
>
> Daniel.
>
>
>> Signed-off-by: Miłosz Rachwał <me@milek7.pl>
>> ---
>> drivers/video/backlight/Kconfig | 7 ++
>> drivers/video/backlight/Makefile | 1 +
>> drivers/video/backlight/ddcci_bl.c | 128 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 136 insertions(+)
>> create mode 100644 drivers/video/backlight/ddcci_bl.c
>>
>> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
>> index 5ffa4b4e..cce62832 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
>> @@ -460,6 +460,13 @@ config BACKLIGHT_BD6107
>> help
>> If you have a Rohm BD6107 say Y to enable the backlight driver.
>>
>> +config BACKLIGHT_DDCCI
>> + tristate "VESA DDC/CI MCCS brightness driver"
>> + depends on BACKLIGHT_CLASS_DEVICE && I2C
>> + default m
>> + help
>> + This supports brightness control via VESA DDC/CI MCCS.
>> +
>> endif # BACKLIGHT_CLASS_DEVICE
>>
>> endif # BACKLIGHT_LCD_SUPPORT
>> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
>> index 16ec534c..2e5352aa 100644
>> --- a/drivers/video/backlight/Makefile
>> +++ b/drivers/video/backlight/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
>> obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
>> obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
>> obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
>> +obj-$(CONFIG_BACKLIGHT_DDCCI) += ddcci_bl.o
>> obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
>> obj-$(CONFIG_BACKLIGHT_GENERIC) += generic_bl.o
>> obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o
>> diff --git a/drivers/video/backlight/ddcci_bl.c b/drivers/video/backlight/ddcci_bl.c
>> new file mode 100644
>> index 00000000..c314cbc8
>> --- /dev/null
>> +++ b/drivers/video/backlight/ddcci_bl.c
>> @@ -0,0 +1,128 @@
>> +/*
>> + * VESA DDC/CI MCCS brightness driver
>> + *
>> + * Copyright (C) 2017 Miłosz Rachwał <me@milek7.pl>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +
>> +static int maxbr;
>> +module_param(maxbr, int, 0644);
>> +MODULE_PARM_DESC(maxbr, "Override maximum brightness value specified by monitor");
>
> Hmnnn... module parameters are
>
>> +
>> +static int ddcci_update_status(struct backlight_device *bl)
>> +{
>> + struct i2c_client *client = bl_get_data(bl);
>> +
>> + char buf[] = { 0x51, 0x84, 0x03, 0x10, 0x00, 0x00, 0x00 };
>> + buf[4] = bl->props.brightness >> 8;
>> + buf[5] = bl->props.brightness;
>> + buf[6] = (client->addr << 1) ^ 0xC6 ^ buf[4] ^ buf[5];
>> +
>> + i2c_master_send(client, buf, 7);
>> + msleep(50);
>> +
>> + return 0;
>> +}
>> +
>> +static int ddcci_read(struct i2c_client *client, struct backlight_properties *props, int init)
>> +{
>> + int i;
>> + char xor;
>> +
>> + char buf[11] = { 0x51, 0x82, 0x01, 0x10, 0x00 };
>> + buf[4] = (client->addr << 1) ^ 0xC2;
>> +
>> + i2c_master_send(client, buf, 5);
>> + msleep(40);
>> + i2c_master_recv(client, buf, 11);
>> +
>> + if (buf[3] != 0)
>> + goto fail;
>> +
>> + xor = 0x50;
>> + for (i = 0; i < 11; i++)
>> + xor ^= buf[i];
>> +
>> + if (xor)
>> + goto fail;
>> +
>> + if (init) {
>> + if (maxbr)
>> + props->max_brightness = maxbr;
>> + else
>> + props->max_brightness = (buf[6] << 8) | buf[7];
>> + }
>> + props->brightness = (buf[8] << 8) | buf[9];
>> +
>> + return 0;
>> +
>> +fail:
>> + dev_err(&client->dev, "failed to read brightness");
>> + return -1;
>> +}
>> +
>> +static int ddcci_get_brightness(struct backlight_device *bl)
>> +{
>> + struct i2c_client *client = bl_get_data(bl);
>> +
>> + ddcci_read(client, &bl->props, 0);
>> + return bl->props.brightness;
>> +}
>> +
>> +static const struct backlight_ops ddcci_ops = {
>> + .update_status = ddcci_update_status,
>> + .get_brightness = ddcci_get_brightness
>> +};
>> +
>> +static int ddcci_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> + struct backlight_properties props;
>> + int retry;
>> + char name[20];
>> +
>> + memset(&props, 0, sizeof(struct backlight_properties));
>> + props.type = BACKLIGHT_FIRMWARE;
>> + snprintf(name, 20, "ddcci_%s", dev_name(&client->dev));
>> +
>> + for (retry = 0; retry < 3; retry++) {
>> + if (ddcci_read(client, &props, 1) >= 0) {
>> + devm_backlight_device_register(&client->dev, name, &client->dev, client, &ddcci_ops, &props);
>> + return 0;
>> + }
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +static int ddcci_remove(struct i2c_client *client)
>> +{
>> + return 0;
>> +}
>> +
>> +static struct i2c_device_id ddcci_idtable[] = {
>> + { "ddcci_bl", 0 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ddcci_idtable);
>> +
>> +static struct i2c_driver ddcci_driver = {
>> + .driver = { .name = "ddcci_bl" },
>> + .id_table = ddcci_idtable,
>> + .probe = ddcci_probe,
>> + .remove = ddcci_remove
>> +};
>> +module_i2c_driver(ddcci_driver);
>> +
>> +MODULE_AUTHOR("Miłosz Rachwał <me@milek7.pl>");
>> +MODULE_DESCRIPTION("VESA DDC/CI MCCS brightness driver");
>> +MODULE_LICENSE("GPL");
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-08-10 12:56 ` Miłosz Rachwał
@ 2017-08-10 17:05 ` Linus Torvalds
2017-08-14 9:48 ` Daniel Thompson
1 sibling, 0 replies; 7+ messages in thread
From: Linus Torvalds @ 2017-08-10 17:05 UTC (permalink / raw)
To: Miłosz Rachwał; +Cc: Daniel Thompson, Jingoo Han, Lee Jones, lkml
[ Leaving the cc's in, just in a futile hope that others will catch
things like this too ]
Miłosz, do you control the DKIM setup on "milek7.pl"?
Because right now your emails get marked as spam when you go through
the kernel mailing list.
Why? The kernel mailing list ends up rewriting white-space in the
email headers, and you have
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=milek7.pl;
s=dkim; ....
where that "simple/simple" means that *any* change - even whitespace -
will make DKIM fail.
And quite frankly, "c=simple/simple" is just wrong. SMTP very much was
never designed to care about whitespace in headers. The DKIM people
were crazy to even allow and to codify that broken garbage.
Yes, yes, the kernel mailing list should also be fixed to not do the
header rewriting, but it is very fundamental to how it works (it
simply doesn't save the headers as a collection of raw lines at all,
it saves them as the cleaned-up thing). So it would involve switching
everything around, and Davem just doesn't have the time and energy.
And "c=simple/simple" really is bogus.
So if at all possible, make the use milek7.pl DKIM setup use
"c=relaxed/relaxed", and LKML will be happy again, and your emails
don't get marked as spam..
Linus
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] backlight: add DDC/CI brightness driver
2017-08-10 12:56 ` Miłosz Rachwał
2017-08-10 17:05 ` Linus Torvalds
@ 2017-08-14 9:48 ` Daniel Thompson
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Thompson @ 2017-08-14 9:48 UTC (permalink / raw)
To: Miłosz Rachwał, Jingoo Han, Lee Jones; +Cc: linux-kernel
On 10/08/17 13:56, Miłosz Rachwał wrote:
> On 13.02.2017 13:12, Daniel Thompson wrote:
>> On 21/01/17 18:31, Miłosz Rachwał wrote:
>>> This patch contains driver for exposing VESA DDC/CI MCCS brightness
>>> settings via kernel backlight API. This allows to control monitor
>>> brightness using standard software which uses /sys/class/backlight/
>>> interface, same as for laptop backlight drivers.
>>> Because some monitors don't correctly implement DDC/CI standard module
>>> parameter allows to override maximum brightness value reported by
>>> monitor.
>>> This module doesn't support autodetection, so driver must be manually
>>> instantiated on monitor I²C bus under 0x37 address.
>>
>> This driver is well written and very clear but I'm afraid I have a
>> few high level concerns about this driver.
>>
>> Firstly, have you discussed this driver with the DRM developers?
>> DDC/CI is closely related to EDID (monitor usually implemented
>> both, same I2C bus, etc). It seems odd for this driver to be
>> standalone rather than integrated into the DRM connector model
>> (allowing it to probe automatically).
>
> Yes, it should be somehow integrated into DRM EDID probing, but I
> haven't looked into that code yet. But I think that this driver
> should stay mostly unchanged, and DRM code should only do probing of
> this driver and pass fb_info* so that it is possible to check
> relation of framebuffer and backlight device via
> backlight_ops.check_fb, yes? Or whole driver should be moved into DRM
> code?
To be honest I was hoping you'd be able to point at discussion with the
DRM developers!
My gut reaction is that this code is best implemented within the DRM
code. I could probably be lobbied to change this view but not without
discussion involving the DRM folks.
>> Secondly, it assumes the Luminance VCD is the best way to control
>> the backlight (ignoring both the current and legacy backlight
>> controls). I'm not totally sure that falling back to luminance is
>> sensible for a backlight control but it is certainly wrong to go
>> directly to it, ignoring the other VCPs.
>
> Oops, I used VCD that worked on my monitor and forgotten about the
> others. It is probably necessary to query and parse capability string
> to get supported VCP codes. But anyway in MCCS specifiaction the only
> other code that seems revelant is 0x6B "backlight level: white", what
> are other "current and legacy backlight controls"?
0x6B is the current one and 0x13 "backlight control (legacy)" is the
legacy one.
>> Finally, I'm also not sure about they way the maxbr module
>> parameter works because it makes a per-device configuration setting
>> and applies it system-wide.
>
> maxbr is written to backlight_properties.max_brightness during
> probing, so it could be changed between device instantiations and
> each instance would get their own max_brightness value. Ugly
> solution, but I don't had any other idea as i2c probe function
> doesn't have any extra arguments. Though while that approach was
> doable with driver instantiatiated by userspace sysfs interface it
> won't be with automatic probing by DRM. I think alternative solution
> is passing quirk list with values for specific monitor models, or
> maybe just hardcoded quirk list.
My take is that the MCCS exhibits most of the common backlight problems
that make it hard to hook up to a GUI (does 0 turn off the backlight
completely or is it just the least bright settings? is the scale linear
or logarithmic?). In fairness the current sysfs interfaces for the
backlight system also exhibit this problems ;-).
Perhaps shelve this until we know whether the code can be properly
integrated with DRM (IIUC that would also allow individual properties to
exist).
Daniel.
^ permalink raw reply [flat|nested] 7+ messages in thread