| Message ID | 20230829101546.483189-1-f.suligoi@asem.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/2] dt-bindings: backlight: Add MPS MP3309C | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 29/08/2023 12:15, 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> > --- > .../bindings/leds/backlight/mps,mp3309c.yaml | 202 ++++++++++++++++++ > 1 file changed, 202 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..a58904f2a271 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > @@ -0,0 +1,202 @@ > +# 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 both analog (via I2C commands) and PWM dimming mode. > + > + The datasheet is available at: > + https://www.monolithicpower.com/en/mp3309c.html > + > +properties: > + compatible: > + const: mps,mp3309c-backlight Drop "-backlight". Can it be anything else? > + > + reg: > + maxItems: 1 > + > + mps,dimming-mode: > + description: The dimming mode (PWM or analog by I2C commands). > + $ref: '/schemas/types.yaml#/definitions/string' Drop quotes, you should see warnings for this. It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. > + enum: > + - pwm > + - analog-i2c Why do you think this is a property of a board? Is PWM signal optional? If so, its presence would define it. Otherwise it seems you want to control the driver. > + > + pinctrl-names: > + items: > + - const: default Drop > + > + pinctrl-0: true Drop > + > + pwms: > + description: PWM channel used for controlling the backlight in "pwm" dimming > + mode. > + maxItems: 1 > + > + default-brightness: > + minimum: 0 0 is minimum. Provide rather maximum? or just skip the property. > + > + max-brightness: > + minimum: 1 Same concerns. > + > + enable-gpios: > + description: GPIO used to enable the backlight in "analog-i2c" dimming mode. > + maxItems: 1 > + > + mps,switch-on-delay-ms: > + description: delay (in ms) before switch on the backlight, to wait for image > + stabilization. > + default: 10 > + > + mps,switch-off-delay-ms: > + description: delay (in ms) after the switch off command to the backlight. > + default: 0 > + > + mps,overvoltage-protection-13v: > + description: overvoltage protection set to 13.5V. > + type: boolean > + mps,overvoltage-protection-24v: > + description: overvoltage protection set to 24V. > + type: boolean > + mps,overvoltage-protection-35v: > + description: overvoltage protection set to 35.5V. > + type: boolean Nope for these three. Use -microvolt suffix for one property. > + > + mps,reset-gpios: > + description: optional GPIO to reset an external device (LCD panel, FPGA, > + etc.) when the backlight is switched on. > + maxItems: 1 No, you should not add here GPIOs for other devices. > + > + mps,reset-on-delay-ms: > + description: delay (in s) before generating the reset-gpios. in ms > + default: 10 > + > + mps,reset-on-length-ms: > + description: pulse length (in ms) for reset-gpios. > + default: 10 > + > +oneOf: > + - required: > + - mps,overvoltage-protection-13v > + - required: > + - mps,overvoltage-protection-24v > + - required: > + - mps,overvoltage-protection-35.5v > + > +allOf: > + - $ref: common.yaml# > + - if: > + properties: > + mps,dimming-mode: > + contains: > + enum: > + - pwm > + then: > + required: > + - pwms So this proves the point - mps,dimming-mode looks redundant and not hardware related. > + not: > + required: > + - enable-gpios > + > + - if: > + properties: > + mps,dimming-mode: > + contains: > + enum: > + - analog-i2c > + then: > + required: > + - enable-gpios > + not: > + required: > + - pwms > + > +required: > + - compatible > + - reg > + - mps,dimming-mode > + - max-brightness > + - default-brightness > + > +additionalProperties: false Instead: unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + i2c3 { i2c > + #address-cells = <1>; > + #size-cells = <0>; > + > + clock-frequency = <100000>; Drop > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c3>; > + status = "okay"; Drop all except of cells. > + > + /* Backlight with PWM control */ > + backlight_pwm: backlight@17 { > + compatible = "mps,mp3309c-backlight"; > + reg = <0x17>; > + mps,dimming-mode = "pwm"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_fpga_reset>; > + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > + max-brightness = <100>; > + default-brightness = <80>; > + mps,switch-on-delay-ms = <800>; > + mps,switch-off-delay-ms = <10>; > + mps,overvoltage-protection-24v; > + > + /* > + * Enable an FPGA reset pulse when MIPI data are stable, > + * before switch on the backlight > + */ > + mps,reset-gpios = <&gpio4 20 GPIO_ACTIVE_HIGH>; Nope, nope. FPGA reset pin is not related to this device. > + mps,reset-on-delay-ms = <100>; > + mps,reset-on-length-ms = <10>; > + }; > + }; > + > + - | > + #include <dt-bindings/gpio/gpio.h> > + /* Backlight with analog control via I2C bus */ > + i2c3 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + clock-frequency = <100000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c3>; > + status = "okay"; Drop entire example. It differs by one property - missing pwms. Best regards, Krzysztof
On 29/08/2023 12:15, 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> > --- > MAINTAINERS | 6 + > drivers/video/backlight/Kconfig | 13 + > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/mp3309c_bl.c | 491 +++++++++++++++++++++++++++ > 4 files changed, 511 insertions(+) > create mode 100644 drivers/video/backlight/mp3309c_bl.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3be1bdfe8ecc..895c56ff4f1e 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_bl.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..65d0ac9f611d 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -389,6 +389,19 @@ config BACKLIGHT_LM3639 > help > This supports TI LM3639 Backlight + 1.5A Flash LED Driver > > +config BACKLIGHT_MP3309C Why is this placed between LM and LP entries? Keep things ordered. > + tristate "Backlight Driver for MPS MP3309C" > + depends on I2C > + select REGMAP_I2C > + select NEW_LEDS > + select LEDS_CLASS > + 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_bl. > + > config BACKLIGHT_LP855X > tristate "Backlight driver for TI LP855X" > depends on I2C && PWM > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile > index f72e1c3c59e9..c42c5bccc5ac 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_bl.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_bl.c b/drivers/video/backlight/mp3309c_bl.c > new file mode 100644 > index 000000000000..7cb7a542ceca > --- /dev/null > +++ b/drivers/video/backlight/mp3309c_bl.c > @@ -0,0 +1,491 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * 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> > +#include <linux/workqueue.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 backlight_status { > + FIRST_POWER_ON, > + BACKLIGHT_OFF, > + BACKLIGHT_ON, > +}; > + > +enum dimming_mode_value { > + DIMMING_PWM, > + DIMMING_ANALOG_I2C, > +}; > + > +struct mp3309c_platform_data { > + u32 max_brightness; > + u32 brightness; > + u32 switch_on_delay_ms; > + u32 switch_off_delay_ms; > + u32 reset_on_delay_ms; > + u32 reset_on_length_ms; > + u8 dimming_mode; > + u8 reset_pulse_enable; > + u8 over_voltage_protection; > + > + unsigned int 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; > + > + struct delayed_work enable_work; > + struct delayed_work reset_gpio_work; > + 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 pm3309c_parse_dt_node(struct mp3309c_chip *chip, > + struct mp3309c_platform_data *pdata) This should be just before probe function. It is part of probe path. > +{ > + struct device_node *node = chip->dev->of_node; > + const char *tmp_string; > + int ret; > + > + if (!node) { > + dev_err(chip->dev, "failed to get DT node\n"); > + return -ENODEV; > + } > + > + /* > + * Dimming mode: the MP3309C provides two dimming methods: > + * > + * - PWM mode > + * - Analog by I2C control mode > + */ > + ret = of_property_read_string(node, "mps,dimming-mode", &tmp_string); > + if (ret < 0) { > + dev_err(chip->dev, "missed dimming-mode in DT\n"); > + return ret; > + } > + if (!strcmp(tmp_string, "pwm")) { > + dev_info(chip->dev, "dimming method: PWM\n"); Drop > + pdata->dimming_mode = DIMMING_PWM; > + } > + if (!strcmp(tmp_string, "analog-i2c")) { > + dev_info(chip->dev, "dimming method: analog by I2C commands\n"); Drop > + pdata->dimming_mode = DIMMING_ANALOG_I2C; > + } > + > + /* PWM 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->brightness); > + if (ret < 0) { > + dev_err(chip->dev, > + "failed to get default-brightness from DT\n"); > + return ret; > + } > + > + /* > + * Optional backlight switch-on/off delay > + * > + * Note: set 10ms as minimal value for switch-on delay, to stabilize > + * video data > + */ > + pdata->switch_on_delay_ms = 50; > + of_property_read_u32(node, "mps,switch-on-delay-ms", > + &pdata->switch_on_delay_ms); > + if (pdata->switch_on_delay_ms < 10) { > + pdata->switch_on_delay_ms = 10; You miss constraints in the bindings. > + dev_warn(chip->dev, > + "switch-on-delay-ms set to 10ms as minimal value\n"); > + } > + pdata->switch_off_delay_ms = 0; > + of_property_read_u32(node, "mps,switch-off-delay-ms", > + &pdata->switch_off_delay_ms); > + > + /* > + * Reset: GPIO, initial delay and pulse length > + * > + * Use this optional GPIO to reset an external device (LCD panel, video > + * FPGA, etc) when the backlight is switched on > + */ > + pdata->reset_pulse_enable = 0; > + chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "mps,reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(chip->reset_gpio)) { > + ret = PTR_ERR(chip->reset_gpio); > + dev_err(chip->dev, "error acquiring reset gpio: %d\n", ret); > + return ret; return dev_err_probe() > + } > + if (chip->reset_gpio) { > + pdata->reset_pulse_enable = 1; > + > + pdata->reset_on_delay_ms = 10; > + of_property_read_u32(node, "mps,reset-on-delay-ms", > + &pdata->reset_on_delay_ms); > + pdata->reset_on_length_ms = 10; > + of_property_read_u32(node, "mps,reset-on-length-ms", > + &pdata->reset_on_length_ms); > + } > + > + /* > + * Over-voltage protection (OVP) > + * > + * These (optional) properties are: > + * > + * - overvoltage-protection-13v --> OVP point set to 13.5V > + * - overvoltage-protection-24v --> OVP point set to 24V > + * - overvoltage-protection-35v --> OVP point set to 35.5V > + * > + * If not chosen, the hw default value for OVP is 35.5V > + */ > + pdata->over_voltage_protection = REG_I2C_1_OVP1; > + if (of_property_read_bool(node, "mps,overvoltage-protection-13v")) > + pdata->over_voltage_protection = 0x00; > + if (of_property_read_bool(node, "mps,overvoltage-protection-24v")) > + pdata->over_voltage_protection = REG_I2C_1_OVP0; > + if (of_property_read_bool(node, "mps,overvoltage-protection-35v")) > + pdata->over_voltage_protection = REG_I2C_1_OVP1; > + > + return 0; > +} > + ... > + > +static const struct backlight_ops mp3309c_bl_ops = { > + .update_status = mp3309c_bl_update_status, > +}; > + > +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; > + 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(struct mp3309c_chip), sizeof(*) > + GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + chip->dev = &client->dev; > + > + chip->regmap = devm_regmap_init_i2c(client, &mp3309c_regmap); > + if (IS_ERR(chip->regmap)) { > + ret = PTR_ERR(chip->regmap); > + dev_err(&client->dev, "failed to allocate register map\n"); Are you sure regmap allocation should be followed by error message? Allocations must not... so if this is not allocation, then syntax is: return dev_err_probe() > + return ret; > + } > + > + i2c_set_clientdata(client, chip); > + > + if (!pdata) { > + pdata = devm_kzalloc(chip->dev, > + sizeof(struct mp3309c_platform_data), sizeof(*) > + GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + ret = pm3309c_parse_dt_node(chip, pdata); > + if (ret) { > + dev_err(&client->dev, "failed parsing DT node\n"); Why do you print errors multiple times? parse_dt_node already does it. > + return ret; > + } > + } > + chip->pdata = pdata; > + > + chip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", > + GPIOD_OUT_HIGH); > + if (IS_ERR(chip->enable_gpio)) { > + ret = PTR_ERR(chip->enable_gpio); > + return ret; return dev_err_probe() > + } > + > + /* Backlight */ > + props.type = BACKLIGHT_RAW; > + props.brightness = pdata->brightness; > + props.max_brightness = pdata->max_brightness; > + props.scale = BACKLIGHT_SCALE_LINEAR; > + chip->bl = > + devm_backlight_device_register(chip->dev, "mp3309c_bl", > + chip->dev, chip, &mp3309c_bl_ops, > + &props); > + if (IS_ERR(chip->bl)) { > + dev_err(&client->dev, "failed registering backlight\n"); > + return PTR_ERR(chip->bl); return dev_err_probe() > + } > + pdata->status = FIRST_POWER_ON; > + > + /* Enable PWM, if required */ > + if (pdata->dimming_mode == DIMMING_PWM) { > + chip->pwmd = devm_pwm_get(chip->dev, NULL); > + if (IS_ERR(chip->pwmd)) { > + dev_err(&client->dev, "failed getting pwm device\n"); > + return PTR_ERR(chip->pwmd); return dev_err_probe() > + } > + pwm_apply_args(chip->pwmd); > + } > + > + /* > + * Workqueue for delayed backlight enabling > + */ > + INIT_DELAYED_WORK(&chip->enable_work, mp3309c_enable); > + > + /* > + * Workqueue for (optional) external device GPIO reset > + */ > + if (pdata->reset_pulse_enable) { > + dev_info(&client->dev, "reset pulse enabled\n"); Drop, not really helpful. > + INIT_DELAYED_WORK(&chip->reset_gpio_work, mp3309c_reset_gpio); > + } > + > + dev_info(&client->dev, "MP3309C backlight initialized"); Drop simple tracing messages. > + return 0; > +} > + > +static int mp3309c_backlight_switch_off(struct pwm_device *pwmd) > +{ > + struct pwm_state pwmstate; > + > + /* Switch-off the backlight */ > + pwm_get_state(pwmd, &pwmstate); > + pwmstate.duty_cycle = 0; > + pwmstate.enabled = false; > + pwm_apply_state(pwmd, &pwmstate); > + > + return 0; > +} > + > +static void mp3309c_remove(struct i2c_client *client) > +{ > + struct mp3309c_chip *chip = i2c_get_clientdata(client); > + > + if (chip->pdata->dimming_mode == DIMMING_PWM) > + mp3309c_backlight_switch_off(chip->pwmd); > + if (chip->pdata->reset_pulse_enable) > + cancel_delayed_work(&chip->reset_gpio_work); > +} > + > +static void mp3309c_shutdown(struct i2c_client *client) > +{ > + struct mp3309c_chip *chip = i2c_get_clientdata(client); > + > + if (chip->pdata->dimming_mode == DIMMING_PWM) > + mp3309c_backlight_switch_off(chip->pwmd); Why do you need shutdown function? > +} > + > +static const struct of_device_id mp3309c_match_table[] = { > + { .compatible = "mps,mp3309c-backlight", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, mp3309c_match_table); > + Best regards, Krzysztof
Hi Krzysztof, Thanks for your quick replay and corrections! Just some questions about some of your remarks: > > @@ -0,0 +1,202 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 > > +--- > > > + > > + reg: > > + maxItems: 1 > > + > > + mps,dimming-mode: > > + description: The dimming mode (PWM or analog by I2C commands). > > + $ref: '/schemas/types.yaml#/definitions/string' > > Drop quotes, you should see warnings for this. > > It does not look like you tested the bindings, at least after quick look. Please > run `make dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > Maybe you need to update your dtschema and yamllint. > > > + enum: > > + - pwm > > + - analog-i2c > > Why do you think this is a property of a board? Is PWM signal optional? > If so, its presence would define it. Otherwise it seems you want to control the > driver. > The MP3309C device always need a I2C bus to rd/wr its internal registers. But the brightness can be controlled in one of the following ways (mutually exclusive, but mandatory): - a PWM input signal or - a I2C command So, the driver needs a property to select the dimming mode used; this property is mandatory. This is the reason of the existence of the ' mps,dimming-mode' property. PWM signal is not optional, it is required if and only if the 'pwm' dimming mode is used. If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not be used. So the property 'mps,dimming-mode' controls how the MP3309C is used. I can add more details about this in the description section. ... > > + > > + mps,overvoltage-protection-13v: > > + description: overvoltage protection set to 13.5V. > > + type: boolean > > + mps,overvoltage-protection-24v: > > + description: overvoltage protection set to 24V. > > + type: boolean > > + mps,overvoltage-protection-35v: > > + description: overvoltage protection set to 35.5V. > > + type: boolean > > Nope for these three. Use -microvolt suffix for one property. Ok > > > + > > + mps,reset-gpios: > > + description: optional GPIO to reset an external device (LCD panel, FPGA, > > + etc.) when the backlight is switched on. > > + maxItems: 1 > > No, you should not add here GPIOs for other devices. Do you mean that I have to remove this property or that I have to move it somewhere else? I added this feature because sometimes, in embedded boards, you need a pulse signal to use after the backlight probing, for example to reset another device in sync with the backlight probe. Do you think I have to remove this feature from the driver? ... > > +allOf: > > + - $ref: common.yaml# > > + - if: > > + properties: > > + mps,dimming-mode: > > + contains: > > + enum: > > + - pwm > > + then: > > + required: > > + - pwms > > So this proves the point - mps,dimming-mode looks redundant and not > hardware related. See my previous comment. > > > + not: > > + required: > > + - enable-gpios > > + > > + - if: > > + properties: > > + mps,dimming-mode: > > + contains: > > + enum: > > + - analog-i2c > > + then: > > + required: > > + - enable-gpios > > + not: > > + required: > > + - pwms > > + > > +required: > > + - compatible > > + - reg > > + - mps,dimming-mode > > + - max-brightness > > + - default-brightness > > + > > +additionalProperties: false > > Instead: > unevaluatedProperties: false > Ok > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + i2c3 { > > i2c > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + clock-frequency = <100000>; > > Drop > > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c3>; > > + status = "okay"; > > Drop all except of cells. Ok > > > + > > + /* Backlight with PWM control */ > > + backlight_pwm: backlight@17 { > > + compatible = "mps,mp3309c-backlight"; > > + reg = <0x17>; > > + mps,dimming-mode = "pwm"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_fpga_reset>; > > + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > > + max-brightness = <100>; > > + default-brightness = <80>; > > + mps,switch-on-delay-ms = <800>; > > + mps,switch-off-delay-ms = <10>; > > + mps,overvoltage-protection-24v; > > + > > + /* > > + * Enable an FPGA reset pulse when MIPI data are stable, > > + * before switch on the backlight > > + */ > > + mps,reset-gpios = <&gpio4 20 GPIO_ACTIVE_HIGH>; > > Nope, nope. FPGA reset pin is not related to this device. See my previous comment/question about this feature. > > > + mps,reset-on-delay-ms = <100>; > > + mps,reset-on-length-ms = <10>; > > + }; > > + }; > > + > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + /* Backlight with analog control via I2C bus */ > > + i2c3 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + clock-frequency = <100000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c3>; > > + status = "okay"; > > Drop entire example. It differs by one property - missing pwms. Ok > > > Best regards, > Krzysztof Thanks and best regards, Flavio
On Tue, Aug 29, 2023 at 12:15:46PM +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> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index 51387b1ef012..65d0ac9f611d 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -389,6 +389,19 @@ config BACKLIGHT_LM3639 > help > This supports TI LM3639 Backlight + 1.5A Flash LED Driver > > +config BACKLIGHT_MP3309C > + tristate "Backlight Driver for MPS MP3309C" > + depends on I2C > + select REGMAP_I2C > + select NEW_LEDS > + select LEDS_CLASS This doesn't seem right. Shouldn't PWM and GPIOLIB be listed here? Why are NEW_LEDS and LEDS_CLASS needed? > + 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_bl. > + > config BACKLIGHT_LP855X > tristate "Backlight driver for TI LP855X" > depends on I2C && PWM > +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_init_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; > + } else { > + /* > + * Analog dimming mode by I2C commands > + * > + * 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) { > + switch (chip->pdata->status) { > + case FIRST_POWER_ON: > + /* > + * Only for the first time, wait for the optional > + * switch-on delay and then enable the device. > + * Otherwise enable the backlight immediately. > + */ > + schedule_delayed_work(&chip->enable_work, > + msecs_to_jiffies(chip->pdata->switch_on_delay_ms)); Delaying this work makes no sense to me, especially when it is only going to happen at initial power on. If you are (ab)using this property to try and sequence the backlight power-on with display initialization then this is not the way to do it. Normally backlight drivers that support sequencing versus the panel work by having a backlight property set on the panel linking it to the backlight. When the panel is ready this power status of the backlight will be updated accordingly. All the backlight driver need do is make sure that is the initial power status is "powerdown" on systems when the link is present (e.g. leave the backlight off and wait to be told the display has settled). > + /* > + * Optional external device GPIO reset, with > + * delay pulse length > + */ > + if (chip->pdata->reset_pulse_enable) > + schedule_delayed_work(&chip->reset_gpio_work, > + msecs_to_jiffies(chip->pdata->reset_on_delay_ms)); Similarly I don't understand what this property is for. A backlight is directly perceivable by the user. There is nothing downstream of a light that needs to be reset! What is this used for? Daniel.
On 29/08/2023 16:18, Flavio Suligoi wrote: > Hi Krzysztof, > > Thanks for your quick replay and corrections! > Just some questions about some of your remarks: > >>> @@ -0,0 +1,202 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 >>> +--- >> >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + mps,dimming-mode: >>> + description: The dimming mode (PWM or analog by I2C commands). >>> + $ref: '/schemas/types.yaml#/definitions/string' >> >> Drop quotes, you should see warnings for this. >> >> It does not look like you tested the bindings, at least after quick look. Please >> run `make dt_binding_check` (see >> Documentation/devicetree/bindings/writing-schema.rst for instructions). >> Maybe you need to update your dtschema and yamllint. >> >>> + enum: >>> + - pwm >>> + - analog-i2c >> >> Why do you think this is a property of a board? Is PWM signal optional? >> If so, its presence would define it. Otherwise it seems you want to control the >> driver. >> > > The MP3309C device always need a I2C bus to rd/wr its internal registers. > But the brightness can be controlled in one of the following ways (mutually exclusive, > but mandatory): > - a PWM input signal > or > - a I2C command > So, the driver needs a property to select the dimming mode used; this property is mandatory. No, it's not a proof. Don't mix properties and hardware signals. > This is the reason of the existence of the ' mps,dimming-mode' property. > PWM signal is not optional, it is required if and only if the 'pwm' dimming mode is used. So the pwms determine the mode. That's it, no need for this property. > If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not be used. > So the property 'mps,dimming-mode' controls how the MP3309C is used. > I can add more details about this in the description section. No, drop the property or explain more, e.g. is I2C mode of control used while having PWMs signals connected? > ... > >>> + >>> + mps,overvoltage-protection-13v: >>> + description: overvoltage protection set to 13.5V. >>> + type: boolean >>> + mps,overvoltage-protection-24v: >>> + description: overvoltage protection set to 24V. >>> + type: boolean >>> + mps,overvoltage-protection-35v: >>> + description: overvoltage protection set to 35.5V. >>> + type: boolean >> >> Nope for these three. Use -microvolt suffix for one property. > > Ok > >> >>> + >>> + mps,reset-gpios: >>> + description: optional GPIO to reset an external device (LCD panel, FPGA, >>> + etc.) when the backlight is switched on. >>> + maxItems: 1 >> >> No, you should not add here GPIOs for other devices. > > Do you mean that I have to remove this property or that I have to move it somewhere else? > I added this feature because sometimes, in embedded boards, you need a pulse signal to How you described it, this is not the property of this device. > use after the backlight probing, for example to reset another device in sync with the backlight > probe. There is no term as "probe" in hardware, so you describe drivers. > Do you think I have to remove this feature from the driver? You cannot request GPIO after removing it from the bindings, obviously, but whether your backlight should reset something else? Don't care, don't know. I talk about bindings. > > ... > >>> +allOf: >>> + - $ref: common.yaml# >>> + - if: >>> + properties: >>> + mps,dimming-mode: >>> + contains: >>> + enum: >>> + - pwm >>> + then: >>> + required: >>> + - pwms >> >> So this proves the point - mps,dimming-mode looks redundant and not >> hardware related. > > See my previous comment. No, it still proves the point till you explain why pwms cannot be used to determine this. Read my messages. Best regards, Krzysztof
Hi Krzysztof, ... > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + mps,dimming-mode: > >>> + description: The dimming mode (PWM or analog by I2C commands). > >>> + $ref: '/schemas/types.yaml#/definitions/string' > >> > >> Drop quotes, you should see warnings for this. > >> > >> It does not look like you tested the bindings, at least after quick > >> look. Please run `make dt_binding_check` (see > >> Documentation/devicetree/bindings/writing-schema.rst for instructions). > >> Maybe you need to update your dtschema and yamllint. > >> > >>> + enum: > >>> + - pwm > >>> + - analog-i2c > >> > >> Why do you think this is a property of a board? Is PWM signal optional? > >> If so, its presence would define it. Otherwise it seems you want to > >> control the driver. > >> > > > > The MP3309C device always need a I2C bus to rd/wr its internal registers. > > But the brightness can be controlled in one of the following ways > > (mutually exclusive, but mandatory): > > - a PWM input signal > > or > > - a I2C command > > So, the driver needs a property to select the dimming mode used; this > property is mandatory. > > No, it's not a proof. Don't mix properties and hardware signals. > > > This is the reason of the existence of the ' mps,dimming-mode' property. > > PWM signal is not optional, it is required if and only if the 'pwm' dimming > mode is used. > > So the pwms determine the mode. That's it, no need for this property. > > > > If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not > be used. > > So the property 'mps,dimming-mode' controls how the MP3309C is used. > > I can add more details about this in the description section. > > No, drop the property or explain more, e.g. is I2C mode of control used while > having PWMs signals connected? Ok, I think I understand what you mean: since the I2C bus is always present to configure the chip, the 'analog-i2c' dimming mode is the default mode. The other type of dimming mode, the 'pwm' dimming mode, is an option. When the pwm is present in the DT, the pwm signal is used as dimming control mode instead of the i2c dimming control mode. In this way the ' mps,dimming-mode' property is not more necessary and can be eliminated. > > > ... > > > >>> + > >>> + mps,overvoltage-protection-13v: > >>> + description: overvoltage protection set to 13.5V. > >>> + type: boolean > >>> + mps,overvoltage-protection-24v: > >>> + description: overvoltage protection set to 24V. > >>> + type: boolean > >>> + mps,overvoltage-protection-35v: > >>> + description: overvoltage protection set to 35.5V. > >>> + type: boolean > >> > >> Nope for these three. Use -microvolt suffix for one property. > > > > Ok > > > >> > >>> + > >>> + mps,reset-gpios: > >>> + description: optional GPIO to reset an external device (LCD panel, > FPGA, > >>> + etc.) when the backlight is switched on. > >>> + maxItems: 1 > >> > >> No, you should not add here GPIOs for other devices. > > > > Do you mean that I have to remove this property or that I have to move it > somewhere else? > > I added this feature because sometimes, in embedded boards, you need a > > pulse signal to > > How you described it, this is not the property of this device. > > > use after the backlight probing, for example to reset another device > > in sync with the backlight probe. > > There is no term as "probe" in hardware, so you describe drivers. > > > Do you think I have to remove this feature from the driver? > > You cannot request GPIO after removing it from the bindings, obviously, but > whether your backlight should reset something else? Don't care, don't know. I > talk about bindings. This GPIO was used in one of our boards to solve a sync problem, but it is no more necessary. I'll eliminate it, thanks. > > > > > ... > > > >>> +allOf: > >>> + - $ref: common.yaml# > >>> + - if: > >>> + properties: > >>> + mps,dimming-mode: > >>> + contains: > >>> + enum: > >>> + - pwm > >>> + then: > >>> + required: > >>> + - pwms > >> > >> So this proves the point - mps,dimming-mode looks redundant and not > >> hardware related. > > > > See my previous comment. > > No, it still proves the point till you explain why pwms cannot be used to > determine this. Read my messages. > > Best regards, > Krzysztof Thanks for your help! Flavio
HI Daniel, > -----Original Message----- > From: Daniel Thompson <daniel.thompson@linaro.org> > Sent: Tuesday, August 29, 2023 6:28 PM > To: Flavio Suligoi <f.suligoi@asem.it> > Cc: Lee Jones <lee@kernel.org>; Jingoo Han <jingoohan1@gmail.com>; Helge > Deller <deller@gmx.de>; Pavel Machek <pavel@ucw.cz>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > dri-devel@lists.freedesktop.org; linux-leds@vger.kernel.org; > devicetree@vger.kernel.org; linux-fbdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v1 2/2] backlight: mp3309c: Add support for MPS > MP3309C > > On Tue, Aug 29, 2023 at 12:15:46PM +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> > > > diff --git a/drivers/video/backlight/Kconfig > > b/drivers/video/backlight/Kconfig index 51387b1ef012..65d0ac9f611d > > 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -389,6 +389,19 @@ config BACKLIGHT_LM3639 > > help > > This supports TI LM3639 Backlight + 1.5A Flash LED Driver > > > > +config BACKLIGHT_MP3309C > > + tristate "Backlight Driver for MPS MP3309C" > > + depends on I2C > > + select REGMAP_I2C > > + select NEW_LEDS > > + select LEDS_CLASS > > This doesn't seem right. > > Shouldn't PWM and GPIOLIB be listed here? Why are NEW_LEDS and > LEDS_CLASS needed? ok, I'll fix it > > > + 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_bl. > > + > > config BACKLIGHT_LP855X > > tristate "Backlight driver for TI LP855X" > > depends on I2C && PWM > > > +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_init_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; > > + } else { > > + /* > > + * Analog dimming mode by I2C commands > > + * > > + * 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) { > > + switch (chip->pdata->status) { > > + case FIRST_POWER_ON: > > + /* > > + * Only for the first time, wait for the optional > > + * switch-on delay and then enable the device. > > + * Otherwise enable the backlight immediately. > > + */ > > + schedule_delayed_work(&chip->enable_work, > > + msecs_to_jiffies(chip->pdata- > >switch_on_delay_ms)); > > Delaying this work makes no sense to me, especially when it is only going to > happen at initial power on. > > If you are (ab)using this property to try and sequence the backlight power-on > with display initialization then this is not the way to do it. > Normally backlight drivers that support sequencing versus the panel work by > having a backlight property set on the panel linking it to the backlight. When > the panel is ready this power status of the backlight will be updated > accordingly. > > All the backlight driver need do is make sure that is the initial power status is > "powerdown" on systems when the link is present (e.g. > leave the backlight off and wait to be told the display has settled). OK, I'll remove this delay. It was used for one of our boards, as a workaround. > > > > + /* > > + * Optional external device GPIO reset, with > > + * delay pulse length > > + */ > > + if (chip->pdata->reset_pulse_enable) > > + schedule_delayed_work(&chip- > >reset_gpio_work, > > + msecs_to_jiffies(chip- > >pdata->reset_on_delay_ms)); > > Similarly I don't understand what this property is for. A backlight is directly > perceivable by the user. There is nothing downstream of a light that needs to > be reset! > > What is this used for? Also this property, this gpio, was used for one of our boards. It is not necessary, I'll remove it. > > > Daniel. Thanks, Daniel! Flavio
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..a58904f2a271 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml @@ -0,0 +1,202 @@ +# 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 both analog (via I2C commands) and PWM dimming mode. + + The datasheet is available at: + https://www.monolithicpower.com/en/mp3309c.html + +properties: + compatible: + const: mps,mp3309c-backlight + + reg: + maxItems: 1 + + mps,dimming-mode: + description: The dimming mode (PWM or analog by I2C commands). + $ref: '/schemas/types.yaml#/definitions/string' + enum: + - pwm + - analog-i2c + + pinctrl-names: + items: + - const: default + + pinctrl-0: true + + pwms: + description: PWM channel used for controlling the backlight in "pwm" dimming + mode. + maxItems: 1 + + default-brightness: + minimum: 0 + + max-brightness: + minimum: 1 + + enable-gpios: + description: GPIO used to enable the backlight in "analog-i2c" dimming mode. + maxItems: 1 + + mps,switch-on-delay-ms: + description: delay (in ms) before switch on the backlight, to wait for image + stabilization. + default: 10 + + mps,switch-off-delay-ms: + description: delay (in ms) after the switch off command to the backlight. + default: 0 + + mps,overvoltage-protection-13v: + description: overvoltage protection set to 13.5V. + type: boolean + mps,overvoltage-protection-24v: + description: overvoltage protection set to 24V. + type: boolean + mps,overvoltage-protection-35v: + description: overvoltage protection set to 35.5V. + type: boolean + + mps,reset-gpios: + description: optional GPIO to reset an external device (LCD panel, FPGA, + etc.) when the backlight is switched on. + maxItems: 1 + + mps,reset-on-delay-ms: + description: delay (in s) before generating the reset-gpios. + default: 10 + + mps,reset-on-length-ms: + description: pulse length (in ms) for reset-gpios. + default: 10 + +oneOf: + - required: + - mps,overvoltage-protection-13v + - required: + - mps,overvoltage-protection-24v + - required: + - mps,overvoltage-protection-35.5v + +allOf: + - $ref: common.yaml# + - if: + properties: + mps,dimming-mode: + contains: + enum: + - pwm + then: + required: + - pwms + not: + required: + - enable-gpios + + - if: + properties: + mps,dimming-mode: + contains: + enum: + - analog-i2c + then: + required: + - enable-gpios + not: + required: + - pwms + +required: + - compatible + - reg + - mps,dimming-mode + - max-brightness + - default-brightness + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + i2c3 { + #address-cells = <1>; + #size-cells = <0>; + + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c3>; + status = "okay"; + + /* Backlight with PWM control */ + backlight_pwm: backlight@17 { + compatible = "mps,mp3309c-backlight"; + reg = <0x17>; + mps,dimming-mode = "pwm"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_fpga_reset>; + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ + max-brightness = <100>; + default-brightness = <80>; + mps,switch-on-delay-ms = <800>; + mps,switch-off-delay-ms = <10>; + mps,overvoltage-protection-24v; + + /* + * Enable an FPGA reset pulse when MIPI data are stable, + * before switch on the backlight + */ + mps,reset-gpios = <&gpio4 20 GPIO_ACTIVE_HIGH>; + mps,reset-on-delay-ms = <100>; + mps,reset-on-length-ms = <10>; + }; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + /* Backlight with analog control via I2C bus */ + i2c3 { + #address-cells = <1>; + #size-cells = <0>; + + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c3>; + status = "okay"; + + backlight_analog_i2c: backlight@18 { + compatible = "mps,mp3309c-backlight"; + reg = <0x17>; + mps,dimming-mode = "analog-i2c"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_reg_pwm_backlight_fixed>, + <&pinctrl_fpga_reset>; + enable-gpios = <&gpio5 5 GPIO_ACTIVE_HIGH>; + max-brightness = <100>; + default-brightness = <80>; + mps,switch-on-delay-ms = <800>; + mps,switch-off-delay-ms = <10>; + mps,overvoltage-protection-24v; + + /* + * Enable an FPGA reset pulse when MIPI data are stable, + * before switch on the backlight + */ + mps,reset-gpios = <&gpio4 20 GPIO_ACTIVE_HIGH>; + mps,reset-on-delay-ms = <100>; + mps,reset-on-length-ms = <10>; + }; + };
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> --- .../bindings/leds/backlight/mps,mp3309c.yaml | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml