Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Morning Hans

On 22/03/2023 17:22, Hans de Goede wrote:
Hi,

On 3/22/23 17:09, Daniel Scally wrote:
We want to extend tps68470_brightness_get() to be usable with the
other LEDs supplied by the IC; refactor it to make that easier.

Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
---
  drivers/leds/leds-tps68470.c | 17 +++++++++--------
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
index d2060fe4259d..44df175d25de 100644
--- a/drivers/leds/leds-tps68470.c
+++ b/drivers/leds/leds-tps68470.c
@@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
  	int ret = 0;
  	int value = 0;
- ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
-	if (ret)
-		return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n");
-
  	switch (led->led_id) {
  	case TPS68470_ILED_A:
-		value = value & TPS68470_ILEDCTL_ENA;
-		break;
  	case TPS68470_ILED_B:
-		value = value & TPS68470_ILEDCTL_ENB;
+		ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
+		if (ret)
+			return dev_err_probe(led_cdev->dev, ret,
+					     "failed to read LED status\n");
I realize this is a pre-existing problem, but I don't think we should
be using dev_err_probe() in functions which are used outside the probe()
path?


I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it.


So maybe fix this up while at it and make this:

		if (ret) {
			dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret);
			return ret;
		}

+
+		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
+					TPS68470_ILEDCTL_ENB;
  		break;
+	default:
+		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
idem.


idem? Sorry, I'm not following here.


With those fixed:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Regards,

Hans



  	}
return value ? LED_ON : LED_OFF;
  }
-
  static int tps68470_ledb_current_init(struct platform_device *pdev,
  				      struct tps68470_device *tps68470)
  {



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux