[PATCH 1/5] backlight: led_bl: fix cosmetic issues

[PATCH 1/5] backlight: led_bl: fix cosmetic issues

[Tomi Valkeinen]

Fix issues reported by checkpatch. No functional changes.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 3f66549997c8..d4e1ce684366 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -126,7 +126,7 @@ static int led_bl_get_leds(struct device *dev,
 }
 
 static int led_bl_parse_levels(struct device *dev,
-			   struct led_bl_data *priv)
+			       struct led_bl_data *priv)
 {
 	struct device_node *node = dev->of_node;
 	int num_levels;
@@ -148,8 +148,8 @@ static int led_bl_parse_levels(struct device *dev,
 			return -ENOMEM;
 
 		ret = of_property_read_u32_array(node, "brightness-levels",
-						levels,
-						num_levels);
+						 levels,
+						 num_levels);
 		if (ret < 0)
 			return ret;
 
@@ -159,14 +159,15 @@ static int led_bl_parse_levels(struct device *dev,
 		 */
 		db = priv->default_brightness;
 		for (i = 0 ; i < num_levels; i++) {
-			if ((i && db > levels[i-1]) && db <= levels[i])
+			if ((i && db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
 		priv->default_brightness = i;
 		priv->max_brightness = num_levels - 1;
 		priv->levels = levels;
-	} else if (num_levels >= 0)
+	} else if (num_levels >= 0) {
 		dev_warn(dev, "Not enough levels defined\n");
+	}
 
 	ret = of_property_read_u32(node, "default-brightness-level", &value);
 	if (!ret && value <= priv->max_brightness)
@@ -208,7 +209,8 @@ static int led_bl_probe(struct platform_device *pdev)
 	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
 		      FB_BLANK_UNBLANK;
 	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
-			&pdev->dev, priv, &led_bl_ops, &props);
+						 &pdev->dev, priv, &led_bl_ops,
+						 &props);
 	if (IS_ERR(priv->bl_dev)) {
 		dev_err(&pdev->dev, "Failed to register backlight\n");
 		return PTR_ERR(priv->bl_dev);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/5] backlight: led_bl: fix cosmetic issues

[Tomi Valkeinen]

Fix issues reported by checkpatch. No functional changes.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 3f66549997c8..d4e1ce684366 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -126,7 +126,7 @@ static int led_bl_get_leds(struct device *dev,
 }
 
 static int led_bl_parse_levels(struct device *dev,
-			   struct led_bl_data *priv)
+			       struct led_bl_data *priv)
 {
 	struct device_node *node = dev->of_node;
 	int num_levels;
@@ -148,8 +148,8 @@ static int led_bl_parse_levels(struct device *dev,
 			return -ENOMEM;
 
 		ret = of_property_read_u32_array(node, "brightness-levels",
-						levels,
-						num_levels);
+						 levels,
+						 num_levels);
 		if (ret < 0)
 			return ret;
 
@@ -159,14 +159,15 @@ static int led_bl_parse_levels(struct device *dev,
 		 */
 		db = priv->default_brightness;
 		for (i = 0 ; i < num_levels; i++) {
-			if ((i && db > levels[i-1]) && db <= levels[i])
+			if ((i && db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
 		priv->default_brightness = i;
 		priv->max_brightness = num_levels - 1;
 		priv->levels = levels;
-	} else if (num_levels >= 0)
+	} else if (num_levels >= 0) {
 		dev_warn(dev, "Not enough levels defined\n");
+	}
 
 	ret = of_property_read_u32(node, "default-brightness-level", &value);
 	if (!ret && value <= priv->max_brightness)
@@ -208,7 +209,8 @@ static int led_bl_probe(struct platform_device *pdev)
 	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
 		      FB_BLANK_UNBLANK;
 	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
-			&pdev->dev, priv, &led_bl_ops, &props);
+						 &pdev->dev, priv, &led_bl_ops,
+						 &props);
 	if (IS_ERR(priv->bl_dev)) {
 		dev_err(&pdev->dev, "Failed to register backlight\n");
 		return PTR_ERR(priv->bl_dev);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] backlight: led_bl: drop useless NULL initialization

[Tomi Valkeinen]

There's no need to set 'levels' to NULL.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index d4e1ce684366..c46ecdfe8b0a 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -140,7 +140,7 @@ static int led_bl_parse_levels(struct device *dev,
 	if (num_levels > 1) {
 		int i;
 		unsigned int db;
-		u32 *levels = NULL;
+		u32 *levels;
 
 		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
 				      GFP_KERNEL);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/5] backlight: led_bl: drop useless NULL initialization

[Tomi Valkeinen]

There's no need to set 'levels' to NULL.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index d4e1ce684366..c46ecdfe8b0a 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -140,7 +140,7 @@ static int led_bl_parse_levels(struct device *dev,
 	if (num_levels > 1) {
 		int i;
 		unsigned int db;
-		u32 *levels = NULL;
+		u32 *levels;
 
 		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
 				      GFP_KERNEL);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] backlight: led_bl: add led_access locking

[Tomi Valkeinen]

led_bl does not lock 'led_access' when calling led_sysfs_disable and
led_sysfs_enable, causing the below WARN. Add the locking.

WARNING: CPU: 0 PID: 223 at drivers/leds/led-core.c:353 led_sysfs_disable+0x4c/0x5c

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index c46ecdfe8b0a..63693c4f0883 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -216,8 +216,11 @@ static int led_bl_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->bl_dev);
 	}
 
-	for (i = 0; i < priv->nb_leds; i++)
+	for (i = 0; i < priv->nb_leds; i++) {
+		mutex_lock(&priv->leds[i]->led_access);
 		led_sysfs_disable(priv->leds[i]);
+		mutex_unlock(&priv->leds[i]->led_access);
+	}
 
 	backlight_update_status(priv->bl_dev);
 
@@ -233,8 +236,11 @@ static int led_bl_remove(struct platform_device *pdev)
 	backlight_device_unregister(bl);
 
 	led_bl_power_off(priv);
-	for (i = 0; i < priv->nb_leds; i++)
+	for (i = 0; i < priv->nb_leds; i++) {
+		mutex_lock(&priv->leds[i]->led_access);
 		led_sysfs_enable(priv->leds[i]);
+		mutex_unlock(&priv->leds[i]->led_access);
+	}
 
 	return 0;
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/5] backlight: led_bl: add led_access locking

[Tomi Valkeinen]

led_bl does not lock 'led_access' when calling led_sysfs_disable and
led_sysfs_enable, causing the below WARN. Add the locking.

WARNING: CPU: 0 PID: 223 at drivers/leds/led-core.c:353 led_sysfs_disable+0x4c/0x5c

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index c46ecdfe8b0a..63693c4f0883 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -216,8 +216,11 @@ static int led_bl_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->bl_dev);
 	}
 
-	for (i = 0; i < priv->nb_leds; i++)
+	for (i = 0; i < priv->nb_leds; i++) {
+		mutex_lock(&priv->leds[i]->led_access);
 		led_sysfs_disable(priv->leds[i]);
+		mutex_unlock(&priv->leds[i]->led_access);
+	}
 
 	backlight_update_status(priv->bl_dev);
 
@@ -233,8 +236,11 @@ static int led_bl_remove(struct platform_device *pdev)
 	backlight_device_unregister(bl);
 
 	led_bl_power_off(priv);
-	for (i = 0; i < priv->nb_leds; i++)
+	for (i = 0; i < priv->nb_leds; i++) {
+		mutex_lock(&priv->leds[i]->led_access);
 		led_sysfs_enable(priv->leds[i]);
+		mutex_unlock(&priv->leds[i]->led_access);
+	}
 
 	return 0;
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Tomi Valkeinen]

The code that maps the LED default brightness to backlight levels has
two issues: 1) if the default brightness is the first backlight level
(usually 0), the code fails to find it, and 2) when the code fails to
find a backlight level, it ends up using max_brightness + 1 as the
default brightness.

Fix these two issues.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 63693c4f0883..021b5edd895c 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
 		 */
 		db = priv->default_brightness;
 		for (i = 0 ; i < num_levels; i++) {
-			if ((i && db > levels[i - 1]) && db <= levels[i])
+			if ((i = 0 || db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
-		priv->default_brightness = i;
+
+		priv->default_brightness = i < num_levels ? i : 0;
 		priv->max_brightness = num_levels - 1;
 		priv->levels = levels;
 	} else if (num_levels >= 0) {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Tomi Valkeinen]

The code that maps the LED default brightness to backlight levels has
two issues: 1) if the default brightness is the first backlight level
(usually 0), the code fails to find it, and 2) when the code fails to
find a backlight level, it ends up using max_brightness + 1 as the
default brightness.

Fix these two issues.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 63693c4f0883..021b5edd895c 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
 		 */
 		db = priv->default_brightness;
 		for (i = 0 ; i < num_levels; i++) {
-			if ((i && db > levels[i - 1]) && db <= levels[i])
+			if ((i == 0 || db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
-		priv->default_brightness = i;
+
+		priv->default_brightness = i < num_levels ? i : 0;
 		priv->max_brightness = num_levels - 1;
 		priv->levels = levels;
 	} else if (num_levels >= 0) {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
more obvious code flow.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 63 ++++++++++++++++----------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 021b5edd895c..7b3889035540 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev,
 	int num_levels;
 	u32 value;
 	int ret;
+	int i;
+	u32 *levels;
 
 	if (!node)
 		return -ENODEV;
 
 	num_levels = of_property_count_u32_elems(node, "brightness-levels");
-	if (num_levels > 1) {
-		int i;
-		unsigned int db;
-		u32 *levels;
-
-		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
-				      GFP_KERNEL);
-		if (!levels)
-			return -ENOMEM;
-
-		ret = of_property_read_u32_array(node, "brightness-levels",
-						 levels,
-						 num_levels);
-		if (ret < 0)
-			return ret;
-
-		/*
-		 * Try to map actual LED brightness to backlight brightness
-		 * level
-		 */
-		db = priv->default_brightness;
+
+	if (num_levels < 0)
+		return 0;
+
+	if (num_levels = 0) {
+		dev_warn(dev, "No brightness-levels defined\n");
+		return -EINVAL;
+	}
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
+			      GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0)
+		return ret;
+
+	priv->max_brightness = num_levels - 1;
+	priv->levels = levels;
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (!ret) {
+		priv->default_brightness = min(value, priv->max_brightness);
+	} else {
+		/* Map LED default-brightness to backlight brightness level */
+		unsigned int db = priv->default_brightness;
+
 		for (i = 0 ; i < num_levels; i++) {
 			if ((i = 0 || db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
 
 		priv->default_brightness = i < num_levels ? i : 0;
-		priv->max_brightness = num_levels - 1;
-		priv->levels = levels;
-	} else if (num_levels >= 0) {
-		dev_warn(dev, "Not enough levels defined\n");
 	}
 
-	ret = of_property_read_u32(node, "default-brightness-level", &value);
-	if (!ret && value <= priv->max_brightness)
-		priv->default_brightness = value;
-	else if (!ret  && value > priv->max_brightness)
-		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
-
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
more obvious code flow.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/led_bl.c | 63 ++++++++++++++++----------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 021b5edd895c..7b3889035540 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev,
 	int num_levels;
 	u32 value;
 	int ret;
+	int i;
+	u32 *levels;
 
 	if (!node)
 		return -ENODEV;
 
 	num_levels = of_property_count_u32_elems(node, "brightness-levels");
-	if (num_levels > 1) {
-		int i;
-		unsigned int db;
-		u32 *levels;
-
-		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
-				      GFP_KERNEL);
-		if (!levels)
-			return -ENOMEM;
-
-		ret = of_property_read_u32_array(node, "brightness-levels",
-						 levels,
-						 num_levels);
-		if (ret < 0)
-			return ret;
-
-		/*
-		 * Try to map actual LED brightness to backlight brightness
-		 * level
-		 */
-		db = priv->default_brightness;
+
+	if (num_levels < 0)
+		return 0;
+
+	if (num_levels == 0) {
+		dev_warn(dev, "No brightness-levels defined\n");
+		return -EINVAL;
+	}
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
+			      GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0)
+		return ret;
+
+	priv->max_brightness = num_levels - 1;
+	priv->levels = levels;
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (!ret) {
+		priv->default_brightness = min(value, priv->max_brightness);
+	} else {
+		/* Map LED default-brightness to backlight brightness level */
+		unsigned int db = priv->default_brightness;
+
 		for (i = 0 ; i < num_levels; i++) {
 			if ((i == 0 || db > levels[i - 1]) && db <= levels[i])
 				break;
 		}
 
 		priv->default_brightness = i < num_levels ? i : 0;
-		priv->max_brightness = num_levels - 1;
-		priv->levels = levels;
-	} else if (num_levels >= 0) {
-		dev_warn(dev, "Not enough levels defined\n");
 	}
 
-	ret = of_property_read_u32(node, "default-brightness-level", &value);
-	if (!ret && value <= priv->max_brightness)
-		priv->default_brightness = value;
-	else if (!ret  && value > priv->max_brightness)
-		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
-
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] backlight: led_bl: fix cosmetic issues

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:08PM +0300, Tomi Valkeinen wrote:
> Fix issues reported by checkpatch. No functional changes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 3f66549997c8..d4e1ce684366 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -126,7 +126,7 @@ static int led_bl_get_leds(struct device *dev,
>  }
>  
>  static int led_bl_parse_levels(struct device *dev,
> -			   struct led_bl_data *priv)
> +			       struct led_bl_data *priv)
>  {
>  	struct device_node *node = dev->of_node;
>  	int num_levels;
> @@ -148,8 +148,8 @@ static int led_bl_parse_levels(struct device *dev,
>  			return -ENOMEM;
>  
>  		ret = of_property_read_u32_array(node, "brightness-levels",
> -						levels,
> -						num_levels);
> +						 levels,
> +						 num_levels);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -159,14 +159,15 @@ static int led_bl_parse_levels(struct device *dev,
>  		 */
>  		db = priv->default_brightness;
>  		for (i = 0 ; i < num_levels; i++) {
> -			if ((i && db > levels[i-1]) && db <= levels[i])
> +			if ((i && db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
>  		priv->default_brightness = i;
>  		priv->max_brightness = num_levels - 1;
>  		priv->levels = levels;
> -	} else if (num_levels >= 0)
> +	} else if (num_levels >= 0) {
>  		dev_warn(dev, "Not enough levels defined\n");
> +	}
>  
>  	ret = of_property_read_u32(node, "default-brightness-level", &value);
>  	if (!ret && value <= priv->max_brightness)
> @@ -208,7 +209,8 @@ static int led_bl_probe(struct platform_device *pdev)
>  	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
>  		      FB_BLANK_UNBLANK;
>  	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> -			&pdev->dev, priv, &led_bl_ops, &props);
> +						 &pdev->dev, priv, &led_bl_ops,
> +						 &props);
>  	if (IS_ERR(priv->bl_dev)) {
>  		dev_err(&pdev->dev, "Failed to register backlight\n");
>  		return PTR_ERR(priv->bl_dev);
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Re: [PATCH 1/5] backlight: led_bl: fix cosmetic issues

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:08PM +0300, Tomi Valkeinen wrote:
> Fix issues reported by checkpatch. No functional changes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 3f66549997c8..d4e1ce684366 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -126,7 +126,7 @@ static int led_bl_get_leds(struct device *dev,
>  }
>  
>  static int led_bl_parse_levels(struct device *dev,
> -			   struct led_bl_data *priv)
> +			       struct led_bl_data *priv)
>  {
>  	struct device_node *node = dev->of_node;
>  	int num_levels;
> @@ -148,8 +148,8 @@ static int led_bl_parse_levels(struct device *dev,
>  			return -ENOMEM;
>  
>  		ret = of_property_read_u32_array(node, "brightness-levels",
> -						levels,
> -						num_levels);
> +						 levels,
> +						 num_levels);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -159,14 +159,15 @@ static int led_bl_parse_levels(struct device *dev,
>  		 */
>  		db = priv->default_brightness;
>  		for (i = 0 ; i < num_levels; i++) {
> -			if ((i && db > levels[i-1]) && db <= levels[i])
> +			if ((i && db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
>  		priv->default_brightness = i;
>  		priv->max_brightness = num_levels - 1;
>  		priv->levels = levels;
> -	} else if (num_levels >= 0)
> +	} else if (num_levels >= 0) {
>  		dev_warn(dev, "Not enough levels defined\n");
> +	}
>  
>  	ret = of_property_read_u32(node, "default-brightness-level", &value);
>  	if (!ret && value <= priv->max_brightness)
> @@ -208,7 +209,8 @@ static int led_bl_probe(struct platform_device *pdev)
>  	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
>  		      FB_BLANK_UNBLANK;
>  	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> -			&pdev->dev, priv, &led_bl_ops, &props);
> +						 &pdev->dev, priv, &led_bl_ops,
> +						 &props);
>  	if (IS_ERR(priv->bl_dev)) {
>  		dev_err(&pdev->dev, "Failed to register backlight\n");
>  		return PTR_ERR(priv->bl_dev);
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] backlight: led_bl: drop useless NULL initialization

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:09PM +0300, Tomi Valkeinen wrote:
> There's no need to set 'levels' to NULL.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index d4e1ce684366..c46ecdfe8b0a 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -140,7 +140,7 @@ static int led_bl_parse_levels(struct device *dev,
>  	if (num_levels > 1) {
>  		int i;
>  		unsigned int db;
> -		u32 *levels = NULL;
> +		u32 *levels;
>  
>  		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
>  				      GFP_KERNEL);
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Re: [PATCH 2/5] backlight: led_bl: drop useless NULL initialization

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:09PM +0300, Tomi Valkeinen wrote:
> There's no need to set 'levels' to NULL.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index d4e1ce684366..c46ecdfe8b0a 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -140,7 +140,7 @@ static int led_bl_parse_levels(struct device *dev,
>  	if (num_levels > 1) {
>  		int i;
>  		unsigned int db;
> -		u32 *levels = NULL;
> +		u32 *levels;
>  
>  		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
>  				      GFP_KERNEL);
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] backlight: led_bl: add led_access locking

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:10PM +0300, Tomi Valkeinen wrote:
> led_bl does not lock 'led_access' when calling led_sysfs_disable and
> led_sysfs_enable, causing the below WARN. Add the locking.
> 
> WARNING: CPU: 0 PID: 223 at drivers/leds/led-core.c:353 led_sysfs_disable+0x4c/0x5c
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I did wonder if it might be better to provide self-locking API from the
LED sub-sys but it looks like elsewhere led_sysfs_disable() is
frequently paired with led_trigger_set() (and both under the same
lock)...

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index c46ecdfe8b0a..63693c4f0883 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -216,8 +216,11 @@ static int led_bl_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->bl_dev);
>  	}
>  
> -	for (i = 0; i < priv->nb_leds; i++)
> +	for (i = 0; i < priv->nb_leds; i++) {
> +		mutex_lock(&priv->leds[i]->led_access);
>  		led_sysfs_disable(priv->leds[i]);
> +		mutex_unlock(&priv->leds[i]->led_access);
> +	}
>  
>  	backlight_update_status(priv->bl_dev);
>  
> @@ -233,8 +236,11 @@ static int led_bl_remove(struct platform_device *pdev)
>  	backlight_device_unregister(bl);
>  
>  	led_bl_power_off(priv);
> -	for (i = 0; i < priv->nb_leds; i++)
> +	for (i = 0; i < priv->nb_leds; i++) {
> +		mutex_lock(&priv->leds[i]->led_access);
>  		led_sysfs_enable(priv->leds[i]);
> +		mutex_unlock(&priv->leds[i]->led_access);
> +	}
>  
>  	return 0;
>  }
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Re: [PATCH 3/5] backlight: led_bl: add led_access locking

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:10PM +0300, Tomi Valkeinen wrote:
> led_bl does not lock 'led_access' when calling led_sysfs_disable and
> led_sysfs_enable, causing the below WARN. Add the locking.
> 
> WARNING: CPU: 0 PID: 223 at drivers/leds/led-core.c:353 led_sysfs_disable+0x4c/0x5c
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I did wonder if it might be better to provide self-locking API from the
LED sub-sys but it looks like elsewhere led_sysfs_disable() is
frequently paired with led_trigger_set() (and both under the same
lock)...

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/led_bl.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index c46ecdfe8b0a..63693c4f0883 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -216,8 +216,11 @@ static int led_bl_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->bl_dev);
>  	}
>  
> -	for (i = 0; i < priv->nb_leds; i++)
> +	for (i = 0; i < priv->nb_leds; i++) {
> +		mutex_lock(&priv->leds[i]->led_access);
>  		led_sysfs_disable(priv->leds[i]);
> +		mutex_unlock(&priv->leds[i]->led_access);
> +	}
>  
>  	backlight_update_status(priv->bl_dev);
>  
> @@ -233,8 +236,11 @@ static int led_bl_remove(struct platform_device *pdev)
>  	backlight_device_unregister(bl);
>  
>  	led_bl_power_off(priv);
> -	for (i = 0; i < priv->nb_leds; i++)
> +	for (i = 0; i < priv->nb_leds; i++) {
> +		mutex_lock(&priv->leds[i]->led_access);
>  		led_sysfs_enable(priv->leds[i]);
> +		mutex_unlock(&priv->leds[i]->led_access);
> +	}
>  
>  	return 0;
>  }
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:11PM +0300, Tomi Valkeinen wrote:
> The code that maps the LED default brightness to backlight levels has
> two issues: 1) if the default brightness is the first backlight level
> (usually 0), the code fails to find it, and 2) when the code fails to
> find a backlight level, it ends up using max_brightness + 1 as the
> default brightness.
> 
> Fix these two issues.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/led_bl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 63693c4f0883..021b5edd895c 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
>  		 */
>  		db = priv->default_brightness;
>  		for (i = 0 ; i < num_levels; i++) {
> -			if ((i && db > levels[i - 1]) && db <= levels[i])
> +			if ((i = 0 || db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
> -		priv->default_brightness = i;
> +
> +		priv->default_brightness = i < num_levels ? i : 0;

This seems a bit odd. If the LED is currently set to brighter than the
maximum brightness level why would we choose a default brightness of
zero?


Daniel.


>  		priv->max_brightness = num_levels - 1;
>  		priv->levels = levels;
>  	} else if (num_levels >= 0) {
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Re: [PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:11PM +0300, Tomi Valkeinen wrote:
> The code that maps the LED default brightness to backlight levels has
> two issues: 1) if the default brightness is the first backlight level
> (usually 0), the code fails to find it, and 2) when the code fails to
> find a backlight level, it ends up using max_brightness + 1 as the
> default brightness.
> 
> Fix these two issues.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/led_bl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 63693c4f0883..021b5edd895c 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
>  		 */
>  		db = priv->default_brightness;
>  		for (i = 0 ; i < num_levels; i++) {
> -			if ((i && db > levels[i - 1]) && db <= levels[i])
> +			if ((i == 0 || db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
> -		priv->default_brightness = i;
> +
> +		priv->default_brightness = i < num_levels ? i : 0;

This seems a bit odd. If the LED is currently set to brighter than the
maximum brightness level why would we choose a default brightness of
zero?


Daniel.


>  		priv->max_brightness = num_levels - 1;
>  		priv->levels = levels;
>  	} else if (num_levels >= 0) {
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
> more obvious code flow.

... that introduces new behaviour.

There's a couple of new behaviours here but the one that particular
attracted my attention is the disregarding the "default-brightness-level" if
there is no table. That looks like a bug to me.

Please can you add any intended changes of behaviour in the patch
header?


Daniel.




> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/led_bl.c | 63 ++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 021b5edd895c..7b3889035540 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev,
>  	int num_levels;
>  	u32 value;
>  	int ret;
> +	int i;
> +	u32 *levels;
>  
>  	if (!node)
>  		return -ENODEV;
>  
>  	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> -	if (num_levels > 1) {
> -		int i;
> -		unsigned int db;
> -		u32 *levels;
> -
> -		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
> -				      GFP_KERNEL);
> -		if (!levels)
> -			return -ENOMEM;
> -
> -		ret = of_property_read_u32_array(node, "brightness-levels",
> -						 levels,
> -						 num_levels);
> -		if (ret < 0)
> -			return ret;
> -
> -		/*
> -		 * Try to map actual LED brightness to backlight brightness
> -		 * level
> -		 */
> -		db = priv->default_brightness;
> +
> +	if (num_levels < 0)
> +		return 0;
> +
> +	if (num_levels = 0) {
> +		dev_warn(dev, "No brightness-levels defined\n");
> +		return -EINVAL;
> +	}
> +
> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
> +			      GFP_KERNEL);
> +	if (!levels)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(node, "brightness-levels",
> +					 levels,
> +					 num_levels);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->max_brightness = num_levels - 1;
> +	priv->levels = levels;
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (!ret) {
> +		priv->default_brightness = min(value, priv->max_brightness);
> +	} else {
> +		/* Map LED default-brightness to backlight brightness level */
> +		unsigned int db = priv->default_brightness;
> +
>  		for (i = 0 ; i < num_levels; i++) {
>  			if ((i = 0 || db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
>  
>  		priv->default_brightness = i < num_levels ? i : 0;
> -		priv->max_brightness = num_levels - 1;
> -		priv->levels = levels;
> -	} else if (num_levels >= 0) {
> -		dev_warn(dev, "Not enough levels defined\n");
>  	}
>  
> -	ret = of_property_read_u32(node, "default-brightness-level", &value);
> -	if (!ret && value <= priv->max_brightness)
> -		priv->default_brightness = value;
> -	else if (!ret  && value > priv->max_brightness)
> -		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
> -
>  	return 0;
>  }
>  
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Daniel Thompson]

On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
> more obvious code flow.

... that introduces new behaviour.

There's a couple of new behaviours here but the one that particular
attracted my attention is the disregarding the "default-brightness-level" if
there is no table. That looks like a bug to me.

Please can you add any intended changes of behaviour in the patch
header?


Daniel.




> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/led_bl.c | 63 ++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 021b5edd895c..7b3889035540 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev,
>  	int num_levels;
>  	u32 value;
>  	int ret;
> +	int i;
> +	u32 *levels;
>  
>  	if (!node)
>  		return -ENODEV;
>  
>  	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> -	if (num_levels > 1) {
> -		int i;
> -		unsigned int db;
> -		u32 *levels;
> -
> -		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
> -				      GFP_KERNEL);
> -		if (!levels)
> -			return -ENOMEM;
> -
> -		ret = of_property_read_u32_array(node, "brightness-levels",
> -						 levels,
> -						 num_levels);
> -		if (ret < 0)
> -			return ret;
> -
> -		/*
> -		 * Try to map actual LED brightness to backlight brightness
> -		 * level
> -		 */
> -		db = priv->default_brightness;
> +
> +	if (num_levels < 0)
> +		return 0;
> +
> +	if (num_levels == 0) {
> +		dev_warn(dev, "No brightness-levels defined\n");
> +		return -EINVAL;
> +	}
> +
> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
> +			      GFP_KERNEL);
> +	if (!levels)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(node, "brightness-levels",
> +					 levels,
> +					 num_levels);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->max_brightness = num_levels - 1;
> +	priv->levels = levels;
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (!ret) {
> +		priv->default_brightness = min(value, priv->max_brightness);
> +	} else {
> +		/* Map LED default-brightness to backlight brightness level */
> +		unsigned int db = priv->default_brightness;
> +
>  		for (i = 0 ; i < num_levels; i++) {
>  			if ((i == 0 || db > levels[i - 1]) && db <= levels[i])
>  				break;
>  		}
>  
>  		priv->default_brightness = i < num_levels ? i : 0;
> -		priv->max_brightness = num_levels - 1;
> -		priv->levels = levels;
> -	} else if (num_levels >= 0) {
> -		dev_warn(dev, "Not enough levels defined\n");
>  	}
>  
> -	ret = of_property_read_u32(node, "default-brightness-level", &value);
> -	if (!ret && value <= priv->max_brightness)
> -		priv->default_brightness = value;
> -	else if (!ret  && value > priv->max_brightness)
> -		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
> -
>  	return 0;
>  }
>  
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 20/04/2020 19:01, Daniel Thompson wrote:
> On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
>> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
>> more obvious code flow.
> 
> ... that introduces new behaviour.
> 
> There's a couple of new behaviours here but the one that particular
> attracted my attention is the disregarding the "default-brightness-level" if
> there is no table. That looks like a bug to me.

I think the previous behavior was a (minor) bug: how can there be default brightness level if there 
are no brightness levels? The led-backlight.txt is a bit lacking (another thing to improve...) but 
led-backlight mimics pwm-backlight, and pwm-backlight.txt says

default-brightness-level: The default brightness level (index into the array defined by the 
"brightness-levels" property)

But I agree, it's a change, so good to mention.

> Please can you add any intended changes of behaviour in the patch
> header?

Ok.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 20/04/2020 19:01, Daniel Thompson wrote:
> On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
>> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
>> more obvious code flow.
> 
> ... that introduces new behaviour.
> 
> There's a couple of new behaviours here but the one that particular
> attracted my attention is the disregarding the "default-brightness-level" if
> there is no table. That looks like a bug to me.

I think the previous behavior was a (minor) bug: how can there be default brightness level if there 
are no brightness levels? The led-backlight.txt is a bit lacking (another thing to improve...) but 
led-backlight mimics pwm-backlight, and pwm-backlight.txt says

default-brightness-level: The default brightness level (index into the array defined by the 
"brightness-levels" property)

But I agree, it's a change, so good to mention.

> Please can you add any intended changes of behaviour in the patch
> header?

Ok.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Tomi Valkeinen]

On 20/04/2020 18:55, Daniel Thompson wrote:
> On Fri, Apr 17, 2020 at 02:33:11PM +0300, Tomi Valkeinen wrote:
>> The code that maps the LED default brightness to backlight levels has
>> two issues: 1) if the default brightness is the first backlight level
>> (usually 0), the code fails to find it, and 2) when the code fails to
>> find a backlight level, it ends up using max_brightness + 1 as the
>> default brightness.
>>
>> Fix these two issues.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/backlight/led_bl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
>> index 63693c4f0883..021b5edd895c 100644
>> --- a/drivers/video/backlight/led_bl.c
>> +++ b/drivers/video/backlight/led_bl.c
>> @@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
>>   		 */
>>   		db = priv->default_brightness;
>>   		for (i = 0 ; i < num_levels; i++) {
>> -			if ((i && db > levels[i - 1]) && db <= levels[i])
>> +			if ((i = 0 || db > levels[i - 1]) && db <= levels[i])
>>   				break;
>>   		}
>> -		priv->default_brightness = i;
>> +
>> +		priv->default_brightness = i < num_levels ? i : 0;
> 
> This seems a bit odd. If the LED is currently set to brighter than the
> maximum brightness level why would we choose a default brightness of
> zero?

Indeed, better to set it to max.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/5] backlight: led_bl: fix led -> backlight brightness mapping

[Tomi Valkeinen]

On 20/04/2020 18:55, Daniel Thompson wrote:
> On Fri, Apr 17, 2020 at 02:33:11PM +0300, Tomi Valkeinen wrote:
>> The code that maps the LED default brightness to backlight levels has
>> two issues: 1) if the default brightness is the first backlight level
>> (usually 0), the code fails to find it, and 2) when the code fails to
>> find a backlight level, it ends up using max_brightness + 1 as the
>> default brightness.
>>
>> Fix these two issues.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/backlight/led_bl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
>> index 63693c4f0883..021b5edd895c 100644
>> --- a/drivers/video/backlight/led_bl.c
>> +++ b/drivers/video/backlight/led_bl.c
>> @@ -159,10 +159,11 @@ static int led_bl_parse_levels(struct device *dev,
>>   		 */
>>   		db = priv->default_brightness;
>>   		for (i = 0 ; i < num_levels; i++) {
>> -			if ((i && db > levels[i - 1]) && db <= levels[i])
>> +			if ((i == 0 || db > levels[i - 1]) && db <= levels[i])
>>   				break;
>>   		}
>> -		priv->default_brightness = i;
>> +
>> +		priv->default_brightness = i < num_levels ? i : 0;
> 
> This seems a bit odd. If the LED is currently set to brighter than the
> maximum brightness level why would we choose a default brightness of
> zero?

Indeed, better to set it to max.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Daniel Thompson]

On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote:
> On 20/04/2020 19:01, Daniel Thompson wrote:
> > On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
> > > led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
> > > more obvious code flow.
> > 
> > ... that introduces new behaviour.
> > 
> > There's a couple of new behaviours here but the one that particular
> > attracted my attention is the disregarding the "default-brightness-level" if
> > there is no table. That looks like a bug to me.
> 
> I think the previous behavior was a (minor) bug: how can there be default
> brightness level if there are no brightness levels?

I don't think this was a bug.

If there is no brightness table then backlight will adopt a 1:1 mapping
versus the underlying LED meaning the concept of default brightness
applies equally well whether or not a brightness table is supplied.


> The led-backlight.txt is
> a bit lacking (another thing to improve...) but led-backlight mimics
> pwm-backlight, and pwm-backlight.txt says
> 
> default-brightness-level: The default brightness level (index into the array
> defined by the "brightness-levels" property)

I think this implies we should improve the binding documentation!

The parenthetic text's main purpose is to make clear which scale should
be used when interpreting the default brightness. Just because the scale
is 1:1 doesn't render it meaningless.


Daniel.

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Daniel Thompson]

On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote:
> On 20/04/2020 19:01, Daniel Thompson wrote:
> > On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
> > > led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
> > > more obvious code flow.
> > 
> > ... that introduces new behaviour.
> > 
> > There's a couple of new behaviours here but the one that particular
> > attracted my attention is the disregarding the "default-brightness-level" if
> > there is no table. That looks like a bug to me.
> 
> I think the previous behavior was a (minor) bug: how can there be default
> brightness level if there are no brightness levels?

I don't think this was a bug.

If there is no brightness table then backlight will adopt a 1:1 mapping
versus the underlying LED meaning the concept of default brightness
applies equally well whether or not a brightness table is supplied.


> The led-backlight.txt is
> a bit lacking (another thing to improve...) but led-backlight mimics
> pwm-backlight, and pwm-backlight.txt says
> 
> default-brightness-level: The default brightness level (index into the array
> defined by the "brightness-levels" property)

I think this implies we should improve the binding documentation!

The parenthetic text's main purpose is to make clear which scale should
be used when interpreting the default brightness. Just because the scale
is 1:1 doesn't render it meaningless.


Daniel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 21/04/2020 13:48, Daniel Thompson wrote:
> On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote:
>> On 20/04/2020 19:01, Daniel Thompson wrote:
>>> On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
>>>> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
>>>> more obvious code flow.
>>>
>>> ... that introduces new behaviour.
>>>
>>> There's a couple of new behaviours here but the one that particular
>>> attracted my attention is the disregarding the "default-brightness-level" if
>>> there is no table. That looks like a bug to me.
>>
>> I think the previous behavior was a (minor) bug: how can there be default
>> brightness level if there are no brightness levels?
> 
> I don't think this was a bug.
> 
> If there is no brightness table then backlight will adopt a 1:1 mapping
> versus the underlying LED meaning the concept of default brightness
> applies equally well whether or not a brightness table is supplied.
> 
> 
>> The led-backlight.txt is
>> a bit lacking (another thing to improve...) but led-backlight mimics
>> pwm-backlight, and pwm-backlight.txt says
>>
>> default-brightness-level: The default brightness level (index into the array
>> defined by the "brightness-levels" property)
> 
> I think this implies we should improve the binding documentation!
> 
> The parenthetic text's main purpose is to make clear which scale should
> be used when interpreting the default brightness. Just because the scale
> is 1:1 doesn't render it meaningless.

If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then 
'default-brightness-level' is ignored. Which matches the binding documentation.

What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 21/04/2020 13:48, Daniel Thompson wrote:
> On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote:
>> On 20/04/2020 19:01, Daniel Thompson wrote:
>>> On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote:
>>>> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a
>>>> more obvious code flow.
>>>
>>> ... that introduces new behaviour.
>>>
>>> There's a couple of new behaviours here but the one that particular
>>> attracted my attention is the disregarding the "default-brightness-level" if
>>> there is no table. That looks like a bug to me.
>>
>> I think the previous behavior was a (minor) bug: how can there be default
>> brightness level if there are no brightness levels?
> 
> I don't think this was a bug.
> 
> If there is no brightness table then backlight will adopt a 1:1 mapping
> versus the underlying LED meaning the concept of default brightness
> applies equally well whether or not a brightness table is supplied.
> 
> 
>> The led-backlight.txt is
>> a bit lacking (another thing to improve...) but led-backlight mimics
>> pwm-backlight, and pwm-backlight.txt says
>>
>> default-brightness-level: The default brightness level (index into the array
>> defined by the "brightness-levels" property)
> 
> I think this implies we should improve the binding documentation!
> 
> The parenthetic text's main purpose is to make clear which scale should
> be used when interpreting the default brightness. Just because the scale
> is 1:1 doesn't render it meaningless.

If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then 
'default-brightness-level' is ignored. Which matches the binding documentation.

What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 21/04/2020 14:26, Tomi Valkeinen wrote:

>>> The led-backlight.txt is
>>> a bit lacking (another thing to improve...) but led-backlight mimics
>>> pwm-backlight, and pwm-backlight.txt says
>>>
>>> default-brightness-level: The default brightness level (index into the array
>>> defined by the "brightness-levels" property)
>>
>> I think this implies we should improve the binding documentation!
>>
>> The parenthetic text's main purpose is to make clear which scale should
>> be used when interpreting the default brightness. Just because the scale
>> is 1:1 doesn't render it meaningless.
> 
> If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then 
> 'default-brightness-level' is ignored. Which matches the binding documentation.
> 
> What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that.

On the other hand... If we want to use LED's (or PWM's) brightness levels directly, should the 
default brightness be defined in the LED's (or PWM's) DT node?

And only if we create a backlight brightness-levels mapping, we need to add a new property to define 
a default for those levels. Which, I think, the name implies with the "-level".

Well, in any case, there should be no harm in using 'default-brightness-level' also for the 1:1 
mapping without the 'brightness-levels'. So maybe that's the best route.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 5/5] backlight: led_bl: rewrite led_bl_parse_levels()

[Tomi Valkeinen]

On 21/04/2020 14:26, Tomi Valkeinen wrote:

>>> The led-backlight.txt is
>>> a bit lacking (another thing to improve...) but led-backlight mimics
>>> pwm-backlight, and pwm-backlight.txt says
>>>
>>> default-brightness-level: The default brightness level (index into the array
>>> defined by the "brightness-levels" property)
>>
>> I think this implies we should improve the binding documentation!
>>
>> The parenthetic text's main purpose is to make clear which scale should
>> be used when interpreting the default brightness. Just because the scale
>> is 1:1 doesn't render it meaningless.
> 
> If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then 
> 'default-brightness-level' is ignored. Which matches the binding documentation.
> 
> What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that.

On the other hand... If we want to use LED's (or PWM's) brightness levels directly, should the 
default brightness be defined in the LED's (or PWM's) DT node?

And only if we create a backlight brightness-levels mapping, we need to add a new property to define 
a default for those levels. Which, I think, the name implies with the "-level".

Well, in any case, there should be no harm in using 'default-brightness-level' also for the 1:1 
mapping without the 'brightness-levels'. So maybe that's the best route.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel