* [PATCH v5 0/2] leds: mt6360: Add LED driver for MT6360 @ 2020-10-07 1:42 Gene Chen 2020-10-07 1:42 ` [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen 2020-10-07 1:42 ` [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 0 siblings, 2 replies; 16+ messages in thread From: Gene Chen @ 2020-10-07 1:42 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, matthias.bgg Cc: dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao This patch series add MT6360 LED support contains driver and binding document Gene Chen (2) dt-bindings: leds: Add bindings for MT6360 LED leds: mt6360: Add LED driver for MT6360 Documentation/devicetree/bindings/leds/leds-mt6360.yaml | 95 + drivers/leds/Kconfig | 12 drivers/leds/Makefile | 1 drivers/leds/leds-mt6360.c | 783 ++++++++++++++++ 4 files changed, 891 insertions(+) changelogs between v1 & v2 - add led driver with mfd changelogs between v2 & v3 - independent add led driver - add dt-binding document - refactor macros definition for easy to debug - parse device tree by fwnode - use devm*ext to register led class device changelogs between v3 & v4 - fix binding document description - use GENMASK and add unit postfix to definition - isink register led class device changelogs between v4 & v5 - change rgb isink to multicolor control - add binding reference to mfd yaml ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED 2020-10-07 1:42 [PATCH v5 0/2] leds: mt6360: Add LED driver for MT6360 Gene Chen @ 2020-10-07 1:42 ` Gene Chen 2020-10-07 13:50 ` Rob Herring 2020-10-08 21:42 ` Jacek Anaszewski 2020-10-07 1:42 ` [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 1 sibling, 2 replies; 16+ messages in thread From: Gene Chen @ 2020-10-07 1:42 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, matthias.bgg Cc: dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao From: Gene Chen <gene_chen@richtek.com> Add bindings document for LED support on MT6360 PMIC Signed-off-by: Gene Chen <gene_chen@richtek.com> --- .../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 0000000..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 <gene_chen@richtek.com> + +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 <dt-bindings/leds/common.h> + led-controller { + compatible = "mediatek,mt6360-led"; + #address-cells = <1>; + #size-cells = <0>; + + led@0 { + reg = <0>; + function = LED_FUNCTION_INDICATOR; + color = <LED_COLOR_ID_RGB>; + led-max-microamp = <24000>; + }; + led@1 { + reg = <1>; + function = LED_FUNCTION_INDICATOR; + color = <LED_COLOR_ID_AMBER>; + default-state = "off"; + led-max-microamp = <150000>; + }; + led@2 { + reg = <2>; + function = LED_FUNCTION_FLASH; + color = <LED_COLOR_ID_WHITE>; + function-enumerator = <1>; + default-state = "off"; + led-max-microamp = <200000>; + flash-max-microamp = <500000>; + flash-max-timeout-us = <1024000>; + }; + led@3 { + reg = <3>; + function = LED_FUNCTION_FLASH; + color = <LED_COLOR_ID_WHITE>; + function-enumerator = <2>; + default-state = "off"; + led-max-microamp = <200000>; + flash-max-microamp = <500000>; + flash-max-timeout-us = <1024000>; + }; + }; +... -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED 2020-10-07 1:42 ` [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen @ 2020-10-07 13:50 ` Rob Herring 2020-10-08 21:42 ` Jacek Anaszewski 1 sibling, 0 replies; 16+ messages in thread From: Rob Herring @ 2020-10-07 13:50 UTC (permalink / raw) To: Gene Chen Cc: robh+dt, shufan_lee, linux-arm-kernel, devicetree, linux-mediatek, gene_chen, matthias.bgg, jacek.anaszewski, pavel, benjamin.chao, cy_huang, dmurphy, linux-kernel, linux-leds, Wilma.Wu On Wed, 07 Oct 2020 09:42:45 +0800, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Add bindings document for LED support on MT6360 PMIC > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > .../devicetree/bindings/leds/leds-mt6360.yaml | 95 ++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml > My bot found errors running 'make dt_binding_check' on your patch: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-mt6360.example.dt.yaml: led-controller: led@0:color:0:0: 9 is greater than the maximum of 8 From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-mt6360.yaml See https://patchwork.ozlabs.org/patch/1377747 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED 2020-10-07 1:42 ` [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen 2020-10-07 13:50 ` Rob Herring @ 2020-10-08 21:42 ` Jacek Anaszewski 1 sibling, 0 replies; 16+ messages in thread From: Jacek Anaszewski @ 2020-10-08 21:42 UTC (permalink / raw) To: Gene Chen, pavel, robh+dt, matthias.bgg Cc: dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Hi Gene, Thanks for the update. On 10/7/20 3:42 AM, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Add bindings document for LED support on MT6360 PMIC > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > .../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 0000000..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 <gene_chen@richtek.com> > + > +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 <dt-bindings/leds/common.h> > + led-controller { > + compatible = "mediatek,mt6360-led"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + led@0 { > + reg = <0>; > + function = LED_FUNCTION_INDICATOR; > + color = <LED_COLOR_ID_RGB>; > + 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 = <LED_COLOR_ID_AMBER>; > + default-state = "off"; > + led-max-microamp = <150000>; > + }; > + led@2 { > + reg = <2>; > + function = LED_FUNCTION_FLASH; > + color = <LED_COLOR_ID_WHITE>; > + function-enumerator = <1>; > + default-state = "off"; > + led-max-microamp = <200000>; > + flash-max-microamp = <500000>; > + flash-max-timeout-us = <1024000>; > + }; > + led@3 { > + reg = <3>; > + function = LED_FUNCTION_FLASH; > + color = <LED_COLOR_ID_WHITE>; > + function-enumerator = <2>; > + default-state = "off"; > + led-max-microamp = <200000>; > + flash-max-microamp = <500000>; > + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-07 1:42 [PATCH v5 0/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 2020-10-07 1:42 ` [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen @ 2020-10-07 1:42 ` Gene Chen 2020-10-07 5:11 ` kernel test robot 2020-10-08 21:51 ` Jacek Anaszewski 1 sibling, 2 replies; 16+ messages in thread From: Gene Chen @ 2020-10-07 1:42 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, matthias.bgg Cc: dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao From: Gene Chen <gene_chen@richtek.com> 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 <gene_chen@richtek.com> --- 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 + 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 0000000..ccb0b4f --- /dev/null +++ b/drivers/leds/leds-mt6360.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/delay.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/led-class-flash.h> +#include <linux/led-class-multicolor.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <media/v4l2-flash-led-class.h> + +enum { + MT6360_LED_ISNKRGB = 0, + MT6360_LED_ISNKML, + MT6360_LED_FLASH1, + MT6360_LED_FLASH2, + MT6360_MAX_LEDS +}; + +#define MT6360_REG_RGBEN 0x380 +#define MT6360_REG_ISNK(_led_no) (0x381 + (_led_no)) +#define MT6360_REG_ISNKML 0x384 +#define MT6360_ISNK_ENMASK(_led_no) BIT(7 - (_led_no)) +#define MT6360_ISNKRGB_ENMASK GENMASK(7, 5) +#define MT6360_ISNKML_ENMASK BIT(4) +#define MT6360_ISNK_MASK GENMASK(4, 0) +#define MT6360_CHRINDSEL_MASK BIT(3) + +#define MULTICOLOR_NUM_CHANNELS 3 + +#define MT6360_REG_FLEDEN 0x37E +#define MT6360_REG_STRBTO 0x373 +#define MT6360_REG_FLEDBASE(_id) (0x372 + 4 * (_id - MT6360_LED_FLASH1)) +#define MT6360_REG_FLEDISTRB(_id) (MT6360_REG_FLEDBASE(_id) + 2) +#define MT6360_REG_FLEDITOR(_id) (MT6360_REG_FLEDBASE(_id) + 3) +#define MT6360_REG_CHGSTAT2 0x3E1 +#define MT6360_REG_FLEDSTAT1 0x3E9 +#define MT6360_ITORCH_MASK GENMASK(4, 0) +#define MT6360_ISTROBE_MASK GENMASK(6, 0) +#define MT6360_STRBTO_MASK GENMASK(6, 0) +#define MT6360_TORCHEN_MASK BIT(3) +#define MT6360_STROBEN_MASK BIT(2) +#define MT6360_FLCSEN_MASK(_id) BIT(MT6360_LED_FLASH2 - _id) +#define MT6360_FLEDCHGVINOVP_MASK BIT(3) +#define MT6360_FLED1STRBTO_MASK BIT(11) +#define MT6360_FLED2STRBTO_MASK BIT(10) +#define MT6360_FLED1STRB_MASK BIT(9) +#define MT6360_FLED2STRB_MASK BIT(8) +#define MT6360_FLED1SHORT_MASK BIT(7) +#define MT6360_FLED2SHORT_MASK BIT(6) +#define MT6360_FLEDLVF_MASK BIT(3) + +#define MT6360_ISNKRGB_STEPUA 2000 +#define MT6360_ISNKRGB_MAXUA 24000 +#define MT6360_ISNKML_STEPUA 5000 +#define MT6360_ISNKML_MAXUA 150000 + +#define MT6360_ITORCH_MINUA 25000 +#define MT6360_ITORCH_STEPUA 12500 +#define MT6360_ITORCH_MAXUA 400000 +#define MT6360_ISTRB_MINUA 50000 +#define MT6360_ISTRB_STEPUA 12500 +#define MT6360_ISTRB_MAXUA 1500000 +#define MT6360_STRBTO_MINUS 64000 +#define MT6360_STRBTO_STEPUS 32000 +#define MT6360_STRBTO_MAXUS 2432000 + +#define STATE_OFF 0 +#define STATE_KEEP 1 +#define STATE_ON 2 + +struct mt6360_led { + union { + struct led_classdev ml; + struct led_classdev_mc rgb; + struct led_classdev_flash flash; + }; + struct v4l2_flash *v4l2_flash; + struct mt6360_priv *priv; + u32 led_no; + u32 default_state; +}; + +struct mt6360_priv { + struct device *dev; + struct regmap *regmap; + struct mutex lock; + unsigned int fled_strobe_used; + unsigned int fled_torch_used; + unsigned int leds_active; + unsigned int leds_count; + struct mt6360_led leds[]; +}; + +static int mt6360_rgb_brightness_set(struct led_classdev *lcdev, enum led_brightness level) +{ + struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev); + struct mt6360_led *led = container_of(mccdev, struct mt6360_led, rgb); + struct mt6360_priv *priv = led->priv; + u32 real_bright, enable = 0; + int i, ret; + + mutex_lock(&priv->lock); + + led_mc_calc_color_components(mccdev, level); + + for (i = 0; i < MULTICOLOR_NUM_CHANNELS; i++) { + real_bright = min(lcdev->max_brightness, mccdev->subled_info[i].brightness); + ret = regmap_update_bits(priv->regmap, MT6360_REG_ISNK(i), MT6360_ISNK_MASK, + real_bright); + if (ret) + goto out; + + if (real_bright) + enable |= MT6360_ISNK_ENMASK(i); + } + + ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, MT6360_ISNKRGB_ENMASK, enable); + +out: + mutex_unlock(&priv->lock); + return ret; +} + +static int mt6360_moonlight_brightness_set(struct led_classdev *lcdev, enum led_brightness level) +{ + struct mt6360_led *led = container_of(lcdev, struct mt6360_led, ml); + struct mt6360_priv *priv = led->priv; + u32 val = level ? MT6360_ISNKML_ENMASK : 0; + int ret; + + mutex_lock(&priv->lock); + + ret = regmap_update_bits(priv->regmap, MT6360_REG_ISNKML, MT6360_ISNK_MASK, level); + if (ret) + goto out; + + ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, MT6360_ISNKML_ENMASK, val); + +out: + mutex_unlock(&priv->lock); + return ret; +} + +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level) +{ + 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; + + mutex_lock(&priv->lock); + + /* Only one set of flash control logic, use the flag to avoid strobe is currently used */ + if (priv->fled_strobe_used) { + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used); + ret = -EBUSY; + goto out; + } + + if (level) + curr = prev | BIT(led->led_no); + else + curr = prev & ~BIT(led->led_no); + + if (curr) + val |= MT6360_TORCHEN_MASK; + + if (level) { + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDITOR(led->led_no), + MT6360_ITORCH_MASK, level - 1); + if (ret) + goto out; + } + + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val); + if (ret) + goto out; + + priv->fled_torch_used = curr; + +out: + mutex_unlock(&priv->lock); + return ret; +} + +static int mt6360_flash_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness) +{ + /* + * Due to the current spike when turning on flash, let brightness to be kept by framework. + * This empty function is used to prevent led_classdev_flash register ops check failure. + */ + return 0; +} + +static int _mt6360_flash_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness) +{ + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash); + struct mt6360_priv *priv = led->priv; + struct led_flash_setting *s = &fl_cdev->brightness; + u32 val = (brightness - s->min) / s->step; + + return regmap_update_bits(priv->regmap, MT6360_REG_FLEDISTRB(led->led_no), + MT6360_ISTROBE_MASK, val); +} + +static int mt6360_strobe_set(struct led_classdev_flash *fl_cdev, bool state) +{ + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash); + struct mt6360_priv *priv = led->priv; + struct led_classdev *lcdev = &fl_cdev->led_cdev; + struct led_flash_setting *s = &fl_cdev->brightness; + u32 enable_mask = MT6360_STROBEN_MASK | MT6360_FLCSEN_MASK(led->led_no); + u32 val = state ? MT6360_FLCSEN_MASK(led->led_no) : 0; + u32 prev = priv->fled_strobe_used, curr; + int ret; + + /* Only one set of flash control logic, use the flag to avoid torch is currently used */ + if (priv->fled_torch_used) { + dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used); + return -EBUSY; + } + + if (state) + curr = prev | BIT(led->led_no); + else + curr = prev & ~BIT(led->led_no); + + if (curr) + val |= MT6360_STROBEN_MASK; + + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val); + if (ret) { + dev_err(lcdev->dev, "[%d] control current source %d fail\n", led->led_no, state); + return ret; + } + + /* + * If the flash need to be on, config the flash current ramping up to the setting value + * Else, always recover back to the minimum one + */ + ret = _mt6360_flash_brightness_set(fl_cdev, state ? s->val : s->min); + if (ret) + return ret; + + /* For the flash turn on/off, HW rampping up/down time is 5ms/500us, respectively */ + if (!prev && curr) + usleep_range(5000, 6000); + else if (prev && !curr) + udelay(500); + + priv->fled_strobe_used = curr; + return 0; +} + +static int mt6360_strobe_get(struct led_classdev_flash *fl_cdev, bool *state) +{ + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash); + struct mt6360_priv *priv = led->priv; + + *state = !!(priv->fled_strobe_used & BIT(led->led_no)); + return 0; +} + +static int mt6360_timeout_set(struct led_classdev_flash *fl_cdev, u32 timeout) +{ + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash); + struct mt6360_priv *priv = led->priv; + struct led_flash_setting *s = &fl_cdev->timeout; + u32 val = (timeout - s->min) / s->step; + + return regmap_update_bits(priv->regmap, MT6360_REG_STRBTO, MT6360_STRBTO_MASK, val); + +} + +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; + + ret = regmap_read(priv->regmap, MT6360_REG_CHGSTAT2, &chg_stat); + if (ret) + return ret; + + ret = regmap_raw_read(priv->regmap, MT6360_REG_FLEDSTAT1, &fled_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; +} + +static const struct led_flash_ops mt6360_flash_ops = { + .flash_brightness_set = mt6360_flash_brightness_set, + .strobe_set = mt6360_strobe_set, + .strobe_get = mt6360_strobe_get, + .timeout_set = mt6360_timeout_set, + .fault_get = mt6360_fault_get, +}; + +static int mt6360_isnk_init_default_state(struct mt6360_led *led) +{ + struct mt6360_priv *priv = led->priv; + unsigned int regval; + u32 level; + int ret; + + ret = regmap_read(priv->regmap, MT6360_REG_ISNKML, ®val); + if (ret) + return ret; + level = regval & MT6360_ISNK_MASK; + + ret = regmap_read(priv->regmap, MT6360_REG_RGBEN, ®val); + if (ret) + return ret; + + if (!(regval & MT6360_ISNKML_ENMASK)) + level = LED_OFF; + + switch (led->default_state) { + case STATE_ON: + led->ml.brightness = led->ml.max_brightness; + break; + case STATE_KEEP: + led->ml.brightness = min(level, led->ml.max_brightness); + break; + default: + led->ml.brightness = LED_OFF; + } + + return mt6360_moonlight_brightness_set(&led->ml, led->ml.brightness); +} + +static int mt6360_flash_init_default_state(struct mt6360_led *led) +{ + struct led_classdev_flash *flash = &led->flash; + struct mt6360_priv *priv = led->priv; + u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no); + u32 level; + unsigned int regval; + int ret; + + ret = regmap_read(priv->regmap, MT6360_REG_FLEDITOR(led->led_no), ®val); + if (ret) + return ret; + level = regval & MT6360_ITORCH_MASK; + + ret = regmap_read(priv->regmap, MT6360_REG_FLEDEN, ®val); + if (ret) + return ret; + + if ((regval & enable_mask) == enable_mask) + level += 1; + else + level = LED_OFF; + + switch (led->default_state) { + case STATE_ON: + flash->led_cdev.brightness = flash->led_cdev.max_brightness; + break; + case STATE_KEEP: + flash->led_cdev.brightness = min(level, flash->led_cdev.max_brightness); + break; + default: + flash->led_cdev.brightness = LED_OFF; + } + + return mt6360_torch_brightness_set(&flash->led_cdev, flash->led_cdev.brightness); +} + +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS) +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable) +{ + struct led_classdev_flash *flash = v4l2_flash->fled_cdev; + struct mt6360_led *led = container_of(flash, struct mt6360_led, flash); + struct mt6360_priv *priv = led->priv; + u32 mask = MT6360_FLCSEN_MASK(led->led_no); + u32 val = enable ? mask : 0; + int ret; + + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, mask, val); + if (ret) + return ret; + + if (enable) + priv->fled_strobe_used |= BIT(led->led_no); + else + priv->fled_strobe_used &= ~BIT(led->led_no); + + return 0; +} + +static const struct v4l2_flash_ops v4l2_flash_ops = { + .external_strobe_set = mt6360_flash_external_strobe_set, +}; + +static void mt6360_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config) +{ + struct led_classdev *lcdev; + struct led_flash_setting *s = &config->intensity; + + if (led->led_no == MT6360_LED_ISNKRGB || led->led_no == MT6360_LED_ISNKML) { + if (led->led_no == MT6360_LED_ISNKRGB) { + lcdev = &led->rgb.led_cdev; + s->step = MT6360_ISNKRGB_STEPUA; + } else { + lcdev = &led->ml; + s->step = MT6360_ISNKML_STEPUA; + } + + s->min = 0; + s->val = lcdev->brightness * s->step; + s->max = lcdev->max_brightness * s->step; + } else { + lcdev = &led->flash.led_cdev; + + s->min = MT6360_ITORCH_MINUA; + s->step = MT6360_ITORCH_STEPUA; + s->val = s->max = s->min + (lcdev->max_brightness - 1) * s->step; + + config->has_external_strobe = 1; + } + + strscpy(config->dev_name, lcdev->dev->kobj.name, sizeof(config->dev_name)); +} +#else +static const struct v4l2_flash_ops v4l2_flash_ops; + +static void mt6360_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config) +{ +} +#endif + +static int mt6360_led_register(struct device *parent, struct mt6360_led *led, + struct led_init_data *init_data) +{ + struct mt6360_priv *priv = led->priv; + struct v4l2_flash_config v4l2_config = {0}; + int ret; + + switch (led->led_no) { + case MT6360_LED_ISNKRGB: + /* Change isink1 to SW control mode, disconnect it with charger state */ + ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, + MT6360_CHRINDSEL_MASK, MT6360_CHRINDSEL_MASK); + if (ret) { + dev_err(parent, "Failed to config ISNK1 to SW mode\n"); + return ret; + } + + ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb, init_data); + if (ret) { + dev_err(parent, "Couldn't register isink %d\n", led->led_no); + return ret; + } + + mt6360_init_v4l2_config(led, &v4l2_config); + led->v4l2_flash = v4l2_flash_indicator_init(parent, init_data->fwnode, + &led->rgb.led_cdev, &v4l2_config); + break; + case MT6360_LED_ISNKML: + ret = mt6360_isnk_init_default_state(led); + if (ret) { + dev_err(parent, "Failed to init %d isnk state\n", led->led_no); + return ret; + } + + ret = devm_led_classdev_register_ext(parent, &led->ml, init_data); + if (ret) { + dev_err(parent, "Couldn't register isink %d\n", led->led_no); + return ret; + } + + mt6360_init_v4l2_config(led, &v4l2_config); + led->v4l2_flash = v4l2_flash_indicator_init(parent, init_data->fwnode, &led->ml, + &v4l2_config); + break; + default: + ret = mt6360_flash_init_default_state(led); + if (ret) { + dev_err(parent, "Failed to init %d flash state\n", led->led_no); + return ret; + } + + ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data); + if (ret) { + dev_err(parent, "Couldn't register flash %d\n", led->led_no); + return ret; + } + + mt6360_init_v4l2_config(led, &v4l2_config); + led->v4l2_flash = v4l2_flash_init(parent, init_data->fwnode, &led->flash, + &v4l2_flash_ops, &v4l2_config); + } + + if (IS_ERR(led->v4l2_flash)) { + dev_err(parent, "Failed to register %d v4l2 sd\n", led->led_no); + return PTR_ERR(led->v4l2_flash); + } + + return 0; +} + +static u32 clamp_align(u32 val, u32 min, u32 max, u32 step) +{ + u32 retval; + + retval = clamp_val(val, min, max); + if (step > 1) + retval = rounddown(retval - min, step) + min; + + return retval; +} + +static int mt6360_init_isnk_properties(struct mt6360_led *led, struct led_init_data *init_data) +{ + struct led_classdev *isnk; + struct mt6360_priv *priv = led->priv; + u32 step_uA, max_uA; + u32 val; + int ret; + + if (led->led_no == MT6360_LED_ISNKRGB) { + struct mc_subled *sub_led; + + sub_led = devm_kzalloc(priv->dev, sizeof(*sub_led) * MULTICOLOR_NUM_CHANNELS, + GFP_KERNEL); + if (!sub_led) + return -ENOMEM; + + sub_led[0].color_index = LED_COLOR_ID_RED; + sub_led[0].channel = 0; + sub_led[1].color_index = LED_COLOR_ID_GREEN; + sub_led[1].channel = 1; + sub_led[2].color_index = LED_COLOR_ID_BLUE; + sub_led[2].channel = 2; + + led->rgb.num_colors = MULTICOLOR_NUM_CHANNELS; + led->rgb.subled_info = sub_led; + + isnk = &led->rgb.led_cdev; + + step_uA = MT6360_ISNKRGB_STEPUA; + max_uA = MT6360_ISNKRGB_MAXUA; + + isnk->brightness_set_blocking = mt6360_rgb_brightness_set; + } else { + isnk = &led->ml; + + step_uA = MT6360_ISNKML_STEPUA; + max_uA = MT6360_ISNKML_MAXUA; + + isnk->brightness_set_blocking = mt6360_moonlight_brightness_set; + } + + ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp", &val); + if (ret) { + dev_warn(priv->dev, "Not specified led-max-microamp, config to the minimum step\n"); + val = 1 * step_uA; + } else + val = clamp_align(val, 0, max_uA, step_uA); + + isnk->max_brightness = val / step_uA; + + fwnode_property_read_string(init_data->fwnode, "linux,default-trigger", + &isnk->default_trigger); + + return 0; +} + +static int mt6360_init_flash_properties(struct mt6360_led *led, struct led_init_data *init_data) +{ + struct led_classdev_flash *flash = &led->flash; + struct led_classdev *lcdev = &flash->led_cdev; + struct mt6360_priv *priv = led->priv; + struct led_flash_setting *s; + u32 val; + int ret; + + ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp", &val); + if (ret) { + dev_warn(priv->dev, "Not specified led-max-microamp, config to the minimum\n"); + val = MT6360_ITORCH_MINUA; + } else + val = clamp_align(val, MT6360_ITORCH_MINUA, MT6360_ITORCH_MAXUA, + MT6360_ITORCH_STEPUA); + + lcdev->max_brightness = (val - MT6360_ITORCH_MINUA) / MT6360_ITORCH_STEPUA + 1; + lcdev->brightness_set_blocking = mt6360_torch_brightness_set; + lcdev->flags |= LED_DEV_CAP_FLASH; + + ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-microamp", &val); + if (ret) { + dev_warn(priv->dev, "Not specified flash-max-microamp, config to the minimum\n"); + val = MT6360_ISTRB_MINUA; + } else + val = clamp_align(val, MT6360_ISTRB_MINUA, MT6360_ISTRB_MAXUA, MT6360_ISTRB_STEPUA); + + s = &flash->brightness; + s->min = MT6360_ISTRB_MINUA; + s->step = MT6360_ISTRB_STEPUA; + s->val = s->max = val; + + /* Always configure as min level when off to prevent flash current spike */ + ret = _mt6360_flash_brightness_set(flash, s->min); + if (ret) + return ret; + + ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-timeout-us", &val); + if (ret) { + dev_warn(priv->dev, "Not specified flash-max-timeout-us, config to the minimum\n"); + val = MT6360_STRBTO_MINUS; + } else + val = clamp_align(val, MT6360_STRBTO_MINUS, MT6360_STRBTO_MAXUS, + MT6360_STRBTO_STEPUS); + + s = &flash->timeout; + s->min = MT6360_STRBTO_MINUS; + s->step = MT6360_STRBTO_STEPUS; + s->val = s->max = val; + + flash->ops = &mt6360_flash_ops; + + return 0; +} + +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data) +{ + const char * const states[] = { "off", "keep", "on" }; + const char *str; + int ret; + + if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) { + 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(&pdev->dev); + if (!count || count > MT6360_MAX_LEDS) { + dev_err(&pdev->dev, "No child node or node count over max led number %d\n", count); + return -EINVAL; + } + + priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds, count), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->leds_count = count; + priv->dev = &pdev->dev; + mutex_init(&priv->lock); + + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!priv->regmap) { + dev_err(&pdev->dev, "Failed to get parent regmap\n"); + return -ENODEV; + } + + device_for_each_child_node(&pdev->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, &init_data); + if (ret) + goto out_flash_release; + + if (reg == MT6360_LED_ISNKRGB || reg == MT6360_LED_ISNKML) + ret = mt6360_init_isnk_properties(led, &init_data); + else + ret = mt6360_init_flash_properties(led, &init_data); + + if (ret) + goto out_flash_release; + + ret = mt6360_led_register(&pdev->dev, led, &init_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 <gene_chen@richtek.com>"); +MODULE_DESCRIPTION("MT6360 LED Driver"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-07 1:42 ` [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 Gene Chen @ 2020-10-07 5:11 ` kernel test robot 2020-10-08 21:51 ` Jacek Anaszewski 1 sibling, 0 replies; 16+ messages in thread From: kernel test robot @ 2020-10-07 5:11 UTC (permalink / raw) To: Gene Chen, jacek.anaszewski, pavel, robh+dt, matthias.bgg Cc: kbuild-all, dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5211 bytes --] Hi Gene, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on pavel-linux-leds/for-next] [also build test WARNING on v5.9-rc8 next-20201006] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Gene-Chen/leds-mt6360-Add-LED-driver-for-MT6360/20201007-094408 base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next config: alpha-allyesconfig (attached as .config) compiler: alpha-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 # https://github.com/0day-ci/linux/commit/cdafaffefe3568d483b764206c6cea243203b370 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Gene-Chen/leds-mt6360-Add-LED-driver-for-MT6360/20201007-094408 git checkout cdafaffefe3568d483b764206c6cea243203b370 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/linux/device.h:15, from include/linux/leds.h:12, from include/linux/led-class-flash.h:11, from drivers/leds/leds-mt6360.c:7: drivers/leds/leds-mt6360.c: In function 'mt6360_led_probe': >> drivers/leds/leds-mt6360.c:696:23: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' {aka 'long unsigned int'} [-Wformat=] 696 | dev_err(&pdev->dev, "No child node or node count over max led number %d\n", count); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/leds/leds-mt6360.c:696:3: note: in expansion of macro 'dev_err' 696 | dev_err(&pdev->dev, "No child node or node count over max led number %d\n", count); | ^~~~~~~ drivers/leds/leds-mt6360.c:696:73: note: format string is defined here 696 | dev_err(&pdev->dev, "No child node or node count over max led number %d\n", count); | ~^ | | | int | %ld vim +696 drivers/leds/leds-mt6360.c 686 687 static int mt6360_led_probe(struct platform_device *pdev) 688 { 689 struct mt6360_priv *priv; 690 struct fwnode_handle *child; 691 size_t count; 692 int i = 0, ret; 693 694 count = device_get_child_node_count(&pdev->dev); 695 if (!count || count > MT6360_MAX_LEDS) { > 696 dev_err(&pdev->dev, "No child node or node count over max led number %d\n", count); 697 return -EINVAL; 698 } 699 700 priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds, count), GFP_KERNEL); 701 if (!priv) 702 return -ENOMEM; 703 704 priv->leds_count = count; 705 priv->dev = &pdev->dev; 706 mutex_init(&priv->lock); 707 708 priv->regmap = dev_get_regmap(pdev->dev.parent, NULL); 709 if (!priv->regmap) { 710 dev_err(&pdev->dev, "Failed to get parent regmap\n"); 711 return -ENODEV; 712 } 713 714 device_for_each_child_node(&pdev->dev, child) { 715 struct mt6360_led *led = priv->leds + i; 716 struct led_init_data init_data = { .fwnode = child, }; 717 u32 reg; 718 719 ret = fwnode_property_read_u32(child, "reg", ®); 720 if (ret) 721 goto out_flash_release; 722 723 if (reg >= MT6360_MAX_LEDS || priv->leds_active & BIT(reg)) 724 return -EINVAL; 725 priv->leds_active |= BIT(reg); 726 727 led->led_no = reg; 728 led->priv = priv; 729 730 ret = mt6360_init_common_properties(led, &init_data); 731 if (ret) 732 goto out_flash_release; 733 734 if (reg == MT6360_LED_ISNKRGB || reg == MT6360_LED_ISNKML) 735 ret = mt6360_init_isnk_properties(led, &init_data); 736 else 737 ret = mt6360_init_flash_properties(led, &init_data); 738 739 if (ret) 740 goto out_flash_release; 741 742 ret = mt6360_led_register(&pdev->dev, led, &init_data); 743 if (ret) 744 goto out_flash_release; 745 746 i++; 747 } 748 749 platform_set_drvdata(pdev, priv); 750 return 0; 751 752 out_flash_release: 753 mt6360_v4l2_flash_release(priv); 754 return ret; 755 } 756 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 65738 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-07 1:42 ` [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 2020-10-07 5:11 ` kernel test robot @ 2020-10-08 21:51 ` Jacek Anaszewski 2020-10-20 6:44 ` Gene Chen 1 sibling, 1 reply; 16+ messages in thread From: Jacek Anaszewski @ 2020-10-08 21:51 UTC (permalink / raw) To: Gene Chen, pavel, robh+dt, matthias.bgg Cc: dmurphy, linux-leds, devicetree, linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Hi Gene, On 10/7/20 3:42 AM, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > 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 <gene_chen@richtek.com> > --- > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-08 21:51 ` Jacek Anaszewski @ 2020-10-20 6:44 ` Gene Chen 2020-10-20 21:46 ` Jacek Anaszewski 0 siblings, 1 reply; 16+ messages in thread From: Gene Chen @ 2020-10-20 6:44 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： > > Hi Gene, > > On 10/7/20 3:42 AM, Gene Chen wrote: > > From: Gene Chen <gene_chen@richtek.com> > > > > 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 <gene_chen@richtek.com> > > --- > > 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. > > + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-20 6:44 ` Gene Chen @ 2020-10-20 21:46 ` Jacek Anaszewski 2020-10-27 9:28 ` Gene Chen 0 siblings, 1 reply; 16+ messages in thread From: Jacek Anaszewski @ 2020-10-20 21:46 UTC (permalink / raw) To: Gene Chen Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao On 10/20/20 8:44 AM, Gene Chen wrote: > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： >> >> Hi Gene, >> >> On 10/7/20 3:42 AM, Gene Chen wrote: >>> From: Gene Chen <gene_chen@richtek.com> >>> >>> 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 <gene_chen@richtek.com> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-20 21:46 ` Jacek Anaszewski @ 2020-10-27 9:28 ` Gene Chen 2020-10-27 17:28 ` Jacek Anaszewski 0 siblings, 1 reply; 16+ messages in thread From: Gene Chen @ 2020-10-27 9:28 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： > > On 10/20/20 8:44 AM, Gene Chen wrote: > > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： > >> > >> Hi Gene, > >> > >> On 10/7/20 3:42 AM, Gene Chen wrote: > >>> From: Gene Chen <gene_chen@richtek.com> > >>> > >>> 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 <gene_chen@richtek.com> > >>> --- > >>> 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"? > 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. I will remove "depends", use "select" instead. > -- > Best regards, > Jacek Anaszewski ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-27 9:28 ` Gene Chen @ 2020-10-27 17:28 ` Jacek Anaszewski 2020-10-30 8:51 ` Gene Chen 0 siblings, 1 reply; 16+ messages in thread From: Jacek Anaszewski @ 2020-10-27 17:28 UTC (permalink / raw) To: Gene Chen Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao On 10/27/20 10:28 AM, Gene Chen wrote: > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： >> >> On 10/20/20 8:44 AM, Gene Chen wrote: >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： >>>> >>>> Hi Gene, >>>> >>>> On 10/7/20 3:42 AM, Gene Chen wrote: >>>>> From: Gene Chen <gene_chen@richtek.com> >>>>> >>>>> 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 <gene_chen@richtek.com> >>>>> --- >>>>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-27 17:28 ` Jacek Anaszewski @ 2020-10-30 8:51 ` Gene Chen 2020-10-30 22:34 ` Jacek Anaszewski 0 siblings, 1 reply; 16+ messages in thread From: Gene Chen @ 2020-10-30 8:51 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道： > > On 10/27/20 10:28 AM, Gene Chen wrote: > > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： > >> > >> On 10/20/20 8:44 AM, Gene Chen wrote: > >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： > >>>> > >>>> Hi Gene, > >>>> > >>>> On 10/7/20 3:42 AM, Gene Chen wrote: > >>>>> From: Gene Chen <gene_chen@richtek.com> > >>>>> > >>>>> 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 <gene_chen@richtek.com> > >>>>> --- > >>>>> 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. > 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 depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH depends on MFD_MT6360 and source code add constraint #if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb, init_data); #endif #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data); #endif ============= Or Should I seperate two drivers? one for RGB LED, one for ML LED and FLED > > I will remove "depends", use "select" instead. > > -- > Best regards, > Jacek Anaszewski ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-30 8:51 ` Gene Chen @ 2020-10-30 22:34 ` Jacek Anaszewski 2020-11-16 10:01 ` Gene Chen 0 siblings, 1 reply; 16+ messages in thread From: Jacek Anaszewski @ 2020-10-30 22:34 UTC (permalink / raw) To: Gene Chen Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao On 10/30/20 9:51 AM, Gene Chen wrote: > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道： >> >> On 10/27/20 10:28 AM, Gene Chen wrote: >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： >>>> >>>> On 10/20/20 8:44 AM, Gene Chen wrote: >>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： >>>>>> >>>>>> Hi Gene, >>>>>> >>>>>> On 10/7/20 3:42 AM, Gene Chen wrote: >>>>>>> From: Gene Chen <gene_chen@richtek.com> >>>>>>> >>>>>>> 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 <gene_chen@richtek.com> >>>>>>> --- >>>>>>> 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, &led->rgb, > init_data); > #endif > > #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > ret = devm_led_classdev_flash_register_ext(parent, &led->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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-10-30 22:34 ` Jacek Anaszewski @ 2020-11-16 10:01 ` Gene Chen 2020-11-16 18:25 ` Jacek Anaszewski 0 siblings, 1 reply; 16+ messages in thread From: Gene Chen @ 2020-11-16 10:01 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月31日 週六 上午6:34寫道： > > On 10/30/20 9:51 AM, Gene Chen wrote: > > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道： > >> > >> On 10/27/20 10:28 AM, Gene Chen wrote: > >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： > >>>> > >>>> On 10/20/20 8:44 AM, Gene Chen wrote: > >>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： > >>>>>> > >>>>>> Hi Gene, > >>>>>> > >>>>>> On 10/7/20 3:42 AM, Gene Chen wrote: > >>>>>>> From: Gene Chen <gene_chen@richtek.com> > >>>>>>> > >>>>>>> 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 <gene_chen@richtek.com> > >>>>>>> --- > >>>>>>> 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 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. Maybe it is more specific to send a new patch? Sample DT as below LED "red" is simple led class device, LED "green&blue" is multicolor devices. led@0 { reg = <0>; function = LED_FUNCTION_INDICATOR; color = <LED_COLOR_ID_RED>; led-max-microamp = <24000>; }; led@6 { reg = <6>; function = LED_FUNCTION_INDICATOR; color = <LED_COLOR_ID_MULTI>; led@1 { reg = <1>; function = LED_FUNCTION_INDICATOR; color = <LED_COLOR_ID_GREEN>; led-max-microamp = <24000>; }; led@2 { reg = <2>; function = LED_FUNCTION_INDICATOR; color = <LED_COLOR_ID_BLUE>; led-max-microamp = <24000>; }; }; > > > >> 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 > LEDS_MT6360 depends on LEDS_CLASS_FLASH, and LEDS_CLASS_FLASH depends on LEDS_CLASS Is "depends on LEDS_CLASS" still needed? > > > > and source code add constraint > > > > #if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) > > ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb, > > init_data); > > #endif > > > > #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > > ret = devm_led_classdev_flash_register_ext(parent, &led->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. > ACK, I will submit a fixed patch about leds-class-flash.h. By the way, if CONFIG_LED_CLASS_MULTICOLOR is not enabled and we don't use #if IS_ENABLED, according to led-class-multicolor.h return -EINVAL, we will register multicolor device fail and cause probe fail. > > ============= > > > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-11-16 10:01 ` Gene Chen @ 2020-11-16 18:25 ` Jacek Anaszewski 2020-11-17 9:54 ` Gene Chen 0 siblings, 1 reply; 16+ messages in thread From: Jacek Anaszewski @ 2020-11-16 18:25 UTC (permalink / raw) To: Gene Chen Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao On 11/16/20 11:01 AM, Gene Chen wrote: > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月31日 週六 上午6:34寫道： >> >> On 10/30/20 9:51 AM, Gene Chen wrote: >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道： >>>> >>>> On 10/27/20 10:28 AM, Gene Chen wrote: >>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： >>>>>> >>>>>> On 10/20/20 8:44 AM, Gene Chen wrote: >>>>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： >>>>>>>> >>>>>>>> Hi Gene, >>>>>>>> >>>>>>>> On 10/7/20 3:42 AM, Gene Chen wrote: >>>>>>>>> From: Gene Chen <gene_chen@richtek.com> >>>>>>>>> >>>>>>>>> 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 <gene_chen@richtek.com> >>>>>>>>> --- >>>>>>>>> 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&blue" is multicolor devices. > led@0 { > reg = <0>; > function = LED_FUNCTION_INDICATOR; > color = <LED_COLOR_ID_RED>; > led-max-microamp = <24000>; > }; > led@6 { > reg = <6>; > function = LED_FUNCTION_INDICATOR; > color = <LED_COLOR_ID_MULTI>; > > led@1 { > reg = <1>; > function = LED_FUNCTION_INDICATOR; > color = <LED_COLOR_ID_GREEN>; > led-max-microamp = <24000>; > }; > led@2 { > reg = <2>; > function = LED_FUNCTION_INDICATOR; > color = <LED_COLOR_ID_BLUE>; > 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 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 >> > > LEDS_MT6360 depends on LEDS_CLASS_FLASH, and LEDS_CLASS_FLASH depends > on LEDS_CLASS > Is "depends on LEDS_CLASS" still needed? Yes, because you should allow disabling CONFIG_LEDS_CLASS_FLASH. In such a case driver should still compile and register ML LED class device when it has corresponding DT node. >>> and source code add constraint >>> >>> #if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) >>> ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb, >>> init_data); >>> #endif >>> >>> #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) >>> ret = devm_led_classdev_flash_register_ext(parent, &led->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. >> > > ACK, I will submit a fixed patch about leds-class-flash.h. > > By the way, if CONFIG_LED_CLASS_MULTICOLOR is not enabled and we don't > use #if IS_ENABLED, > according to led-class-multicolor.h return -EINVAL, > we will register multicolor device fail and cause probe fail. Good point. So led-class-multicolor.h no-ops need to be fixed to return 0 instead of -EINVAL. And no-ops in include/linux/led-class-flash.h should also return 0. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 2020-11-16 18:25 ` Jacek Anaszewski @ 2020-11-17 9:54 ` Gene Chen 0 siblings, 0 replies; 16+ messages in thread From: Gene Chen @ 2020-11-17 9:54 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Rob Herring, Matthias Brugger, Dan Murphy, Linux LED Subsystem, devicetree, linux-arm Mailing List, moderated list:ARM/Mediatek SoC support, Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年11月17日 週二 上午2:25寫道： > > On 11/16/20 11:01 AM, Gene Chen wrote: > > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月31日 週六 上午6:34寫道： > >> > >> On 10/30/20 9:51 AM, Gene Chen wrote: > >>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道： > >>>> > >>>> On 10/27/20 10:28 AM, Gene Chen wrote: > >>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道： > >>>>>> > >>>>>> On 10/20/20 8:44 AM, Gene Chen wrote: > >>>>>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道： > >>>>>>>> > >>>>>>>> Hi Gene, > >>>>>>>> > >>>>>>>> On 10/7/20 3:42 AM, Gene Chen wrote: > >>>>>>>>> From: Gene Chen <gene_chen@richtek.com> > >>>>>>>>> > >>>>>>>>> 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 <gene_chen@richtek.com> > >>>>>>>>> --- > >>>>>>>>> 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 > ACK > > 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&blue" is multicolor devices. > > led@0 { > > reg = <0>; > > function = LED_FUNCTION_INDICATOR; > > color = <LED_COLOR_ID_RED>; > > led-max-microamp = <24000>; > > }; > > led@6 { > > reg = <6>; > > function = LED_FUNCTION_INDICATOR; > > color = <LED_COLOR_ID_MULTI>; > > > > led@1 { > > reg = <1>; > > function = LED_FUNCTION_INDICATOR; > > color = <LED_COLOR_ID_GREEN>; > > led-max-microamp = <24000>; > > }; > > led@2 { > > reg = <2>; > > function = LED_FUNCTION_INDICATOR; > > color = <LED_COLOR_ID_BLUE>; > > 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 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 > >> > > > > LEDS_MT6360 depends on LEDS_CLASS_FLASH, and LEDS_CLASS_FLASH depends > > on LEDS_CLASS > > Is "depends on LEDS_CLASS" still needed? > > Yes, because you should allow disabling CONFIG_LEDS_CLASS_FLASH. > In such a case driver should still compile and register ML LED class > device when it has corresponding DT node. > ACK > >>> and source code add constraint > >>> > >>> #if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) > >>> ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb, > >>> init_data); > >>> #endif > >>> > >>> #if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > >>> ret = devm_led_classdev_flash_register_ext(parent, &led->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. > >> > > > > ACK, I will submit a fixed patch about leds-class-flash.h. > > > > By the way, if CONFIG_LED_CLASS_MULTICOLOR is not enabled and we don't > > use #if IS_ENABLED, > > according to led-class-multicolor.h return -EINVAL, > > we will register multicolor device fail and cause probe fail. > > Good point. So led-class-multicolor.h no-ops need to be fixed to return > 0 instead of -EINVAL. > > And no-ops in include/linux/led-class-flash.h should also return 0. DT node is first priority to decide how leds work. If RGB LED use multicolor form in DT, CONFIG_LEDS_CLASS_FLASH should be defined. Otherwise It is right action to probe fail. > > -- > Best regards, > Jacek Anaszewski ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-11-17 9:55 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-07 1:42 [PATCH v5 0/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 2020-10-07 1:42 ` [PATCH v5 1/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen 2020-10-07 13:50 ` Rob Herring 2020-10-08 21:42 ` Jacek Anaszewski 2020-10-07 1:42 ` [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360 Gene Chen 2020-10-07 5:11 ` kernel test robot 2020-10-08 21:51 ` Jacek Anaszewski 2020-10-20 6:44 ` Gene Chen 2020-10-20 21:46 ` Jacek Anaszewski 2020-10-27 9:28 ` Gene Chen 2020-10-27 17:28 ` Jacek Anaszewski 2020-10-30 8:51 ` Gene Chen 2020-10-30 22:34 ` Jacek Anaszewski 2020-11-16 10:01 ` Gene Chen 2020-11-16 18:25 ` Jacek Anaszewski 2020-11-17 9:54 ` Gene Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).