Re: [PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
flat view
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>
Date: 2022-08-30 08:49:54
Also in:
dri-devel, linux-arm-kernel, linux-devicetree, linux-iio, linux-leds, linux-mediatek, linux-pm, lkml
Il 30/08/22 05:40, ChiaEn Wu ha scritto:
quoted hunk ↗ jump to hunk
From: Alice Chen <redacted> The MediaTek MT6370 is a highly-integrated smart power management IC, which includes a single cell Li-Ion/Li-Polymer switching battery charger, a USB Type-C & Power Delivery (PD) controller, dual Flash LED current sources, a RGB LED driver, a backlight WLED driver, a display bias driver and a general LDO for portable devices. Add support for the MT6370 Flash LED driver. Flash LED in MT6370 has 2 channels and support torch/strobe mode. Signed-off-by: Alice Chen <redacted> Signed-off-by: ChiaEn Wu <redacted> --- v9 - Revise the format of the comments. --- drivers/leds/flash/Kconfig | 14 + drivers/leds/flash/Makefile | 1 + drivers/leds/flash/leds-mt6370-flash.c | 632 +++++++++++++++++++++++++++++++++ 3 files changed, 647 insertions(+) create mode 100644 drivers/leds/flash/leds-mt6370-flash.cdiff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig index d3eb689..405bd16 100644 --- a/drivers/leds/flash/Kconfig +++ b/drivers/leds/flash/Kconfig@@ -61,6 +61,20 @@ config LEDS_MT6360 Independent current sources supply for each flash LED support torch and strobe mode. +config LEDS_MT6370_FLASH + tristate "Flash LED Support for MediaTek MT6370 PMIC" + depends on LEDS_CLASS && OF + depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS + depends on MFD_MT6370 + help + Support 2 channels and torch/strobe mode. + Say Y here to enable support for + MT6370_FLASH_LED device. + + This driver can also be built as a module. If so, the module + will be called "leds-mt6370-flash". + config LEDS_RT4505 tristate "LED support for RT4505 flashlight controller" depends on I2C && OFdiff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile index 0acbddc..0c1f3c5 100644 --- a/drivers/leds/flash/Makefile +++ b/drivers/leds/flash/Makefile@@ -9,3 +9,4 @@ obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o obj-$(CONFIG_LEDS_RT4505) += leds-rt4505.o obj-$(CONFIG_LEDS_RT8515) += leds-rt8515.o obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o +obj-$(CONFIG_LEDS_MT6370_FLASH) += leds-mt6370-flash.odiff --git a/drivers/leds/flash/leds-mt6370-flash.c b/drivers/leds/flash/leds-mt6370-flash.c new file mode 100644 index 0000000..a8ec7e1 --- /dev/null +++ b/drivers/leds/flash/leds-mt6370-flash.c@@ -0,0 +1,632 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Richtek Technology Corp. + * + * Author: Alice Chen <alice_chen@richtek.com> + */ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/led-class-flash.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 { + MT6370_LED_FLASH1, + MT6370_LED_FLASH2, + MT6370_MAX_LEDS +}; + +/* Virtual definition for multicolor */ + +#define MT6370_REG_FLEDEN 0x17E +#define MT6370_REG_STRBTO 0x173 +#define MT6370_REG_CHGSTAT2 0x1D1 +#define MT6370_REG_FLEDSTAT1 0x1D9 +#define MT6370_REG_FLEDISTRB(_id) (0x174 + 4 * (_id)) +#define MT6370_REG_FLEDITOR(_id) (0x175 + 4 * (_id)) +#define MT6370_ITORCH_MASK GENMASK(4, 0) +#define MT6370_ISTROBE_MASK GENMASK(6, 0) +#define MT6370_STRBTO_MASK GENMASK(6, 0) +#define MT6370_TORCHEN_MASK BIT(3) +#define MT6370_STROBEN_MASK BIT(2) +#define MT6370_FLCSEN_MASK(_id) BIT(MT6370_LED_FLASH2 - (_id)) +#define MT6370_FLCSEN_MASK_ALL GENMASK(1, 0) +#define MT6370_FLEDCHGVINOVP_MASK BIT(3) +#define MT6370_FLED1STRBTO_MASK BIT(11) +#define MT6370_FLED2STRBTO_MASK BIT(10) +#define MT6370_FLED1STRB_MASK BIT(9) +#define MT6370_FLED2STRB_MASK BIT(8) +#define MT6370_FLED1SHORT_MASK BIT(7) +#define MT6370_FLED2SHORT_MASK BIT(6) +#define MT6370_FLEDLVF_MASK BIT(3) + +#define MT6370_LED_JOINT 2 +#define MT6370_RANGE_FLED_REG 4 +#define MT6370_ITORCH_MIN_uA 25000 +#define MT6370_ITORCH_STEP_uA 12500 +#define MT6370_ITORCH_MAX_uA 400000 +#define MT6370_ITORCH_DOUBLE_MAX_uA 800000 +#define MT6370_ISTRB_MIN_uA 50000 +#define MT6370_ISTRB_STEP_uA 12500 +#define MT6370_ISTRB_MAX_uA 1500000 +#define MT6370_ISTRB_DOUBLE_MAX_uA 3000000 +#define MT6370_STRBTO_MIN_US 64000 +#define MT6370_STRBTO_STEP_US 32000 +#define MT6370_STRBTO_MAX_US 2432000 + +#define STATE_OFF 0 +#define STATE_KEEP 1 +#define STATE_ON 2 + +#define to_mt6370_led(ptr, member) container_of(ptr, struct mt6370_led, member) + +struct mt6370_led { + struct led_classdev_flash flash; + struct v4l2_flash *v4l2_flash; + struct mt6370_priv *priv; + u32 led_no;
The maximum value that we will ever see for `led_no` is 2: this means that we don't need 32 full bits here. u8 led_no;
+ u32 default_state;
This should be... enum led_default_state default_state; ...but then, you're never using this variable as far as I can see, apart from initializing it in mt6370_init_common_properties, for which, there's a comment below. In any case, please either fix the initialization and actually make use of this variable, or remove it entirely.
quoted hunk ↗ jump to hunk
+}; + +struct mt6370_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 mt6370_led leds[]; +}; + +static int mt6370_torch_brightness_set(struct led_classdev *lcdev, + enum led_brightness level) +{ + struct mt6370_led *led = to_mt6370_led(lcdev, flash.led_cdev); + struct mt6370_priv *priv = led->priv; + u32 led_enable_mask = (led->led_no == MT6370_LED_JOINT) ? + MT6370_FLCSEN_MASK_ALL : + MT6370_FLCSEN_MASK(led->led_no); + u32 enable_mask = MT6370_TORCHEN_MASK | led_enable_mask; + u32 val = level ? led_enable_mask : 0; + u32 curr; + int ret, i; + + mutex_lock(&priv->lock); + + /* + * There is only one set of flash control logic, and this + * flag is used to check if 'strobe' is currently being used. + */ + if (priv->fled_strobe_used) { + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", + priv->fled_strobe_used); + ret = -EBUSY; + goto unlock; + } + + if (level) + curr = priv->fled_torch_used | BIT(led->led_no); + else + curr = priv->fled_torch_used & ~BIT(led->led_no); + + if (curr) + val |= MT6370_TORCHEN_MASK; + + if (level) { + level -= 1; + if (led->led_no == MT6370_LED_JOINT) { + int flevel[MT6370_MAX_LEDS]; + + flevel[0] = level / 2; + flevel[1] = level - flevel[0]; + for (i = 0; i < MT6370_MAX_LEDS; i++) { + ret = regmap_update_bits(priv->regmap, + MT6370_REG_FLEDITOR(i), + MT6370_ITORCH_MASK, flevel[i]); + if (ret) + goto unlock; + } + } else { + ret = regmap_update_bits(priv->regmap, + MT6370_REG_FLEDITOR(led->led_no), + MT6370_ITORCH_MASK, level); + if (ret) + goto unlock; + } + } + + ret = regmap_update_bits(priv->regmap, MT6370_REG_FLEDEN, + enable_mask, val); + if (ret) + goto unlock; + + priv->fled_torch_used = curr; + +unlock: + mutex_unlock(&priv->lock); + return ret; +} + +static int mt6370_flash_brightness_set(struct led_classdev_flash *fl_cdev, + u32 brightness) +{ + /* + * Because of the current spikes when turning on the flash, + * the brightness should be kept by the LED framework. This + * empty function is used to prevent checking failure when + * led_classdev_flash registers ops. + */ + return 0; +} + +static int _mt6370_flash_brightness_set(struct led_classdev_flash *fl_cdev, + u32 brightness) +{ + struct mt6370_led *led = to_mt6370_led(fl_cdev, flash); + struct mt6370_priv *priv = led->priv; + struct led_flash_setting *s = &fl_cdev->brightness; + u32 val = (brightness - s->min) / s->step; + int ret, i; + + if (led->led_no == MT6370_LED_JOINT) { + int flevel[MT6370_MAX_LEDS];
Since you're writing this value to registers, please use fixed size variables. If this is really supposed to be a signed value, it would be: s32 flevel[MT6370_MAX_LEDS];
quoted hunk ↗ jump to hunk
+ + flevel[0] = val / 2; + flevel[1] = val - flevel[0]; + for (i = 0; i < MT6370_MAX_LEDS; i++) { + ret = regmap_update_bits(priv->regmap, + MT6370_REG_FLEDISTRB(i), + MT6370_ISTROBE_MASK, flevel[i]); + if (ret) + return ret; + } + } else { + return regmap_update_bits(priv->regmap, + MT6370_REG_FLEDISTRB(led->led_no), + MT6370_ISTROBE_MASK, val); + } + + return 0;
If we reach this point, this means that led_no is out of range (which is never supposed to happen - I know). Since that condition is something unexpected and unhandled, you should return an error here.
quoted hunk ↗ jump to hunk
+} + +static int mt6370_strobe_set(struct led_classdev_flash *fl_cdev, bool state) +{ + struct mt6370_led *led = to_mt6370_led(fl_cdev, flash); + struct mt6370_priv *priv = led->priv; + struct led_classdev *lcdev = &fl_cdev->led_cdev; + struct led_flash_setting *s = &fl_cdev->brightness; + u32 led_enable_mask = (led->led_no == MT6370_LED_JOINT) ? + MT6370_FLCSEN_MASK_ALL : + MT6370_FLCSEN_MASK(led->led_no); + u32 enable_mask = MT6370_STROBEN_MASK | led_enable_mask; + u32 val = state ? led_enable_mask : 0; + u32 curr; + int ret; + + mutex_lock(&priv->lock); + + /* + * There is only one set of flash control logic, and this + * flag is used to check if 'torch' is currently being used. + */ + if (priv->fled_torch_used) { + dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", + priv->fled_torch_used); + ret = -EBUSY; + goto unlock; + } + + if (state) + curr = priv->fled_strobe_used | BIT(led->led_no); + else + curr = priv->fled_strobe_used & ~BIT(led->led_no); + + if (curr) + val |= MT6370_STROBEN_MASK; + + ret = regmap_update_bits(priv->regmap, MT6370_REG_FLEDEN, enable_mask, + val); + if (ret) { + dev_err(lcdev->dev, "[%d] control current source %d fail\n", + led->led_no, state); + goto unlock; + } + + /* + * If the flash needs to turn on, configure the flash current to + * ramp up to the setting value. Otherwise, always revert to the + * minimum one. + */ + ret = _mt6370_flash_brightness_set(fl_cdev, state ? s->val : s->min); + if (ret) + goto unlock;
If this fails, I think that we should revert the previous changes to FLEDEN before returning.
quoted hunk ↗ jump to hunk
+ + /* + * For the flash to turn on/off, we must wait for HW ramping + * up/down time 5ms/500us to prevent the unexpected problem. + */ + if (!priv->fled_strobe_used && curr) + usleep_range(5000, 6000); + else if (priv->fled_strobe_used && !curr) + usleep_range(500, 600); + + priv->fled_strobe_used = curr; + +unlock: + mutex_unlock(&priv->lock); + return ret; +} +
..snip..
quoted hunk ↗ jump to hunk
+ +static int mt6370_init_common_properties(struct mt6370_led *led, + struct led_init_data *init_data) +{ + const char * const states[] = { "off", "keep", "on" }; + const char *str = states[STATE_OFF]; + int ret; + + fwnode_property_read_string(init_data->fwnode, "default-state", &str); + ret = match_string(states, ARRAY_SIZE(states), str); + if (ret >= 0) + led->default_state = ret; + + return 0; +} + +static int mt6370_led_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct mt6370_priv *priv; + struct fwnode_handle *child; + size_t count; + int i = 0, ret; + + count = device_get_child_node_count(dev); + if (!count || count > MT6370_MAX_LEDS) + return dev_err_probe(dev, -EINVAL, + "No child node or node count over max led number %zu\n", count); + + priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->leds_count = count; + priv->dev = dev; + mutex_init(&priv->lock); + + priv->regmap = dev_get_regmap(dev->parent, NULL); + if (!priv->regmap) + return dev_err_probe(dev, -ENODEV, "Failed to get parent regmap\n"); + + device_for_each_child_node(dev, child) { + struct mt6370_led *led = priv->leds + i; + struct led_init_data init_data = { .fwnode = child, }; + + led->priv = priv; + ret = mt6370_init_common_properties(led, &init_data);
This is only initializing the default state, for which you do have a helper. Please use led_init_default_state_get(fwnode).
quoted hunk ↗ jump to hunk
+ if (ret) + return ret; + + ret = mt6370_init_flash_properties(led, &init_data); + if (ret) + return ret; + + ret = mt6370_led_register(dev, led, &init_data); + if (ret) + return ret; + + i++; + } + + return 0; +} +
Regards, Angelo