Re: [PATCH v2 2/3] backlight: tdo24m: add the reset line gpio

Daniel Thompson [off-list ref] writes:

On Fri, Oct 13, 2017 at 09:42:48PM +0200, Robert Jarzmik wrote:

quoted

The Toppoly panels have a global reset line. Add an optional gpio
control for this line, for platforms which have the ability to drive it.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/video/backlight/tdo24m.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/backlight/tdo24m.c b/drivers/video/backlight/tdo24m.c
index e4bd63e9db6b..e4b2dfabf192 100644
--- a/drivers/video/backlight/tdo24m.c
+++ b/drivers/video/backlight/tdo24m.c

@@ -10,6 +10,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/device.h>

@@ -32,6 +33,7 @@ struct tdo24m {
 	struct spi_message	msg;
 	struct spi_transfer	xfer;
 	uint8_t			*buf;
+	struct gpio_desc	*xres;

Why do we need a write-only variable?

It's written to by devm_gpiod_get() below.

quoted

+	lcd->xres = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);

devm_gpiod_get_optional() would be better, if only for its commenting
value.

Yes, you're perfectly right.

Also, this adds a new optional property, doesn't the devicetree binding 
docs need to be update to match this?

Ah yes, that too. I'll add it for v3.

Cheers.

-- 
Robert

On Sat, Dec 16, 2017 at 12:39:50PM +0100, Robert Jarzmik wrote:

Daniel Thompson [off-list ref] writes:

quoted

On Fri, Oct 13, 2017 at 09:42:48PM +0200, Robert Jarzmik wrote:

quoted

The Toppoly panels have a global reset line. Add an optional gpio
control for this line, for platforms which have the ability to drive it.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/video/backlight/tdo24m.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/backlight/tdo24m.c b/drivers/video/backlight/tdo24m.c
index e4bd63e9db6b..e4b2dfabf192 100644
--- a/drivers/video/backlight/tdo24m.c
+++ b/drivers/video/backlight/tdo24m.c

@@ -10,6 +10,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/device.h>

@@ -32,6 +33,7 @@ struct tdo24m {
 	struct spi_message	msg;
 	struct spi_transfer	xfer;
 	uint8_t			*buf;
+	struct gpio_desc	*xres;

Why do we need a write-only variable?

It's written to by devm_gpiod_get() below.

Yes, but it not *read* anywhere because we rely on devm to release the GPIO.
Why do we need a write-only variable?

quoted

quoted

+	lcd->xres = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);

devm_gpiod_get_optional() would be better, if only for its commenting
value.

Yes, you're perfectly right.

quoted

Also, this adds a new optional property, doesn't the devicetree binding 
docs need to be update to match this?

Ah yes, that too. I'll add it for v3.

Thanks!


Daniel.