On Tue 2021-01-26 14:05:54, Nícolas F. R. A. Prado wrote:

Add driver for the Qualcomm SPMI Flash LEDs. These are controlled
through an SPMI bus and are part of the PM8941 PMIC. There are two LEDs
present in the chip, and can be used independently as camera flash or
together in torch mode to act as a lantern.

 drivers/leds/Kconfig                |    8 +
 drivers/leds/Makefile               |    1 +
 drivers/leds/leds-qcom-spmi-flash.c | 1153 +++++++++++++++++++++++++++
 3 files changed, 1162 insertions(+)

Ok, please make this go to drivers/leds/flash/

+static int qcom_flash_fled_regulator_operate(struct qcom_flash_device *leds_dev,
+					     bool on)
+{
+	int rc;
+
+	if (!on)
+		goto regulator_turn_off;
+
+	if (!leds_dev->flash_regulator_on) {
+		if (leds_dev->flash_boost_reg) {
+			rc = regulator_enable(leds_dev->flash_boost_reg);
+			if (rc) {
+				dev_err(&leds_dev->pdev->dev,
+					"Regulator enable failed(%d)\n", rc);
+				return rc;
+			}
+			leds_dev->flash_regulator_on = true;
+		}
+	}
+
+	return 0;
+
+regulator_turn_off:
+	if (leds_dev->flash_regulator_on) {
+		if (leds_dev->flash_boost_reg) {
+			rc = qcom_flash_masked_write(leds_dev,
+				FLASH_ENABLE_CONTROL,
+				FLASH_ENABLE_MASK,
+				FLASH_DISABLE_ALL);
+			if (rc)
+				dev_err(&leds_dev->pdev->dev,
+					"Enable reg write failed(%d)\n", rc);
+
+			rc = regulator_disable(leds_dev->flash_boost_reg);
+			if (rc) {
+				dev_err(&leds_dev->pdev->dev,
+					"Regulator disable failed(%d)\n", rc);
+				return rc;
+			}
+			leds_dev->flash_regulator_on = false;
+		}
+	}
+
+	return 0;
+}

Try to find a way to write this without gotos and with less
indentation. Separate functions may be useful.

+static int qcom_flash_fled_set(struct qcom_flash_led *led, bool on)
+{
+	int rc, error;
+	u8 curr;
+	struct qcom_flash_device *leds_dev = led_to_leds_dev(led);
+	struct device *dev = &leds_dev->pdev->dev;
+
+	/* dump flash registers */
+	pr_debug("Regdump before\n");
+	qcom_flash_dump_regs(leds_dev, flash_debug_regs,
+			     ARRAY_SIZE(flash_debug_regs));

I believe this kind of debugging is not needed for production.

+	/* Set led current */
+	if (on) {
+		if (led->torch_enable)
+			curr = qcom_flash_current_to_reg(led->cdev.led_cdev.brightness);
+		else
+			curr = qcom_flash_current_to_reg(led->cdev.brightness.val);
+
+		if (led->torch_enable) {
+			if (leds_dev->peripheral_subtype == FLASH_SUBTYPE_DUAL) {
+				rc = qcom_flash_torch_regulator_operate(leds_dev, true);
+				if (rc) {
+					dev_err(dev,
+					"Torch regulator operate failed(%d)\n",
+					rc);
+					return rc;
+				}

No need to goto here?

+			} else if (leds_dev->peripheral_subtype == FLASH_SUBTYPE_SINGLE) {
+				rc = qcom_flash_fled_regulator_operate(leds_dev, true);
+				if (rc) {
+					dev_err(dev,
+					"Flash regulator operate failed(%d)\n",
+					rc);
+					goto error_flash_set;
+				}
+
+				/*
+				 * Write 0x80 to MODULE_ENABLE before writing
+				 * 0xE0 in order to avoid a hardware bug caused
+				 * by register value going from 0x00 to 0xE0.
+				 */
+				rc = qcom_flash_masked_write(leds_dev,
+					FLASH_ENABLE_CONTROL,
+					FLASH_ENABLE_MODULE_MASK,
+					FLASH_ENABLE_MODULE);
+				if (rc) {
+					dev_err(dev,
+						"Enable reg write failed(%d)\n",
+						rc);
+					return rc;
+				}
+			}

Anyway, pleae find a way to split this function so that it is less
indented.

+		/* TODO try to not busy wait*/
+		mdelay(2);
+		udelay(160);

What?

Best regards,
								Pavel

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