linux-kernel - Re: [PATCH v2 2/4] 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]

Date:   Fri, 19 Feb 2021 12:01:21 +0100
From:   Pavel Machek <pavel@....cz>
To:     Nícolas F. R. A. Prado 
        <nfraprado@...tonmail.com>
Cc:     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,
        ~postmarketos/upstreaming@...ts.sr.ht,
        lkcamp@...ts.libreplanetbr.org, andrealmeid@...labora.com
Subject: Re: [PATCH v2 2/4] leds: Add driver for QCOM SPMI Flash LEDs

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

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