[PATCH v1 1/4] auxdisplay: lcd2s: make use of device property API

[PATCH v1 1/4] auxdisplay: lcd2s: make use of device property API

[Andy Shevchenko]

Make use of device property API in this driver so that both OF based
system and ACPI based system can use this driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/lcd2s.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index 2578b2d45439..c75e9d66aaa1 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -12,7 +12,9 @@
  *  All rights reserved.
  */
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
@@ -355,20 +357,16 @@ static const struct i2c_device_id lcd2s_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, lcd2s_i2c_id);
 
-#ifdef CONFIG_OF
 static const struct of_device_id lcd2s_of_table[] = {
 	{ .compatible = "modtronix,lcd2s" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, lcd2s_of_table);
-#endif
 
 static struct i2c_driver lcd2s_i2c_driver = {
 	.driver = {
 		.name = "lcd2s",
-#ifdef CONFIG_OF
-		.of_match_table = of_match_ptr(lcd2s_of_table),
-#endif
+		.of_match_table = lcd2s_of_table,
 	},
 	.probe = lcd2s_i2c_probe,
 	.remove = lcd2s_i2c_remove,
-- 
2.34.1

[PATCH v1 2/4] auxdisplay: lcd2s: use module_i2c_driver to simplify the code

[Andy Shevchenko]

Use the module_i2c_driver() macro to make the code smaller
and a bit simpler.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/lcd2s.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index c75e9d66aaa1..5201a621de45 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -372,24 +372,7 @@ static struct i2c_driver lcd2s_i2c_driver = {
 	.remove = lcd2s_i2c_remove,
 	.id_table = lcd2s_i2c_id,
 };
-
-static int __init lcd2s_modinit(void)
-{
-	int ret = 0;
-
-	ret = i2c_add_driver(&lcd2s_i2c_driver);
-	if (ret != 0)
-		pr_err("Failed to register lcd2s driver\n");
-
-	return ret;
-}
-module_init(lcd2s_modinit)
-
-static void __exit lcd2s_exit(void)
-{
-	i2c_del_driver(&lcd2s_i2c_driver);
-}
-module_exit(lcd2s_exit)
+module_i2c_driver(lcd2s_i2c_driver);
 
 MODULE_DESCRIPTION("LCD2S character display driver");
 MODULE_AUTHOR("Lars Poeschel");
-- 
2.34.1

[PATCH v1 3/4] auxdisplay: lcd2s: Switch to i2c ->probe_new()

[Andy Shevchenko]

The deprecated i2c ->probe() functionality doesn't work with
OF compatible strings, as it only checks for the i2c device id.
While it's not a problem right now, it would still bring a
better code. Switch to the new way of probing.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/lcd2s.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index 5201a621de45..a4503765a95d 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -288,8 +288,7 @@ static const struct charlcd_ops lcd2s_ops = {
 	.redefine_char	= lcd2s_redefine_char,
 };
 
-static int lcd2s_i2c_probe(struct i2c_client *i2c,
-				const struct i2c_device_id *id)
+static int lcd2s_i2c_probe(struct i2c_client *i2c)
 {
 	struct charlcd *lcd;
 	struct lcd2s_data *lcd2s;
@@ -368,7 +367,7 @@ static struct i2c_driver lcd2s_i2c_driver = {
 		.name = "lcd2s",
 		.of_match_table = lcd2s_of_table,
 	},
-	.probe = lcd2s_i2c_probe,
+	.probe_new = lcd2s_i2c_probe,
 	.remove = lcd2s_i2c_remove,
 	.id_table = lcd2s_i2c_id,
 };
-- 
2.34.1

[PATCH v1 4/4] auxdisplay: lcd2s: Use array size explicitly in lcd2s_gotoxy()

[Andy Shevchenko]

Currently the reading of the onstack array is confusing since two
out of three members are of different types. Let it be more clear
by explicitly set the array size, so everybody will understand that
parameters are cast to the type of the array.

While at it, add a missed space.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/lcd2s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index a4503765a95d..86103cecdd3e 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -106,7 +106,7 @@ static int lcd2s_print(struct charlcd *lcd, int c)
 static int lcd2s_gotoxy(struct charlcd *lcd, unsigned int x, unsigned int y)
 {
 	struct lcd2s_data *lcd2s = lcd->drvdata;
-	u8 buf[] = { LCD2S_CMD_CUR_POS, y + 1, x + 1};
+	u8 buf[3] = { LCD2S_CMD_CUR_POS, y + 1, x + 1 };
 
 	lcd2s_i2c_master_send(lcd2s->i2c, buf, sizeof(buf));
 
-- 
2.34.1

Re: [PATCH v1 4/4] auxdisplay: lcd2s: Use array size explicitly in lcd2s_gotoxy()

[Miguel Ojeda]

Hi Andy,

On Tue, Mar 8, 2022 at 4:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Currently the reading of the onstack array is confusing since two
> out of three members are of different types. Let it be more clear
> by explicitly set the array size, so everybody will understand that
> parameters are cast to the type of the array.

I am not sure what you mean by this, could you please elaborate why is
it more clear if the size is given?

Cheers,
Miguel

Re: [PATCH v1 4/4] auxdisplay: lcd2s: Use array size explicitly in lcd2s_gotoxy()

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 6:46 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Tue, Mar 8, 2022 at 4:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Currently the reading of the onstack array is confusing since two
> > out of three members are of different types. Let it be more clear
> > by explicitly set the array size, so everybody will understand that
> > parameters are cast to the type of the array.
>
> I am not sure what you mean by this, could you please elaborate why is
> it more clear if the size is given?

With [] and parameters not being bytes by type one needs an additional
processing step to figure that out. When I see u8 ...[3] I immediately
understand that there are 3 _bytes_. Moreover, [] is error prone when
one may add something to the buffer with the expectation that it will
work somehow (for example, with an old device you need 3, with new
device revision you need 4 bytes and passing 4 bytes breaks the old
device). Of course most of that quite likely won't happen.

If you think this change is not worth it, please, drop.


--
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] auxdisplay: lcd2s: make use of device property API

[Miguel Ojeda]

Hi Andy,

On Tue, Mar 8, 2022 at 4:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Make use of device property API in this driver so that both OF based
> system and ACPI based system can use this driver.

I applied the series to -next, but for my understanding: the device
property API was already being used, even if non-OF platforms couldn't
use the driver before this patch, right? i.e. the commit message seems
like the patch modified the calls.

Cheers,
Miguel

Re: [PATCH v1 4/4] auxdisplay: lcd2s: Use array size explicitly in lcd2s_gotoxy()

[Miguel Ojeda]

Hi Andy,

On Sat, Mar 19, 2022 at 12:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> With [] and parameters not being bytes by type one needs an additional
> processing step to figure that out. When I see u8 ...[3] I immediately
> understand that there are 3 _bytes_. Moreover, [] is error prone when
> one may add something to the buffer with the expectation that it will
> work somehow (for example, with an old device you need 3, with new
> device revision you need 4 bytes and passing 4 bytes breaks the old
> device). Of course most of that quite likely won't happen.

I agree it is better to be explicit -- I just found the commit message
very confusing, i.e. the type will always be `u8`, what is unclear is
the total length, not so much the type change.

I think the best approach when one needs an array to stay a particular
size due to some external constraint (like hardware) would be to use a
`static_assert` with a comment mentioning where the requirement comes
from. Or at least a comment above the array.

Either way, I applied this one too to -next a few days ago. Thanks!

Cheers,
Miguel

Re: [PATCH v1 1/4] auxdisplay: lcd2s: make use of device property API

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 08:02:20PM +0100, Miguel Ojeda wrote:
> On Tue, Mar 8, 2022 at 4:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Make use of device property API in this driver so that both OF based
> > system and ACPI based system can use this driver.
> 
> I applied the series to -next, but for my understanding: the device
> property API was already being used, even if non-OF platforms couldn't
> use the driver before this patch, right? i.e. the commit message seems
> like the patch modified the calls.

Not really. The device property supports the PRP0001 special ACPI device
that makes use of OF matching mechanism (via usage of OF match table).
Without above change when CONFIG_OF=n, this may not work. I.o.w. device
properties per se in use, but the mechanism of matching is not working.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] auxdisplay: lcd2s: Use array size explicitly in lcd2s_gotoxy()

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 08:15:57PM +0100, Miguel Ojeda wrote:
> On Sat, Mar 19, 2022 at 12:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > With [] and parameters not being bytes by type one needs an additional
> > processing step to figure that out. When I see u8 ...[3] I immediately
> > understand that there are 3 _bytes_. Moreover, [] is error prone when
> > one may add something to the buffer with the expectation that it will
> > work somehow (for example, with an old device you need 3, with new
> > device revision you need 4 bytes and passing 4 bytes breaks the old
> > device). Of course most of that quite likely won't happen.
> 
> I agree it is better to be explicit -- I just found the commit message
> very confusing, i.e. the type will always be `u8`, what is unclear is
> the total length, not so much the type change.
> 
> I think the best approach when one needs an array to stay a particular
> size due to some external constraint (like hardware) would be to use a
> `static_assert` with a comment mentioning where the requirement comes
> from. Or at least a comment above the array.
> 
> Either way, I applied this one too to -next a few days ago. Thanks!

Thanks!

-- 
With Best Regards,
Andy Shevchenko