dt-bindings: leds: Document rohm,bd65b60 bindings
Subsystems (2)
PATCH 1/2 dt-bindings: leds: Document rohm,bd65b60 bindings
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:
Added devicetree bindings documentation for the rohm,bd65b60 led driver.
--- .../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
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +77 -0
@@ -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 + +maintainers: + - Bogdan Ionescu <[email protected]> + +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: + 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> + $ref: "/schemas/types.yaml#/definitions/uint32" + 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" + enum: [ 0, 8, 16 ] + default: 16 + + required: + - rohm,enable-outputs + +required: + - compatible + - reg +additionalProperties: false + +examples: + - | + #include <dt-bindings/leds/common.h> + #include <dt-bindings/leds/leds-bd65b60.h> + + i2c { + + led_controler@64 { + 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"; + }; + }; + }; + +...
include/dt-bindings/leds/leds-bd65b60.h new file +20 -0
@@ -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 <[email protected]> +*/ + +#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) + +#define BD65B60_OVP_25V 0 +#define BD65B60_OVP_30V 0x08 +#define BD65B60_OVP_35V 0x10 + +#endif /* _DT_BINDINGS_LEDS_BD65B60_H */
Signature · 1 line
2.39.2
PATCH 2/2 leds: Add support for rohm,bd65b60 led driver
This commit adds support for ROHM BD65B60 led driver. The chip supports 2 outpus sharing the same current setting and is controlled over I2C.
--- drivers/leds/Kconfig | 11 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-bd65b60.c | 287 ++++++++++++++++++++++++++++++++++++ 3 files changed, 299 insertions(+) create mode 100644 drivers/leds/leds-bd65b60.c
drivers/leds/Kconfig +11 -0
@@ -717,6 +717,17 @@ config LEDS_BLINKM This option enables support for the BlinkM RGB LED connected through I2C. Say Y to enable support for the BlinkM LED. +config LEDS_BD65B60 + tristate "LED Driver for BD65B60" + depends on LEDS_CLASS + depends on I2C + help + If you have a LCD backlight connected to the ROHM BD65B60, + say Y here to enable this driver. + + To compile this driver as a module, choose M here: the module will + be called leds-bd65b60. + config LEDS_POWERNV tristate "LED support for PowerNV Platform" depends on LEDS_CLASS
drivers/leds/Makefile +1 -0
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o +obj-$(CONFIG_LEDS_BD65B60) += leds-bd65b60.o obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o
drivers/leds/leds-bd65b60.c new file +287 -0
287 lines not on this page · raw message
Signature · 1 line
2.39.2
Re: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver
Quoted 1 line · Bogdan Ionescu
This commit adds support for ROHM BD65B60 led driver.
Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Quoted 5 lines
The chip supports 2 outpus sharing the same current setting and is controlled over I2C.
Signed-off-by Bogdan Ionescu <[email protected]>---
Thank you for your patch. There is something to discuss/improve.
drivers/leds/leds-bd65b60.c line 146
Quoted 5 lines
drivers/leds/leds-bd65b60.c new file +5
-0+ +/* Check required properties */ +if (!fwnode_property_present (child,"rohm,enable-outputs" )) { +dev_err (dev,"No rohm,enable-outputs property found" ); +return -ENOENT ;
The property is not required by your binding, so you cannot bail here.
drivers/leds/leds-bd65b60.c line 151
Quoted 5 lines
drivers/leds/leds-bd65b60.c new file +5
-0+ } + + ret =fwnode_property_read_u32 (child,"rohm,enable-outputs" , &led->enable); +if (ret || (led->enable &LEDSEL_MASK ) != led->enable) { +dev_err (dev,"Failed to read rohm,enable-outputs property" );
No need to check for properties twice...
Quoted 60 lines
drivers/leds/leds-bd65b60.c new file +60
-0+return ret; + } + +/* Check optional properties */ + led->state =BD65B60_OFF ; +if (!fwnode_property_present (child,"default-state" )) { + ret =fwnode_property_read_string (child,"default-state" , + (const char **)&default_state); +if (ret) { +dev_err (dev,"Failed to read default-state property" ); +return ret; + } + +if (strcmp (default_state,"keep" ) ==0 ) { + led->state =BD65B60_KEEP ; + }else if (strcmp (default_state,"on" ) ==0 ) { + led->state =BD65B60_ON ; + }else if (strcmp (default_state,"off" ) ==0 ) { + led->state =BD65B60_OFF ; + }else { +dev_err (dev,"Invalid default-state property" ); +return -EINVAL ; + } + } + + led->ovp =BD65B60_DEFAULT_OVP_VAL ; +if (fwnode_property_present (child,"rohm,ovp" )) { + ret =fwnode_property_read_u32 (child,"rohm,ovp" , &led->ovp); + +if (ret || (led->ovp &OVP_MASK ) != led->ovp) { +dev_err (dev,"Failed to read rohm,ovp property" ); +return ret; + } + } + + *fwnode = child; + +return 0 ; +} + +static int bd65b60_probe (struct i2c_client *client) +{ +struct bd65b60_led *led; +struct led_init_data init_data = {}; +struct fwnode_handle *fwnode =NULL ; +int ret; + + led =devm_kzalloc (&client->dev,sizeof (*led),GFP_KERNEL ); +if (!led) +return -ENOMEM ; + + led->client = client; +i2c_set_clientdata (client, led); + + ret =bd65b60_parse_dt (led, &fwnode); +if (ret) +return ret; + + led->cdev.name =BD65B60_DEFAULT_NAME ; + led->cdev.brightness_set = bd65b60_brightness_set;
drivers/leds/leds-bd65b60.c line 221
Quoted 10 lines
drivers/leds/leds-bd65b60.c new file +10
-0+ led->cdev.brightness =BD65B60_DEFAULT_BRIGHTNESS ; + led->cdev.max_brightness =BD65B60_MAX_BRIGHTNESS ; + led->cdev.default_trigger =BD65B60_DEFAULT_TRIGGER ; + led->client = client; + + led->regmap =devm_regmap_init_i2c (client, &bd65b60_regmap_config); +if (IS_ERR (led->regmap)) { + ret =PTR_ERR (led->regmap); +dev_err (&client->dev,"Failed to allocate register map: %d" , + ret);
return dev_err_probe
drivers/leds/leds-bd65b60.c line 229
Quoted 8 lines
drivers/leds/leds-bd65b60.c new file +8
-0+return ret; + } + +mutex_init (&led->lock); + + ret =bd65b60_init (led); +if (ret) { +dev_err (&client->dev,"Failed to initialize led: %d" , ret);
return dev_err_probe
Review
Quoted 1 line
(quoted diff) +1
-0+ mutex_destroy(&led->lock);
or ret = dev_err_probe and goto to common cleanup part
drivers/leds/leds-bd65b60.c line 240
Quoted 10 lines
drivers/leds/leds-bd65b60.c new file +10
-0+return ret; + } + + init_data.fwnode = fwnode; + init_data.devicename = led->client->name; + init_data.default_label =":" ; + ret =devm_led_classdev_register_ext (&client->dev, &led->cdev, + &init_data); +if (ret) { +dev_err (&client->dev,"Failed to register led: %d" , ret);
return dev_err_probe
Quoted 26 lines
drivers/leds/leds-bd65b60.c new file +26
-0+mutex_destroy (&led->lock); +return ret; + } + +return 0 ; +} + +static void bd65b60_remove (struct i2c_client *client) +{ +int ret; +struct bd65b60_led *led =i2c_get_clientdata (client); + + ret =regmap_write (led->regmap,REG_PON ,BD65B60_OFF ); +if (ret) +dev_err (&client->dev,"Failed to turn off led: %d" , ret); + +mutex_destroy (&led->lock); +} + +static const struct i2c_device_id bd65b60_id[] = { + {"bd65b60" ,0 }, + {}, +}; +MODULE_DEVICE_TABLE (i2c, bd65b60_id); + +static const struct of_device_id of_bd65b60_leds_match[] = {
drivers/leds/leds-bd65b60.c line 276
Quoted 10 lines
drivers/leds/leds-bd65b60.c new file +10
-0+ { .compatible = "rohm,bd65b60" }, + {}, +}; +MODULE_DEVICE_TABLE (of, of_bd65b60_leds_match); + +static struct i2c_driver bd65b60_i2c_driver = { + .driver = { + .name ="bd65b60" , + .owner =THIS_MODULE , + .of_match_table =of_match_ptr (of_bd65b60_leds_match),
Drop of_match_ptr. It requires maybe_unused which you do not have, so this will cause warnings.
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): CALfPHoVFVpnLVEjzccbyMvmNR_=3vjJ+Ub8tkriJOvvV0cLzsA@mail.gmail.com
Re: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver
Quoted 13 lines · Bogdan Ionescu
Quoted 7 lines
Quoted 7 lines
+ /* Check required properties */
+ if (!fwnode_property_present(child, "rohm,enable-outputs")) {
+dev_err(dev, "No rohm,enable-outputs property found");
+ return-ENOENT;
+ }
+
+ ret = fwnode_property_read_u32(child, "rohm,enable-outputs",
&led->enable);Quoted 4 lines
Quoted 2 lines
+ if (ret || (
led->enable&LEDSEL_MASK) !=led->enable) {
+dev_err(dev, "Failed to read rohm,enable-outputs property");No need to check for properties twice...
Quoted 7 lines
I understand that you can just check the return value, but what is then the point of the
fwnode_property_present()function? Where is it otherwise supposed to be used?I am happy to remove the first check, but I would like to understand the reasoning behind it. I found value in separating a value not found error from an incorrect value error.
ENOENT is not valid code - No such file or directory. Thus you cannot return different codes. The code should be simple and printing error on fwnode_property_read_u32() is enough to report it to the developer.
Best regards,
Krzysztof
kernel test robot
build botRe: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver
In reply to Bogdan IonescuThank you for the patch! Perhaps something to improve:
Hi Bogdan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on lee-leds/for-leds-next]
[also build test WARNING on robh/for-next pavel-leds/for-next linus/master v6.3-rc2 next-20230317] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bogdan-Ionescu/leds-Add-support-for-rohm-bd65b60-led-driver/20230309-041730 base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next patch link: https://lore.kernel.org/r/20230308201451.11629-2-bogdan.ionescu.work%40gmail.com patch subject: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver config: s390-randconfig-r003-20230318 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/72417c0228f115628b1ad307be9d8e53532de6a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bogdan-Ionescu/leds-Add-support-for-rohm-bd65b60-led-driver/20230309-041730 git checkout 72417c0228f115628b1ad307be9d8e53532de6a6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/leds/ drivers/net/usb/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/leds/leds-bd65b60.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/leds/leds-bd65b60.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/leds/leds-bd65b60.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
drivers/leds/leds-bd65b60.c:111:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (led->state != BD65B60_KEEP) {
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/leds/leds-bd65b60.c:116:2: note: uninitialized use occurs here
ret |= regmap_update_bits(led->regmap, REG_COMSET1, OVP_MASK, led->ovp);
^~~
drivers/leds/leds-bd65b60.c:111:2: note: remove the 'if' if its condition is always true
if (led->state != BD65B60_KEEP) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/leds/leds-bd65b60.c:107:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
>> drivers/leds/leds-bd65b60.c:266:34: warning: unused variable 'of_bd65b60_leds_match' [-Wunused-const-variable]
static const struct of_device_id of_bd65b60_leds_match[] = {
^14 warnings generated.
vim +/of_bd65b60_leds_match +266 drivers/leds/leds-bd65b60.c
265
> 266 static const struct of_device_id of_bd65b60_leds_match[] = { 267 { .compatible = "rohm,bd65b60" },
268 {},
269 };
270 MODULE_DEVICE_TABLE(of, of_bd65b60_leds_match);
271
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Rob Herring
maintainerQuoted 15 lines · Bogdan Ionescu
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
MAINTAINERSfile for such a small patch, so I didn't.Patch begins here:
Added devicetree bindings documentation for the rohm,bd65b60 led driver.
Quoted 8 lines
Signed-off-by Bogdan Ionescu <[email protected]>--- .../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
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/rohm,bd65b60-leds.example.dts:25.17-30: Warning (reg_format): /example-0/i2c/led_controler@64:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1) Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dts:21.13-35.11: Warning (i2c_bus_bridge): /example-0/i2c: incorrect #address-cells for I2C bus Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dts:21.13-35.11: Warning (i2c_bus_bridge): /example-0/i2c: incorrect #size-cells for I2C bus Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'i2c_bus_bridge' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dts:23.30-34.15: Warning (avoid_default_addr_size): /example-0/i2c/led_controler@64: Relying on default #address-cells value Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dts:23.30-34.15: Warning (avoid_default_addr_size): /example-0/i2c/led_controler@64: Relying on default #size-cells value Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
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.
Quoted 11 lines · Bogdan Ionescu
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
MAINTAINERSfile 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.
Quoted 5 lines
Added devicetree bindings documentation for the rohm,bd65b60 led driver.
Signed-off-by Bogdan Ionescu <[email protected]>
Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings.
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml
Quoted 12 lines
--- .../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.hDocumentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +0
-0
Drop "-leds". This should match compatible.
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 7
Quoted 8 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +7
-0@@ -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"
Quoted 10 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +10
-0+ +maintainers: + - Bogdan Ionescu <[email protected]> + +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
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 27
Quoted 10 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +10
-0+ input. + +properties: + compatible: + const: rohm,bd65b60 + + reg: + maxItems: 1 + + led:
Drop led object. There is no real need for it, is it?
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 36
Quoted 9 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +9
-0+ 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.
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 37
Quoted 1 line
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +1
-0+ $ref: "/schemas/types.yaml#/definitions/uint32"
Drop quotes
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 44
Quoted 7 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +7
-0+ 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
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 46
Quoted 2 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +2
-0+ 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
Quoted 6 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +6
-0+ + required: + - rohm,enable-outputs + +required: + - compatible
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 62
Quoted 10 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +10
-0+ - 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).
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 63
Quoted 1 line
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +1
-0+ led_controler@64 {
No underscores in node names
include/dt-bindings/leds/leds-bd65b60.h
Quoted 19 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +14
-0+ 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"; + }; + }; + }; + +...include/dt-bindings/leds/leds-bd65b60.h new file +0
-0
Filename matching binding, but I think entire file will be gone.
Quoted 5 lines
include/dt-bindings/leds/leds-bd65b60.h new file +4
-0@@ -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. +*
include/dt-bindings/leds/leds-bd65b60.h line 14
Quoted 10 lines
include/dt-bindings/leds/leds-bd65b60.h new file +10
-0+* Copyright (C) 2023 Bogdan Ionescu <[email protected]> +*/ + +#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.
include/dt-bindings/leds/leds-bd65b60.h line 18
Quoted 4 lines
include/dt-bindings/leds/leds-bd65b60.h new file +4
-0+ +#define BD65B60_OVP_25V 0 +#define BD65B60_OVP_30V 0x08 +#define BD65B60_OVP_35V 0x10
Drop, register values are not bindings.
include/dt-bindings/leds/leds-bd65b60.h line 20
Quoted 2 lines
include/dt-bindings/leds/leds-bd65b60.h new file +2
-0+ +#endif /* _DT_BINDINGS_LEDS_BD65B60_H */
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): CALfPHoVT007ifyTepr_p-saDQtv5Sk7iYXM4qSMXyaCLL3iE2A@mail.gmail.com
Quoted 33 lines · Bogdan Ionescu
Thanks for the feedback, I am working on addressing the issues you have raised. If I haven't mentioned something, please assume I am implementing your suggestion.
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ properties: + rohm,enable-outputs:I don't understand why do you need this property. You should use generic/existing properties, if possible.
Drop led object. There is no real need for it, is it?
Would this structure be more appropriate:
+ led-controler@64 {
(quoted diff) +17
-0+ compatible = "rohm,bd65b60"; + reg = <0x64>; + default-state = "keep"; + linux,default-trigger = "backlight"; + + led@1 { + reg = <1>; + function = LED_FUNCTION_BACKLIGHT; + color = <LED_COLOR_ID_WHITE>; + }; + + led@2 { + reg = <2>; + function = LED_FUNCTION_BACKLIGHT; + color = <LED_COLOR_ID_WHITE>; + }; + };
Quoted 1 line
Can I have 2 led nodes if they share the same brightness?
I think yes, but what about LED on/off? Is it also shared or separate? Because if it is shared, then basically it is just one LED...
Quoted 10 lines
Quoted 6 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml line 46
Quoted 3 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +2
-0+ enum: [ 0, 8, 16 ] + default: 16What are the units? percent? Volts? Then use unit suffix in property name. Your other file suggests volts so make it microvolts.
The chip has 3 possible values 25V, 30V and 35V. How should the driver
handle a value that is not exactly that?
Should it round down for safety or return an error?
Then use -microvolt and real units
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
Quoted 10 lines
This was the reason for creating the bindings header file. Is it possible to reference defines from the bindings header file?
+ enum:
(quoted diff) +4
-0+ - BD65B60_OVP_25V + - BD65B60_OVP_30V + - BD65B60_OVP_35V + default: BD65B60_OVP_35V
Quoted 4 lines
I would prefer it if I didn't have to expose the weird bitmasks that ROHM have decided to use and instead use the actual values.
Apologies for the long email, but I wanted to make sure I was clear.
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): CALfPHoVR=kmHmbhNzQg-bW-bV6P8e_sE+vEebQXOfL-nEWg5TQ@mail.gmail.com
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml
Quoted 11 lines · Bogdan Ionescu
Quoted 2 lines
I think yes, but what about LED on/off? Is it also shared or separate? Because if it is shared, then basically it is just one LED...
No, the outputs can be enabled independently, but I don't see any function in the
led_classdevto do that.
The led-sources property looks like the right thing to use for this. If I have effectively only one led per device, do I still need an led child node or is this sufficient:Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +3
-0+ #include <dt-bindings/leds/common.h> + i2c { + backlight@64 {
First of all, backlight is something a bit different/more specific - see Documentation/devicetree/bindings/leds/backlight/
Anyway for led-sources and everything in one node, I think you do not need any children. But this will allow you only one function and one color...
Quoted 11 lines
Documentation/devicetree/bindings/leds/rohm,bd65b60-leds.yaml new file +10
-0+ compatible = "rohm,bd65b60"; + reg = <0x64>; + + led-sources = <1>, <2>; + function = LED_FUNCTION_BACKLIGHT; + color = <LED_COLOR_ID_WHITE>; + + rohm,ovp = <30000000>; + }; + };
Quoted 3 lines
Best regards,
Bogdan Ionescu
Best regards,
Krzysztof