Re: [PATCH v2 2/4] leds: Add driver for QCOM SPMI Flash LEDs
On 2/19/21 12:02 PM, Pavel Machek wrote: Hi! + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include Please sort includes alphabetically. No need to do that. Keeping the includes sorted eliminates the risk of introducing duplicates and allows for faster lookup. What gain is in having them unsorted? +#define FLASH_SAFETY_TIMER 0x40 Namespacing prefix is needed for macros, e.g. QCOM_FLASH*. No need for that in .c files. In general it eliminates the risk of name clash with other subsystems headers. And actually the prefix here should be QCOM_LED_FLASH to avoid ambiguity with flash memory. If you dropped the vendor prefix then you'd get possible name clash with led-class-flash.h namespace prefix. -- Best regards, Jacek Anaszewski
Re: AW: [PATCH v2 1/4] leds: lp50xx: add setting of default intensity from DT
Sven,
On 2/6/21 2:14 PM, Sven Schuchmann wrote:
Hello Dan,
Von: Jacek Anaszewski
Gesendet: Freitag, 5. Februar 2021 19:37
Hi Pavel,
On 2/5/21 11:23 AM, Pavel Machek wrote:
Hi!
patternProperties:
"(^led-[0-9a-f]$|led)":
@@ -99,6 +104,7 @@ examples:
reg = <0x1>;
color = ;
function = LED_FUNCTION_CHARGING;
+ default-intensity = <100 0 0>;
How will you know which array position is for which child LED?
I presume DT child nodes are not guaranteed to be parsed in the order
of declaration?
I tried to fiddle this out, but it seems Jacek is right over here.
The multi-led definition looks like this (from the documentation
leds-lp50xx.yaml)
multi-led@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0x1>;
color = ;
function = LED_FUNCTION_CHARGING;
led-0 {
color = ;
};
led-1 {
color = ;
};
led-2 {
color = ;
};
};
But it seems that the color definition of each led is ignored.
By ignored I mean the driver does not take care which color
is at which position. So if I change led-0 to be LED_COLOR_ID_BLUE
and led-2 to be LED_COLOR_ID_RED nothing will change if I write
from userspace. Could you help to clarify?
Then it is even hard to know which led to set with default-intensity.
See Documentation/ABI/testing/sysfs-class-led-multicolor and
documentation of multi_index and multi_intensity files.
It is the multi_index file that tells what is the order of colors
in the multi_intensity file.
And that depends on the order of enumeration of the nodes by DT parser.
Also it seems that the enumeration of the multi-leds should
start with multi-led@0 (and not 1 as in the documentation).
The @unit-address part of node name must match the first address
specified in the reg property of the node, so this is correct.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v2 1/4] leds: lp50xx: add setting of default intensity from DT
Hi Pavel,
On 2/5/21 11:23 AM, Pavel Machek wrote:
Hi!
patternProperties:
"(^led-[0-9a-f]$|led)":
@@ -99,6 +104,7 @@ examples:
reg = <0x1>;
color = ;
function = LED_FUNCTION_CHARGING;
+ default-intensity = <100 0 0>;
How will you know which array position is for which child LED?
I presume DT child nodes are not guaranteed to be parsed in the order
of declaration?
led-0 {
color = ;
Should this go to leds-class-multicolor.yaml ?
I think then all drivers should support it, but I cannot change all drivers.
So I would only leave it in there.
It really should be in common binding, and no, that does not mean you
need to change all the drivers.
Plus there's at most two of them at the moment.
Best regards,
Pavel
--
Best regards,
Jacek Anaszewski
Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT
Hi Sven,
On 1/19/21 11:53 AM, Sven Schuchmann wrote:
In order to use a multicolor-led together with a trigger
from DT the led needs to have an intensity set to see something.
The trigger changes the brightness of the led but if there
is no intensity we actually see nothing.
This patch adds the ability to set the default intensity
of each led so that it is turned on from DT.
Signed-off-by: Sven Schuchmann
---
Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++-
drivers/leds/leds-lp50xx.c | 4
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
index c192b5feadc7..5ad2a0c3c052 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
@@ -69,7 +69,12 @@ patternProperties:
patternProperties:
"(^led-[0-9a-f]$|led)":
type: object
-$ref: common.yaml#
+allOf:
+ - $ref: common.yaml#
+properties:
+ default-intensity:
+maxItems: 1
+description: The intensity the LED gets initialised with.
required:
- compatible
@@ -102,6 +107,7 @@ examples:
led-0 {
color = ;
+ default-intensity = <100>;
};
led-1 {
Please split this into a separate patch, preceding this one in the series.
diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index f13117eed976..ba760fa33bdc 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
}
mc_led_info[num_colors].color_index = color_id;
+
+ fwnode_property_read_u32(led_node, "default-intensity",
+
_led_info[num_colors].intensity);
+
num_colors++;
}
For this part:
Acked-by: Jacek Anaszewski
--
Best regards,
Jacek Anaszewski
Re: [PATCH v2 2/4] leds: Add driver for QCOM SPMI Flash LEDs
if (of_find_property(node, "flash-boost-supply", NULL)) {
+ leds_dev->flash_boost_reg = regulator_get(>dev,
"flash-boost");
+ if (IS_ERR(leds_dev->flash_boost_reg)) {
+ rc = PTR_ERR(leds_dev->flash_boost_reg);
+ dev_err(>dev, "Regulator get failed(%d)\n", rc);
+ regulator_put(leds_dev->flash_boost_reg);
+ leds_dev->flash_boost_reg = NULL;
+ return rc;
+ }
+ }
+
+ if (of_find_property(node, "torch-boost-supply", NULL)) {
+ leds_dev->torch_boost_reg = regulator_get(>dev,
"torch-boost");
+ if (IS_ERR(leds_dev->torch_boost_reg)) {
+ rc = PTR_ERR(leds_dev->torch_boost_reg);
+ dev_err(>dev, "Torch regulator get failed(%d)\n",
rc);
+ regulator_put(leds_dev->flash_boost_reg);
+ regulator_put(leds_dev->torch_boost_reg);
+ leds_dev->flash_boost_reg = NULL;
+ leds_dev->torch_boost_reg = NULL;
+ return rc;
+ }
+ leds_dev->torch_enable = FLASH_ENABLE_MODULE;
+ } else {
+ leds_dev->torch_enable = FLASH_ENABLE_ALL;
+ }
+
+ rc = led_read_reg(leds_dev, FLASH_PERIPHERAL_SUBTYPE,
+ _dev->peripheral_subtype);
+ if (rc) {
+ dev_err(>dev,
+ "Unable to read from addr=%x, rc(%d)\n",
+ FLASH_PERIPHERAL_SUBTYPE, rc);
+ }
+
+ mutex_init(_dev->lock);
+
+ return 0;
+}
+
+static int qcom_flash_probe(struct platform_device *pdev)
+{
+ struct device *dev = >dev;
+ struct qcom_flash_device *leds_dev;
+ struct device_node *node = dev->of_node;
+ struct qcom_flash_led *led;
+ struct device_node *temp;
+ int rc, i, parsed_leds = 0;
+
+ leds_dev = devm_kzalloc(dev, sizeof(struct qcom_flash_device),
GFP_KERNEL);
+ if (!leds_dev)
+ return -ENOMEM;
+
+ rc = qcom_flash_setup_leds_device(leds_dev, node, pdev);
+ if (rc) {
+ pr_debug("Probe failed setting up base (%d)\n", rc);
+ return rc;
+ }
Please implement qcom_flash_probe_dt() function that will first parse
all DT settings global for this device and then iterate through the
child nodes and parse child LED properties. Afterwards you can call a
function that will configure all device global settings and particular
LEDs. You can compare existing LED class drivers. It simplifies code
analysis if all DT parsing is in one place.
+ platform_set_drvdata(pdev, leds_dev);
+
+ for_each_child_of_node(node, temp) {
+ led = _dev->leds[parsed_leds];
+
+ rc = qcom_flash_setup_led(led, temp);
+ if (rc) {
+ for (i = 0; i < parsed_leds; i++)
+
led_classdev_flash_unregister(_dev->leds[i].cdev);
+ pr_debug("Probe failed setting up leds (%d)\n", rc);
+ return rc;
+ }
+
+ parsed_leds++;
+ }
+ leds_dev->num_leds = parsed_leds;
+ return 0;
+}
+
+static int qcom_flash_remove(struct platform_device *pdev)
+{
+ struct qcom_flash_device *leds_dev = platform_get_drvdata(pdev);
+ int i, parsed_leds = leds_dev->num_leds;
+
+ mutex_destroy(_dev->lock);
+ if (leds_dev->flash_boost_reg)
+ regulator_put(leds_dev->flash_boost_reg);
+ if (leds_dev->torch_boost_reg)
+ regulator_put(leds_dev->torch_boost_reg);
+ for (i = 0; i < parsed_leds; i++)
+ led_classdev_flash_unregister(_dev->leds[i].cdev);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_flash_spmi_of_match[] = {
+ { .compatible = "qcom,spmi-flash" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, qcom_flash_spmi_of_match);
+
+static struct platform_driver qcom_flash_driver = {
+ .driver = {
+ .name = "qcom,spmi-flash",
+ .of_match_table = of_match_ptr(qcom_flash_spmi_of_match),
+ },
+ .probe = qcom_flash_probe,
+ .remove = qcom_flash_remove,
+};
+module_platform_driver(qcom_flash_driver);
+
+MODULE_DESCRIPTION("Qualcomm SPMI Flash LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Nícolas F. R. A. Prado ");
--
Best regards,
Jacek Anaszewski
Re: [PATCH v2 1/4] dt-bindings: leds: Add binding for qcom-spmi-flash
Hi Nicolas,
On 1/26/21 3:04 PM, Nícolas F. R. A. Prado wrote:
Add devicetree binding for QCOM SPMI Flash LEDs, which are part of
PM8941, and are used both as lantern and camera flash.
Signed-off-by: Nícolas F. R. A. Prado
---
Changes in v2:
- Add this commit
.../bindings/leds/leds-qcom-spmi-flash.yaml | 94 +++
.../dt-bindings/leds/leds-qcom-spmi-flash.h | 15 +++
2 files changed, 109 insertions(+)
create mode 100644
Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
create mode 100644 include/dt-bindings/leds/leds-qcom-spmi-flash.h
diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
b/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
new file mode 100644
index ..169716e14f67
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-qcom-spmi-flash.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SPMI Flash LEDs
+
+maintainers:
+ - Nícolas F. R. A. Prado
+
+description: |
+ The Qualcomm SPMI Flash LEDs are part of Qualcomm PMICs and are used
primarily
+ as a camera or video flash. They can also be used as a lantern when on torch
+ mode.
+ The PMIC is connected to Host processor via SPMI bus.
+
+properties:
+ compatible:
+const: qcom,spmi-flash
+
+ reg:
+maxItems: 1
+
+ flash-boost-supply:
+description: SMBB regulator for LED flash mode
+
+ torch-boost-supply:
+description: SMBB regulator for LED torch mode
+
+patternProperties:
+ "^led[0-1]$":
+type: object
+$ref: common.yaml#
+
+properties:
+ qcom,clamp-curr:
+description: current to clamp at, in uA
You're already using led-max-microamp and flash-max-microamp,
so I'm not sure how this is related to those.
+$ref: /schemas/types.yaml#definitions/uint32
+
+ qcom,headroom:
+description: |
+ headroom to use. Use one of QCOM_SPMI_FLASH_HEADROOM_* defined in
+ include/dt-bindings/leds/leds-qcom-spmi-flash.h
More information would be welcome here on how it impacts device
operation.
+$ref: /schemas/types.yaml#definitions/uint32
+minimum: 0
+maximum: 3
+
+ qcom,startup-dly:
+description: |
+ delay before flashing. Use one of QCOM_SPMI_FLASH_STARTUP_DLY_*
+ defined in include/dt-bindings/leds/leds-qcom-spmi-flash.h
I don't see much gain in having this exposed.
We don't have related constructs in the API to handle that, so I propose
to always set it to 0 and not expose this setting at all.
+$ref: /schemas/types.yaml#definitions/uint32
+minimum: 0
+maximum: 3
+
+ qcom,safety-timer:
+description: include for safety timer use, otherwise watchdog timer
will be used
+type: boolean
What's the difference between watchdog timer and safety timer?
Either way, I propose to choose the option best fitting for handling
flash timeout setting and hide this inside the driver.
+required:
+ - compatible
+ - reg
+ - flash-boost-supply
+ - torch-boost-supply
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+#include
+
+qcom,leds@d300 {
+compatible = "qcom,spmi-flash";
+reg = <0xd300 0x100>;
+flash-boost-supply = <_5vs1>;
+torch-boost-supply = <_5v>;
+
+led0 {
+led-sources = <0>;
Please use reg property for this purpose instead.
+function = LED_FUNCTION_FLASH;
+color = ;
+led-max-microamp = <20>;
+flash-max-microamp = <100>;
+flash-max-timeout-us = <128>;
You should mention this properties above and provide constraints
(min, max, step).
+default-state = "off";
There's no point in having this in the example.
+qcom,clamp-curr = <20>; > +qcom,headroom =
;
+qcom,startup-dly = ;
+qcom,safety-timer;
+};
+};
+...
diff --git a/include/dt-bindings/leds/leds-qcom-spmi-flash.h
b/include/dt-bindings/leds/leds-qcom-spmi-flash.h
new file mode 100644
index ..8bd54a8e831d
--- /dev/null
+++ b/include/dt-bindings/leds/leds-qcom-spmi-flash.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_LEDS_QCOM_SPMI_FLASH_H
+#define _DT_BINDINGS_LEDS_QCOM_SPMI_FLASH_H
+
+#define QCOM_SPMI_FLASH_HEADROOM_250MV 0
+#define QCOM_SPMI_FLASH_HEADROOM_300MV 1
+#define QCOM_SPMI_FLASH_HEADROOM_400MV 2
+#define QCOM_SPMI_FLASH_HEADROOM_500MV 3
+
+#define QCOM_SPMI_FLASH_STARTUP_DLY_10US 0
+#define QCOM_SPMI_FLASH_STARTUP_DLY_32US 1
+#define QCOM_SPMI_FLASH_STARTUP_DLY_64US 2
+#define QCOM_SPMI_FLASH_STARTUP_DLY_128US 3
+
+#endif
--
Best regards,
Jacek Anaszewski
Re: [PATCH v2 0/4] Add support for QCOM SPMI Flash LEDs
Hi Nicolas. On 1/26/21 3:03 PM, Nícolas F. R. A. Prado wrote: Hi, this patch series adds support for Qualcomm's SPMI Flash LEDs present in the PM8941 PMIC. It is used as part of MSM8974 based devices, like the Nexus 5 (hammerhead), as a camera flash or as a lantern when in torch mode. Patch 1 adds the dt-bindings for the driver, together with a header for the values of some properties. Patch 2 adds the driver, which was ported from downstream [1], and is now using the flash LED class framework. Patch 3 enables the driver as a module in qcom_defconfig, and also enables CONFIG_LEDS_CLASS_FLASH since it is required by the driver. Patch 4 adds the device tree nodes configuring the driver in the pm8941 dtsi. After the feedback I received from the v1 RFC patch (thank you Jacek and Bjorn!), I implemented the flash LED class framework, renamed the driver to qcom-spmi-flash and added the dt-bindings. I also did a whole lot of cleanup. Some caveats: - I still didn't implement get_strobe() and get_fault() for the flash LEDs, because I'm still not sure how to do it. get_strobe() in particular I'm not even sure if is possible, since after the flash turns off automatically after the timeout, I don't see any change in the SPMI registers. So I'm unsure how one would get the current strobe state. strobe_get is optional - you can leave it uninitialized if there is no obvious way to get strobe status. Regarding faults - I see you have FLASH_FAULT_DETECT but have no information on its impact whatsoever. Usually devices report the faults by settings some register bits and then we can map those errors to LED flash framework generic errors. - I have yet to add the V4L2 flash wrapper for the flash LEDs. I still didn't do it because I wasn't sure if it was needed, so wanted to double check. But being a camera flash it seems that would be useful. Also, it would be great if someone could point me how I would go about testing the flash usage through V4L2. You need a V4L2 media device driver with which this driver would register a V4L2 flash LED sub-device. Such a device is usually implemented for platform ISP devices. Provided it is present in the mainline you would have to associate this driver DT node with the media device DT node. Then you can test the V4L2 Flash control with v4l2-ctl or yavta user space tools. Let's skip the V4L2 support for now - it can be added later, if needed. Another thing worth mentioning: for v1 the dt nodes were added in hammerhead's dts (just to simplify testing), but I have now moved them to pm8941's dtsi, since it was like that in downstream. So if folks using devices based on PM8941/MSM8974 other than the Nexus 5 could test it, that would be great, since I have only tested on the Nexus 5. v1 RFC: https://lore.kernel.org/lkml/20201106165737.1029106-1-nfrapr...@protonmail.com/ [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/leds/leds-qpnp.c Nícolas F. R. A. Prado (4): dt-bindings: leds: Add binding for qcom-spmi-flash leds: Add driver for QCOM SPMI Flash LEDs ARM: qcom_defconfig: Enable QCOM SPMI Flash LEDs ARM: dts: qcom: pm8941: Add nodes for QCOM SPMI Flash LEDs .../bindings/leds/leds-qcom-spmi-flash.yaml | 94 ++ arch/arm/boot/dts/qcom-pm8941.dtsi| 38 + arch/arm/configs/qcom_defconfig |2 + drivers/leds/Kconfig |8 + drivers/leds/Makefile |1 + drivers/leds/leds-qcom-spmi-flash.c | 1153 + .../dt-bindings/leds/leds-qcom-spmi-flash.h | 15 + 7 files changed, 1311 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml create mode 100644 drivers/leds/leds-qcom-spmi-flash.c create mode 100644 include/dt-bindings/leds/leds-qcom-spmi-flash.h -- Best regards, Jacek Anaszewski
Re: [PATCH resend v13 5/5] leds: mt6360: Add LED driver for MT6360
Hi Gene, On 12/24/20 2:47 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. You can carry my ack also for patches 4/5 and 5/5: Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v11 5/5] leds: mt6360: Add LED driver for MT6360
Hi Gene,
On 12/2/20 11:46 AM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for
moonlight LED.
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 13 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 827 +
3 files changed, 841 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..4f533bc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,19 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS && OF
+ depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
+ depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on MFD_MT6360
+ help
+ This option enables support for dual Flash LED drivers found on
+ Mediatek MT6360 PMIC.
+ Independent current sources supply for each flash LED support torch
+ and strobe mode.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7a..5596427 100644
[...]
+static int mt6360_led_probe(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv;
+ struct fwnode_handle *child;
+ size_t count;
+ int i = 0, ret;
+
+ count = device_get_child_node_count(>dev);
+ if (!count || count > MT6360_MAX_LEDS) {
+ dev_err(>dev, "No child node or node count over max led number
%lu\n", count);
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(>dev, struct_size(priv, leds, count),
GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->leds_count = count;
+ priv->dev = >dev;
+ mutex_init(>lock);
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap) {
+ dev_err(>dev, "Failed to get parent regmap\n");
+ return -ENODEV;
+ }
+
+ device_for_each_child_node(>dev, child) {
+ struct mt6360_led *led = priv->leds + i;
+ struct led_init_data init_data = { .fwnode = child, };
+ u32 reg, led_color;
+
+ ret = fwnode_property_read_u32(child, "color", _color);
+ if (ret)
+ goto out_flash_release;
+
+ if (led_color == LED_COLOR_ID_RGB || led_color ==
LED_COLOR_ID_MULTI)
+ reg = MT6360_VIRTUAL_MULTICOLOR;
+ else {
+ ret = fwnode_property_read_u32(child, "reg", );
+ if (ret)
+ goto out_flash_release;
+
+ if (reg >= MT6360_MAX_LEDS) {
+ ret = -EINVAL;
+ goto out_flash_release;
+ }
+ }
+
+ if (priv->leds_active & BIT(reg)) {
+ ret = -EINVAL;
+ goto out_flash_release;
+ }
+ priv->leds_active |= BIT(reg);
+
+ led->led_no = reg;
+ led->priv = priv;
+
+ ret = mt6360_init_common_properties(led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ if (reg == MT6360_VIRTUAL_MULTICOLOR ||
+ (reg >= MT6360_LED_ISNK1 && reg <= MT6360_LED_ISNKML))
+ ret = mt6360_init_isnk_properties(led, _data);
+ else
+ ret = mt6360_init_flash_properties(led, _data);
+
+ if (ret)
+ goto out_flash_release;
+
+ ret = mt6360_led_register(>dev, led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ i++;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+
+out_flash_release:
+ mt6360_v4l2_flash_release(priv);
The need for releasing v4l2_flash devices here and not other LEDs is not
obvious at first glance. Of course this is due to the fact that
LED/flash registration functions have devm support but v4l2_flash
doesn't. It would be good to cover it at some point.
If you added that support to
drivers/media/v4l2-core/v4l2-flash-led-class.c then you could
simplify the code in this driver quite nicely.
But it's up to you.
That being said:
Acked-by: Jacek Anaszewski
--
Best regards,
Jacek Anaszewski
Re: [PATCH v11 4/5] dt-bindings: leds: Add bindings for MT6360 LED
Hi Gene, Thanks for the update. On 12/2/20 11:46 AM, Gene Chen wrote: From: Gene Chen Add bindings document for LED support on MT6360 PMIC Signed-off-by: Gene Chen --- .../devicetree/bindings/leds/leds-mt6360.yaml | 159 + 1 file changed, 159 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml new file mode 100644 index 000..73c67b1 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v11 3/5] dt-bindings: leds: Add LED_FUNCTION_MOONLIGHT definitions
Hi Pavel, On 12/3/20 12:40 PM, Pavel Machek wrote: Hi! +++ b/include/dt-bindings/leds/common.h @@ -78,6 +78,7 @@ #define LED_FUNCTION_INDICATOR "indicator" #define LED_FUNCTION_LAN "lan" #define LED_FUNCTION_MAIL "mail" +#define LED_FUNCTION_MOONLIGHT "moonlight" There's "torch" function that should be used for this. I guess comment should be added with explanation what exactly that is and how should the LED be named. According to mail, 11/25 "Re: [PATCH v7 2/5] dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions", The Moonlight LED is LED which maximum current more than torch, but less than flash. Such as front camera fill light. I think our channel is moonlight, not torch. I will add this description to comment. We can't exactly define moonlight current level, because every vendor has their own specification. So... what is the timelimit on moonlight? But if it is used for camera illumination, I believe it should be simply called flash. Let's keep FLASH reserved for LED flash class devices. This device has already two other flash iouts. Also iouts amperage gives clue that they have three different functions. -- Best regards, Jacek Anaszewski
Re: [PATCH v10 5/6] dt-bindings: leds: Add bindings for MT6360 LED
function = LED_FUNCTION_MOONLIGHT;
+ color = ;
+ led-max-microamp = <15>;
+ };
+ led@4 {
+ reg = <4>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <1>;
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@5 {
+ reg = <5>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <2>;
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ };
+...
--
Best regards,
Jacek Anaszewski
Re: [PATCH v9 2/6] leds: flash: Fix multicolor registration no-ops by return 0
Hi Gene,
On 11/26/20 12:37 PM, Gene Chen wrote:
From: Gene Chen
Fix multicolor registration no-ops by return 0
Signed-off-by: Gene Chen
---
include/linux/led-class-multicolor.h | 42 +---
1 file changed, 15 insertions(+), 27 deletions(-)
diff --git a/include/linux/led-class-multicolor.h
b/include/linux/led-class-multicolor.h
index 5116f9a..210d57b 100644
--- a/include/linux/led-class-multicolor.h
+++ b/include/linux/led-class-multicolor.h
@@ -44,12 +44,6 @@ int led_classdev_multicolor_register_ext(struct device
*parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data);
-static inline int led_classdev_multicolor_register(struct device *parent,
- struct led_classdev_mc *mcled_cdev)
-{
- return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
-}
-
/**
* led_classdev_multicolor_unregister - unregisters an object of led_classdev
*class with support for multicolor LEDs
@@ -68,13 +62,6 @@ int devm_led_classdev_multicolor_register_ext(struct device
*parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data);
-static inline int devm_led_classdev_multicolor_register(struct device *parent,
-struct led_classdev_mc *mcled_cdev)
-{
- return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
-NULL);
-}
-
void devm_led_classdev_multicolor_unregister(struct device *parent,
struct led_classdev_mc *mcled_cdev);
#else
@@ -83,27 +70,33 @@ static inline int
led_classdev_multicolor_register_ext(struct device *parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data)
{
- return -EINVAL;
-}
-
-static inline int led_classdev_multicolor_register(struct device *parent,
- struct led_classdev_mc *mcled_cdev)
-{
- return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
+ return 0;
}
static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
static inline int led_mc_calc_color_components(struct led_classdev_mc
*mcled_cdev,
enum led_brightness brightness)
{
- return -EINVAL;
+ return 0;
}
static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data)
{
- return -EINVAL;
+ return 0;
+}
+
+static inline void devm_led_classdev_multicolor_unregister(struct device
*parent,
+ struct led_classdev_mc *mcled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) */
+
+static inline int led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
}
static inline int devm_led_classdev_multicolor_register(struct device *parent,
@@ -113,9 +106,4 @@ static inline int
devm_led_classdev_multicolor_register(struct device *parent,
NULL);
}
-static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
- struct led_classdev_mc *mcled_cdev)
-{};
-
-#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) */
#endif/* _LINUX_MULTICOLOR_LEDS_H_INCLUDED */
Here the same comment as for the patch 1/6.
With that:
Acked-by: Jacek Anaszewski
--
Best regards,
Jacek Anaszewski
Re: [PATCH v9 1/6] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Hi Gene,
Thank you for addressing my remarks.
On 11/26/20 12:37 PM, Gene Chen wrote:
From: Gene Chen
Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Signed-off-by: Gene Chen
---
include/linux/led-class-flash.h | 42 -
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 21a3358..612b4ca 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
return container_of(lcdev, struct led_classdev_flash, led_cdev);
}
+#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
/**
* led_classdev_flash_register_ext - register a new object of LED class with
* init data and with support for flash LEDs
@@ -98,12 +99,6 @@ int led_classdev_flash_register_ext(struct device *parent,
struct led_classdev_flash *fled_cdev,
struct led_init_data *init_data);
-static inline int led_classdev_flash_register(struct device *parent,
- struct led_classdev_flash *fled_cdev)
-{
- return led_classdev_flash_register_ext(parent, fled_cdev, NULL);
-}
-
/**
* led_classdev_flash_unregister - unregisters an object of led_classdev class
* with support for flash LEDs
@@ -118,15 +113,44 @@ int devm_led_classdev_flash_register_ext(struct device
*parent,
struct led_init_data *init_data);
+void devm_led_classdev_flash_unregister(struct device *parent,
+ struct led_classdev_flash *fled_cdev);
+
+#else
+
+static inline int led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return 0;
+}
+
+static inline void led_classdev_flash_unregister(struct led_classdev_flash
*fled_cdev) {};
+static inline int devm_led_classdev_flash_register_ext(struct device *parent,
+struct led_classdev_flash *fled_cdev,
+struct led_init_data *init_data)
+{
+ return 0;
+}
+
+static inline void devm_led_classdev_flash_unregister(struct device *parent,
+ struct led_classdev_flash *fled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */
+
+static inline int led_classdev_flash_register(struct device *parent,
+ struct led_classdev_flash *fled_cdev)
+{
+ return led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
+
static inline int devm_led_classdev_flash_register(struct device *parent,
struct led_classdev_flash *fled_cdev)
{
return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL);
}
-void devm_led_classdev_flash_unregister(struct device *parent,
- struct led_classdev_flash *fled_cdev);
-
/**
* led_set_flash_strobe - setup flash strobe
* @fled_cdev: the flash LED to set strobe on
It would be good if patch description mentioned also moving the
functions outside of #ifdef block.
With that:
Acked-by: Jacek Anaszewski
--
Best regards,
Jacek Anaszewski
Re: [PATCH v8 3/6] dt-bindings: leds: Add LED_FUNCTION_MOONLIGHT definitions
Hi Gene, On 11/25/20 11:51 AM, Gene Chen wrote: From: Gene Chen Add LED_FUNCTION_MOONLIGHT definitions Signed-off-by: Gene Chen --- include/dt-bindings/leds/common.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h index 52b619d..843e65d 100644 --- a/include/dt-bindings/leds/common.h +++ b/include/dt-bindings/leds/common.h @@ -78,6 +78,7 @@ #define LED_FUNCTION_INDICATOR "indicator" #define LED_FUNCTION_LAN "lan" #define LED_FUNCTION_MAIL "mail" +#define LED_FUNCTION_MOONLIGHT "moonlight" #define LED_FUNCTION_MTD "mtd" #define LED_FUNCTION_PANIC "panic" #define LED_FUNCTION_PROGRAMMING "programming" Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v8 2/6] leds: flash: Fix multicolor registration no-ops by return 0
Hi Gene,
Thank you for the fix.
Would you mind fixing in the same patch also a duplication of
led_classdev_multicolor_register() and
devm_led_classdev_multicolor_register(), by moving them
outside of #ifdef block ?
They look identical for both CONFIG_LEDS_CLASS_MULTICOLOR states.
On 11/25/20 11:51 AM, Gene Chen wrote:
From: Gene Chen
Fix multicolor registration no-ops by return 0
Signed-off-by: Gene Chen
---
include/linux/led-class-multicolor.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/led-class-multicolor.h
b/include/linux/led-class-multicolor.h
index 5116f9a..dbf3832 100644
--- a/include/linux/led-class-multicolor.h
+++ b/include/linux/led-class-multicolor.h
@@ -83,7 +83,7 @@ static inline int led_classdev_multicolor_register_ext(struct
device *parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data)
{
- return -EINVAL;
+ return 0;
}
static inline int led_classdev_multicolor_register(struct device *parent,
@@ -96,14 +96,14 @@ static inline void
led_classdev_multicolor_unregister(struct led_classdev_mc *mc
static inline int led_mc_calc_color_components(struct led_classdev_mc
*mcled_cdev,
enum led_brightness brightness)
{
- return -EINVAL;
+ return 0;
}
static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
struct led_classdev_mc *mcled_cdev,
struct led_init_data *init_data)
{
- return -EINVAL;
+ return 0;
}
static inline int devm_led_classdev_multicolor_register(struct device *parent,
--
Best regards,
Jacek Anaszewski
Re: [PATCH v8 6/6] leds: mt6360: Add LED driver for MT6360
Hi Gene, Thank you for the update. On 11/25/20 11:51 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. Signed-off-by: Gene Chen --- drivers/leds/Kconfig | 13 + drivers/leds/Makefile | 1 + drivers/leds/leds-mt6360.c | 811 + 3 files changed, 825 insertions(+) create mode 100644 drivers/leds/leds-mt6360.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df..4f533bc 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -271,6 +271,19 @@ config LEDS_MT6323 This option enables support for on-chip LED drivers found on Mediatek MT6323 PMIC. +config LEDS_MT6360 + tristate "LED Support for Mediatek MT6360 PMIC" + depends on LEDS_CLASS && OF + depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH + depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS + depends on MFD_MT6360 + help + This option enables support for dual Flash LED drivers found on + Mediatek MT6360 PMIC. + Independent current sources supply for each flash LED support torch + and strobe mode. Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v8 1/6] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Hi Gene,
On 11/25/20 11:51 AM, Gene Chen wrote:
From: Gene Chen
Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Signed-off-by: Gene Chen
---
include/linux/led-class-flash.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 21a3358..5f36eae 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
return container_of(lcdev, struct led_classdev_flash, led_cdev);
}
+#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
/**
* led_classdev_flash_register_ext - register a new object of LED class with
* init data and with support for flash LEDs
@@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct
device *parent,
void devm_led_classdev_flash_unregister(struct device *parent,
struct led_classdev_flash *fled_cdev);
+#else
+
+static inline int led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return 0;
+}
+
+static inline int led_classdev_flash_register(struct device *parent,
+ struct led_classdev_flash *fled_cdev)
+{
+ return led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
This function can be placed after #ifdef block - now it is in two copies
in this file.
+
+static inline void led_classdev_flash_unregister(struct led_classdev_flash
*fled_cdev) {};
+static inline int devm_led_classdev_flash_register_ext(struct device *parent,
+struct led_classdev_flash *fled_cdev,
+struct led_init_data *init_data)
+{
+ return 0;
+}
+
+static inline int devm_led_classdev_flash_register(struct device *parent,
+struct led_classdev_flash *fled_cdev)
+{
+ return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
Ditto.
+static inline void devm_led_classdev_flash_unregister(struct device *parent,
+ struct led_classdev_flash *fled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */
+
/**
* led_set_flash_strobe - setup flash strobe
* @fled_cdev: the flash LED to set strobe on
--
Best regards,
Jacek Anaszewski
Re: [PATCH v7 2/5] dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions
On 11/24/20 8:33 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年11月24日 週二 上午4:52寫道： On 11/23/20 4:00 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年11月20日 週五 上午6:26寫道： On 11/19/20 10:57 PM, Pavel Machek wrote: On Thu 2020-11-19 22:03:14, Jacek Anaszewski wrote: Hi Pavel, Gene, On 11/18/20 10:37 PM, Pavel Machek wrote: Hi! From: Gene Chen Add LED_COLOR_ID_MOONLIGHT definitions Why is moonlight a color? Camera flashes are usually white, no? At least it needs a comment... That's my fault, In fact I should have asked about adding LED_FUNCTION_MOONLIGHT, it was evidently too late for me that evening... Aha, that makes more sense. But please let's call it "torch" if we do that, as that is already used in kernel sources... and probably in the interface, too: I'd say that torch is something different that moonlight, but we would need more input from Gene to learn more about the nature of light emitted by ML LED on his device. Please note that torch is usually meant as the other mode of flash LED (sometimes it is called "movie mode"), which is already handled by brightness file of LED class flash device (i.e. its LED class subset), and which also maps to v4l2-flash TORCH mode. It's used to front camera fill light. More brightness than screen backlight, and more soft light than flash. I think LED_ID_COLOR_WHITE is okay. So why in v6 you assigned LED_COLOR_ID_AMBER to it? Regardless of that, now we're talking about LED function - you chose LED_FUNCTION_INDICATOR for it, but inferring from your above description - it certainly doesn't fit here. Also register names, containing part "ML" indicate that this LED's intended function is moonlinght, which your description somehow corroborates. Moonlight LEDs become ubiquitous nowadays so sooner or later we will need to add this function anyway [0]. [0] https://landscapelightingoakville.com/what-is-moon-lighting-and-why-does-it-remain-so-popular/ We use term "Moonlight" as reference says "When you are trying to imitate moonlight you need to use low voltage, softer lighting. You don’t want something that’s too bright" which is focus on brightness instead of color. So we surpose Moonlight can be white or amber. Should I add LED_FUNCTION_MOONLIGHT and set LED_COLOR_ID_WHITE? Regarding the function - yes, the reference backs that up. Regarding the color - if you feel that it properly describes the LED color then go for it. -- Best regards, Jacek Anaszewski
Re: [PATCH v7 1/5] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
On 11/24/20 7:08 AM, Gene Chen wrote:
[...]
This function should be placed after #ifdef block because its
shape is the same for both cases.
+static inline void led_classdev_flash_unregister(struct led_classdev_flash
*fled_cdev) {};
+static inline int devm_led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
/-EINVAL/0/
Please do the same fix in all no-ops in the led-class-multicolor.h,
as we've discussed.
I think return -EINVAL is correct, because I should register flash
light device if I define FLED in DTS node.
OK, I think I'm getting your concerns now. So you're only partially
correct - the driver should register flash LED device if there is
corresponding node in DT, but only if CONFIG_LEDS_CLASS_FLASH is
enabled. In case it is disabled the no-op will come into play
and return 0, allowing the probe() to proceed as if the registration
succeeded.
From the driver point of view nothing changes, except that flash LED
ops will not be called afterwards. This is common pattern. If in doubt
skim through the headers in include/linux.
I don't quite follow your logic here.
No-op function's purpose is to simplify the code on the caller's side.
Therefore it should report success.
Please return 0 from it.
Just like those functions in led-class-multicolor.h, caller may use
return value to check whether FLED is registered successfully or not.
For this case, is returning 0 a little bit misleading?
Please note that I've already admitted that led-class-multicolor.h
class is buggy and should also be fixed to return 0 from its no-ops.
Please apply the "s/-EINVAL/0/" fixes to it as well - your driver will
need that.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v7 1/5] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
On 11/23/20 4:20 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年11月20日 週五 上午6:29寫道：
Hi Gene,
On 11/18/20 11:47 AM, Gene Chen wrote:
From: Gene Chen
Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Signed-off-by: Gene Chen
---
include/linux/led-class-flash.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 21a3358..4f56c28 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
return container_of(lcdev, struct led_classdev_flash, led_cdev);
}
+#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
/**
* led_classdev_flash_register_ext - register a new object of LED class with
* init data and with support for flash LEDs
@@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct
device *parent,
void devm_led_classdev_flash_unregister(struct device *parent,
struct led_classdev_flash *fled_cdev);
+#else
+
+static inline int led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
s/-EINVAL/0/
The goal here is to assure that client will not fail when using no-op.
+}
+
+static inline int led_classdev_flash_register(struct device *parent,
+struct led_classdev_flash *fled_cdev)
+{
+ return led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
This function should be placed after #ifdef block because its
shape is the same for both cases.
+static inline void led_classdev_flash_unregister(struct led_classdev_flash
*fled_cdev) {};
+static inline int devm_led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
/-EINVAL/0/
Please do the same fix in all no-ops in the led-class-multicolor.h,
as we've discussed.
I think return -EINVAL is correct, because I should register flash
light device if I define FLED in DTS node.
I don't quite follow your logic here.
No-op function's purpose is to simplify the code on the caller's side.
Therefore it should report success.
Please return 0 from it.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v7 2/5] dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions
On 11/23/20 4:00 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年11月20日 週五 上午6:26寫道： On 11/19/20 10:57 PM, Pavel Machek wrote: On Thu 2020-11-19 22:03:14, Jacek Anaszewski wrote: Hi Pavel, Gene, On 11/18/20 10:37 PM, Pavel Machek wrote: Hi! From: Gene Chen Add LED_COLOR_ID_MOONLIGHT definitions Why is moonlight a color? Camera flashes are usually white, no? At least it needs a comment... That's my fault, In fact I should have asked about adding LED_FUNCTION_MOONLIGHT, it was evidently too late for me that evening... Aha, that makes more sense. But please let's call it "torch" if we do that, as that is already used in kernel sources... and probably in the interface, too: I'd say that torch is something different that moonlight, but we would need more input from Gene to learn more about the nature of light emitted by ML LED on his device. Please note that torch is usually meant as the other mode of flash LED (sometimes it is called "movie mode"), which is already handled by brightness file of LED class flash device (i.e. its LED class subset), and which also maps to v4l2-flash TORCH mode. It's used to front camera fill light. More brightness than screen backlight, and more soft light than flash. I think LED_ID_COLOR_WHITE is okay. So why in v6 you assigned LED_COLOR_ID_AMBER to it? Regardless of that, now we're talking about LED function - you chose LED_FUNCTION_INDICATOR for it, but inferring from your above description - it certainly doesn't fit here. Also register names, containing part "ML" indicate that this LED's intended function is moonlinght, which your description somehow corroborates. Moonlight LEDs become ubiquitous nowadays so sooner or later we will need to add this function anyway [0]. [0] https://landscapelightingoakville.com/what-is-moon-lighting-and-why-does-it-remain-so-popular/ -- Best regards, Jacek Anaszewski
Re: [PATCH v7 5/5] leds: mt6360: Add LED driver for MT6360
Hi Gene,
On 11/18/20 11:47 AM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for
moonlight LED.
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 13 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 808 +
3 files changed, 822 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..4f533bc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,19 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS && OF
+ depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
+ depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on MFD_MT6360
+ help
+ This option enables support for dual Flash LED drivers found on
+ Mediatek MT6360 PMIC.
+ Independent current sources supply for each flash LED support torch
+ and strobe mode.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7a..5596427 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532) += leds-rb532.o
obj-$(CONFIG_LEDS_MLXCPLD)+= leds-mlxcpld.o
obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
+obj-$(CONFIG_LEDS_MT6360) += leds-mt6360.o
obj-$(CONFIG_LEDS_NET48XX)+= leds-net48xx.o
obj-$(CONFIG_LEDS_NETXBIG)+= leds-netxbig.o
obj-$(CONFIG_LEDS_NIC78BX)+= leds-nic78bx.o
diff --git a/drivers/leds/leds-mt6360.c b/drivers/leds/leds-mt6360.c
new file mode 100644
index 000..8432901
--- /dev/null
+++ b/drivers/leds/leds-mt6360.c
@@ -0,0 +1,808 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
[...]
+static int mt6360_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
+{
+ struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led,
flash);
+ struct mt6360_priv *priv = led->priv;
+ u16 fled_stat;
+ unsigned int chg_stat, strobe_timeout_mask, fled_short_mask;
+ u32 rfault = 0;
+ int ret;
You need mutex here as well because you're making two readouts and
you have to assure atomicity of this operation.
+ ret = regmap_read(priv->regmap, MT6360_REG_CHGSTAT2, _stat);
+ if (ret)
+ return ret;
+
+ ret = regmap_raw_read(priv->regmap, MT6360_REG_FLEDSTAT1, _stat,
sizeof(fled_stat));
+ if (ret)
+ return ret;
+
+ if (led->led_no == MT6360_LED_FLASH1) {
+ strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
+ fled_short_mask = MT6360_FLED1SHORT_MASK;
+ } else {
+ strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
+ fled_short_mask = MT6360_FLED2SHORT_MASK;
+ }
+
+ if (chg_stat & MT6360_FLEDCHGVINOVP_MASK)
+ rfault |= LED_FAULT_INPUT_VOLTAGE;
+
+ if (fled_stat & strobe_timeout_mask)
+ rfault |= LED_FAULT_TIMEOUT;
+
+ if (fled_stat & fled_short_mask)
+ rfault |= LED_FAULT_SHORT_CIRCUIT;
+
+ if (fled_stat & MT6360_FLEDLVF_MASK)
+ rfault |= LED_FAULT_UNDER_VOLTAGE;
+
+ *fault = rfault;
+ return 0;
+}
+
[...]
--
Best regards,
Jacek Anaszewski
Re: [PATCH v7 1/5] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Hi Gene,
On 11/18/20 11:47 AM, Gene Chen wrote:
From: Gene Chen
Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
Signed-off-by: Gene Chen
---
include/linux/led-class-flash.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 21a3358..4f56c28 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
return container_of(lcdev, struct led_classdev_flash, led_cdev);
}
+#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
/**
* led_classdev_flash_register_ext - register a new object of LED class with
* init data and with support for flash LEDs
@@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct
device *parent,
void devm_led_classdev_flash_unregister(struct device *parent,
struct led_classdev_flash *fled_cdev);
+#else
+
+static inline int led_classdev_flash_register_ext(struct device *parent,
+ struct led_classdev_flash *fled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
s/-EINVAL/0/
The goal here is to assure that client will not fail when using no-op.
+}
+
+static inline int led_classdev_flash_register(struct device *parent,
+ struct led_classdev_flash *fled_cdev)
+{
+ return led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
This function should be placed after #ifdef block because its
shape is the same for both cases.
+static inline void led_classdev_flash_unregister(struct led_classdev_flash
*fled_cdev) {};
+static inline int devm_led_classdev_flash_register_ext(struct device *parent,
+struct led_classdev_flash *fled_cdev,
+struct led_init_data *init_data)
+{
+ return -EINVAL;
/-EINVAL/0/
Please do the same fix in all no-ops in the led-class-multicolor.h,
as we've discussed.
+}
+
+static inline int devm_led_classdev_flash_register(struct device *parent,
+struct led_classdev_flash *fled_cdev)
+{
+ return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL);
+}
This function should also be placed after #ifdef block.
Please make the same optimizations in the led-class-multicolor.h as you
are at it.
+
+void devm_led_classdev_flash_unregister(struct device *parent,
s/void/static inline void/
That's the reason why you got warning from buildbot.
+ struct led_classdev_flash *fled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */
+
/**
* led_set_flash_strobe - setup flash strobe
* @fled_cdev: the flash LED to set strobe on
--
Best regards,
Jacek Anaszewski
Re: [PATCH v7 2/5] dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions
On 11/19/20 10:57 PM, Pavel Machek wrote: On Thu 2020-11-19 22:03:14, Jacek Anaszewski wrote: Hi Pavel, Gene, On 11/18/20 10:37 PM, Pavel Machek wrote: Hi! From: Gene Chen Add LED_COLOR_ID_MOONLIGHT definitions Why is moonlight a color? Camera flashes are usually white, no? At least it needs a comment... That's my fault, In fact I should have asked about adding LED_FUNCTION_MOONLIGHT, it was evidently too late for me that evening... Aha, that makes more sense. But please let's call it "torch" if we do that, as that is already used in kernel sources... and probably in the interface, too: I'd say that torch is something different that moonlight, but we would need more input from Gene to learn more about the nature of light emitted by ML LED on his device. Please note that torch is usually meant as the other mode of flash LED (sometimes it is called "movie mode"), which is already handled by brightness file of LED class flash device (i.e. its LED class subset), and which also maps to v4l2-flash TORCH mode. ./arch/arm/mach-pxa/ezx.c:.name = "a910::torch", Best regards, Pavel -- Best regards, Jacek Anaszewski
Re: [PATCH v7 2/5] dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions
Hi Pavel, Gene, On 11/18/20 10:37 PM, Pavel Machek wrote: Hi! From: Gene Chen Add LED_COLOR_ID_MOONLIGHT definitions Why is moonlight a color? Camera flashes are usually white, no? At least it needs a comment... That's my fault, In fact I should have asked about adding LED_FUNCTION_MOONLIGHT, it was evidently too late for me that evening... -- Best regards, Jacek Anaszewski
Re: [PATCH v6 2/2] leds: mt6360: Add LED driver for MT6360
(>dev);
+ if (!count || count > MT6360_MAX_LEDS) {
+ dev_err(>dev, "No child node or node count over max led number
%lu\n", count);
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(>dev, struct_size(priv, leds, count),
GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->leds_count = count;
+ priv->dev = >dev;
+ mutex_init(>lock);
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap) {
+ dev_err(>dev, "Failed to get parent regmap\n");
+ return -ENODEV;
+ }
+
+ device_for_each_child_node(>dev, child) {
+ struct mt6360_led *led = priv->leds + i;
+ struct led_init_data init_data = { .fwnode = child, };
+ u32 reg;
+
+ ret = fwnode_property_read_u32(child, "reg", );
+ if (ret)
+ goto out_flash_release;
+
+ if (reg > MT6360_MAX_LEDS || priv->leds_active & BIT(reg))
+ return -EINVAL;
+ priv->leds_active |= BIT(reg);
+
+ led->led_no = reg;
+ led->priv = priv;
+
+ ret = mt6360_init_common_properties(led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ if (reg == MT6360_LED_MULTICOLOR ||
+ (reg >= MT6360_LED_ISNK1 && reg <= MT6360_LED_ISNKML))
+ ret = mt6360_init_isnk_properties(led, _data);
+ else
+ ret = mt6360_init_flash_properties(led, _data);
+
+ if (ret)
+ goto out_flash_release;
+
+ ret = mt6360_led_register(>dev, led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ i++;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+
+out_flash_release:
+ mt6360_v4l2_flash_release(priv);
+ return ret;
+}
+
+static int mt6360_led_remove(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv = platform_get_drvdata(pdev);
+
+ mt6360_v4l2_flash_release(priv);
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
+ { .compatible = "mediatek,mt6360-led", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mt6360_led_of_id);
+
+static struct platform_driver mt6360_led_driver = {
+ .driver = {
+ .name = "mt6360-led",
+ .of_match_table = mt6360_led_of_id,
+ },
+ .probe = mt6360_led_probe,
+ .remove = mt6360_led_remove,
+};
+module_platform_driver(mt6360_led_driver);
+
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 LED Driver");
+MODULE_LICENSE("GPL v2");
--
Best regards,
Jacek Anaszewski
Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for MT6360 LED
Hi Gene,
Thank you for the patch.
On 11/17/20 11:55 AM, Gene Chen wrote:
From: Gene Chen
Add bindings document for LED support on MT6360 PMIC
Signed-off-by: Gene Chen
---
.../devicetree/bindings/leds/leds-mt6360.yaml | 114 +
1 file changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
new file mode 100644
index 000..871db4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for MT6360 PMIC from MediaTek Integrated.
+
+maintainers:
+ - Gene Chen
+
+description: |
+ This module is part of the MT6360 MFD device.
+ see Documentation/devicetree/bindings/mfd/mt6360.yaml
+ Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
+ and 4-channel RGB LED support Register/Flash/Breath Mode
What actually is the Register mode?
+
+properties:
+ compatible:
+const: mediatek,mt6360-led
+
+ "#address-cells":
+const: 1
+
+ "#size-cells":
+const: 0
+
+patternProperties:
+ "^led@[0-6]$":
+type: object
+$ref: common.yaml#
+description:
+ Properties for a single LED.
+
+properties:
+ reg:
+description: Index of the LED.
+enum:
+ - 0 # LED output INDICATOR1_RED
+ - 1 # LED output INDICATOR1_GREEN
+ - 2 # LED output INDICATOR1_BLUE
+ - 3 # LED output INDICATOR2_ML
+ - 4 # LED output FLED1
+ - 5 # LED output FLED2
+ - 6 # LED output MULTICOLOR
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include
+ led-controller {
+ compatible = "mediatek,mt6360-led";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
+ led-max-microamp = <24000>;
+ };
+ led@3 {
+ reg = <3>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
You should really have here LED_COLOR_ID_MOONLIGHT if this is
a moonlight LED. You'll need to add it to dt-bindings/leds/common.h.
+ led-max-microamp = <15>;
+ };
+ led@4 {
+ reg = <4>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <1>;
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@5 {
+ reg = <5>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <2>;
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@6 {
+ reg = <6>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
+ led-max-microamp = <24000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@1 {
+ reg = <1>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
+ };
+ led@2 {
+ reg = <2>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
+ };
+ };
It is of little avail to have multicolor LED with only two LEDs.
I propose not to allow such setup - i.e. you should have either
one multicolor LED comprising three sub-LEDs (regs: 0, 1, 2),
and with main color property set to LED_COLOR_ID_RGB, or three separate
LEDs.
Effectively, you should have two separate DT examples here to make it
clear: one for the case with three LED class devices and one with
LED multicolor class device.
+ };
+...
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360
On 11/16/20 11:01 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年10月31日 週六 上午6:34寫道：
On 10/30/20 9:51 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年10月28日 週三 上午1:28寫道：
On 10/27/20 10:28 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年10月21日 週三 上午5:47寫道：
On 10/20/20 8:44 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年10月9日 週五 上午5:51寫道：
Hi Gene,
On 10/7/20 3:42 AM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for
moonlight LED.
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 12 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 783
+
3 files changed, 796 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..c7192dd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,18 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS_FLASH && OF
+ depends on LEDS_CLASS_MULTICOLOR
Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have
below instead:
depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR
My typo here, should be one "!":
depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
And you should also have
depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
But to make it work correctly you would have to add registration
stubs to include/linux/led-class-flash.h similarly to LED mc stubs
in include/linux/led-class-multicolor.h.
Unless you want to prevent enabling the driver without RGB LED,
but that does not seem to be reasonable at first glance.
May I change to "select LEDS_CLASS_MULTICOLOR"?
I suppose RGB always use multicolor mode.
You will also have moonlight LED that will not need multicolor
framework. Is it somehow troublesome to keep "depends on"?
If only use ML LED and FLED, DTSI will only define ML LED and FLED.
Therefore, the drivers probe will not register rgb multicolor device.
Please test your use case again with my fixed "depends on".
In case when there is only ML LED and FLED in the DT it should
register both devices if LEDS_CLASS_FLASH is turned on.
Multicolor framework has nothing to do in this case.
But if you additionally had MC LED node, then it should
be registered only if LEDS_CLASS_MULTICOLOR is enabled.
Similarly, when FLED node is present, but LEDS_CLASS_FLASH
is off, and LEDS_CLASS_MULTICOLOR is on, the driver should still
compile, but register only LED MC device (if its node is present).
I think this case only register LED device, not LED "MC" device.
Because our FLASH is not a multicolor device.
No, here I was describing following setup:
- DT FLED node is present, CONFIG_LEDS_CLASS_FLASH is off
- DT MC node is present, CONFIG_LEDS_CLASS_MULTICOLOR is on
ML LED presence in DT is irrelevant in this case.
It should be always registered if there is corresponding DT node
and LEDS_CLASS is on.
As a long time discussion, we conclude some rules about MT6360 LED driver.
FLED is necessary, so Kconfig depends on LED_CLASS_FLASH
Maybe it is necessary in your use case, but probably it is possible to
use the device without FLED. If so, then you should allow users
disabling it. Therefore you should have:
depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
ML LED is optional, which is registered as led class device.
RGB LED can be either simple led class device or multicolor device,
which is decided in DT node
If Multicolor LED DT node is exist, it should be register multicolor
device success.
But only if CONFIG_LEDS_CLASS_MULTICOLOR is enabled.
Maybe it is more specific to send a new patch?
Sample DT as below
LED "red" is simple led class device, LED "green" is multicolor devices.
led@0 {
reg = <0>;
function = LED_FUNCTION_INDICATOR;
color = ;
led-max-microamp = <24000>;
};
led@6 {
reg = <6>;
function = LED_FUNCTION_INDICATOR;
color = ;
led@1 {
reg = <1>;
function = LED_FUNCTION_INDICATOR;
color = ;
led-max-microamp = <24000>;
};
led@2 {
reg = <2>;
function = LED_FUNCTION_INDICATOR;
color = ;
led-max-microamp = <24000>;
};
};
It looks OK.
Possible should be also the case when both LEDS_CLASS_FLASH
and LEDS_CLASS_MULTICOLOR are off. Then only LED class device
for ML LED will be registered (provided there is ML DT node).
But to make it possible you should
Re: [PATCH] s5p-jpeg: hangle error condition in s5p_jpeg_probe
Hi Evgeiny, Thank you for the patch. There is a typo in the subject: s/hangle/handle/ Otherwise: Acked-by: Jacek Anaszewski On 11/13/20 5:06 PM, Baskov Evgeiny wrote: If an error happens in jpeg_get_drv_data(), i.e. match fails, jpeg->variant field is NULL, so we cannot access it. Consider device probe failed if jpeg->variant is NULL. -- Best regards, Jacek Anaszewski
Re: [RFC PATCH 1/3] leds: Add driver for QPNP flash led
Hi Nicolas, We have LED flash class framework since 2015. Please refer to the following files: Documentation/leds/leds-class-flash.rst Documentation/ABI/testing/sysfs-class-led-flash Documentation/devicetree/bindings/leds/common.yaml drivers/leds/led-class-flash.c Thare are also few LED flash drivers in the tree. Since there seems to be boost feature present on the the device then you might want to compare drivers/leds/leds-max77693.c with its bindings Documentation/devicetree/bindings/mfd/max77693.txt (refer to LED part). Please also remember to include DT bindings patch to your series. On 11/6/20 5:58 PM, Nícolas F. R. A. Prado wrote: Add driver for the QPNP flash LED. It works over SPMI and is part of the PM8941 PMIC. Signed-off-by: Nícolas F. R. A. Prado --- drivers/leds/Kconfig |9 + drivers/leds/Makefile|1 + drivers/leds/leds-qpnp.c | 1351 ++ 3 files changed, 1361 insertions(+) create mode 100644 drivers/leds/leds-qpnp.c -- Best regards, Jacek Anaszewski
Re: [PATCH v2 1/2] leds: rt4505: Add support for Richtek RT4505 flash LED controller
Hi ChiYuan, On 11/2/20 3:42 AM, cy_huang wrote: From: ChiYuan Huang Add support for RT4505 flash LED controller. It can support up to 1.5A flash current with hardware timeout and low input voltage protection. Signed-off-by: ChiYuan Huang --- Changes since v1 to v2 - Create flash directory into drvers/leds. - Coding style fix to meet 80 charactors per line limit. - Refine the description in the Kconfig help text. - Change all descriptions for 'led' text to uppercase 'LED'. --- drivers/leds/Kconfig | 2 + drivers/leds/Makefile| 3 + drivers/leds/flash/Kconfig | 17 ++ drivers/leds/flash/Makefile | 2 + drivers/leds/flash/leds-rt4505.c | 430 +++ 5 files changed, 454 insertions(+) create mode 100644 drivers/leds/flash/Kconfig create mode 100644 drivers/leds/flash/Makefile create mode 100644 drivers/leds/flash/leds-rt4505.c Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360
On 10/30/20 9:51 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月28日 週三 上午1:28寫道： On 10/27/20 10:28 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月21日 週三 上午5:47寫道： On 10/20/20 8:44 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月9日 週五 上午5:51寫道： Hi Gene, On 10/7/20 3:42 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. Signed-off-by: Gene Chen --- drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-mt6360.c | 783 + 3 files changed, 796 insertions(+) create mode 100644 drivers/leds/leds-mt6360.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df..c7192dd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -271,6 +271,18 @@ config LEDS_MT6323 This option enables support for on-chip LED drivers found on Mediatek MT6323 PMIC. +config LEDS_MT6360 + tristate "LED Support for Mediatek MT6360 PMIC" + depends on LEDS_CLASS_FLASH && OF + depends on LEDS_CLASS_MULTICOLOR Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have below instead: depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR My typo here, should be one "!": depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR And you should also have depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH But to make it work correctly you would have to add registration stubs to include/linux/led-class-flash.h similarly to LED mc stubs in include/linux/led-class-multicolor.h. Unless you want to prevent enabling the driver without RGB LED, but that does not seem to be reasonable at first glance. May I change to "select LEDS_CLASS_MULTICOLOR"? I suppose RGB always use multicolor mode. You will also have moonlight LED that will not need multicolor framework. Is it somehow troublesome to keep "depends on"? If only use ML LED and FLED, DTSI will only define ML LED and FLED. Therefore, the drivers probe will not register rgb multicolor device. Please test your use case again with my fixed "depends on". In case when there is only ML LED and FLED in the DT it should register both devices if LEDS_CLASS_FLASH is turned on. Multicolor framework has nothing to do in this case. But if you additionally had MC LED node, then it should be registered only if LEDS_CLASS_MULTICOLOR is enabled. Similarly, when FLED node is present, but LEDS_CLASS_FLASH is off, and LEDS_CLASS_MULTICOLOR is on, the driver should still compile, but register only LED MC device (if its node is present). I think this case only register LED device, not LED "MC" device. Because our FLASH is not a multicolor device. No, here I was describing following setup: - DT FLED node is present, CONFIG_LEDS_CLASS_FLASH is off - DT MC node is present, CONFIG_LEDS_CLASS_MULTICOLOR is on ML LED presence in DT is irrelevant in this case. It should be always registered if there is corresponding DT node and LEDS_CLASS is on. Possible should be also the case when both LEDS_CLASS_FLASH and LEDS_CLASS_MULTICOLOR are off. Then only LED class device for ML LED will be registered (provided there is ML DT node). But to make it possible you should have also "depends on LEDS_CLASS" in the Kconfig entry. According to your suggestion, depends on LED_CLASS && LEDS_CLASS_FLASH && OF s/LED_CLASS/LEDS_CLASS/ And you have to remove LEDS_CLASS_FLASH from above line. depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR s/!!LEDS_CLASS_MULTICOLOR/!LEDS_CLASS_MULTICOLOR/ depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH depends on MFD_MT6360 You will need V4L2_FLASH_LED_CLASS dependency as well, to avoid build break, when it is set to 'm'. To recap, following block of dependencies is required: depends on LEDS_CLASS && OF depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS depends on MFD_MT6360 and source code add constraint #if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) ret = devm_led_classdev_multicolor_register_ext(parent, >rgb, init_data); #endif #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) ret = devm_led_classdev_flash_register_ext(parent, >flash, init_data); #endif No, the guards should be in headers. That's why I recommended adding no ops for LED flash class registration functions in previous email. Please compare include/linux/led-class-multicolor.h and do similar changes in include/linux/led-class-flash.h. = Or Should I seperate two drivers? one for RGB LED, one for ML LED and FLED This would incur unnecessary code duplication. -- Best regards, Jacek Anaszewski
Re: [PATCH 33/33] docs: ABI: unify /sys/class/leds//max_brightness documentation
Hi Mauro, Patch title needs fixing: s/max_brightness/brightness/ On 10/28/20 3:23 PM, Mauro Carvalho Chehab wrote: This ABI is defined twice, one for normal leds and another one for multicolor ones. Ensure that just one definition is there at ABI. Signed-off-by: Mauro Carvalho Chehab --- Documentation/ABI/testing/sysfs-class-led | 25 --- .../ABI/testing/sysfs-class-led-multicolor| 23 + 2 files changed, 28 insertions(+), 20 deletions(-) [...] + What: /sys/class/leds//max_brightness Date: March 2006 KernelVersion:2.6.17 diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor index eeeddcbdbbe3..16fc827b10cb 100644 --- a/Documentation/ABI/testing/sysfs-class-led-multicolor +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor @@ -1,20 +1,3 @@ -What: /sys/class/leds//brightness -Date: March 2020 -KernelVersion: 5.9 -Contact: Dan Murphy -Description: read/write - Writing to this file will update all LEDs within the group to a - calculated percentage of what each color LED intensity is set - to. The percentage is calculated for each grouped LED via the - equation below: -- Best regards, Jacek Anaszewski
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
On 10/28/20 5:57 AM, ChiYuan Huang wrote:
Hi,
Jacek Anaszewski 於 2020年10月28日 週三 上午12:40寫道：
Hi Pavel, ChiYuan,
On 10/27/20 9:29 AM, Pavel Machek wrote:
Hi!
From: ChiYuan Huang
Add support for RT4505 flash led controller. It can support up to 1.5A
flash current with hardware timeout and low input voltage
protection.
Please use upper-case "LED" everywhere.
This should be 2nd in the series, after DT changes.
Signed-off-by: ChiYuan Huang
---
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-rt4505.c | 397
+
3 files changed, 409 insertions(+)
create mode 100644 drivers/leds/leds-rt4505.c
[...]
+static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
led_brightness level)
+{
80 columns, where easy.
+struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
flash.led_cdev);
+u32 val = 0;
+int ret;
+
+mutex_lock(>lock);
+
+if (level != LED_OFF) {
+ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
RT4505_ITORCH_MASK,
+ (level - 1) << RT4505_ITORCH_SHIFT);
+if (ret)
+goto unlock;
+
+val = RT4505_TORCH_SET;
+}
+
+ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
RT4505_ENABLE_MASK, val);
+
+unlock:
+mutex_unlock(>lock);
+return ret;
+}
Why is the locking needed? What will the /sys/class/leds interface
look like on system with your flash?
The locking is needed since this can be called via led_set_brightness()
from any place in the kernel, and especially from triggers.
From this case, It means only led classdev
brihtness_get/brightness_set need to be protected.
I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
Like as described in last mail, flash related operation is protected
by led access_lock and v4l2 framework.
I'll keep the locking only in led classdev brightness_get/brightness_set API.
If I misunderstand something, please help to point out.
Locking have to be used consistently for each access to the resource
being protected with the lock. Otherwise you can end up in a situation
when rt4505_torch_brightness_set and rt4505_flash_brightness_set will
try concurrently alter hardware state. Regardless of how harmful could
it be in case of this particular device it is certainly against
programming rules.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360
On 10/27/20 10:28 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月21日 週三 上午5:47寫道： On 10/20/20 8:44 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月9日 週五 上午5:51寫道： Hi Gene, On 10/7/20 3:42 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. Signed-off-by: Gene Chen --- drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-mt6360.c | 783 + 3 files changed, 796 insertions(+) create mode 100644 drivers/leds/leds-mt6360.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df..c7192dd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -271,6 +271,18 @@ config LEDS_MT6323 This option enables support for on-chip LED drivers found on Mediatek MT6323 PMIC. +config LEDS_MT6360 + tristate "LED Support for Mediatek MT6360 PMIC" + depends on LEDS_CLASS_FLASH && OF + depends on LEDS_CLASS_MULTICOLOR Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have below instead: depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR My typo here, should be one "!": depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR And you should also have depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH But to make it work correctly you would have to add registration stubs to include/linux/led-class-flash.h similarly to LED mc stubs in include/linux/led-class-multicolor.h. Unless you want to prevent enabling the driver without RGB LED, but that does not seem to be reasonable at first glance. May I change to "select LEDS_CLASS_MULTICOLOR"? I suppose RGB always use multicolor mode. You will also have moonlight LED that will not need multicolor framework. Is it somehow troublesome to keep "depends on"? If only use ML LED and FLED, DTSI will only define ML LED and FLED. Therefore, the drivers probe will not register rgb multicolor device. Please test your use case again with my fixed "depends on". In case when there is only ML LED and FLED in the DT it should register both devices if LEDS_CLASS_FLASH is turned on. Multicolor framework has nothing to do in this case. But if you additionally had MC LED node, then it should be registered only if LEDS_CLASS_MULTICOLOR is enabled. Similarly, when FLED node is present, but LEDS_CLASS_FLASH is off, and LEDS_CLASS_MULTICOLOR is on, the driver should still compile, but register only LED MC device (if its node is present). Possible should be also the case when both LEDS_CLASS_FLASH and LEDS_CLASS_MULTICOLOR are off. Then only LED class device for ML LED will be registered (provided there is ML DT node). But to make it possible you should have also "depends on LEDS_CLASS" in the Kconfig entry. I will remove "depends", use "select" instead. -- Best regards, Jacek Anaszewski
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Hi Pavel, ChiYuan,
On 10/27/20 9:29 AM, Pavel Machek wrote:
Hi!
From: ChiYuan Huang
Add support for RT4505 flash led controller. It can support up to 1.5A
flash current with hardware timeout and low input voltage
protection.
Please use upper-case "LED" everywhere.
This should be 2nd in the series, after DT changes.
Signed-off-by: ChiYuan Huang
---
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-rt4505.c | 397 +
3 files changed, 409 insertions(+)
create mode 100644 drivers/leds/leds-rt4505.c
[...]
+static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
led_brightness level)
+{
80 columns, where easy.
+ struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
flash.led_cdev);
+ u32 val = 0;
+ int ret;
+
+ mutex_lock(>lock);
+
+ if (level != LED_OFF) {
+ ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
RT4505_ITORCH_MASK,
+(level - 1) << RT4505_ITORCH_SHIFT);
+ if (ret)
+ goto unlock;
+
+ val = RT4505_TORCH_SET;
+ }
+
+ ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
RT4505_ENABLE_MASK, val);
+
+unlock:
+ mutex_unlock(>lock);
+ return ret;
+}
Why is the locking needed? What will the /sys/class/leds interface
look like on system with your flash?
The locking is needed since this can be called via led_set_brightness()
from any place in the kernel, and especially from triggers.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360
On 10/20/20 8:44 AM, Gene Chen wrote: Jacek Anaszewski 於 2020年10月9日 週五 上午5:51寫道： Hi Gene, On 10/7/20 3:42 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. Signed-off-by: Gene Chen --- drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-mt6360.c | 783 + 3 files changed, 796 insertions(+) create mode 100644 drivers/leds/leds-mt6360.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df..c7192dd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -271,6 +271,18 @@ config LEDS_MT6323 This option enables support for on-chip LED drivers found on Mediatek MT6323 PMIC. +config LEDS_MT6360 + tristate "LED Support for Mediatek MT6360 PMIC" + depends on LEDS_CLASS_FLASH && OF + depends on LEDS_CLASS_MULTICOLOR Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have below instead: depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR Unless you want to prevent enabling the driver without RGB LED, but that does not seem to be reasonable at first glance. May I change to "select LEDS_CLASS_MULTICOLOR"? I suppose RGB always use multicolor mode. You will also have moonlight LED that will not need multicolor framework. Is it somehow troublesome to keep "depends on"? -- Best regards, Jacek Anaszewski
Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360
Hi Gene, On 10/7/20 3:42 AM, Gene Chen wrote: From: Gene Chen Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode, 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for moonlight LED. Signed-off-by: Gene Chen --- drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-mt6360.c | 783 + 3 files changed, 796 insertions(+) create mode 100644 drivers/leds/leds-mt6360.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df..c7192dd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -271,6 +271,18 @@ config LEDS_MT6323 This option enables support for on-chip LED drivers found on Mediatek MT6323 PMIC. +config LEDS_MT6360 + tristate "LED Support for Mediatek MT6360 PMIC" + depends on LEDS_CLASS_FLASH && OF + depends on LEDS_CLASS_MULTICOLOR Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have below instead: depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR Unless you want to prevent enabling the driver without RGB LED, but that does not seem to be reasonable at first glance. + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS + depends on MFD_MT6360 + help + This option enables support for dual Flash LED drivers found on + Mediatek MT6360 PMIC. + Independent current sources supply for each flash LED support torch + and strobe mode. + -- Best regards, Jacek Anaszewski
Re: [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED
Hi Gene,
Thanks for the update.
On 10/7/20 3:42 AM, Gene Chen wrote:
From: Gene Chen
Add bindings document for LED support on MT6360 PMIC
Signed-off-by: Gene Chen
---
.../devicetree/bindings/leds/leds-mt6360.yaml | 95 ++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
new file mode 100644
index 000..2fa636f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for MT6360 PMIC from MediaTek Integrated.
+
+maintainers:
+ - Gene Chen
+
+description: |
+ This module is part of the MT6360 MFD device.
+ see Documentation/devicetree/bindings/mfd/mt6360.yaml
+ Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
+ and 4-channel RGB LED support Register/Flash/Breath Mode
+
+properties:
+ compatible:
+const: mediatek,mt6360-led
+
+ "#address-cells":
+const: 1
+
+ "#size-cells":
+const: 0
+
+patternProperties:
+ "^led@[0-3]$":
+type: object
+$ref: common.yaml#
+description:
+ Properties for a single LED.
+
+properties:
+ reg:
+description: Index of the LED.
+enum:
+ - 0 # LED output INDICATOR1_RGB
+ - 1 # LED output INDICATOR2
+ - 2 # LED output FLED1
+ - 3 # LED output FLED2
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include
+ led-controller {
+ compatible = "mediatek,mt6360-led";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ function = LED_FUNCTION_INDICATOR;
+ color = ;
+ led-max-microamp = <24000>;
+ };
This should be multi-led node. See [0] for a reference.
+ led@1 {
+ reg = <1>;
+ function = LED_FUNCTION_INDICATOR;
Maybe add LED_FUNCTION_MOONLIGHT ?
+ color = ;
+ default-state = "off";
+ led-max-microamp = <15>;
+ };
+ led@2 {
+ reg = <2>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <1>;
+ default-state = "off";
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@3 {
+ reg = <3>;
+ function = LED_FUNCTION_FLASH;
+ color = ;
+ function-enumerator = <2>;
+ default-state = "off";
+ led-max-microamp = <20>;
+ flash-max-microamp = <50>;
+ flash-max-timeout-us = <1024000>;
+ };
+ };
+...
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
On 9/25/20 11:40 AM, Pavel Machek wrote: Hi! So.. no, it is not causing kernel crashes or something. But it is example of bad interface, and _that_ is causing problems. (And yes, if I realized it is simply possible to limit it, maybe the BIN_ATTR conversion would not be neccessary...) The limitation you proposed breaks the trigger on many plafforms. Actually it precludes its use. I still see the patch in your linux-next, so I reserve myself the right to comment on your pull request. You are free to comment on anything. I believe probability someone uses that with more than 4 CPUs is < 5%. So you even didn't bother to check: $ git grep "default-trigger = \"cpu[4-9]" arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: linux,default-trigger = "cpu4"; arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: linux,default-trigger = "cpu5"; arch/arm/boot/dts/vexpress-v2m.dtsi: linux,default-trigger = "cpu4"; arch/arm/boot/dts/vexpress-v2m.dtsi: linux,default-trigger = "cpu5"; cpus are enumerated starting from 0, so there are more reasons for which your patch is broken: 1. There are mainline users. 2. You claim that you limit trigger use to 4 cpus, while the number is actually 5, due to your condition: + if (cpu > 4) + continue; 3. For platforms exceeding the limit the number of triggers registered would not match the number all available cpus, for no obvious reason. Better solution would be to prevent use of the trigger entirely in such cases, which would need only to alter first instruction in ledtrig_cpu_init(), which currently is: BUILD_BUG_ON(CONFIG_NR_CPUS > ); However I am not in favor of this approach since after removing PAGE_LIMIT size on triggers file, the trigger doesn't longer cause problems even with hypothetical 1000 cpus. The correct approach would be to create new trigger with better interface and then advise people switching to it. Probability that someone uses it with more than 100 CPUs is << 1% I'd say. Systems just don't have that many LEDs. I'll take the risk. If I broke someone's real, existing setup, I'll raise the limit. Is this professional approach - throw a potential bug at users and check if it will hit them? :-) And for no reason - you're not fixing anything. (With exception of uled setups. In such cases, I'll just laugh.) If you know or can find out someone using it with more than 4 CPUs, I can obviously raise the limit before the merge. -- Best regards, Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
On 9/22/20 10:41 PM, Jacek Anaszewski wrote:
Hi Pavel,
On 9/22/20 12:42 AM, Pavel Machek wrote:
Hi!
Can I get details of your setup?
I don't use this trigger, but I can imagine that someone does.
Well, if someone exists, we can increase the limit, or convince them
to change their setup.
Linux is used in many commercial projects and each such change generates
a cost, so this is not a sheer matter of convincing someone.
What CPU type that is, and how are you mapping CPU activity to LEDs?
The type of CPU is irrelevant here. What is important is the fact
that with this trigger it is possible to visually monitor CPU core
online state. Probably it would be good to ask the author of that
trigger about his use case.
It is relevant -- cpu trigger never worked on x86. I had patch fixing
it, but got pushback.
You mean literally x86 (32-bit)? Because I checked yesterday on my
x86_64 and it worked just fine, i.e. changing cpu online state
generated events on all userspace LEDs I registered for each cpuN
trigger.
I have spoken up, because I don't get the reason for your patch.
This driver was reworked year ago to remove PAGE_SIZE limit,
and I even applied it to my for-next tree, but that was at
the time of handling maintainership to yourself, and you
seem to not have picked that commit.
Was that intentional? We had even Greg's ack [0].
I checked, and I believe the commit is in:
Indeed, I blindly sought the changeset in git log for ledtrig-cpu.c
file.
#ifdef CONFIG_LEDS_TRIGGERS
static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
0);
static struct bin_attribute *led_trigger_bin_attrs[] = {
So.. no, it is not causing kernel crashes or something. But it is
example of bad interface, and _that_ is causing problems. (And yes, if
I realized it is simply possible to limit it, maybe the BIN_ATTR
conversion would not be neccessary...)
The limitation you proposed breaks the trigger on many plafforms.
Actually it precludes its use.
I still see the patch in your linux-next, so I reserve myself the
right to comment on your pull request.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 2/2] leds: mt6360: Add LED driver for MT6360
On 9/24/20 8:21 AM, Gene Chen wrote:
Jacek Anaszewski 於 2020年9月24日 週四 上午5:49寫道：
Hi Gene,
Thank you for the update. I have some more comments below.
On 9/23/20 2:50 PM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
and 4-channel RGB LED support Register/Flash/Breath Mode
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 705
+
3 files changed, 717 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..5561b08 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,17 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS_FLASH && OF
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on MFD_MT6360
+ help
+ This option enables support for dual Flash LED drivers found on
+ Mediatek MT6360 PMIC.
+ Independent current sources supply for each flash LED support torch
+ and strobe mode.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7a..5596427 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532) += leds-rb532.o
obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o
obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
+obj-$(CONFIG_LEDS_MT6360)+= leds-mt6360.o
obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o
obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o
diff --git a/drivers/leds/leds-mt6360.c b/drivers/leds/leds-mt6360.c
new file mode 100644
index 000..1c3486e
--- /dev/null
+++ b/drivers/leds/leds-mt6360.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+enum {
+ MT6360_LED_ISNK1 = 0,
+ MT6360_LED_ISNK2,
+ MT6360_LED_ISNK3,
+ MT6360_LED_ISNK4,
One question about these ISINKs - how are they exploited in your device?
Are these LEDs used to indicate camera activity or it is one RGB LED
for status? And what functionality has the remaining amber one (sticking
to the naming from your DT bindings)?
Can you share how the documenation for this device describes the purpose
of these sinks, if it does it at all?
I got probably mislead by your naming in the driver and got fixed on
their function as camera activity indicators, for which V4L2 has
support. If that is not the case, then you'd better switch to using
multicolor framework for all four "indicator" LEDs.
It's one RGB LED for status, not for camera.
The MT6360 integrates a three-channel RGB LED driver, designed to
provide a variety of lighting effects for mobile device applications.
The RGB LED driver includes a smart LED string controller, and it can
drive 3 channels of LEDs with a sink current of up to 24mA. The
default setting of RGB_ISINK1 is auto mode for TA charging indicator,
and RGB_ISINK1 also supports software mode. It provides three
operation modes for the RGB LEDs: flash mode, breath mode, and
register mode. The device can increase or decrease the brightness of
the RGB LEDs upon command via the I2C interface. The RGB_ISINK4
provide more sink current up to 150mA, which we can moonlight mode.
Do you mean we should remove "isink register v4l2 device, only need
register ledclass device"?
I'd say that in addition to the LED flash class device, you should
allow for registering LED multicolor device comprising RGB LEDs,
and one LED class device for ISINK4 (you could even add
LED_FUNCTION_MOONLIGHT for it).
I wonder what others think.
[...]
+static int mt6360_led_probe(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv;
+ struct fwnode_handle *child;
+ size_t count;
+ int i = 0, ret;
+
+ count = device_get_child_node_count(>dev);
+ if (!count || count > MT6360_MAX_LEDS)
Please add dev_err() here.
dev_err(>dev, "No child node or node count over max led number
%d\n", count);
If we will exploit also LED multicolor class then DT bindings will
look different, so let's discuss this detail later.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 2/2] leds: mt6360: Add LED driver for MT6360
ret = match_string(states, ARRAY_SIZE(states), str);
+ if (ret < 0)
+ ret = STATE_OFF;
+
+ led->default_state = ret;
+ }
+
+ return 0;
+}
+
+static void mt6360_v4l2_flash_release(struct mt6360_priv *priv)
+{
+ int i;
+
+ for (i = 0; i < priv->leds_count; i++) {
+ struct mt6360_led *led = priv->leds + i;
+
+ if (led->v4l2_flash)
+ v4l2_flash_release(led->v4l2_flash);
+
+ }
+}
+
+static int mt6360_led_probe(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv;
+ struct fwnode_handle *child;
+ size_t count;
+ int i = 0, ret;
+
+ count = device_get_child_node_count(>dev);
+ if (!count || count > MT6360_MAX_LEDS)
Please add dev_err() here.
+ return -EINVAL;
+
+ priv = devm_kzalloc(>dev, struct_size(priv, leds, count),
GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->leds_count = count;
+ priv->dev = >dev;
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap) {
+ dev_err(>dev, "Failed to get parent regmap\n");
+ return -ENODEV;
+ }
+
+ device_for_each_child_node(>dev, child) {
+ struct mt6360_led *led = priv->leds + i;
+ struct led_init_data init_data = { .fwnode = child, };
+ u32 reg;
+
+ ret = fwnode_property_read_u32(child, "reg", );
+ if (ret)
+ goto out_flash_release;
+
+ if (reg >= MT6360_MAX_LEDS || priv->leds_active & BIT(reg))
+ return -EINVAL;
+ priv->leds_active |= BIT(reg);
+
+ led->led_no = reg;
+ led->priv = priv;
+
+ ret = mt6360_init_common_properties(led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ switch (reg) {
+ case MT6360_LED_ISNK1 ... MT6360_LED_ISNK4:
+ ret = mt6360_init_isnk_properties(led, _data);
+ break;
+ default:
+ ret = mt6360_init_flash_properties(led, _data);
+ }
+
+ if (ret)
+ goto out_flash_release;
+
+ ret = mt6360_led_register(>dev, led, _data);
+ if (ret)
+ goto out_flash_release;
+
+ i++;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+
+out_flash_release:
+ mt6360_v4l2_flash_release(priv);
+ return ret;
+}
+
+static int mt6360_led_remove(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv = platform_get_drvdata(pdev);
+
+ mt6360_v4l2_flash_release(priv);
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
+ { .compatible = "mediatek,mt6360-led", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mt6360_led_of_id);
+
+static struct platform_driver mt6360_led_driver = {
+ .driver = {
+ .name = "mt6360-led",
+ .of_match_table = mt6360_led_of_id,
+ },
+ .probe = mt6360_led_probe,
+ .remove = mt6360_led_remove,
+};
+module_platform_driver(mt6360_led_driver);
+
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 LED Driver");
+MODULE_LICENSE("GPL v2");
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
Hi Pavel,
On 9/22/20 12:42 AM, Pavel Machek wrote:
Hi!
Can I get details of your setup?
I don't use this trigger, but I can imagine that someone does.
Well, if someone exists, we can increase the limit, or convince them
to change their setup.
Linux is used in many commercial projects and each such change generates
a cost, so this is not a sheer matter of convincing someone.
What CPU type that is, and how are you mapping CPU activity to LEDs?
The type of CPU is irrelevant here. What is important is the fact
that with this trigger it is possible to visually monitor CPU core
online state. Probably it would be good to ask the author of that
trigger about his use case.
It is relevant -- cpu trigger never worked on x86. I had patch fixing
it, but got pushback.
You mean literally x86 (32-bit)? Because I checked yesterday on my
x86_64 and it worked just fine, i.e. changing cpu online state
generated events on all userspace LEDs I registered for each cpuN
trigger.
I have spoken up, because I don't get the reason for your patch.
This driver was reworked year ago to remove PAGE_SIZE limit,
and I even applied it to my for-next tree, but that was at
the time of handling maintainership to yourself, and you
seem to not have picked that commit.
Was that intentional? We had even Greg's ack [0].
I checked, and I believe the commit is in:
Indeed, I blindly sought the changeset in git log for ledtrig-cpu.c
file.
#ifdef CONFIG_LEDS_TRIGGERS
static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
0);
static struct bin_attribute *led_trigger_bin_attrs[] = {
So.. no, it is not causing kernel crashes or something. But it is
example of bad interface, and _that_ is causing problems. (And yes, if
I realized it is simply possible to limit it, maybe the BIN_ATTR
conversion would not be neccessary...)
The limitation you proposed breaks the trigger on many plafforms.
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
On 9/20/20 8:34 PM, Pavel Machek wrote:
Hi!
*
* It can be bound to any LED just like other triggers using either a
* board file or via sysfs interface.
*
* An API named ledtrig_cpu is exported for any user, who want to add CPU
- * activity indication in their code
+ * activity indication in their code.
*
* Copyright 2011 Linus Walleij
* Copyright 2011 - 2012 Bryan Wu
@@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
for_each_possible_cpu(cpu) {
struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
+ if (cpu > 4)
NACK. The workaround for this trigger was implemented for a reason -
to make it working on platforms with arbitrary number of logical cpus.
I've got 8, so I am discriminated now. Not saying, that it precludes
trigger registration with no single line of warning.
Can I get details of your setup?
I don't use this trigger, but I can imagine that someone does.
What CPU type that is, and how are you mapping CPU activity to LEDs?
The type of CPU is irrelevant here. What is important is the fact
that with this trigger it is possible to visually monitor CPU core
online state. Probably it would be good to ask the author of that
trigger about his use case.
We've had also another patch in 2017 adding "cpu" trigger to
ledtrig-cpu.c that expressed number of online cores in a function of
brightness.
The commit message said:
"This is particularly useful on tiny linux boards with more CPU cores
than LED pins."
So apparently there are still users thereof.
As I've checked it now it has a bug, as it assumes max brightness to be
always LED_FULL, so that will need a fix.
I have spoken up, because I don't get the reason for your patch.
This driver was reworked year ago to remove PAGE_SIZE limit,
and I even applied it to my for-next tree, but that was at
the time of handling maintainership to yourself, and you
seem to not have picked that commit.
Was that intentional? We had even Greg's ack [0].
[0] https://lkml.org/lkml/2019/9/30/397
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
On 9/20/20 7:33 PM, Marek Behun wrote:
On Sun, 20 Sep 2020 18:55:28 +0200
Jacek Anaszewski wrote:
On 9/20/20 5:39 PM, Marek Behun wrote:
On Sun, 20 Sep 2020 16:15:09 +0200
Jacek Anaszewski wrote:
Hi Pavel,
On 9/19/20 11:38 AM, Pavel Machek wrote:
commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
Author: Pavel Machek
Date: Sat Sep 19 11:34:58 2020 +0200
ledtrig-cpu: Limit to 4 CPUs
Some machines have thousands of CPUs... and trigger mechanisms was not
really meant for thousands of triggers. I doubt anyone uses this
trigger on many-CPU machine; but if they do, they'll need to do it
properly.
Signed-off-by: Pavel Machek
diff --git a/drivers/leds/trigger/ledtrig-cpu.c
b/drivers/leds/trigger/ledtrig-cpu.c
index 869976d1b734..b7e00b09b137 100644
--- a/drivers/leds/trigger/ledtrig-cpu.c
+++ b/drivers/leds/trigger/ledtrig-cpu.c
@@ -2,14 +2,18 @@
/*
* ledtrig-cpu.c - LED trigger based on CPU activity
*
- * This LED trigger will be registered for each possible CPU and named as
- * cpu0, cpu1, cpu2, cpu3, etc.
+ * This LED trigger will be registered for first four CPUs and named
+ * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
+ * is on when any CPU is active.
+ *
+ * If you want support for arbitrary number of CPUs, make it one trigger,
+ * with additional sysfs file selecting which CPU to watch.
*
* It can be bound to any LED just like other triggers using either a
* board file or via sysfs interface.
*
* An API named ledtrig_cpu is exported for any user, who want to add CPU
- * activity indication in their code
+ * activity indication in their code.
*
* Copyright 2011 Linus Walleij
* Copyright 2011 - 2012 Bryan Wu
@@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
for_each_possible_cpu(cpu) {
struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
+ if (cpu > 4)
NACK. The workaround for this trigger was implemented for a reason -
to make it working on platforms with arbitrary number of logical cpus.
I've got 8, so I am discriminated now. Not saying, that it precludes
trigger registration with no single line of warning.
Regardless of that - you have no guarantee that you're not breaking
anyone - "I doubt" is not a sufficient argument.
If that is the case Jacek, I would try 16 and then see if people
complain. Do you really think that someone sets a specific LED to
trigger on activity on CPU id > 16?
I have an access to the machine with 80 cpus, so I could once
get surprised not being able to find cpuN triggers not being
listed among available triggers.
And say that I have a solution where I install 80 userspace LEDs
(drivers/leds/uleds.c) and register them on each cpuN triggers to get
notifications on how cpus work.
Hi Jacek,
I understand (and Pavel does for sure too) that many people
currently have that possibility, that they have access to machines with
many CPUs and many LEDs. We also understand that currently it is
possible for users to set 1847th LED to trigger on activity on CPU ID
1337. What we are suggesting is that practically no one uses this, and
for those 10 people who do, well it would be better for them to migrate
to new ABI than for kernel developers having forever maintain this
legacy ABI.
Legacy drivers get removed from kernel from time to time, if no one
uses them. So I think Pavel's proposal (although I may not agree with
the limit 4) has some merit. If we try this, and someone complains, we
can then discuss. If we don't try, we may never know.
Just go ahead without my ack. I just wanted not to let it go without
any discussion. At least we leave a trace...
If you do not agree, then I think we should implement a "cpu" trigger
where the cpu ID (or maybe mask of multiple CPUs) is configurable via
another sysfs file. And then declare current cpu trigger (with names
"cpu%d") as legacy.
Yes, we can do that, and even mark the cpu trigger as legacy but we
cannot prevent people from using it if that was present in kernel
for many years.
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
On 9/20/20 5:39 PM, Marek Behun wrote:
On Sun, 20 Sep 2020 16:15:09 +0200
Jacek Anaszewski wrote:
Hi Pavel,
On 9/19/20 11:38 AM, Pavel Machek wrote:
commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
Author: Pavel Machek
Date: Sat Sep 19 11:34:58 2020 +0200
ledtrig-cpu: Limit to 4 CPUs
Some machines have thousands of CPUs... and trigger mechanisms was not
really meant for thousands of triggers. I doubt anyone uses this
trigger on many-CPU machine; but if they do, they'll need to do it
properly.
Signed-off-by: Pavel Machek
diff --git a/drivers/leds/trigger/ledtrig-cpu.c
b/drivers/leds/trigger/ledtrig-cpu.c
index 869976d1b734..b7e00b09b137 100644
--- a/drivers/leds/trigger/ledtrig-cpu.c
+++ b/drivers/leds/trigger/ledtrig-cpu.c
@@ -2,14 +2,18 @@
/*
* ledtrig-cpu.c - LED trigger based on CPU activity
*
- * This LED trigger will be registered for each possible CPU and named as
- * cpu0, cpu1, cpu2, cpu3, etc.
+ * This LED trigger will be registered for first four CPUs and named
+ * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
+ * is on when any CPU is active.
+ *
+ * If you want support for arbitrary number of CPUs, make it one trigger,
+ * with additional sysfs file selecting which CPU to watch.
*
* It can be bound to any LED just like other triggers using either a
* board file or via sysfs interface.
*
* An API named ledtrig_cpu is exported for any user, who want to add CPU
- * activity indication in their code
+ * activity indication in their code.
*
* Copyright 2011 Linus Walleij
* Copyright 2011 - 2012 Bryan Wu
@@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
for_each_possible_cpu(cpu) {
struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
+ if (cpu > 4)
NACK. The workaround for this trigger was implemented for a reason -
to make it working on platforms with arbitrary number of logical cpus.
I've got 8, so I am discriminated now. Not saying, that it precludes
trigger registration with no single line of warning.
Regardless of that - you have no guarantee that you're not breaking
anyone - "I doubt" is not a sufficient argument.
If that is the case Jacek, I would try 16 and then see if people
complain. Do you really think that someone sets a specific LED to
trigger on activity on CPU id > 16?
I have an access to the machine with 80 cpus, so I could once
get surprised not being able to find cpuN triggers not being
listed among available triggers.
And say that I have a solution where I install 80 userspace LEDs
(drivers/leds/uleds.c) and register them on each cpuN triggers to get
notifications on how cpus work.
If you do not agree, then I think we should implement a "cpu" trigger
where the cpu ID (or maybe mask of multiple CPUs) is configurable via
another sysfs file. And then declare current cpu trigger (with names
"cpu%d") as legacy.
Yes, we can do that, and even mark the cpu trigger as legacy but we
cannot prevent people from using it if that was present in kernel
for many years.
--
Best regards,
Jacek Anaszewski
Re: ledtrig-cpu: Limit to 4 CPUs
Hi Pavel,
On 9/19/20 11:38 AM, Pavel Machek wrote:
commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
Author: Pavel Machek
Date: Sat Sep 19 11:34:58 2020 +0200
ledtrig-cpu: Limit to 4 CPUs
Some machines have thousands of CPUs... and trigger mechanisms was not
really meant for thousands of triggers. I doubt anyone uses this
trigger on many-CPU machine; but if they do, they'll need to do it
properly.
Signed-off-by: Pavel Machek
diff --git a/drivers/leds/trigger/ledtrig-cpu.c
b/drivers/leds/trigger/ledtrig-cpu.c
index 869976d1b734..b7e00b09b137 100644
--- a/drivers/leds/trigger/ledtrig-cpu.c
+++ b/drivers/leds/trigger/ledtrig-cpu.c
@@ -2,14 +2,18 @@
/*
* ledtrig-cpu.c - LED trigger based on CPU activity
*
- * This LED trigger will be registered for each possible CPU and named as
- * cpu0, cpu1, cpu2, cpu3, etc.
+ * This LED trigger will be registered for first four CPUs and named
+ * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
+ * is on when any CPU is active.
+ *
+ * If you want support for arbitrary number of CPUs, make it one trigger,
+ * with additional sysfs file selecting which CPU to watch.
*
* It can be bound to any LED just like other triggers using either a
* board file or via sysfs interface.
*
* An API named ledtrig_cpu is exported for any user, who want to add CPU
- * activity indication in their code
+ * activity indication in their code.
*
* Copyright 2011 Linus Walleij
* Copyright 2011 - 2012 Bryan Wu
@@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
for_each_possible_cpu(cpu) {
struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
+ if (cpu > 4)
NACK. The workaround for this trigger was implemented for a reason -
to make it working on platforms with arbitrary number of logical cpus.
I've got 8, so I am discriminated now. Not saying, that it precludes
trigger registration with no single line of warning.
Regardless of that - you have no guarantee that you're not breaking
anyone - "I doubt" is not a sufficient argument.
+ continue;
+
snprintf(trig->name, MAX_NAME_LEN, "cpu%d", cpu);
led_trigger_register_simple(trig->name, >_trig);
--
Best regards,
Jacek Anaszewski
Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree
On 9/16/20 2:15 AM, Marek Behun wrote:
On Tue, 15 Sep 2020 23:35:25 +0200
Jacek Anaszewski wrote:
Hi Marek,
On 9/15/20 5:26 PM, Marek Behún wrote:
Allow setting netdev LED trigger as default when given LED DT node has
the `trigger-sources` property pointing to a node corresponding to a
network device.
The specific netdev trigger mode is determined from the `function` LED
property.
Example:
eth0: ethernet@3 {
compatible = "xyz";
#trigger-source-cells = <0>;
};
led {
color = ;
function = LED_FUNCTION_LINK;
trigger-sources = <>;
};
Signed-off-by: Marek Behún
Cc: Rob Herring
Cc: devicet...@vger.kernel.org
---
drivers/leds/trigger/ledtrig-netdev.c | 80 ++-
include/dt-bindings/leds/common.h | 1 +
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c
b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d830215..99fc2f0c68e12 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,7 @@
[...]
static int netdev_trig_activate(struct led_classdev *led_cdev)
{
struct led_netdev_data *trigger_data;
@@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev
*led_cdev)
trigger_data->last_activity = 0;
led_set_trigger_data(led_cdev, trigger_data);
+ netdev_trig_of_parse(led_cdev, trigger_data);
Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
sense to use it here so as not to unnecessarily call
netdev_trig_of_parse(), which makes sense only if trigger will be
default, I presume.
See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
for reference.
Hmmm. Jacek, all the triggers that work with the macro
LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
If this macro is set, they all call pattern_init function where they
read led-pattern from fwnode.
The fact that they all call pattern_init() does not mean that
the use of flag is limited only to this type of triggers.
It has been introduced to allow initialization of default trigger
with required parameters, but in the same time, not to enforce
the same initial parameters each next time the trigger is set
for the LED.
But there is no device tree in Linux sources using this property.
In fact the command
git grep led-pattern
yields only 2 files:
Documentation/devicetree/bindings/leds/common.yaml
drivers/leds/led-core.c
What is the purpose if no device tree uses this property? Is this used
from other fwnode sources, like acpi or efi?
This is mainly useful for debugging purposes, probably that's why
it is not present in official dts files yet.
The reason why I am asking this is that the `led-pattern` property in
device tree goes against the principle of device tree, that it
shouldn't set settings settable from userspace, only describe the
devices on the system and how they are connected to each other.
If that was a hard principle then we wouldn't have properties like
linux,default-trigger. I have once asked Rob about that - see the reply
at the and of message [0].
[0] https://lore.kernel.org/linux-leds/20181025195444.GA12737@bogus/
--
Best regards,
Jacek Anaszewski
Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree
Hi Marek,
On 9/15/20 5:26 PM, Marek Behún wrote:
Allow setting netdev LED trigger as default when given LED DT node has
the `trigger-sources` property pointing to a node corresponding to a
network device.
The specific netdev trigger mode is determined from the `function` LED
property.
Example:
eth0: ethernet@3 {
compatible = "xyz";
#trigger-source-cells = <0>;
};
led {
color = ;
function = LED_FUNCTION_LINK;
trigger-sources = <>;
};
Signed-off-by: Marek Behún
Cc: Rob Herring
Cc: devicet...@vger.kernel.org
---
drivers/leds/trigger/ledtrig-netdev.c | 80 ++-
include/dt-bindings/leds/common.h | 1 +
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c
b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d830215..99fc2f0c68e12 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,7 @@
[...]
static int netdev_trig_activate(struct led_classdev *led_cdev)
{
struct led_netdev_data *trigger_data;
@@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev
*led_cdev)
trigger_data->last_activity = 0;
led_set_trigger_data(led_cdev, trigger_data);
+ netdev_trig_of_parse(led_cdev, trigger_data);
Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
sense to use it here so as not to unnecessarily call
netdev_trig_of_parse(), which makes sense only if trigger will be
default, I presume.
See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
for reference.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 1/3] leds: Require valid fwnode pointer for composing name
Hi Alexander,
On 9/15/20 11:14 AM, Alexander Dahl wrote:
Hello Jacek,
thanks for your feedback. See below.
Am Freitag, 11. September 2020, 23:26:43 CEST schrieb Jacek Anaszewski:
On 9/11/20 5:40 PM, Alexander Dahl wrote:
The function 'led_compose_name()' is called in
'led_classdev_register_ext(()' only and in its implementation it always
parses the fwnode passed with the init_data struct. If there's no
fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
early.
If this is detected early the same fallback mechanism can be used , as
if init_data itself is NULL. This will allow drivers to pass fully
populated 'init_data' or sparse initialized 'init_data' with a NULL
fwnode in a more elegant way with only one function call.
Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class
device names") Suggested-by: Pavel Machek
Signed-off-by: Alexander Dahl
---
Notes:
v4:
* added this patch to series (Suggested-by: Pavel Machek)
drivers/leds/led-class.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cc3929f858b6..3da50c7ecfe7 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
const char *proposed_name = composed_name;
int ret;
- if (init_data) {
+ if (init_data && init_data->fwnode) {
This does not cover the case when we don't have fwnode but we
have init_data->default_label that led_compose_name() can make use of.
if (init_data->devname_mandatory && !init_data->devicename) {
dev_err(parent, "Mandatory device name is missing");
return -EINVAL;
You're right, I missed that part in that if/else if construct in
led_compose_name() … I looked at the code for some more time now and could not
come up with an elegant change to the led-core or led-class. :-/
However I also had another look at leds-pwm and for me it seems that it is
used by fwnode (DT, ACPI, ??) based devices only. I could not find a single
user of leds-pwm as a platform driver, which is probably why 141f15c66d94
("leds: pwm: remove header") was possible?
In fact it looks like that patch was pointless, since it precluded the
use of struct led_pwm_platform_data anywhere besides the leds-pwm
driver.
I had a look at the history of the leds-pwm driver and when introduced in 2009
platform based board files where a thing, no dt, of, or fwnode yet, at least
for arm, right? Device tree support for leds-pwm was added in 2012 by Peter
Ujfalusi.
So if those code paths in leds-pwm are not used anymore, what about dropping
that platform support in leds-pwm driver? That would mean we always have
fwnode non-null and would not require a change in led-class at all?
git grep led_pwm_platform_data in fact shows only references in
leds-pwm.c, so yes, I think the platform support seems to be redundant.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 1/3] leds: Require valid fwnode pointer for composing name
Hi Alexander,
On 9/11/20 5:40 PM, Alexander Dahl wrote:
The function 'led_compose_name()' is called in
'led_classdev_register_ext(()' only and in its implementation it always
parses the fwnode passed with the init_data struct. If there's no
fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
early.
If this is detected early the same fallback mechanism can be used , as
if init_data itself is NULL. This will allow drivers to pass fully
populated 'init_data' or sparse initialized 'init_data' with a NULL
fwnode in a more elegant way with only one function call.
Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class device
names")
Suggested-by: Pavel Machek
Signed-off-by: Alexander Dahl
---
Notes:
v4:
* added this patch to series (Suggested-by: Pavel Machek)
drivers/leds/led-class.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cc3929f858b6..3da50c7ecfe7 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
const char *proposed_name = composed_name;
int ret;
- if (init_data) {
+ if (init_data && init_data->fwnode) {
This does not cover the case when we don't have fwnode but we
have init_data->default_label that led_compose_name() can make use of.
if (init_data->devname_mandatory && !init_data->devicename) {
dev_err(parent, "Mandatory device name is missing");
return -EINVAL;
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/11/20 1:24 AM, Gene Chen wrote:
Pavel Machek 於 2020年9月11日 週五 下午3:05寫道：
Hi!
+{
+struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+struct mt6360_priv *priv = led->priv;
+u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+u32 prev = priv->fled_torch_used, curr;
+int ret;
+
+dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+if (priv->fled_strobe_used) {
+dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
Doesn't hardware handle that? IOW, what happens when you have enabled
both torch and flash? If flash just overrides torch mode, than you
should not prevent enabling torch in this case.
Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.
Could I get you to remove code you are not commenting at when
reviewing?
MT6360 FLED register define is STROBE_EN/TORCH_EN/CS1/CS2 (current
source) 4 bits.
The STROBE_EN/TORCH_EN is shared by FLED1 and FLED2.
If I want to enable FLED1 torch mode, I set TORCH_EN and CS1
If I want to enable FLED2 strobe mode, I set STROBE_EN and CS2
For example I set FLED1 torch, then I set FLED2 strobe.
When I set FLED2 strobe, I will see the strobe current is FLED2 add
FLED1 current which is not I want.
So I need disable FLED1 torch first.
Considering every circumstances is complicated when share same H/W
logic control.
And the other problem is torch mode switch to strobe mode needs ramp
time because strobe and torch mode can't be co-exist.
Thank you for the explanation. So we have to keep your guards
but I would return -EBUSY instead of -EINVAL.
This would be also consistent with what
drivers/media/v4l2-core/v4l2-flash-led-class.c
does in its v4l2_flash_s_ctrl(), case V4L2_CID_FLASH_STROBE - it returns
-EBUSY if __software_strobe_mode_inactive() returns false.
The advantage of V4L2 Flash interface is that it has LED_MODE that
can be set to torch or flash, but in LED subsystem we don't have
the counterpart.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi Pavel,
On 9/11/20 9:05 AM, Pavel Machek wrote:
Hi!
+{
+ struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+ struct mt6360_priv *priv = led->priv;
+ u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+ u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+ u32 prev = priv->fled_torch_used, curr;
+ int ret;
+
+ dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+ if (priv->fled_strobe_used) {
+ dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
Doesn't hardware handle that? IOW, what happens when you have enabled
both torch and flash? If flash just overrides torch mode, than you
should not prevent enabling torch in this case.
Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.
What you say now is even more confusing when we look at your ack
under this patch:
commit 7aea8389a77abf9fde254aca2434a605c7704f58
Author: Jacek Anaszewski
Date: Fri Jan 9 07:22:51 2015 -0800
leds: Add LED Flash class extension to the LED subsystem
Some LED devices support two operation modes - torch and flash.
This patch provides support for flash LED devices in the LED subsystem
by introducing new sysfs attributes and kernel internal interface.
The attributes being introduced are: flash_brightness, flash_strobe,
flash_timeout, max_flash_timeout, max_flash_brightness, flash_fault,
flash_sync_strobe and available_sync_leds. All the flash related
features are placed in a separate module.
The modifications aim to be compatible with V4L2 framework requirements
related to the flash devices management. The design assumes that V4L2
sub-device can take of the LED class device control and communicate
with it through the kernel internal interface. When V4L2 Flash
sub-device
file is opened, the LED class device sysfs interface is made
unavailable.
Signed-off-by: Jacek Anaszewski
Acked-by: Kyungmin Park
Cc: Richard Purdie
Acked-by: Pavel Machek
Signed-off-by: Bryan Wu
Could I get you to remove code you are not commenting at when
reviewing?
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 Led Driver");
Led -> LED.
Pavel
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
s[reg] = led;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+out:
+ for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
+ struct mt6360_led *led = priv->leds[i];
+
+ if (led && led->v4l2_flash)
+ v4l2_flash_release(led->v4l2_flash);
+
+ }
+
+ return ret;
+}
+
+static int mt6360_led_remove(struct platform_device *pdev)
+{
+ struct mt6360_priv *priv = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
+ struct mt6360_led *led = priv->leds[i];
+
+ if (led && led->v4l2_flash)
+ v4l2_flash_release(led->v4l2_flash);
+
+ }
You're using this for loop twice, so it would be nice to wrap
it with a function to avoid code redundancy.
+
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
+ { .compatible = "mediatek,mt6360-led", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mt6360_led_of_id);
+
+static struct platform_driver mt6360_led_driver = {
+ .driver = {
+ .name = "mt6360-led",
+ .of_match_table = mt6360_led_of_id,
+ },
+ .probe = mt6360_led_probe,
+ .remove = mt6360_led_remove,
+};
+module_platform_driver(mt6360_led_driver);
+
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 Led Driver");
+MODULE_LICENSE("GPL v2");
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/10/20 10:25 PM, Pavel Machek wrote: Hi! 1. set FLED1 brightness # echo 1 > /sys/class/leds/white:flash1/flash_brightness 2. enable FLED1 strobe # echo 1 > /sys/class/leds/white:flash1/flash_strobe 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode flash leds must be turned off) # echo 0 > /sys/class/leds/white:flash1/flash_strobe I believe I'd preffer only exposing torch functionality in /sys/class/leds. .. strobe can be supported using v4l2 APIs. Actually having LED flash class without strobe is pointless. If you looked at led_classdev_flash_register_ext() you would see that it fails with uninitialized strobe_set op. And V4L2 API for strobing flash calls strobe_set from LED flash class beneath. That was the idea behind LED and V4L2 flash API unification - there is one hardware driver needed, the V4L2 Flash layer just takes over control over it when needed. I agree that one driver is enough. But we should not need flash_strobe file in sysfs. Simply use V4L2 for that. Apart from the fact that we can't remove it from LED flash class ABI now, why would you like to prevent the user from exploiting main feature of the hardware? -- Best regards, Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/10/20 2:29 PM, Pavel Machek wrote:
Hi!
+{
+ struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+ struct mt6360_priv *priv = led->priv;
+ u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+ u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+ u32 prev = priv->fled_torch_used, curr;
+ int ret;
+
+ dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+ if (priv->fled_strobe_used) {
+ dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
+ return -EINVAL;
+ }
So... how does its userland interface look like?
1. set FLED1 brightness
# echo 1 > /sys/class/leds/white:flash1/flash_brightness
2. enable FLED1 strobe
# echo 1 > /sys/class/leds/white:flash1/flash_strobe
3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
flash leds must be turned off)
# echo 0 > /sys/class/leds/white:flash1/flash_strobe
I believe I'd preffer only exposing torch functionality in
/sys/class/leds. .. strobe can be supported using v4l2 APIs.
Actually having LED flash class without strobe is pointless.
If you looked at led_classdev_flash_register_ext() you would see that
it fails with uninitialized strobe_set op. And V4L2 API for strobing
flash calls strobe_set from LED flash class beneath.
That was the idea behind LED and V4L2 flash API unification - there
is one hardware driver needed, the V4L2 Flash layer just takes over
control over it when needed.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: pwm: Allow automatic labels for DT based devices
Hi Alexander,
On 9/9/20 10:29 PM, Alexander Dahl wrote:
Hei hei,
On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
Hi!
pwm_init_state(led_data->pwm, _data->pwmstate);
- ret = devm_led_classdev_register(dev, _data->cdev);
+ if (fwnode) {
+ init_data.fwnode = fwnode;
+ ret = devm_led_classdev_register_ext(dev, _data->cdev,
+_data);
+ } else {
+ ret = devm_led_classdev_register(dev, _data->cdev);
+ }
Can you always use _ext version, even with null fwnode?
I did not try on real hardware, but from reading the code I would say
the following would happen: led_classdev_register_ext() calls
led_compose_name(parent, init_data, composed_name) which itself calls
led_parse_fwnode_props(dev, fwnode, ); that returns early due to
fwnode==NULL without changing props, thus this stays as initialized
with {}, so led_compose_name() would return -EINVAL which would let
led_classdev_register_ext() fail, too.
If not, can you fix the core to accept that? Having that conditional
in driver is ugly.
It is ugly, although the approach is inspired by the leds-gpio driver.
I'll see if I can come up with a change to led-core, but I'm also open
for suggestions. ;-)
devm_led_classdev_register() calls devm_led_classdev_register_ext()
with NULL passed in place of init_data, so you could do something like
below to achieve the same without touching LED core:
struct led_init_data init_data_impl = { .fwnode = fwnode };
struct led_init_data *init_data = NULL;
if (fwnode)
init_data = _data_impl;
devm_led_classdev_register_ext(dev, _data->cdev, init_data);
fyi: Peter Ujfalusi answered and would give his Ack to the changed
dual license for the yaml file. You can expect that for v4.
Stay tuned
Alex
--
Best regards,
Jacek Anaszewski
Re: [PATCH v2 0/2] leds: pwm: Make automatic labels work
Hi Alexander, On 9/4/20 9:53 AM, Alexander Dahl wrote: Hi Jacek, Am Dienstag, 1. September 2020, 23:08:09 CEST schrieb Jacek Anaszewski: Hi Alexander, Thanks for the v2. On 8/31/20 11:02 PM, Alexander Dahl wrote: Hei hei, for leds-gpio you can use the properties 'function' and 'color' in the devicetree node and omit 'label', the label is constructed automatically. This is a common feature supposed to be working for all LED drivers. However it did not yet work for the 'leds-pwm' driver. This series fixes the driver and takes the opportunity to update the dt-bindings accordingly. v1: based on v5.9-rc2, backport on v5.4.59 tested and working v2: based on v5.9-rc3, added the dt-bindings update patch Greets Alex Alexander Dahl (2): leds: pwm: Allow automatic labels for DT based devices dt-bindings: leds: Convert pwm to yaml .../devicetree/bindings/leds/leds-pwm.txt | 50 --- .../devicetree/bindings/leds/leds-pwm.yaml| 85 +++ drivers/leds/leds-pwm.c | 9 +- 3 files changed, 93 insertions(+), 51 deletions(-) delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml For both patches: Acked-by: Jacek Anaszewski I'd like to make a v3 and change the license of the .yaml file to "(GPL-2.0- only OR BSD-2-Clause)" as suggested by checkpatch and [1]. Can I keep your Acked-by for that? Go ahead. Besides: those suggestions are obviously valid for new bindings. What about old bindings (.txt), which had no explicit SPDX tag or license note before? What license would apply there? Is the .yaml file technically new, when it was mostly just converted from .txt? I don't know what was the rationale behind adding license to DT bindings, probably Rob will be able to share some details. Possibly the fact that DT examples can be now compile-tested makes some difference here. -- Best regards, Jacek Anaszewski
Re: [PATCH v2 0/2] leds: pwm: Make automatic labels work
Hi Alexander, Thanks for the v2. On 8/31/20 11:02 PM, Alexander Dahl wrote: Hei hei, for leds-gpio you can use the properties 'function' and 'color' in the devicetree node and omit 'label', the label is constructed automatically. This is a common feature supposed to be working for all LED drivers. However it did not yet work for the 'leds-pwm' driver. This series fixes the driver and takes the opportunity to update the dt-bindings accordingly. v1: based on v5.9-rc2, backport on v5.4.59 tested and working v2: based on v5.9-rc3, added the dt-bindings update patch Greets Alex Alexander Dahl (2): leds: pwm: Allow automatic labels for DT based devices dt-bindings: leds: Convert pwm to yaml .../devicetree/bindings/leds/leds-pwm.txt | 50 --- .../devicetree/bindings/leds/leds-pwm.yaml| 85 +++ drivers/leds/leds-pwm.c | 9 +- 3 files changed, 93 insertions(+), 51 deletions(-) delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml For both patches: Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices
Hi Alexander,
On 8/28/20 9:00 AM, Alexander Dahl wrote:
Hello Jacek,
Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
On 8/26/20 11:37 AM, Alexander Dahl wrote:
From: Alexander Dahl
If LEDs are configured through device tree and the property 'label' is
omitted, the label is supposed to be generated from the properties
'function' and 'color' if present. While this works fine for e.g. the
'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED
device through 'devm_led_classdev_register_ext()' and pass a pointer to
the current device tree node. The approach to fix this was adopted from
the 'leds-gpio' driver.
For the following node from dts the LED appeared as 'led5' in sysfs
before and as 'red:debug' after this change.
pwm_leds {
compatible = "pwm-leds";
led5 {
function = LED_FUNCTION_DEBUG;
color = ;
pwms = < 2 1000 0>;
max-brightness = <127>;
linux,default-trigger = "heartbeat";
panic-indicator;
};
};
Signed-off-by: Alexander Dahl
---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also works
drivers/leds/leds-pwm.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index ef7b91bd2064..a27a1d75a3e9 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv *priv,>
struct led_pwm *led, struct fwnode_handle *fwnode)
{
struct led_pwm_data *led_data = >leds[priv->num_leds];
+ struct led_init_data init_data = {};
int ret;
led_data->active_low = led->active_low;
@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv *priv,>
pwm_init_state(led_data->pwm, _data->pwmstate);
- ret = devm_led_classdev_register(dev, _data->cdev);
+ if (fwnode) {
+ init_data.fwnode = fwnode;
+ ret = devm_led_classdev_register_ext(dev, _data->cdev,
+_data);
+ } else {
+ ret = devm_led_classdev_register(dev, _data->cdev);
+ }
if (ret) {
dev_err(dev, "failed to register PWM led for %s: %d\n",
led->name, ret);
This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txt is needed as well.
I'm not sure, what needs updating. The properties 'function' and 'color' are
already documented in Documentation/devicetree/bindings/leds/common.yaml … the
only thing I can think of here is updating the examples? That would be nice,
as would be updating to yaml, but I don't see the strong relation, yet.
It is necessary to tell the user that given driver is capable of
utilizing a property. I thought of something like in commit [0].
It would be good to switch to yaml by this occassion.
Is there some guidance on that in general?
I am not aware of, but surely sooner or later all bindings will
need to be unified. Touching the file is always a good opportunity
to address that. It's up to you, though.
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/leds/leds-lm3692x.txt?id=4dcbc8f8c59f4b618d651f5ba884ee5bf562c8de
--
Best regards,
Jacek Anaszewski
Re: [PATCH 0/2] leds: mt6360: Add LED driver for MT6360
He Gene, Please resend the set with updated version (I suppose it should be v3). On 8/26/20 1:37 PM, Gene Chen wrote: In-Reply-To: This patch series add MT6360 LED support contains driver and binding document Gene Chen (2) leds: mt6360: Add LED driver for MT6360 dt-bindings: leds: Add bindings for MT6360 LED Documentation/devicetree/bindings/leds/leds-mt6360.yaml | 50 + drivers/leds/Kconfig| 11 drivers/leds/Makefile |1 drivers/leds/leds-mt6360.c | 680 4 files changed, 742 insertions(+) -- Best regards, Jacek Anaszewski
Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices
Hi Alexander,
On 8/26/20 11:37 AM, Alexander Dahl wrote:
From: Alexander Dahl
If LEDs are configured through device tree and the property 'label' is
omitted, the label is supposed to be generated from the properties
'function' and 'color' if present. While this works fine for e.g. the
'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED
device through 'devm_led_classdev_register_ext()' and pass a pointer to
the current device tree node. The approach to fix this was adopted from
the 'leds-gpio' driver.
For the following node from dts the LED appeared as 'led5' in sysfs
before and as 'red:debug' after this change.
pwm_leds {
compatible = "pwm-leds";
led5 {
function = LED_FUNCTION_DEBUG;
color = ;
pwms = < 2 1000 0>;
max-brightness = <127>;
linux,default-trigger = "heartbeat";
panic-indicator;
};
};
Signed-off-by: Alexander Dahl
---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also works
drivers/leds/leds-pwm.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index ef7b91bd2064..a27a1d75a3e9 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv *priv,
struct led_pwm *led, struct fwnode_handle *fwnode)
{
struct led_pwm_data *led_data = >leds[priv->num_leds];
+ struct led_init_data init_data = {};
int ret;
led_data->active_low = led->active_low;
@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv *priv,
pwm_init_state(led_data->pwm, _data->pwmstate);
- ret = devm_led_classdev_register(dev, _data->cdev);
+ if (fwnode) {
+ init_data.fwnode = fwnode;
+ ret = devm_led_classdev_register_ext(dev, _data->cdev,
+_data);
+ } else {
+ ret = devm_led_classdev_register(dev, _data->cdev);
+ }
if (ret) {
dev_err(dev, "failed to register PWM led for %s: %d\n",
led->name, ret);
This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txt is needed as well.
It would be good to switch to yaml by this occassion.
--
Best regards,
Jacek Anaszewski
[PATCH] MAINTAINERS: Remove myself as LED subsystem maintainer
It don't have enough time for reviewing patches and thus don't want to be listed as regular LED maintainer. Nonetheless I may still give a review from time to time. Signed-off-by: Jacek Anaszewski --- MAINTAINERS | 2 -- 1 file changed, 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 4e2698cc7e23..71e16ba36048 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9681,12 +9681,10 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/tobin/leaks.git F: scripts/leaking_addresses.pl LED SUBSYSTEM -M: Jacek Anaszewski M: Pavel Machek R: Dan Murphy L: linux-l...@vger.kernel.org S: Maintained -T: git git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git T: git git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git F: Documentation/devicetree/bindings/leds/ F: drivers/leds/ -- 2.11.0
Re: [PATCH RFC leds + net-next 1/3] leds: trigger: add support for LED-private device triggers
ers have this set */
+ struct led_hw_trigger_type *trigger_type;
#endif
#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -345,6 +352,9 @@ struct led_trigger {
int (*activate)(struct led_classdev *led_cdev);
void(*deactivate)(struct led_classdev *led_cdev);
+ /* LED-private triggers have this set */
+ struct led_hw_trigger_type *trigger_type;
+
/* LEDs under control by this trigger (for simple triggers) */
rwlock_t leddev_list_lock;
struct list_head led_cdevs;
Looks good to me:
Acked-by: Jacek Anaszewski
--
Best regards,
Jacek Anaszewski
Re: [PATCH] leds: add NCT6795D driver
On 7/17/20 9:44 AM, Pavel Machek wrote: Hi! Add support for the LED feature of the NCT6795D chip found on some motherboards, notably MSI ones. The LEDs are typically used using a RGB connector so this driver creates one LED device for each color component. Ok, let me take a look. What entries does it present in /sys? Right now these 3 directories in /sys/class/leds: nct6795d:blue: nct6795d:green: nct6795d:red: with the usual suspects `brightness` and `max_brightness` in each. I am not 100% sure I got the names right so please let me know if that is not correct. You miss LED function, that should be in the second section. third section? Ekhm, right. The reason for not having a function at the moment is that I took a look at include/dt-bindings/leds/common.h and could not find any function that could reasonably apply. This basically controls a RGB connector on the motherboard which serves no particular function - you can plug a RGB fan or anything else you want and control it in any fashion. Is there a function that applies to this use-case? According to common LED bindings you should propose a new function if none of the existing ones fits your needs. This is normally used for motherboard lightning, right? I believe this is getting common on gaming boards, and we want common support for that. I agree. -- Best regards, Jacek Anaszewski
Re: [PATCH] leds: add NCT6795D driver
Hi Alexandre, On 7/15/20 3:54 AM, Alexandre Courbot wrote: Hi Pavel, On Wed, Jul 15, 2020 at 7:33 AM Pavel Machek wrote: Hi! Add support for the LED feature of the NCT6795D chip found on some motherboards, notably MSI ones. The LEDs are typically used using a RGB connector so this driver creates one LED device for each color component. Ok, let me take a look. What entries does it present in /sys? Right now these 3 directories in /sys/class/leds: nct6795d:blue: nct6795d:green: nct6795d:red: with the usual suspects `brightness` and `max_brightness` in each. I am not 100% sure I got the names right so please let me know if that is not correct. You miss LED function, that should be in the second section. -- Best regards, Jacek Anaszewski
Re: [PATCH v29 13/16] leds: lp5523: Update the lp5523 code to add multicolor brightness function
On 7/11/20 10:24 PM, Pavel Machek wrote: On Sat 2020-07-11 19:19:22, Jacek Anaszewski wrote: On 7/11/20 5:57 PM, Pavel Machek wrote: Hi! Add the multicolor brightness call back to support the multicolor framework. This call back allows setting brightness on grouped channels Extra space before "brightness". And before "This". That one is intentional, I believe. https://www.independent.co.uk/life-style/gadgets-and-tech/news/one-space-or-two-spaces-after-a-full-stop-scientists-have-finally-found-the-answer-a8337646.html We are using fixed width fonts, so typewriter rules still apply here. But see the article [0]. Also, in [1], in the section "Computer era" you can find opposite examples in the modern systems. And grep returns following numbers for kernel Documentation folder: $ rgrep "[A-Za-z0-9]\. " Documentation/ | wc -l 18449 $ rgrep "[A-Za-z0-9]\. " Documentation/ | wc -l 63067 I prefer single space but will not fight for that too hard. Nonetheless, it would be good to use one style consistently, since most files I've looked at had problem with that. Both DT and LED documentation in this set is not consistent in this regard as well. [0] https://www.instructionalsolutions.com/blog/one-space-vs-two-after-period [1] https://en.wikipedia.org/wiki/Sentence_spacing -- Best regards, Jacek Anaszewski
Re: [PATCH v29 13/16] leds: lp5523: Update the lp5523 code to add multicolor brightness function
On 7/11/20 5:57 PM, Pavel Machek wrote: Hi! Add the multicolor brightness call back to support the multicolor framework. This call back allows setting brightness on grouped channels Extra space before "brightness". And before "This". -- Best regards, Jacek Anaszewski
Re: [PATCH v2] leds: core: Flush scheduled work for system suspend
Hi Kai-Heng,
Thank you for the update.
On 7/2/20 7:45 AM, Kai-Heng Feng wrote:
Sometimes LED won't be turned off by LED_CORE_SUSPENDRESUME flag upon
system suspend.
led_set_brightness_nopm() uses schedule_work() to set LED brightness.
However, there's no guarantee that the scheduled work gets executed
because no one flushes the work.
So flush the scheduled work to make sure LED gets turned off.
Signed-off-by: Kai-Heng Feng
---
v2:
- Use flush_work() instead.
drivers/leds/led-class.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3363a6551a70..cc3929f858b6 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -173,6 +173,7 @@ void led_classdev_suspend(struct led_classdev *led_cdev)
{
led_cdev->flags |= LED_SUSPENDED;
led_set_brightness_nopm(led_cdev, 0);
+ flush_work(_cdev->set_brightness_work);
}
EXPORT_SYMBOL_GPL(led_classdev_suspend);
Acked-by: Jacek Anaszewski
Pavel, this needs to go to stable as well, so let's add the tag:
Fixes: 81fe8e5b73e3 ("leds: core: Add led_set_brightness_nosleep{nopm}
functions")
--
Best regards,
Jacek Anaszewski
Re: [PATCH] leds: core: Use blocking op for system suspend
Hi Kai-Heng,
Thank you for the patch.
On 7/1/20 11:35 AM, Kai-Heng Feng wrote:
Sometimes LED won't be turned off by LED_CORE_SUSPENDRESUME flag upon
system suspend.
Just out of curiosity - are you experiencing that on some hardware?
led_set_brightness_nopm() uses schedule_work() to set LED brightness.
However, there's no guarantee that the scheduled work gets executed
because no one calls flush_scheduled_work().
As flush_scheduled_work() may affect other drivers' suspend routines,
take a more contained approach which uses blocking op to make sure the
LED gets turned off.
Signed-off-by: Kai-Heng Feng
---
drivers/leds/led-core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..9a5bfcd7a704 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -269,6 +269,11 @@ EXPORT_SYMBOL_GPL(led_set_brightness);
void led_set_brightness_nopm(struct led_classdev *led_cdev,
enum led_brightness value)
{
+
+ if (led_cdev->flags & LED_SUSPENDED &&
+ !__led_set_brightness_blocking(led_cdev, value))
+ return;
+
This function is "nopm" for a reason - we do not make here any
pm management related operations.
Instead of that, please just add
flush_work(_cdev->set_brightness_work);
at the end of led_classdev_suspend()
in drivers/leds/led-class.c.
--
Best regards,
Jacek Anaszewski
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/21/20 10:24 PM, Jacek Anaszewski wrote:
Dan,
On 6/21/20 4:12 PM, Dan Murphy wrote:
Jacek
On 6/19/20 5:10 PM, Jacek Anaszewski wrote:
Dan,
On 6/19/20 6:35 PM, Dan Murphy wrote:
Jacek
On 6/18/20 6:26 PM, Jacek Anaszewski wrote:
On 6/19/20 12:09 AM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 11:44 PM, Dan Murphy wrote:
Jacek
On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop
us a note to help
improve the system. BTW, we also suggest to use '--base'
option to specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in
function `lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference
to `led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in
function `devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined
reference to `devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct
led_classdev *cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev =
lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led =
mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg =
led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev,
brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR ||
!LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol
that was set by the COMMON flag which is inherited by
majority from the child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562
|| LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR ||
!LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] ||
LEDS_LP5523 [=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
&& OF [=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
I2C [=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
I2C [=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
I2C [=y] && (LEDS_CLASS_MULTI_COLOR [=m] ||
!LEDS_CLASS_MULTI_COLOR [=m])
When I was testing that yesterday I also had the same warning at
some
point of testing different Kconfig setups, but with what I
showed above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the
config
from the test bot.
That is not what I saw in my testing especially after doing a
distclean
Could you please give your exact steps after "make distclean" and
copying test bot config to the kernel root directory?
Also, please share the toolchain you're using for tests.
Actually at this stage the toolchain is of lesser relevance.
I've tried once more
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/21/20 4:12 PM, Dan Murphy wrote:
Jacek
On 6/19/20 5:10 PM, Jacek Anaszewski wrote:
Dan,
On 6/19/20 6:35 PM, Dan Murphy wrote:
Jacek
On 6/18/20 6:26 PM, Jacek Anaszewski wrote:
On 6/19/20 12:09 AM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 11:44 PM, Dan Murphy wrote:
Jacek
On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop
us a note to help
improve the system. BTW, we also suggest to use '--base'
option to specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference
to `led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined
reference to `devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct
led_classdev *cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev =
lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led =
mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg =
led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol
that was set by the COMMON flag which is inherited by majority
from the child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR ||
!LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] ||
LEDS_LP5523 [=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m]) &&
OF [=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C
[=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C
[=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C
[=y] && (LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR
[=m])
When I was testing that yesterday I also had the same warning at
some
point of testing different Kconfig setups, but with what I showed
above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the config
from the test bot.
That is not what I saw in my testing especially after doing a
distclean
Could you please give your exact steps after "make distclean" and
copying test bot config to the kernel root directory?
Also, please share the toolchain you're using for tests.
Actually at this stage the toolchain is of lesser relevance.
I've tried once more and indeed the problem shows up.
It is caused by the driver entries
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/19/20 6:35 PM, Dan Murphy wrote:
Jacek
On 6/18/20 6:26 PM, Jacek Anaszewski wrote:
On 6/19/20 12:09 AM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 11:44 PM, Dan Murphy wrote:
Jacek
On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop us
a note to help
improve the system. BTW, we also suggest to use '--base' option
to specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference to
`led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined reference
to `devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct
led_classdev *cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev =
lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led = mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg = led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol that
was set by the COMMON flag which is inherited by majority from
the child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] ||
LEDS_LP5523 [=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m]) &&
OF [=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C
[=y] && (LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
When I was testing that yesterday I also had the same warning at some
point of testing different Kconfig setups, but with what I showed
above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the config
from the test bot.
That is not what I saw in my testing especially after doing a distclean
Could you please give your exact steps after "make distclean" and
copying test bot config to the kernel root directory?
Also, please share the toolchain you're using for tests.
Actually at this stage the toolchain is of lesser relevance.
I've tried once more and indeed the problem shows up.
It is caused by the driver entries doing
"select LEDS_LP55XX_COMMON".
Select sets config to "y" so it conflicts wi
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
On 6/19/20 12:09 AM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 11:44 PM, Dan Murphy wrote:
Jacek
On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop us a
note to help
improve the system. BTW, we also suggest to use '--base' option
to specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference to
`led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined reference to
`devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct
led_classdev *cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev =
lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led = mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg = led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol that
was set by the COMMON flag which is inherited by majority from the
child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] || LEDS_LP5523
[=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m]) && OF
[=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C
[=y] && (LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
When I was testing that yesterday I also had the same warning at some
point of testing different Kconfig setups, but with what I showed above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the config
from the test bot.
That is not what I saw in my testing especially after doing a distclean
Could you please give your exact steps after "make distclean" and
copying test bot config to the kernel root directory?
Also, please share the toolchain you're using for tests.
Actually at this stage the toolchain is of lesser relevance.
I've tried once more and indeed the problem shows up.
It is caused by the driver entries doing
"select LEDS_LP55XX_COMMON".
Select sets config to "y" so it conflicts with
"depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR"
in the "config
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/18/20 11:44 PM, Dan Murphy wrote:
Jacek
On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop us a
note to help
improve the system. BTW, we also suggest to use '--base' option to
specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference to
`led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined reference to
`devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct led_classdev
*cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev =
lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led = mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg = led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol that was
set by the COMMON flag which is inherited by majority from the
child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] || LEDS_LP5523
[=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m]) && OF [=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
&& (LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
When I was testing that yesterday I also had the same warning at some
point of testing different Kconfig setups, but with what I showed above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the config
from the test bot.
That is not what I saw in my testing especially after doing a distclean
Could you please give your exact steps after "make distclean" and
copying test bot config to the kernel root directory?
Also, please share the toolchain you're using for tests.
--
Best regards,
Jacek Anaszewski
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/18/20 12:33 AM, Dan Murphy wrote:
Jacek
On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop us a
note to help
improve the system. BTW, we also suggest to use '--base' option to
specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference to
`led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined reference to
`devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct led_classdev
*cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev = lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led = mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg = led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol that was
set by the COMMON flag which is inherited by majority from the child
flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
Yes I did
That gave unmet dependencies.
WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] || LEDS_LP5523
[=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) && (LEDS_CLASS_MULTI_COLOR
[=m] || !LEDS_CLASS_MULTI_COLOR [=m]) && OF [=y]
Selected by [y]:
- LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
- LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y]
Selected by [m]:
- LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] && I2C [=y] &&
(LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
When I was testing that yesterday I also had the same warning at some
point of testing different Kconfig setups, but with what I showed above
it ceased to appear. Now every time I am doing "make oldconfig" the
CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the config
from the test bot.
--
Best regards,
Jacek Anaszewski
Re: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 6/17/20 9:22 PM, Dan Murphy wrote:
Pavel/Jacek
On 6/17/20 11:28 AM, kernel test robot wrote:
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on pavel-linux-leds/for-next]
[cannot apply to j.anaszewski-leds/for-next]
[if your patch is applied to the wrong git tree, please drop us a note
to help
improve the system. BTW, we also suggest to use '--base' option to
specify the
base tree in git format-patch, please see
https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
base:
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
for-next
config: ia64-randconfig-r015-20200617 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`lp55xx_set_mc_brightness':
drivers/leds/leds-lp55xx-common.c:146: undefined reference to
`led_mc_calc_color_components'
ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in function
`devm_led_classdev_multicolor_register':
include/linux/led-class-multicolor.h:74: undefined reference to
`devm_led_classdev_multicolor_register_ext'
vim +146 drivers/leds/leds-lp55xx-common.c
138
139 static int lp55xx_set_mc_brightness(struct led_classdev *cdev,
140 enum led_brightness brightness)
141 {
142 struct led_classdev_mc *mc_dev = lcdev_to_mccdev(cdev);
143 struct lp55xx_led *led = mcled_cdev_to_led(mc_dev);
144 struct lp55xx_device_config *cfg = led->chip->cfg;
145
> 146 led_mc_calc_color_components(>mc_cdev, brightness);
147 return cfg->multicolor_brightness_fn(led);
148
Well this was a mess to figure out.
The only fix I can figure out here is to remove the
depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
from each child device and add
select LEDS_CLASS_MULTI_COLOR
to the LP55XX_COMMON
This way the Multi color framework will inherit the symbol that was set
by the COMMON flag which is inherited by majority from the child flags.
Did you try this?
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -398,6 +398,7 @@ config LEDS_LP50XX
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National
LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
--
Best regards,
Jacek Anaszewski
Re: [PATCH v26 03/15] leds: multicolor: Introduce a multicolor class definition
Dan, On 6/8/20 4:34 PM, Dan Murphy wrote: Jacek On 6/6/20 2:59 PM, Jacek Anaszewski wrote: Dan, On 6/6/20 6:39 PM, Dan Murphy wrote: Pavek Thanks for the review On 6/6/20 10:53 AM, Pavel Machek wrote: Hi! Introduce a multicolor class that groups colored LEDs within a LED node. The multi color class groups monochrome LEDs and allows controlling two aspects of the final combined color: hue and lightness. The former is controlled via the intensity file and the latter is controlled via brightness file. Acked-by: Jacek Anaszewski Signed-off-by: Dan Murphy diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor new file mode 100644 [...] --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9533,6 +9533,14 @@ F: Documentation/devicetree/bindings/leds/ F: drivers/leds/ F: include/linux/leds.h +LED MULTICOLOR FRAMEWORK +M: Dan Murphy +L: linux-l...@vger.kernel.org I'd like to be mentioned here, too. "M: Pavel Machek ". And I'm not sure if I should be taking MAINTAINER file update through a LED tree. Should definitely go to separate patch. Oh definitely. I thought it was implied that you and Jacek are both Maintainers as well. I will add you but will wait to see if Jacek wants to be added. Actually I don't think that we need to add this separate entry for LED multicolor class. This is still under LED subsystem, and I didn't add anything for LED class flash. We only need this because I am not a maintainer of the LED flash class or the LED class. But since I authored the code it only made sense to add me as a maintainer for this specific class. You are one of the maintainers of the LED subsystem and wrote the Flash class so your maintainer ship is implied and you will be CC'd for all patches. This will not be the case for the multi color class scripts/get_maintainer.pl returns yourself as well for LED drivers. But it's up to you. -- Best regards, Jacek Anaszewski
Re: [PATCH v3 2/3] leds: pwm: add support for default-state device property
Hi Dennis, On 6/8/20 8:32 AM, Denis Osterland-Heim wrote: Hi Jacek, is your ack still valid for the new versions of the patch-set? Due to the changes I made, I am not sure. Yes, you can keep it. -- Best regards, Jacek Anaszewski
Re: [PATCH v26 03/15] leds: multicolor: Introduce a multicolor class definition
Dan, On 6/6/20 6:39 PM, Dan Murphy wrote: Pavek Thanks for the review On 6/6/20 10:53 AM, Pavel Machek wrote: Hi! Introduce a multicolor class that groups colored LEDs within a LED node. The multi color class groups monochrome LEDs and allows controlling two aspects of the final combined color: hue and lightness. The former is controlled via the intensity file and the latter is controlled via brightness file. Acked-by: Jacek Anaszewski Signed-off-by: Dan Murphy diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor new file mode 100644 [...] --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9533,6 +9533,14 @@ F: Documentation/devicetree/bindings/leds/ F: drivers/leds/ F: include/linux/leds.h +LED MULTICOLOR FRAMEWORK +M: Dan Murphy +L: linux-l...@vger.kernel.org I'd like to be mentioned here, too. "M: Pavel Machek ". And I'm not sure if I should be taking MAINTAINER file update through a LED tree. Should definitely go to separate patch. Oh definitely. I thought it was implied that you and Jacek are both Maintainers as well. I will add you but will wait to see if Jacek wants to be added. Actually I don't think that we need to add this separate entry for LED multicolor class. This is still under LED subsystem, and I didn't add anything for LED class flash. I will separate this out and make it a separate patch -- Best regards, Jacek Anaszewski
Re: [PATCH] leds: mt6360: Add LED driver for MT6360
>dev, "Failed to register fled[%d]\n", i);
+ goto out_fled_cdev;
+ }
+ }
+
+ for (i = 0; i < MT6360_FLED_MAX; i++) {
+ mtfled_cdev = mld->mtfled_cdev + i;
+ memset(_config, 0, sizeof(v4l2_config));
+ mt6360_init_v4l2_flash_config(mtfled_cdev, _config);
+ mtfled_cdev->v4l2_flash = v4l2_flash_init(>dev,
+ of_fwnode_handle(mtfled_cdev->np),
+ _cdev->fl_cdev,
+ _flash_ops, _config);
+ if (IS_ERR(mtfled_cdev->v4l2_flash)) {
+ dev_err(>dev, "Failed to register v4l2_sd\n");
+ ret = PTR_ERR(mtfled_cdev->v4l2_flash);
+ goto out_v4l2_sd;
+ }
+ }
+
+ ret = mt6360_fled_irq_register(pdev);
+ if (ret < 0) {
+ dev_err(>dev, "Failed to register irqs\n");
+ goto out_v4l2_sd;
+ }
+
+ return 0;
+out_v4l2_sd:
+ while (--i >= 0) {
+ mtfled_cdev = mld->mtfled_cdev + i;
+ v4l2_flash_release(mtfled_cdev->v4l2_flash);
+ }
+ i = MT6360_FLED_MAX;
+out_fled_cdev:
+ while (--i >= 0) {
+ mtfled_cdev = mld->mtfled_cdev + i;
+ led_classdev_flash_unregister(_cdev->fl_cdev);
+ }
+ return ret;
+}
+
+static int mt6360_led_remove(struct platform_device *pdev)
+{
+ struct mt6360_led_data *mld = platform_get_drvdata(pdev);
+ struct mt6360_fled_classdev *mtfled_cdev;
+ int i;
+
+ for (i = 0; i < MT6360_FLED_MAX; i++) {
+ mtfled_cdev = mld->mtfled_cdev + i;
+ v4l2_flash_release(mtfled_cdev->v4l2_flash);
+ led_classdev_flash_unregister(_cdev->fl_cdev);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
+ { .compatible = "mediatek,mt6360_led", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mt6360_led_of_id);
+
+static struct platform_driver mt6360_led_driver = {
+ .driver = {
+ .name = "mt6360_led",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(mt6360_led_of_id),
+ },
+ .probe = mt6360_led_probe,
+ .remove = mt6360_led_remove,
+};
+module_platform_driver(mt6360_led_driver);
+
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 Led Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/mt6360.h b/include/linux/mfd/mt6360.h
index ea13040..2b81ab5 100644
--- a/include/linux/mfd/mt6360.h
+++ b/include/linux/mfd/mt6360.h
@@ -29,6 +29,11 @@ struct mt6360_pmu_data {
unsigned int chip_rev;
};
+struct mt6360_pmu_irq_desc {
+ const char *name;
+ irq_handler_t irq_handler;
+};
+
/* PMU register defininition */
#define MT6360_PMU_DEV_INFO (0x00)
#define MT6360_PMU_CORE_CTRL1 (0x01)
@@ -236,5 +241,4 @@ struct mt6360_pmu_data {
#define CHIP_VEN_MASK (0xF0)
#define CHIP_VEN_MT6360 (0x50)
#define CHIP_REV_MASK (0x0F)
-
#endif /* __MT6360_H__ */
--
Best regards,
Jacek Anaszewski
Re: [PATCH v25 03/16] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers
Hi Pavel and Dan, On 5/31/20 9:06 PM, Pavel Machek wrote: Hi! + There can only be one instance of the ti,led-bank + property for each device node. This is a required node is the LED + modules are to be backed. I don't understand the second sentence. Pretty sure it is not valid english. If I make these changes is this still viable for 5.8 or would you then go into 5.9? It really depends if we get -rc8 or not, and if you'll need to do any changes to C code or not... I think that we need to simmer such a big extension of the LED subsystem for a whole cycle in linux-next, especially taking into account addition of new sysfs interface, that is bit quirky. Effectively 5.8 seems to not have been viable since few weeks. -- Best regards, Jacek Anaszewski
Re: [PATCH v1] Add support for MediaTek regulator vibrator driver
Hi Pavel, On 5/7/20 7:45 PM, Pavel Machek wrote: Hi! This patchset add regulator vibrator driver for MTK Soc. The driver controls vibrator through regulator's enable and disable. We'd prefer not to have vibrators in led subsystem. Xing Zhang (3): dt-bindings: add regulator vibrator documentation arm64: mediatek: Add regulator vibrator support Vibrator: Add regulator vibrator driver .../bindings/leds/regulator-vibrator.txt | 39 ++ arch/arm64/configs/defconfig | 1 + drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/regulator-vibrator.c | 450 ++ OTOH, connecting LED to regulator might make some sense. I can take the driver with vibrator functionality stripped, provided it is named the usual way... We already had an attempt of solving this in more generic way [0], but you opposed then [1]. Just for the record. [0] https://lore.kernel.org/linux-leds/20170913175400.42744-1-dtw...@google.com/ [1] https://lore.kernel.org/linux-leds/20170914205804.GA24339@amd/ -- Best regards, Jacek Anaszewski
Re: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition
Dan, On 5/3/20 2:32 PM, Dan Murphy wrote: Introduce a multicolor class that groups colored LEDs within a LED node. The multi color class groups monochrome LEDs and allows controlling two aspects of the final combined color: hue and lightness. The former is controlled via the intensity file and the latter is controlled via brightness file. Acked-by: Jacek Anaszewski Signed-off-by: Dan Murphy --- [...] --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -30,6 +30,17 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_CLASS_MULTI_COLOR + tristate "LED MultiColor LED Class Support" + depends on LEDS_CLASS + depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR I was saying about adding this dependency to the drivers based on LED mc class. This way it does not make any sense. Moreover it is erroneous: $ make menuconfig drivers/leds/Kconfig:33:error: recursive dependency detected! Instead you should add it to the Kconfig entries of all drivers that depend on LED mc class, i.e.: - config LEDS_LP50XX - config LEDS_LP5521 - config LEDS_LP5523 Moreover there are still some checkpatch.pl problems: --- 0003-leds-multicolor-Introduce-a-multicolor-class-definit.patch --- WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #89: FILE: Documentation/leds/leds-class-multicolor.rst:1: + ERROR: spaces required around that '=' (ctx:WxO) #294: FILE: drivers/leds/led-class-multicolor.c:62: + ret =-EINVAL; ^ ERROR: space required before that '-' (ctx:OxV) #294: FILE: drivers/leds/led-class-multicolor.c:62: + ret =-EINVAL; WARNING: DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause) #31: FILE: Documentation/devicetree/bindings/leds/leds-lp50xx.yaml:1: +# SPDX-License-Identifier: GPL-2.0 WARNING: Block comments use * on subsequent lines #705: FILE: drivers/leds/leds-lp50xx.c:636: + /* There are only 3 LEDs per module otherwise they should be + banked which also is presented as 3 LEDs*/ WARNING: Block comments use a trailing */ on a separate line #705: FILE: drivers/leds/leds-lp50xx.c:636: + banked which also is presented as 3 LEDs*/ --- 0008-ARM-dts-n900-Add-reg-property-to-the-LP5523-channel-.patch --- WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'? --- 0009-ARM-dts-imx6dl-yapp4-Add-reg-property-to-the-lp5562-.patch --- WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'? --- 0010-ARM-dts-ste-href-Add-reg-property-to-the-LP5521-chan.patch --- WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'? + help + This option enables the multicolor LED sysfs class in /sys/class/leds. + It wraps LED class and adds multicolor LED specific sysfs attributes + and kernel internal API to it. You'll need this to provide support + for multicolor LEDs that are grouped together. This class is not + intended for single color LEDs. It can be built as a module. + -- Best regards, Jacek Anaszewski
Re: [PATCH 1/3] leds: add aw2013 driver
ret);
+ goto error;
+ }
+
+ if (chipid != AW2013_RSTR_CHIP_ID) {
+ dev_err(>dev, "Chip reported wrong ID: %x\n",
+ chipid);
+ ret = -ENODEV;
+ goto error;
+ }
+
+ ret = aw2013_chip_init(chip);
+ if (ret)
+ goto error;
+
+ aw2013_read_current_state(chip);
+
+ for (i = 0; i < chip->num_leds; i++) {
+ struct led_init_data init_data = {};
+
+ aw2013_init_default_state(>leds[i]);
+
+ chip->leds[i].cdev.brightness_set_blocking =
+ aw2013_brightness_set;
+ chip->leds[i].cdev.blink_set = aw2013_blink_set;
+
+ init_data.fwnode = chip->leds[i].fwnode;
+ init_data.devicename = AW2013_NAME;
+ init_data.default_label = ":";
Please drop two above lines, they are needed only when converting
old drivers to the new LED registration API.
+
+ ret = devm_led_classdev_register_ext(>dev,
+>leds[i].cdev,
+_data);
+ if (ret < 0)
+ goto error;
+ }
+ return 0;
+
+error:
+ mutex_destroy(>mutex);
+ return ret;
+}
+
+static int aw2013_remove(struct i2c_client *client)
+{
+ struct aw2013 *chip = i2c_get_clientdata(client);
+
+ mutex_destroy(>mutex);
+
+ return 0;
+}
+
+static const struct of_device_id aw2013_match_table[] = {
+ { .compatible = "awinic,aw2013", },
+ { /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, aw2013_match_table);
+
+static struct i2c_driver aw2013_driver = {
+ .driver = {
+ .name = "leds-aw2013",
+ .of_match_table = of_match_ptr(aw2013_match_table),
+ },
+ .probe_new = aw2013_probe,
+ .remove = aw2013_remove,
+};
+
+module_i2c_driver(aw2013_driver);
+
+MODULE_AUTHOR("Nikita Travkin ");
+MODULE_DESCRIPTION("AW2013 LED driver");
+MODULE_LICENSE("GPL v2");
--
Best regards,
Jacek Anaszewski
Re: [PATCH v24 01/16] dt: bindings: Add multicolor class dt bindings documention
Dan,
On 5/3/20 2:32 PM, Dan Murphy wrote:
Add DT bindings for the LEDs multicolor class framework.
Add multicolor ID to the color ID list for device tree bindings.
CC: Rob Herring
Acked-by: Pavel Machek
Acked-by: Jacek Anaszewski
Signed-off-by: Dan Murphy
---
.../bindings/leds/leds-class-multicolor.yaml | 71 +++
drivers/leds/led-core.c | 1 +
include/dt-bindings/leds/common.h | 3 +-
3 files changed, 74 insertions(+), 1 deletion(-)
create mode 100644
Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
new file mode 100644
index ..16ffafc062ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-class-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for the multicolor LED class.
+
+maintainers:
+ - Dan Murphy
+
+description: |
+ Bindings for multi color LEDs show how to describe current outputs of
+ either integrated multi-color LED elements (like RGB, RGBW, RGBWA-UV
+ etc.) or standalone LEDs, to achieve logically grouped multi-color LED
+ modules. This is achieved by adding multi-led nodes layer to the
+ monochrome LED bindings.
+ The nodes and properties defined in this document are unique to the
multicolor
+ LED class. Common LED nodes and properties are inherited from the common.txt
+ within this documentation directory.
+
+properties:
+ color:
+description: |
+ For multicolor LED support this property should be defined as
+ LED_COLOR_ID_MULTI and further definition can be found in
+ include/linux/leds/common.h.
+
+required:
+ - color
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+led-controller@14 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "ti,lp5009";
+ reg = <0x14>; > +
+multi-led@1 {
Whole multi-led subnode is contained in the led-controller
node, so it must have one more indentation level.
No need to resend whole patch set, just bump the version
of this single patch.
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ color = ;
+ function = LED_FUNCTION_CHARGING;
+
+ led@0 {
+reg = <0>;
+color = ;
+ };
+
+ led@1 {
+reg = <1>;
+color = ;
+ };
+
+ led@2 {
+reg = <2>;
+color = ;
+ };
+};
+ };
+};
+
+additionalProperties: false
+...
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..846248a0693d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -34,6 +34,7 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
[LED_COLOR_ID_VIOLET] = "violet",
[LED_COLOR_ID_YELLOW] = "yellow",
[LED_COLOR_ID_IR] = "ir",
+ [LED_COLOR_ID_MULTI] = "multicolor",
};
EXPORT_SYMBOL_GPL(led_colors);
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 0ce7dfc00dcb..a463ce6a8794 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -30,7 +30,8 @@
#define LED_COLOR_ID_VIOLET 5
#define LED_COLOR_ID_YELLOW 6
#define LED_COLOR_ID_IR 7
-#define LED_COLOR_ID_MAX 8
+#define LED_COLOR_ID_MULTI 8
+#define LED_COLOR_ID_MAX 9
/* Standard LED functions */
/* Keyboard LEDs, usually it would be input4::capslock etc. */
--
Best regards,
Jacek Anaszewski
Re: [PATCH v23 02/16] leds: multicolor: Introduce a multicolor class definition
Dan, I've converted drivers/leds/leds-an30259a.c to LED mc framework and tested it on Samsung Galaxy S3 (exysnos4412-trats2 board). Works as expected. And now the framework usability is indeed neater. One thing to improve: LED mc based drivers' entries in Kconfig should have this dependency: depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR It is required to enforce building driver as a module if LED mc framework is configured as such. With this (and DT bindings nits) addressed, for patches 1-6: Acked-by: Jacek Anaszewski It's been a long journey. Thank you for your determination to drive this work to the end. Best regards, Jacek Anaszewski On 4/29/20 10:28 PM, Dan Murphy wrote: Introduce a multicolor class that groups colored LEDs within a LED node. The multi color class groups monochrome LEDs and allows controlling two aspects of the final combined color: hue and lightness. The former is controlled via the intensity file and the latter is controlled via brightness file. Signed-off-by: Dan Murphy --- .../ABI/testing/sysfs-class-led-multicolor| 34 +++ Documentation/leds/index.rst | 1 + Documentation/leds/leds-class-multicolor.rst | 86 +++ MAINTAINERS | 8 + drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/led-class-multicolor.c | 210 ++ include/linux/led-class-multicolor.h | 121 ++ 8 files changed, 471 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor create mode 100644 Documentation/leds/leds-class-multicolor.rst create mode 100644 drivers/leds/led-class-multicolor.c create mode 100644 include/linux/led-class-multicolor.h [...]
Re: [PATCH v23 01/16] dt: bindings: Add multicolor class dt bindings documention
Dan,
Thanks for improving the bindings. Now we have one indentation
related issue, please look below at the example.
On 4/29/20 10:28 PM, Dan Murphy wrote:
Add DT bindings for the LEDs multicolor class framework.
Add multicolor ID to the color ID list for device tree bindings.
CC: Rob Herring
Acked-by: Pavel Machek
Signed-off-by: Dan Murphy
---
.../bindings/leds/leds-class-multicolor.yaml | 70 +++
drivers/leds/led-core.c | 1 +
include/dt-bindings/leds/common.h | 3 +-
3 files changed, 73 insertions(+), 1 deletion(-)
create mode 100644
Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
new file mode 100644
index ..e6169ed5ed12
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-class-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for the multicolor LED class.
+
+maintainers:
+ - Dan Murphy
+
+description: |
+ Bindings for multi color LEDs show how to describe current outputs of
+ either integrated multi-color LED elements (like RGB, RGBW, RGBWA-UV
+ etc.) or standalone LEDs, to achieve logically grouped multi-color LED
+ modules. This is achieved by adding multi-led nodes layer to the
+ monochrome LED bindings.
+ The nodes and properties defined in this document are unique to the
multicolor
+ LED class. Common LED nodes and properties are inherited from the common.txt
+ within this documentation directory.
+
+properties:
+ color:
+description: |
+ For multicolor LED support this property should be defined as
+ LED_COLOR_ID_MULTI and further definition can be found in
+ include/linux/leds/common.h.
+
+required:
+ - color
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
It would look neater if we had an empty line here.
+led-controller@14 {
We should have one more level of indentation below
+#address-cells = <1>;
+#size-cells = <0>;
+compatible = "ti,lp5009";
+reg = <0x14>;
+
+multi-led@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ color = ;
+ function = LED_FUNCTION_CHARGING;
+
+ led@0 {
+reg = <0>;
+color = ;
+ };
+
+ led@1 {
+reg = <1>;
+color = ;
+ };
+
+ led@2 {
+reg = <2>;
+color = ;
+ };
+};
+ };
+};
+
+additionalProperties: false
+...
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..846248a0693d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -34,6 +34,7 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
[LED_COLOR_ID_VIOLET] = "violet",
[LED_COLOR_ID_YELLOW] = "yellow",
[LED_COLOR_ID_IR] = "ir",
+ [LED_COLOR_ID_MULTI] = "multicolor",
};
EXPORT_SYMBOL_GPL(led_colors);
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 0ce7dfc00dcb..a463ce6a8794 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -30,7 +30,8 @@
#define LED_COLOR_ID_VIOLET 5
#define LED_COLOR_ID_YELLOW 6
#define LED_COLOR_ID_IR 7
-#define LED_COLOR_ID_MAX 8
+#define LED_COLOR_ID_MULTI 8
+#define LED_COLOR_ID_MAX 9
/* Standard LED functions */
/* Keyboard LEDs, usually it would be input4::capslock etc. */
Re: [PATCH v22 01/16] dt: bindings: Add multicolor class dt bindings documention
Hi Dan,
Thanks for the conversion, but now the binding example is missing.
In Documentation/devicetree/bindings/leds/common.yaml we do have
examples.
Best regards,
Jacek Anaszewski
On 4/29/20 2:56 PM, Dan Murphy wrote:
Add DT bindings for the LEDs multicolor class framework.
Add multicolor ID to the color ID list for device tree bindings.
CC: Rob Herring
Acked-by: Pavel Machek
Signed-off-by: Dan Murphy
---
.../bindings/leds/leds-class-multicolor.yaml | 34 +++
drivers/leds/led-core.c | 1 +
include/dt-bindings/leds/common.h | 3 +-
3 files changed, 37 insertions(+), 1 deletion(-)
create mode 100644
Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
new file mode 100644
index ..3d4f23d07440
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-class-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for the multicolor LED class.
+
+maintainers:
+ - Dan Murphy
+
+description: |
+ Bindings for multi color LEDs show how to describe current outputs of
+ either integrated multi-color LED elements (like RGB, RGBW, RGBWA-UV
+ etc.) or standalone LEDs, to achieve logically grouped multi-color LED
+ modules. This is achieved by adding multi-led nodes layer to the
+ monochrome LED bindings.
+ The nodes and properties defined in this document are unique to the
multicolor
+ LED class. Common LED nodes and properties are inherited from the common.txt
+ within this documentation directory.
+
+properties:
+ color:
+description: |
+ For multicolor LED support this property should be defined as
+ LED_COLOR_ID_MULTI and further definition can be found in
+ include/linux/leds/common.h.
+
+required:
+ - color
+
+additionalProperties: false
+
+...
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..846248a0693d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -34,6 +34,7 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
[LED_COLOR_ID_VIOLET] = "violet",
[LED_COLOR_ID_YELLOW] = "yellow",
[LED_COLOR_ID_IR] = "ir",
+ [LED_COLOR_ID_MULTI] = "multicolor",
};
EXPORT_SYMBOL_GPL(led_colors);
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 0ce7dfc00dcb..a463ce6a8794 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -30,7 +30,8 @@
#define LED_COLOR_ID_VIOLET 5
#define LED_COLOR_ID_YELLOW 6
#define LED_COLOR_ID_IR 7
-#define LED_COLOR_ID_MAX 8
+#define LED_COLOR_ID_MULTI 8
+#define LED_COLOR_ID_MAX 9
/* Standard LED functions */
/* Keyboard LEDs, usually it would be input4::capslock etc. */
Re: [PATCH v21 11/16] leds: lp55xx: Add multicolor framework support to lp55xx
Dan, On 4/28/20 6:03 PM, Dan Murphy wrote: Add multicolor framework support for the lp55xx family. Acked-by: Pavel Machek Signed-off-by: Dan Murphy --- drivers/leds/Kconfig | 1 + drivers/leds/leds-lp5521.c| 14 +- drivers/leds/leds-lp5523.c| 14 +- drivers/leds/leds-lp5562.c| 13 +- drivers/leds/leds-lp55xx-common.c | 178 +++--- drivers/leds/leds-lp55xx-common.h | 14 +- drivers/leds/leds-lp8501.c| 14 +- include/linux/platform_data/leds-lp55xx.h | 8 + 8 files changed, 209 insertions(+), 47 deletions(-) For patches 10/16 - 16/16: Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH v21 02/16] leds: multicolor: Introduce a multicolor class definition
root root 4096 Oct 19 16:16 multi_intensity What about max_brightness? + +Multicolor Class Brightness Control +=== +The multicolor class framework will calculate each monochrome LEDs intensity. + +The brightness level for each LED is calculated based on the color LED +intensity setting divided by the parent max_brightness setting multiplied by Parent is counter-intuitive in this case. I'd call it global. +the requested brightness. + +led_brightness = brightness * multi_intensity/max_brightness + +Example: +A user first writes the multi_intensity file with the brightness levels +for each LED that are necessary to achieve a certain color output from a +multicolor LED group. + +cat /sys/class/leds/multicolor:status/multi_index +green blue red + +echo 43 226 138 > /sys/class/leds/multicolor:status/multi_intensity + +red - + intensity = 138 + max_brightness = 255 +green - + intensity = 43 + max_brightness = 255 +blue - + intensity = 226 + max_brightness = 255 + +The user can control the brightness of that multicolor LED group by writing the +parent 'brightness' control. Assuming a max_brightness of 255 the user s/parent/global/ +may want to dim the LED color group to half. The user would write a value of +128 to the parent brightness file then the values written to each LED will be Ditto. +adjusted base on this value. + +cat /sys/class/leds/multicolor:status/max_brightness +255 +echo 128 > /sys/class/leds/multicolor:status/brightness + +adjusted_red_value = 128 * 138/255 = 69 +adjusted_green_value = 128 * 43/255 = 21 +adjusted_blue_value = 128 * 226/255 = 113 + +Reading the parent brightness file will return the current brightness value of Ditto. -- Best regards, Jacek Anaszewski
Re: [PATCH v21 01/16] dt: bindings: Add multicolor class dt bindings documention
Dan, On 4/28/20 6:03 PM, Dan Murphy wrote: Add DT bindings for the LEDs multicolor class framework. Add multicolor ID to the color ID list for device tree bindings. CC: Rob Herring Acked-by: Pavel Machek Signed-off-by: Dan Murphy --- .../bindings/leds/leds-class-multicolor.txt | 98 +++ drivers/leds/led-core.c | 1 + include/dt-bindings/leds/common.h | 3 +- 3 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt new file mode 100644 index ..4b1bd82f2a42 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt Why isn't it yaml? -- Best regards, Jacek Anaszewski
Re: [PATCH v21 04/16] leds: lp50xx: Add the LP50XX family of the RGB LED driver
Hi Dan,
Thanks for the update. There is one remnant from the
previous stages that you already scheduled for removal AFAIR.
On 4/28/20 6:03 PM, Dan Murphy wrote:
Introduce the LP5036/30/24/18/12/9 RGB LED driver.
The difference in these parts are the number of
LED outputs where the:
LP5036 can control 36 LEDs
LP5030 can control 30 LEDs
LP5024 can control 24 LEDs
LP5018 can control 18 LEDs
LP5012 can control 12 LEDs
LP5009 can control 9 LEDs
The device has the ability to group LED output into control banks
so that multiple LED banks can be controlled with the same mixing and
brightness. Inversely the LEDs can also be controlled independently.
Signed-off-by: Dan Murphy
---
[...]
+
+ init_data.fwnode = child;
+ init_data.devicename = priv->client->name;
Namely this line. We don't need devicename for new drivers like this.
+ fwnode_property_read_string(child, "linux,default-trigger",
+ >led_dev.default_trigger);
+ num_colors = 0;
+
+ /* There are only 3 LEDs per module otherwise they should be
+ banked which also is presented as 3 LEDs*/
+ mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
+ sizeof(*mc_led_info), GFP_KERNEL);
+ if (!mc_led_info)
+ return -ENOMEM;
+
+ fwnode_for_each_child_node(child, led_node) {
+ ret = fwnode_property_read_u32(led_node, "color",
+ _id);
+ if (ret)
+ dev_err(priv->dev, "Cannot read color\n");
+
+ mc_led_info[num_colors].color_index = color_id;
+ num_colors++;
+ }
+
+ led->priv = priv;
+ led->mc_cdev.num_colors = num_colors;
+ led->mc_cdev.subled_info = mc_led_info;
+ led_cdev = >mc_cdev.led_cdev;
+ led_cdev->brightness_set_blocking = lp50xx_brightness_set;
+ ret =
devm_led_classdev_multicolor_register_ext(>client->dev,
+ >mc_cdev,
+ _data);
[...]
--
Best regards,
Jacek Anaszewski
Re: [PATCH v14 13/19] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
On 10/22/19 6:37 PM, Dan Murphy wrote:
> Jacek
>
> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>> Dan,
>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>> I went into details of this parsing and finally came up with
>>> the code which is a bit greater in size, but IMHO cleaner.
>>> Note changes in variable naming. It is not even compile-tested.
>>>
>>> static int lp55xx_parse_common_child(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int led_number, int *chan_nr)
>>> {
>>> int ret;
>>>
>>> of_property_read_string(np, "chan-name",
>>> [led_number].name);
>>> of_property_read_u8(np, "led-cur",
>>> [led_number].led_current);
>>> of_property_read_u8(np, "max-cur",
>>> [led_number].max_current);
>>>
>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>> if (ret)
>>> return ret;
>>>
>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>> new
>>> max_chan_nr property needed in cfg */
>>> return -EINVAL;
>>>
>>> return 0;
>>> }
>>>
>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int child_number,
>>> int color_number)
>>> {
>>> int chan_nr, color_id;
>>>
>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>> color_number,
>>> _nr);
>>> if (ret)
>>> return ret;
>>>
>>> ret = of_property_read_u32(child, "color", _id);
>>> if (ret)
>>> return ret;
>>>
>>> cfg[child_number].color_components[color_number].color_id =
>>> color_id;
>>> cfg[child_number].color_components[color_number].output_num =
>>> chan_nr;
>>> set_bit(color_id, [child_number].available_colors);
>>>
>>> return 0;
>>> }
>>>
>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int child_number)
>>> {
>>> struct device_node *child;
>>> int num_colors = 0, i = 0;
>> s/, i = 0//
>>
>>> for_each_child_of_node(np, child) {
>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>> num_colors,
>>> child_number, i))
>> Replace above call with below:
>>
>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number, num_colors);
>>
> I applied your DT parser patch from the v13 series. Which eliminates
> this comment correct?
Yes, it contains this fix.
--
Best regards,
Jacek Anaszewski
Re: [RFC PATCH 11/13] led: bd71828: Support LED outputs on ROHM BD71828 PMIC
Matti,
On 10/22/19 2:40 PM, Vaittinen, Matti wrote:
> Hello Jacek,
>
> Thanks for the clarifications. I think I now understand the LED
> subsystem a bit better :)
>
> On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote:
>> Hi Matti,
>>
>> On 10/21/19 10:00 AM, Vaittinen, Matti wrote:
>>> Hello Dan,
>>>
>>> Thanks for taking the time to check my driver :) I truly appreciate
>>> all
>>> the help!
>>>
>>> A "fundamental question" regarding these review comments is whether
>>> I
>>> should add DT entries for these LEDs or not. I thought I shouldn't
>>> but
>>> I would like to get a comment from Rob regarding it.
>>
>> If the LED controller is a part of MFD device probed from DT then
>> there is no doubt it should have corresponding DT sub-node.
>
> Sorry but I still see no much benefit from adding this information in
> DT. Why should it have corresponding DT-node if the LED properties are
> fixed and if we only wish to allow user-space control and have no
> dependencies to other devices in DT?
>
> In this specific case the information we can provide from DT is
> supposed to be fixed. No board based variation. Furthermore, there is
> not much generic driver/led core functionality which would be able to
> parse and utilize relevant information from DT. I think we can only
> give the name (function) and colour. And they are supposed to be fixed
> and thus could be just hard-coded in driver. Hard-coding these would be
> simpler and less error prone for users (no DT bindings to write) and
> simpler to create and probably also to maintain (no separate binding
> documents needed for LEDs).
AFAICS it is possible to connect LED of arbitrary color to the iouts
of this device. If this is the case then it is justified to have DT
node only to allow for LED name customization.
> But assuming this is Ok to DT-folks and if you insist - I will add LED
> information to DT for the next patches. Hopefully this extra complexity
> helps in some oddball use-case which I can't foresee =)
>
> Then what comes to the DT format.
>
> Do you think LED subsystem should try to follow the convention with
> other sub-systems and not introduce multiple compatibles for single
> device? MFD can handle instantiating the sub-devices just fine even
> when sub-devices have no own compatible property or of_match. Maybe we
> should also avoid unnecessary sub-nodes when they are not really
> required.
This is beyond my scope of responsibility. It is MFD subsystem thing to
choose the way of LED class driver instantiation. When it comes to
LED subsystem - it expects single compatible pertaining to a physical
device.
Nonetheless, so far we used to have separate compatibles for drivers of
MFD devices' LED cells. If we are going to change that I'd like to see
explicit DT maintainer's statement confirming that.
And one benefit of having separate nodes per MFD cells is that we can
easily discern the support for which cells is to be turned on.
> If we look at clk and GPIOs it is nowadays preferred (as per my
> understanding after discussions with Stephen, Rob and some others -
> please correct me if I am wrong) to place the 'provider' information in
> the MFD node and obtain the relevant properties from parent->of_node in
> the sub-device driver (or for generic properties, in the core
> framework) - at least for simple cases. I guess rationale was that it
> reflects the real hardware better when no artificial sub-nodes are
> required?
>
> I see the example bindings (like max77693 below) you pointed to me
> don't follow that convention but create own sub nodes with own
> compatible properties in MFD for all the logical blocks.
>
> I am asking this as I was "strongly advieced" (a.k.a told to change my
> approach if I wish to get driver ion their trees ;]) by Rob and Stephen
> to not do that with previous PMIC drivers I upstreamed.
>
> As example - the relevant bindings for BD71837 clock output were
> originally:
>
> pmic: bd71837@4b {
> compatible = "rohm,bd71837";
>
> regulators {
> ...
> };
>
> /* Clock node */
> clk: bd71837-32k-out {
> compatible = "rohm,bd71837-clk";
> #clock-cells = <0>;
> clock-frequency = <32768>;
> clock-output-names = "bd71837-32k-out";
> };
> };
>
> but it was preferred to not have the the clk sub-node and place the
> provider information directly in pmic node instead. Result was:
>
> pmic: bd71837@4b {
> compatible = "rohm,bd71837";
>
> #clock-cells = <0&
Re: [RFC PATCH 11/13] led: bd71828: Support LED outputs on ROHM BD71828 PMIC
v.parent); >>> + l = devm_kzalloc(>dev, sizeof(*l), GFP_KERNEL); >>> + if (!l) >>> + return -ENOMEM; >>> + l->bd71828 = bd71828; >>> + a = >amber; >>> + g = >green; >>> + a->id = ID_AMBER_LED; >>> + g->id = ID_GREEN_LED; >>> + a->force_mask = BD71828_MASK_LED_AMBER; >>> + g->force_mask = BD71828_MASK_LED_GREEN; >>> + >>> + a->l.name = ANAME; >>> + g->l.name = GNAME; >>> + a->l.brightness_set_blocking = bd71828_led_brightness_set; >>> + g->l.brightness_set_blocking = bd71828_led_brightness_set; >>> + >>> + ret = devm_led_classdev_register(>dev, >l); >>> + if (ret) >>> + return ret; >>> + >>> + return devm_led_classdev_register(>dev, >l); This way you force users to always register two LED class devices whereas they might need only one. Please compare how other LED class drivers handle DT parsing and LED class device registration. >>> +} >>> + >> >> This looks different. Not sure why you register both LEDs in this >> probe. >> >> You can use the DT to define both LEDs and then each will be probed >> and >> registered separately. > > As I mentioned above, this driver is currently not using DT at all. > Reason why it does not is that I didn't know all the LEDs are usually > having own DT entries/drivers. > > But there is actually reason for bundling the LEDs to same driver. What > is not shown in driver is that LEDs can be controlled by PMIC state > machine so that they are indicating charger states. Other option is This can be handled by the LED trigger that your driver should expose. On activation the trigger would setup the hardware to control the LEDs. But that can be covered later. > driving LEDs using this driver - but forcing one of the LEDs will cause > also the other LED to be under SW control. Eg, one can't control just > single LED using SW, its both of them or none. So this limitation will have to by documented in the trigger ABI. >> This is how it is commonly done. >> >> You can reference the LM36274 led driver as this is a MFD device to >> the >> ti-lmu.c in the MFD directory. > > Thanks for pointing this out. I will gladly see how others have it > implemented! I just would like to know if the DT binding is really a > must? In this case I am unsure what LED related extra information we > could really give from DT (for this specific device). > > I just checked the lm36274 you mentioned. I see that also lm36274 do > parse the dt and set the name itself (so maybe the led_class is not > doing this after all?) - although the name setting code in lm36274 is a > bit peculiar as it loops through all child nodes and overwrites the old > name if it finds more than one "label" properties from nodes (if I read > the code correctly). > > In any case I am unsure what is the benefit from using DT and adding > the DT parsing code for this PMIC's LEDs. I could understand DT usage > if LED class handled dt parsing and if there was something to configure > in BD71828 LEDs - but I see no such benefits in this case. I hope I was able to clarify most of your doubts. -- Best regards, Jacek Anaszewski
Re: [PATCH v13 04/18] leds: multicolor: Introduce a multicolor class definition
Dan,
On 10/16/19 5:59 PM, Dan Murphy wrote:
> Introduce a multicolor class that groups colored LEDs
> within a LED node.
>
> The multi color class groups monochrome LEDs and allows controlling two
> aspects of the final combined color: hue and lightness. The former is
> controlled via _intensity files and the latter is controlled
> via brightness file.
>
> Signed-off-by: Dan Murphy
> ---
[...]
> +static int led_multicolor_init_color(struct led_classdev_mc *mcled_cdev,
> + int color_id)
> +{
> + struct led_classdev *led_cdev = mcled_cdev->led_cdev;
> + struct led_mc_color_entry *mc_priv;
> + char *intensity_file_name;
> + char *max_intensity_file_name;
> + size_t len;
> + int ret;
> +
> + mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), GFP_KERNEL);
> + if (!mc_priv)
> + return -ENOMEM;
> +
> + mc_priv->led_color_id = color_id;
> + mc_priv->mcled_cdev = mcled_cdev;
> +
> + sysfs_attr_init(_priv->intensity_attr.attr);
> + len = strlen(led_colors[color_id]) + strlen(INTENSITY_NAME) + 1;
> + intensity_file_name = kzalloc(len, GFP_KERNEL);
> + if (!intensity_file_name)
> + return -ENOMEM;
> +
> + snprintf(intensity_file_name, len, "%s%s",
> + led_colors[color_id], INTENSITY_NAME);
> + mc_priv->intensity_attr.attr.name = intensity_file_name;
> + mc_priv->intensity_attr.attr.mode = 0644;
> + mc_priv->intensity_attr.store = intensity_store;
> + mc_priv->intensity_attr.show = intensity_show;
> + ret = sysfs_add_file_to_group(_cdev->dev->kobj,
> + _priv->intensity_attr.attr,
> + led_color_group.name);
> + if (ret)
> + goto intensity_err_out;
> +
> + sysfs_attr_init(_priv->max_intensity_attr.attr);
> + len = strlen(led_colors[color_id]) + strlen(MAX_INTENSITY_NAME) + 1;
> + max_intensity_file_name = kzalloc(len, GFP_KERNEL);
> + if (!max_intensity_file_name) {
> + ret = -ENOMEM;
> + goto intensity_err_out;
> + }
> +
> + snprintf(max_intensity_file_name, len, "%s%s",
> + led_colors[color_id], MAX_INTENSITY_NAME);
> + mc_priv->max_intensity_attr.attr.name = max_intensity_file_name;
> + mc_priv->max_intensity_attr.attr.mode = 0444;
> + mc_priv->max_intensity_attr.show = max_intensity_show;
> + ret = sysfs_add_file_to_group(_cdev->dev->kobj,
> + _priv->max_intensity_attr.attr,
> + led_color_group.name);
> + if (ret)
> + goto max_intensity_err_out;
> +
> + mc_priv->max_intensity = LED_FULL;
> + list_add_tail(_priv->list, _cdev->color_list);
We don't need the list here since our collection of color LEDs will
be fixed. Instead of the list we can do with a dynamically allocated
array of a size depending on available color LEDs.
It will allow also to get rid of lp55xx_map_channel() since random
access to array elements will be possible via lookup tables mapping
colors to array element id.
And regarding my amendments to the DT parser - attached is the
patch for your patch, that is compile-tested.
> +
> +max_intensity_err_out:
> + kfree(max_intensity_file_name);
> +intensity_err_out:
> + kfree(intensity_file_name);
> + return ret;
> +}
> +
>
--
Best regards,
Jacek Anaszewski
From fb0ce79b97acf6ee68ec4a2a9e24d56080826766 Mon Sep 17 00:00:00 2001
From: Jacek Anaszewski
Date: Sat, 19 Oct 2019 19:45:18 +0200
Subject: [PATCH] DT parser amendments
---
drivers/leds/leds-lp55xx-common.c | 109 --
include/linux/platform_data/leds-lp55xx.h | 1 +
2 files changed, 61 insertions(+), 49 deletions(-)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 0764520bc4a8..0244ec9bad8d 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -590,82 +590,93 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
}
EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
-static int lp5xx_parse_common_child(struct device_node *np,
-struct lp55xx_led_config *cfg,
-int chan_num, bool is_multicolor,
-int color_num)
+static int lp55xx_parse_common_child(struct device_node *np,
+ struct lp55xx_led_config *cfg,
+ int led_number, int *chan_nr)
{
- u32 led_number;
int ret;
of_property_read_string(np, "chan-name",
-[chan_num].name);
+[led_number].name);
of_property_read_u8(np, "led-cur",
- [chan_num].led_current);
+ [led_number].led_c
Re: [PATCH v14 13/19] leds: lp55xx: Add multicolor framework support to lp55xx
Dan,
I forgot to mention one thing below.
On 10/18/19 2:25 PM, Dan Murphy wrote:
> Add multicolor framework support for the lp55xx family.
>
> Signed-off-by: Dan Murphy
> ---
[...]
> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
>
> if (led->chan_nr >= max_channel) {
> dev_err(dev, "Use channel numbers between 0 and %d\n",
> @@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
> return -EINVAL;
> }
>
> - led->cdev.brightness_set_blocking = lp55xx_set_brightness;
> - led->cdev.groups = lp55xx_led_groups;
> -
> - if (pdata->led_config[chan].name) {
> - led->cdev.name = pdata->led_config[chan].name;
> - } else {
> - snprintf(name, sizeof(name), "%s:channel%d",
> - pdata->label ? : chip->cl->name, chan);
> - led->cdev.name = name;
> - }
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> + if (pdata->led_config[chan].num_colors > 1)
> + ret = devm_led_classdev_multicolor_register(dev, >mc_cdev);
> + else
> +#endif
This looks odd. I think that no-ops from previous version were OK.
> + ret = devm_led_classdev_register(dev, >cdev);
>
> - ret = devm_led_classdev_register(dev, >cdev);
> if (ret) {
> dev_err(dev, "led register err: %d\n", ret);
> return ret;
> @@ -525,6 +592,82 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
> }
> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
>
--
Best regards,
Jacek Anaszewski
