[PATCH v1 0/7] auxdisplay: charlcd: Refactor memory allocation

[PATCH v1 0/7] auxdisplay: charlcd: Refactor memory allocation

[Andy Shevchenko]

The users of charlcd_alloc() call for additional memory allocation.
We may do it at the time of the main call as many other APIs do.
For this partially revert the change that brought us to the current
state of affairs, and refactor the code based on the original implementation.

Andy Shevchenko (7):
  auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to
    struct hd44780_common"
  auxdisplay: lcd2s: Allocate memory for custom data in charlcd_alloc()
  auxdisplay: hd44780: Introduce hd44780_common_free()
  auxdisplay: hd44780: Make use of hd44780_common_free()
  auxdisplay: panel: Make use of hd44780_common_free()
  auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()
  auxdisplay: hd44780: Rename hd to hdc in hd44780_common_alloc()

 drivers/auxdisplay/charlcd.c        |  5 +++--
 drivers/auxdisplay/charlcd.h        |  5 +++--
 drivers/auxdisplay/hd44780.c        | 19 ++++++-------------
 drivers/auxdisplay/hd44780_common.c | 24 ++++++++++++++++--------
 drivers/auxdisplay/hd44780_common.h |  4 +++-
 drivers/auxdisplay/lcd2s.c          | 12 ++++--------
 drivers/auxdisplay/panel.c          | 17 +++++------------
 7 files changed, 40 insertions(+), 46 deletions(-)

-- 
2.45.1.3035.g276e886db78b

[PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
hd44780_common") makes charlcd_alloc() argument-less effectively dropping
the single allocation for the struct charlcd_priv object along with
the driver specific one. Restore that behaviour here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/charlcd.c | 5 +++--
 drivers/auxdisplay/charlcd.h | 5 +++--
 drivers/auxdisplay/hd44780.c | 2 +-
 drivers/auxdisplay/lcd2s.c   | 2 +-
 drivers/auxdisplay/panel.c   | 2 +-
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 19b619376d48..09020bb8ad15 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -595,18 +595,19 @@ static int charlcd_init(struct charlcd *lcd)
 	return 0;
 }
 
-struct charlcd *charlcd_alloc(void)
+struct charlcd *charlcd_alloc(unsigned int drvdata_size)
 {
 	struct charlcd_priv *priv;
 	struct charlcd *lcd;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv) + drvdata_size, GFP_KERNEL);
 	if (!priv)
 		return NULL;
 
 	priv->esc_seq.len = -1;
 
 	lcd = &priv->lcd;
+	lcd->drvdata = priv->drvdata;
 
 	return lcd;
 }
diff --git a/drivers/auxdisplay/charlcd.h b/drivers/auxdisplay/charlcd.h
index 4d4287209d04..d10b89740bca 100644
--- a/drivers/auxdisplay/charlcd.h
+++ b/drivers/auxdisplay/charlcd.h
@@ -51,7 +51,7 @@ struct charlcd {
 		unsigned long y;
 	} addr;
 
-	void *drvdata;
+	void *drvdata;			/* Set by charlcd_alloc() */
 };
 
 /**
@@ -95,7 +95,8 @@ struct charlcd_ops {
 };
 
 void charlcd_backlight(struct charlcd *lcd, enum charlcd_onoff on);
-struct charlcd *charlcd_alloc(void);
+
+struct charlcd *charlcd_alloc(unsigned int drvdata_size);
 void charlcd_free(struct charlcd *lcd);
 
 int charlcd_register(struct charlcd *lcd);
diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index 9d0ae9c02e9b..1d67fe324341 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -226,7 +226,7 @@ static int hd44780_probe(struct platform_device *pdev)
 	if (!hdc)
 		return -ENOMEM;
 
-	lcd = charlcd_alloc();
+	lcd = charlcd_alloc(0);
 	if (!lcd)
 		goto fail1;
 
diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index f831ce762508..d573d36e3067 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -310,7 +310,7 @@ static int lcd2s_i2c_probe(struct i2c_client *i2c)
 	if (err < 0)
 		return err;
 
-	lcd = charlcd_alloc();
+	lcd = charlcd_alloc(0);
 	if (!lcd)
 		return -ENOMEM;
 
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index 6dc8798d01f9..4da142692d55 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -835,7 +835,7 @@ static void lcd_init(void)
 	if (!hdc)
 		return;
 
-	charlcd = charlcd_alloc();
+	charlcd = charlcd_alloc(0);
 	if (!charlcd) {
 		kfree(hdc);
 		return;
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Geert Uytterhoeven]

Hi Andy,

Thanks for your patch!

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct

s/The commit/Commit/

> hd44780_common") makes charlcd_alloc() argument-less effectively dropping
> the single allocation for the struct charlcd_priv object along with
> the driver specific one. Restore that behaviour here.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> Hi Andy,
> On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> 
> s/The commit/Commit/

Why? We know that we are talking about the very specific commit.

My English is not native I would appreciate a link to a material to study
the case you pointed out.

> > hd44780_common") makes charlcd_alloc() argument-less effectively dropping
> > the single allocation for the struct charlcd_priv object along with
> > the driver specific one. Restore that behaviour here.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Geert Uytterhoeven]

Hi Andy,

On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> >
> > s/The commit/Commit/
>
> Why? We know that we are talking about the very specific commit.

You can have a noun with or without an article:
  - "a commit": an unspecified commit,
  - "the commit": a specific commit, specified by context.
  - "commit 1234abcd": a specific commit, specified by what follows.

> My English is not native I would appreciate a link to a material to study
> the case you pointed out.

Neither is mine, but the use of articles is similar in English and Dutch.
(I am aware your mother tongue does not have articles ;-)

I found plenty of articles explaining cases 1 and 2.
Case 3 can be considered equivalent to "Mount Everest" in
https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > >
> > > s/The commit/Commit/
> >
> > Why? We know that we are talking about the very specific commit.
> 
> You can have a noun with or without an article:

This is not so simple :-), esp. if a noun is a weekday or a toponym.

>   - "a commit": an unspecified commit,
>   - "the commit": a specific commit, specified by context.
>   - "commit 1234abcd": a specific commit, specified by what follows.
> 
> > My English is not native I would appreciate a link to a material to study
> > the case you pointed out.
> 
> Neither is mine, but the use of articles is similar in English and Dutch.
> (I am aware your mother tongue does not have articles ;-)
> 
> I found plenty of articles explaining cases 1 and 2.
> Case 3 can be considered equivalent to "Mount Everest" in
> https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article

Okay, so you actually think that the hash and the title can be considered as
"name of a place". Hmm... I don't think it applies here. It's not a place.
Moreover some places require "the" article.

Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
for example, the sentence "The 2003 federal budget" sounds to me closer to
our case. Every year there is a federal budget, but we explicitly point out
to one and reader knows what is this. The same with the commit.

Sorry, but I am still not convinced.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Geert Uytterhoeven]

Hi Andy,

On Fri, 7 Mar 2025 at 19:57, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> > On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > > >
> > > > s/The commit/Commit/
> > >
> > > Why? We know that we are talking about the very specific commit.
> >
> > You can have a noun with or without an article:
>
> This is not so simple :-), esp. if a noun is a weekday or a toponym.
>
> >   - "a commit": an unspecified commit,
> >   - "the commit": a specific commit, specified by context.
> >   - "commit 1234abcd": a specific commit, specified by what follows.
> >
> > > My English is not native I would appreciate a link to a material to study
> > > the case you pointed out.
> >
> > Neither is mine, but the use of articles is similar in English and Dutch.
> > (I am aware your mother tongue does not have articles ;-)
> >
> > I found plenty of articles explaining cases 1 and 2.
> > Case 3 can be considered equivalent to "Mount Everest" in
> > https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article
>
> Okay, so you actually think that the hash and the title can be considered as
> "name of a place". Hmm... I don't think it applies here. It's not a place.
> Moreover some places require "the" article.

Only if they are a region, not if they are a country (yes, that's
unrelated here).

> Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
> for example, the sentence "The 2003 federal budget" sounds to me closer to
> our case. Every year there is a federal budget, but we explicitly point out
> to one and reader knows what is this. The same with the commit.
>
> Sorry, but I am still not convinced.

In "The 2003 federal budget", both "2003" and "federal" are adjectives.
In "commit 1234abcd", "1234abcd" is a name.

Cfr. "King Charles".  "The King Charles" would be used only when
putting a very special emphasis on "king".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 08:05:56PM +0100, Geert Uytterhoeven wrote:
> On Fri, 7 Mar 2025 at 19:57, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> > > On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > > > >
> > > > > s/The commit/Commit/
> > > >
> > > > Why? We know that we are talking about the very specific commit.
> > >
> > > You can have a noun with or without an article:
> >
> > This is not so simple :-), esp. if a noun is a weekday or a toponym.
> >
> > >   - "a commit": an unspecified commit,
> > >   - "the commit": a specific commit, specified by context.
> > >   - "commit 1234abcd": a specific commit, specified by what follows.
> > >
> > > > My English is not native I would appreciate a link to a material to study
> > > > the case you pointed out.
> > >
> > > Neither is mine, but the use of articles is similar in English and Dutch.
> > > (I am aware your mother tongue does not have articles ;-)
> > >
> > > I found plenty of articles explaining cases 1 and 2.
> > > Case 3 can be considered equivalent to "Mount Everest" in
> > > https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article
> >
> > Okay, so you actually think that the hash and the title can be considered as
> > "name of a place". Hmm... I don't think it applies here. It's not a place.
> > Moreover some places require "the" article.
> 
> Only if they are a region, not if they are a country (yes, that's
> unrelated here).
> 
> > Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
> > for example, the sentence "The 2003 federal budget" sounds to me closer to
> > our case. Every year there is a federal budget, but we explicitly point out
> > to one and reader knows what is this. The same with the commit.
> >
> > Sorry, but I am still not convinced.
> 
> In "The 2003 federal budget", both "2003" and "federal" are adjectives.
> In "commit 1234abcd", "1234abcd" is a name.
> 
> Cfr. "King Charles".  "The King Charles" would be used only when
> putting a very special emphasis on "king".

I have talked to the language teacher (okay, her native is not English),
and she told me that no article is for the cases of location, person, or
character. None of that category the commit falls into.

So, still not convinced.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Geert Uytterhoeven]

Hi Andy,

On Mon, 10 Mar 2025 at 09:12, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 08:05:56PM +0100, Geert Uytterhoeven wrote:
> > On Fri, 7 Mar 2025 at 19:57, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> > > > On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > > > > >
> > > > > > s/The commit/Commit/
> > > > >
> > > > > Why? We know that we are talking about the very specific commit.
> > > >
> > > > You can have a noun with or without an article:
> > >
> > > This is not so simple :-), esp. if a noun is a weekday or a toponym.
> > >
> > > >   - "a commit": an unspecified commit,
> > > >   - "the commit": a specific commit, specified by context.
> > > >   - "commit 1234abcd": a specific commit, specified by what follows.
> > > >
> > > > > My English is not native I would appreciate a link to a material to study
> > > > > the case you pointed out.
> > > >
> > > > Neither is mine, but the use of articles is similar in English and Dutch.
> > > > (I am aware your mother tongue does not have articles ;-)
> > > >
> > > > I found plenty of articles explaining cases 1 and 2.
> > > > Case 3 can be considered equivalent to "Mount Everest" in
> > > > https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article
> > >
> > > Okay, so you actually think that the hash and the title can be considered as
> > > "name of a place". Hmm... I don't think it applies here. It's not a place.
> > > Moreover some places require "the" article.
> >
> > Only if they are a region, not if they are a country (yes, that's
> > unrelated here).
> >
> > > Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
> > > for example, the sentence "The 2003 federal budget" sounds to me closer to
> > > our case. Every year there is a federal budget, but we explicitly point out
> > > to one and reader knows what is this. The same with the commit.
> > >
> > > Sorry, but I am still not convinced.
> >
> > In "The 2003 federal budget", both "2003" and "federal" are adjectives.
> > In "commit 1234abcd", "1234abcd" is a name.
> >
> > Cfr. "King Charles".  "The King Charles" would be used only when
> > putting a very special emphasis on "king".
>
> I have talked to the language teacher (okay, her native is not English),
> and she told me that no article is for the cases of location, person, or
> character. None of that category the commit falls into.
>
> So, still not convinced.

I have a hard time finding the official rule (git commits did not
exist when English grammar was written ;-)... Examples are easier to
find.  E.g. the first sentence on [1] does not start with an article:

   European route E40 is the longest European route.

[1] https://en.wikipedia.org/wiki/European_route_E40

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

On Mon, Mar 10, 2025 at 10:39:49AM +0100, Geert Uytterhoeven wrote:
> On Mon, 10 Mar 2025 at 09:12, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 08:05:56PM +0100, Geert Uytterhoeven wrote:
> > > On Fri, 7 Mar 2025 at 19:57, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> > > > > On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > > > > > >
> > > > > > > s/The commit/Commit/
> > > > > >
> > > > > > Why? We know that we are talking about the very specific commit.
> > > > >
> > > > > You can have a noun with or without an article:
> > > >
> > > > This is not so simple :-), esp. if a noun is a weekday or a toponym.
> > > >
> > > > >   - "a commit": an unspecified commit,
> > > > >   - "the commit": a specific commit, specified by context.
> > > > >   - "commit 1234abcd": a specific commit, specified by what follows.
> > > > >
> > > > > > My English is not native I would appreciate a link to a material to study
> > > > > > the case you pointed out.
> > > > >
> > > > > Neither is mine, but the use of articles is similar in English and Dutch.
> > > > > (I am aware your mother tongue does not have articles ;-)
> > > > >
> > > > > I found plenty of articles explaining cases 1 and 2.
> > > > > Case 3 can be considered equivalent to "Mount Everest" in
> > > > > https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article
> > > >
> > > > Okay, so you actually think that the hash and the title can be considered as
> > > > "name of a place". Hmm... I don't think it applies here. It's not a place.
> > > > Moreover some places require "the" article.
> > >
> > > Only if they are a region, not if they are a country (yes, that's
> > > unrelated here).
> > >
> > > > Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
> > > > for example, the sentence "The 2003 federal budget" sounds to me closer to
> > > > our case. Every year there is a federal budget, but we explicitly point out
> > > > to one and reader knows what is this. The same with the commit.
> > > >
> > > > Sorry, but I am still not convinced.
> > >
> > > In "The 2003 federal budget", both "2003" and "federal" are adjectives.
> > > In "commit 1234abcd", "1234abcd" is a name.
> > >
> > > Cfr. "King Charles".  "The King Charles" would be used only when
> > > putting a very special emphasis on "king".
> >
> > I have talked to the language teacher (okay, her native is not English),
> > and she told me that no article is for the cases of location, person, or
> > character. None of that category the commit falls into.
> >
> > So, still not convinced.
> 
> I have a hard time finding the official rule (git commits did not
> exist when English grammar was written ;-)... Examples are easier to
> find.  E.g. the first sentence on [1] does not start with an article:
> 
>    European route E40 is the longest European route.

I believe this falls to the "location" category.
Can you find something which is not related to geography?

> [1] https://en.wikipedia.org/wiki/European_route_E40

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/7] auxdisplay: charlcd: Partially revert "Move hwidth and bwidth to struct hd44780_common"

[Andy Shevchenko]

On Mon, Mar 10, 2025 at 10:39:49AM +0100, Geert Uytterhoeven wrote:
> On Mon, 10 Mar 2025 at 09:12, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 08:05:56PM +0100, Geert Uytterhoeven wrote:
> > > On Fri, 7 Mar 2025 at 19:57, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Mar 07, 2025 at 07:14:02PM +0100, Geert Uytterhoeven wrote:
> > > > > On Fri, 7 Mar 2025 at 17:57, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Mar 07, 2025 at 10:03:31AM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > The commit 2545c1c948a6 ("auxdisplay: Move hwidth and bwidth to struct
> > > > > > >
> > > > > > > s/The commit/Commit/
> > > > > >
> > > > > > Why? We know that we are talking about the very specific commit.
> > > > >
> > > > > You can have a noun with or without an article:
> > > >
> > > > This is not so simple :-), esp. if a noun is a weekday or a toponym.
> > > >
> > > > >   - "a commit": an unspecified commit,
> > > > >   - "the commit": a specific commit, specified by context.
> > > > >   - "commit 1234abcd": a specific commit, specified by what follows.
> > > > >
> > > > > > My English is not native I would appreciate a link to a material to study
> > > > > > the case you pointed out.
> > > > >
> > > > > Neither is mine, but the use of articles is similar in English and Dutch.
> > > > > (I am aware your mother tongue does not have articles ;-)
> > > > >
> > > > > I found plenty of articles explaining cases 1 and 2.
> > > > > Case 3 can be considered equivalent to "Mount Everest" in
> > > > > https://learnenglish.britishcouncil.org/grammar/a1-a2-grammar/articles-the-or-no-article
> > > >
> > > > Okay, so you actually think that the hash and the title can be considered as
> > > > "name of a place". Hmm... I don't think it applies here. It's not a place.
> > > > Moreover some places require "the" article.
> > >
> > > Only if they are a region, not if they are a country (yes, that's
> > > unrelated here).
> > >
> > > > Here https://www.butte.edu/departments/cas/tipsheets/grammar/articles.html,
> > > > for example, the sentence "The 2003 federal budget" sounds to me closer to
> > > > our case. Every year there is a federal budget, but we explicitly point out
> > > > to one and reader knows what is this. The same with the commit.
> > > >
> > > > Sorry, but I am still not convinced.
> > >
> > > In "The 2003 federal budget", both "2003" and "federal" are adjectives.
> > > In "commit 1234abcd", "1234abcd" is a name.
> > >
> > > Cfr. "King Charles".  "The King Charles" would be used only when
> > > putting a very special emphasis on "king".
> >
> > I have talked to the language teacher (okay, her native is not English),
> > and she told me that no article is for the cases of location, person, or
> > character. None of that category the commit falls into.
> >
> > So, still not convinced.
> 
> I have a hard time finding the official rule (git commits did not
> exist when English grammar was written ;-)... Examples are easier to
> find.  E.g. the first sentence on [1] does not start with an article:
> 
>    European route E40 is the longest European route.

Okay, seems AI may help here. It tells that grammatically article is needed,
but in technical texts (like this one) it's the usual case to drop it and
it's considered grammatically correct.

We both are right but from different angles. And yours seems the winner today
:)

I'll update the message accordingly.

> [1] https://en.wikipedia.org/wiki/European_route_E40

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/7] auxdisplay: lcd2s: Allocate memory for custom data in charlcd_alloc()

[Andy Shevchenko]

Allocate memory for custom data in charlcd_alloc() instead of doing that
explicitly in the driver.

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

diff --git a/drivers/auxdisplay/lcd2s.c b/drivers/auxdisplay/lcd2s.c
index d573d36e3067..d18735665706 100644
--- a/drivers/auxdisplay/lcd2s.c
+++ b/drivers/auxdisplay/lcd2s.c
@@ -301,20 +301,18 @@ static int lcd2s_i2c_probe(struct i2c_client *i2c)
 			I2C_FUNC_SMBUS_WRITE_BLOCK_DATA))
 		return -EIO;
 
-	lcd2s = devm_kzalloc(&i2c->dev, sizeof(*lcd2s), GFP_KERNEL);
-	if (!lcd2s)
-		return -ENOMEM;
-
 	/* Test, if the display is responding */
 	err = lcd2s_i2c_smbus_write_byte(i2c, LCD2S_CMD_DISPLAY_OFF);
 	if (err < 0)
 		return err;
 
-	lcd = charlcd_alloc(0);
+	lcd = charlcd_alloc(sizeof(*lcd2s));
 	if (!lcd)
 		return -ENOMEM;
 
-	lcd->drvdata = lcd2s;
+	lcd->ops = &lcd2s_ops;
+
+	lcd2s = lcd->drvdata;
 	lcd2s->i2c = i2c;
 	lcd2s->charlcd = lcd;
 
@@ -329,8 +327,6 @@ static int lcd2s_i2c_probe(struct i2c_client *i2c)
 	if (err)
 		goto fail1;
 
-	lcd->ops = &lcd2s_ops;
-
 	err = charlcd_register(lcd2s->charlcd);
 	if (err)
 		goto fail1;
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 2/7] auxdisplay: lcd2s: Allocate memory for custom data in charlcd_alloc()

[Geert Uytterhoeven]

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Allocate memory for custom data in charlcd_alloc() instead of doing that
> explicitly in the driver.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v1 3/7] auxdisplay: hd44780: Introduce hd44780_common_free()

[Andy Shevchenko]

Introduce hd44780_common_free() for symmetrical operation
to hd44780_common_alloc(). It will allow to modify the both
in the future without touching the users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/hd44780_common.c | 6 ++++++
 drivers/auxdisplay/hd44780_common.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/auxdisplay/hd44780_common.c b/drivers/auxdisplay/hd44780_common.c
index 4ef87c3118c0..3f8a496ccb8e 100644
--- a/drivers/auxdisplay/hd44780_common.c
+++ b/drivers/auxdisplay/hd44780_common.c
@@ -366,5 +366,11 @@ struct hd44780_common *hd44780_common_alloc(void)
 }
 EXPORT_SYMBOL_GPL(hd44780_common_alloc);
 
+void hd44780_common_free(struct hd44780_common *hd)
+{
+	kfree(hd);
+}
+EXPORT_SYMBOL_GPL(hd44780_common_free);
+
 MODULE_DESCRIPTION("Common functions for HD44780 (and compatibles) LCD displays");
 MODULE_LICENSE("GPL");
diff --git a/drivers/auxdisplay/hd44780_common.h b/drivers/auxdisplay/hd44780_common.h
index a16aa8c29c99..fe1386e3cf79 100644
--- a/drivers/auxdisplay/hd44780_common.h
+++ b/drivers/auxdisplay/hd44780_common.h
@@ -30,4 +30,6 @@ int hd44780_common_blink(struct charlcd *lcd, enum charlcd_onoff on);
 int hd44780_common_fontsize(struct charlcd *lcd, enum charlcd_fontsize size);
 int hd44780_common_lines(struct charlcd *lcd, enum charlcd_lines lines);
 int hd44780_common_redefine_char(struct charlcd *lcd, char *esc);
+
 struct hd44780_common *hd44780_common_alloc(void);
+void hd44780_common_free(struct hd44780_common *hd);
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 3/7] auxdisplay: hd44780: Introduce hd44780_common_free()

[Geert Uytterhoeven]

Hi Andy,

Thanks for your patch!

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Introduce hd44780_common_free() for symmetrical operation
> to hd44780_common_alloc(). It will allow to modify the both

s/the both/both

> in the future without touching the users.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v1 4/7] auxdisplay: hd44780: Make use of hd44780_common_free()

[Andy Shevchenko]

Use the symmetrical API to free the common resources.

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

diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index 1d67fe324341..ef38cb7bf13d 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -315,7 +315,7 @@ static int hd44780_probe(struct platform_device *pdev)
 fail2:
 	charlcd_free(lcd);
 fail1:
-	kfree(hdc);
+	hd44780_common_free(hdc);
 	return ret;
 }
 
@@ -326,8 +326,7 @@ static void hd44780_remove(struct platform_device *pdev)
 
 	charlcd_unregister(lcd);
 	kfree(hdc->hd44780);
-	kfree(lcd->drvdata);
-
+	hd44780_common_free(hdc);
 	charlcd_free(lcd);
 }
 
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 4/7] auxdisplay: hd44780: Make use of hd44780_common_free()

[Geert Uytterhoeven]

Hi Andy,

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Use the symmetrical API to free the common resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Perhaps fold this into [PATCH v1 3/7]?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 4/7] auxdisplay: hd44780: Make use of hd44780_common_free()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 10:05:38AM +0100, Geert Uytterhoeven wrote:
> On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Use the symmetrical API to free the common resources.

> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks!

> Perhaps fold this into [PATCH v1 3/7]?

I still think they are logically split.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/7] auxdisplay: panel: Make use of hd44780_common_free()

[Andy Shevchenko]

Use the symmetrical API to free the common resources.

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

diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index 4da142692d55..aa1d03fef22e 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -837,7 +837,7 @@ static void lcd_init(void)
 
 	charlcd = charlcd_alloc(0);
 	if (!charlcd) {
-		kfree(hdc);
+		hd44780_common_free(hdc);
 		return;
 	}
 
@@ -1691,7 +1691,7 @@ static void panel_detach(struct parport *port)
 	if (lcd.enabled) {
 		charlcd_unregister(lcd.charlcd);
 		lcd.initialized = false;
-		kfree(lcd.charlcd->drvdata);
+		hd44780_common_free(lcd.charlcd->drvdata);
 		charlcd_free(lcd.charlcd);
 		lcd.charlcd = NULL;
 	}
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 5/7] auxdisplay: panel: Make use of hd44780_common_free()

[Geert Uytterhoeven]

Hi Andy,

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Use the symmetrical API to free the common resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Perhaps fold this into [PATCH v1 3/7]?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 5/7] auxdisplay: panel: Make use of hd44780_common_free()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 10:05:56AM +0100, Geert Uytterhoeven wrote:
> On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Use the symmetrical API to free the common resources.
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks!

> Perhaps fold this into [PATCH v1 3/7]?

Since we are changing not only hd44780 parts, I made the split.
But if you are insisting I may fold them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/7] auxdisplay: panel: Make use of hd44780_common_free()

[Geert Uytterhoeven]

Hi Andy,

On Fri, 7 Mar 2025 at 18:02, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 10:05:56AM +0100, Geert Uytterhoeven wrote:
> > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > Use the symmetrical API to free the common resources.
> >
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Thanks!
>
> > Perhaps fold this into [PATCH v1 3/7]?
>
> Since we are changing not only hd44780 parts, I made the split.
> But if you are insisting I may fold them.

Up to you, you are the M ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Andy Shevchenko]

HD44780 APIs are operate on struct charlcd object. Moreover, the current users
always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
the former, so eliminate the additional allocation, make it consistent with
the rest of API and avoid duplication.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/hd44780.c        | 18 ++++++------------
 drivers/auxdisplay/hd44780_common.c | 14 ++++++++------
 drivers/auxdisplay/hd44780_common.h |  4 ++--
 drivers/auxdisplay/panel.c          | 17 +++++------------
 4 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index ef38cb7bf13d..cef42656c4b0 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -222,20 +222,17 @@ static int hd44780_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	hdc = hd44780_common_alloc();
-	if (!hdc)
-		return -ENOMEM;
-
-	lcd = charlcd_alloc(0);
+	lcd = hd44780_common_alloc();
 	if (!lcd)
-		goto fail1;
+		return -ENOMEM;
 
 	hd = kzalloc(sizeof(*hd), GFP_KERNEL);
 	if (!hd)
 		goto fail2;
 
+	hdc = lcd->drvdata;
 	hdc->hd44780 = hd;
-	lcd->drvdata = hdc;
+
 	for (i = 0; i < ifwidth; i++) {
 		hd->pins[base + i] = devm_gpiod_get_index(dev, "data", i,
 							  GPIOD_OUT_LOW);
@@ -313,9 +310,7 @@ static int hd44780_probe(struct platform_device *pdev)
 fail3:
 	kfree(hd);
 fail2:
-	charlcd_free(lcd);
-fail1:
-	hd44780_common_free(hdc);
+	hd44780_common_free(lcd);
 	return ret;
 }
 
@@ -326,8 +321,7 @@ static void hd44780_remove(struct platform_device *pdev)
 
 	charlcd_unregister(lcd);
 	kfree(hdc->hd44780);
-	hd44780_common_free(hdc);
-	charlcd_free(lcd);
+	hd44780_common_free(lcd);
 }
 
 static const struct of_device_id hd44780_of_match[] = {
diff --git a/drivers/auxdisplay/hd44780_common.c b/drivers/auxdisplay/hd44780_common.c
index 3f8a496ccb8e..fb340d18fcad 100644
--- a/drivers/auxdisplay/hd44780_common.c
+++ b/drivers/auxdisplay/hd44780_common.c
@@ -351,24 +351,26 @@ int hd44780_common_redefine_char(struct charlcd *lcd, char *esc)
 }
 EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
 
-struct hd44780_common *hd44780_common_alloc(void)
+struct charlcd *hd44780_common_alloc(void)
 {
 	struct hd44780_common *hd;
+	struct charlcd *lcd;
 
-	hd = kzalloc(sizeof(*hd), GFP_KERNEL);
-	if (!hd)
+	lcd = charlcd_alloc(sizeof(*hd));
+	if (!lcd)
 		return NULL;
 
+	hd = lcd->drvdata;
 	hd->ifwidth = 8;
 	hd->bwidth = DEFAULT_LCD_BWIDTH;
 	hd->hwidth = DEFAULT_LCD_HWIDTH;
-	return hd;
+	return lcd;
 }
 EXPORT_SYMBOL_GPL(hd44780_common_alloc);
 
-void hd44780_common_free(struct hd44780_common *hd)
+void hd44780_common_free(struct charlcd *lcd)
 {
-	kfree(hd);
+	charlcd_free(lcd);
 }
 EXPORT_SYMBOL_GPL(hd44780_common_free);
 
diff --git a/drivers/auxdisplay/hd44780_common.h b/drivers/auxdisplay/hd44780_common.h
index fe1386e3cf79..4c87f55722b6 100644
--- a/drivers/auxdisplay/hd44780_common.h
+++ b/drivers/auxdisplay/hd44780_common.h
@@ -31,5 +31,5 @@ int hd44780_common_fontsize(struct charlcd *lcd, enum charlcd_fontsize size);
 int hd44780_common_lines(struct charlcd *lcd, enum charlcd_lines lines);
 int hd44780_common_redefine_char(struct charlcd *lcd, char *esc);
 
-struct hd44780_common *hd44780_common_alloc(void);
-void hd44780_common_free(struct hd44780_common *hd);
+struct charlcd *hd44780_common_alloc(void);
+void hd44780_common_free(struct charlcd *lcd);
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index aa1d03fef22e..91ccb9789d43 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -831,18 +831,12 @@ static void lcd_init(void)
 	struct charlcd *charlcd;
 	struct hd44780_common *hdc;
 
-	hdc = hd44780_common_alloc();
-	if (!hdc)
+	charlcd = hd44780_common_alloc();
+	if (!charlcd)
 		return;
 
-	charlcd = charlcd_alloc(0);
-	if (!charlcd) {
-		hd44780_common_free(hdc);
-		return;
-	}
-
+	hdc = charlcd->drvdata;
 	hdc->hd44780 = &lcd;
-	charlcd->drvdata = hdc;
 
 	/*
 	 * Init lcd struct with load-time values to preserve exact
@@ -1664,7 +1658,7 @@ static void panel_attach(struct parport *port)
 	if (lcd.enabled)
 		charlcd_unregister(lcd.charlcd);
 err_unreg_device:
-	charlcd_free(lcd.charlcd);
+	hd44780_common_free(lcd.charlcd);
 	lcd.charlcd = NULL;
 	parport_unregister_device(pprt);
 	pprt = NULL;
@@ -1691,8 +1685,7 @@ static void panel_detach(struct parport *port)
 	if (lcd.enabled) {
 		charlcd_unregister(lcd.charlcd);
 		lcd.initialized = false;
-		hd44780_common_free(lcd.charlcd->drvdata);
-		charlcd_free(lcd.charlcd);
+		hd44780_common_free(lcd.charlcd);
 		lcd.charlcd = NULL;
 	}
 
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Geert Uytterhoeven]

Hi Andy,

Thanks for your patch!

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> HD44780 APIs are operate on struct charlcd object. Moreover, the current users

s/are/all/
s/object/objects/

> always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> the former, so eliminate the additional allocation, make it consistent with

either s/make/making/, or s/make/to make/

> the rest of API and avoid duplication.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

As the code looks correct to me:
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

> --- a/drivers/auxdisplay/hd44780_common.c
> +++ b/drivers/auxdisplay/hd44780_common.c
> @@ -351,24 +351,26 @@ int hd44780_common_redefine_char(struct charlcd *lcd, char *esc)
>  }
>  EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
>
> -struct hd44780_common *hd44780_common_alloc(void)
> +struct charlcd *hd44780_common_alloc(void)
>  {
>         struct hd44780_common *hd;
> +       struct charlcd *lcd;
>
> -       hd = kzalloc(sizeof(*hd), GFP_KERNEL);
> -       if (!hd)
> +       lcd = charlcd_alloc(sizeof(*hd));
> +       if (!lcd)
>                 return NULL;
>
> +       hd = lcd->drvdata;
>         hd->ifwidth = 8;
>         hd->bwidth = DEFAULT_LCD_BWIDTH;
>         hd->hwidth = DEFAULT_LCD_HWIDTH;
> -       return hd;
> +       return lcd;
>  }
>  EXPORT_SYMBOL_GPL(hd44780_common_alloc);

While I like the general idea, there are two things in the API I do
not like:
  1. The function is called "hd44780_common_alloc()", but returns
     a pointer to a different struct type than the name suggests,
  2. The real "struct hd44780_common" must be obtained by the caller
     from charlcd.drvdata, which is of type "void *", i.e. unsafe.

What about changing it to e.g.?

    struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)

so you can return pointers to both structs?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > HD44780 APIs are operate on struct charlcd object. Moreover, the current users
> 
> s/are/all/
> s/object/objects/
> 
> > always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> > the former, so eliminate the additional allocation, make it consistent with
> 
> either s/make/making/, or s/make/to make/
> 
> > the rest of API and avoid duplication.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> As the code looks correct to me:
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks I have corrected all grammar issues in the commit messages except one in
the first patch which I do not understand.

...

> While I like the general idea, there are two things in the API I do
> not like:
>   1. The function is called "hd44780_common_alloc()", but returns
>      a pointer to a different struct type than the name suggests,
>   2. The real "struct hd44780_common" must be obtained by the caller
>      from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> 
> What about changing it to e.g.?
> 
>     struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> 
> so you can return pointers to both structs?

I don't like this prototype as it seems and feels confusing. Also note,
the APIs are using struct charlcd while being in the hd44780 namespace.
perhaps better to rename the function to hd44780_common_and_lcd_alloc()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Geert Uytterhoeven]

Hi Andy,

On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > HD44780 APIs are operate on struct charlcd object. Moreover, the current users
> >
> > s/are/all/
> > s/object/objects/
> >
> > > always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> > > the former, so eliminate the additional allocation, make it consistent with
> >
> > either s/make/making/, or s/make/to make/
> >
> > > the rest of API and avoid duplication.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > As the code looks correct to me:
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Thanks I have corrected all grammar issues in the commit messages except one in
> the first patch which I do not understand.
>
> ...
>
> > While I like the general idea, there are two things in the API I do
> > not like:
> >   1. The function is called "hd44780_common_alloc()", but returns
> >      a pointer to a different struct type than the name suggests,
> >   2. The real "struct hd44780_common" must be obtained by the caller
> >      from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> >
> > What about changing it to e.g.?
> >
> >     struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> >
> > so you can return pointers to both structs?
>
> I don't like this prototype as it seems and feels confusing. Also note,
> the APIs are using struct charlcd while being in the hd44780 namespace.
> perhaps better to rename the function to hd44780_common_and_lcd_alloc()?

That is one option.

Another option would be to add a "charlcd *lcd" member to
struct hd44780_common.

That would allow to fix the other odd part in the API:

    -void hd44780_common_free(struct charlcd *lcd)
    +void hd44780_common_free(struct hd4480_common *hd)


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote:
> On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > While I like the general idea, there are two things in the API I do
> > > not like:
> > >   1. The function is called "hd44780_common_alloc()", but returns
> > >      a pointer to a different struct type than the name suggests,
> > >   2. The real "struct hd44780_common" must be obtained by the caller
> > >      from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> > >
> > > What about changing it to e.g.?
> > >
> > >     struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> > >
> > > so you can return pointers to both structs?
> >
> > I don't like this prototype as it seems and feels confusing. Also note,
> > the APIs are using struct charlcd while being in the hd44780 namespace.
> > perhaps better to rename the function to hd44780_common_and_lcd_alloc()?
> 
> That is one option.
> 
> Another option would be to add a "charlcd *lcd" member to
> struct hd44780_common.
> 
> That would allow to fix the other odd part in the API:
> 
>     -void hd44780_common_free(struct charlcd *lcd)
>     +void hd44780_common_free(struct hd4480_common *hd)

This I like better. In a separate patch I think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Geert Uytterhoeven]

Hi Andy,

On Fri, 7 Mar 2025 at 20:01, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote:
> > On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > While I like the general idea, there are two things in the API I do
> > > > not like:
> > > >   1. The function is called "hd44780_common_alloc()", but returns
> > > >      a pointer to a different struct type than the name suggests,
> > > >   2. The real "struct hd44780_common" must be obtained by the caller
> > > >      from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> > > >
> > > > What about changing it to e.g.?
> > > >
> > > >     struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> > > >
> > > > so you can return pointers to both structs?
> > >
> > > I don't like this prototype as it seems and feels confusing. Also note,
> > > the APIs are using struct charlcd while being in the hd44780 namespace.
> > > perhaps better to rename the function to hd44780_common_and_lcd_alloc()?
> >
> > That is one option.
> >
> > Another option would be to add a "charlcd *lcd" member to
> > struct hd44780_common.
> >
> > That would allow to fix the other odd part in the API:
> >
> >     -void hd44780_common_free(struct charlcd *lcd)
> >     +void hd44780_common_free(struct hd4480_common *hd)
>
> This I like better. In a separate patch I think?

Fine for me...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 08:06:37PM +0100, Geert Uytterhoeven wrote:
> On Fri, 7 Mar 2025 at 20:01, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote:
> > > On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > While I like the general idea, there are two things in the API I do
> > > > > not like:
> > > > >   1. The function is called "hd44780_common_alloc()", but returns
> > > > >      a pointer to a different struct type than the name suggests,
> > > > >   2. The real "struct hd44780_common" must be obtained by the caller
> > > > >      from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> > > > >
> > > > > What about changing it to e.g.?
> > > > >
> > > > >     struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> > > > >
> > > > > so you can return pointers to both structs?
> > > >
> > > > I don't like this prototype as it seems and feels confusing. Also note,
> > > > the APIs are using struct charlcd while being in the hd44780 namespace.
> > > > perhaps better to rename the function to hd44780_common_and_lcd_alloc()?
> > >
> > > That is one option.
> > >
> > > Another option would be to add a "charlcd *lcd" member to
> > > struct hd44780_common.
> > >
> > > That would allow to fix the other odd part in the API:
> > >
> > >     -void hd44780_common_free(struct charlcd *lcd)
> > >     +void hd44780_common_free(struct hd4480_common *hd)
> >
> > This I like better. In a separate patch I think?
> 
> Fine for me...

Thanks, I will check what can I do, but currently it's not
a high priority to me.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 7/7] auxdisplay: hd44780: Rename hd to hdc in hd44780_common_alloc()

[Andy Shevchenko]

The hd44780_common_alloc() uses hd for local variable while
the respective header uses hdc, rename to make it consistent
and avoid potential confuse with the drivers that use both
for different reasons. No functional changes intended.

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

diff --git a/drivers/auxdisplay/hd44780_common.c b/drivers/auxdisplay/hd44780_common.c
index fb340d18fcad..1792fe2a4460 100644
--- a/drivers/auxdisplay/hd44780_common.c
+++ b/drivers/auxdisplay/hd44780_common.c
@@ -353,17 +353,17 @@ EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
 
 struct charlcd *hd44780_common_alloc(void)
 {
-	struct hd44780_common *hd;
+	struct hd44780_common *hdc;
 	struct charlcd *lcd;
 
-	lcd = charlcd_alloc(sizeof(*hd));
+	lcd = charlcd_alloc(sizeof(*hdc));
 	if (!lcd)
 		return NULL;
 
-	hd = lcd->drvdata;
-	hd->ifwidth = 8;
-	hd->bwidth = DEFAULT_LCD_BWIDTH;
-	hd->hwidth = DEFAULT_LCD_HWIDTH;
+	hdc = lcd->drvdata;
+	hdc->ifwidth = 8;
+	hdc->bwidth = DEFAULT_LCD_BWIDTH;
+	hdc->hwidth = DEFAULT_LCD_HWIDTH;
 	return lcd;
 }
 EXPORT_SYMBOL_GPL(hd44780_common_alloc);
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 0/7] auxdisplay: charlcd: Refactor memory allocation

[Andy Shevchenko]

On Mon, Feb 24, 2025 at 07:27:37PM +0200, Andy Shevchenko wrote:
> The users of charlcd_alloc() call for additional memory allocation.
> We may do it at the time of the main call as many other APIs do.
> For this partially revert the change that brought us to the current
> state of affairs, and refactor the code based on the original implementation.

Geert, if you are okay with this series, I want to proceed with it today.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/7] auxdisplay: charlcd: Refactor memory allocation

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 12:30:59PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 24, 2025 at 07:27:37PM +0200, Andy Shevchenko wrote:
> > The users of charlcd_alloc() call for additional memory allocation.
> > We may do it at the time of the main call as many other APIs do.
> > For this partially revert the change that brought us to the current
> > state of affairs, and refactor the code based on the original implementation.
> 
> Geert, if you are okay with this series, I want to proceed with it today.

Pushed to my review and testing queue, thanks!

P.S. If you think this shouldn't have been done, please tell me.

-- 
With Best Regards,
Andy Shevchenko