Re: [PATCH v2 2/4] leds: Add driver for QCOM SPMI Flash LEDs

[Pavel Machek]

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

Attachment: signature.asc
Description: PGP signature