| Message ID | 1394788369-5096-3-git-send-email-denis@eukrea.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
???????, 14 ????? 2014, 10:12 +01:00 ?? Denis Carikli <denis@eukrea.com>: > This commit is based on the following commit by Fabio Estevam: > 4344429 video: mxsfb: Introduce regulator support > > Signed-off-by: Denis Carikli <denis@eukrea.com> > --- ... > + if (np) { > + if (regulator_name) > + mx3fbi->reg_lcd = regulator_get(NULL, regulator_name); > + > + if (IS_ERR(mx3fbi->reg_lcd)) > + return PTR_ERR(mx3fbi->reg_lcd); > + } else { > + /* Permit that driver without a regulator in non-dt mode */ > + mx3fbi->reg_lcd = regulator_get(dev, "lcd"); > + } I am still completely do not understand why do you need to have "regulator_name" property? Why this cannot be devm_regulator_get(dev, "lcd") in both DT and non-DT case? ---
On 03/14/2014 10:23 AM, Alexander Shiyan wrote:
> Why this cannot be devm_regulator_get(dev, "lcd") in both DT and non-DT case?
I need to add device tree support to the mx3fb driver.
My first approach gave a binding that looked like that:
cmo_qvga: display {
model = "CMO-QVGA";
[...]
display-timings {
qvga_timings: 320x240 {
hactive = <320>;
vactive = <240>;
[...]
};
};
ipu: ipu@53fc0000 {
compatible = "fsl,imx31-ipu";
reg = < 0x53fc0000 0x5f
0x53fc0088 0x2b >;
interrupts = <42 41>;
dma-channels = <32>;
#dma-cells = <1>;
clocks = <&clks 55>;
clock-names = "";
};
lcdc: mx3fb@53fc00b4 {
compatible = "fsl,mx3-fb";
reg = <0x53fc00b4 0x0b>;
clocks = <&clks 55>;
dmas = <&ipu 14>;
dma-names = "tx";
display = <&cmo_qvga>;
};
The issue was that exporting the "dma ipu driver" was not a good idea.
I was told to instead make bindings that looks very similar to the ipuv3
driver[1]
So at the end that gave something like that:
cmo_qvga: display@di0 {
compatible = "fsl,mx3-parallel-display";
regulator-name = "lcd";
lcd-supply = <®_lcd_3v3>;
model = "CMO-QVGA";
display-timings {
qvga_timings: 320x240 {
hactive = <320>;
vactive = <240>;
[...]
};
};
ipu: ipu@53fc0000 {
compatible = "fsl,imx35-ipu";
reg = <0x53fc0000 0x4000>;
clocks = <&clks 55>;
display = <&cmo_qvga>;
};
So here fsl,imx35-ipu is binded to the mx3fb driver.
But the mx3fb driver still need to use the dma-ipu driver somehow.
That's why the dma-ipu driver is handled behind the scenes, that way
it's not exported to the device tree bindings.
Now, since the mx3fb driver is binded to the "fsl,imx35-ipu" compatible,
if I would do a "mx3fbi->reg_lcd = devm_regulator_get(dev, "lcd");",
that would then lookup for the regulator in the mx3fb node
(The last "ipu@53fc0000" here).
Instead the regulator can be found in the display node,
which has no driver associated with it.
In the case of the ipuv3, the parallel display driver is associated
with the display@di0 node, so the device matches with the device tree
node directly.
References:
-----------
[1] The ipuv3 driver is in drivers/staging/imx-drm/
[2] the dma ipu driver is in drivers/dma/ipu/
Denis.
???????, 14 ????? 2014, 12:23 +01:00 ?? Denis Carikli <denis@eukrea.com>: Sascha, Shawn, can you comment on that? > On 03/14/2014 10:23 AM, Alexander Shiyan wrote: > > Why this cannot be devm_regulator_get(dev, "lcd") in both DT and non-DT case? > > I need to add device tree support to the mx3fb driver. > My first approach gave a binding that looked like that: > > cmo_qvga: display { > model = "CMO-QVGA"; > [...] > display-timings { > qvga_timings: 320x240 { > hactive = <320>; > vactive = <240>; > [...] > }; > }; > > ipu: ipu@53fc0000 { > compatible = "fsl,imx31-ipu"; > reg = < 0x53fc0000 0x5f > 0x53fc0088 0x2b >; > interrupts = <42 41>; > dma-channels = <32>; > #dma-cells = <1>; > clocks = <&clks 55>; > clock-names = ""; > }; > > lcdc: mx3fb@53fc00b4 { > compatible = "fsl,mx3-fb"; > reg = <0x53fc00b4 0x0b>; > clocks = <&clks 55>; > dmas = <&ipu 14>; > dma-names = "tx"; > display = <&cmo_qvga>; > }; > > The issue was that exporting the "dma ipu driver" was not a good idea. > I was told to instead make bindings that looks very similar to the ipuv3 > driver[1] > So at the end that gave something like that: > > cmo_qvga: display@di0 { > compatible = "fsl,mx3-parallel-display"; > regulator-name = "lcd"; > lcd-supply = <®_lcd_3v3>; > model = "CMO-QVGA"; > display-timings { > qvga_timings: 320x240 { > hactive = <320>; > vactive = <240>; > [...] > }; > }; > > ipu: ipu@53fc0000 { > compatible = "fsl,imx35-ipu"; > reg = <0x53fc0000 0x4000>; > clocks = <&clks 55>; > display = <&cmo_qvga>; > }; > > So here fsl,imx35-ipu is binded to the mx3fb driver. > But the mx3fb driver still need to use the dma-ipu driver somehow. > That's why the dma-ipu driver is handled behind the scenes, that way > it's not exported to the device tree bindings. > > Now, since the mx3fb driver is binded to the "fsl,imx35-ipu" compatible, > if I would do a "mx3fbi->reg_lcd = devm_regulator_get(dev, "lcd");", > that would then lookup for the regulator in the mx3fb node > (The last "ipu@53fc0000" here). > > Instead the regulator can be found in the display node, > which has no driver associated with it. > > In the case of the ipuv3, the parallel display driver is associated > with the display@di0 node, so the device matches with the device tree > node directly. > > References: > ----------- > [1] The ipuv3 driver is in drivers/staging/imx-drm/ > [2] the dma ipu driver is in drivers/dma/ipu/ > ---
[Added Mark to Cc] On Fri, Mar 14, 2014 at 10:12:47AM +0100, Denis Carikli wrote: > > + of_property_read_string(display_np, "regulator-name", > + ®ulator_name); > + [...] > + /* In dt mode, > + * using devm_regulator_get would require that the proprety referencing > + * the regulator phandle has to be inside the mx3fb node. > + */ > + if (np) { > + if (regulator_name) > + mx3fbi->reg_lcd = regulator_get(NULL, regulator_name); > + > + if (IS_ERR(mx3fbi->reg_lcd)) > + return PTR_ERR(mx3fbi->reg_lcd); > + } else { > + /* Permit that driver without a regulator in non-dt mode */ > + mx3fbi->reg_lcd = regulator_get(dev, "lcd"); > + } This patch adds regulator support for the display of a i.MX3 IPU. The problem Denis has to solve here is that he needs to get the regulator, but the display devicenode doesn't have a struct device associated with it, so he cannot provide one to regulator_get(). One way out here could be a of_regulator_get(struct device_node *). Mark, would this be ok with you? (Of course a proper driver for the display would be nicer, but this would immediately bring us to some Common display framework, something which is being worked on quite a while now without success) Sascha
On 03/17/2014 07:20 AM, Sascha Hauer wrote: > On Fri, Mar 14, 2014 at 10:12:47AM +0100, Denis Carikli wrote: >> >> + of_property_read_string(display_np, "regulator-name", >> + ®ulator_name); >> + > > [...] > >> + /* In dt mode, >> + * using devm_regulator_get would require that the proprety referencing >> + * the regulator phandle has to be inside the mx3fb node. >> + */ >> + if (np) { >> + if (regulator_name) >> + mx3fbi->reg_lcd = regulator_get(NULL, regulator_name); >> + >> + if (IS_ERR(mx3fbi->reg_lcd)) >> + return PTR_ERR(mx3fbi->reg_lcd); >> + } else { >> + /* Permit that driver without a regulator in non-dt mode */ >> + mx3fbi->reg_lcd = regulator_get(dev, "lcd"); >> + } > > This patch adds regulator support for the display of a i.MX3 IPU. The > problem Denis has to solve here is that he needs to get the regulator, > but the display devicenode doesn't have a struct device associated with > it, so he cannot provide one to regulator_get(). One way out here could > be a of_regulator_get(struct device_node *). Mark, would this be ok with > you? Here, the display devicenode has no struct device associated with it like mentioned above. Because of that, retriving the regulator from the devicetree, for instance like regulator_dev_lookup() does, would require a different approach. As I understand it, what happen in regulator_dev_lookup when regulator_get(NULL, regulator_name) is used is the following: Since the struct device is NULL, it skips the struct device based lookup, but also the devicetree lookup (it uses dev->of_node for that) At the end it falls back on a match on the regulator name to find it. would keeping regulator_get(NULL, regulator_name); in the driver instead be better for now? Denis.
On 03/14/2014 10:12 AM, Denis Carikli wrote: > + /* In dt mode, > + * using devm_regulator_get would require that the proprety referencing > + * the regulator phandle has to be inside the mx3fb node. > + */ > + if (np) { > + if (regulator_name) > + mx3fbi->reg_lcd = regulator_get(NULL, regulator_name); mx3fbi->reg_lcd is NULL if no regulator is present in the dts(i). I'll fix it in the driver (instead of forcing the use of a dummy regulator). Denis.
On Tue, Jun 10, 2014 at 03:29:33PM +0200, Denis Carikli wrote: > On 03/17/2014 07:20 AM, Sascha Hauer wrote: > >This patch adds regulator support for the display of a i.MX3 IPU. The > >problem Denis has to solve here is that he needs to get the regulator, > >but the display devicenode doesn't have a struct device associated with > >it, so he cannot provide one to regulator_get(). One way out here could > >be a of_regulator_get(struct device_node *). Mark, would this be ok with > >you? > Here, the display devicenode has no struct device associated with it like > mentioned above. > Because of that, retriving the regulator from the devicetree, for instance > like regulator_dev_lookup() does, would require a different approach. > As I understand it, what happen in regulator_dev_lookup when > regulator_get(NULL, regulator_name) is used is the following: > Since the struct device is NULL, it skips the struct device based lookup, > but also the devicetree lookup (it uses dev->of_node for that) > At the end it falls back on a match on the regulator name to find it. > would keeping regulator_get(NULL, regulator_name); in the driver instead be > better for now? It would be much better to have an actual device to get things for, using a hard coded regulator name is very much deprecated. Using a hard coded name is fragile and we don't have support for mapping this properly in the device tree.
diff --git a/drivers/video/fbdev/mx3fb.c b/drivers/video/fbdev/mx3fb.c index 952d2b5..40b47dd 100644 --- a/drivers/video/fbdev/mx3fb.c +++ b/drivers/video/fbdev/mx3fb.c @@ -27,6 +27,7 @@ #include <linux/clk.h> #include <linux/mutex.h> #include <linux/dma/ipu-dma.h> +#include <linux/regulator/consumer.h> #include <linux/platform_data/dma-imx.h> #include <linux/platform_data/video-mx3fb.h> @@ -273,6 +274,7 @@ struct mx3fb_info { struct dma_async_tx_descriptor *txd; dma_cookie_t cookie; struct scatterlist sg[2]; + struct regulator *reg_lcd; struct fb_var_screeninfo cur_var; /* current var info */ uint32_t flags; @@ -1042,6 +1044,12 @@ static void __blank(int blank, struct fb_info *fbi) case FB_BLANK_HSYNC_SUSPEND: case FB_BLANK_NORMAL: sdc_set_brightness(mx3fb, 0); + if (!IS_ERR(mx3_fbi->reg_lcd)) { + if (regulator_disable(mx3_fbi->reg_lcd)) { + dev_err(fbi->device, + "Failed to disable regulator.\n"); + } + } memset((char *)fbi->screen_base, 0, fbi->fix.smem_len); /* Give LCD time to update - enough for 50 and 60 Hz */ msleep(25); @@ -1049,6 +1057,12 @@ static void __blank(int blank, struct fb_info *fbi) break; case FB_BLANK_UNBLANK: sdc_enable_channel(mx3_fbi); + if (!IS_ERR(mx3_fbi->reg_lcd)) { + if (regulator_enable(mx3_fbi->reg_lcd)) { + dev_err(fbi->device, + "Failed to enable regulator.\n"); + } + } sdc_set_brightness(mx3fb, mx3fb->backlight_level); break; } @@ -1233,7 +1247,12 @@ static int mx3fb_suspend(struct platform_device *pdev, pm_message_t state) if (mx3_fbi->blank == FB_BLANK_UNBLANK) { sdc_disable_channel(mx3_fbi); sdc_set_brightness(mx3fb, 0); - + if (!IS_ERR(mx3_fbi->reg_lcd)) { + if (regulator_disable(mx3_fbi->reg_lcd)) { + dev_err(&pdev->dev, + "Failed to disable regulator.\n"); + } + } } return 0; } @@ -1249,6 +1268,12 @@ static int mx3fb_resume(struct platform_device *pdev) if (mx3_fbi->blank == FB_BLANK_UNBLANK) { sdc_enable_channel(mx3_fbi); sdc_set_brightness(mx3fb, mx3fb->backlight_level); + if (!IS_ERR(mx3_fbi->reg_lcd)) { + if (regulator_enable(mx3_fbi->reg_lcd)) { + dev_err(&pdev->dev, + "Failed to enable regulator.\n"); + } + } } console_lock(); @@ -1394,6 +1419,7 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan) struct mx3fb_platform_data *mx3fb_pdata = dev_get_platdata(dev); struct device_node *np = dev->of_node; const char *name; + const char *regulator_name; const char *ipu_disp_format; unsigned int irq; struct fb_info *fbi; @@ -1409,6 +1435,9 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan) return -EINVAL; } + of_property_read_string(display_np, "regulator-name", + ®ulator_name); + of_property_read_string(display_np, "interface-pix-fmt", &ipu_disp_format); if (!ipu_disp_format) { @@ -1526,6 +1555,21 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan) if (ret < 0) goto esetpar; + /* In dt mode, + * using devm_regulator_get would require that the proprety referencing + * the regulator phandle has to be inside the mx3fb node. + */ + if (np) { + if (regulator_name) + mx3fbi->reg_lcd = regulator_get(NULL, regulator_name); + + if (IS_ERR(mx3fbi->reg_lcd)) + return PTR_ERR(mx3fbi->reg_lcd); + } else { + /* Permit that driver without a regulator in non-dt mode */ + mx3fbi->reg_lcd = regulator_get(dev, "lcd"); + } + __blank(FB_BLANK_UNBLANK, fbi); dev_info(dev, "registered, using mode %s\n", fb_mode); @@ -1589,6 +1633,7 @@ static int mx3fb_probe(struct platform_device *pdev) dma_cap_mask_t mask; struct dma_chan *chan; struct dma_chan_request rq; + struct mx3fb_info *mx3_fbi; struct device_node *np = dev->of_node; struct videomode *vm; struct fb_videomode *fb_vm; @@ -1673,6 +1718,12 @@ ersdc0: dmaengine_put(); iounmap(mx3fb->reg_base); eremap: + if (mx3fb->fbi) { + mx3_fbi = mx3fb->fbi->par; + + if ((!IS_ERR(mx3_fbi->reg_lcd)) && mx3_fbi->reg_lcd) + regulator_put(mx3_fbi->reg_lcd); + } dev_err(dev, "mx3fb: failed to register fb\n"); return ret; } @@ -1684,6 +1735,9 @@ static int mx3fb_remove(struct platform_device *dev) struct mx3fb_info *mx3_fbi = fbi->par; struct dma_chan *chan; + if ((!IS_ERR(mx3_fbi->reg_lcd)) && mx3_fbi->reg_lcd) + regulator_put(mx3_fbi->reg_lcd); + chan = &mx3_fbi->idmac_channel->dma_chan; release_fbi(fbi);
This commit is based on the following commit by Fabio Estevam: 4344429 video: mxsfb: Introduce regulator support Signed-off-by: Denis Carikli <denis@eukrea.com> --- ChangeLog v6->v7: - Removed the Cc from the patch, they went into git send-email instead. ChangeLog v5->v6: - Shrinked the Cc list. - still permit non-dt boards to use that driver without a regulator. ChangeLog v4->v5: - Added Shawn Guo in the Cc list. - Rebased to make it apply. ChangeLog v3->v4: - Some code style fixes. - Improved error handling in eremap. ChangeLog v2->v3: - The prints are now replaced with non line wrapped prints. - The regulator retrival has been adapted to the new DT bindings which looks more like the IPUv3 ones. - The regulator_is_enabled checks were kept, because regulator_disable do not do such check. --- drivers/video/mx3fb.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)