From: Pavel Machek <pavel@ucw.cz>
To: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: "Nícolas F. R. A. Prado" <nfraprado@collabora.com>,
"Dan Murphy" <dmurphy@ti.com>,
"Bjorn Andersson" <bjorn.andersson@linaro.org>,
"Andy Gross" <agross@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
"Brian Masney" <masneyb@onstation.org>,
"Luca Weiss" <luca@z3ntu.xyz>,
"Russell King" <linux@armlinux.org.uk>,
"Georgi Djakov" <georgi.djakov@linaro.org>,
linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
~lkcamp/patches@lists.sr.ht,
"André Almeida" <andrealmeid@collabora.com>,
kernel@collabora.com
Subject: Re: [PATCH v3 2/5] leds: Add driver for QCOM SPMI Flash LEDs
Date: Sun, 29 Aug 2021 19:59:07 +0200 [thread overview]
Message-ID: <20210829175906.GA663@amd> (raw)
In-Reply-To: <278ea1e8-8b21-457d-78d7-fbb32544fe0a@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]
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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
next prev parent reply other threads:[~2021-08-29 17:59 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-03 16:26 [PATCH v3 0/5] Add support for QCOM SPMI Flash LEDs Nícolas F. R. A. Prado
2021-08-03 16:26 ` [PATCH v3 1/5] dt-bindings: leds: Add binding for qcom-spmi-flash Nícolas F. R. A. Prado
2021-08-11 18:35 ` Rob Herring
2021-08-03 16:26 ` [PATCH v3 2/5] leds: Add driver for QCOM SPMI Flash LEDs Nícolas F. R. A. Prado
2021-08-16 22:03 ` Jacek Anaszewski
2021-08-24 21:45 ` Nícolas F. R. A. Prado
2021-08-29 11:15 ` Jacek Anaszewski
2021-08-29 17:59 ` Pavel Machek [this message]
2021-10-08 2:12 ` Nícolas F. R. A. Prado
2021-08-03 16:26 ` [PATCH v3 3/5] ARM: qcom_defconfig: Enable " Nícolas F. R. A. Prado
2021-08-03 16:26 ` [PATCH v3 4/5] ARM: dts: qcom: pm8941: Add nodes for " Nícolas F. R. A. Prado
2021-08-03 16:26 ` [PATCH v3 5/5] ARM: dts: qcom: msm8974-hammerhead: Enable and configure flash LED node Nícolas F. R. A. Prado
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210829175906.GA663@amd \
--to=pavel@ucw.cz \
--cc=agross@kernel.org \
--cc=andrealmeid@collabora.com \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dmurphy@ti.com \
--cc=georgi.djakov@linaro.org \
--cc=jacek.anaszewski@gmail.com \
--cc=kernel@collabora.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=luca@z3ntu.xyz \
--cc=masneyb@onstation.org \
--cc=nfraprado@collabora.com \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=~lkcamp/patches@lists.sr.ht \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).