| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c1c76969-3ab7-a350-8988-4994e2493ff3@linaro.org>
Date: Mon, 13 Feb 2017 12:12:14 +0000
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Miłosz Rachwał <me@...ek7.pl>,
Jingoo Han <jingoohan1@...il.com>,
Lee Jones <lee.jones@...aro.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] backlight: add DDC/CI brightness driver
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@...ek7.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@...ek7.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@...ek7.pl>");
> +MODULE_DESCRIPTION("VESA DDC/CI MCCS brightness driver");
> +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists