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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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 <[email protected]>
Daniel.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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
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 <[email protected]>
>
> 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.