[PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add support for RT4505 flash led controller. It can support up to 1.5A
flash current with hardware timeout and low input voltage protection.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/leds/Kconfig       |  11 ++
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 409 insertions(+)
 create mode 100644 drivers/leds/leds-rt4505.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 849d3c5..71ebd1d 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -717,6 +717,17 @@ config LEDS_MAX8997
 	  This option enables support for on-chip LED drivers on
 	  MAXIM MAX8997 PMIC.
 
+config LEDS_RT4505
+	tristate "LED driver for RT4505 single channel flash led controller"
+	depends on I2C
+	depends on LEDS_CLASS_FLASH && OF
+	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+	select REGMAP_I2C
+	help
+	  This option enables support for the RT4505 flash led controller.
+	  It can support up to 1.5A flash strobe current with hardware timeout
+	  and low input voltage protection.
+
 config LEDS_LM355x
 	tristate "LED support for LM3554 and LM3556 chips"
 	depends on LEDS_CLASS && I2C
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 73e603e..1aca2b6 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
 obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
+obj-$(CONFIG_LEDS_RT4505)		+= leds-rt4505.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_SGM3140)		+= leds-sgm3140.o
diff --git a/drivers/leds/leds-rt4505.c b/drivers/leds/leds-rt4505.c
new file mode 100644
index 00000000..46d2c030
--- /dev/null
+++ b/drivers/leds/leds-rt4505.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <media/v4l2-flash-led-class.h>
+
+#define RT4505_REG_RESET	0x0
+#define RT4505_REG_CONFIG	0x8
+#define RT4505_REG_ILED		0x9
+#define RT4505_REG_ENABLE	0xA
+#define RT4505_REG_FLAGS	0xB
+
+#define RT4505_RESET_MASK	BIT(7)
+#define RT4505_FLASHTO_MASK	GENMASK(2, 0)
+#define RT4505_ITORCH_MASK	GENMASK(7, 5)
+#define RT4505_ITORCH_SHIFT	5
+#define RT4505_IFLASH_MASK	GENMASK(4, 0)
+#define RT4505_ENABLE_MASK	GENMASK(5, 0)
+#define RT4505_TORCH_SET	(BIT(0) | BIT(4))
+#define RT4505_FLASH_SET	(BIT(0) | BIT(1) | BIT(2) | BIT(4))
+#define RT4505_EXT_FLASH_SET	(BIT(0) | BIT(1) | BIT(4) | BIT(5))
+#define RT4505_FLASH_GET	(BIT(0) | BIT(1) | BIT(4))
+#define RT4505_OVP_MASK		BIT(3)
+#define RT4505_SHORT_MASK	BIT(2)
+#define RT4505_OTP_MASK		BIT(1)
+#define RT4505_TIMEOUT_MASK	BIT(0)
+
+#define RT4505_ITORCH_MINUA	46000
+#define RT4505_ITORCH_MAXUA	375000
+#define RT4505_ITORCH_STPUA	47000
+#define RT4505_IFLASH_MINUA	93750
+#define RT4505_IFLASH_MAXUA	1500000
+#define RT4505_IFLASH_STPUA	93750
+#define RT4505_FLASHTO_MINUS	100000
+#define RT4505_FLASHTO_MAXUS	800000
+#define RT4505_FLASHTO_STPUS	100000
+
+struct rt4505_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex lock;
+	struct led_classdev_flash flash;
+	struct v4l2_flash *v4l2_flash;
+};
+
+static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
+{
+	struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
+	u32 val = 0;
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	if (level != LED_OFF) {
+		ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
+					 (level - 1) << RT4505_ITORCH_SHIFT);
+		if (ret)
+			goto unlock;
+
+		val = RT4505_TORCH_SET;
+	}
+
+	ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
+
+unlock:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static enum led_brightness rt4505_torch_brightness_get(struct led_classdev *lcdev)
+{
+	struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
+	u32 val = 0;
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
+	if (ret) {
+		dev_err(lcdev->dev, "Failed to get LED enable\n");
+		ret = LED_OFF;
+		goto unlock;
+	}
+
+	if ((val & RT4505_ENABLE_MASK) != RT4505_TORCH_SET) {
+		ret = LED_OFF;
+		goto unlock;
+	}
+
+	ret = regmap_read(priv->regmap, RT4505_REG_ILED, &val);
+	if (ret) {
+		dev_err(lcdev->dev, "Failed to get LED brightness\n");
+		ret = LED_OFF;
+		goto unlock;
+	}
+
+	ret = ((val & RT4505_ITORCH_MASK) >> RT4505_ITORCH_SHIFT) + 1;
+
+unlock:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static int rt4505_flash_brightness_set(struct led_classdev_flash *fled_cdev, u32 brightness)
+{
+	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
+	struct led_flash_setting *s = &fled_cdev->brightness;
+	u32 val = (brightness - s->min) / s->step;
+	int ret;
+
+	mutex_lock(&priv->lock);
+	ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_IFLASH_MASK, val);
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int rt4505_flash_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
+{
+	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
+	u32 val = state ? RT4505_FLASH_SET : 0;
+	int ret;
+
+	mutex_lock(&priv->lock);
+	ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
+{
+	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
+	u32 val;
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
+	if (ret)
+		goto unlock;
+
+	*state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
+
+unlock:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static int rt4505_flash_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout)
+{
+	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
+	struct led_flash_setting *s = &fled_cdev->timeout;
+	u32 val = (timeout - s->min) / s->step;
+	int ret;
+
+	mutex_lock(&priv->lock);
+	ret = regmap_update_bits(priv->regmap, RT4505_REG_CONFIG, RT4505_FLASHTO_MASK, val);
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int rt4505_fault_get(struct led_classdev_flash *fled_cdev, u32 *fault)
+{
+	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
+	u32 val, led_faults = 0;
+	int ret;
+
+	ret = regmap_read(priv->regmap, RT4505_REG_FLAGS, &val);
+	if (ret)
+		return ret;
+
+	if (val & RT4505_OVP_MASK)
+		led_faults |= LED_FAULT_OVER_VOLTAGE;
+
+	if (val & RT4505_SHORT_MASK)
+		led_faults |= LED_FAULT_SHORT_CIRCUIT;
+
+	if (val & RT4505_OTP_MASK)
+		led_faults |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (val & RT4505_TIMEOUT_MASK)
+		led_faults |= LED_FAULT_TIMEOUT;
+
+	*fault = led_faults;
+	return 0;
+}
+
+static const struct led_flash_ops rt4505_flash_ops = {
+	.flash_brightness_set = rt4505_flash_brightness_set,
+	.strobe_set = rt4505_flash_strobe_set,
+	.strobe_get = rt4505_flash_strobe_get,
+	.timeout_set = rt4505_flash_timeout_set,
+	.fault_get = rt4505_fault_get,
+};
+
+static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
+{
+	if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
+		return true;
+	return false;
+}
+
+static const struct regmap_config rt4505_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RT4505_REG_FLAGS,
+
+	.readable_reg = rt4505_is_accessible_reg,
+	.writeable_reg = rt4505_is_accessible_reg,
+};
+
+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
+static int rt4505_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
+{
+	struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
+	struct rt4505_priv *priv = container_of(flash, struct rt4505_priv, flash);
+	u32 val = enable ? RT4505_EXT_FLASH_SET : 0;
+	int ret;
+
+	mutex_lock(&priv->lock);
+	ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static const struct v4l2_flash_ops v4l2_flash_ops = {
+	.external_strobe_set = rt4505_flash_external_strobe_set,
+};
+
+static void rt4505_init_v4l2_config(struct rt4505_priv *priv, struct v4l2_flash_config *config)
+{
+	struct led_classdev_flash *flash = &priv->flash;
+	struct led_classdev *lcdev = &flash->led_cdev;
+	struct led_flash_setting *s;
+
+	strscpy(config->dev_name, lcdev->dev->kobj.name, sizeof(config->dev_name));
+
+	s = &config->intensity;
+	s->min = RT4505_ITORCH_MINUA;
+	s->step = RT4505_ITORCH_STPUA;
+	s->max = s->val = s->min + (lcdev->max_brightness - 1) * s->step;
+
+	config->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_SHORT_CIRCUIT |
+			       LED_FAULT_LED_OVER_TEMPERATURE | LED_FAULT_TIMEOUT;
+	config->has_external_strobe = 1;
+}
+#else
+static const struct v4l2_flash_ops v4l2_flash_ops;
+static void rt4505_init_v4l2_config(struct rt4505_priv *priv, struct v4l2_flash_config *config)
+{
+}
+#endif
+
+static void rt4505_init_flash_properties(struct rt4505_priv *priv, struct fwnode_handle *child)
+{
+	struct led_classdev_flash *flash = &priv->flash;
+	struct led_classdev *lcdev = &flash->led_cdev;
+	struct led_flash_setting *s;
+	u32 val;
+	int ret;
+
+	ret = fwnode_property_read_u32(child, "led-max-microamp", &val);
+	if (ret) {
+		dev_warn(priv->dev, "led-max-microamp DT property missing\n");
+		val = RT4505_ITORCH_MINUA;
+	} else
+		val = clamp_val(val, RT4505_ITORCH_MINUA, RT4505_ITORCH_MAXUA);
+
+	lcdev->max_brightness = (val - RT4505_ITORCH_MINUA) / RT4505_ITORCH_STPUA + 1;
+	lcdev->brightness_set_blocking = rt4505_torch_brightness_set;
+	lcdev->brightness_get = rt4505_torch_brightness_get;
+	lcdev->flags |= LED_DEV_CAP_FLASH;
+
+	ret = fwnode_property_read_u32(child, "flash-max-microamp", &val);
+	if (ret) {
+		dev_warn(priv->dev, "flash-max-microamp DT property missing\n");
+		val = RT4505_IFLASH_MINUA;
+	} else
+		val = clamp_val(val, RT4505_IFLASH_MINUA, RT4505_IFLASH_MAXUA);
+
+	s = &flash->brightness;
+	s->min = RT4505_IFLASH_MINUA;
+	s->step = RT4505_IFLASH_STPUA;
+	s->max = s->val = val;
+
+	ret = fwnode_property_read_u32(child, "flash-max-timeout-us", &val);
+	if (ret) {
+		dev_warn(priv->dev, "flash-max-timeout-us DT property missing\n");
+		val = RT4505_FLASHTO_MINUS;
+	} else
+		val = clamp_val(val, RT4505_FLASHTO_MINUS, RT4505_FLASHTO_MAXUS);
+
+	s = &flash->timeout;
+	s->min = RT4505_FLASHTO_MINUS;
+	s->step = RT4505_FLASHTO_STPUS;
+	s->max = s->val = val;
+
+	flash->ops = &rt4505_flash_ops;
+}
+
+static int rt4505_probe(struct i2c_client *client)
+{
+	struct rt4505_priv *priv;
+	struct fwnode_handle *child;
+	struct led_init_data init_data = {};
+	struct v4l2_flash_config v4l2_config = {};
+	int ret;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &client->dev;
+	mutex_init(&priv->lock);
+
+	priv->regmap = devm_regmap_init_i2c(client, &rt4505_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		dev_err(priv->dev, "Failed to allocate register map\n");
+		return PTR_ERR(priv->regmap);
+	}
+
+	ret = regmap_write(priv->regmap, RT4505_REG_RESET, RT4505_RESET_MASK);
+	if (ret) {
+		dev_err(priv->dev, "Failed to reset registers\n");
+		return ret;
+	}
+
+	child = fwnode_get_next_available_child_node(client->dev.fwnode, NULL);
+	if (!child) {
+		dev_err(priv->dev, "Failed to get child node\n");
+		return -EINVAL;
+	}
+	init_data.fwnode = child;
+
+	rt4505_init_flash_properties(priv, child);
+	ret = devm_led_classdev_flash_register_ext(priv->dev, &priv->flash, &init_data);
+	if (ret) {
+		dev_err(priv->dev, "Failed to register flash\n");
+		return ret;
+	}
+
+	rt4505_init_v4l2_config(priv, &v4l2_config);
+	priv->v4l2_flash = v4l2_flash_init(priv->dev, init_data.fwnode, &priv->flash,
+					   &v4l2_flash_ops, &v4l2_config);
+	if (IS_ERR(priv->v4l2_flash)) {
+		dev_err(priv->dev, "Failed to register v4l2 flash\n");
+		return PTR_ERR(priv->v4l2_flash);
+	}
+
+	i2c_set_clientdata(client, priv);
+	return 0;
+}
+
+static int rt4505_remove(struct i2c_client *client)
+{
+	struct rt4505_priv *priv = i2c_get_clientdata(client);
+
+	v4l2_flash_release(priv->v4l2_flash);
+	return 0;
+}
+
+static void rt4505_shutdown(struct i2c_client *client)
+{
+	struct rt4505_priv *priv = i2c_get_clientdata(client);
+
+	/* Reset registers to make sure all off before shutdown */
+	regmap_write(priv->regmap, RT4505_REG_RESET, RT4505_RESET_MASK);
+}
+
+static const struct of_device_id __maybe_unused rt4505_leds_match[] = {
+	{ .compatible = "richtek,rt4505", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rt4505_leds_match);
+
+static struct i2c_driver rt4505_driver = {
+	.driver = {
+		.name = "rt4505",
+		.of_match_table = of_match_ptr(rt4505_leds_match),
+	},
+	.probe_new = rt4505_probe,
+	.remove = rt4505_remove,
+	.shutdown = rt4505_shutdown,
+};
+module_i2c_driver(rt4505_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v1 2/2] leds: rt4505: Add DT binding document for Richtek RT4505

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add DT binding document for Richtek RT4505 flash led controller.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 .../devicetree/bindings/leds/leds-rt4505.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-rt4505.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-rt4505.yaml b/Documentation/devicetree/bindings/leds/leds-rt4505.yaml
new file mode 100644
index 00000000..da1681a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-rt4505.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-rt4505.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT4505 Single Channel LED Driver
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  The RT4505 is a flash led driver that can support up to 375mA and 1.5A for
+  torch and flash mode, respectively.
+
+  The data sheet can be found at:
+    https://www.richtek.com/assets/product_file/RT4505/DS4505-02.pdf
+
+properties:
+  compatible:
+    const: richtek,rt4505
+
+  reg:
+    description: I2C slave address of the controller.
+    maxItems: 1
+
+  led:
+    type: object
+    $ref: common.yaml#
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      led-controller@63 {
+        compatible = "richtek,rt4505";
+        reg = <0x63>;
+
+        rt4505_flash: led {
+          function = LED_FUNCTION_FLASH;
+          color = <LED_COLOR_ID_WHITE>;
+          led-max-microamp = <375000>;
+          flash-max-microamp = <1500000>;
+          flash-max-timeout-us = <800000>;
+        };
+      };
+    };
-- 
2.7.4

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

Hi!

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add support for RT4505 flash led controller. It can support up to 1.5A
> flash current with hardware timeout and low input voltage
> protection.

Please use upper-case "LED" everywhere.

This should be 2nd in the series, after DT changes.

> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/leds/Kconfig       |  11 ++
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 409 insertions(+)
>  create mode 100644 drivers/leds/leds-rt4505.c

Lets put this into drivers/leds/flash/. (Yes, you'll have to create
it).


> +	help
> +	  This option enables support for the RT4505 flash led controller.

Information where it is used would be welcome here.

> +	  It can support up to 1.5A flash strobe current with hardware timeout
> +	  and low input voltage protection.

This does not / should not be here.
> +
> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{

80 columns, where easy.

> +	struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> +	u32 val = 0;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (level != LED_OFF) {
> +		ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> +					 (level - 1) << RT4505_ITORCH_SHIFT);
> +		if (ret)
> +			goto unlock;
> +
> +		val = RT4505_TORCH_SET;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}

Why is the locking needed? What will the /sys/class/leds interface
look like on system with your flash?

> +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> +{
> +	struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
> +	u32 val;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> +	if (ret)
> +		goto unlock;
> +
> +	*state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;

No need for ? ... part.

> +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> +		return true;

Make this two stagements.

Otherwise... looks like easy simple driver, thanks.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Pavel Machek <pavel@ucw.cz> 於 2020年10月27日 週二 下午4:29寫道：
>
> Hi!
>
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add support for RT4505 flash led controller. It can support up to 1.5A
> > flash current with hardware timeout and low input voltage
> > protection.
>
> Please use upper-case "LED" everywhere.
>
> This should be 2nd in the series, after DT changes.
Sure, will ack in next series patch.
>
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  drivers/leds/Kconfig       |  11 ++
> >  drivers/leds/Makefile      |   1 +
> >  drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 409 insertions(+)
> >  create mode 100644 drivers/leds/leds-rt4505.c
>
> Lets put this into drivers/leds/flash/. (Yes, you'll have to create
> it).
OK
>
>
> > +     help
> > +       This option enables support for the RT4505 flash led controller.
>
> Information where it is used would be welcome here.
How about to add the below line for the extra information?
Usually used to company with the camera device on smartphone/tablet products
>
> > +       It can support up to 1.5A flash strobe current with hardware timeout
> > +       and low input voltage protection.
>
> This does not / should not be here.
OK
> > +
> > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> > +{
>
> 80 columns, where easy.
I'll review all source code to meet the 80 column limit.
>
> > +     struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> > +     u32 val = 0;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     if (level != LED_OFF) {
> > +             ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> > +                                      (level - 1) << RT4505_ITORCH_SHIFT);
> > +             if (ret)
> > +                     goto unlock;
> > +
> > +             val = RT4505_TORCH_SET;
> > +     }
> > +
> > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > +
> > +unlock:
> > +     mutex_unlock(&priv->lock);
> > +     return ret;
> > +}
>
> Why is the locking needed? What will the /sys/class/leds interface
> look like on system with your flash?

The original thought is because there's still another way to control
flash like as v4l2.
But after reviewing the source code, led sysfs node will be protected
by led_cdev->led_access.
And V4L2 flash will also be protected by v4l2_fh_is_singular API.
I think the whole locking in the source code code may be removed. Right?
>
> > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> > +{
> > +     struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
> > +     u32 val;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> > +     if (ret)
> > +             goto unlock;
> > +
> > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
>
> No need for ? ... part.
Do you mean this function is not needed? If yes, it can be removed.
But if it removed, led sysfs flash_strobe show will be not supported.
>
> > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > +             return true;
>
> Make this two stagements.
Like as the below one?? Or separate it into two if case.
if (reg == RT4505_REG_RESET ||
       reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
>
> Otherwise... looks like easy simple driver, thanks.
>
> Best regards,
>                                                                 Pavel
> --
> http://www.livejournal.com/~pavelmachek

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Hi, Parvel:
  Continue the last mail, I'm confused about the rule 80-characters-per-line.
I check some information about submitting changes.
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
Could you help to explain the current rule about this limit? still 80
characters per line? or it has been changed to 100.

ChiYuan Huang <u0084500@gmail.com> 於 2020年10月27日 週二 下午5:27寫道：
>
> Pavel Machek <pavel@ucw.cz> 於 2020年10月27日 週二 下午4:29寫道：
> >
> > Hi!
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > Add support for RT4505 flash led controller. It can support up to 1.5A
> > > flash current with hardware timeout and low input voltage
> > > protection.
> >
> > Please use upper-case "LED" everywhere.
> >
> > This should be 2nd in the series, after DT changes.
> Sure, will ack in next series patch.
> >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > >  drivers/leds/Kconfig       |  11 ++
> > >  drivers/leds/Makefile      |   1 +
> > >  drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 409 insertions(+)
> > >  create mode 100644 drivers/leds/leds-rt4505.c
> >
> > Lets put this into drivers/leds/flash/. (Yes, you'll have to create
> > it).
> OK
> >
> >
> > > +     help
> > > +       This option enables support for the RT4505 flash led controller.
> >
> > Information where it is used would be welcome here.
> How about to add the below line for the extra information?
> Usually used to company with the camera device on smartphone/tablet products
> >
> > > +       It can support up to 1.5A flash strobe current with hardware timeout
> > > +       and low input voltage protection.
> >
> > This does not / should not be here.
> OK
> > > +
> > > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> > > +{
> >
> > 80 columns, where easy.
> I'll review all source code to meet the 80 column limit.
> >
> > > +     struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> > > +     u32 val = 0;
> > > +     int ret;
> > > +
> > > +     mutex_lock(&priv->lock);
> > > +
> > > +     if (level != LED_OFF) {
> > > +             ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> > > +                                      (level - 1) << RT4505_ITORCH_SHIFT);
> > > +             if (ret)
> > > +                     goto unlock;
> > > +
> > > +             val = RT4505_TORCH_SET;
> > > +     }
> > > +
> > > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > > +
> > > +unlock:
> > > +     mutex_unlock(&priv->lock);
> > > +     return ret;
> > > +}
> >
> > Why is the locking needed? What will the /sys/class/leds interface
> > look like on system with your flash?
>
> The original thought is because there's still another way to control
> flash like as v4l2.
> But after reviewing the source code, led sysfs node will be protected
> by led_cdev->led_access.
> And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> I think the whole locking in the source code code may be removed. Right?
> >
> > > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> > > +{
> > > +     struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
> > > +     u32 val;
> > > +     int ret;
> > > +
> > > +     mutex_lock(&priv->lock);
> > > +
> > > +     ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> > > +     if (ret)
> > > +             goto unlock;
> > > +
> > > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
> >
> > No need for ? ... part.
> Do you mean this function is not needed? If yes, it can be removed.
> But if it removed, led sysfs flash_strobe show will be not supported.
> >
> > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > > +             return true;
> >
> > Make this two stagements.
> Like as the below one?? Or separate it into two if case.
> if (reg == RT4505_REG_RESET ||
>        reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> >
> > Otherwise... looks like easy simple driver, thanks.
> >
> > Best regards,
> >                                                                 Pavel
> > --
> > http://www.livejournal.com/~pavelmachek

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

Hi!

>   Continue the last mail, I'm confused about the rule 80-characters-per-line.
> I check some information about submitting changes.
> https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
> Could you help to explain the current rule about this limit? still 80
> characters per line? or it has been changed to 100.

Please see Documentation/process/coding-style.rst .

										Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

Hi!

> > Please use upper-case "LED" everywhere.
> >
> > This should be 2nd in the series, after DT changes.
> Sure, will ack in next series patch.

Feel free to wait for dt ACKs before resending.

> > > +     help
> > > +       This option enables support for the RT4505 flash led controller.
> >
> > Information where it is used would be welcome here.
> How about to add the below line for the extra information?
> Usually used to company with the camera device on smartphone/tablet
> products

Yes, that would help.

"It is commonly used in smartphones, such as Bell Packard T899" would
be even better.

> > > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > > +
> > > +unlock:
> > > +     mutex_unlock(&priv->lock);
> > > +     return ret;
> > > +}
> >
> > Why is the locking needed? What will the /sys/class/leds interface
> > look like on system with your flash?
> 
> The original thought is because there's still another way to control
> flash like as v4l2.
> But after reviewing the source code, led sysfs node will be protected
> by led_cdev->led_access.
> And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> I think the whole locking in the source code code may be removed. Right?

Well, maybe you need it, I did not check..

What will the /sys/class/leds interface look like on system with your flash?

> > > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
> >
> > No need for ? ... part.
> Do you mean this function is not needed? If yes, it can be removed.
> But if it removed, led sysfs flash_strobe show will be not supported.

I meant "replace line with: *state = (val & RT4505_FLASH_GET) == RT4505_FLASH_GET;"

> > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > > +             return true;
> >
> > Make this two stagements.
> Like as the below one?? Or separate it into two if case.
> if (reg == RT4505_REG_RESET ||
>        reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))

That would be fine, too... if you use just one space before "&&" :-).

Best regards,
							Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Pavel Machek <pavel@ucw.cz> 於 2020年10月27日 週二 下午6:15寫道：
>
> Hi!
>
> > > Please use upper-case "LED" everywhere.
> > >
> > > This should be 2nd in the series, after DT changes.
> > Sure, will ack in next series patch.
>
> Feel free to wait for dt ACKs before resending.
>
Yes, sure.
> > > > +     help
> > > > +       This option enables support for the RT4505 flash led controller.
> > >
> > > Information where it is used would be welcome here.
> > How about to add the below line for the extra information?
> > Usually used to company with the camera device on smartphone/tablet
> > products
>
> Yes, that would help.
>
> "It is commonly used in smartphones, such as Bell Packard T899" would
> be even better.

Sorry, We don't focus on specific products. It's a general part flash
led controller.
I'll change it like as below
"It's commonly used in smartphones and tablets to assist the builtin camera."
>
> > > > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > > > +
> > > > +unlock:
> > > > +     mutex_unlock(&priv->lock);
> > > > +     return ret;
> > > > +}
> > >
> > > Why is the locking needed? What will the /sys/class/leds interface
> > > look like on system with your flash?
> >
> > The original thought is because there's still another way to control
> > flash like as v4l2.
> > But after reviewing the source code, led sysfs node will be protected
> > by led_cdev->led_access.
> > And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> > I think the whole locking in the source code code may be removed. Right?
>
> Well, maybe you need it, I did not check..
>
> What will the /sys/class/leds interface look like on system with your flash?
>
> > > > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
> > >
> > > No need for ? ... part.
> > Do you mean this function is not needed? If yes, it can be removed.
> > But if it removed, led sysfs flash_strobe show will be not supported.
>
> I meant "replace line with: *state = (val & RT4505_FLASH_GET) == RT4505_FLASH_GET;"
Oh, I got it. redundant judgement.
>
> > > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > > > +{
> > > > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > > > +             return true;
> > >
> > > Make this two stagements.
> > Like as the below one?? Or separate it into two if case.
> > if (reg == RT4505_REG_RESET ||
> >        reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
>
> That would be fine, too... if you use just one space before "&&" :-).
Thx.
>
> Best regards,
>                                                         Pavel
> --
> http://www.livejournal.com/~pavelmachek

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

ChiYuan Huang <u0084500@gmail.com> 於 2020年10月27日 週二 下午6:46寫道：
>
> Pavel Machek <pavel@ucw.cz> 於 2020年10月27日 週二 下午6:15寫道：
> >
> > Hi!
> >
> > > > Please use upper-case "LED" everywhere.
> > > >
> > > > This should be 2nd in the series, after DT changes.
> > > Sure, will ack in next series patch.
> >
> > Feel free to wait for dt ACKs before resending.
> >
> Yes, sure.
> > > > > +     help
> > > > > +       This option enables support for the RT4505 flash led controller.
> > > >
> > > > Information where it is used would be welcome here.
> > > How about to add the below line for the extra information?
> > > Usually used to company with the camera device on smartphone/tablet
> > > products
> >
> > Yes, that would help.
> >
> > "It is commonly used in smartphones, such as Bell Packard T899" would
> > be even better.
>
> Sorry, We don't focus on specific products. It's a general part flash
> led controller.
> I'll change it like as below
> "It's commonly used in smartphones and tablets to assist the builtin camera."
> >
> > > > > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > > > > +
> > > > > +unlock:
> > > > > +     mutex_unlock(&priv->lock);
> > > > > +     return ret;
> > > > > +}
> > > >
> > > > Why is the locking needed? What will the /sys/class/leds interface
> > > > look like on system with your flash?
> > >
> > > The original thought is because there's still another way to control
> > > flash like as v4l2.
> > > But after reviewing the source code, led sysfs node will be protected
> > > by led_cdev->led_access.
> > > And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> > > I think the whole locking in the source code code may be removed. Right?
> >
> > Well, maybe you need it, I did not check..
I found all led class is guaranteed by led_cdev->led_access lock.
And v4l2 ctrl handleris guaranteed by ctrl->handler->lock that defined
in v4l2-ctrl.c.
When entering v4l2 control mode, all led sysfs will be disabled.
To remove all lock operation is correct. Due to all operations are guaranteed.
In next series patch, I'll remove it.
How do you think?
> >
> > What will the /sys/class/leds interface look like on system with your flash?
> >
> > > > > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
> > > >
> > > > No need for ? ... part.
> > > Do you mean this function is not needed? If yes, it can be removed.
> > > But if it removed, led sysfs flash_strobe show will be not supported.
> >
> > I meant "replace line with: *state = (val & RT4505_FLASH_GET) == RT4505_FLASH_GET;"
> Oh, I got it. redundant judgement.
> >
> > > > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > > > > +             return true;
> > > >
> > > > Make this two stagements.
> > > Like as the below one?? Or separate it into two if case.
> > > if (reg == RT4505_REG_RESET ||
> > >        reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> >
> > That would be fine, too... if you use just one space before "&&" :-).
> Thx.
> >
> > Best regards,
> >                                                         Pavel
> > --
> > http://www.livejournal.com/~pavelmachek

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[Jacek Anaszewski]

Hi Pavel, ChiYuan,

On 10/27/20 9:29 AM, Pavel Machek wrote:
> Hi!
> 
>> From: ChiYuan Huang <cy_huang@richtek.com>
>>
>> Add support for RT4505 flash led controller. It can support up to 1.5A
>> flash current with hardware timeout and low input voltage
>> protection.
> 
> Please use upper-case "LED" everywhere.
> 
> This should be 2nd in the series, after DT changes.
> 
>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>> ---
>>   drivers/leds/Kconfig       |  11 ++
>>   drivers/leds/Makefile      |   1 +
>>   drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 409 insertions(+)
>>   create mode 100644 drivers/leds/leds-rt4505.c
[...]
>> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
>> +{
> 
> 80 columns, where easy.
> 
>> +	struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
>> +	u32 val = 0;
>> +	int ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	if (level != LED_OFF) {
>> +		ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
>> +					 (level - 1) << RT4505_ITORCH_SHIFT);
>> +		if (ret)
>> +			goto unlock;
>> +
>> +		val = RT4505_TORCH_SET;
>> +	}
>> +
>> +	ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
>> +
>> +unlock:
>> +	mutex_unlock(&priv->lock);
>> +	return ret;
>> +}
> 
> Why is the locking needed? What will the /sys/class/leds interface
> look like on system with your flash?

The locking is needed since this can be called via led_set_brightness()
from any place in the kernel, and especially from triggers.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Hi,

Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午12:40寫道：
>
> Hi Pavel, ChiYuan,
>
> On 10/27/20 9:29 AM, Pavel Machek wrote:
> > Hi!
> >
> >> From: ChiYuan Huang <cy_huang@richtek.com>
> >>
> >> Add support for RT4505 flash led controller. It can support up to 1.5A
> >> flash current with hardware timeout and low input voltage
> >> protection.
> >
> > Please use upper-case "LED" everywhere.
> >
> > This should be 2nd in the series, after DT changes.
> >
> >> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >> ---
> >>   drivers/leds/Kconfig       |  11 ++
> >>   drivers/leds/Makefile      |   1 +
> >>   drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 409 insertions(+)
> >>   create mode 100644 drivers/leds/leds-rt4505.c
> [...]
> >> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> >> +{
> >
> > 80 columns, where easy.
> >
> >> +    struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> >> +    u32 val = 0;
> >> +    int ret;
> >> +
> >> +    mutex_lock(&priv->lock);
> >> +
> >> +    if (level != LED_OFF) {
> >> +            ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> >> +                                     (level - 1) << RT4505_ITORCH_SHIFT);
> >> +            if (ret)
> >> +                    goto unlock;
> >> +
> >> +            val = RT4505_TORCH_SET;
> >> +    }
> >> +
> >> +    ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> >> +
> >> +unlock:
> >> +    mutex_unlock(&priv->lock);
> >> +    return ret;
> >> +}
> >
> > Why is the locking needed? What will the /sys/class/leds interface
> > look like on system with your flash?
>
> The locking is needed since this can be called via led_set_brightness()
> from any place in the kernel, and especially from triggers.
From this case, It means only led classdev
brihtness_get/brightness_set need to be protected.
I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
Like as described in last mail, flash related operation is protected
by led access_lock and v4l2 framework.
I'll keep the locking only in led classdev brightness_get/brightness_set API.
If I misunderstand something, please help to point out.
Thx.
>
> --
> Best regards,
> Jacek Anaszewski

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[Jacek Anaszewski]

On 10/28/20 5:57 AM, ChiYuan Huang wrote:
> Hi,
> 
> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午12:40寫道：
>>
>> Hi Pavel, ChiYuan,
>>
>> On 10/27/20 9:29 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>>
>>>> Add support for RT4505 flash led controller. It can support up to 1.5A
>>>> flash current with hardware timeout and low input voltage
>>>> protection.
>>>
>>> Please use upper-case "LED" everywhere.
>>>
>>> This should be 2nd in the series, after DT changes.
>>>
>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>>> ---
>>>>    drivers/leds/Kconfig       |  11 ++
>>>>    drivers/leds/Makefile      |   1 +
>>>>    drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 409 insertions(+)
>>>>    create mode 100644 drivers/leds/leds-rt4505.c
>> [...]
>>>> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
>>>> +{
>>>
>>> 80 columns, where easy.
>>>
>>>> +    struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
>>>> +    u32 val = 0;
>>>> +    int ret;
>>>> +
>>>> +    mutex_lock(&priv->lock);
>>>> +
>>>> +    if (level != LED_OFF) {
>>>> +            ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
>>>> +                                     (level - 1) << RT4505_ITORCH_SHIFT);
>>>> +            if (ret)
>>>> +                    goto unlock;
>>>> +
>>>> +            val = RT4505_TORCH_SET;
>>>> +    }
>>>> +
>>>> +    ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
>>>> +
>>>> +unlock:
>>>> +    mutex_unlock(&priv->lock);
>>>> +    return ret;
>>>> +}
>>>
>>> Why is the locking needed? What will the /sys/class/leds interface
>>> look like on system with your flash?
>>
>> The locking is needed since this can be called via led_set_brightness()
>> from any place in the kernel, and especially from triggers.
>>From this case, It means only led classdev
> brihtness_get/brightness_set need to be protected.
> I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
> Like as described in last mail, flash related operation is protected
> by led access_lock and v4l2 framework.
> I'll keep the locking only in led classdev brightness_get/brightness_set API.
> If I misunderstand something, please help to point out.

Locking have to be used consistently for each access to the resource
being protected with the lock. Otherwise you can end up in a situation
when rt4505_torch_brightness_set and rt4505_flash_brightness_set will
try concurrently alter hardware state. Regardless of how harmful could
it be in case of this particular device it is certainly against
programming rules.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 下午7:07寫道：
>
> On 10/28/20 5:57 AM, ChiYuan Huang wrote:
> > Hi,
> >
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午12:40寫道：
> >>
> >> Hi Pavel, ChiYuan,
> >>
> >> On 10/27/20 9:29 AM, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>>
> >>>> Add support for RT4505 flash led controller. It can support up to 1.5A
> >>>> flash current with hardware timeout and low input voltage
> >>>> protection.
> >>>
> >>> Please use upper-case "LED" everywhere.
> >>>
> >>> This should be 2nd in the series, after DT changes.
> >>>
> >>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>>> ---
> >>>>    drivers/leds/Kconfig       |  11 ++
> >>>>    drivers/leds/Makefile      |   1 +
> >>>>    drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>    3 files changed, 409 insertions(+)
> >>>>    create mode 100644 drivers/leds/leds-rt4505.c
> >> [...]
> >>>> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> >>>> +{
> >>>
> >>> 80 columns, where easy.
> >>>
> >>>> +    struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> >>>> +    u32 val = 0;
> >>>> +    int ret;
> >>>> +
> >>>> +    mutex_lock(&priv->lock);
> >>>> +
> >>>> +    if (level != LED_OFF) {
> >>>> +            ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> >>>> +                                     (level - 1) << RT4505_ITORCH_SHIFT);
> >>>> +            if (ret)
> >>>> +                    goto unlock;
> >>>> +
> >>>> +            val = RT4505_TORCH_SET;
> >>>> +    }
> >>>> +
> >>>> +    ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> >>>> +
> >>>> +unlock:
> >>>> +    mutex_unlock(&priv->lock);
> >>>> +    return ret;
> >>>> +}
> >>>
> >>> Why is the locking needed? What will the /sys/class/leds interface
> >>> look like on system with your flash?
> >>
> >> The locking is needed since this can be called via led_set_brightness()
> >> from any place in the kernel, and especially from triggers.
> >>From this case, It means only led classdev
> > brihtness_get/brightness_set need to be protected.
> > I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
> > Like as described in last mail, flash related operation is protected
> > by led access_lock and v4l2 framework.
> > I'll keep the locking only in led classdev brightness_get/brightness_set API.
> > If I misunderstand something, please help to point out.
>
> Locking have to be used consistently for each access to the resource
> being protected with the lock. Otherwise you can end up in a situation
> when rt4505_torch_brightness_set and rt4505_flash_brightness_set will
> try concurrently alter hardware state. Regardless of how harmful could
> it be in case of this particular device it is certainly against
> programming rules.
>
Sure, any resource access must be protected.
I'll keep the locking like as the original patch.
Thx.
> --
> Best regards,
> Jacek Anaszewski

Re: [PATCH v1 2/2] leds: rt4505: Add DT binding document for Richtek RT4505

[Rob Herring]

On Tue, 27 Oct 2020 15:34:29 +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add DT binding document for Richtek RT4505 flash led controller.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../devicetree/bindings/leds/leds-rt4505.yaml      | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-rt4505.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>