[PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
programmable switching frequency to optimize efficiency.
The brightness can be controlled either by I2C commands (called "analog"
mode) or by a PWM input signal (PWM mode).
This driver supports both modes.

For device driver details, please refer to:
- drivers/video/backlight/mp3309c_bl.c

The datasheet is available at:
- https://www.monolithicpower.com/en/mp3309c.html

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
Reviewed-by: Rob Herring <robh@kernel.org>
---

v3:
 - add default value for mps,overvoltage-protection-microvolt property
 - fix the example, changing from "mps,mp3309c-backlight" to "mps,mp3309c" in
   compatible property
v2:
 - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0,
   switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms,
   reset-on-length-ms)
 - add common.yaml#
 - remove already included properties (default-brightness, max-brightness)
 - substitute three boolean properties, used for the overvoltage-protection
   values, with a single enum property
 - remove some conditional definitions
 - remove the 2nd example
v1:
 - first version

 .../bindings/leds/backlight/mps,mp3309c.yaml  | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml

diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
new file mode 100644
index 000000000000..4191e33626f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MPS MP3309C backlight
+
+maintainers:
+  - Flavio Suligoi <f.suligoi@asem.it>
+
+description: |
+  The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
+  programmable switching frequency to optimize efficiency.
+  It supports two different dimming modes:
+
+  - analog mode, via I2C commands (default)
+  - PWM controlled mode.
+
+  The datasheet is available at:
+  https://www.monolithicpower.com/en/mp3309c.html
+
+allOf:
+  - $ref: common.yaml#
+
+properties:
+  compatible:
+    const: mps,mp3309c
+
+  reg:
+    maxItems: 1
+
+  pwms:
+    description: if present, the backlight is controlled in PWM mode.
+    maxItems: 1
+
+  enable-gpios:
+    description: GPIO used to enable the backlight in "analog-i2c" dimming mode.
+    maxItems: 1
+
+  mps,overvoltage-protection-microvolt:
+    description: Overvoltage protection (13.5V, 24V or 35.5V).
+    enum: [ 13500000, 24000000, 35500000 ]
+    default: 35500000
+
+  mps,no-sync-mode:
+    description: disable synchronous rectification mode
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - max-brightness
+  - default-brightness
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* Backlight with PWM control */
+        backlight_pwm: backlight@17 {
+            compatible = "mps,mp3309c";
+            reg = <0x17>;
+            pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
+            max-brightness = <100>;
+            default-brightness = <80>;
+            mps,overvoltage-protection-microvolt = <24000000>;
+        };
+    };
-- 
2.34.1

[PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C

[Flavio Suligoi]

The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
programmable switching frequency to optimize efficiency.
The brightness can be controlled either by I2C commands (called "analog"
mode) or by a PWM input signal (PWM mode).
This driver supports both modes.

For DT configuration details, please refer to:
- Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml

The datasheet is available at:
- https://www.monolithicpower.com/en/mp3309c.html

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
---

v3:
 - fix SPDX obsolete spelling
 - in mp3309c_bl_update_status, change from msleep_interruptible() to msleep()
   and improve the related comment
v2:
 - fix dependecies in Kconfig
 - fix Kconfig MP3309C entry order
 - remove switch-on-delay-ms property
 - remove optional gpio property to reset external devices
 - remove dimming-mode property (the analog-i2c dimming mode is the default; the
   presence of the pwms property, in DT, selects automatically the pwm dimming
   mode)
 - substitute three boolean properties, used for the overvoltage-protection
   values, with a single enum property
 - drop simple tracing messages
 - use dev_err_probe() in probe function
 - change device name from mp3309c_bl to the simple mp3309c
 - remove shutdown function
v1:
 - first version

 MAINTAINERS                       |   6 +
 drivers/video/backlight/Kconfig   |  11 +
 drivers/video/backlight/Makefile  |   1 +
 drivers/video/backlight/mp3309c.c | 398 ++++++++++++++++++++++++++++++
 4 files changed, 416 insertions(+)
 create mode 100644 drivers/video/backlight/mp3309c.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3be1bdfe8ecc..f779df433af1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14333,6 +14333,12 @@ S:	Maintained
 F:	Documentation/driver-api/tty/moxa-smartio.rst
 F:	drivers/tty/mxser.*
 
+MP3309C BACKLIGHT DRIVER
+M:	Flavio Suligoi <f.suligoi@asem.it>
+S:	Maintained
+F:	Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
+F:	drivers/video/backlight/mp3309c.c
+
 MR800 AVERMEDIA USB FM RADIO DRIVER
 M:	Alexey Klimov <klimov.linux@gmail.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 51387b1ef012..1144a54a35c0 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -402,6 +402,17 @@ config BACKLIGHT_LP8788
 	help
 	  This supports TI LP8788 backlight driver.
 
+config BACKLIGHT_MP3309C
+	tristate "Backlight Driver for MPS MP3309C"
+	depends on I2C && PWM
+	select REGMAP_I2C
+	help
+	  This supports MPS MP3309C backlight WLED driver in both PWM and
+	  analog/I2C dimming modes.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called mp3309c.
+
 config BACKLIGHT_PANDORA
 	tristate "Backlight driver for Pandora console"
 	depends on TWL4030_CORE
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index f72e1c3c59e9..1af583de665b 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_BACKLIGHT_LP855X)		+= lp855x_bl.o
 obj-$(CONFIG_BACKLIGHT_LP8788)		+= lp8788_bl.o
 obj-$(CONFIG_BACKLIGHT_LV5207LP)	+= lv5207lp.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)		+= max8925_bl.o
+obj-$(CONFIG_BACKLIGHT_MP3309C)		+= mp3309c.o
 obj-$(CONFIG_BACKLIGHT_MT6370)		+= mt6370-backlight.o
 obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c
new file mode 100644
index 000000000000..923ac7f7b291
--- /dev/null
+++ b/drivers/video/backlight/mp3309c.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for MPS MP3309C White LED driver with I2C interface
+ *
+ * Copyright (C) 2023 ASEM Srl
+ * Author: Flavio Suligoi <f.suligoi@asem.it>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define REG_I2C_0	0x00
+#define REG_I2C_1	0x01
+
+#define REG_I2C_0_EN	0x80
+#define REG_I2C_0_D0	0x40
+#define REG_I2C_0_D1	0x20
+#define REG_I2C_0_D2	0x10
+#define REG_I2C_0_D3	0x08
+#define REG_I2C_0_D4	0x04
+#define REG_I2C_0_RSRV1	0x02
+#define REG_I2C_0_RSRV2	0x01
+
+#define REG_I2C_1_RSRV1	0x80
+#define REG_I2C_1_DIMS	0x40
+#define REG_I2C_1_SYNC	0x20
+#define REG_I2C_1_OVP0	0x10
+#define REG_I2C_1_OVP1	0x08
+#define REG_I2C_1_VOS	0x04
+#define REG_I2C_1_LEDO	0x02
+#define REG_I2C_1_OTP	0x01
+
+#define ANALOG_MAX_VAL	31
+#define ANALOG_REG_MASK 0x7c
+
+enum mp3309c_status_value {
+	FIRST_POWER_ON,
+	BACKLIGHT_OFF,
+	BACKLIGHT_ON,
+};
+
+enum mp3309c_dimming_mode_value {
+	DIMMING_PWM,
+	DIMMING_ANALOG_I2C,
+};
+
+struct mp3309c_platform_data {
+	u32 max_brightness;
+	u32 default_brightness;
+	u8  dimming_mode;
+	u8  over_voltage_protection;
+	bool sync_mode;
+	u8 status;
+};
+
+struct mp3309c_chip {
+	struct device *dev;
+	struct mp3309c_platform_data *pdata;
+	struct backlight_device *bl;
+	struct gpio_desc *enable_gpio;
+	struct regmap *regmap;
+	struct pwm_device *pwmd;
+	int irq;
+	struct gpio_desc *reset_gpio;
+};
+
+static const struct regmap_config mp3309c_regmap = {
+	.name = "mp3309c_regmap",
+	.reg_bits = 8,
+	.reg_stride = 1,
+	.val_bits = 8,
+	.max_register = REG_I2C_1,
+};
+
+static int mp3309c_enable_device(struct mp3309c_chip *chip)
+{
+	u8 reg_val;
+	int ret;
+
+	/* I2C register #0 - Device enable */
+	ret = regmap_update_bits(chip->regmap, REG_I2C_0, REG_I2C_0_EN,
+				 REG_I2C_0_EN);
+	if (ret)
+		return ret;
+
+	/*
+	 * I2C register #1 - Set working mode:
+	 *  - set one of the two dimming mode:
+	 *    - PWM dimming using an external PWM dimming signal
+	 *    - analog dimming using I2C commands
+	 *  - enable/disable synchronous mode
+	 *  - set overvoltage protection (OVP)
+	 */
+	reg_val = 0x00;
+	if (chip->pdata->dimming_mode == DIMMING_PWM)
+		reg_val |= REG_I2C_1_DIMS;
+	if (chip->pdata->sync_mode)
+		reg_val |= REG_I2C_1_SYNC;
+	reg_val |= chip->pdata->over_voltage_protection;
+	ret = regmap_write(chip->regmap, REG_I2C_1, reg_val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mp3309c_bl_update_status(struct backlight_device *bl)
+{
+	struct mp3309c_chip *chip = bl_get_data(bl);
+	int brightness = backlight_get_brightness(bl);
+	struct pwm_state pwmstate;
+	unsigned int analog_val, bits_val;
+	int i, ret;
+
+	if (chip->pdata->dimming_mode == DIMMING_PWM) {
+		/*
+		 * PWM dimming mode
+		 */
+		pwm_get_state(chip->pwmd, &pwmstate);
+		pwm_set_relative_duty_cycle(&pwmstate, brightness,
+					    chip->pdata->max_brightness);
+		pwmstate.enabled = true;
+		ret = pwm_apply_state(chip->pwmd, &pwmstate);
+		if (ret)
+			return ret;
+
+		switch (chip->pdata->status) {
+		case FIRST_POWER_ON:
+		case BACKLIGHT_OFF:
+			/*
+			 * After 20ms of low pwm signal level, the chip turns
+			   off automatically. In this case, before enabling the
+			   chip again, we must wait about 10ms for pwm signal to
+			   stabilize.
+			 */
+			if (brightness > 0) {
+				msleep(10);
+				mp3309c_enable_device(chip);
+				chip->pdata->status = BACKLIGHT_ON;
+			} else {
+				chip->pdata->status = BACKLIGHT_OFF;
+			}
+			break;
+		case BACKLIGHT_ON:
+			if (brightness == 0)
+				chip->pdata->status = BACKLIGHT_OFF;
+			break;
+		}
+	} else {
+		/*
+		 * Analog dimming (by I2C command) dimming mode
+		 *
+		 * The first time, before setting brightness, we must enable the
+		 * device
+		 */
+		if (chip->pdata->status == FIRST_POWER_ON)
+			mp3309c_enable_device(chip);
+
+		/*
+		 * Dimming mode I2C command
+		 *
+		 * The 5 bits of the dimming analog value D4..D0 is allocated
+		 * in the I2C register #0, in the following way:
+		 *
+		 *     +--+--+--+--+--+--+--+--+
+		 *     |EN|D0|D1|D2|D3|D4|XX|XX|
+		 *     +--+--+--+--+--+--+--+--+
+		 */
+		analog_val = DIV_ROUND_UP(ANALOG_MAX_VAL * brightness,
+					  chip->pdata->max_brightness);
+		bits_val = 0;
+		for (i = 0; i <= 5; i++)
+			bits_val += ((analog_val >> i) & 0x01) << (6 - i);
+		ret = regmap_update_bits(chip->regmap, REG_I2C_0,
+					 ANALOG_REG_MASK, bits_val);
+		if (ret)
+			return ret;
+
+		if (brightness > 0)
+			chip->pdata->status = BACKLIGHT_ON;
+		else
+			chip->pdata->status = BACKLIGHT_OFF;
+	}
+
+	return 0;
+}
+
+static const struct backlight_ops mp3309c_bl_ops = {
+	.update_status = mp3309c_bl_update_status,
+};
+
+static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
+				 struct mp3309c_platform_data *pdata)
+{
+	struct device_node *node = chip->dev->of_node;
+	struct property *prop;
+	int length, ret;
+	unsigned int tmp_value;
+
+	if (!node) {
+		dev_err(chip->dev, "failed to get DT node\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Dimming mode: the MP3309C provides two dimming control mode:
+	 *
+	 * - PWM mode
+	 * - Analog by I2C control mode (default)
+	 *
+	 * I2C control mode is assumed as default but, if the pwms property is
+	 * found in the backlight node, the mode switches to PWM mode.
+	 */
+	pdata->dimming_mode = DIMMING_ANALOG_I2C;
+	prop = of_find_property(node, "pwms", &length);
+	if (prop) {
+		chip->pwmd = devm_pwm_get(chip->dev, NULL);
+		if (IS_ERR(chip->pwmd))
+			return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
+					     "error getting pwm data\n");
+		pdata->dimming_mode = DIMMING_PWM;
+		pwm_apply_args(chip->pwmd);
+	}
+
+	/* Dimming steps (levels): 0 .. max_brightness */
+	ret = of_property_read_u32(node, "max-brightness",
+				   &pdata->max_brightness);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to get max-brightness from DT\n");
+		return ret;
+	}
+
+	/* Default brightness at startup */
+	ret = of_property_read_u32(node, "default-brightness",
+				   &pdata->default_brightness);
+	if (ret < 0) {
+		dev_err(chip->dev,
+			"failed to get default-brightness from DT\n");
+		return ret;
+	}
+
+	/* Enable GPIO used in I2C dimming mode */
+	if (pdata->dimming_mode == DIMMING_ANALOG_I2C) {
+		chip->enable_gpio = devm_gpiod_get(chip->dev, "enable",
+						   GPIOD_OUT_HIGH);
+		if (IS_ERR(chip->enable_gpio))
+			return dev_err_probe(chip->dev, PTR_ERR(chip->enable_gpio),
+					     "error getting enable gpio\n");
+	}
+
+	/*
+	 * Over-voltage protection (OVP)
+	 *
+	 * This (optional) property values are:
+	 *
+	 *  - 13.5V
+	 *  - 24V
+	 *  - 35.5V (hardware default setting)
+	 *
+	 * If missing, the default value for OVP is 35.5V
+	 */
+	pdata->over_voltage_protection = REG_I2C_1_OVP1;
+	if (!of_property_read_u32(node, "mps,overvoltage-protection-microvolt",
+				  &tmp_value)) {
+		switch (tmp_value) {
+		case 13500000:
+			pdata->over_voltage_protection = 0x00;
+			break;
+		case 24000000:
+			pdata->over_voltage_protection = REG_I2C_1_OVP0;
+			break;
+		case 35500000:
+			pdata->over_voltage_protection = REG_I2C_1_OVP1;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	/* Synchronous (default) and non-synchronous mode */
+	pdata->sync_mode = true;
+	if (of_property_read_bool(node, "mps,no-sync-mode"))
+		pdata->sync_mode = false;
+
+	return 0;
+}
+
+static int mp3309c_probe(struct i2c_client *client)
+{
+	struct mp3309c_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct mp3309c_chip *chip;
+	struct backlight_properties props;
+	struct pwm_state pwmstate;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "failed to check i2c functionality\n");
+		return -EOPNOTSUPP;
+	}
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &client->dev;
+
+	chip->regmap = devm_regmap_init_i2c(client, &mp3309c_regmap);
+	if (IS_ERR(chip->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+				     "failed to allocate register map\n");
+
+	i2c_set_clientdata(client, chip);
+
+	if (!pdata) {
+		pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		ret = pm3309c_parse_dt_node(chip, pdata);
+		if (ret)
+			return ret;
+	}
+	chip->pdata = pdata;
+
+	/* Backlight properties */
+	props.brightness = pdata->default_brightness;
+	props.max_brightness = pdata->max_brightness;
+	props.scale = BACKLIGHT_SCALE_LINEAR;
+	props.type = BACKLIGHT_RAW;
+	props.power = FB_BLANK_UNBLANK;
+	props.fb_blank = FB_BLANK_UNBLANK;
+	chip->bl = devm_backlight_device_register(chip->dev, "mp3309c",
+						  chip->dev, chip, &mp3309c_bl_ops,
+						  &props);
+	if (IS_ERR(chip->bl))
+		return dev_err_probe(chip->dev, PTR_ERR(chip->bl),
+				     "error registering backlight device\n");
+
+	/* In PWM dimming mode, enable pwm device */
+	if (chip->pdata->dimming_mode == DIMMING_PWM) {
+		pwm_init_state(chip->pwmd, &pwmstate);
+		pwm_set_relative_duty_cycle(&pwmstate,
+					    chip->pdata->default_brightness,
+					    chip->pdata->max_brightness);
+		pwmstate.enabled = true;
+		ret = pwm_apply_state(chip->pwmd, &pwmstate);
+		if (ret)
+			return dev_err_probe(chip->dev, ret,
+					     "error setting pwm device\n");
+	}
+
+	chip->pdata->status = FIRST_POWER_ON;
+	backlight_update_status(chip->bl);
+
+	return 0;
+}
+
+static void mp3309c_remove(struct i2c_client *client)
+{
+	struct mp3309c_chip *chip = i2c_get_clientdata(client);
+	struct backlight_device *bl = chip->bl;
+
+	bl->props.power = FB_BLANK_POWERDOWN;
+	bl->props.brightness = 0;
+	backlight_update_status(chip->bl);
+}
+
+static const struct of_device_id mp3309c_match_table[] = {
+	{ .compatible = "mps,mp3309c", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mp3309c_match_table);
+
+static const struct i2c_device_id mp3309c_id[] = {
+	{ "mp3309c", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mp3309c_id);
+
+static struct i2c_driver mp3309c_i2c_driver = {
+	.driver	= {
+			.name		= KBUILD_MODNAME,
+			.of_match_table	= mp3309c_match_table,
+	},
+	.probe		= mp3309c_probe,
+	.remove		= mp3309c_remove,
+	.id_table	= mp3309c_id,
+};
+
+module_i2c_driver(mp3309c_i2c_driver);
+
+MODULE_DESCRIPTION("Backlight Driver for MPS MP3309C");
+MODULE_AUTHOR("Flavio Suligoi <f.suligoi@asem.it>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C

[Daniel Thompson]

On Mon, Sep 25, 2023 at 02:26:09PM +0200, Flavio Suligoi wrote:
> diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c
> new file mode 100644
> index 000000000000..923ac7f7b291
> --- /dev/null
> +++ b/drivers/video/backlight/mp3309c.c
> @@ -0,0 +1,398 @@
> ...
> +static int mp3309c_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mp3309c_chip *chip = bl_get_data(bl);
> +	int brightness = backlight_get_brightness(bl);
> +	struct pwm_state pwmstate;
> +	unsigned int analog_val, bits_val;
> +	int i, ret;
> +
> +	if (chip->pdata->dimming_mode == DIMMING_PWM) {
> +		/*
> +		 * PWM dimming mode
> +		 */
> +		pwm_get_state(chip->pwmd, &pwmstate);
> +		pwm_set_relative_duty_cycle(&pwmstate, brightness,
> +					    chip->pdata->max_brightness);
> +		pwmstate.enabled = true;
> +		ret = pwm_apply_state(chip->pwmd, &pwmstate);
> +		if (ret)
> +			return ret;
> +
> +		switch (chip->pdata->status) {
> +		case FIRST_POWER_ON:
> +		case BACKLIGHT_OFF:
> +			/*
> +			 * After 20ms of low pwm signal level, the chip turns
> +			   off automatically. In this case, before enabling the
> +			   chip again, we must wait about 10ms for pwm signal to
> +			   stabilize.
> +			 */
> +			if (brightness > 0) {
> +				msleep(10);
> +				mp3309c_enable_device(chip);
> +				chip->pdata->status = BACKLIGHT_ON;
> +			} else {
> +				chip->pdata->status = BACKLIGHT_OFF;
> +			}
> +			break;
> +		case BACKLIGHT_ON:
> +			if (brightness == 0)
> +				chip->pdata->status = BACKLIGHT_OFF;
> +			break;
> +		}
> +	} else {
> +		/*
> +		 * Analog dimming (by I2C command) dimming mode
> +		 *
> +		 * The first time, before setting brightness, we must enable the
> +		 * device
> +		 */
> +		if (chip->pdata->status == FIRST_POWER_ON)
> +			mp3309c_enable_device(chip);
> +
> +		/*
> +		 * Dimming mode I2C command
> +		 *
> +		 * The 5 bits of the dimming analog value D4..D0 is allocated
> +		 * in the I2C register #0, in the following way:
> +		 *
> +		 *     +--+--+--+--+--+--+--+--+
> +		 *     |EN|D0|D1|D2|D3|D4|XX|XX|
> +		 *     +--+--+--+--+--+--+--+--+
> +		 */
> +		analog_val = DIV_ROUND_UP(ANALOG_MAX_VAL * brightness,
> +					  chip->pdata->max_brightness);

Sorry to only notice after sharing a Reviewed-by:[1] but...

Scaling brightness here isn't right. When running in I2C dimming mode then
max_brightness *must* be 31 or lower, meaning the value in brightness can
be applied directly to the hardware without scaling.

Quoting the DT binding docs about how max-brightness should be
interpretted:

  Normally the maximum brightness is determined by the hardware and this
  property is not required. This property is used to put a software
  limit on the brightness apart from what the driver says, as it could
  happen that a LED can be made so bright that it gets damaged or causes
  damage due to restrictions in a specific system, such as mounting
  conditions.


Daniel.


[1] I remember checking if this code could overflow the field but I was
    so distracted by that I ended up missing the obvious!

RE: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C

[Flavio Suligoi]

Hi Daniel,

...

> > +static int mp3309c_bl_update_status(struct backlight_device *bl) {
> > +	struct mp3309c_chip *chip = bl_get_data(bl);
> > +	int brightness = backlight_get_brightness(bl);
> > +	struct pwm_state pwmstate;
> > +	unsigned int analog_val, bits_val;
> > +	int i, ret;
> > +
> > +	if (chip->pdata->dimming_mode == DIMMING_PWM) {
> > +		/*
> > +		 * PWM dimming mode
> > +		 */
> > +		pwm_get_state(chip->pwmd, &pwmstate);
> > +		pwm_set_relative_duty_cycle(&pwmstate, brightness,
> > +					    chip->pdata->max_brightness);
> > +		pwmstate.enabled = true;
> > +		ret = pwm_apply_state(chip->pwmd, &pwmstate);
> > +		if (ret)
> > +			return ret;
> > +
> > +		switch (chip->pdata->status) {
> > +		case FIRST_POWER_ON:
> > +		case BACKLIGHT_OFF:
> > +			/*
> > +			 * After 20ms of low pwm signal level, the chip turns
> > +			   off automatically. In this case, before enabling the
> > +			   chip again, we must wait about 10ms for pwm signal
> to
> > +			   stabilize.
> > +			 */
> > +			if (brightness > 0) {
> > +				msleep(10);
> > +				mp3309c_enable_device(chip);
> > +				chip->pdata->status = BACKLIGHT_ON;
> > +			} else {
> > +				chip->pdata->status = BACKLIGHT_OFF;
> > +			}
> > +			break;
> > +		case BACKLIGHT_ON:
> > +			if (brightness == 0)
> > +				chip->pdata->status = BACKLIGHT_OFF;
> > +			break;
> > +		}
> > +	} else {
> > +		/*
> > +		 * Analog dimming (by I2C command) dimming mode
> > +		 *
> > +		 * The first time, before setting brightness, we must enable the
> > +		 * device
> > +		 */
> > +		if (chip->pdata->status == FIRST_POWER_ON)
> > +			mp3309c_enable_device(chip);
> > +
> > +		/*
> > +		 * Dimming mode I2C command
> > +		 *
> > +		 * The 5 bits of the dimming analog value D4..D0 is allocated
> > +		 * in the I2C register #0, in the following way:
> > +		 *
> > +		 *     +--+--+--+--+--+--+--+--+
> > +		 *     |EN|D0|D1|D2|D3|D4|XX|XX|
> > +		 *     +--+--+--+--+--+--+--+--+
> > +		 */
> > +		analog_val = DIV_ROUND_UP(ANALOG_MAX_VAL *
> brightness,
> > +					  chip->pdata->max_brightness);
> 
> Sorry to only notice after sharing a Reviewed-by:[1] but...
> 
> Scaling brightness here isn't right. When running in I2C dimming mode then
> max_brightness *must* be 31 or lower, meaning the value in brightness can
> be applied directly to the hardware without scaling.

ok, right; max brightness is 31, fixed

> 
> Quoting the DT binding docs about how max-brightness should be
> interpretted:
> 
>   Normally the maximum brightness is determined by the hardware and this
>   property is not required. This property is used to put a software
>   limit on the brightness apart from what the driver says, as it could
>   happen that a LED can be made so bright that it gets damaged or causes
>   damage due to restrictions in a specific system, such as mounting
>   conditions.
> 

ok

> 
> Daniel.
> 
> 
> [1] I remember checking if this code could overflow the field but I was
>     so distracted by that I ended up missing the obvious!

Thanks and best regards,
Flavio

Re: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Daniel Thompson]

On Mon, Sep 25, 2023 at 02:26:08PM +0200, Flavio Suligoi wrote:
> The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
> programmable switching frequency to optimize efficiency.
> The brightness can be controlled either by I2C commands (called "analog"
> mode) or by a PWM input signal (PWM mode).
> This driver supports both modes.
>
> For device driver details, please refer to:
> - drivers/video/backlight/mp3309c_bl.c
>
> The datasheet is available at:
> - https://www.monolithicpower.com/en/mp3309c.html
>
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>
> v3:
>  - add default value for mps,overvoltage-protection-microvolt property
>  - fix the example, changing from "mps,mp3309c-backlight" to "mps,mp3309c" in
>    compatible property
> v2:
>  - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0,
>    switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms,
>    reset-on-length-ms)
>  - add common.yaml#
>  - remove already included properties (default-brightness, max-brightness)
>  - substitute three boolean properties, used for the overvoltage-protection
>    values, with a single enum property
>  - remove some conditional definitions
>  - remove the 2nd example
> v1:
>  - first version
>
>  .../bindings/leds/backlight/mps,mp3309c.yaml  | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> new file mode 100644
> index 000000000000..4191e33626f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MPS MP3309C backlight
> +
> +maintainers:
> +  - Flavio Suligoi <f.suligoi@asem.it>
> +
> +description: |
> +  The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
> +  programmable switching frequency to optimize efficiency.
> +  It supports two different dimming modes:
> +
> +  - analog mode, via I2C commands (default)
> +  - PWM controlled mode.
> +
> +  The datasheet is available at:
> +  https://www.monolithicpower.com/en/mp3309c.html
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: mps,mp3309c
> +
> +  reg:
> +    maxItems: 1
> +
> +  pwms:
> +    description: if present, the backlight is controlled in PWM mode.
> +    maxItems: 1
> +
> +  enable-gpios:
> +    description: GPIO used to enable the backlight in "analog-i2c" dimming mode.
> +    maxItems: 1
> +
> +  mps,overvoltage-protection-microvolt:
> +    description: Overvoltage protection (13.5V, 24V or 35.5V).
> +    enum: [ 13500000, 24000000, 35500000 ]
> +    default: 35500000
> +
> +  mps,no-sync-mode:
> +    description: disable synchronous rectification mode
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - max-brightness

Why is this mandatory?

There's no point in setting max-brightness when running in I2C mode
(max-brightness should default to 31 in that case).


> +  - default-brightness

Again. I'm not clear why this needs to be mandatory.


Daniel.

RE: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

Hi Daniel,

...

> > +required:
> > +  - compatible
> > +  - reg
> > +  - max-brightness
> 
> Why is this mandatory?
> 
> There's no point in setting max-brightness when running in I2C mode (max-
> brightness should default to 31 in that case).
> 
> 
> > +  - default-brightness
> 
> Again. I'm not clear why this needs to be mandatory.
> 
> 

Ok, you are right, I'll remove max-brightness and default-brightness from required properties list.
I think to change these properties, for the pwm dimming, into a clearer:

- brightness-levels (uint32) 
- default-brightness-levels (uint32).

For example:

  brightness-levels:
    description:
      Number of brightness levels. The actual brightness
      level (PWM duty cycle) will be interpolated from 0 to this value.
      0 means a  0% duty cycle (darkest/off), while the brightness-levels represents
      a 100% duty cycle (brightest).
    $ref: /schemas/types.yaml#/definitions/uint32

  default-brightness-level:
    description:
      The default brightness level (from 0 to brightness-levels)
    $ref: /schemas/types.yaml#/definitions/uint32

Example:
brightness-levels = <10>;
default-brightness-level = <6>;

What do you think about this solution?

> Daniel.

Thanks for your help,
Flavio

Re: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Daniel Thompson]

On Tue, Oct 03, 2023 at 09:43:15AM +0000, Flavio Suligoi wrote:
> Hi Daniel,
>
> ...
>
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - max-brightness
> >
> > Why is this mandatory?
> >
> > There's no point in setting max-brightness when running in I2C mode (max-
> > brightness should default to 31 in that case).
> >
> >
> > > +  - default-brightness
> >
> > Again. I'm not clear why this needs to be mandatory.
> >
> >
>
> Ok, you are right, I'll remove max-brightness and default-brightness
> from required properties list. I think to change these properties,
> for the pwm dimming, into a clearer:
>
> - brightness-levels (uint32)
> - default-brightness-levels (uint32).
>
> For example:
>
>   brightness-levels:
>     description:
>       Number of brightness levels. The actual brightness
>       level (PWM duty cycle) will be interpolated from 0 to this value.
>       0 means a  0% duty cycle (darkest/off), while the brightness-levels represents
>       a 100% duty cycle (brightest).
>     $ref: /schemas/types.yaml#/definitions/uint32
>
>   default-brightness-level:
>     description:
>       The default brightness level (from 0 to brightness-levels)
>     $ref: /schemas/types.yaml#/definitions/uint32
>
> Example:
> brightness-levels = <10>;
> default-brightness-level = <6>;
>
> What do you think about this solution?

If you want to introduce a brightness-levels property then I would
expect it to be defined with the same meaning as pwm-backlight (it's not
relevant to the bindings but ideally it would be implemented by
refactoring and reusing the code from pwm_bl.c).

Same with default-brightness-level although I'm not sure why one
wouldn't just use default-brightness for new bindings (doesn't
default-brightness-level simply do exactly the same thing as
default-brightness).


Daniel.

RE: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

Hi Daniel,

...
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - max-brightness
> > >
> > > Why is this mandatory?
> > >
> > > There's no point in setting max-brightness when running in I2C mode
> > > (max- brightness should default to 31 in that case).
> > >
> > >
> > > > +  - default-brightness
> > >
> > > Again. I'm not clear why this needs to be mandatory.
> > >
> > >
> >
> > Ok, you are right, I'll remove max-brightness and default-brightness
> > from required properties list. I think to change these properties, for
> > the pwm dimming, into a clearer:
> >
> > - brightness-levels (uint32)
> > - default-brightness-levels (uint32).
> >
> > For example:
> >
> >   brightness-levels:
> >     description:
> >       Number of brightness levels. The actual brightness
> >       level (PWM duty cycle) will be interpolated from 0 to this value.
> >       0 means a  0% duty cycle (darkest/off), while the brightness-levels
> represents
> >       a 100% duty cycle (brightest).
> >     $ref: /schemas/types.yaml#/definitions/uint32
> >
> >   default-brightness-level:
> >     description:
> >       The default brightness level (from 0 to brightness-levels)
> >     $ref: /schemas/types.yaml#/definitions/uint32
> >
> > Example:
> > brightness-levels = <10>;
> > default-brightness-level = <6>;
> >
> > What do you think about this solution?
> 
> If you want to introduce a brightness-levels property then I would expect it to
> be defined with the same meaning as pwm-backlight (it's not relevant to the
> bindings but ideally it would be implemented by refactoring and reusing the
> code from pwm_bl.c).

ok, I'll use the brightness-levels property as used in pwm-backlight

> 
> Same with default-brightness-level although I'm not sure why one wouldn't
> just use default-brightness for new bindings (doesn't default-brightness-level
> simply do exactly the same thing as default-brightness).

ok for default-brightness instead of default-brightness-level

> 
> 
> Daniel.

Thanks an best regards,
Flavio

RE: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

Hi Daniel,
...
> ...
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - max-brightness
> > > >
> > > > Why is this mandatory?
> > > >
> > > > There's no point in setting max-brightness when running in I2C
> > > > mode
> > > > (max- brightness should default to 31 in that case).
> > > >
> > > >
> > > > > +  - default-brightness
> > > >
> > > > Again. I'm not clear why this needs to be mandatory.
> > > >
> > > >
> > >
> > > Ok, you are right, I'll remove max-brightness and default-brightness
> > > from required properties list. I think to change these properties,
> > > for the pwm dimming, into a clearer:
> > >
> > > - brightness-levels (uint32)
> > > - default-brightness-levels (uint32).
> > >
> > > For example:
> > >
> > >   brightness-levels:
> > >     description:
> > >       Number of brightness levels. The actual brightness
> > >       level (PWM duty cycle) will be interpolated from 0 to this value.
> > >       0 means a  0% duty cycle (darkest/off), while the
> > > brightness-levels
> > represents
> > >       a 100% duty cycle (brightest).
> > >     $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > >   default-brightness-level:
> > >     description:
> > >       The default brightness level (from 0 to brightness-levels)
> > >     $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > > Example:
> > > brightness-levels = <10>;
> > > default-brightness-level = <6>;
> > >
> > > What do you think about this solution?
> >
> > If you want to introduce a brightness-levels property then I would
> > expect it to be defined with the same meaning as pwm-backlight (it's
> > not relevant to the bindings but ideally it would be implemented by
> > refactoring and reusing the code from pwm_bl.c).
> 
> ok, I'll use the brightness-levels property as used in pwm-backlight
> 
> >
> > Same with default-brightness-level although I'm not sure why one
> > wouldn't just use default-brightness for new bindings (doesn't
> > default-brightness-level simply do exactly the same thing as default-
> brightness).
> 
> ok for default-brightness instead of default-brightness-level

Just a question: default-brightness-level is the index into the brightness-levels array.
But, if I use default-brightness instead of default-brightness-level,  
should I consider default-brightness also as an index into brightness-levels array?
Or, in this case, have the default-brightness to be equal to one of the values inside the
brightness-levels array?

> 
> >
> >
> > Daniel.
> 
> Thanks an best regards,
> Flavio

Thanks,

Flavio

Re: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Daniel Thompson]

On Wed, Oct 04, 2023 at 03:18:24PM +0000, Flavio Suligoi wrote:
> Hi Daniel,
> ...
> > ...
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - reg
> > > > > > +  - max-brightness
> > > > >
> > > > > Why is this mandatory?
> > > > >
> > > > > There's no point in setting max-brightness when running in I2C
> > > > > mode
> > > > > (max- brightness should default to 31 in that case).
> > > > >
> > > > >
> > > > > > +  - default-brightness
> > > > >
> > > > > Again. I'm not clear why this needs to be mandatory.
> > > > >
> > > > >
> > > >
> > > > Ok, you are right, I'll remove max-brightness and default-brightness
> > > > from required properties list. I think to change these properties,
> > > > for the pwm dimming, into a clearer:
> > > >
> > > > - brightness-levels (uint32)
> > > > - default-brightness-levels (uint32).
> > > >
> > > > For example:
> > > >
> > > >   brightness-levels:
> > > >     description:
> > > >       Number of brightness levels. The actual brightness
> > > >       level (PWM duty cycle) will be interpolated from 0 to this value.
> > > >       0 means a  0% duty cycle (darkest/off), while the
> > > > brightness-levels
> > > represents
> > > >       a 100% duty cycle (brightest).
> > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > >
> > > >   default-brightness-level:
> > > >     description:
> > > >       The default brightness level (from 0 to brightness-levels)
> > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > >
> > > > Example:
> > > > brightness-levels = <10>;
> > > > default-brightness-level = <6>;
> > > >
> > > > What do you think about this solution?
> > >
> > > If you want to introduce a brightness-levels property then I would
> > > expect it to be defined with the same meaning as pwm-backlight (it's
> > > not relevant to the bindings but ideally it would be implemented by
> > > refactoring and reusing the code from pwm_bl.c).
> >
> > ok, I'll use the brightness-levels property as used in pwm-backlight
> >
> > >
> > > Same with default-brightness-level although I'm not sure why one
> > > wouldn't just use default-brightness for new bindings (doesn't
> > > default-brightness-level simply do exactly the same thing as default-
> > brightness).
> >
> > ok for default-brightness instead of default-brightness-level
>
> Just a question: default-brightness-level is the index into the brightness-levels array.
> But, if I use default-brightness instead of default-brightness-level,
> should I consider default-brightness also as an index into brightness-levels array?

Yes.


> Or, in this case, have the default-brightness to be equal to one of the values inside the
> brightness-levels array?

When there is a brightness array (and there is no interpolation) then
it is indexed by brightness. The values in the array are not
brightness (e.g. the controlable value describing the output of the
hardware). The values in the table are merely the PWM duty cycle...

Main difference is, with a correct table the brightness can use an
appropriate logarithmic power scale (which matches how humans perceive
brightness) instead of the linear scale provided by the PWM duty cycle.


Daniel.


Brightness and "index into the brightness-levels array" should be one
and the same thing
>
> >
> > >
> > >
> > > Daniel.
> >
> > Thanks an best regards,
> > Flavio
>
> Thanks,
>
> Flavio

RE: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

HI Daniel,

...
> > ...
> > > ...
> > > > > > > +required:
> > > > > > > +  - compatible
> > > > > > > +  - reg
> > > > > > > +  - max-brightness
> > > > > >
> > > > > > Why is this mandatory?
> > > > > >
> > > > > > There's no point in setting max-brightness when running in I2C
> > > > > > mode
> > > > > > (max- brightness should default to 31 in that case).
> > > > > >
> > > > > >
> > > > > > > +  - default-brightness
> > > > > >
> > > > > > Again. I'm not clear why this needs to be mandatory.
> > > > > >
> > > > > >
> > > > >
> > > > > Ok, you are right, I'll remove max-brightness and
> > > > > default-brightness from required properties list. I think to
> > > > > change these properties, for the pwm dimming, into a clearer:
> > > > >
> > > > > - brightness-levels (uint32)
> > > > > - default-brightness-levels (uint32).
> > > > >
> > > > > For example:
> > > > >
> > > > >   brightness-levels:
> > > > >     description:
> > > > >       Number of brightness levels. The actual brightness
> > > > >       level (PWM duty cycle) will be interpolated from 0 to this value.
> > > > >       0 means a  0% duty cycle (darkest/off), while the
> > > > > brightness-levels
> > > > represents
> > > > >       a 100% duty cycle (brightest).
> > > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > > >
> > > > >   default-brightness-level:
> > > > >     description:
> > > > >       The default brightness level (from 0 to brightness-levels)
> > > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > > >
> > > > > Example:
> > > > > brightness-levels = <10>;
> > > > > default-brightness-level = <6>;
> > > > >
> > > > > What do you think about this solution?
> > > >
> > > > If you want to introduce a brightness-levels property then I would
> > > > expect it to be defined with the same meaning as pwm-backlight
> > > > (it's not relevant to the bindings but ideally it would be
> > > > implemented by refactoring and reusing the code from pwm_bl.c).
> > >
> > > ok, I'll use the brightness-levels property as used in pwm-backlight
> > >
> > > >
> > > > Same with default-brightness-level although I'm not sure why one
> > > > wouldn't just use default-brightness for new bindings (doesn't
> > > > default-brightness-level simply do exactly the same thing as
> > > > default-
> > > brightness).
> > >
> > > ok for default-brightness instead of default-brightness-level
> >
> > Just a question: default-brightness-level is the index into the brightness-
> levels array.
> > But, if I use default-brightness instead of default-brightness-level,
> > should I consider default-brightness also as an index into brightness-levels
> array?
> 
> Yes.
> 
> 
> > Or, in this case, have the default-brightness to be equal to one of
> > the values inside the brightness-levels array?
> 
> When there is a brightness array (and there is no interpolation) then it is
> indexed by brightness. The values in the array are not brightness (e.g. the
> controlable value describing the output of the hardware). The values in the
> table are merely the PWM duty cycle...

ok

> 
> Main difference is, with a correct table the brightness can use an appropriate
> logarithmic power scale (which matches how humans perceive
> brightness) instead of the linear scale provided by the PWM duty cycle.
> 
> 
> Daniel.
> 
> 
> Brightness and "index into the brightness-levels array" should be one and the
> same thing

ok, I'll use default-brightness, thanks for the explanations!

> >
> > >
> > > >
> > > >
> > > > Daniel.
> > >
> > > Thanks an best regards,
> > > Flavio
> >
> > Thanks,
> >
> > Flavio

Flavio

Re: (subset) [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C

[Lee Jones]

On Mon, 25 Sep 2023 14:26:08 +0200, Flavio Suligoi wrote:
> The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a
> programmable switching frequency to optimize efficiency.
> The brightness can be controlled either by I2C commands (called "analog"
> mode) or by a PWM input signal (PWM mode).
> This driver supports both modes.
> 
> For device driver details, please refer to:
> - drivers/video/backlight/mp3309c_bl.c
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: backlight: Add MPS MP3309C
      commit: 02c4e661658f73d3c266c68f89f0b14bd8ba6bd8

--
Lee Jones [李琼斯]