Re: [PATCH v3 2/5] leds: Add driver for QCOM SPMI Flash LEDs

[Pavel Machek]

Hi both!

Please trim your replies (removing code you are not commenting
on). Scolling 600 lines to find where discussion is is not fun.

Best regards,
								Pavel

> >>>+static int qcom_flash_torch_on(struct qcom_flash_led *led)
> >>>+{
> >>>+	int rc, error;
> >>>+	struct qcom_flash_device *leds_dev = led_to_leds_dev(led);
> >>>+	struct device *dev = leds_dev->dev;
> >>>+
> >>>+	if (leds_dev->peripheral_subtype == QCOM_FLASH_SUBTYPE_DUAL) {
> >>>+		rc = qcom_flash_torch_regulator_on(leds_dev);
> >>>+		if (rc)
> >>>+			goto error_reg_write;
> >>>+	} else if (leds_dev->peripheral_subtype == QCOM_FLASH_SUBTYPE_SINGLE) {
> >>>+		rc = qcom_flash_fled_regulator_on(leds_dev);
> >>
> >>Why for torch mode you need to enable fled regulator?
> >
> >Based on [1], apparently the hardware present in the Single variant of the PMIC
> >has some limitation that requires the use of the flash regulator and the value
> >QCOM_FLASH_ENABLE_ALL to enable the LEDs for the torch mode. The Dual variant on
> >the other hand can just use the torch regulator and enables the LEDs with
> >QCOM_FLASH_ENABLE_MODULE.
> >
> >[1] https://github.com/AICP/kernel_lge_hammerhead/commit/0f47c747c074993655d0bfebd045e8ddd228fe4c
> >
> >I'm honestly not sure what the impact is on using the different regulators and
> >enable values. I have tested enabling the Dual PMIC with different enable values
> >and all seemed to work the same, so must be some hardware detail.
> >
> >I left that Single codepath in the hope that it is useful for devices that have
> >that variant of the hardware, but I have only actually tested the Dual PMIC,
> >which is the one present on the Nexus 5.
> 
> Thanks for the explanation. Just wanted to confirm that it was not
> a mistake.
> 
> >>
> >>>+		if (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,
> >>>+					     QCOM_FLASH_ADDR_ENABLE_CONTROL,
> >>>+					     QCOM_FLASH_ENABLE_MODULE_MASK,
> >>>+					     QCOM_FLASH_ENABLE_MODULE);
> >>>+		if (rc) {
> >>>+			dev_err(dev, "Enable reg write failed(%d)\n", rc);
> >>>+			goto error_flash_set;
> >>>+		}
> >>>+	}
> >>>+
> >>>+	rc = qcom_flash_torch_reg_enable(leds_dev, true);
> >>>+	if (rc)
> >>>+		goto error_reg_write;
> >>>+
> >>>+	rc = qcom_flash_masked_write(leds_dev, QCOM_FLASH_ADDR_ENABLE_CONTROL,
> >>>+				     QCOM_FLASH_ENABLE_MASK,
> >>>+				     leds_dev->torch_enable_cmd);
> >>>+	if (rc) {
> >>>+		dev_err(dev, "Enable reg write failed(%d)\n", rc);
> >>>+		goto error_reg_write;
> >>>+	}
> >>>+
> >>>+	rc = qcom_flash_masked_write(leds_dev, QCOM_FLASH_ADDR_LED_STROBE_CTRL,
> >>>+				     led->flash_strobe_cmd,
> >>>+				     led->flash_strobe_cmd);
> >>
> >>Just to make sure - the hardware requires strobe cmd to enable torch?
> >
> >Yes. The strobe value is the one that actually turns each of the LEDs on,
> >doesn't matter if it's on flash or torch mode. The difference in torch mode is
> >actually just that the timeout on the LEDs is disabled (done by writing 0x00
> >into the TORCH, 0xE4, register).
> >So for both modes, the LEDs are turned on by writing to the STROBE_CTRL, 0x47,
> >register. If torch is on they'll stay on indefinitely, while on flash mode
> >they'll turn off after the timeout.
> >
> >Perhaps it's just a naming issue?
> 
> I propose to add these comments next to the calls in question.


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

Attachment: signature.asc
Description: Digital signature