| 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: <b21dbdbe-c779-4593-9a6a-892ef1298adb@wanadoo.fr>
Date: Thu, 25 Jul 2024 07:20:21 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: lanzano.alex@...il.com
Cc: airlied@...il.com, 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: [PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD
Le 25/07/2024 à 02:47, Alex Lanzano a écrit :
> Add support for the monochrome Sharp Memory LCDs.
>
> Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@...lic.gmane.org>
> ---
> MAINTAINERS | 8 +
> drivers/gpu/drm/tiny/Kconfig | 20 +
> drivers/gpu/drm/tiny/Makefile | 1 +
> drivers/gpu/drm/tiny/sharp-memory.c | 742 ++++++++++++++++++++++++++++
> 4 files changed, 771 insertions(+)
> create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
>
Hi,
below a few comments, mostly cosmetic and 2 more interesting things
related to error handling at the end.
Hope it helps.
CJ
...
> diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
> new file mode 100644
> index 000000000000..5e61e348ce3a
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/sharp-memory.c
> @@ -0,0 +1,742 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +#include <linux/kernel.h>
Is it really needed?
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +#include <linux/bitrev.h>
> +#include <linux/pwm.h>
> +#include <linux/mutex.h>
Nitpick: usually, alphabetical order is preferred.
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_dma_helper.h>
...
> +static inline void sharp_memory_set_tx_buffer_addresses(uint8_t *buffer,
> + struct drm_rect clip,
> + uint32_t pitch)
> +{
> + for (uint32_t line = 0; line < clip.y2; ++line)
> + buffer[line * pitch] = line + 1;
> +
Nitpick: No need for an empty line.
> +}
> +
> +static void sharp_memory_set_tx_buffer_data(uint8_t *buffer,
> + struct drm_framebuffer *fb,
> + struct drm_rect clip,
> + uint32_t pitch,
> + struct drm_format_conv_state *fmtcnv_state)
> +{
> + int ret;
> + struct iosys_map dst, vmap;
> + struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return;
> +
> +
Nitpick: No need for 2 newlines.
> + iosys_map_set_vaddr(&dst, buffer);
> + iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
> +
> + drm_fb_xrgb8888_to_mono(&dst, &pitch, &vmap, fb, &clip, fmtcnv_state);
> +
> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}
...
> +static void sharp_memory_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_state *state)
> +{
> +
Nitpick: No need for an empty line.
> + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> + struct drm_plane_state *plane_state = plane->state;
> + struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
> + struct sharp_memory_device *smd;
> + struct drm_rect rect;
> +
> + smd = container_of(plane, struct sharp_memory_device, plane);
> + if (!smd->crtc.state->active)
> + return;
> +
> +
> + if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
> + sharp_memory_fb_dirty(plane_state->fb, &rect, &fmtcnv_state);
> +
> + drm_format_conv_state_release(&fmtcnv_state);
> +}
...
> +static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct pwm_state pwm_state;
> + struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> + sharp_memory_clear_display(smd);
> +
> + if (smd->enable_gpio)
> + gpiod_set_value(smd->enable_gpio, 1);
> +
> +
Nitpick: No need for 2 newlines.
> + switch (smd->vcom_mode) {
> + case SHARP_MEMORY_SOFTWARE_VCOM:
> + smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
> + smd, "sw_vcom_signal");
> + break;
> +
> + case SHARP_MEMORY_EXTERNAL_VCOM:
> + break;
> +
> + case SHARP_MEMORY_PWM_VCOM:
> + pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
> + pwm_state.period = 1000000000;
> + pwm_state.duty_cycle = 100000000;
> + pwm_state.enabled = true;
> + pwm_apply_state(smd->pwm_vcom_signal, &pwm_state);
> + break;
> + }
> +}
> +
> +static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> + sharp_memory_clear_display(smd);
> +
> + if (smd->enable_gpio)
> + gpiod_set_value(smd->enable_gpio, 0);
> +
> +
Nitpick: No need for 2 newlines.
> + switch (smd->vcom_mode) {
> + case SHARP_MEMORY_SOFTWARE_VCOM:
> + kthread_stop(smd->sw_vcom_signal);
> + break;
> +
> + case SHARP_MEMORY_EXTERNAL_VCOM:
> + break;
> +
> + case SHARP_MEMORY_PWM_VCOM:
> + pwm_disable(smd->pwm_vcom_signal);
> + break;
> + }
> +}
...
> +static const struct drm_connector_funcs sharp_memory_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +
Nitpick: No need for an empty line.
> +};
> +
> +static const struct drm_mode_config_funcs sharp_memory_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create_with_dirty,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
...
> +static int sharp_memory_pipe_init(struct drm_device *dev,
> + struct sharp_memory_device *smd,
> + const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers)
> +{
> + int ret;
> + struct drm_encoder *encoder = &smd->encoder;
> + struct drm_plane *plane = &smd->plane;
> + struct drm_crtc *crtc = &smd->crtc;
> + struct drm_connector *connector = &smd->connector;
> +
> + drm_plane_helper_add(plane, &sharp_memory_plane_helper_funcs);
> + ret = drm_universal_plane_init(dev, plane, 0,
> + &sharp_memory_plane_funcs,
> + formats, format_count,
> + format_modifiers,
> + DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret)
> + return ret;
> +
> +
> + drm_crtc_helper_add(crtc, &sharp_memory_crtc_helper_funcs);
> + ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> + &sharp_memory_crtc_funcs, NULL);
> + if (ret)
> + return ret;
> +
> +
Nitpick: No need for 2 newlines. (here and 2 other time just below)
> + encoder->possible_crtcs = drm_crtc_mask(crtc);
> + ret = drm_encoder_init(dev, encoder, &sharp_memory_encoder_funcs,
> + DRM_MODE_ENCODER_NONE, NULL);
> + if (ret)
> + return ret;
> +
> +
> + ret = drm_connector_init(&smd->drm, &smd->connector,
> + &sharp_memory_connector_funcs,
> + DRM_MODE_CONNECTOR_SPI);
> + if (ret)
> + return ret;
> +
> +
> + drm_connector_helper_add(&smd->connector,
> + &sharp_memory_connector_hfuncs);
> +
> + return drm_connector_attach_encoder(connector, encoder);
> +}
> +
> +static int sharp_memory_init_pwm_vcom_signal(struct sharp_memory_device *smd)
> +{
> + struct pwm_state state;
> + struct device *dev = &smd->spi->dev;
> +
> + smd->pwm_vcom_signal = devm_pwm_get(dev, NULL);
> + if (IS_ERR(smd->pwm_vcom_signal)) {
> + dev_err(dev, "Could not get pwm device\n");
> + return PTR_ERR(smd->pwm_vcom_signal);
Called from probe() opnly,so could be return dev_err_probe().
> + }
> +
> + pwm_init_state(smd->pwm_vcom_signal, &state);
> + state.enabled = false;
> + pwm_apply_state(smd->pwm_vcom_signal, &state);
> +
> + return 0;
> +}
> +
> +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;
> +
> + ret = spi_setup(spi);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
> +
> + dev = &spi->dev;
> + 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);
> +
> + 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");
> +
> +
Nitpick: No need for 2 newlines.
> + 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");
> +
> +
Nitpick: No need for 2 newlines.
> + /*
> + * 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_VCOM (EXTMODE = L) - This method uses a kthread to
> + * periodically send a "maintain display" message to the display,
> + * toggling the vcom bit on and off with each message
> + *
> + * EXTERNAL_VCOM (EXTMODE = H) - This method relies on an external
> + * clock to generate the signal on the EXTCOMM pin
> + *
> + * PWM_VCOM (EXTMODE = H) - This method uses a pwm device to generate
> + * the signal on the EXTCOMM pin
> + *
> + */
> + smd->vcom = 0;
> + if (device_property_read_u32(&spi->dev, "vcom-mode", &smd->vcom_mode))
> + return dev_err_probe(dev, ret, "Unable to find vcom-mode node in device tree\n");
ret is 0 at this point, so we return 'sucess'.
Is it what is expected?
> +
> +
Nitpick: No need for 2 newlines.
> + switch (smd->vcom_mode) {
> + case SHARP_MEMORY_SOFTWARE_VCOM:
> + case SHARP_MEMORY_EXTERNAL_VCOM:
> + /* No init is needed for these two modes */
> + break;
> +
> + case SHARP_MEMORY_PWM_VCOM:
> + ret = sharp_memory_init_pwm_vcom_signal(smd);
...
> + 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);
Error handling?
> +
> + 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;
...
CJ
Powered by blists - more mailing lists