| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3c8359ae-9a12-41c8-9799-86de9024fcd4@wanadoo.fr>
Date: Tue, 20 Aug 2024 08:53:07 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: lanzano.alex@...il.com
Cc: airlied@...il.com, christophe.jaillet@...adoo.fr, conor+dt@...nel.org,
daniel@...ll.ch, devicetree@...r.kernel.org,
dri-devel@...ts.freedesktop.org, krzk+dt@...nel.org,
linux-kernel@...r.kernel.org, maarten.lankhorst@...ux.intel.com,
mehdi.djait@...tlin.com, mripard@...nel.org, robh@...nel.org,
tzimmermann@...e.de
Subject: Re: [RESEND PATCH v4 2/2] drm/tiny: Add driver for Sharp Memory LCD
Le 19/08/2024 à 23:49, Alex Lanzano a écrit :
> Add support for the monochrome Sharp Memory LCDs.
Hi,
a few nitpick below, should thre be a v5.
...
> +struct sharp_memory_device {
> + struct drm_device drm;
> + struct spi_device *spi;
> +
> + const struct drm_display_mode *mode;
> +
> + struct drm_crtc crtc;
> + struct drm_plane plane;
> + struct drm_encoder encoder;
> + struct drm_connector connector;
> +
> + struct gpio_desc *enable_gpio;
> +
> + struct task_struct *sw_vcom_signal;
> + struct pwm_device *pwm_vcom_signal;
> +
> + enum sharp_memory_vcom_mode vcom_mode;
> + u8 vcom;
> +
> + u32 pitch;
> + u32 tx_buffer_size;
> + u8 *tx_buffer;
> +
> + /* When vcom_mode == "software" a kthread is used to
> + * periodically send a 'maintain display' message over
> + * spi. This mutex ensures tx_buffer access and spi bus
> + * usage is synchronized in this case
This comment could take only 3 lines and still be with < 80 lines.
A dot could also be added at the end of the 2nd sentence.
> + */
> + struct mutex tx_mutex;
> +};
...
> +static int sharp_memory_probe(struct spi_device *spi)
> +{
> + int ret;
> + struct device *dev;
> + struct sharp_memory_device *smd;
> + 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;
If done earlier (when dev is declared?), it could already be used in the
dev_err_probe() just above?
> + 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(dev, &sharp_memory_drm_driver,
> + struct sharp_memory_device, drm);
> + if (!smd)
> + return -ENOMEM;
> +
> + 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)
> + dev_warn(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
> + *
> + */
> + if (device_property_read_string(&spi->dev, "sharp,vcom-mode", &vcom_mode_str))
just dev?
> + return dev_err_probe(dev, -EINVAL,
> + "Unable to find sharp,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;
> + smd->mode = spi_get_device_match_data(spi);
> +
> + 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);
Just dev?
> + if (!smd->tx_buffer)
> + return -ENOMEM;
> +
> + 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,
> + 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
Powered by blists - more mailing lists