| Message ID | 20200224232126.3385250-5-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel | expand |
Hi Sebastian, Thank you for the patch. On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote: > The panel-dsi-cm's ddata->pin_config is always NULL, so this > callback is never called. Instead the DSI encoder gets the pin > configuration directly from DT. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 ----------- > drivers/gpu/drm/omapdrm/dss/dsi.c | 1 - > drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- > 3 files changed, 14 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > index 3484b5d4a91c..e7fe5d702337 100644 > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > @@ -68,8 +68,6 @@ struct panel_drv_data { > int width_mm; > int height_mm; > > - struct omap_dsi_pin_config pin_config; > - > /* runtime variables */ > bool enabled; > > @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata) > } > } > > - if (ddata->pin_config.num_pins > 0) { > - r = src->ops->dsi.configure_pins(src, &ddata->pin_config); > - if (r) { > - dev_err(&ddata->pdev->dev, > - "failed to configure DSI pins\n"); > - goto err_vddi; > - } > - } > - > r = src->ops->dsi.set_config(src, &dsi_config); > if (r) { > dev_err(&ddata->pdev->dev, "failed to configure DSI\n"); > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > index 79ddfbfd1b58..8c39823a8295 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = { > > .enable_hs = dsi_vc_enable_hs, > > - .configure_pins = dsi_configure_pins, > .set_config = dsi_set_config, > > .enable_video_output = dsi_enable_video_output, > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h > index cbbe10b2b60d..b0424daaceed 100644 > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > @@ -292,8 +292,6 @@ struct omapdss_dsi_ops { I think you can drop the definition of the omap_dsi_pin_config structure earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > /* bus configuration */ > int (*set_config)(struct omap_dss_device *dssdev, > const struct omap_dss_dsi_config *cfg); > - int (*configure_pins)(struct omap_dss_device *dssdev, > - const struct omap_dsi_pin_config *pin_cfg); > > void (*enable_hs)(struct omap_dss_device *dssdev, int channel, > bool enable);
Hi Laurent, On Tue, Feb 25, 2020 at 01:42:49AM +0200, Laurent Pinchart wrote: > Hi Sebastian, > > Thank you for the patch. > > On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote: > > The panel-dsi-cm's ddata->pin_config is always NULL, so this > > callback is never called. Instead the DSI encoder gets the pin > > configuration directly from DT. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 ----------- > > drivers/gpu/drm/omapdrm/dss/dsi.c | 1 - > > drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- > > 3 files changed, 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > index 3484b5d4a91c..e7fe5d702337 100644 > > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > @@ -68,8 +68,6 @@ struct panel_drv_data { > > int width_mm; > > int height_mm; > > > > - struct omap_dsi_pin_config pin_config; > > - > > /* runtime variables */ > > bool enabled; > > > > @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata) > > } > > } > > > > - if (ddata->pin_config.num_pins > 0) { > > - r = src->ops->dsi.configure_pins(src, &ddata->pin_config); > > - if (r) { > > - dev_err(&ddata->pdev->dev, > > - "failed to configure DSI pins\n"); > > - goto err_vddi; > > - } > > - } > > - > > r = src->ops->dsi.set_config(src, &dsi_config); > > if (r) { > > dev_err(&ddata->pdev->dev, "failed to configure DSI\n"); > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > > index 79ddfbfd1b58..8c39823a8295 100644 > > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > > @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = { > > > > .enable_hs = dsi_vc_enable_hs, > > > > - .configure_pins = dsi_configure_pins, > > .set_config = dsi_set_config, > > > > .enable_video_output = dsi_enable_video_output, > > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h > > index cbbe10b2b60d..b0424daaceed 100644 > > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > > @@ -292,8 +292,6 @@ struct omapdss_dsi_ops { > > I think you can drop the definition of the omap_dsi_pin_config structure > earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro. > With this fixed, No, the struct is still used by the code setting up the pins from DT. -- Sebastian > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > /* bus configuration */ > > int (*set_config)(struct omap_dss_device *dssdev, > > const struct omap_dss_dsi_config *cfg); > > - int (*configure_pins)(struct omap_dss_device *dssdev, > > - const struct omap_dsi_pin_config *pin_cfg); > > > > void (*enable_hs)(struct omap_dss_device *dssdev, int channel, > > bool enable); > > -- > Regards, > > Laurent Pinchart
Hi Sebastian, On Wed, Feb 26, 2020 at 10:28:19PM +0100, Sebastian Reichel wrote: > On Tue, Feb 25, 2020 at 01:42:49AM +0200, Laurent Pinchart wrote: > > On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote: > > > The panel-dsi-cm's ddata->pin_config is always NULL, so this > > > callback is never called. Instead the DSI encoder gets the pin > > > configuration directly from DT. > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > > --- > > > drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 ----------- > > > drivers/gpu/drm/omapdrm/dss/dsi.c | 1 - > > > drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- > > > 3 files changed, 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > > index 3484b5d4a91c..e7fe5d702337 100644 > > > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > > > @@ -68,8 +68,6 @@ struct panel_drv_data { > > > int width_mm; > > > int height_mm; > > > > > > - struct omap_dsi_pin_config pin_config; > > > - > > > /* runtime variables */ > > > bool enabled; > > > > > > @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata) > > > } > > > } > > > > > > - if (ddata->pin_config.num_pins > 0) { > > > - r = src->ops->dsi.configure_pins(src, &ddata->pin_config); > > > - if (r) { > > > - dev_err(&ddata->pdev->dev, > > > - "failed to configure DSI pins\n"); > > > - goto err_vddi; > > > - } > > > - } > > > - > > > r = src->ops->dsi.set_config(src, &dsi_config); > > > if (r) { > > > dev_err(&ddata->pdev->dev, "failed to configure DSI\n"); > > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > > > index 79ddfbfd1b58..8c39823a8295 100644 > > > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > > > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > > > @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = { > > > > > > .enable_hs = dsi_vc_enable_hs, > > > > > > - .configure_pins = dsi_configure_pins, > > > .set_config = dsi_set_config, > > > > > > .enable_video_output = dsi_enable_video_output, > > > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h > > > index cbbe10b2b60d..b0424daaceed 100644 > > > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > > > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > > > @@ -292,8 +292,6 @@ struct omapdss_dsi_ops { > > > > I think you can drop the definition of the omap_dsi_pin_config structure > > earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro. > > With this fixed, > > No, the struct is still used by the code setting up the pins from > DT. Indeed, my bad. I think I'd pass the unsigned int num_pins and const int *pins to dsi_configure_pins() directly to drop the structure, but that can be done in a subsequent patch (maybe it is already :-)). > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This tag holds. > > > /* bus configuration */ > > > int (*set_config)(struct omap_dss_device *dssdev, > > > const struct omap_dss_dsi_config *cfg); > > > - int (*configure_pins)(struct omap_dss_device *dssdev, > > > - const struct omap_dsi_pin_config *pin_cfg); > > > > > > void (*enable_hs)(struct omap_dss_device *dssdev, int channel, > > > bool enable);
Hi Laurent, On Wed, Feb 26, 2020 at 11:36:30PM +0200, Laurent Pinchart wrote: > > > I think you can drop the definition of the omap_dsi_pin_config structure > > > earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro. > > > With this fixed, > > > > No, the struct is still used by the code setting up the pins from > > DT. > > Indeed, my bad. I think I'd pass the unsigned int num_pins and const int > *pins to dsi_configure_pins() directly to drop the structure, but that > can be done in a subsequent patch (maybe it is already :-)). I added quite some cleanups at the end of the series, but there is still quite a few cleanups possible within the DSI encoder (including this one). Cleaning up dsi.c takes some time and rebasing this code gets annoying. After this series the cleanups mostly are internal to dsi.c and should reduce merge conflict probability. Also I feel a bit uncomfortable, since we currently have no DSI video mode user. Nikolaus Schaller is working on adding support for such a system, so it would be nice to get that supported first making it possible to do easy bisecting for issues introduced by refactoring. (This is not specifically about dsi_configure_pins of course) > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > This tag holds. Thanks. -- Sebastian
On 25/02/2020 01:20, Sebastian Reichel wrote: > The panel-dsi-cm's ddata->pin_config is always NULL, so this > callback is never called. Instead the DSI encoder gets the pin > configuration directly from DT. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 ----------- > drivers/gpu/drm/omapdrm/dss/dsi.c | 1 - > drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- > 3 files changed, 14 deletions(-) Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Tomi
diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c index 3484b5d4a91c..e7fe5d702337 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c @@ -68,8 +68,6 @@ struct panel_drv_data { int width_mm; int height_mm; - struct omap_dsi_pin_config pin_config; - /* runtime variables */ bool enabled; @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata) } } - if (ddata->pin_config.num_pins > 0) { - r = src->ops->dsi.configure_pins(src, &ddata->pin_config); - if (r) { - dev_err(&ddata->pdev->dev, - "failed to configure DSI pins\n"); - goto err_vddi; - } - } - r = src->ops->dsi.set_config(src, &dsi_config); if (r) { dev_err(&ddata->pdev->dev, "failed to configure DSI\n"); diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index 79ddfbfd1b58..8c39823a8295 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = { .enable_hs = dsi_vc_enable_hs, - .configure_pins = dsi_configure_pins, .set_config = dsi_set_config, .enable_video_output = dsi_enable_video_output, diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index cbbe10b2b60d..b0424daaceed 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -292,8 +292,6 @@ struct omapdss_dsi_ops { /* bus configuration */ int (*set_config)(struct omap_dss_device *dssdev, const struct omap_dss_dsi_config *cfg); - int (*configure_pins)(struct omap_dss_device *dssdev, - const struct omap_dsi_pin_config *pin_cfg); void (*enable_hs)(struct omap_dss_device *dssdev, int channel, bool enable);
The panel-dsi-cm's ddata->pin_config is always NULL, so this callback is never called. Instead the DSI encoder gets the pin configuration directly from DT. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 ----------- drivers/gpu/drm/omapdrm/dss/dsi.c | 1 - drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- 3 files changed, 14 deletions(-)