| Message ID | 20240726194456.1336484-1-lanzano.alex@gmail.com |
|---|---|
| Headers | show |
| Series | Add driver for Sharp Memory LCD | expand |
Le 26/07/2024 à 21:44, Alex Lanzano a écrit : > Add support for the monochrome Sharp Memory LCDs. > > Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Co-developed-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> > --- > MAINTAINERS | 7 + > drivers/gpu/drm/tiny/Kconfig | 20 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++ > 4 files changed, 754 insertions(+) > create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c > Hi, below some other tiny comments, hoping it helps. Also "./scripts/checkpatch.pl --strict" gives some hints, should some be of interest. > +static int sharp_memory_probe(struct spi_device *spi) > +{ > + int ret; > + struct device *dev; > + struct sharp_memory_device *smd; > + enum sharp_memory_model model; > + struct drm_device *drm; > + const char *vcom_mode_str; > + > + ret = spi_setup(spi); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n"); > + > + dev = &spi->dev; 6 times below, &spi->dev could be replaced by dev, to slightly simplify things. > + if (!dev->coherent_dma_mask) { > + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to set dma mask\n"); > + } > + > + smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver, > + struct sharp_memory_device, drm); Missing error handling. > + > + spi_set_drvdata(spi, smd); > + > + smd->spi = spi; > + drm = &smd->drm; > + ret = drmm_mode_config_init(drm); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to initialize drm config\n"); > + > + smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH); > + if (smd->enable_gpio == NULL) Nitpick: if (!smd->enable_gpio) > + dev_warn(&spi->dev, "Enable gpio not defined\n"); > + > + /* > + * VCOM is a signal that prevents DC bias from being built up in > + * the panel resulting in pixels being forever stuck in one state. > + * > + * This driver supports three different methods to generate this > + * signal depending on EXTMODE pin: > + * > + * software (EXTMODE = L) - This mode uses a kthread to > + * periodically send a "maintain display" message to the display, > + * toggling the vcom bit on and off with each message > + * > + * external (EXTMODE = H) - This mode relies on an external > + * clock to generate the signal on the EXTCOMM pin > + * > + * pwm (EXTMODE = H) - This mode uses a pwm device to generate > + * the signal on the EXTCOMM pin > + * > + */ > + smd->vcom = 0; Nitpick: devm_drm_dev_alloc() already zeroes the allocated memory. So this is useless. Unless you think it gives some useful hint, it could be removed. > + if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str)) > + return dev_err_probe(dev, -EINVAL, > + "Unable to find vcom-mode node in device tree\n"); > + > + if (!strcmp("software", vcom_mode_str)) { > + smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM; ... > + smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8; > + smd->tx_buffer_size = (SHARP_MODE_PERIOD + > + (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) * > + smd->mode->vdisplay) / 8; > + > + smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL); > + if (!smd->tx_buffer) > + return dev_err_probe(&spi->dev, -ENOMEM, "Unable to alloc tx buffer\n"); There is no need to log a message for memory allocation failure. return -ENOMEM; should be just fine IMHO. > + > + mutex_init(&smd->tx_mutex); > + > + drm->mode_config.min_width = smd->mode->hdisplay; > + drm->mode_config.max_width = smd->mode->hdisplay; > + drm->mode_config.min_height = smd->mode->vdisplay; > + drm->mode_config.max_height = smd->mode->vdisplay; > + > + ret = sharp_memory_pipe_init(drm, smd, > + sharp_memory_formats, Nitpick: you could save 1 LoC if this is put at the end of the previous line. > + ARRAY_SIZE(sharp_memory_formats), > + NULL); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n"); > + > + drm_plane_enable_fb_damage_clips(&smd->plane); > + drm_mode_config_reset(drm); > + > + ret = drm_dev_register(drm, 0); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register drm device.\n"); > + > + drm_fbdev_dma_setup(drm, 0); > + > + return 0; > +} ... CJ
On Sat, Jul 27, 2024 at 12:12:01AM GMT, Christophe JAILLET wrote: > Le 26/07/2024 à 21:44, Alex Lanzano a écrit : > > Add support for the monochrome Sharp Memory LCDs. > > > > Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > Co-developed-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> > > Signed-off-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> > > --- > > MAINTAINERS | 7 + > > drivers/gpu/drm/tiny/Kconfig | 20 + > > drivers/gpu/drm/tiny/Makefile | 1 + > > drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++ > > 4 files changed, 754 insertions(+) > > create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c > > > > Hi, > > below some other tiny comments, hoping it helps. > > Also "./scripts/checkpatch.pl --strict" gives some hints, should some be of > interest. > Ah! Thank you. I'll be sure to use strict from now on > > +static int sharp_memory_probe(struct spi_device *spi) > > +{ > > + int ret; > > + struct device *dev; > > + struct sharp_memory_device *smd; > > + enum sharp_memory_model model; > > + struct drm_device *drm; > > + const char *vcom_mode_str; > > + > > + ret = spi_setup(spi); > > + if (ret < 0) > > + return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n"); > > + > > + dev = &spi->dev; > > 6 times below, &spi->dev could be replaced by dev, to slightly simplify > things. > > > + if (!dev->coherent_dma_mask) { > > + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to set dma mask\n"); > > + } > > + > > + smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver, > > + struct sharp_memory_device, drm); > > Missing error handling. > > > + > > + spi_set_drvdata(spi, smd); > > + > > + smd->spi = spi; > > + drm = &smd->drm; > > + ret = drmm_mode_config_init(drm); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to initialize drm config\n"); > > + > > + smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH); > > + if (smd->enable_gpio == NULL) > > Nitpick: if (!smd->enable_gpio) > > > + dev_warn(&spi->dev, "Enable gpio not defined\n"); > > + > > + /* > > + * VCOM is a signal that prevents DC bias from being built up in > > + * the panel resulting in pixels being forever stuck in one state. > > + * > > + * This driver supports three different methods to generate this > > + * signal depending on EXTMODE pin: > > + * > > + * software (EXTMODE = L) - This mode uses a kthread to > > + * periodically send a "maintain display" message to the display, > > + * toggling the vcom bit on and off with each message > > + * > > + * external (EXTMODE = H) - This mode relies on an external > > + * clock to generate the signal on the EXTCOMM pin > > + * > > + * pwm (EXTMODE = H) - This mode uses a pwm device to generate > > + * the signal on the EXTCOMM pin > > + * > > + */ > > + smd->vcom = 0; > > Nitpick: devm_drm_dev_alloc() already zeroes the allocated memory. So this > is useless. Unless you think it gives some useful hint, it could be removed. > > > + if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str)) > > + return dev_err_probe(dev, -EINVAL, > > + "Unable to find vcom-mode node in device tree\n"); > > + > > + if (!strcmp("software", vcom_mode_str)) { > > + smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM; > > ... > > > + smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8; > > + smd->tx_buffer_size = (SHARP_MODE_PERIOD + > > + (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) * > > + smd->mode->vdisplay) / 8; > > + > > + smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL); > > + if (!smd->tx_buffer) > > + return dev_err_probe(&spi->dev, -ENOMEM, "Unable to alloc tx buffer\n"); > > There is no need to log a message for memory allocation failure. > return -ENOMEM; should be just fine IMHO. > > > + > > + mutex_init(&smd->tx_mutex); > > + > > + drm->mode_config.min_width = smd->mode->hdisplay; > > + drm->mode_config.max_width = smd->mode->hdisplay; > > + drm->mode_config.min_height = smd->mode->vdisplay; > > + drm->mode_config.max_height = smd->mode->vdisplay; > > + > > + ret = sharp_memory_pipe_init(drm, smd, > > + sharp_memory_formats, > > Nitpick: you could save 1 LoC if this is put at the end of the previous > line. > > > + ARRAY_SIZE(sharp_memory_formats), > > + NULL); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n"); > > + > > + drm_plane_enable_fb_damage_clips(&smd->plane); > > + drm_mode_config_reset(drm); > > + > > + ret = drm_dev_register(drm, 0); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to register drm device.\n"); > > + > > + drm_fbdev_dma_setup(drm, 0); > > + > > + return 0; > > +} > > ... > > CJ > > I appreciate the review! I'll get these addressed in v3 Best regards, Alex
On 26/07/2024 21:44, Alex Lanzano wrote: > This patch series add support for the monochrome Sharp Memory LCD > panels. This series is based off of the work done by Mehdi Djait. > > References: > https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/ > https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> Do not attach (thread) your patchsets to some other threads (unrelated or older versions). This buries them deep in the mailbox and might interfere with applying entire sets. Best regards, Krzysztof
On 26/07/2024 21:44, Alex Lanzano wrote: > Add support for the monochrome Sharp Memory LCDs. > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> > --- > MAINTAINERS | 7 + > drivers/gpu/drm/tiny/Kconfig | 20 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++ > 4 files changed, 754 insertions(+) > create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 71b739b40921..b5b08247a994 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7123,6 +7123,13 @@ S: Maintained > F: Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml > F: drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c > > +DRM DRIVER FOR SHARP MEMORY LCD > +M: Alex Lanzano <lanzano.alex@gmail.com> > +S: Maintained > +T: git git://anongit.freedesktop.org/drm/drm-misc Do you have drm-misc commit rights? If not, drop it. There is no point to put some other maintainer's tree in your entry. Git tree is already present in the maintainer's entry who is going to apply the patches. > +F: Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml If you rename the file in your patchset, you must rename it EVERYWHERE. > +F: drivers/gpu/drm/tiny/sharp-memory.c > + > DRM DRIVER FOR SITRONIX ST7586 PANELS ... > + smd->spi = spi; > + drm = &smd->drm; > + ret = drmm_mode_config_init(drm); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to initialize drm config\n"); > + > + smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH); > + if (smd->enable_gpio == NULL) > + dev_warn(&spi->dev, "Enable gpio not defined\n"); > + > + /* > + * VCOM is a signal that prevents DC bias from being built up in > + * the panel resulting in pixels being forever stuck in one state. > + * > + * This driver supports three different methods to generate this > + * signal depending on EXTMODE pin: > + * > + * software (EXTMODE = L) - This mode uses a kthread to > + * periodically send a "maintain display" message to the display, > + * toggling the vcom bit on and off with each message > + * > + * external (EXTMODE = H) - This mode relies on an external > + * clock to generate the signal on the EXTCOMM pin > + * > + * pwm (EXTMODE = H) - This mode uses a pwm device to generate > + * the signal on the EXTCOMM pin > + * > + */ > + smd->vcom = 0; > + if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str)) > + return dev_err_probe(dev, -EINVAL, > + "Unable to find vcom-mode node in device tree\n"); > + > + if (!strcmp("software", vcom_mode_str)) { > + smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM; > + > + } else if (!strcmp("external", vcom_mode_str)) { > + smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM; > + > + } else if (!strcmp("pwm", vcom_mode_str)) { > + smd->vcom_mode = SHARP_MEMORY_PWM_VCOM; > + ret = sharp_memory_init_pwm_vcom_signal(smd); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to initialize external COM signal\n"); > + } else { > + return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n"); > + } > + > + drm->mode_config.funcs = &sharp_memory_mode_config_funcs; > + > + /* Set the DRM display mode depending on panel model */ > + model = (uintptr_t)spi_get_device_match_data(spi); > + switch (model) { > + case LS013B7DH03: > + smd->mode = &sharp_memory_ls013b7dh03_mode; > + break; > + > + case LS010B7DH04: > + smd->mode = &sharp_memory_ls010b7dh04_mode; > + break; > + > + case LS011B7DH03: > + smd->mode = &sharp_memory_ls011b7dh03_mode; > + break; > + > + case LS012B7DD01: > + smd->mode = &sharp_memory_ls012b7dd01_mode; > + break; > + > + case LS013B7DH05: > + smd->mode = &sharp_memory_ls013b7dh05_mode; > + break; > + > + case LS018B7DH02: > + smd->mode = &sharp_memory_ls018b7dh02_mode; > + break; > + > + case LS027B7DH01: > + fallthrough; > + case LS027B7DH01A: > + smd->mode = &sharp_memory_ls027b7dh01_mode; > + break; > + > + case LS032B7DD02: > + smd->mode = &sharp_memory_ls032b7dd02_mode; > + break; > + > + case LS044Q7DH01: > + smd->mode = &sharp_memory_ls044q7dh01_mode; > + break; This is over-complicated. Just store the mode in device match data. > + > + default: > + return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n"); > + } Best regards, Krzysztof
On Sat, Jul 27, 2024 at 11:00:08AM GMT, Krzysztof Kozlowski wrote: > On 26/07/2024 21:44, Alex Lanzano wrote: > > This patch series add support for the monochrome Sharp Memory LCD > > panels. This series is based off of the work done by Mehdi Djait. > > > > References: > > https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/ > > https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf > > > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com> > > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> > > Do not attach (thread) your patchsets to some other threads (unrelated > or older versions). This buries them deep in the mailbox and might > interfere with applying entire sets. > > Best regards, > Krzysztof > Will do! Sorry about that Best regards, Alex
On Sat, Jul 27, 2024 at 11:03:19AM GMT, Krzysztof Kozlowski wrote: > On 26/07/2024 21:44, Alex Lanzano wrote: > > Add support for the monochrome Sharp Memory LCDs. > > > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com> > > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> > > --- > > MAINTAINERS | 7 + > > drivers/gpu/drm/tiny/Kconfig | 20 + > > drivers/gpu/drm/tiny/Makefile | 1 + > > drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++ > > 4 files changed, 754 insertions(+) > > create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 71b739b40921..b5b08247a994 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -7123,6 +7123,13 @@ S: Maintained > > F: Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml > > F: drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c > > > > +DRM DRIVER FOR SHARP MEMORY LCD > > +M: Alex Lanzano <lanzano.alex@gmail.com> > > +S: Maintained > > +T: git git://anongit.freedesktop.org/drm/drm-misc > > > Do you have drm-misc commit rights? If not, drop it. There is no point > to put some other maintainer's tree in your entry. Git tree is already > present in the maintainer's entry who is going to apply the patches. > > Will remove. > > +F: Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > > If you rename the file in your patchset, you must rename it EVERYWHERE. > > Will do. > > +F: drivers/gpu/drm/tiny/sharp-memory.c > > + > > DRM DRIVER FOR SITRONIX ST7586 PANELS > > > ... > Oh! Did you need me to rename sharp-memory.c as well? > > + smd->spi = spi; > > + drm = &smd->drm; > > + ret = drmm_mode_config_init(drm); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to initialize drm config\n"); > > + > > + smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH); > > + if (smd->enable_gpio == NULL) > > + dev_warn(&spi->dev, "Enable gpio not defined\n"); > > + > > + /* > > + * VCOM is a signal that prevents DC bias from being built up in > > + * the panel resulting in pixels being forever stuck in one state. > > + * > > + * This driver supports three different methods to generate this > > + * signal depending on EXTMODE pin: > > + * > > + * software (EXTMODE = L) - This mode uses a kthread to > > + * periodically send a "maintain display" message to the display, > > + * toggling the vcom bit on and off with each message > > + * > > + * external (EXTMODE = H) - This mode relies on an external > > + * clock to generate the signal on the EXTCOMM pin > > + * > > + * pwm (EXTMODE = H) - This mode uses a pwm device to generate > > + * the signal on the EXTCOMM pin > > + * > > + */ > > + smd->vcom = 0; > > + if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str)) > > + return dev_err_probe(dev, -EINVAL, > > + "Unable to find vcom-mode node in device tree\n"); > > + > > + if (!strcmp("software", vcom_mode_str)) { > > + smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM; > > + > > + } else if (!strcmp("external", vcom_mode_str)) { > > + smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM; > > + > > + } else if (!strcmp("pwm", vcom_mode_str)) { > > + smd->vcom_mode = SHARP_MEMORY_PWM_VCOM; > > + ret = sharp_memory_init_pwm_vcom_signal(smd); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to initialize external COM signal\n"); > > + } else { > > + return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n"); > > + } > > + > > + drm->mode_config.funcs = &sharp_memory_mode_config_funcs; > > + > > + /* Set the DRM display mode depending on panel model */ > > + model = (uintptr_t)spi_get_device_match_data(spi); > > + switch (model) { > > + case LS013B7DH03: > > + smd->mode = &sharp_memory_ls013b7dh03_mode; > > + break; > > + > > + case LS010B7DH04: > > + smd->mode = &sharp_memory_ls010b7dh04_mode; > > + break; > > + > > + case LS011B7DH03: > > + smd->mode = &sharp_memory_ls011b7dh03_mode; > > + break; > > + > > + case LS012B7DD01: > > + smd->mode = &sharp_memory_ls012b7dd01_mode; > > + break; > > + > > + case LS013B7DH05: > > + smd->mode = &sharp_memory_ls013b7dh05_mode; > > + break; > > + > > + case LS018B7DH02: > > + smd->mode = &sharp_memory_ls018b7dh02_mode; > > + break; > > + > > + case LS027B7DH01: > > + fallthrough; > > + case LS027B7DH01A: > > + smd->mode = &sharp_memory_ls027b7dh01_mode; > > + break; > > + > > + case LS032B7DD02: > > + smd->mode = &sharp_memory_ls032b7dd02_mode; > > + break; > > + > > + case LS044Q7DH01: > > + smd->mode = &sharp_memory_ls044q7dh01_mode; > > + break; > > This is over-complicated. Just store the mode in device match data. > > Will simplify in v3 > > + > > + default: > > + return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n"); > > + } > > > Best regards, > Krzysztof > Thank you for taking the time to review! Best regards, Alex