On 08/03/2023 21:14, Bogdan Ionescu wrote: > Hi all, > > This is my first patch to the kernel, so I expect to have made > some mistakes. I did my best to follow the documentation and > submitting patches guidelines, but if I missed something, please > let me know. > > I wasn't sure if I had to add myself to the MAINTAINERS file for such a > small patch, so I didn't. > > Patch begins here: This all goes to cover letter. Patch begins with first line. `git am` this patch so you will see the issues. > > Added devicetree bindings documentation for the rohm,bd65b60 > led driver. > > Signed-off-by: Bogdan Ionescu <bogdan.ionescu.work@xxxxxxxxx> Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. > --- > .../bindings/leds/rohm,bd65b60-leds.yaml | 77 +++++++++++++++++++ > include/dt-bindings/leds/leds-bd65b60.h | 20 +++++ > 2 files changed, 97 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml > create mode 100644 include/dt-bindings/leds/leds-bd65b60.h > > diff --git a/Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml b/Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml > new file mode 100644 > index 000000000000..f69d7acd39c8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml Drop "-leds". This should match compatible. > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/rohm,bd65b60-leds.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ROHM BD65B60 device tree bindings Drop "device tree bindings" > + > +maintainers: > + - Bogdan Ionescu <bogdan.ionescu.work+kernel@xxxxxxxxx> > + > +description: | > + BD65B60 is a white LED driver IC that integrates PWM > + step-up DC/DC converter with boost-capability of up to > + maximum 28.5V and current driver with drive capability of up > + to 25mA(Typ.) maximum setting. Precise brightness can be > + controlled at wide ranges through the external PWM pulse > + input. > + > +properties: > + compatible: > + const: rohm,bd65b60 > + > + reg: > + maxItems: 1 > + > + led: Drop led object. There is no real need for it, is it? > + type: object > + $ref: common.yaml# > + unevaluatedProperties: false > + > + properties: > + rohm,enable-outputs: > + description: > + Select which leds are being controlled. > + Values defined in <dt-bindings/leds/leds-bd65b60.h> I don't understand why do you need this property. You should use generic/existing properties, if possible. > + $ref: "/schemas/types.yaml#/definitions/uint32" Drop quotes > + enum: [ 0, 1, 4, 5 ] > + > + rohm,ovp: > + description: > + Select Over-Voltage-Portection level. > + Values defined in <dt-bindings/leds/leds-bd65b60.h> > + $ref: "/schemas/types.yaml#/definitions/uint32" Drop quotes > + enum: [ 0, 8, 16 ] > + default: 16 What are the units? percent? Volts? Then use unit suffix in property name. Your other file suggests volts so make it microvolts. https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml > + > + required: > + - rohm,enable-outputs > + > +required: > + - compatible > + - reg > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/leds/common.h> > + #include <dt-bindings/leds/leds-bd65b60.h> > + > + i2c { > + Drop blank line Does not look like you tested the bindings. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + led_controler@64 { No underscores in node names > + compatible = "rohm,bd65b60"; > + reg = <0x64>; > + > + backlight_led: led { > + function = LED_FUNCTION_BACKLIGHT; > + color = <LED_COLOR_ID_WHITE>; > + rohm,enable-outputs = <BD65B60_ENABLE_LED1>; > + default-state = "keep"; > + linux,default-trigger = "backlight"; > + }; > + }; > + }; > + > +... > diff --git a/include/dt-bindings/leds/leds-bd65b60.h b/include/dt-bindings/leds/leds-bd65b60.h > new file mode 100644 > index 000000000000..8634faae58f4 > --- /dev/null > +++ b/include/dt-bindings/leds/leds-bd65b60.h Filename matching binding, but I think entire file will be gone. > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > +/* > + * This header provides macros for the ROHM BD65B60 device tree bindings. > + * > + * Copyright (C) 2023 Bogdan Ionescu <bogdan.ionescu.work+kernel@xxxxxxxxx> > + */ > + > +#ifndef _DT_BINDINGS_LEDS_BD65B60_H > +#define _DT_BINDINGS_LEDS_BD65B60_H > + > +#define BD65B60_ENABLE_NONE 0 > +#define BD65B60_ENABLE_LED1 1 > +#define BD65B60_ENABLE_LED2 4 > +#define BD65B60_ENABLE_BOTH (BD65B60_ENABLE_LED1 | BD65B60_ENABLE_LED2) I don't understand this. > + > +#define BD65B60_OVP_25V 0 > +#define BD65B60_OVP_30V 0x08 > +#define BD65B60_OVP_35V 0x10 Drop, register values are not bindings. > + > +#endif /* _DT_BINDINGS_LEDS_BD65B60_H */ Best regards, Krzysztof
