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
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 <[email protected]>
---
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
Simplify the error handling in probe function by switching from
dev_err() to dev_err_probe().
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Move OF table near to the user.
While at it, drop comma at terminator entry.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
We have a temporary variable to keep pointer to struct device.
Utilise it inside the ->probe() implementation.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Andy Shevchenko <[email protected]> writes:
> Move OF table near to the user.
>
> While at it, drop comma at terminator entry.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Andy Shevchenko <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Andy Shevchenko <[email protected]> writes:
> Simplify the error handling in probe function by switching from
> dev_err() to dev_err_probe().
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Andy Shevchenko <[email protected]> writes:
> We have a temporary variable to keep pointer to struct device.
> Utilise it inside the ->probe() implementation.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On Mon, Jan 15, 2024 at 09:22:19AM +0100, Javier Martinez Canillas wrote:
> Andy Shevchenko <[email protected]> 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 <[email protected]>
Thank you for the review!
--
With Best Regards,
Andy Shevchenko
On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote:
> Andy Shevchenko <[email protected]> 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 <[email protected]>
Thank you for the review!
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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.
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 <[email protected]> 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.
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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
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 <[email protected]>
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