Re: [PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support

From: AngeloGioacchino Del Regno
Date: Tue Aug 30 2022 - 04:49:54 EST

Next message: Ilpo Järvinen: "[PATCH v2 1/5] serial: ucc_uart: Remove custom frame size calculation"
Previous message: Greg Kroah-Hartman: "Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space due to .padding"
In reply to: ChiaEn Wu: "[PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support"
Next in thread: ChiaEn Wu: "[PATCH v9 10/10] video: backlight: mt6370: Add MediaTek MT6370 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Il 30/08/22 05:40, ChiaEn Wu ha scritto:

From: Alice Chen <alice_chen@xxxxxxxxxxx>

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 <alice_chen@xxxxxxxxxxx>
Signed-off-by: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>
---

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.c

diff --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 && OF
diff --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.o
diff --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@xxxxxxxxxxx>
+ */
+
+#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.

+};
+
+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];

+
+ 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.

+}
+
+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.

+
+ /*
+ * 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..

+
+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).

+ 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

Next message: Ilpo Järvinen: "[PATCH v2 1/5] serial: ucc_uart: Remove custom frame size calculation"
Previous message: Greg Kroah-Hartman: "Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space due to .padding"
In reply to: ChiaEn Wu: "[PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support"
Next in thread: ChiaEn Wu: "[PATCH v9 10/10] video: backlight: mt6370: Add MediaTek MT6370 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]