| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6dfae492-5533-df97-5c72-373d5e89444f@linaro.org>
Date: Tue, 29 Aug 2023 12:46:56 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Flavio Suligoi <f.suligoi@...m.it>, Lee Jones <lee@...nel.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Jingoo Han <jingoohan1@...il.com>,
Helge Deller <deller@....de>, Pavel Machek <pavel@....cz>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-fbdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C
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@...m.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@...m.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
Powered by blists - more mailing lists