| Message ID | 20230915140516.1294925-1-f.suligoi@asem.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] dt-bindings: backlight: Add MPS MP3309C | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Fri, 15 Sep 2023 16:05:15 +0200, Flavio Suligoi wrote: > The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > programmable switching frequency to optimize efficiency. > The brightness can be controlled either by I2C commands (called "analog" > mode) or by a PWM input signal (PWM mode). > This driver supports both modes. > > For device driver details, please refer to: > - drivers/video/backlight/mp3309c_bl.c > > The datasheet is available at: > - https://www.monolithicpower.com/en/mp3309c.html > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > --- > > v2: > - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0, > switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms, > reset-on-length-ms) > - add common.yaml# > - remove already included properties (default-brightness, max-brightness) > - substitute three boolean properties, used for the overvoltage-protection > values, with a single enum property > - remove some conditional definitions > - remove the 2nd example > v1: > - first version > > .../bindings/leds/backlight/mps,mp3309c.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.example.dtb: /example-0/i2c/backlight@17: failed to match any schema with compatible: ['mps,mp3309c-backlight'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230915140516.1294925-1-f.suligoi@asem.it The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Yo, On Fri, Sep 15, 2023 at 04:05:15PM +0200, Flavio Suligoi wrote: > The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > programmable switching frequency to optimize efficiency. > The brightness can be controlled either by I2C commands (called "analog" > mode) or by a PWM input signal (PWM mode). > This driver supports both modes. > > For device driver details, please refer to: > - drivers/video/backlight/mp3309c_bl.c > > The datasheet is available at: > - https://www.monolithicpower.com/en/mp3309c.html > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > --- > > v2: > - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0, > switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms, > reset-on-length-ms) > - add common.yaml# > - remove already included properties (default-brightness, max-brightness) > - substitute three boolean properties, used for the overvoltage-protection > values, with a single enum property > - remove some conditional definitions > - remove the 2nd example > v1: > - first version > > .../bindings/leds/backlight/mps,mp3309c.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > new file mode 100644 > index 000000000000..99ccdba2c08f > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MPS MP3309C backlight > + > +maintainers: > + - Flavio Suligoi <f.suligoi@asem.it> > + > +description: | > + The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > + programmable switching frequency to optimize efficiency. > + It supports two different dimming modes: > + > + - analog mode, via I2C commands (default) > + - PWM controlled mode. > + > + The datasheet is available at: > + https://www.monolithicpower.com/en/mp3309c.html > + > +allOf: > + - $ref: common.yaml# > + > +properties: > + compatible: > + const: mps,mp3309c > + > + reg: > + maxItems: 1 > + > + pwms: > + description: if present, the backlight is controlled in PWM mode. > + maxItems: 1 > + > + enable-gpios: > + description: GPIO used to enable the backlight in "analog-i2c" dimming mode. > + maxItems: 1 > + > + mps,overvoltage-protection-microvolt: > + description: Overvoltage protection (13.5V, 24V or 35.5V). If missing, the > + hardware default of 35.5V is used. > + enum: [ 13500000, 24000000, 35500000 ] You can add "default: 35500000" and drop the free form default as text in the description. Cheers, Conor. > + > + mps,no-sync-mode: > + description: disable synchronous rectification mode > + type: boolean > + > +required: > + - compatible > + - reg > + - max-brightness > + - default-brightness > + > +unevaluatedProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* Backlight with PWM control */ > + backlight_pwm: backlight@17 { > + compatible = "mps,mp3309c-backlight"; As the bot pointed out, the compatible doesn't contain "backlight". > + reg = <0x17>; > + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > + max-brightness = <100>; > + default-brightness = <80>; > + overvoltage-protection-microvolt = <24000000>; > + }; > + }; > -- > 2.34.1 >
On Fri, Sep 15, 2023 at 04:05:15PM +0200, Flavio Suligoi wrote: > The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > programmable switching frequency to optimize efficiency. > The brightness can be controlled either by I2C commands (called "analog" > mode) or by a PWM input signal (PWM mode). > This driver supports both modes. > > For device driver details, please refer to: > - drivers/video/backlight/mp3309c_bl.c > > The datasheet is available at: > - https://www.monolithicpower.com/en/mp3309c.html > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > --- > > v2: > - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0, > switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms, > reset-on-length-ms) > - add common.yaml# > - remove already included properties (default-brightness, max-brightness) > - substitute three boolean properties, used for the overvoltage-protection > values, with a single enum property > - remove some conditional definitions > - remove the 2nd example > v1: > - first version > > .../bindings/leds/backlight/mps,mp3309c.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > new file mode 100644 > index 000000000000..99ccdba2c08f > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MPS MP3309C backlight > + > +maintainers: > + - Flavio Suligoi <f.suligoi@asem.it> > + > +description: | > + The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > + programmable switching frequency to optimize efficiency. > + It supports two different dimming modes: > + > + - analog mode, via I2C commands (default) > + - PWM controlled mode. > + > + The datasheet is available at: > + https://www.monolithicpower.com/en/mp3309c.html > + > +allOf: > + - $ref: common.yaml# > + > +properties: > + compatible: > + const: mps,mp3309c > + > + reg: > + maxItems: 1 > + > + pwms: > + description: if present, the backlight is controlled in PWM mode. > + maxItems: 1 > + > + enable-gpios: > + description: GPIO used to enable the backlight in "analog-i2c" dimming mode. > + maxItems: 1 > + > + mps,overvoltage-protection-microvolt: > + description: Overvoltage protection (13.5V, 24V or 35.5V). If missing, the > + hardware default of 35.5V is used. default: 35500000 instead of prose saying the same thing. With that, Reviewed-by: Rob Herring <robh@kernel.org> > + enum: [ 13500000, 24000000, 35500000 ] > + > + mps,no-sync-mode: > + description: disable synchronous rectification mode > + type: boolean > + > +required: > + - compatible > + - reg > + - max-brightness > + - default-brightness > + > +unevaluatedProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* Backlight with PWM control */ > + backlight_pwm: backlight@17 { > + compatible = "mps,mp3309c-backlight"; > + reg = <0x17>; > + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > + max-brightness = <100>; > + default-brightness = <80>; > + overvoltage-protection-microvolt = <24000000>; > + }; > + }; > -- > 2.34.1 >
Hi Conor, ... > On Fri, Sep 15, 2023 at 04:05:15PM +0200, Flavio Suligoi wrote: > > The Monolithic Power (MPS) MP3309C is a WLED step-up converter, > > featuring a programmable switching frequency to optimize efficiency. > > The brightness can be controlled either by I2C commands (called "analog" > > mode) or by a PWM input signal (PWM mode). > > This driver supports both modes. > > > > For device driver details, please refer to: > > - drivers/video/backlight/mp3309c_bl.c > > > > The datasheet is available at: > > - https://www.monolithicpower.com/en/mp3309c.html > > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > > --- > > > > v2: > > - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0, > > switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms, > > reset-on-length-ms) > > - add common.yaml# > > - remove already included properties (default-brightness, > > max-brightness) > > - substitute three boolean properties, used for the overvoltage-protection > > values, with a single enum property > > - remove some conditional definitions > > - remove the 2nd example > > v1: > > - first version > > > > .../bindings/leds/backlight/mps,mp3309c.yaml | 73 > > +++++++++++++++++++ > > 1 file changed, 73 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > new file mode 100644 > > index 000000000000..99ccdba2c08f > > --- /dev/null > > +++ > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yam > > +++ l > > @@ -0,0 +1,73 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: MPS MP3309C backlight > > + > > +maintainers: > > + - Flavio Suligoi <f.suligoi@asem.it> ... > > + mps,overvoltage-protection-microvolt: > > + description: Overvoltage protection (13.5V, 24V or 35.5V). If missing, the > > + hardware default of 35.5V is used. > > + enum: [ 13500000, 24000000, 35500000 ] > You can add "default: 35500000" and drop the free form default as text in the > description. Ok, thanks. > > Cheers, > Conor. > > > + > > + mps,no-sync-mode: > > + description: disable synchronous rectification mode > > + type: boolean > > + > > +required: > > + - compatible > > + - reg > > + - max-brightness > > + - default-brightness > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + /* Backlight with PWM control */ > > + backlight_pwm: backlight@17 { > > + compatible = "mps,mp3309c-backlight"; > > As the bot pointed out, the compatible doesn't contain "backlight". Right, fixed! Thanks and regards, Flavio
HI Rob, ... > Subject: Re: [PATCH v2 1/2] dt-bindings: backlight: Add MPS MP3309C > ... > > + > > + mps,overvoltage-protection-microvolt: > > + description: Overvoltage protection (13.5V, 24V or 35.5V). If missing, the > > + hardware default of 35.5V is used. > > default: 35500000 > > instead of prose saying the same thing. > > With that, > > Reviewed-by: Rob Herring <robh@kernel.org> Ok, right, I'll fix it in my next version. I prefer to wait a few days before sending the next version, to wait if there are other comments on the device driver file as well. Just a question about the procedure: in my next version, I have to add the sentence: "Reviewed-by: Rob Herring <robh@kernel.org>" in my commit or it will be added again by you? Thanks for your help, Flavio
On Fri, Sep 15, 2023 at 04:05:16PM +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> > --- > > v2: > - fix dependecies in Kconfig > - fix Kconfig MP3309C entry order > - remove switch-on-delay-ms property > - remove optional gpio property to reset external devices > - remove dimming-mode property (the analog-i2c dimming mode is the default; the > presence of the pwms property, in DT, selects automatically the pwm dimming > mode) > - substitute three boolean properties, used for the overvoltage-protection > values, with a single enum property > - drop simple tracing messages > - use dev_err_probe() in probe function > - change device name from mp3309c_bl to the simple mp3309c > - remove shutdown function > v1: > - first version > > MAINTAINERS | 6 + > drivers/video/backlight/Kconfig | 11 + > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/mp3309c.c | 395 ++++++++++++++++++++++++++++++ > 4 files changed, 413 insertions(+) > create mode 100644 drivers/video/backlight/mp3309c.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3be1bdfe8ecc..f779df433af1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14333,6 +14333,12 @@ S: Maintained > F: Documentation/driver-api/tty/moxa-smartio.rst > F: drivers/tty/mxser.* > > +MP3309C BACKLIGHT DRIVER > +M: Flavio Suligoi <f.suligoi@asem.it> > +S: Maintained > +F: Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > +F: drivers/video/backlight/mp3309c.c > + > MR800 AVERMEDIA USB FM RADIO DRIVER > M: Alexey Klimov <klimov.linux@gmail.com> > L: linux-media@vger.kernel.org > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index 51387b1ef012..1144a54a35c0 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -402,6 +402,17 @@ config BACKLIGHT_LP8788 > help > This supports TI LP8788 backlight driver. > > +config BACKLIGHT_MP3309C > + tristate "Backlight Driver for MPS MP3309C" > + depends on I2C && PWM > + select REGMAP_I2C > + help > + This supports MPS MP3309C backlight WLED driver in both PWM and > + analog/I2C dimming modes. > + > + To compile this driver as a module, choose M here: the module will > + be called mp3309c. > + > config BACKLIGHT_PANDORA > tristate "Backlight driver for Pandora console" > depends on TWL4030_CORE > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile > index f72e1c3c59e9..1af583de665b 100644 > --- a/drivers/video/backlight/Makefile > +++ b/drivers/video/backlight/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.o > obj-$(CONFIG_BACKLIGHT_LP8788) += lp8788_bl.o > obj-$(CONFIG_BACKLIGHT_LV5207LP) += lv5207lp.o > obj-$(CONFIG_BACKLIGHT_MAX8925) += max8925_bl.o > +obj-$(CONFIG_BACKLIGHT_MP3309C) += mp3309c.o > obj-$(CONFIG_BACKLIGHT_MT6370) += mt6370-backlight.o > obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o > obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o > diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c > new file mode 100644 > index 000000000000..470c960d7438 > --- /dev/null > +++ b/drivers/video/backlight/mp3309c.c > @@ -0,0 +1,395 @@ > +// SPDX-License-Identifier: GPL-2.0+ This is an obsolete spelling. Should be: https://spdx.org/licenses/GPL-2.0-or-later.html > +static int mp3309c_bl_update_status(struct backlight_device *bl) > +{ > + struct mp3309c_chip *chip = bl_get_data(bl); > + int brightness = backlight_get_brightness(bl); > + struct pwm_state pwmstate; > + unsigned int analog_val, bits_val; > + int i, ret; > + > + if (chip->pdata->dimming_mode == DIMMING_PWM) { > + /* > + * PWM dimming mode > + */ > + pwm_get_state(chip->pwmd, &pwmstate); > + pwm_set_relative_duty_cycle(&pwmstate, brightness, > + chip->pdata->max_brightness); > + pwmstate.enabled = true; > + ret = pwm_apply_state(chip->pwmd, &pwmstate); > + if (ret) > + return ret; > + > + switch (chip->pdata->status) { > + case FIRST_POWER_ON: > + case BACKLIGHT_OFF: > + /* > + * After 20ms of pwm off, we must enable the chip again > + */ > + if (brightness > 0) { > + msleep_interruptible(10); This is either missing a return code check (and working bail-out logic) or, more likely, shouldn't be interruptible. Also it looks like the delay time and the comment do not match. However, when these and the license comment is addressed please add my: Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel.
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..99ccdba2c08f --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MPS MP3309C backlight + +maintainers: + - Flavio Suligoi <f.suligoi@asem.it> + +description: | + The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a + programmable switching frequency to optimize efficiency. + It supports two different dimming modes: + + - analog mode, via I2C commands (default) + - PWM controlled mode. + + The datasheet is available at: + https://www.monolithicpower.com/en/mp3309c.html + +allOf: + - $ref: common.yaml# + +properties: + compatible: + const: mps,mp3309c + + reg: + maxItems: 1 + + pwms: + description: if present, the backlight is controlled in PWM mode. + maxItems: 1 + + enable-gpios: + description: GPIO used to enable the backlight in "analog-i2c" dimming mode. + maxItems: 1 + + mps,overvoltage-protection-microvolt: + description: Overvoltage protection (13.5V, 24V or 35.5V). If missing, the + hardware default of 35.5V is used. + enum: [ 13500000, 24000000, 35500000 ] + + mps,no-sync-mode: + description: disable synchronous rectification mode + type: boolean + +required: + - compatible + - reg + - max-brightness + - default-brightness + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + /* Backlight with PWM control */ + backlight_pwm: backlight@17 { + compatible = "mps,mp3309c-backlight"; + reg = <0x17>; + pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ + max-brightness = <100>; + default-brightness = <80>; + overvoltage-protection-microvolt = <24000000>; + }; + };
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> --- v2: - remove useless properties (dimming-mode, pinctrl-names, pinctrl-0, switch-on-delay-ms, switch-off-delay-ms, reset-gpios, reset-on-delay-ms, reset-on-length-ms) - add common.yaml# - remove already included properties (default-brightness, max-brightness) - substitute three boolean properties, used for the overvoltage-protection values, with a single enum property - remove some conditional definitions - remove the 2nd example v1: - first version .../bindings/leds/backlight/mps,mp3309c.yaml | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml