| Message ID | 20220209091204.2513437-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm: Add driver for Solomon SSD130X OLED displays | expand |
Hi Javier, On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > The ssd130x driver only provides the core support for these devices but it > does not have any bus transport logic. Add a driver to interface over SPI. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Thanks for your patch! > --- /dev/null > +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c > +static const struct of_device_id ssd130x_of_match[] = { > + { > + .compatible = "solomon,ssd1305fb-spi", This needs an update to the DT bindings. Hence this may be a good time to deprecate the existing "solomon,ssd130*fb-i2c" compatible values, and switch to "solomon,ssd130*fb" instead, for both I2C and SPI. Of course the I2C subdriver still has to bind against the old values, too, for backwards compatibility. > + .data = (void *)&ssd130x_ssd1305_deviceinfo, The casts are not needed. > + }, > + { > + .compatible = "solomon,ssd1306fb-spi", > + .data = (void *)&ssd130x_ssd1306_deviceinfo, > + }, > + { > + .compatible = "solomon,ssd1307fb-spi", > + .data = (void *)&ssd130x_ssd1307_deviceinfo, > + }, > + { > + .compatible = "solomon,ssd1309fb-spi", > + .data = (void *)&ssd130x_ssd1309_deviceinfo, > + }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, ssd130x_of_match); > + 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
On 2/9/22 13:25, Geert Uytterhoeven wrote: > Hi Javier, > > On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> The ssd130x driver only provides the core support for these devices but it >> does not have any bus transport logic. Add a driver to interface over SPI. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Thanks for your patch! > >> --- /dev/null >> +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c > >> +static const struct of_device_id ssd130x_of_match[] = { >> + { >> + .compatible = "solomon,ssd1305fb-spi", > > This needs an update to the DT bindings. Yes, I know. Didn't feel like it, because the patch is a WIP anyways (I haven't tested it but was included just for illustration purposes). If someone confirms that works then I will include a proper DT binding in the next revision. > Hence this may be a good time to deprecate the existing > "solomon,ssd130*fb-i2c" compatible values, and switch to > "solomon,ssd130*fb" instead, for both I2C and SPI. Is this the preferred approach ? Asking because most of the drivers I know use this -$bus suffix. From a device <--> driver matching point of view, shouldn't be an issue to have two different drivers to use the same compatible strings, as long as these are for different buses. Since AFAIK the match only happens within the same struct bus_type. But I wonder if this could cause issues in other places, for example the module loading. IIRC the OF modaliases don't include the device type. If instead the drivers were old platform drivers and have an i2c_device_id and spi_device_id tables, then using the same device name would not be an issue due the modalias having a i2c: and spi: prefix to make a distinction. What I think we should do is drop the "fb" part, since that seemed to me that was included because it was an fbdev driver. And not really hardware description. > Of course the I2C subdriver still has to bind against the old values, > too, for backwards compatibility. > Yes, agreed. >> + .data = (void *)&ssd130x_ssd1305_deviceinfo, > > The casts are not needed. > Ok. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, Feb 09, 2022 at 10:12:04AM +0100, Javier Martinez Canillas wrote: > The ssd130x driver only provides the core support for these devices but it > does not have any bus transport logic. Add a driver to interface over SPI. Thanks! Same set of comments as per I²C patch.
On Wed, Feb 09, 2022 at 01:25:46PM +0100, Geert Uytterhoeven wrote: > On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: ... > > + .compatible = "solomon,ssd1305fb-spi", > > This needs an update to the DT bindings. > Hence this may be a good time to deprecate the existing > "solomon,ssd130*fb-i2c" compatible values, and switch to > "solomon,ssd130*fb" instead, for both I2C and SPI. Agree! > Of course the I2C subdriver still has to bind against the old values, > too, for backwards compatibility.
On Wed, Feb 09, 2022 at 02:04:17PM +0100, Javier Martinez Canillas wrote: > On 2/9/22 13:25, Geert Uytterhoeven wrote: > > On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas > > <javierm@redhat.com> wrote: > Yes, I know. Didn't feel like it, because the patch is a WIP anyways > (I haven't tested it but was included just for illustration purposes). > > If someone confirms that works then I will include a proper DT binding > in the next revision. In a few weeks I hope I can do this. But you know Linux is almost always broken (*) on the certain embedded device if nobody keep an eye each rc cycle. It might take time to return it in shape first. *) Speaking out of my own experience with device(s) that I possess.
On 2/9/22 16:16, Andy Shevchenko wrote: > On Wed, Feb 09, 2022 at 10:12:04AM +0100, Javier Martinez Canillas wrote: >> The ssd130x driver only provides the core support for these devices but it >> does not have any bus transport logic. Add a driver to interface over SPI. > > Thanks! > > Same set of comments as per I²C patch. > Sure, thanks for your feedback! Best regards,
On 2/9/22 16:17, Andy Shevchenko wrote: > On Wed, Feb 09, 2022 at 01:25:46PM +0100, Geert Uytterhoeven wrote: >> On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas >> <javierm@redhat.com> wrote: > > ... > >>> + .compatible = "solomon,ssd1305fb-spi", >> >> This needs an update to the DT bindings. >> Hence this may be a good time to deprecate the existing >> "solomon,ssd130*fb-i2c" compatible values, and switch to >> "solomon,ssd130*fb" instead, for both I2C and SPI. > > Agree! > Did you see my comment about automatic module loading ? I think that would be an issue if we use the same compatible for both I2C and SPI drivers. Best regards,
On 2/9/22 16:19, Andy Shevchenko wrote: > On Wed, Feb 09, 2022 at 02:04:17PM +0100, Javier Martinez Canillas wrote: >> On 2/9/22 13:25, Geert Uytterhoeven wrote: >>> On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas >>> <javierm@redhat.com> wrote: > >> Yes, I know. Didn't feel like it, because the patch is a WIP anyways >> (I haven't tested it but was included just for illustration purposes). >> >> If someone confirms that works then I will include a proper DT binding >> in the next revision. > > In a few weeks I hope I can do this. > Thanks, there's no rush. I just included this for your convenience because you mentioned that have an display with a SPI interface. > But you know Linux is almost always broken (*) on the certain embedded device > if nobody keep an eye each rc cycle. It might take time to return it in shape > first. > > *) Speaking out of my own experience with device(s) that I possess. > Unfortunately that's my experience too. I'll keep this patch in the series and marked as RFC. But in v4 will extend the DT binding to mention the SPI devices. Best regards,
Hi Javier, On Wed, Feb 9, 2022 at 5:07 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 2/9/22 16:17, Andy Shevchenko wrote: > > On Wed, Feb 09, 2022 at 01:25:46PM +0100, Geert Uytterhoeven wrote: > >> On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas > >> <javierm@redhat.com> wrote: > > > > ... > > > >>> + .compatible = "solomon,ssd1305fb-spi", > >> > >> This needs an update to the DT bindings. > >> Hence this may be a good time to deprecate the existing > >> "solomon,ssd130*fb-i2c" compatible values, and switch to > >> "solomon,ssd130*fb" instead, for both I2C and SPI. > > > > Agree! > > > > Did you see my comment about automatic module loading ? I think > that would be an issue if we use the same compatible for both > I2C and SPI drivers. We have several drivers that have a core and separate i2c and spi wrappers, see e.g. $ git grep -w ltc2947_of_match drivers/hwmon/ltc2947-core.c:const struct of_device_id ltc2947_of_match[] = { drivers/hwmon/ltc2947-core.c:EXPORT_SYMBOL_GPL(ltc2947_of_match); drivers/hwmon/ltc2947-core.c:MODULE_DEVICE_TABLE(of, ltc2947_of_match); drivers/hwmon/ltc2947-i2c.c: .of_match_table = ltc2947_of_match, drivers/hwmon/ltc2947-spi.c: .of_match_table = ltc2947_of_match, drivers/hwmon/ltc2947.h:extern const struct of_device_id ltc2947_of_match[]; Are they all broken? 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
On Wed, Feb 09, 2022 at 05:07:03PM +0100, Javier Martinez Canillas wrote: > On 2/9/22 16:17, Andy Shevchenko wrote: > > On Wed, Feb 09, 2022 at 01:25:46PM +0100, Geert Uytterhoeven wrote: > >> On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas > >> <javierm@redhat.com> wrote: > > > > ... > > > >>> + .compatible = "solomon,ssd1305fb-spi", > >> > >> This needs an update to the DT bindings. > >> Hence this may be a good time to deprecate the existing > >> "solomon,ssd130*fb-i2c" compatible values, and switch to > >> "solomon,ssd130*fb" instead, for both I2C and SPI. > > > > Agree! > > > > Did you see my comment about automatic module loading ? I think > that would be an issue if we use the same compatible for both > I2C and SPI drivers. Bug in OF code? This is not a problem in ACPI AFAICT.
On Wed, Feb 09, 2022 at 05:25:03PM +0100, Geert Uytterhoeven wrote: > On Wed, Feb 9, 2022 at 5:07 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: ... > Are they all broken? I guess it's incorrect question. The one we need to ask is is OF code broken? B/c ACPI can easily cope with this (they are different buses, can't clash).
Sorry for the long reply but I don't really know how to make it shorter. On 2/9/22 17:25, Geert Uytterhoeven wrote: > Hi Javier, [snip] >> Did you see my comment about automatic module loading ? I think >> that would be an issue if we use the same compatible for both >> I2C and SPI drivers. > > We have several drivers that have a core and separate i2c and spi > wrappers, see e.g. > > $ git grep -w ltc2947_of_match > drivers/hwmon/ltc2947-core.c:const struct of_device_id ltc2947_of_match[] = { > drivers/hwmon/ltc2947-core.c:EXPORT_SYMBOL_GPL(ltc2947_of_match); > drivers/hwmon/ltc2947-core.c:MODULE_DEVICE_TABLE(of, ltc2947_of_match); > drivers/hwmon/ltc2947-i2c.c: .of_match_table = ltc2947_of_match, > drivers/hwmon/ltc2947-spi.c: .of_match_table = ltc2947_of_match, > drivers/hwmon/ltc2947.h:extern const struct of_device_id ltc2947_of_match[]; > > Are they all broken? > It doesn't have an easy answer. It depends on what device ID tables have to match, what modalias are present in the kernel modules and for what buses are since not all subsystem reports the modalias uevent in the same manner. Let's take this particular driver for example, the module aliases are the following for each kernel module: $ modinfo drivers/hwmon/ltc2947-core.ko | grep alias alias: of:N*T*Cadi,ltc2947C* alias: of:N*T*Cadi,ltc2947 $ modinfo drivers/hwmon/ltc2947-i2c.ko | grep alias alias: i2c:ltc2947 $ modinfo drivers/hwmon/ltc2947-spi.ko | grep alias alias: spi:ltc2947 The DT node will just contain a node with compatible = "adi,ltc2947", and depending if the device is on a I2C or SPI bus, the OF subsystem will do a device registration with the proper bus_type. If is SPI, the subsystem always report a modalias uevent of the type "spi:ltc2947", even if the device was registered by OF. So for SPI the proper module will be loaded since it contains an alias "spi:ltc2947". But I2C does report a proper OF modalias, so it will report instead "of:N*T*Cadi,ltc2947" which will match the core module but not the I2C. The I2C ltc2947-i2c.ko module is missing a "of:N*T*Cadi,ltc2947" alias and the module loading likely isn't working and needs to be done manually. Conversely, if ltc2947-spi.ko only add "of:N*T*Cadi,ltc2947", then the automatic module loading wouldn't work because the modalias uevent will be "spi:ltc2947" which won't match "of:N*T*Cadi,ltc2947". So everything is really fragile. Note also that the "T*" in the alias, that's to denote the device type according to the ePAPR spec but that's only filled in the DT node has a device_type DT property, and most DT nodes don't fill this. So the driver module is greedy and will match any device type. And also the modalias reported by the kernel won't set this, i.e: $ cat /sys/bus/i2c/drivers/ssd130x-i2c/1-003c/modalias of:NoledT(null)Csolomon,ssd1306fb-i2c The only way I found to make this robust and always working is with each driver to define its own tables depending on what is used to match and report the modalias to user-space. That means having an of_device_id and spi_device_id (just for modalias reporting and alias table in module) for SPI and a of_device_id table for I2C and platform devices. And also having different compatible strings with a "-i2c" and "-spi" as suffixes to allow kmod / udev to differentiate and match the modules. Otherwise you will get "of:N(null)T(null)Cadi,ltc2947" in both cases and user-space won't be able to figure out which module to match AFAICT. Best regards,
On 2/9/22 17:41, Andy Shevchenko wrote: > On Wed, Feb 09, 2022 at 05:25:03PM +0100, Geert Uytterhoeven wrote: >> On Wed, Feb 9, 2022 at 5:07 PM Javier Martinez Canillas >> <javierm@redhat.com> wrote: > > ... > >> Are they all broken? > > I guess it's incorrect question. The one we need to ask is is OF code broken? > B/c ACPI can easily cope with this (they are different buses, can't clash). > Yes, it's a problem specific to OF. It works correctly with both ACPI and legacy platform code. Best regards,
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig index 47e16bc20e0d..16a2098f438c 100644 --- a/drivers/gpu/drm/solomon/Kconfig +++ b/drivers/gpu/drm/solomon/Kconfig @@ -19,3 +19,12 @@ config DRM_SSD130X_I2C Say Y here if the SSD130X OLED display is connected via I2C bus. If M is selected the module will be called ssd130x-i2c. + +config DRM_SSD130X_SPI + tristate "DRM support for Solomon SSD130X OLED displays (SPI bus)" + depends on DRM_SSD130X && SPI + select REGMAP_SPI + help + Say Y here if the SSD130X OLED display is connected via SPI bus. + + If M is selected the module will be called ssd130x-spi. diff --git a/drivers/gpu/drm/solomon/Makefile b/drivers/gpu/drm/solomon/Makefile index 4bfc5acb0447..b5fc792257d7 100644 --- a/drivers/gpu/drm/solomon/Makefile +++ b/drivers/gpu/drm/solomon/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_DRM_SSD130X) += ssd130x.o obj-$(CONFIG_DRM_SSD130X_I2C) += ssd130x-i2c.o +obj-$(CONFIG_DRM_SSD130X_SPI) += ssd130x-spi.o diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c new file mode 100644 index 000000000000..ccc56d2f3026 --- /dev/null +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DRM driver for Solomon SSD130X OLED displays (SPI bus) + * + * Copyright 2022 Red Hat Inc. + * Authors: Javier Martinez Canillas <javierm@redhat.com> + */ +#include <linux/spi/spi.h> +#include <linux/module.h> + +#include "ssd130x.h" + +#define DRIVER_NAME "ssd130x-spi" +#define DRIVER_DESC "DRM driver for Solomon SSD130X OLED displays (SPI)" + +static const struct regmap_config ssd130x_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +static int ssd130x_spi_probe(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x; + struct regmap *regmap; + + regmap = devm_regmap_init_spi(spi, &ssd130x_spi_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + ssd130x = ssd130x_probe(&spi->dev, regmap); + + if (IS_ERR(ssd130x)) + return PTR_ERR(ssd130x); + + spi_set_drvdata(spi, ssd130x); + + return 0; +} + +static int ssd130x_spi_remove(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + return ssd130x_remove(ssd130x); +} + +static void ssd130x_spi_shutdown(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + ssd130x_shutdown(ssd130x); +} + +static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = { + .default_vcomh = 0x34, + .default_dclk_div = 1, + .default_dclk_frq = 7, +}; + +static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = { + .default_vcomh = 0x20, + .default_dclk_div = 1, + .default_dclk_frq = 8, + .need_chargepump = 1, +}; + +static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = { + .default_vcomh = 0x20, + .default_dclk_div = 2, + .default_dclk_frq = 12, + .need_pwm = 1, +}; + +static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = { + .default_vcomh = 0x34, + .default_dclk_div = 1, + .default_dclk_frq = 10, +}; + +static const struct of_device_id ssd130x_of_match[] = { + { + .compatible = "solomon,ssd1305fb-spi", + .data = (void *)&ssd130x_ssd1305_deviceinfo, + }, + { + .compatible = "solomon,ssd1306fb-spi", + .data = (void *)&ssd130x_ssd1306_deviceinfo, + }, + { + .compatible = "solomon,ssd1307fb-spi", + .data = (void *)&ssd130x_ssd1307_deviceinfo, + }, + { + .compatible = "solomon,ssd1309fb-spi", + .data = (void *)&ssd130x_ssd1309_deviceinfo, + }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ssd130x_of_match); + +static struct spi_driver ssd130x_spi_driver = { + .driver = { + .name = DRIVER_NAME, + .of_match_table = ssd130x_of_match, + }, + .probe = ssd130x_spi_probe, + .remove = ssd130x_spi_remove, + .shutdown = ssd130x_spi_shutdown, +}; +module_spi_driver(ssd130x_spi_driver); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>"); +MODULE_LICENSE("GPL v2");
The ssd130x driver only provides the core support for these devices but it does not have any bus transport logic. Add a driver to interface over SPI. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes in v3: - Add a separate driver for SSD130X chips SPI support (Andy Shevchenko) drivers/gpu/drm/solomon/Kconfig | 9 ++ drivers/gpu/drm/solomon/Makefile | 1 + drivers/gpu/drm/solomon/ssd130x-spi.c | 114 ++++++++++++++++++++++++++ 3 files changed, 124 insertions(+) create mode 100644 drivers/gpu/drm/solomon/ssd130x-spi.c