[PATCHv5][ 5/8] staging: imx-drm: parallel display: add regulator support.

[Thierry Reding]

On Thu, Dec 05, 2013 at 07:28:09PM +0100, Denis Carikli wrote:
[...]
> diff --git a/drivers/staging/imx-drm/parallel-display.c 
> b/drivers/staging/imx-drm/parallel-display.c
[...]
> @@ -260,6 +275,13 @@ static int imx_pd_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
>  
> + imxpd->disp_reg = devm_regulator_get(>dev, "display");
> + if (PTR_ERR(imxpd->disp_reg) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (IS_ERR(imxpd->disp_reg))
> + dev_dbg(>dev, "Operating without display regulator.\n");

I don't think this is necessary. There is code in the regulator core
nowadays that supplies a dummy regulator if one hasn't been hooked up in
devicetree explicitly. So any error that you get at this point is likely
a valid one rather than just a missing regulator.

The advantage is that you no longer have to check at every step of the
way that the regulator is valid before calling the regulator API.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCHv5][ 5/8] staging: imx-drm: parallel display: add regulator support.

[Marek Vasut]

On Thursday, December 05, 2013 at 07:28:09 PM, Denis Carikli wrote:
> Cc: Dan Carpenter 
> Cc: Rob Herring 
> Cc: Pawel Moll 
> Cc: Mark Rutland 
> Cc: Stephen Warren 
> Cc: Ian Campbell 
> Cc: devicetree at vger.kernel.org
> Cc: Greg Kroah-Hartman 
> Cc: driverdev-devel at linuxdriverproject.org
> Cc: David Airlie 
> Cc: dri-devel at lists.freedesktop.org
> Cc: Sascha Hauer 
> Cc: Shawn Guo 
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Eric B?nard 
> Signed-off-by: Denis Carikli 
> ---
> ChangeLog v3->v5:
> - Code clenaup.
> 
> ChangeLog v2->v3:
> - Added some interested people in the Cc list.
> - the lcd-supply is now called display-supply (not all display are LCD).
> - The code and documentation was updated accordingly.
> - regulator_is_enabled now guard the regulator enables/disables because
>   regulator_disable does not check the regulator state.
> ---
>  .../bindings/staging/imx-drm/fsl-imx-drm.txt   |1 +
>  drivers/staging/imx-drm/parallel-display.c |   22
>  2 files changed, 23 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt index
> 2d24425..4dd7ce5 100644
> --- a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> +++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> @@ -28,6 +28,7 @@ Required properties:
>  - compatible: Should be "fsl,imx-parallel-display"
>  - crtc: the crtc this display is connected to, see below
>  Optional properties:
> +- display-supply : phandle to the regulator device tree node if needed.
>  - interface_pix_fmt: How this display is connected to the
>crtc. Currently supported types: "rgb24", "rgb565", "bgr666", "rgb666"
>  - edid: verbatim EDID data block describing attached display.
> diff --git a/drivers/staging/imx-drm/parallel-display.c
> b/drivers/staging/imx-drm/parallel-display.c index 65d0c18..61c0aeb 100644
> --- a/drivers/staging/imx-drm/parallel-display.c
> +++ b/drivers/staging/imx-drm/parallel-display.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  #include "imx-drm.h"
> @@ -35,6 +36,7 @@ struct imx_parallel_display {
>   struct drm_encoder encoder;
>   struct imx_drm_encoder *imx_drm_encoder;
>   struct device *dev;
> + struct regulator *disp_reg;
>   void *edid;
>   int edid_len;
>   u32 interface_pix_fmt;
> @@ -141,6 +143,13 @@ static void imx_pd_encoder_prepare(struct drm_encoder
> *encoder) {
>   struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> 
> + if (!IS_ERR(imxpd->disp_reg) &&
> + !regulator_is_enabled(imxpd->disp_reg)) {
> + if (regulator_enable(imxpd->disp_reg))
> + dev_err(imxpd->dev,
> + "Failed to enable regulator.\n");

I wonder, is this linebreak needed for this function call ?
[...]

[PATCHv5][ 5/8] staging: imx-drm: parallel display: add regulator support.

[Denis Carikli]

Cc: Dan Carpenter 
Cc: Rob Herring 
Cc: Pawel Moll 
Cc: Mark Rutland 
Cc: Stephen Warren 
Cc: Ian Campbell 
Cc: devicetree at vger.kernel.org
Cc: Greg Kroah-Hartman 
Cc: driverdev-devel at linuxdriverproject.org
Cc: David Airlie 
Cc: dri-devel at lists.freedesktop.org
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: linux-arm-kernel at lists.infradead.org
Cc: Eric B?nard 
Signed-off-by: Denis Carikli 
---
ChangeLog v3->v5:
- Code clenaup.

ChangeLog v2->v3:
- Added some interested people in the Cc list.
- the lcd-supply is now called display-supply (not all display are LCD).
- The code and documentation was updated accordingly.
- regulator_is_enabled now guard the regulator enables/disables because
  regulator_disable does not check the regulator state.
---
 .../bindings/staging/imx-drm/fsl-imx-drm.txt   |1 +
 drivers/staging/imx-drm/parallel-display.c |   22 
 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt 
b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
index 2d24425..4dd7ce5 100644
--- a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
+++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
@@ -28,6 +28,7 @@ Required properties:
 - compatible: Should be "fsl,imx-parallel-display"
 - crtc: the crtc this display is connected to, see below
 Optional properties:
+- display-supply : phandle to the regulator device tree node if needed.
 - interface_pix_fmt: How this display is connected to the
   crtc. Currently supported types: "rgb24", "rgb565", "bgr666", "rgb666"
 - edid: verbatim EDID data block describing attached display.
diff --git a/drivers/staging/imx-drm/parallel-display.c 
b/drivers/staging/imx-drm/parallel-display.c
index 65d0c18..61c0aeb 100644
--- a/drivers/staging/imx-drm/parallel-display.c
+++ b/drivers/staging/imx-drm/parallel-display.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #include "imx-drm.h"
@@ -35,6 +36,7 @@ struct imx_parallel_display {
struct drm_encoder encoder;
struct imx_drm_encoder *imx_drm_encoder;
struct device *dev;
+   struct regulator *disp_reg;
void *edid;
int edid_len;
u32 interface_pix_fmt;
@@ -141,6 +143,13 @@ static void imx_pd_encoder_prepare(struct drm_encoder 
*encoder)
 {
struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);

+   if (!IS_ERR(imxpd->disp_reg) &&
+   !regulator_is_enabled(imxpd->disp_reg)) {
+   if (regulator_enable(imxpd->disp_reg))
+   dev_err(imxpd->dev,
+   "Failed to enable regulator.\n");
+   }
+
imx_drm_crtc_panel_format(encoder->crtc, DRM_MODE_ENCODER_NONE,
imxpd->interface_pix_fmt);
 }
@@ -157,6 +166,12 @@ static void imx_pd_encoder_mode_set(struct drm_encoder 
*encoder,

 static void imx_pd_encoder_disable(struct drm_encoder *encoder)
 {
+   struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+
+   if (!IS_ERR(imxpd->disp_reg) && regulator_is_enabled(imxpd->disp_reg)) {
+   if (regulator_disable(imxpd->disp_reg))
+   dev_err(imxpd->dev, "Failed to disable regulator.\n");
+   }
 }

 static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
@@ -260,6 +275,13 @@ static int imx_pd_probe(struct platform_device *pdev)
if (ret)
return ret;

+   imxpd->disp_reg = devm_regulator_get(>dev, "display");
+   if (PTR_ERR(imxpd->disp_reg) == -EPROBE_DEFER)
+   return -EPROBE_DEFER;
+
+   if (IS_ERR(imxpd->disp_reg))
+   dev_dbg(>dev, "Operating without display regulator.\n");
+
ret = imx_drm_encoder_add_possible_crtcs(imxpd->imx_drm_encoder, np);

platform_set_drvdata(pdev, imxpd);
-- 
1.7.9.5