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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210829175906.GA663@amd>
Date:   Sun, 29 Aug 2021 19:59:07 +0200
From:   Pavel Machek <pavel@....cz>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc:     Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>, Dan Murphy <dmurphy@...com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, linux-leds@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        Brian Masney <masneyb@...tation.org>,
        Luca Weiss <luca@...tu.xyz>,
        Russell King <linux@...linux.org.uk>,
        Georgi Djakov <georgi.djakov@...aro.org>,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht, ~lkcamp/patches@...ts.sr.ht,
        André Almeida <andrealmeid@...labora.com>,
        kernel@...labora.com
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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)