lkml.org 
[lkml]   [2021]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-02-19 12:05    [W:0.276 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site