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-re5jqeeqqe8avxtiumw...@public.gmane.org>
> > Co-developed-by: Mehdi Djait 
> > <mehdi.djait-ldxbnhwyfcjbdgjk7y7...@public.gmane.org>
> > Signed-off-by: Mehdi Djait 
> > <mehdi.djait-ldxbnhwyfcjbdgjk7y7...@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

Reply via email to