[PATCH 0/1] dt-bindings: backlight: mp3309c: remove two required properties

[PATCH 0/1] dt-bindings: backlight: mp3309c: remove two required properties

[Flavio Suligoi]

This patch remove the following two not-required properties from the
"required" section:

- max-brightness
- default brightness

Other changes:

- improve the backlight working mode descripion in the "description"
  section
- update the example, removing the "max-brightness" and introducing the
  "brightess-levels" property

Note: the "brightess-levels" property is present in the last version of the
      common.yaml file, so it is not decalared here.
      For this last version see my patch:
      
[1/1] dt-bindings: backlight: add brightness-levels related common
 properties
commit: d5272d39995f4150062a67e6f2cef556edece740

Flavio Suligoi (1):
  dt-bindings: backlight: mp3309c: remove two required properties

 .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Flavio Suligoi]

The two properties:

- max-brightness
- default brightness

are not really required, so they can be removed from the "required"
section.

Other changes:

- improve the backlight working mode description, in the "description"
  section
- update the example, removing the "max-brightness" and introducing the
  "brightess-levels" property

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
index 4191e33626f5..527a37368ed7 100644
--- a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
@@ -14,8 +14,8 @@ description: |
   programmable switching frequency to optimize efficiency.
   It supports two different dimming modes:
 
-  - analog mode, via I2C commands (default)
-  - PWM controlled mode.
+  - analog mode, via I2C commands, as default mode (32 dimming levels)
+  - PWM controlled mode (optional)
 
   The datasheet is available at:
   https://www.monolithicpower.com/en/mp3309c.html
@@ -50,8 +50,6 @@ properties:
 required:
   - compatible
   - reg
-  - max-brightness
-  - default-brightness
 
 unevaluatedProperties: false
 
@@ -66,8 +64,8 @@ examples:
             compatible = "mps,mp3309c";
             reg = <0x17>;
             pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
-            max-brightness = <100>;
-            default-brightness = <80>;
+            brightness-levels = <0 4 8 16 32 64 128 255>;
+            default-brightness = <6>;
             mps,overvoltage-protection-microvolt = <24000000>;
         };
     };
-- 
2.34.1

[PATCH v5] 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>
---

v5:
 - checked and resend for updated kernel 6.6.0-rc1
v4:
 - add brightness-levels property
 - force fixed 32 brightness levels (0..31) in analog-i2c dimming mode
 - remove useless irq and reset_gpio from mp3309c_chip structure
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                       |   7 +
 drivers/video/backlight/Kconfig   |  11 +
 drivers/video/backlight/Makefile  |   1 +
 drivers/video/backlight/mp3309c.c | 443 ++++++++++++++++++++++++++++++
 4 files changed, 462 insertions(+)
 create mode 100644 drivers/video/backlight/mp3309c.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..3d23b869e2aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14474,6 +14474,13 @@ S:	Maintained
 F:	Documentation/driver-api/tty/moxa-smartio.rst
 F:	drivers/tty/mxser.*
 
+MP3309C BACKLIGHT DRIVER
+M:	Flavio Suligoi <f.suligoi@asem.it>
+L:	dri-devel@lists.freedesktop.org
+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..3fe4469ef43f
--- /dev/null
+++ b/drivers/video/backlight/mp3309c.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for MPS MP3309C White LED driver with I2C interface
+ *
+ * This driver support both analog (by I2C commands) and PWM dimming control
+ * modes.
+ *
+ * Copyright (C) 2023 ASEM Srl
+ * Author: Flavio Suligoi <f.suligoi@asem.it>
+ *
+ * Based on pwm_bl.c
+ */
+
+#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_I2C_NUM_LEVELS	32		/* 0..31 */
+#define ANALOG_I2C_REG_MASK	0x7c
+
+#define MP3309C_PWM_DEFAULT_NUM_LEVELS	256	/* 0..255 */
+
+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 {
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+	unsigned int *levels;
+	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;
+};
+
+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 control mode
+		 */
+		pwm_get_state(chip->pwmd, &pwmstate);
+		pwm_set_relative_duty_cycle(&pwmstate,
+					    chip->pdata->levels[brightness],
+					    chip->pdata->levels[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 (by I2C command) control 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 (fixed dimming range 0..31)
+		 *
+		 * 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 = 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_I2C_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_pwms, *prop_levels;
+	int length = 0;
+	int ret, i;
+	unsigned int num_levels, 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_pwms = of_find_property(node, "pwms", &length);
+	if (prop_pwms) {
+		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);
+	}
+
+	/*
+	 * In I2C control mode the dimming levels (0..31) are fixed by the
+	 * hardware, while in PWM control mode they can be chosen by the user,
+	 * to allow nonlinear mappings.
+	 */
+	if  (pdata->dimming_mode == DIMMING_ANALOG_I2C) {
+		/*
+		 * Analog (by I2C commands) control mode: fixed 0..31 brightness
+		 * levels
+		 */
+		num_levels = ANALOG_I2C_NUM_LEVELS;
+
+		/* Enable GPIO used in I2C dimming mode only */
+		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");
+	} else {
+		/*
+		 * PWM control mode: check for brightness level in DT
+		 */
+		prop_levels = of_find_property(node, "brightness-levels",
+					       &length);
+		if (prop_levels) {
+			/* Read brightness levels from DT */
+			num_levels = length / sizeof(u32);
+			if (num_levels < 2)
+				return -EINVAL;
+		} else {
+			/* Use default brightness levels */
+			num_levels = MP3309C_PWM_DEFAULT_NUM_LEVELS;
+		}
+	}
+
+	/* Fill brightness levels array */
+	pdata->levels = devm_kcalloc(chip->dev, num_levels,
+				     sizeof(*pdata->levels), GFP_KERNEL);
+	if (!pdata->levels)
+		return -ENOMEM;
+	if (prop_levels) {
+		ret = of_property_read_u32_array(node, "brightness-levels",
+						 pdata->levels,
+						 num_levels);
+		if (ret < 0)
+			return ret;
+	} else {
+		for (i = 0; i < num_levels; i++)
+			pdata->levels[i] = i;
+	}
+
+	pdata->max_brightness = num_levels - 1;
+
+	ret = of_property_read_u32(node, "default-brightness",
+				   &pdata->default_brightness);
+	if (ret)
+		pdata->default_brightness = pdata->max_brightness;
+	if (pdata->default_brightness > pdata->max_brightness) {
+		dev_err(chip->dev,
+			"default brightness exceeds max brightness\n");
+		pdata->default_brightness = pdata->max_brightness;
+	}
+
+	/*
+	 * 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 v5] backlight: mp3309c: Add support for MPS MP3309C

[Conor Dooley]

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

On Fri, Oct 20, 2023 at 03:54:34PM +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 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>
> ---
> 
> v5:
>  - checked and resend for updated kernel 6.6.0-rc1

Why is this v5 patch attached to a 1 patch "series" that only purports
to contain a binding patch?

Thanks,
Conor.

> v4:
>  - add brightness-levels property
>  - force fixed 32 brightness levels (0..31) in analog-i2c dimming mode
>  - remove useless irq and reset_gpio from mp3309c_chip structure
> 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                       |   7 +
>  drivers/video/backlight/Kconfig   |  11 +
>  drivers/video/backlight/Makefile  |   1 +
>  drivers/video/backlight/mp3309c.c | 443 ++++++++++++++++++++++++++++++
>  4 files changed, 462 insertions(+)
>  create mode 100644 drivers/video/backlight/mp3309c.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..3d23b869e2aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14474,6 +14474,13 @@ S:	Maintained
>  F:	Documentation/driver-api/tty/moxa-smartio.rst
>  F:	drivers/tty/mxser.*
>  
> +MP3309C BACKLIGHT DRIVER
> +M:	Flavio Suligoi <f.suligoi@asem.it>
> +L:	dri-devel@lists.freedesktop.org
> +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..3fe4469ef43f
> --- /dev/null
> +++ b/drivers/video/backlight/mp3309c.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for MPS MP3309C White LED driver with I2C interface
> + *
> + * This driver support both analog (by I2C commands) and PWM dimming control
> + * modes.
> + *
> + * Copyright (C) 2023 ASEM Srl
> + * Author: Flavio Suligoi <f.suligoi@asem.it>
> + *
> + * Based on pwm_bl.c
> + */
> +
> +#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_I2C_NUM_LEVELS	32		/* 0..31 */
> +#define ANALOG_I2C_REG_MASK	0x7c
> +
> +#define MP3309C_PWM_DEFAULT_NUM_LEVELS	256	/* 0..255 */
> +
> +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 {
> +	unsigned int max_brightness;
> +	unsigned int default_brightness;
> +	unsigned int *levels;
> +	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;
> +};
> +
> +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 control mode
> +		 */
> +		pwm_get_state(chip->pwmd, &pwmstate);
> +		pwm_set_relative_duty_cycle(&pwmstate,
> +					    chip->pdata->levels[brightness],
> +					    chip->pdata->levels[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 (by I2C command) control 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 (fixed dimming range 0..31)
> +		 *
> +		 * 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 = 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_I2C_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_pwms, *prop_levels;
> +	int length = 0;
> +	int ret, i;
> +	unsigned int num_levels, 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_pwms = of_find_property(node, "pwms", &length);
> +	if (prop_pwms) {
> +		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);
> +	}
> +
> +	/*
> +	 * In I2C control mode the dimming levels (0..31) are fixed by the
> +	 * hardware, while in PWM control mode they can be chosen by the user,
> +	 * to allow nonlinear mappings.
> +	 */
> +	if  (pdata->dimming_mode == DIMMING_ANALOG_I2C) {
> +		/*
> +		 * Analog (by I2C commands) control mode: fixed 0..31 brightness
> +		 * levels
> +		 */
> +		num_levels = ANALOG_I2C_NUM_LEVELS;
> +
> +		/* Enable GPIO used in I2C dimming mode only */
> +		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");
> +	} else {
> +		/*
> +		 * PWM control mode: check for brightness level in DT
> +		 */
> +		prop_levels = of_find_property(node, "brightness-levels",
> +					       &length);
> +		if (prop_levels) {
> +			/* Read brightness levels from DT */
> +			num_levels = length / sizeof(u32);
> +			if (num_levels < 2)
> +				return -EINVAL;
> +		} else {
> +			/* Use default brightness levels */
> +			num_levels = MP3309C_PWM_DEFAULT_NUM_LEVELS;
> +		}
> +	}
> +
> +	/* Fill brightness levels array */
> +	pdata->levels = devm_kcalloc(chip->dev, num_levels,
> +				     sizeof(*pdata->levels), GFP_KERNEL);
> +	if (!pdata->levels)
> +		return -ENOMEM;
> +	if (prop_levels) {
> +		ret = of_property_read_u32_array(node, "brightness-levels",
> +						 pdata->levels,
> +						 num_levels);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		for (i = 0; i < num_levels; i++)
> +			pdata->levels[i] = i;
> +	}
> +
> +	pdata->max_brightness = num_levels - 1;
> +
> +	ret = of_property_read_u32(node, "default-brightness",
> +				   &pdata->default_brightness);
> +	if (ret)
> +		pdata->default_brightness = pdata->max_brightness;
> +	if (pdata->default_brightness > pdata->max_brightness) {
> +		dev_err(chip->dev,
> +			"default brightness exceeds max brightness\n");
> +		pdata->default_brightness = pdata->max_brightness;
> +	}
> +
> +	/*
> +	 * 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Conor Dooley]

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

Yo,

On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> The two properties:
> 
> - max-brightness
> - default brightness
> 
> are not really required, so they can be removed from the "required"
> section.

Why are they not required? You need to provide an explanation.

> Other changes:
> 
> - improve the backlight working mode description, in the "description"
>   section

> - update the example, removing the "max-brightness" and introducing the
>   "brightess-levels" property

Why is this more useful?

Cheers,
Conor.

> 
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> ---
>  .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> index 4191e33626f5..527a37368ed7 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> @@ -14,8 +14,8 @@ description: |
>    programmable switching frequency to optimize efficiency.
>    It supports two different dimming modes:
>  
> -  - analog mode, via I2C commands (default)
> -  - PWM controlled mode.
> +  - analog mode, via I2C commands, as default mode (32 dimming levels)
> +  - PWM controlled mode (optional)
>  
>    The datasheet is available at:
>    https://www.monolithicpower.com/en/mp3309c.html
> @@ -50,8 +50,6 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - max-brightness
> -  - default-brightness
>  
>  unevaluatedProperties: false
>  
> @@ -66,8 +64,8 @@ examples:
>              compatible = "mps,mp3309c";
>              reg = <0x17>;
>              pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
> -            max-brightness = <100>;
> -            default-brightness = <80>;
> +            brightness-levels = <0 4 8 16 32 64 128 255>;
> +            default-brightness = <6>;
>              mps,overvoltage-protection-microvolt = <24000000>;
>          };
>      };
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v5] backlight: mp3309c: Add support for MPS MP3309C

[Flavio Suligoi]

Hi Conor,

...

> Subject: Re: [PATCH v5] backlight: mp3309c: Add support for MPS MP3309C
> 
> On Fri, Oct 20, 2023 at 03:54:34PM +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 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>
> > ---
> >
> > v5:
> >  - checked and resend for updated kernel 6.6.0-rc1
> 
> Why is this v5 patch attached to a 1 patch "series" that only purports to
> contain a binding patch?

Sorry Conor, what do you mean for "binding patch"? The cover letter of this patch or the
other patch about the dt-bindings for the mp3309c backlight ?

> Thanks,
> Conor.

Thanks,
Flavio.


> 
> > v4:
> >  - add brightness-levels property
> >  - force fixed 32 brightness levels (0..31) in analog-i2c dimming mode
> >  - remove useless irq and reset_gpio from mp3309c_chip structure
> > 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                       |   7 +
> >  drivers/video/backlight/Kconfig   |  11 +
> >  drivers/video/backlight/Makefile  |   1 +
> >  drivers/video/backlight/mp3309c.c | 443
> > ++++++++++++++++++++++++++++++
> >  4 files changed, 462 insertions(+)
> >  create mode 100644 drivers/video/backlight/mp3309c.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 90f13281d297..3d23b869e2aa 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14474,6 +14474,13 @@ S:	Maintained
> >  F:	Documentation/driver-api/tty/moxa-smartio.rst
> >  F:	drivers/tty/mxser.*
> >
> > +MP3309C BACKLIGHT DRIVER
> > +M:	Flavio Suligoi <f.suligoi@asem.it>
> > +L:	dri-devel@lists.freedesktop.org
> > +S:	Maintained
> > +F:
> 	Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.ya
> ml
> > +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..3fe4469ef43f
> > --- /dev/null
> > +++ b/drivers/video/backlight/mp3309c.c
> > @@ -0,0 +1,443 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Driver for MPS MP3309C White LED driver with I2C interface
> > + *
> > + * This driver support both analog (by I2C commands) and PWM dimming
> > +control
> > + * modes.
> > + *
> > + * Copyright (C) 2023 ASEM Srl
> > + * Author: Flavio Suligoi <f.suligoi@asem.it>
> > + *
> > + * Based on pwm_bl.c
> > + */
> > +
> > +#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_I2C_NUM_LEVELS	32		/* 0..31 */
> > +#define ANALOG_I2C_REG_MASK	0x7c
> > +
> > +#define MP3309C_PWM_DEFAULT_NUM_LEVELS	256	/* 0..255 */
> > +
> > +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 {
> > +	unsigned int max_brightness;
> > +	unsigned int default_brightness;
> > +	unsigned int *levels;
> > +	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;
> > +};
> > +
> > +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 control mode
> > +		 */
> > +		pwm_get_state(chip->pwmd, &pwmstate);
> > +		pwm_set_relative_duty_cycle(&pwmstate,
> > +					    chip->pdata->levels[brightness],
> > +					    chip->pdata->levels[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 (by I2C command) control 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 (fixed dimming range 0..31)
> > +		 *
> > +		 * 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 = 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_I2C_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_pwms, *prop_levels;
> > +	int length = 0;
> > +	int ret, i;
> > +	unsigned int num_levels, 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_pwms = of_find_property(node, "pwms", &length);
> > +	if (prop_pwms) {
> > +		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);
> > +	}
> > +
> > +	/*
> > +	 * In I2C control mode the dimming levels (0..31) are fixed by the
> > +	 * hardware, while in PWM control mode they can be chosen by the
> user,
> > +	 * to allow nonlinear mappings.
> > +	 */
> > +	if  (pdata->dimming_mode == DIMMING_ANALOG_I2C) {
> > +		/*
> > +		 * Analog (by I2C commands) control mode: fixed 0..31
> brightness
> > +		 * levels
> > +		 */
> > +		num_levels = ANALOG_I2C_NUM_LEVELS;
> > +
> > +		/* Enable GPIO used in I2C dimming mode only */
> > +		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");
> > +	} else {
> > +		/*
> > +		 * PWM control mode: check for brightness level in DT
> > +		 */
> > +		prop_levels = of_find_property(node, "brightness-levels",
> > +					       &length);
> > +		if (prop_levels) {
> > +			/* Read brightness levels from DT */
> > +			num_levels = length / sizeof(u32);
> > +			if (num_levels < 2)
> > +				return -EINVAL;
> > +		} else {
> > +			/* Use default brightness levels */
> > +			num_levels =
> MP3309C_PWM_DEFAULT_NUM_LEVELS;
> > +		}
> > +	}
> > +
> > +	/* Fill brightness levels array */
> > +	pdata->levels = devm_kcalloc(chip->dev, num_levels,
> > +				     sizeof(*pdata->levels), GFP_KERNEL);
> > +	if (!pdata->levels)
> > +		return -ENOMEM;
> > +	if (prop_levels) {
> > +		ret = of_property_read_u32_array(node, "brightness-levels",
> > +						 pdata->levels,
> > +						 num_levels);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		for (i = 0; i < num_levels; i++)
> > +			pdata->levels[i] = i;
> > +	}
> > +
> > +	pdata->max_brightness = num_levels - 1;
> > +
> > +	ret = of_property_read_u32(node, "default-brightness",
> > +				   &pdata->default_brightness);
> > +	if (ret)
> > +		pdata->default_brightness = pdata->max_brightness;
> > +	if (pdata->default_brightness > pdata->max_brightness) {
> > +		dev_err(chip->dev,
> > +			"default brightness exceeds max brightness\n");
> > +		pdata->default_brightness = pdata->max_brightness;
> > +	}
> > +
> > +	/*
> > +	 * 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 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Flavio Suligoi]

Hi Conor,

...

> On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> > The two properties:
> >
> > - max-brightness
> > - default brightness
> >
> > are not really required, so they can be removed from the "required"
> > section.
> 
> Why are they not required? You need to provide an explanation.

The "max-brightness" is not more used now in the driver (I used it in the first version
of the driver).
The "default-brightness", if omitted in the DT, is managed by the device driver,
using a default value. This depends on the dimming mode used:

- for the "analog mode", via I2C commands, this value is fixed by hardware (=31)
- while in case of pwm mode the default used is the last value of the 
  brightness-levels array.

Also the brightness-levels array is not required; if it is omitted, the driver uses 
a default array of 0..255 and the "default-brightness" is the last one, which is "255".

> > Other changes:
> >
> > - improve the backlight working mode description, in the "description"
> >   section
> 
> > - update the example, removing the "max-brightness" and introducing the
> >   "brightess-levels" property
> 
> Why is this more useful?

I introduced the "brightness-levels" instead of "max-brightness" for homogeneity,
since the "analog mode" dimming has a brightness-levels array fixed by hardware (0..31).
In this way also the "pwm" mode can use the same concepts of array of levels.

> 
> Cheers,
> Conor.

Thanks for your help.
Flavio.

> 
> >
> > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> > ---
> >  .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git
> a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > index 4191e33626f5..527a37368ed7 100644
> > ---
> a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > +++
> b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > @@ -14,8 +14,8 @@ description: |
> >    programmable switching frequency to optimize efficiency.
> >    It supports two different dimming modes:
> >
> > -  - analog mode, via I2C commands (default)
> > -  - PWM controlled mode.
> > +  - analog mode, via I2C commands, as default mode (32 dimming levels)
> > +  - PWM controlled mode (optional)
> >
> >    The datasheet is available at:
> >    https://www.monolithicpower.com/en/mp3309c.html
> > @@ -50,8 +50,6 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - max-brightness
> > -  - default-brightness
> >
> >  unevaluatedProperties: false
> >
> > @@ -66,8 +64,8 @@ examples:
> >              compatible = "mps,mp3309c";
> >              reg = <0x17>;
> >              pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
> > -            max-brightness = <100>;
> > -            default-brightness = <80>;
> > +            brightness-levels = <0 4 8 16 32 64 128 255>;
> > +            default-brightness = <6>;
> >              mps,overvoltage-protection-microvolt = <24000000>;
> >          };
> >      };
> > --
> > 2.34.1
> >

Re: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Conor Dooley]

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

On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote:
> > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> > > The two properties:
> > >
> > > - max-brightness
> > > - default brightness
> > >
> > > are not really required, so they can be removed from the "required"
> > > section.
> > 
> > Why are they not required? You need to provide an explanation.
> 
> The "max-brightness" is not more used now in the driver (I used it in the first version
> of the driver).

If it is not used any more, what happens when someone passes an old
devicetree to the kernel, that contains max-brightness, but not any of
your new properties?

> The "default-brightness", if omitted in the DT, is managed by the device driver,
> using a default value. This depends on the dimming mode used:

For default-brightness, has here always been support in the driver for
the property being omitted, or is this newly added?

> - for the "analog mode", via I2C commands, this value is fixed by hardware (=31)
> - while in case of pwm mode the default used is the last value of the 
>   brightness-levels array.
> 
> Also the brightness-levels array is not required; if it is omitted, the driver uses 
> a default array of 0..255 and the "default-brightness" is the last one, which is "255".

Firstly, this is the sort of rationale that needs to be put into your
commit messages, rather than bullet pointed lists of what you have done.

Secondly, what about other operating systems etc, do any of those support
this platform and depend on presence of these properties?

> 
> > > Other changes:
> > >
> > > - improve the backlight working mode description, in the "description"
> > >   section
> > 
> > > - update the example, removing the "max-brightness" and introducing the
> > >   "brightess-levels" property
> > 
> > Why is this more useful?
> 
> I introduced the "brightness-levels" instead of "max-brightness" for homogeneity,
> since the "analog mode" dimming has a brightness-levels array fixed by hardware (0..31).
> In this way also the "pwm" mode can use the same concepts of array of levels.

What I would like is an explanation in the commit message as to why the
revised example is more helpful than the existing (and
must-remain-valid) one.

Cheers,
Conor.

> > 
> > >
> > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> > > ---
> > >  .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > index 4191e33626f5..527a37368ed7 100644
> > > ---
> > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > +++
> > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > @@ -14,8 +14,8 @@ description: |
> > >    programmable switching frequency to optimize efficiency.
> > >    It supports two different dimming modes:
> > >
> > > -  - analog mode, via I2C commands (default)
> > > -  - PWM controlled mode.
> > > +  - analog mode, via I2C commands, as default mode (32 dimming levels)
> > > +  - PWM controlled mode (optional)
> > >
> > >    The datasheet is available at:
> > >    https://www.monolithicpower.com/en/mp3309c.html
> > > @@ -50,8 +50,6 @@ properties:
> > >  required:
> > >    - compatible
> > >    - reg
> > > -  - max-brightness
> > > -  - default-brightness
> > >
> > >  unevaluatedProperties: false
> > >
> > > @@ -66,8 +64,8 @@ examples:
> > >              compatible = "mps,mp3309c";
> > >              reg = <0x17>;
> > >              pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
> > > -            max-brightness = <100>;
> > > -            default-brightness = <80>;
> > > +            brightness-levels = <0 4 8 16 32 64 128 255>;
> > > +            default-brightness = <6>;
> > >              mps,overvoltage-protection-microvolt = <24000000>;
> > >          };
> > >      };
> > > --
> > > 2.34.1
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Flavio Suligoi]

Hi Conor,

...

> On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote:
> > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> > > > The two properties:
> > > >
> > > > - max-brightness
> > > > - default brightness
> > > >
> > > > are not really required, so they can be removed from the "required"
> > > > section.
> > >
> > > Why are they not required? You need to provide an explanation.
> >
> > The "max-brightness" is not more used now in the driver (I used it in
> > the first version of the driver).
> 
> If it is not used any more, what happens when someone passes an old
> devicetree to the kernel, that contains max-brightness, but not any of your
> new properties?

This is not a problem, because the device driver has not yet been included in any kernel.
My patch for the device driver is still being analyzed by the maintainers.
Only this dt-binding yaml file is already included in the "for-backlight-next" branch
of the "backlight" kernel repository.
At the moment, this driver is used only in a i.MX8MM board produced in my company,
under my full control. No other developer is using it now.

> > The "default-brightness", if omitted in the DT, is managed by the
> > device driver, using a default value. This depends on the dimming mode
> used:
> 
> For default-brightness, has here always been support in the driver for the
> property being omitted, or is this newly added?

In the first version of the driver this property was a "required property",
but nobody has used this driver before, so this should be not a problem.

> 
> > - for the "analog mode", via I2C commands, this value is fixed by
> > hardware (=31)
> > - while in case of pwm mode the default used is the last value of the
> >   brightness-levels array.
> >
> > Also the brightness-levels array is not required; if it is omitted,
> > the driver uses a default array of 0..255 and the "default-brightness" is the
> last one, which is "255".
> 
> Firstly, this is the sort of rationale that needs to be put into your commit
> messages, rather than bullet pointed lists of what you have done.

You are absolutely right, I'll include these details in the next commit message.

> 
> Secondly, what about other operating systems etc, do any of those support
> this platform and depend on presence of these properties?

I used this backlight driver in our i.MX8MM board only, with Linux only.

> 
> >
> > > > Other changes:
> > > >
> > > > - improve the backlight working mode description, in the "description"
> > > >   section
> > >
> > > > - update the example, removing the "max-brightness" and introducing
> the
> > > >   "brightess-levels" property
> > >
> > > Why is this more useful?
> >
> > I introduced the "brightness-levels" instead of "max-brightness" for
> > homogeneity, since the "analog mode" dimming has a brightness-levels array
> fixed by hardware (0..31).
> > In this way also the "pwm" mode can use the same concepts of array of
> levels.
> 
> What I would like is an explanation in the commit message as to why the
> revised example is more helpful than the existing (and
> must-remain-valid) one.

As said before, no one may have ever used this device driver,
so I would leave only this new version of the example.

> 
> Cheers,
> Conor.

Thanks for your help and best regards,
Flavio.

> 
> > >
> > > >
> > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> > > > ---
> > > >  .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++------
> > > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git
> > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > index 4191e33626f5..527a37368ed7 100644
> > > > ---
> > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > +++
> > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > @@ -14,8 +14,8 @@ description: |
> > > >    programmable switching frequency to optimize efficiency.
> > > >    It supports two different dimming modes:
> > > >
> > > > -  - analog mode, via I2C commands (default)
> > > > -  - PWM controlled mode.
> > > > +  - analog mode, via I2C commands, as default mode (32 dimming
> > > > + levels)
> > > > +  - PWM controlled mode (optional)
> > > >
> > > >    The datasheet is available at:
> > > >    https://www.monolithicpower.com/en/mp3309c.html
> > > > @@ -50,8 +50,6 @@ properties:
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > -  - max-brightness
> > > > -  - default-brightness
> > > >
> > > >  unevaluatedProperties: false
> > > >
> > > > @@ -66,8 +64,8 @@ examples:
> > > >              compatible = "mps,mp3309c";
> > > >              reg = <0x17>;
> > > >              pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
> > > > -            max-brightness = <100>;
> > > > -            default-brightness = <80>;
> > > > +            brightness-levels = <0 4 8 16 32 64 128 255>;
> > > > +            default-brightness = <6>;
> > > >              mps,overvoltage-protection-microvolt = <24000000>;
> > > >          };
> > > >      };
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Conor Dooley]

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

On Tue, Oct 24, 2023 at 07:53:38AM +0000, Flavio Suligoi wrote:
> > On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote:
> > > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> > > > > The two properties:
> > > > >
> > > > > - max-brightness
> > > > > - default brightness
> > > > >
> > > > > are not really required, so they can be removed from the "required"
> > > > > section.
> > > >
> > > > Why are they not required? You need to provide an explanation.
> > >
> > > The "max-brightness" is not more used now in the driver (I used it in
> > > the first version of the driver).
> > 
> > If it is not used any more, what happens when someone passes an old
> > devicetree to the kernel, that contains max-brightness, but not any of your
> > new properties?
> 
> This is not a problem, because the device driver has not yet been included in any kernel.
> My patch for the device driver is still being analyzed by the maintainers.
> Only this dt-binding yaml file is already included in the "for-backlight-next" branch
> of the "backlight" kernel repository.
> At the moment, this driver is used only in a i.MX8MM board produced in my company,
> under my full control. No other developer is using it now.

Right. This is exactly the sort of commentary that you need to provide
up front, to have us spent a bunch of time going back and forth to
figure out :(

> > > The "default-brightness", if omitted in the DT, is managed by the
> > > device driver, using a default value. This depends on the dimming mode
> > used:
> > 
> > For default-brightness, has here always been support in the driver for the
> > property being omitted, or is this newly added?
> 
> In the first version of the driver this property was a "required property",
> but nobody has used this driver before, so this should be not a problem.

> > What I would like is an explanation in the commit message as to why the
> > revised example is more helpful than the existing (and
> > must-remain-valid) one.
> 
> As said before, no one may have ever used this device driver,
> so I would leave only this new version of the example.

Okay. Please improve the commit message explaining why it is okay to
make these changes & send a v2.
The alternative is that Lee drops the dt-binding patch & you submit a
revised version of the binding alongside the next iteration of the
driver.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required properties

[Flavio Suligoi]

Hi Conor,

...

> > > > > > The two properties:
> > > > > >
> > > > > > - max-brightness
> > > > > > - default brightness
> > > > > >
> > > > > > are not really required, so they can be removed from the "required"
> > > > > > section.
> > > > >
> > > > > Why are they not required? You need to provide an explanation.
> > > >
> > > > The "max-brightness" is not more used now in the driver (I used it
> > > > in the first version of the driver).
> > >
> > > If it is not used any more, what happens when someone passes an old
> > > devicetree to the kernel, that contains max-brightness, but not any
> > > of your new properties?
> >
> > This is not a problem, because the device driver has not yet been included in
> any kernel.
> > My patch for the device driver is still being analyzed by the maintainers.
> > Only this dt-binding yaml file is already included in the
> > "for-backlight-next" branch of the "backlight" kernel repository.
> > At the moment, this driver is used only in a i.MX8MM board produced in
> > my company, under my full control. No other developer is using it now.
> 
> Right. This is exactly the sort of commentary that you need to provide up
> front, to have us spent a bunch of time going back and forth to figure out :(

I'm sorry for wasting your time, I'll add this information in the next commit.

> 
> > > > The "default-brightness", if omitted in the DT, is managed by the
> > > > device driver, using a default value. This depends on the dimming
> > > > mode
> > > used:
> > >
> > > For default-brightness, has here always been support in the driver
> > > for the property being omitted, or is this newly added?
> >
> > In the first version of the driver this property was a "required
> > property", but nobody has used this driver before, so this should be not a
> problem.
> 
> > > What I would like is an explanation in the commit message as to why
> > > the revised example is more helpful than the existing (and
> > > must-remain-valid) one.
> >
> > As said before, no one may have ever used this device driver, so I
> > would leave only this new version of the example.
> 
> Okay. Please improve the commit message explaining why it is okay to make
> these changes & send a v2.
> The alternative is that Lee drops the dt-binding patch & you submit a revised
> version of the binding alongside the next iteration of the driver.

Ok, I'll send a new commit v2, explaining the reasons for the changes.

> 
> Cheers,
> Conor.

Thank you Conor,
Flavio.