| Message ID | 1369906493-27538-7-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12:34 Thu 30 May , Tomi Valkeinen wrote: > On some platforms DPI requires a regulator to be enabled to power up the > output pins. This regulator is, for some reason, currently attached to > the virtual omapdss device, instead of the DPI device. This does not > work for DT, as the regulator mappings need to be described in the DT > data, and the virtual omapdss device is not present there. > > Fix the issue by acquiring the regulator in the DPI device. To retain > compatibility with the current board files, the old method of getting > the regulator is kept. The old method can be removed when the board > files have been changed to pass the regulator to DPI. as discuss with Arnd we should handle regular enable and disable at device probe for every device as we do for pinctrl Best Regards, J. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/dpi.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c > index ef8fca2..43f6b26 100644 > --- a/drivers/video/omap2/dss/dpi.c > +++ b/drivers/video/omap2/dss/dpi.c > @@ -37,7 +37,10 @@ > #include "dss_features.h" > > static struct { > + struct platform_device *pdev; > + > struct regulator *vdds_dsi_reg; > + bool vdds_dsi_from_core; > struct platform_device *dsidev; > > struct mutex lock; > @@ -583,10 +586,15 @@ static int dpi_init_display(struct omap_dss_device *dssdev) > struct regulator *vdds_dsi; > > vdds_dsi = dss_get_vdds_dsi(); > - > if (IS_ERR(vdds_dsi)) { > - DSSERR("can't get VDDS_DSI regulator\n"); > - return PTR_ERR(vdds_dsi); > + vdds_dsi = regulator_get(&dpi.pdev->dev, "vdds_dsi"); > + if (IS_ERR(vdds_dsi)) { > + DSSERR("can't get VDDS_DSI regulator\n"); > + return PTR_ERR(vdds_dsi); > + } > + dpi.vdds_dsi_from_core = false; > + } else { > + dpi.vdds_dsi_from_core = true; > } > > dpi.vdds_dsi_reg = vdds_dsi; > @@ -702,6 +710,8 @@ static int omap_dpi_probe(struct platform_device *pdev) > { > int r; > > + dpi.pdev = pdev; > + > mutex_init(&dpi.lock); > > dpi_init_output(pdev); > @@ -725,6 +735,11 @@ static int __exit omap_dpi_remove(struct platform_device *pdev) > > dpi_uninit_output(pdev); > > + if (dpi.vdds_dsi_reg != NULL && dpi.vdds_dsi_from_core == false) { > + regulator_put(dpi.vdds_dsi_reg); > + dpi.vdds_dsi_reg = NULL; > + } > + > return 0; > } > > -- > 1.8.1.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30/05/13 14:12, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 12:34 Thu 30 May , Tomi Valkeinen wrote: >> On some platforms DPI requires a regulator to be enabled to power up the >> output pins. This regulator is, for some reason, currently attached to >> the virtual omapdss device, instead of the DPI device. This does not >> work for DT, as the regulator mappings need to be described in the DT >> data, and the virtual omapdss device is not present there. >> >> Fix the issue by acquiring the regulator in the DPI device. To retain >> compatibility with the current board files, the old method of getting >> the regulator is kept. The old method can be removed when the board >> files have been changed to pass the regulator to DPI. > > as discuss with Arnd we should handle regular enable and disable at device > probe for every device as we do for pinctrl I'm not sure what you mean. Enable of what? The regulator? Why would we enable it in the device's probe, as the device may never even be used? Or do you mean regulator_get()? Doing that in DPI driver's probe is problematic, as not all boards have any panels using DPI, and thus they may not have the regulator set up, but all boards have the DPI device added automatically. So for those boards the DPI probe would fail, always, if we tried to get the regulator. Tomi
On Thursday 30 May 2013, Tomi Valkeinen wrote: > On 30/05/13 14:12, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 12:34 Thu 30 May , Tomi Valkeinen wrote: > >> On some platforms DPI requires a regulator to be enabled to power up the > >> output pins. This regulator is, for some reason, currently attached to > >> the virtual omapdss device, instead of the DPI device. This does not > >> work for DT, as the regulator mappings need to be described in the DT > >> data, and the virtual omapdss device is not present there. > >> > >> Fix the issue by acquiring the regulator in the DPI device. To retain > >> compatibility with the current board files, the old method of getting > >> the regulator is kept. The old method can be removed when the board > >> files have been changed to pass the regulator to DPI. > > > > as discuss with Arnd we should handle regular enable and disable at device > > probe for every device as we do for pinctrl > > I'm not sure what you mean. Enable of what? The regulator? Why would we > enable it in the device's probe, as the device may never even be used? It's an idea I had a while ago, but not yet discussed in the open. Jean-Christophe just posted patches to move the mapping of interrupt numbers into platform_drv_probe(), just before calling the driver ->probe() callback, and we already have similar code to set up the default pinctrl state of a device before calling probe(). This can be extended to further subsystems, but that has to be done carefully to avoid regressions. Ideally we would move a lot of boilerplate code out of the driver specific ->probe() function into common code. Possible candidates for this include: * calling devm_request_mem_region for the "reg" property * calling devm_ioremap on the "reg" property" * calling devm_gpio_request for all gpio lines * calling devm_regulator_get on all regulators * calling devm_reset_control_get on all reset lines * calling devm_dma_request_slave_channel on all dma channels * calling devm_of_pwm_get for all pwm channels * ... For most of these (maybe all), I think we need some form of opt-in model on the driver side because there are cases where aquiring some of these resources is not mandatory, and it only works if the driver is using DT probing. IF we want to do this, it also needs a lot of thought, and we shouldn't do it carelessly. We might also need some extra infrastructure in revres to simplify access to the resources we got from the OF node. The irq resources are particularly trivial because we already claim them in of_platform_populate, so moving that to platform_drv_probe() is straightforward and solves existing bugs without creating a huge regression potential, but it's harder for the others. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15:05 Thu 30 May , Arnd Bergmann wrote: > On Thursday 30 May 2013, Tomi Valkeinen wrote: > > On 30/05/13 14:12, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > On 12:34 Thu 30 May , Tomi Valkeinen wrote: > > >> On some platforms DPI requires a regulator to be enabled to power up the > > >> output pins. This regulator is, for some reason, currently attached to > > >> the virtual omapdss device, instead of the DPI device. This does not > > >> work for DT, as the regulator mappings need to be described in the DT > > >> data, and the virtual omapdss device is not present there. > > >> > > >> Fix the issue by acquiring the regulator in the DPI device. To retain > > >> compatibility with the current board files, the old method of getting > > >> the regulator is kept. The old method can be removed when the board > > >> files have been changed to pass the regulator to DPI. > > > > > > as discuss with Arnd we should handle regular enable and disable at device > > > probe for every device as we do for pinctrl > > > > I'm not sure what you mean. Enable of what? The regulator? Why would we > > enable it in the device's probe, as the device may never even be used? > > It's an idea I had a while ago, but not yet discussed in the open. > > Jean-Christophe just posted patches to move the mapping of interrupt numbers > into platform_drv_probe(), just before calling the driver ->probe() callback, > and we already have similar code to set up the default pinctrl state of > a device before calling probe(). > > This can be extended to further subsystems, but that has to be done > carefully to avoid regressions. Ideally we would move a lot of boilerplate > code out of the driver specific ->probe() function into common code. > Possible candidates for this include: > > * calling devm_request_mem_region for the "reg" property > * calling devm_ioremap on the "reg" property" > * calling devm_gpio_request for all gpio lines > * calling devm_regulator_get on all regulators > * calling devm_reset_control_get on all reset lines > * calling devm_dma_request_slave_channel on all dma channels > * calling devm_of_pwm_get for all pwm channels > * ... > > For most of these (maybe all), I think we need some form of opt-in > model on the driver side because there are cases where aquiring some > of these resources is not mandatory, and it only works if the driver > is using DT probing. > > IF we want to do this, it also needs a lot of thought, and we shouldn't > do it carelessly. We might also need some extra infrastructure in revres > to simplify access to the resources we got from the OF node. > > The irq resources are particularly trivial because we already claim > them in of_platform_populate, so moving that to platform_drv_probe() > is straightforward and solves existing bugs without creating a huge > regression potential, but it's harder for the others. Yeah I agree with Arnd we need to start to move this way but for DT only first and carefully Best Regards, J. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 May 2013 03:04 PM, Tomi Valkeinen wrote: > On some platforms DPI requires a regulator to be enabled to power up the > output pins. This regulator is, for some reason, currently attached to > the virtual omapdss device, instead of the DPI device. This does not > work for DT, as the regulator mappings need to be described in the DT > data, and the virtual omapdss device is not present there. > > Fix the issue by acquiring the regulator in the DPI device. To retain > compatibility with the current board files, the old method of getting > the regulator is kept. The old method can be removed when the board > files have been changed to pass the regulator to DPI. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/dpi.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c > index ef8fca2..43f6b26 100644 > --- a/drivers/video/omap2/dss/dpi.c > +++ b/drivers/video/omap2/dss/dpi.c > @@ -37,7 +37,10 @@ > #include "dss_features.h" > > static struct { > + struct platform_device *pdev; We don't really need to add a new member here for getting the output's device pointer. It's already there in dpi.output.dev. Archit > + > struct regulator *vdds_dsi_reg; > + bool vdds_dsi_from_core; > struct platform_device *dsidev; > > struct mutex lock; > @@ -583,10 +586,15 @@ static int dpi_init_display(struct omap_dss_device *dssdev) > struct regulator *vdds_dsi; > > vdds_dsi = dss_get_vdds_dsi(); > - > if (IS_ERR(vdds_dsi)) { > - DSSERR("can't get VDDS_DSI regulator\n"); > - return PTR_ERR(vdds_dsi); > + vdds_dsi = regulator_get(&dpi.pdev->dev, "vdds_dsi"); > + if (IS_ERR(vdds_dsi)) { > + DSSERR("can't get VDDS_DSI regulator\n"); > + return PTR_ERR(vdds_dsi); > + } > + dpi.vdds_dsi_from_core = false; > + } else { > + dpi.vdds_dsi_from_core = true; > } > > dpi.vdds_dsi_reg = vdds_dsi; > @@ -702,6 +710,8 @@ static int omap_dpi_probe(struct platform_device *pdev) > { > int r; > > + dpi.pdev = pdev; > + > mutex_init(&dpi.lock); > > dpi_init_output(pdev); > @@ -725,6 +735,11 @@ static int __exit omap_dpi_remove(struct platform_device *pdev) > > dpi_uninit_output(pdev); > > + if (dpi.vdds_dsi_reg != NULL && dpi.vdds_dsi_from_core == false) { > + regulator_put(dpi.vdds_dsi_reg); > + dpi.vdds_dsi_reg = NULL; > + } > + > return 0; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c index ef8fca2..43f6b26 100644 --- a/drivers/video/omap2/dss/dpi.c +++ b/drivers/video/omap2/dss/dpi.c @@ -37,7 +37,10 @@ #include "dss_features.h" static struct { + struct platform_device *pdev; + struct regulator *vdds_dsi_reg; + bool vdds_dsi_from_core; struct platform_device *dsidev; struct mutex lock; @@ -583,10 +586,15 @@ static int dpi_init_display(struct omap_dss_device *dssdev) struct regulator *vdds_dsi; vdds_dsi = dss_get_vdds_dsi(); - if (IS_ERR(vdds_dsi)) { - DSSERR("can't get VDDS_DSI regulator\n"); - return PTR_ERR(vdds_dsi); + vdds_dsi = regulator_get(&dpi.pdev->dev, "vdds_dsi"); + if (IS_ERR(vdds_dsi)) { + DSSERR("can't get VDDS_DSI regulator\n"); + return PTR_ERR(vdds_dsi); + } + dpi.vdds_dsi_from_core = false; + } else { + dpi.vdds_dsi_from_core = true; } dpi.vdds_dsi_reg = vdds_dsi; @@ -702,6 +710,8 @@ static int omap_dpi_probe(struct platform_device *pdev) { int r; + dpi.pdev = pdev; + mutex_init(&dpi.lock); dpi_init_output(pdev); @@ -725,6 +735,11 @@ static int __exit omap_dpi_remove(struct platform_device *pdev) dpi_uninit_output(pdev); + if (dpi.vdds_dsi_reg != NULL && dpi.vdds_dsi_from_core == false) { + regulator_put(dpi.vdds_dsi_reg); + dpi.vdds_dsi_reg = NULL; + } + return 0; }
On some platforms DPI requires a regulator to be enabled to power up the output pins. This regulator is, for some reason, currently attached to the virtual omapdss device, instead of the DPI device. This does not work for DT, as the regulator mappings need to be described in the DT data, and the virtual omapdss device is not present there. Fix the issue by acquiring the regulator in the DPI device. To retain compatibility with the current board files, the old method of getting the regulator is kept. The old method can be removed when the board files have been changed to pass the regulator to DPI. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/omap2/dss/dpi.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)