El lun, 17 nov 2025 a las 11:57, Krzysztof Kozlowski
(<[email protected]>) 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
