by Pavel Machek

[permalink] [raw]

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

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

Attachments:

(No filename) (3.58 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2021-10-08 02:15:39

by Nícolas F. R. A. Prado

[permalink] [raw]

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

Hi Jacek,

> > > > +static int qcom_flash_flcdev_strobe_set(struct led_classdev_flash *fled_cdev,
> > > > + bool state)
> > > > +{
> > > > + struct qcom_flash_led *led = flcdev_to_led(fled_cdev);
> > > > + struct qcom_flash_device *leds_dev = led_to_leds_dev(led);
> > > > + unsigned int bright;
> > > > + unsigned int i;
> > > > + int rc;
> > > > +
> > > > + /* Can't operate on flash if torch is on */
> > > > + if (leds_dev->torch_enabled)
> > > > + return -EBUSY;
> > > > +
> > > > + mutex_lock(&leds_dev->lock);
> > > > + if (!state) {
> > > > + rc = qcom_flash_fled_off(led);
> > > > + } else {
> > > > + /*
> > > > + * Turn off flash LEDs from previous strobe
> > > > + */
> > > > + rc = qcom_flash_check_timedout(leds_dev);
> > > > + if (rc > 0) {
> > > > + for (i = 0; i < leds_dev->num_leds; i++) {
> > > > + rc = qcom_flash_fled_off(&leds_dev->leds[i]);
> > > > + if (rc)
> > > > + goto unlock;
> > > > + }
> > > > + } else if (rc < 0) {
> > > > + goto unlock;
> > > > + }
> > >
> > > What if flash gets timed out after this check here? Why do you need to
> > > call qcom_flash_fled_off() if it has already timed out?
> >
> > The issue is that after the flash times out, the hardware is not ready for
> > another strobe.
> >
> > When I strobe LED0 for example, the STATUS register, 0x10, gets set to 0x08
> > indicating the LED0 is on. After the timeout, it changes to 0x04. At that point
> > if I try to strobe LED0 again, it doesn't work. When I turn the LED0 off (write
> > 0x00 to either the ENABLE or STROBE register), the STATUS is reset to 0x00. Now
> > I'm able to strobe the LED0 again.
> >
> > I'm not sure if this is the normal behavior on other flash LED controllers, and
> > maybe there's even some configuration of this PMIC that resets the LED status
> > automatically after the strobe timeout, but I have not been able to do that. So
> > that's why I reset the status manually everytime it's needed.
>
> My point was that the flash may time out after reading STATUS register
> and before writing QCOM_FLASH_ADDR_LED_STROBE_CTRL.
> You can't be 100% sure that you know the exact STATUS state just
> a moment before strobing.

That's true, but that scenario only happens if there's an ongoing flash strobe
happening and userspace triggers another strobe. Is that a scenario that really
needs to be taken care of, and if so, what would be the correct behavior? Does
the timeout need to be reset for this new strobe, possibly using updated
brightness and timeout values? (Currently none of this happens)

The purpose of this check is not to know if an ongoing flash strobe has
timed out, but rather to differentiate if the previous time the LED was strobed
was as a flash (with timeout) or torch (no timeout), because the flash
case needs an extra reset step that can be ommited in the torch case. For this
purpose there's no race condition.

>
> To alleviate that I propose to avoid checking the status and always
> calling qcom_flash_fled_off() before initiating a new strobe.

Thanks,
Nicolas