linux-kernel - Re: [PATCH v4 2/3] drm: Add driver for Sitronix ST7920 LCD displays

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABdCQ=MiuMqrf1LpLP=q0by3v9RZNagkrJnJoWOmwSCKGPVO+w@mail.gmail.com>
Date: Wed, 26 Nov 2025 16:37:24 +0100
From: Iker Pedrosa <ikerpedrosam@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Javier Martinez Canillas <javierm@...hat.com>, linux-kernel@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/3] drm: Add driver for Sitronix ST7920 LCD displays

El lun, 17 nov 2025 a las 11:57, Krzysztof Kozlowski
(<krzk@...nel.org>) escribió:
>
> On 31/10/2025 10:05, Iker Pedrosa wrote:
> > +
> > +static void st7920_hw_reset(struct st7920_device *st7920)
> > +{
> > +     if (!st7920->reset_gpio)
> > +             return;
> > +
> > +     gpiod_set_value_cansleep(st7920->reset_gpio, 0);
> > +     usleep_range(15, 20);
> > +     gpiod_set_value_cansleep(st7920->reset_gpio, 1);
>
> This is odd. Why are you keeping device in reset state?
>
> I don't think you tested your driver with DTS.
>
> > +     msleep(40);
> > +}
> > +
>
> > +
> > +     regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
> > +     if (IS_ERR(regmap))
> > +             return PTR_ERR(regmap);
> > +
> > +     st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
> > +                                 struct st7920_device, drm);
> > +     if (IS_ERR(st7920))
> > +             return PTR_ERR(st7920);
> > +
> > +     drm = &st7920->drm;
> > +
> > +     st7920->dev = dev;
> > +     st7920->regmap = regmap;
> > +     st7920->spi = spi;
> > +     st7920->device_info = device_get_match_data(dev);
> > +     st7920->width = st7920->device_info->default_width;
> > +     st7920->height = st7920->device_info->default_height;
> > +
> > +     st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>
> So you keep device in reset state, then why do you toggle it in
> st7920_hw_reset()? Remember that this is the logical, not line level.

You are right. I was confused between the logical state and the
physical line level regarding gpiod.

I have fixed the logic in st7920_hw_reset() to correctly assert the
reset (logical 1) and then de-assert it (logical 0). Accordingly, I
have also changed the probe initialization to GPIOD_OUT_LOW to ensure
the device starts in a non-reset state.

I have also addressed the other comments. These fixes will be
available in the next version which I will submit shortly.

>
> > +     if (IS_ERR(st7920->reset_gpio)) {
> > +             ret = PTR_ERR(st7920->reset_gpio);
> > +             dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);
>
> return dev_err_probe, didn't you get comment on that?
>
> > +             return ret;
> > +     }
> > +
> > +     spi_set_drvdata(spi, st7920);
> > +
> > +     ret = st7920_init_modeset(st7920);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = drm_dev_register(drm, 0);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "DRM device register failed\n");
> > +
> > +     drm_client_setup(drm, NULL);
> > +
>
> Best regards,
> Krzysztof