[PATCH 1/2] backlight: hx8357: switch to using gpiod API

[PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Dmitry Torokhov]

Switch the driver from legacy gpio API that is deprecated to the newer
gpiod API that respects line polarities described in ACPI/DT.

This makes driver use standard property name for the reset gpio
("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
to also recognize the legacy name and keep compatibility with older
DTSes.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

All preparation gpiolib work to handle legacy names and polarity quirks
has landed in mainline...

 drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 9b50bc96e00f..a93e14adb846 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -6,11 +6,12 @@
  */
 
 #include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/lcd.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>
 
 #define HX8357_NUM_IM_PINS	3
@@ -83,8 +84,8 @@
 #define HX8369_SET_GAMMA_CURVE_RELATED		0xe0
 
 struct hx8357_data {
-	unsigned		im_pins[HX8357_NUM_IM_PINS];
-	unsigned		reset;
+	struct gpio_desc	*im_pins[HX8357_NUM_IM_PINS];
+	struct gpio_desc	*reset;
 	struct spi_device	*spi;
 	int			state;
 	bool			use_im_pins;
@@ -321,11 +322,11 @@ static void hx8357_lcd_reset(struct lcd_device *lcdev)
 	struct hx8357_data *lcd = lcd_get_data(lcdev);
 
 	/* Reset the screen */
-	gpio_set_value(lcd->reset, 1);
+	gpiod_set_value_cansleep(lcd->reset, 0);
 	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 0);
+	gpiod_set_value_cansleep(lcd->reset, 1);
 	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 1);
+	gpiod_set_value_cansleep(lcd->reset, 0);
 
 	/* The controller needs 120ms to recover from reset */
 	msleep(120);
@@ -341,9 +342,9 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
 	 * wires
 	 */
 	if (lcd->use_im_pins) {
-		gpio_set_value_cansleep(lcd->im_pins[0], 1);
-		gpio_set_value_cansleep(lcd->im_pins[1], 0);
-		gpio_set_value_cansleep(lcd->im_pins[2], 1);
+		gpiod_set_value_cansleep(lcd->im_pins[0], 1);
+		gpiod_set_value_cansleep(lcd->im_pins[1], 0);
+		gpiod_set_value_cansleep(lcd->im_pins[2], 1);
 	}
 
 	ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
@@ -601,48 +602,39 @@ static int hx8357_probe(struct spi_device *spi)
 	if (!match || !match->data)
 		return -EINVAL;
 
-	lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
-	if (!gpio_is_valid(lcd->reset)) {
-		dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
-		return -EINVAL;
-	}
-
-	ret = devm_gpio_request_one(&spi->dev, lcd->reset,
-				    GPIOF_OUT_INIT_HIGH,
-				    "hx8357-reset");
+	lcd->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	ret = PTR_ERR_OR_ZERO(lcd->reset);
 	if (ret) {
-		dev_err(&spi->dev,
-			"failed to request gpio %d: %d\n",
-			lcd->reset, ret);
-		return -EINVAL;
+		dev_err(&spi->dev, "failed to request reset gpio: %d\n", ret);
+		return ret;
 	}
 
-	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
-		lcd->use_im_pins = 1;
-
-		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
-			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
-							    "im-gpios", i);
-			if (lcd->im_pins[i] == -EPROBE_DEFER) {
-				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
-				return -EPROBE_DEFER;
-			}
-			if (!gpio_is_valid(lcd->im_pins[i])) {
-				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
-				return -EINVAL;
+	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
+
+	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
+		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
+						       "im", i, GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
+		if (ret) {
+			if (ret == -ENOENT) {
+				if (i == 0)
+					break;
+				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
+				ret = -EINVAL;
+			} if (ret == -EPROBE_DEFER) {
+				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
+					 i);
+			} else {
+				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
+					i, ret);
 			}
 
-			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
-						    GPIOF_OUT_INIT_LOW,
-						    "im_pins");
-			if (ret) {
-				dev_err(&spi->dev, "failed to request gpio %d: %d\n",
-					lcd->im_pins[i], ret);
-				return -EINVAL;
-			}
+			return ret;
 		}
-	} else {
-		lcd->use_im_pins = 0;
+
+		gpiod_set_consumer_name(lcd->im_pins[i], "im_pins");
+
+		lcd->use_im_pins = true;
 	}
 
 	lcdev = devm_lcd_device_register(&spi->dev, "mxsfb", &spi->dev, lcd,
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Dmitry Torokhov]

Switch the driver from legacy gpio API that is deprecated to the newer
gpiod API that respects line polarities described in ACPI/DT.

This makes driver use standard property name for the reset gpio
("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
to also recognize the legacy name and keep compatibility with older
DTSes.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

All preparation gpiolib work to handle legacy names and polarity quirks
has landed in mainline...

 drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 9b50bc96e00f..a93e14adb846 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -6,11 +6,12 @@
  */
 
 #include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/lcd.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>
 
 #define HX8357_NUM_IM_PINS	3
@@ -83,8 +84,8 @@
 #define HX8369_SET_GAMMA_CURVE_RELATED		0xe0
 
 struct hx8357_data {
-	unsigned		im_pins[HX8357_NUM_IM_PINS];
-	unsigned		reset;
+	struct gpio_desc	*im_pins[HX8357_NUM_IM_PINS];
+	struct gpio_desc	*reset;
 	struct spi_device	*spi;
 	int			state;
 	bool			use_im_pins;
@@ -321,11 +322,11 @@ static void hx8357_lcd_reset(struct lcd_device *lcdev)
 	struct hx8357_data *lcd = lcd_get_data(lcdev);
 
 	/* Reset the screen */
-	gpio_set_value(lcd->reset, 1);
+	gpiod_set_value_cansleep(lcd->reset, 0);
 	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 0);
+	gpiod_set_value_cansleep(lcd->reset, 1);
 	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 1);
+	gpiod_set_value_cansleep(lcd->reset, 0);
 
 	/* The controller needs 120ms to recover from reset */
 	msleep(120);
@@ -341,9 +342,9 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
 	 * wires
 	 */
 	if (lcd->use_im_pins) {
-		gpio_set_value_cansleep(lcd->im_pins[0], 1);
-		gpio_set_value_cansleep(lcd->im_pins[1], 0);
-		gpio_set_value_cansleep(lcd->im_pins[2], 1);
+		gpiod_set_value_cansleep(lcd->im_pins[0], 1);
+		gpiod_set_value_cansleep(lcd->im_pins[1], 0);
+		gpiod_set_value_cansleep(lcd->im_pins[2], 1);
 	}
 
 	ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
@@ -601,48 +602,39 @@ static int hx8357_probe(struct spi_device *spi)
 	if (!match || !match->data)
 		return -EINVAL;
 
-	lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
-	if (!gpio_is_valid(lcd->reset)) {
-		dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
-		return -EINVAL;
-	}
-
-	ret = devm_gpio_request_one(&spi->dev, lcd->reset,
-				    GPIOF_OUT_INIT_HIGH,
-				    "hx8357-reset");
+	lcd->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	ret = PTR_ERR_OR_ZERO(lcd->reset);
 	if (ret) {
-		dev_err(&spi->dev,
-			"failed to request gpio %d: %d\n",
-			lcd->reset, ret);
-		return -EINVAL;
+		dev_err(&spi->dev, "failed to request reset gpio: %d\n", ret);
+		return ret;
 	}
 
-	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
-		lcd->use_im_pins = 1;
-
-		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
-			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
-							    "im-gpios", i);
-			if (lcd->im_pins[i] == -EPROBE_DEFER) {
-				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
-				return -EPROBE_DEFER;
-			}
-			if (!gpio_is_valid(lcd->im_pins[i])) {
-				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
-				return -EINVAL;
+	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
+
+	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
+		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
+						       "im", i, GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
+		if (ret) {
+			if (ret == -ENOENT) {
+				if (i == 0)
+					break;
+				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
+				ret = -EINVAL;
+			} if (ret == -EPROBE_DEFER) {
+				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
+					 i);
+			} else {
+				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
+					i, ret);
 			}
 
-			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
-						    GPIOF_OUT_INIT_LOW,
-						    "im_pins");
-			if (ret) {
-				dev_err(&spi->dev, "failed to request gpio %d: %d\n",
-					lcd->im_pins[i], ret);
-				return -EINVAL;
-			}
+			return ret;
 		}
-	} else {
-		lcd->use_im_pins = 0;
+
+		gpiod_set_consumer_name(lcd->im_pins[i], "im_pins");
+
+		lcd->use_im_pins = true;
 	}
 
 	lcdev = devm_lcd_device_register(&spi->dev, "mxsfb", &spi->dev, lcd,
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 2/2] backlight: hx8357: stop using of-specific APIs

[Dmitry Torokhov]

There is no need for this driver to be OF-specific, so switch it to
use device_get_match_data() and stop including various of-related
headers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/video/backlight/hx8357.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index a93e14adb846..2e162a70c1ce 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -10,8 +10,8 @@
 #include <linux/gpio/consumer.h>
 #include <linux/lcd.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 
 #define HX8357_NUM_IM_PINS	3
@@ -581,11 +581,15 @@ MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
 
 static int hx8357_probe(struct spi_device *spi)
 {
+	int (*lcd_init_fn)(struct lcd_device *);
 	struct lcd_device *lcdev;
 	struct hx8357_data *lcd;
-	const struct of_device_id *match;
 	int i, ret;
 
+	lcd_init_fn = device_get_match_data(&spi->dev);
+	if (!lcd_init_fn)
+		return -EINVAL;
+
 	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
 	if (!lcd)
 		return -ENOMEM;
@@ -598,10 +602,6 @@ static int hx8357_probe(struct spi_device *spi)
 
 	lcd->spi = spi;
 
-	match = of_match_device(hx8357_dt_ids, &spi->dev);
-	if (!match || !match->data)
-		return -EINVAL;
-
 	lcd->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
 	ret = PTR_ERR_OR_ZERO(lcd->reset);
 	if (ret) {
@@ -647,7 +647,7 @@ static int hx8357_probe(struct spi_device *spi)
 
 	hx8357_lcd_reset(lcdev);
 
-	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
+	ret = lcd_init_fn(lcdev);
 	if (ret) {
 		dev_err(&spi->dev, "Couldn't initialize panel\n");
 		return ret;
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 2/2] backlight: hx8357: stop using of-specific APIs

[Dmitry Torokhov]

There is no need for this driver to be OF-specific, so switch it to
use device_get_match_data() and stop including various of-related
headers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/video/backlight/hx8357.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index a93e14adb846..2e162a70c1ce 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -10,8 +10,8 @@
 #include <linux/gpio/consumer.h>
 #include <linux/lcd.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 
 #define HX8357_NUM_IM_PINS	3
@@ -581,11 +581,15 @@ MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
 
 static int hx8357_probe(struct spi_device *spi)
 {
+	int (*lcd_init_fn)(struct lcd_device *);
 	struct lcd_device *lcdev;
 	struct hx8357_data *lcd;
-	const struct of_device_id *match;
 	int i, ret;
 
+	lcd_init_fn = device_get_match_data(&spi->dev);
+	if (!lcd_init_fn)
+		return -EINVAL;
+
 	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
 	if (!lcd)
 		return -ENOMEM;
@@ -598,10 +602,6 @@ static int hx8357_probe(struct spi_device *spi)
 
 	lcd->spi = spi;
 
-	match = of_match_device(hx8357_dt_ids, &spi->dev);
-	if (!match || !match->data)
-		return -EINVAL;
-
 	lcd->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
 	ret = PTR_ERR_OR_ZERO(lcd->reset);
 	if (ret) {
@@ -647,7 +647,7 @@ static int hx8357_probe(struct spi_device *spi)
 
 	hx8357_lcd_reset(lcdev);
 
-	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
+	ret = lcd_init_fn(lcdev);
 	if (ret) {
 		dev_err(&spi->dev, "Couldn't initialize panel\n");
 		return ret;
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Daniel Thompson]

On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> Switch the driver from legacy gpio API that is deprecated to the newer
> gpiod API that respects line polarities described in ACPI/DT.
>
> This makes driver use standard property name for the reset gpio
> ("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
> to also recognize the legacy name and keep compatibility with older
> DTSes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> All preparation gpiolib work to handle legacy names and polarity quirks
> has landed in mainline...
>
>  drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index 9b50bc96e00f..a93e14adb846 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> [snip]
> -	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
> -		lcd->use_im_pins = 1;
> -
> -		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> -			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> -							    "im-gpios", i);
> -			if (lcd->im_pins[i] == -EPROBE_DEFER) {
> -				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
> -				return -EPROBE_DEFER;
> -			}
> -			if (!gpio_is_valid(lcd->im_pins[i])) {
> -				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> -				return -EINVAL;
> +	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
> +
> +	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> +		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
> +						       "im", i, GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
> +		if (ret) {
> +			if (ret == -ENOENT) {
> +				if (i == 0)
> +					break;
> +				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> +				ret = -EINVAL;
> +			} if (ret == -EPROBE_DEFER) {
> +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> +					 i);
> +			} else {
> +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> +					i, ret);
>  			}

These last two clauses should be updated to return dev_err_probe(...)
instead.

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


Daniel.

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Daniel Thompson]

On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> Switch the driver from legacy gpio API that is deprecated to the newer
> gpiod API that respects line polarities described in ACPI/DT.
>
> This makes driver use standard property name for the reset gpio
> ("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
> to also recognize the legacy name and keep compatibility with older
> DTSes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> All preparation gpiolib work to handle legacy names and polarity quirks
> has landed in mainline...
>
>  drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index 9b50bc96e00f..a93e14adb846 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> [snip]
> -	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
> -		lcd->use_im_pins = 1;
> -
> -		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> -			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> -							    "im-gpios", i);
> -			if (lcd->im_pins[i] == -EPROBE_DEFER) {
> -				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
> -				return -EPROBE_DEFER;
> -			}
> -			if (!gpio_is_valid(lcd->im_pins[i])) {
> -				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> -				return -EINVAL;
> +	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
> +
> +	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> +		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
> +						       "im", i, GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
> +		if (ret) {
> +			if (ret == -ENOENT) {
> +				if (i == 0)
> +					break;
> +				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> +				ret = -EINVAL;
> +			} if (ret == -EPROBE_DEFER) {
> +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> +					 i);
> +			} else {
> +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> +					i, ret);
>  			}

These last two clauses should be updated to return dev_err_probe(...)
instead.

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


Daniel.

Re: [PATCH 2/2] backlight: hx8357: stop using of-specific APIs

[Daniel Thompson]

On Tue, Jan 31, 2023 at 02:57:07PM -0800, Dmitry Torokhov wrote:
> There is no need for this driver to be OF-specific, so switch it to
> use device_get_match_data() and stop including various of-related
> headers.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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


Daniel.

Re: [PATCH 2/2] backlight: hx8357: stop using of-specific APIs

[Daniel Thompson]

On Tue, Jan 31, 2023 at 02:57:07PM -0800, Dmitry Torokhov wrote:
> There is no need for this driver to be OF-specific, so switch it to
> use device_get_match_data() and stop including various of-related
> headers.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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


Daniel.

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 11:35:32AM +0000, Daniel Thompson wrote:
> On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> > Switch the driver from legacy gpio API that is deprecated to the newer
> > gpiod API that respects line polarities described in ACPI/DT.
> >
> > This makes driver use standard property name for the reset gpio
> > ("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
> > to also recognize the legacy name and keep compatibility with older
> > DTSes.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >
> > All preparation gpiolib work to handle legacy names and polarity quirks
> > has landed in mainline...
> >
> >  drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
> >  1 file changed, 37 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> > index 9b50bc96e00f..a93e14adb846 100644
> > --- a/drivers/video/backlight/hx8357.c
> > +++ b/drivers/video/backlight/hx8357.c
> > [snip]
> > -	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
> > -		lcd->use_im_pins = 1;
> > -
> > -		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> > -			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> > -							    "im-gpios", i);
> > -			if (lcd->im_pins[i] == -EPROBE_DEFER) {
> > -				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
> > -				return -EPROBE_DEFER;
> > -			}
> > -			if (!gpio_is_valid(lcd->im_pins[i])) {
> > -				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> > -				return -EINVAL;
> > +	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
> > +
> > +	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> > +		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
> > +						       "im", i, GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
> > +		if (ret) {
> > +			if (ret == -ENOENT) {
> > +				if (i == 0)
> > +					break;
> > +				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> > +				ret = -EINVAL;
> > +			} if (ret == -EPROBE_DEFER) {

I see I miss "else" here...

> > +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> > +					 i);
> > +			} else {
> > +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> > +					i, ret);
> >  			}
> 
> These last two clauses should be updated to return dev_err_probe(...)
> instead.
> 
> With that change:
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

So you want to actually suppress the deferral message unless debug
printks are enabled? So you want this to read:


		if (ret) {
			if (ret == -ENOENT) {
				if (i == 0)
					break;

				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
				return -EINVAL;
			}

			return dev_err_probe(&spi->dev, ret,
					     "failed to request im gpio[%d]\n", i);
		}


Did I get it right?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 11:35:32AM +0000, Daniel Thompson wrote:
> On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> > Switch the driver from legacy gpio API that is deprecated to the newer
> > gpiod API that respects line polarities described in ACPI/DT.
> >
> > This makes driver use standard property name for the reset gpio
> > ("reset-gpios" vs "gpios-reset"), however there is a quirk in gpiolib
> > to also recognize the legacy name and keep compatibility with older
> > DTSes.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >
> > All preparation gpiolib work to handle legacy names and polarity quirks
> > has landed in mainline...
> >
> >  drivers/video/backlight/hx8357.c | 82 ++++++++++++++------------------
> >  1 file changed, 37 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> > index 9b50bc96e00f..a93e14adb846 100644
> > --- a/drivers/video/backlight/hx8357.c
> > +++ b/drivers/video/backlight/hx8357.c
> > [snip]
> > -	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
> > -		lcd->use_im_pins = 1;
> > -
> > -		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> > -			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> > -							    "im-gpios", i);
> > -			if (lcd->im_pins[i] == -EPROBE_DEFER) {
> > -				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
> > -				return -EPROBE_DEFER;
> > -			}
> > -			if (!gpio_is_valid(lcd->im_pins[i])) {
> > -				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> > -				return -EINVAL;
> > +	gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
> > +
> > +	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> > +		lcd->im_pins[i] = devm_gpiod_get_index(&spi->dev,
> > +						       "im", i, GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(lcd->im_pins[i]);
> > +		if (ret) {
> > +			if (ret == -ENOENT) {
> > +				if (i == 0)
> > +					break;
> > +				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> > +				ret = -EINVAL;
> > +			} if (ret == -EPROBE_DEFER) {

I see I miss "else" here...

> > +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> > +					 i);
> > +			} else {
> > +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> > +					i, ret);
> >  			}
> 
> These last two clauses should be updated to return dev_err_probe(...)
> instead.
> 
> With that change:
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

So you want to actually suppress the deferral message unless debug
printks are enabled? So you want this to read:


		if (ret) {
			if (ret == -ENOENT) {
				if (i == 0)
					break;

				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
				return -EINVAL;
			}

			return dev_err_probe(&spi->dev, ret,
					     "failed to request im gpio[%d]\n", i);
		}


Did I get it right?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Daniel Thompson]

On Mon, Feb 06, 2023 at 08:15:56PM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 06, 2023 at 11:35:32AM +0000, Daniel Thompson wrote:
> > On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> > > +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> > > +					 i);
> > > +			} else {
> > > +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> > > +					i, ret);
> > >  			}
> >
> > These last two clauses should be updated to return dev_err_probe(...)
> > instead.
> >
> > With that change:
> > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> So you want to actually suppress the deferral message unless debug
> printks are enabled? So you want this to read:
>
>
> 		if (ret) {
> 			if (ret == -ENOENT) {
> 				if (i == 0)
> 					break;
>
> 				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> 				return -EINVAL;
> 			}
>
> 			return dev_err_probe(&spi->dev, ret,
> 					     "failed to request im gpio[%d]\n", i);
> 		}
>
> Did I get it right?

LGTM.


Daniel.

Re: [PATCH 1/2] backlight: hx8357: switch to using gpiod API

[Daniel Thompson]

On Mon, Feb 06, 2023 at 08:15:56PM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 06, 2023 at 11:35:32AM +0000, Daniel Thompson wrote:
> > On Tue, Jan 31, 2023 at 02:57:06PM -0800, Dmitry Torokhov wrote:
> > > +				dev_info(&spi->dev, "im gpio[%d] is not here yet, deferring the probe\n",
> > > +					 i);
> > > +			} else {
> > > +				dev_err(&spi->dev, "failed to request im gpio[%d]: %d\n",
> > > +					i, ret);
> > >  			}
> >
> > These last two clauses should be updated to return dev_err_probe(...)
> > instead.
> >
> > With that change:
> > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> So you want to actually suppress the deferral message unless debug
> printks are enabled? So you want this to read:
>
>
> 		if (ret) {
> 			if (ret == -ENOENT) {
> 				if (i == 0)
> 					break;
>
> 				dev_err(&spi->dev, "Missing im gpios[%d]\n", i);
> 				return -EINVAL;
> 			}
>
> 			return dev_err_probe(&spi->dev, ret,
> 					     "failed to request im gpio[%d]\n", i);
> 		}
>
> Did I get it right?

LGTM.


Daniel.