Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
Co-developed-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>
Signed-off-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>
---
  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






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux