Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
On Wed, Jan 24, 2024 at 05:19:53PM +, 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
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
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
> ---
> 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
> #include
> #include
> +#include
> #include
> -#include
> -#include
> +#include
> #include
>
> #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 = >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
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 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
On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote: > Andy Shevchenko 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 Thank you for the review! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
Andy Shevchenko 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
> ---
> 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
> #include
> #include
> +#include
> #include
> -#include
> -#include
> +#include
> #include
>
> #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 = >dev;
> struct lcd_device *lcdev;
> struct hx8357_data *lcd;
> - const struct of_device_id *match;
> + hx8357_init init;
> int i, ret;
>
> lcd = devm_kzalloc(>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, >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
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
[PATCH v1 1/4] backlight: hx8357: Make use of device properties
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
---
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
#include
#include
+#include
#include
-#include
-#include
+#include
#include
#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 = >dev;
struct lcd_device *lcdev;
struct hx8357_data *lcd;
- const struct of_device_id *match;
+ hx8357_init init;
int i, ret;
lcd = devm_kzalloc(>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, >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(>dev, "Couldn't initialize panel\n");
return ret;
--
2.43.0.rc1.1.gbec44491f096
