Message ID | 20171013145944.26557-35-laurent.pinchart@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, On Fri, Oct 13, 2017 at 05:59:30PM +0300, Laurent Pinchart wrote: > The sdi private data structure is currently stored as a global > variable. While no platform with multiple SDI encoders currently exists > nor is planned, this doesn't comply with the kernel device model and > should thus be fixed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > drivers/gpu/drm/omapdrm/dss/sdi.c | 122 +++++++++++++++++++++----------------- > 1 file changed, 69 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c > index ac436826914a..a35dc51c5a6a 100644 > --- a/drivers/gpu/drm/omapdrm/dss/sdi.c > +++ b/drivers/gpu/drm/omapdrm/dss/sdi.c > @@ -31,7 +31,7 @@ > #include "omapdss.h" > #include "dss.h" > > -static struct { > +struct sdi_device { > struct platform_device *pdev; > struct dss_device *dss; > > @@ -43,9 +43,9 @@ static struct { > int datapairs; > > struct omap_dss_device output; > +}; > > - bool port_initialized; > -} sdi; > +#define dssdev_to_sdi(dssdev) container_of(dssdev, struct sdi_device, output) > > struct sdi_clk_calc_ctx { > unsigned long pck_min, pck_max; > @@ -77,9 +77,9 @@ static bool dpi_calc_dss_cb(unsigned long fck, void *data) > dpi_calc_dispc_cb, ctx); > } > > -static int sdi_calc_clock_div(unsigned long pclk, > - unsigned long *fck, > - struct dispc_clock_info *dispc_cinfo) > +static int sdi_calc_clock_div(struct sdi_device *sdi, unsigned long pclk, > + unsigned long *fck, > + struct dispc_clock_info *dispc_cinfo) > { > int i; > struct sdi_clk_calc_ctx ctx; > @@ -101,7 +101,7 @@ static int sdi_calc_clock_div(unsigned long pclk, > ctx.pck_min = 0; > ctx.pck_max = pclk + 1000 * i * i * i; > > - ok = dss_div_calc(sdi.dss, pclk, ctx.pck_min, > + ok = dss_div_calc(sdi->dss, pclk, ctx.pck_min, > dpi_calc_dss_cb, &ctx); > if (ok) { > *fck = ctx.fck; > @@ -113,26 +113,27 @@ static int sdi_calc_clock_div(unsigned long pclk, > return -EINVAL; > } > > -static void sdi_config_lcd_manager(struct omap_dss_device *dssdev) > +static void sdi_config_lcd_manager(struct sdi_device *sdi) > { > - enum omap_channel channel = dssdev->dispc_channel; > + enum omap_channel channel = sdi->output.dispc_channel; > > - sdi.mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS; > + sdi->mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS; > > - sdi.mgr_config.stallmode = false; > - sdi.mgr_config.fifohandcheck = false; > + sdi->mgr_config.stallmode = false; > + sdi->mgr_config.fifohandcheck = false; > > - sdi.mgr_config.video_port_width = 24; > - sdi.mgr_config.lcden_sig_polarity = 1; > + sdi->mgr_config.video_port_width = 24; > + sdi->mgr_config.lcden_sig_polarity = 1; > > - dss_mgr_set_lcd_config(channel, &sdi.mgr_config); > + dss_mgr_set_lcd_config(channel, &sdi->mgr_config); > } > > static int sdi_display_enable(struct omap_dss_device *dssdev) > { > - struct omap_dss_device *out = &sdi.output; > + struct sdi_device *sdi = dssdev_to_sdi(dssdev); > + struct omap_dss_device *out = &sdi->output; > enum omap_channel channel = dssdev->dispc_channel; > - struct videomode *vm = &sdi.vm; > + struct videomode *vm = &sdi->vm; > unsigned long fck; > struct dispc_clock_info dispc_cinfo; > unsigned long pck; > @@ -143,7 +144,7 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) > return -ENODEV; > } > > - r = regulator_enable(sdi.vdds_sdi_reg); > + r = regulator_enable(sdi->vdds_sdi_reg); > if (r) > goto err_reg_enable; > > @@ -154,11 +155,11 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) > /* 15.5.9.1.2 */ > vm->flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_SYNC_POSEDGE; > > - r = sdi_calc_clock_div(vm->pixelclock, &fck, &dispc_cinfo); > + r = sdi_calc_clock_div(sdi, vm->pixelclock, &fck, &dispc_cinfo); > if (r) > goto err_calc_clock_div; > > - sdi.mgr_config.clock_info = dispc_cinfo; > + sdi->mgr_config.clock_info = dispc_cinfo; > > pck = fck / dispc_cinfo.lck_div / dispc_cinfo.pck_div; > > @@ -172,11 +173,11 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) > > dss_mgr_set_timings(channel, vm); > > - r = dss_set_fck_rate(sdi.dss, fck); > + r = dss_set_fck_rate(sdi->dss, fck); > if (r) > goto err_set_dss_clock_div; > > - sdi_config_lcd_manager(dssdev); > + sdi_config_lcd_manager(sdi); > > /* > * LCLK and PCLK divisors are located in shadow registers, and we > @@ -189,10 +190,10 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) > * need to care about the shadow register mechanism for pck-free. The > * exact reason for this is unknown. > */ > - dispc_mgr_set_clock_div(channel, &sdi.mgr_config.clock_info); > + dispc_mgr_set_clock_div(channel, &sdi->mgr_config.clock_info); > > - dss_sdi_init(sdi.dss, sdi.datapairs); > - r = dss_sdi_enable(sdi.dss); > + dss_sdi_init(sdi->dss, sdi->datapairs); > + r = dss_sdi_enable(sdi->dss); > if (r) > goto err_sdi_enable; > mdelay(2); > @@ -204,40 +205,45 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) > return 0; > > err_mgr_enable: > - dss_sdi_disable(sdi.dss); > + dss_sdi_disable(sdi->dss); > err_sdi_enable: > err_set_dss_clock_div: > err_calc_clock_div: > dispc_runtime_put(); > err_get_dispc: > - regulator_disable(sdi.vdds_sdi_reg); > + regulator_disable(sdi->vdds_sdi_reg); > err_reg_enable: > return r; > } > > static void sdi_display_disable(struct omap_dss_device *dssdev) > { > + struct sdi_device *sdi = dssdev_to_sdi(dssdev); > enum omap_channel channel = dssdev->dispc_channel; > > dss_mgr_disable(channel); > > - dss_sdi_disable(sdi.dss); > + dss_sdi_disable(sdi->dss); > > dispc_runtime_put(); > > - regulator_disable(sdi.vdds_sdi_reg); > + regulator_disable(sdi->vdds_sdi_reg); > } > > static void sdi_set_timings(struct omap_dss_device *dssdev, > struct videomode *vm) > { > - sdi.vm = *vm; > + struct sdi_device *sdi = dssdev_to_sdi(dssdev); > + > + sdi->vm = *vm; > } > > static void sdi_get_timings(struct omap_dss_device *dssdev, > struct videomode *vm) > { > - *vm = sdi.vm; > + struct sdi_device *sdi = dssdev_to_sdi(dssdev); > + > + *vm = sdi->vm; > } > > static int sdi_check_timings(struct omap_dss_device *dssdev, > @@ -254,21 +260,21 @@ static int sdi_check_timings(struct omap_dss_device *dssdev, > return 0; > } > > -static int sdi_init_regulator(void) > +static int sdi_init_regulator(struct sdi_device *sdi) > { > struct regulator *vdds_sdi; > > - if (sdi.vdds_sdi_reg) > + if (sdi->vdds_sdi_reg) > return 0; > > - vdds_sdi = devm_regulator_get(&sdi.pdev->dev, "vdds_sdi"); > + vdds_sdi = devm_regulator_get(&sdi->pdev->dev, "vdds_sdi"); > if (IS_ERR(vdds_sdi)) { > if (PTR_ERR(vdds_sdi) != -EPROBE_DEFER) > DSSERR("can't get VDDS_SDI regulator\n"); > return PTR_ERR(vdds_sdi); > } > > - sdi.vdds_sdi_reg = vdds_sdi; > + sdi->vdds_sdi_reg = vdds_sdi; > > return 0; > } > @@ -276,10 +282,11 @@ static int sdi_init_regulator(void) > static int sdi_connect(struct omap_dss_device *dssdev, > struct omap_dss_device *dst) > { > + struct sdi_device *sdi = dssdev_to_sdi(dssdev); > enum omap_channel channel = dssdev->dispc_channel; > int r; > > - r = sdi_init_regulator(); > + r = sdi_init_regulator(sdi); > if (r) > return r; > > @@ -325,11 +332,11 @@ static const struct omapdss_sdi_ops sdi_ops = { > .get_timings = sdi_get_timings, > }; > > -static void sdi_init_output(struct platform_device *pdev) > +static void sdi_init_output(struct sdi_device *sdi) > { > - struct omap_dss_device *out = &sdi.output; > + struct omap_dss_device *out = &sdi->output; > > - out->dev = &pdev->dev; > + out->dev = &sdi->pdev->dev; > out->id = OMAP_DSS_OUTPUT_SDI; > out->output_type = OMAP_DISPLAY_TYPE_SDI; > out->name = "sdi.0"; > @@ -342,23 +349,28 @@ static void sdi_init_output(struct platform_device *pdev) > omapdss_register_output(out); > } > > -static void sdi_uninit_output(struct platform_device *pdev) > +static void sdi_uninit_output(struct sdi_device *sdi) > { > - struct omap_dss_device *out = &sdi.output; > - > - omapdss_unregister_output(out); > + omapdss_unregister_output(&sdi->output); > } > > int sdi_init_port(struct dss_device *dss, struct platform_device *pdev, > struct device_node *port) > { > + struct sdi_device *sdi; > struct device_node *ep; > u32 datapairs; > int r; > > + sdi = kzalloc(sizeof(*sdi), GFP_KERNEL); > + if (!sdi) > + return -ENOMEM; > + > ep = of_get_next_child(port, NULL); > - if (!ep) > - return 0; > + if (!ep) { > + r = 0; > + goto err_free; > + } > > r = of_property_read_u32(ep, "datapairs", &datapairs); > if (r) { > @@ -366,29 +378,33 @@ int sdi_init_port(struct dss_device *dss, struct platform_device *pdev, > goto err_datapairs; > } > > - sdi.datapairs = datapairs; > - sdi.dss = dss; > + sdi->datapairs = datapairs; > + sdi->dss = dss; > > of_node_put(ep); > > - sdi.pdev = pdev; > + sdi->pdev = pdev; > + port->data = sdi; > > - sdi_init_output(pdev); > - > - sdi.port_initialized = true; > + sdi_init_output(sdi); > > return 0; > > err_datapairs: > of_node_put(ep); > +err_free: > + kfree(sdi); > > return r; > } > > void sdi_uninit_port(struct device_node *port) > { > - if (!sdi.port_initialized) > + struct sdi_device *sdi = port->data; > + > + if (!sdi) > return; > > - sdi_uninit_output(sdi.pdev); > + sdi_uninit_output(sdi); > + kfree(sdi); > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c index ac436826914a..a35dc51c5a6a 100644 --- a/drivers/gpu/drm/omapdrm/dss/sdi.c +++ b/drivers/gpu/drm/omapdrm/dss/sdi.c @@ -31,7 +31,7 @@ #include "omapdss.h" #include "dss.h" -static struct { +struct sdi_device { struct platform_device *pdev; struct dss_device *dss; @@ -43,9 +43,9 @@ static struct { int datapairs; struct omap_dss_device output; +}; - bool port_initialized; -} sdi; +#define dssdev_to_sdi(dssdev) container_of(dssdev, struct sdi_device, output) struct sdi_clk_calc_ctx { unsigned long pck_min, pck_max; @@ -77,9 +77,9 @@ static bool dpi_calc_dss_cb(unsigned long fck, void *data) dpi_calc_dispc_cb, ctx); } -static int sdi_calc_clock_div(unsigned long pclk, - unsigned long *fck, - struct dispc_clock_info *dispc_cinfo) +static int sdi_calc_clock_div(struct sdi_device *sdi, unsigned long pclk, + unsigned long *fck, + struct dispc_clock_info *dispc_cinfo) { int i; struct sdi_clk_calc_ctx ctx; @@ -101,7 +101,7 @@ static int sdi_calc_clock_div(unsigned long pclk, ctx.pck_min = 0; ctx.pck_max = pclk + 1000 * i * i * i; - ok = dss_div_calc(sdi.dss, pclk, ctx.pck_min, + ok = dss_div_calc(sdi->dss, pclk, ctx.pck_min, dpi_calc_dss_cb, &ctx); if (ok) { *fck = ctx.fck; @@ -113,26 +113,27 @@ static int sdi_calc_clock_div(unsigned long pclk, return -EINVAL; } -static void sdi_config_lcd_manager(struct omap_dss_device *dssdev) +static void sdi_config_lcd_manager(struct sdi_device *sdi) { - enum omap_channel channel = dssdev->dispc_channel; + enum omap_channel channel = sdi->output.dispc_channel; - sdi.mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS; + sdi->mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS; - sdi.mgr_config.stallmode = false; - sdi.mgr_config.fifohandcheck = false; + sdi->mgr_config.stallmode = false; + sdi->mgr_config.fifohandcheck = false; - sdi.mgr_config.video_port_width = 24; - sdi.mgr_config.lcden_sig_polarity = 1; + sdi->mgr_config.video_port_width = 24; + sdi->mgr_config.lcden_sig_polarity = 1; - dss_mgr_set_lcd_config(channel, &sdi.mgr_config); + dss_mgr_set_lcd_config(channel, &sdi->mgr_config); } static int sdi_display_enable(struct omap_dss_device *dssdev) { - struct omap_dss_device *out = &sdi.output; + struct sdi_device *sdi = dssdev_to_sdi(dssdev); + struct omap_dss_device *out = &sdi->output; enum omap_channel channel = dssdev->dispc_channel; - struct videomode *vm = &sdi.vm; + struct videomode *vm = &sdi->vm; unsigned long fck; struct dispc_clock_info dispc_cinfo; unsigned long pck; @@ -143,7 +144,7 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) return -ENODEV; } - r = regulator_enable(sdi.vdds_sdi_reg); + r = regulator_enable(sdi->vdds_sdi_reg); if (r) goto err_reg_enable; @@ -154,11 +155,11 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) /* 15.5.9.1.2 */ vm->flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_SYNC_POSEDGE; - r = sdi_calc_clock_div(vm->pixelclock, &fck, &dispc_cinfo); + r = sdi_calc_clock_div(sdi, vm->pixelclock, &fck, &dispc_cinfo); if (r) goto err_calc_clock_div; - sdi.mgr_config.clock_info = dispc_cinfo; + sdi->mgr_config.clock_info = dispc_cinfo; pck = fck / dispc_cinfo.lck_div / dispc_cinfo.pck_div; @@ -172,11 +173,11 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) dss_mgr_set_timings(channel, vm); - r = dss_set_fck_rate(sdi.dss, fck); + r = dss_set_fck_rate(sdi->dss, fck); if (r) goto err_set_dss_clock_div; - sdi_config_lcd_manager(dssdev); + sdi_config_lcd_manager(sdi); /* * LCLK and PCLK divisors are located in shadow registers, and we @@ -189,10 +190,10 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) * need to care about the shadow register mechanism for pck-free. The * exact reason for this is unknown. */ - dispc_mgr_set_clock_div(channel, &sdi.mgr_config.clock_info); + dispc_mgr_set_clock_div(channel, &sdi->mgr_config.clock_info); - dss_sdi_init(sdi.dss, sdi.datapairs); - r = dss_sdi_enable(sdi.dss); + dss_sdi_init(sdi->dss, sdi->datapairs); + r = dss_sdi_enable(sdi->dss); if (r) goto err_sdi_enable; mdelay(2); @@ -204,40 +205,45 @@ static int sdi_display_enable(struct omap_dss_device *dssdev) return 0; err_mgr_enable: - dss_sdi_disable(sdi.dss); + dss_sdi_disable(sdi->dss); err_sdi_enable: err_set_dss_clock_div: err_calc_clock_div: dispc_runtime_put(); err_get_dispc: - regulator_disable(sdi.vdds_sdi_reg); + regulator_disable(sdi->vdds_sdi_reg); err_reg_enable: return r; } static void sdi_display_disable(struct omap_dss_device *dssdev) { + struct sdi_device *sdi = dssdev_to_sdi(dssdev); enum omap_channel channel = dssdev->dispc_channel; dss_mgr_disable(channel); - dss_sdi_disable(sdi.dss); + dss_sdi_disable(sdi->dss); dispc_runtime_put(); - regulator_disable(sdi.vdds_sdi_reg); + regulator_disable(sdi->vdds_sdi_reg); } static void sdi_set_timings(struct omap_dss_device *dssdev, struct videomode *vm) { - sdi.vm = *vm; + struct sdi_device *sdi = dssdev_to_sdi(dssdev); + + sdi->vm = *vm; } static void sdi_get_timings(struct omap_dss_device *dssdev, struct videomode *vm) { - *vm = sdi.vm; + struct sdi_device *sdi = dssdev_to_sdi(dssdev); + + *vm = sdi->vm; } static int sdi_check_timings(struct omap_dss_device *dssdev, @@ -254,21 +260,21 @@ static int sdi_check_timings(struct omap_dss_device *dssdev, return 0; } -static int sdi_init_regulator(void) +static int sdi_init_regulator(struct sdi_device *sdi) { struct regulator *vdds_sdi; - if (sdi.vdds_sdi_reg) + if (sdi->vdds_sdi_reg) return 0; - vdds_sdi = devm_regulator_get(&sdi.pdev->dev, "vdds_sdi"); + vdds_sdi = devm_regulator_get(&sdi->pdev->dev, "vdds_sdi"); if (IS_ERR(vdds_sdi)) { if (PTR_ERR(vdds_sdi) != -EPROBE_DEFER) DSSERR("can't get VDDS_SDI regulator\n"); return PTR_ERR(vdds_sdi); } - sdi.vdds_sdi_reg = vdds_sdi; + sdi->vdds_sdi_reg = vdds_sdi; return 0; } @@ -276,10 +282,11 @@ static int sdi_init_regulator(void) static int sdi_connect(struct omap_dss_device *dssdev, struct omap_dss_device *dst) { + struct sdi_device *sdi = dssdev_to_sdi(dssdev); enum omap_channel channel = dssdev->dispc_channel; int r; - r = sdi_init_regulator(); + r = sdi_init_regulator(sdi); if (r) return r; @@ -325,11 +332,11 @@ static const struct omapdss_sdi_ops sdi_ops = { .get_timings = sdi_get_timings, }; -static void sdi_init_output(struct platform_device *pdev) +static void sdi_init_output(struct sdi_device *sdi) { - struct omap_dss_device *out = &sdi.output; + struct omap_dss_device *out = &sdi->output; - out->dev = &pdev->dev; + out->dev = &sdi->pdev->dev; out->id = OMAP_DSS_OUTPUT_SDI; out->output_type = OMAP_DISPLAY_TYPE_SDI; out->name = "sdi.0"; @@ -342,23 +349,28 @@ static void sdi_init_output(struct platform_device *pdev) omapdss_register_output(out); } -static void sdi_uninit_output(struct platform_device *pdev) +static void sdi_uninit_output(struct sdi_device *sdi) { - struct omap_dss_device *out = &sdi.output; - - omapdss_unregister_output(out); + omapdss_unregister_output(&sdi->output); } int sdi_init_port(struct dss_device *dss, struct platform_device *pdev, struct device_node *port) { + struct sdi_device *sdi; struct device_node *ep; u32 datapairs; int r; + sdi = kzalloc(sizeof(*sdi), GFP_KERNEL); + if (!sdi) + return -ENOMEM; + ep = of_get_next_child(port, NULL); - if (!ep) - return 0; + if (!ep) { + r = 0; + goto err_free; + } r = of_property_read_u32(ep, "datapairs", &datapairs); if (r) { @@ -366,29 +378,33 @@ int sdi_init_port(struct dss_device *dss, struct platform_device *pdev, goto err_datapairs; } - sdi.datapairs = datapairs; - sdi.dss = dss; + sdi->datapairs = datapairs; + sdi->dss = dss; of_node_put(ep); - sdi.pdev = pdev; + sdi->pdev = pdev; + port->data = sdi; - sdi_init_output(pdev); - - sdi.port_initialized = true; + sdi_init_output(sdi); return 0; err_datapairs: of_node_put(ep); +err_free: + kfree(sdi); return r; } void sdi_uninit_port(struct device_node *port) { - if (!sdi.port_initialized) + struct sdi_device *sdi = port->data; + + if (!sdi) return; - sdi_uninit_output(sdi.pdev); + sdi_uninit_output(sdi); + kfree(sdi); }
The sdi private data structure is currently stored as a global variable. While no platform with multiple SDI encoders currently exists nor is planned, this doesn't comply with the kernel device model and should thus be fixed. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/dss/sdi.c | 122 +++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 53 deletions(-)