| Message ID | 20191128105508.3916-1-kbingham@kernel.org |
|---|---|
| Headers | show |
| Series | drivers/auxdisplay: Provide support for JHD1313 | expand |
Hi Kieran, On Thu, Nov 28, 2019 at 11:55 AM <kbingham@kernel.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). Also missing the "S:" entry. > 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). > 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. Thanks for the driver! Cheers, Miguel
Hi Miguel, On 28/11/2019 13:43, Miguel Ojeda wrote: > Hi Kieran, > > On Thu, Nov 28, 2019 at 11:55 AM <kbingham@kernel.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 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
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@kernel.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
On Thu, Nov 28, 2019 at 3:08 PM Kieran Bingham <kbingham@kernel.org> wrote: > > Do you have any experience in these various part numbers, to suggest > perhaps a better naming? Not at all, sorry :( > Anyway, certainly adding in I2C would be beneficial though. I'd go with whatever you think will be the best for whoever reads the option in the config. It is not a big deal in any case -- whoever wants to use this kind of drivers have to know what they are looking for :) Cheers, Miguel
From: Kieran Bingham <kbingham@kernel.org> The JHD1313 is a 16x2 LCD controller with an I2C interface. It is used in the Seeed RGB Backlight LCD [0] which has the LCD at the I2C address 0x3e. (A PCA9633 is also available at 0x62, to control the RGB backlight) This series introduces a new Vendor prefix for JHD, and introduces bindings for the LCD controller. A driver for the JHD1313 is added to the auxdisplay subsystem providing a charlcd to control the display. [0] http://wiki.seeedstudio.com/Grove-LCD_RGB_Backlight/ Because this interface is quite common, and generic - this could be potentially extended to other similar devices later, possibly with optional bindings to configure the display width and height. If so - perhaps a more generic naming for the binding/driver might be appropriate at that time. Kieran Bingham (3): dt-bindings: vendor: Add JHD LCD vendor dt-bindings: auxdisplay: Add JHD1313 bindings drivers: auxdisplay: Add JHD1313 I2C interface driver .../bindings/auxdisplay/jhd,jhd1313.yaml | 33 ++++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 4 + drivers/auxdisplay/Kconfig | 12 ++ drivers/auxdisplay/Makefile | 1 + drivers/auxdisplay/jhd1313.c | 111 ++++++++++++++++++ 6 files changed, 163 insertions(+) create mode 100644 Documentation/devicetree/bindings/auxdisplay/jhd,jhd1313.yaml create mode 100644 drivers/auxdisplay/jhd1313.c