[PATCH v1 0/4] backlight: hx8357: Clean up and make OF-independent

[PATCH v1 0/4] backlight: hx8357: Clean up and make OF-independent

[Andy Shevchenko]

A few ad-hoc cleanups and one patch to make driver OF-independent.

Andy Shevchenko (4):
  backlight: hx8357: Make use of device properties
  backlight: hx8357: Move OF table closer to its consumer
  backlight: hx8357: Make use of dev_err_probe()
  backlight: hx8357: Utilise temporary variable for struct device

 drivers/video/backlight/hx8357.c | 57 +++++++++++++++-----------------
 1 file changed, 27 insertions(+), 30 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Andy Shevchenko]

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Include mod_devicetable.h explicitly to replace the dropped of.h
which included mod_devicetable.h indirectly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/video/backlight/hx8357.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index bf18337ff0c2..c7fd10d55c5d 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -8,9 +8,9 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/lcd.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 
 #define HX8357_NUM_IM_PINS	3
@@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
 	.get_power	= hx8357_get_power,
 };
 
+typedef int (*hx8357_init)(struct lcd_device *);
+
 static const struct of_device_id hx8357_dt_ids[] = {
 	{
 		.compatible = "himax,hx8357",
@@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	struct lcd_device *lcdev;
 	struct hx8357_data *lcd;
-	const struct of_device_id *match;
+	hx8357_init init;
 	int i, ret;
 
 	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
@@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi)
 
 	lcd->spi = spi;
 
-	match = of_match_device(hx8357_dt_ids, &spi->dev);
-	if (!match || !match->data)
+	init = device_get_match_data(dev);
+	if (!init)
 		return -EINVAL;
 
 	lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
@@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi)
 
 	hx8357_lcd_reset(lcdev);
 
-	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
+	ret = init(lcdev);
 	if (ret) {
 		dev_err(&spi->dev, "Couldn't initialize panel\n");
 		return ret;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Javier Martinez Canillas]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

Hello Andy,

> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
>
> Include mod_devicetable.h explicitly to replace the dropped of.h
> which included mod_devicetable.h indirectly.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/video/backlight/hx8357.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index bf18337ff0c2..c7fd10d55c5d 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -8,9 +8,9 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/lcd.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_device.h>
> +#include <linux/property.h>
>  #include <linux/spi/spi.h>
>  
>  #define HX8357_NUM_IM_PINS	3
> @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
>  	.get_power	= hx8357_get_power,
>  };
>  
> +typedef int (*hx8357_init)(struct lcd_device *);
> +

This kind of typedef usage is frowned upon in the Linux coding style [0]
(per my understanding at least) and indeed in my opinion it makes harder
to grep.

[0] https://www.kernel.org/doc/Documentation/process/coding-style.rst

>  static const struct of_device_id hx8357_dt_ids[] = {
>  	{
>  		.compatible = "himax,hx8357",
> @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	struct lcd_device *lcdev;
>  	struct hx8357_data *lcd;
> -	const struct of_device_id *match;
> +	hx8357_init init;
>  	int i, ret;
>  
>  	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
> @@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi)
>  
>  	lcd->spi = spi;
>  
> -	match = of_match_device(hx8357_dt_ids, &spi->dev);
> -	if (!match || !match->data)
> +	init = device_get_match_data(dev);
> +	if (!init)
>  		return -EINVAL;
>  
>  	lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> @@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi)
>  
>  	hx8357_lcd_reset(lcdev);
>  
> -	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);

This is what I mean, before it was clear what was stored in match->data.
But after you changes, what is returned by the device_get_match_data()
function is opaque and you need to look at the typedef hx8357_init to
figure that out.

No strong opinion though and I see other drivers doing the same (but no
other driver in drivers/video/backlight).

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Andy Shevchenko]

On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

...

> > +typedef int (*hx8357_init)(struct lcd_device *);
> 
> This kind of typedef usage is frowned upon in the Linux coding style [0]
> (per my understanding at least) and indeed in my opinion it makes harder
> to grep.
> 
> [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst

Thanks for pointing this out. However, this piece does _not_ clarify typedef:s
for function pointers which I personally find a good to have.

...

> > -	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
> 
> This is what I mean, before it was clear what was stored in match->data.
> But after you changes, what is returned by the device_get_match_data()
> function is opaque and you need to look at the typedef hx8357_init to
> figure that out.

The above is so ugly in my opinion, that justifies using typedef:s even
if you are quite skeptical about them.

...

> No strong opinion though and I see other drivers doing the same (but no
> other driver in drivers/video/backlight).
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Daniel Thompson]

On Sun, Jan 21, 2024 at 03:48:05PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> ...
>
> > > +typedef int (*hx8357_init)(struct lcd_device *);
> >
> > This kind of typedef usage is frowned upon in the Linux coding style [0]
> > (per my understanding at least) and indeed in my opinion it makes harder
> > to grep.
> >
> > [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst
>
> Thanks for pointing this out. However, this piece does _not_ clarify typedef:s
> for function pointers which I personally find a good to have.
>
> ...
>
> > > -	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
> >
> > This is what I mean, before it was clear what was stored in match->data.
> > But after you changes, what is returned by the device_get_match_data()
> > function is opaque and you need to look at the typedef hx8357_init to
> > figure that out.
>
> The above is so ugly in my opinion, that justifies using typedef:s even
> if you are quite skeptical about them.

FWIW I was pretty skeptical about it to. Largely because the three
touchs (typedef, variable initialization, use) spread things
around a bit too much.

Can we at least name the type to make it obvious that it is a function
pointer? Something like hx8357_init_fn .


Daniel.

Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Daniel Thompson]

On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote:
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
>
> Include mod_devicetable.h explicitly to replace the dropped of.h
> which included mod_devicetable.h indirectly.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/video/backlight/hx8357.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index bf18337ff0c2..c7fd10d55c5d 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -8,9 +8,9 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/lcd.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_device.h>
> +#include <linux/property.h>
>  #include <linux/spi/spi.h>
>
>  #define HX8357_NUM_IM_PINS	3
> @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
>  	.get_power	= hx8357_get_power,
>  };
>
> +typedef int (*hx8357_init)(struct lcd_device *);
> +
>  static const struct of_device_id hx8357_dt_ids[] = {
>  	{
>  		.compatible = "himax,hx8357",
> @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	struct lcd_device *lcdev;
>  	struct hx8357_data *lcd;
> -	const struct of_device_id *match;
> +	hx8357_init init;

As somewhere else in this thread, I'd find this a lot more readable
as:
	hx8357_init_fn init_fn;

Other than that, LGTM.


Daniel.

Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties

[Andy Shevchenko]

On Wed, Jan 24, 2024 at 05:19:53PM +0000, Daniel Thompson wrote:
> On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote:

...

> > +typedef int (*hx8357_init)(struct lcd_device *);

> > +	hx8357_init init;
> 
> As somewhere else in this thread, I'd find this a lot more readable
> as:
> 	hx8357_init_fn init_fn;

I agree with you, dunno why I haven't added _fn in the first place...

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer

[Andy Shevchenko]

Move OF table near to the user.

While at it, drop comma at terminator entry.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/video/backlight/hx8357.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index c7fd10d55c5d..8709d9141cfb 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -566,19 +566,6 @@ static struct lcd_ops hx8357_ops = {
 
 typedef int (*hx8357_init)(struct lcd_device *);
 
-static const struct of_device_id hx8357_dt_ids[] = {
-	{
-		.compatible = "himax,hx8357",
-		.data = hx8357_lcd_init,
-	},
-	{
-		.compatible = "himax,hx8369",
-		.data = hx8369_lcd_init,
-	},
-	{},
-};
-MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
-
 static int hx8357_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -640,6 +627,19 @@ static int hx8357_probe(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id hx8357_dt_ids[] = {
+	{
+		.compatible = "himax,hx8357",
+		.data = hx8357_lcd_init,
+	},
+	{
+		.compatible = "himax,hx8369",
+		.data = hx8369_lcd_init,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
+
 static struct spi_driver hx8357_driver = {
 	.probe  = hx8357_probe,
 	.driver = {
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer

[Javier Martinez Canillas]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> Move OF table near to the user.
>
> While at it, drop comma at terminator entry.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/video/backlight/hx8357.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index c7fd10d55c5d..8709d9141cfb 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -566,19 +566,6 @@ static struct lcd_ops hx8357_ops = {
>  
>  typedef int (*hx8357_init)(struct lcd_device *);
>  
> -static const struct of_device_id hx8357_dt_ids[] = {
> -	{
> -		.compatible = "himax,hx8357",
> -		.data = hx8357_lcd_init,
> -	},
> -	{
> -		.compatible = "himax,hx8369",
> -		.data = hx8369_lcd_init,
> -	},
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
> -
>  static int hx8357_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -640,6 +627,19 @@ static int hx8357_probe(struct spi_device *spi)
>  	return 0;
>  }
>  
> +static const struct of_device_id hx8357_dt_ids[] = {
> +	{
> +		.compatible = "himax,hx8357",
> +		.data = hx8357_lcd_init,
> +	},
> +	{
> +		.compatible = "himax,hx8369",
> +		.data = hx8369_lcd_init,
> +	},
> +	{}

While at it, maybe add the { /* sentinel */ } convention to the last entry ?

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer

[Andy Shevchenko]

On Mon, Jan 15, 2024 at 09:22:19AM +0100, Javier Martinez Canillas wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

...

> > +	{}
> 
> While at it, maybe add the { /* sentinel */ } convention to the last entry ?

Maybe. Is it a common for this subsystem?

...

> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer

[Daniel Thompson]

On Sun, Jan 21, 2024 at 03:49:23PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 15, 2024 at 09:22:19AM +0100, Javier Martinez Canillas wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> ...
>
> > > +	{}
> >
> > While at it, maybe add the { /* sentinel */ } convention to the last entry ?
>
> Maybe. Is it a common for this subsystem?

I'd answer that slightly differently. Backlight does not aspire to be
special regarding this sort of thing. If this pattern is becoming common
within the rest of the kernel then its absolutely fine to use it here!

There are certainly backlights that use this convention... although they
are not yet the majority.


Daniel.

Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer

[Daniel Thompson]

On Sun, Jan 14, 2024 at 05:25:09PM +0200, Andy Shevchenko wrote:
> Move OF table near to the user.
>
> While at it, drop comma at terminator entry.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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


Daniel.

[PATCH v1 3/4] backlight: hx8357: Make use of dev_err_probe()

[Andy Shevchenko]

Simplify the error handling in probe function by switching from
dev_err() to dev_err_probe().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/video/backlight/hx8357.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 8709d9141cfb..fbe02fd73272 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -579,10 +579,8 @@ static int hx8357_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	ret = spi_setup(spi);
-	if (ret < 0) {
-		dev_err(&spi->dev, "SPI setup failed.\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "SPI setup failed.\n");
 
 	lcd->spi = spi;
 
@@ -617,10 +615,8 @@ static int hx8357_probe(struct spi_device *spi)
 	hx8357_lcd_reset(lcdev);
 
 	ret = init(lcdev);
-	if (ret) {
-		dev_err(&spi->dev, "Couldn't initialize panel\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Couldn't initialize panel\n");
 
 	dev_info(&spi->dev, "Panel probed\n");
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 3/4] backlight: hx8357: Make use of dev_err_probe()

[Javier Martinez Canillas]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> Simplify the error handling in probe function by switching from
> dev_err() to dev_err_probe().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v1 3/4] backlight: hx8357: Make use of dev_err_probe()

[Daniel Thompson]

On Sun, Jan 14, 2024 at 05:25:10PM +0200, Andy Shevchenko wrote:
> Simplify the error handling in probe function by switching from
> dev_err() to dev_err_probe().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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


Daniel.

[PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device

[Andy Shevchenko]

We have a temporary variable to keep pointer to struct device.
Utilise it inside the ->probe() implementation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/video/backlight/hx8357.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index fbe02fd73272..e4dc76f25f8e 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -574,7 +574,7 @@ static int hx8357_probe(struct spi_device *spi)
 	hx8357_init init;
 	int i, ret;
 
-	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
+	lcd = devm_kzalloc(dev, sizeof(*lcd), GFP_KERNEL);
 	if (!lcd)
 		return -ENOMEM;
 
@@ -604,8 +604,7 @@ static int hx8357_probe(struct spi_device *spi)
 			gpiod_set_consumer_name(lcd->im_pins->desc[i], "im_pins");
 	}
 
-	lcdev = devm_lcd_device_register(&spi->dev, "mxsfb", &spi->dev, lcd,
-					&hx8357_ops);
+	lcdev = devm_lcd_device_register(dev, "mxsfb", dev, lcd, &hx8357_ops);
 	if (IS_ERR(lcdev)) {
 		ret = PTR_ERR(lcdev);
 		return ret;
@@ -618,7 +617,7 @@ static int hx8357_probe(struct spi_device *spi)
 	if (ret)
 		return dev_err_probe(dev, ret, "Couldn't initialize panel\n");
 
-	dev_info(&spi->dev, "Panel probed\n");
+	dev_info(dev, "Panel probed\n");
 
 	return 0;
 }
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device

[Javier Martinez Canillas]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> We have a temporary variable to keep pointer to struct device.
> Utilise it inside the ->probe() implementation.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device

[Daniel Thompson]

On Sun, Jan 14, 2024 at 05:25:11PM +0200, Andy Shevchenko wrote:
> We have a temporary variable to keep pointer to struct device.
> Utilise it inside the ->probe() implementation.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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


Daniel.

Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device

[Andy Shevchenko]

On Wed, Jan 24, 2024 at 05:25:07PM +0000, Daniel Thompson wrote:
> On Sun, Jan 14, 2024 at 05:25:11PM +0200, Andy Shevchenko wrote:

...

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

Thank you for the review, I will address comments and send a new version
at the end of the next week I hope.

-- 
With Best Regards,
Andy Shevchenko