Message-ID: <22736d5f-b078-eb1f-8ee7-bff2f7774a6f@kernel.org>
Date:   Thu, 28 Nov 2019 14:34:21 +0000
From:   Kieran Bingham <kbingham@...nel.org>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Simon Goda <simon.goda@...los.com>
Subject: Re: [PATCH 3/3] drivers: auxdisplay: Add JHD1313 I2C interface driver

Hello again,

On 28/11/2019 14:08, Kieran Bingham wrote:
> Hi Miguel,
> 
> On 28/11/2019 13:43, Miguel Ojeda wrote:
>> Hi Kieran,
>>
>> On Thu, Nov 28, 2019 at 11:55 AM <kbingham@...nel.org> wrote:
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 8f075b866aaf..640f099ff7fb 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -8837,6 +8837,10 @@ S:       Maintained
>>>  F:     Documentation/admin-guide/jfs.rst
>>>  F:     fs/jfs/
>>>
>>> +JHD1313 LCD Dispaly driver
>>
>> Typo (and it should be all uppercase; and perhaps "Display" is not
>> needed given LCD is there; but see also comments on the title below).
> 
> Good spot, and good point "Liquid Crystal Display Display" is a bit
> redundant.
> 
>> Also missing the "S:" entry.
> 
> Ah yes, I think we can add the following here:
> 
> S:      Maintained
> 
>>> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
>>> index b8313a04422d..cfc61c1abdee 100644
>>> --- a/drivers/auxdisplay/Kconfig
>>> +++ b/drivers/auxdisplay/Kconfig
>>> @@ -27,6 +27,18 @@ config HD44780
>>>           kernel and started at boot.
>>>           If you don't understand what all this is about, say N.
>>>
>>> +config JHD1313
>>> +       tristate "JHD1313 Character LCD support"
>>> +       depends on I2C
>>> +       select CHARLCD
>>> +       ---help---
>>> +         Enable support for Character LCDs using a JHD1313 controller on I2C.
>>> +         The LCD is accessible through the /dev/lcd char device (10, 156).
>>> +         This code can either be compiled as a module, or linked into the
>>> +         kernel and started at boot.
>>> +         This supports the LCD panel on the Grove 16x2 LCD series.
>>> +         If you don't understand what all this is about, say N.
>>
>> Would it be useful/worth for users to put "Grove series" and/or "I2C"
>> in the tristate title? (i.e. like the help section explains and also
>> like the MODULE_DESCRIPTION says).
> 
> I have struggled with the difference between the definition of this
> driver (which supports a 'JHD1313') vs the 'product' that uses it (the
> Grove display), and I also suspect that as it's just an implementation
> of a more generic part, so I'm also contemplating renaming this. For
> instance, the products at:
> 
> 	http://www.jhdlcd.com.cn/162character.html
> 
> All seem to reference an SPLC780D controller, and have varying
> properties of backlight colour, and text colour.
> 
> Thus I highly suspect that the JHD1313 is just a specific
> variant/implementation of the range which is utilised by the Grove LCD
> board. (which makes jhd1313 a bad name for this driver...)
> 
> Do you have any experience in these various part numbers, to suggest
> perhaps a better naming?
> 
> Or I wonder if anyone has any relevant contacts at either Seeed, or JHD
> or any other related part here...
> 
> Now that I track down the SPLC780D, I see:
> 
> 	https://www.newhavendisplay.com/app_notes/SPLC780D.pdf

Following the rabbit-hole even deeper, I have now discovered that Seed
have now put up a /real/ JHD1313 datasheet:

https://github.com/SeeedDocument/Grove_LCD_RGB_Backlight/blob/master/res/JHD1313%20FP-RGB-1%201.4.pdf

However, that leads down a further path - as it references in the block
diagram on page 12 that the I2C component (which is really what this
driver is about) is an AIP31068L (or equivalence) which handles the
bridging between the I2C interface, and the segment driver.

(And this is yet again, another newhaven-display part)

So that's pushing me towards the idea that this is in fact a
newhaven,aip31068l device driver.

Perhaps we would support compatibles such as "jhd,jhd1313",
"jhd,jhd1214" as convenience aliases though ?


Any thoughts from anyone?


> Which leads to 'yet another vendor', and actually I already see
> newhaven,.* in vendor-prefixes.yaml ... however this looks like it
> relates to just the 'LCD driver', and does not provide the I2C interface
> - so the device is of course more complex than a single part.
> 
> Anyway, certainly adding in I2C would be beneficial though.
> 
> 
>>> diff --git a/drivers/auxdisplay/jhd1313.c b/drivers/auxdisplay/jhd1313.c
>>> new file mode 100644
>>> index 000000000000..abf270e128ac
>>> --- /dev/null
>>> +++ b/drivers/auxdisplay/jhd1313.c
>>> @@ -0,0 +1,111 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +
>>
>> Unconventional (AFAIK) empty line.
> 
> Ooops, I can drop that one :-D
> 
>> Thanks for the driver!
> 
> You're welcome. The charlcd_ framework makes it easy to add an LCD over
> I2C, and I hope this can be useful to others.
> 
>> Cheers,
>> Miguel

--
Kieran

Powered by blists - more mailing lists