| Message ID | 20240927135306.857617-1-hugo@hugovil.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: panel: jd9365da-h3: fix reset signal polarity | expand |
Hi, On 27/09/2024 15:53, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > In jadard_prepare() a reset pulse is generated with the following > statements (delays ommited for clarity): > > gpiod_set_value(jadard->reset, 1); --> Deassert reset > gpiod_set_value(jadard->reset, 0); --> Assert reset for 10ms > gpiod_set_value(jadard->reset, 1); --> Deassert reset > > However, specifying second argument of "0" to gpiod_set_value() means to > deassert the GPIO, and "1" means to assert it. If the reset signal is > defined as GPIO_ACTIVE_LOW in the DTS, the above statements will > incorrectly generate the reset pulse (inverted) and leave it asserted > (LOW) at the end of jadard_prepare(). Did you check the polarity in DTS of _all_ users of this driver ? Neil > > Fix reset behavior by inverting gpiod_set_value() second argument > in jadard_prepare(). Also modify second argument to devm_gpiod_get() > in jadard_dsi_probe() to assert the reset when probing. > > Do not modify it in jadard_unprepare() as it is already properly > asserted with "1", which seems to be the intended behavior. > > Fixes: 6b818c533dd8 ("drm: panel: Add Jadard JD9365DA-H3 DSI panel") > Cc: <stable@vger.kernel.org> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > --- > drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > index 44897e5218a69..6fec99cf4d935 100644 > --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > @@ -110,13 +110,13 @@ static int jadard_prepare(struct drm_panel *panel) > if (jadard->desc->lp11_to_reset_delay_ms) > msleep(jadard->desc->lp11_to_reset_delay_ms); > > - gpiod_set_value(jadard->reset, 1); > + gpiod_set_value(jadard->reset, 0); > msleep(5); > > - gpiod_set_value(jadard->reset, 0); > + gpiod_set_value(jadard->reset, 1); > msleep(10); > > - gpiod_set_value(jadard->reset, 1); > + gpiod_set_value(jadard->reset, 0); > msleep(130); > > ret = jadard->desc->init(jadard); > @@ -1131,7 +1131,7 @@ static int jadard_dsi_probe(struct mipi_dsi_device *dsi) > dsi->format = desc->format; > dsi->lanes = desc->lanes; > > - jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > if (IS_ERR(jadard->reset)) { > DRM_DEV_ERROR(&dsi->dev, "failed to get our reset GPIO\n"); > return PTR_ERR(jadard->reset); > > base-commit: 18ba6034468e7949a9e2c2cf28e2e123b4fe7a50
On Mon, 30 Sep 2024 16:38:14 +0200 Neil Armstrong <neil.armstrong@linaro.org> wrote: > Hi, > > On 27/09/2024 15:53, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > In jadard_prepare() a reset pulse is generated with the following > > statements (delays ommited for clarity): > > > > gpiod_set_value(jadard->reset, 1); --> Deassert reset > > gpiod_set_value(jadard->reset, 0); --> Assert reset for 10ms > > gpiod_set_value(jadard->reset, 1); --> Deassert reset > > > > However, specifying second argument of "0" to gpiod_set_value() means to > > deassert the GPIO, and "1" means to assert it. If the reset signal is > > defined as GPIO_ACTIVE_LOW in the DTS, the above statements will > > incorrectly generate the reset pulse (inverted) and leave it asserted > > (LOW) at the end of jadard_prepare(). > > Did you check the polarity in DTS of _all_ users of this driver ? Hi Neil, I confirmed that no in-tree DTS is referencing this driver. I did a search of all the compatible strings defined in the "jadard,jd9365da-h3.yaml" file. I also did the same on Debian code search. Hugo. > > Neil > > > > > Fix reset behavior by inverting gpiod_set_value() second argument > > in jadard_prepare(). Also modify second argument to devm_gpiod_get() > > in jadard_dsi_probe() to assert the reset when probing. > > > > Do not modify it in jadard_unprepare() as it is already properly > > asserted with "1", which seems to be the intended behavior. > > > > Fixes: 6b818c533dd8 ("drm: panel: Add Jadard JD9365DA-H3 DSI panel") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > --- > > drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > > index 44897e5218a69..6fec99cf4d935 100644 > > --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > > +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > > @@ -110,13 +110,13 @@ static int jadard_prepare(struct drm_panel *panel) > > if (jadard->desc->lp11_to_reset_delay_ms) > > msleep(jadard->desc->lp11_to_reset_delay_ms); > > > > - gpiod_set_value(jadard->reset, 1); > > + gpiod_set_value(jadard->reset, 0); > > msleep(5); > > > > - gpiod_set_value(jadard->reset, 0); > > + gpiod_set_value(jadard->reset, 1); > > msleep(10); > > > > - gpiod_set_value(jadard->reset, 1); > > + gpiod_set_value(jadard->reset, 0); > > msleep(130); > > > > ret = jadard->desc->init(jadard); > > @@ -1131,7 +1131,7 @@ static int jadard_dsi_probe(struct mipi_dsi_device *dsi) > > dsi->format = desc->format; > > dsi->lanes = desc->lanes; > > > > - jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > + jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > if (IS_ERR(jadard->reset)) { > > DRM_DEV_ERROR(&dsi->dev, "failed to get our reset GPIO\n"); > > return PTR_ERR(jadard->reset); > > > > base-commit: 18ba6034468e7949a9e2c2cf28e2e123b4fe7a50 > >
On 30/09/2024 17:05, Hugo Villeneuve wrote: > On Mon, 30 Sep 2024 16:38:14 +0200 > Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> Hi, >> >> On 27/09/2024 15:53, Hugo Villeneuve wrote: >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com> >>> >>> In jadard_prepare() a reset pulse is generated with the following >>> statements (delays ommited for clarity): >>> >>> gpiod_set_value(jadard->reset, 1); --> Deassert reset >>> gpiod_set_value(jadard->reset, 0); --> Assert reset for 10ms >>> gpiod_set_value(jadard->reset, 1); --> Deassert reset >>> >>> However, specifying second argument of "0" to gpiod_set_value() means to >>> deassert the GPIO, and "1" means to assert it. If the reset signal is >>> defined as GPIO_ACTIVE_LOW in the DTS, the above statements will >>> incorrectly generate the reset pulse (inverted) and leave it asserted >>> (LOW) at the end of jadard_prepare(). >> >> Did you check the polarity in DTS of _all_ users of this driver ? > > Hi Neil, > I confirmed that no in-tree DTS is referencing this driver. I did a > search of all the compatible strings defined in the > "jadard,jd9365da-h3.yaml" file. I also did the same on Debian code > search. OK then: Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > > Hugo. > > >> >> Neil >> >>> >>> Fix reset behavior by inverting gpiod_set_value() second argument >>> in jadard_prepare(). Also modify second argument to devm_gpiod_get() >>> in jadard_dsi_probe() to assert the reset when probing. >>> >>> Do not modify it in jadard_unprepare() as it is already properly >>> asserted with "1", which seems to be the intended behavior. >>> >>> Fixes: 6b818c533dd8 ("drm: panel: Add Jadard JD9365DA-H3 DSI panel") >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> >>> --- >>> drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c >>> index 44897e5218a69..6fec99cf4d935 100644 >>> --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c >>> +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c >>> @@ -110,13 +110,13 @@ static int jadard_prepare(struct drm_panel *panel) >>> if (jadard->desc->lp11_to_reset_delay_ms) >>> msleep(jadard->desc->lp11_to_reset_delay_ms); >>> >>> - gpiod_set_value(jadard->reset, 1); >>> + gpiod_set_value(jadard->reset, 0); >>> msleep(5); >>> >>> - gpiod_set_value(jadard->reset, 0); >>> + gpiod_set_value(jadard->reset, 1); >>> msleep(10); >>> >>> - gpiod_set_value(jadard->reset, 1); >>> + gpiod_set_value(jadard->reset, 0); >>> msleep(130); >>> >>> ret = jadard->desc->init(jadard); >>> @@ -1131,7 +1131,7 @@ static int jadard_dsi_probe(struct mipi_dsi_device *dsi) >>> dsi->format = desc->format; >>> dsi->lanes = desc->lanes; >>> >>> - jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >>> + jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); >>> if (IS_ERR(jadard->reset)) { >>> DRM_DEV_ERROR(&dsi->dev, "failed to get our reset GPIO\n"); >>> return PTR_ERR(jadard->reset); >>> >>> base-commit: 18ba6034468e7949a9e2c2cf28e2e123b4fe7a50 >> >> > >
diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c index 44897e5218a69..6fec99cf4d935 100644 --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c @@ -110,13 +110,13 @@ static int jadard_prepare(struct drm_panel *panel) if (jadard->desc->lp11_to_reset_delay_ms) msleep(jadard->desc->lp11_to_reset_delay_ms); - gpiod_set_value(jadard->reset, 1); + gpiod_set_value(jadard->reset, 0); msleep(5); - gpiod_set_value(jadard->reset, 0); + gpiod_set_value(jadard->reset, 1); msleep(10); - gpiod_set_value(jadard->reset, 1); + gpiod_set_value(jadard->reset, 0); msleep(130); ret = jadard->desc->init(jadard); @@ -1131,7 +1131,7 @@ static int jadard_dsi_probe(struct mipi_dsi_device *dsi) dsi->format = desc->format; dsi->lanes = desc->lanes; - jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); + jadard->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(jadard->reset)) { DRM_DEV_ERROR(&dsi->dev, "failed to get our reset GPIO\n"); return PTR_ERR(jadard->reset);