| Message ID | 1323074921-29291-1-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Dec 05, 2011 at 02:18:41PM +0530, Thomas Abraham wrote: > The power source to the lcd panel or the lcd interface such as lvds > transmitters could be controlled by a regulator supply. Add support > for enabling/disabling the regulator when switching power to lcd. > > Two new elements 'min_uV' and 'max_uV' in the platform data are added > to allow platform code to specifiy the desired output voltage from the > regulator. > > Cc: Ben Dooks <ben-linux@fluff.org> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/video/backlight/platform_lcd.c | 29 +++++++++++++++++++++++++++++ > include/video/platform_lcd.h | 7 +++++++ > 2 files changed, 36 insertions(+), 0 deletions(-) > > diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c > index 302330a..ffa8108 100644 > --- a/drivers/video/backlight/platform_lcd.c > +++ b/drivers/video/backlight/platform_lcd.c > @@ -17,6 +17,8 @@ > #include <linux/backlight.h> > #include <linux/lcd.h> > #include <linux/slab.h> > +#include <linux/regulator/consumer.h> > +#include <linux/regulator/machine.h> > > #include <video/platform_lcd.h> > > @@ -44,11 +46,38 @@ static int platform_lcd_get_power(struct lcd_device *lcd) > static int platform_lcd_set_power(struct lcd_device *lcd, int power) > { > struct platform_lcd *plcd = to_our_lcd(lcd); > + struct regulator *lcd_regulator; > int lcd_power = 1; > > if (power == FB_BLANK_POWERDOWN || plcd->suspended) > lcd_power = 0; > > + /* > + * If power to lcd and/or lcd interface is controlled using a regulator, > + * enable or disable the regulator based in the power setting. > + */ > + lcd_regulator = regulator_get(plcd->us, "vcc_lcd"); > + if (IS_ERR(lcd_regulator)) { > + dev_info(plcd->us, "could not get regulator\n"); > + goto set_power; > + } > + > + if (lcd_power) { > + if (plcd->pdata->min_uV || plcd->pdata->max_uV) > + if (regulator_set_voltage(lcd_regulator, > + plcd->pdata->min_uV, plcd->pdata->max_uV)) > + dev_info(plcd->us, > + "regulator voltage set failed\n"); > + > + if (regulator_enable(lcd_regulator)) > + dev_info(plcd->us, "failed to enable regulator\n"); > + } else { > + regulator_disable(lcd_regulator); > + } > + > + regulator_put(lcd_regulator); I wonder why you ->get() and ->put() everytime here. Wouldn't it be enough to ->get() on probe() and ->put() on remove() ??
Hi Felipe, On 5 December 2011 14:22, Felipe Balbi <balbi@ti.com> wrote: [...] >> + if (lcd_power) { >> + if (plcd->pdata->min_uV || plcd->pdata->max_uV) >> + if (regulator_set_voltage(lcd_regulator, >> + plcd->pdata->min_uV, plcd->pdata->max_uV)) >> + dev_info(plcd->us, >> + "regulator voltage set failed\n"); >> + >> + if (regulator_enable(lcd_regulator)) >> + dev_info(plcd->us, "failed to enable regulator\n"); >> + } else { >> + regulator_disable(lcd_regulator); >> + } >> + >> + regulator_put(lcd_regulator); > > I wonder why you ->get() and ->put() everytime here. Wouldn't it be > enough to ->get() on probe() and ->put() on remove() ?? Thanks for the suggestion. It would be simpler if done in probe. I will redo this patch. Regards, Thomas. > > -- > balbi >
On 12/5/11, Thomas Abraham <thomas.abraham@linaro.org> wrote: > The power source to the lcd panel or the lcd interface such as lvds > transmitters could be controlled by a regulator supply. Add support > for enabling/disabling the regulator when switching power to lcd. > > Two new elements 'min_uV' and 'max_uV' in the platform data are added > to allow platform code to specifiy the desired output voltage from the > regulator. > > Cc: Ben Dooks <ben-linux@fluff.org> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/video/backlight/platform_lcd.c | 29 +++++++++++++++++++++++++++++ > include/video/platform_lcd.h | 7 +++++++ > 2 files changed, 36 insertions(+), 0 deletions(-) > > diff --git a/drivers/video/backlight/platform_lcd.c > b/drivers/video/backlight/platform_lcd.c > index 302330a..ffa8108 100644 > --- a/drivers/video/backlight/platform_lcd.c > +++ b/drivers/video/backlight/platform_lcd.c > @@ -17,6 +17,8 @@ > #include <linux/backlight.h> > #include <linux/lcd.h> > #include <linux/slab.h> > +#include <linux/regulator/consumer.h> > +#include <linux/regulator/machine.h> > > #include <video/platform_lcd.h> > > @@ -44,11 +46,38 @@ static int platform_lcd_get_power(struct lcd_device > *lcd) > static int platform_lcd_set_power(struct lcd_device *lcd, int power) > { > struct platform_lcd *plcd = to_our_lcd(lcd); > + struct regulator *lcd_regulator; > int lcd_power = 1; > > if (power == FB_BLANK_POWERDOWN || plcd->suspended) > lcd_power = 0; > > + /* > + * If power to lcd and/or lcd interface is controlled using a regulator, > + * enable or disable the regulator based in the power setting. > + */ > + lcd_regulator = regulator_get(plcd->us, "vcc_lcd"); > + if (IS_ERR(lcd_regulator)) { > + dev_info(plcd->us, "could not get regulator\n"); > + goto set_power; > + } Recent regulator discussion. it should be failed instead fall through gracefully. > + > + if (lcd_power) { > + if (plcd->pdata->min_uV || plcd->pdata->max_uV) > + if (regulator_set_voltage(lcd_regulator, > + plcd->pdata->min_uV, plcd->pdata->max_uV)) > + dev_info(plcd->us, > + "regulator voltage set failed\n"); Are there case to change the voltage? Doesn't it easy to set the voltage at board file? I mean don't need to setup at drivers? Thank you, Kyungmin Park > + > + if (regulator_enable(lcd_regulator)) > + dev_info(plcd->us, "failed to enable regulator\n"); > + } else { > + regulator_disable(lcd_regulator); > + } > + > + regulator_put(lcd_regulator); > + > +set_power: > plcd->pdata->set_power(plcd->pdata, lcd_power); > plcd->power = power; > > diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h > index ad3bdfe..acd5d21 100644 > --- a/include/video/platform_lcd.h > +++ b/include/video/platform_lcd.h > @@ -14,8 +14,15 @@ > struct plat_lcd_data; > struct fb_info; > > +/** > + * struct plat_lcd_data - platform data for platform_lcd driver. > + * @min_uV: Minimum required voltage output from the regulator. > + * @max_uV: Maximum acceptable voltage output from the regulator. > + */ > struct plat_lcd_data { > void (*set_power)(struct plat_lcd_data *, unsigned int power); > int (*match_fb)(struct plat_lcd_data *, struct fb_info *); > + int min_uV; > + int max_uV; > }; > > -- > 1.6.6.rc2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Dec 05, 2011 at 07:10:03PM +0900, Kyungmin Park wrote: > > + if (plcd->pdata->min_uV || plcd->pdata->max_uV) > > + if (regulator_set_voltage(lcd_regulator, > > + plcd->pdata->min_uV, plcd->pdata->max_uV)) > > + dev_info(plcd->us, > > + "regulator voltage set failed\n"); > Are there case to change the voltage? Doesn't it easy to set the > voltage at board file? I mean don't need to setup at drivers? Yes, unless the driver changes the voltage it should just assume the voltage is already set up sensibly and leave it up to the board to do any configuration which is required on boot. There is one existing driver which implements things this way in the video subsystem but that should be fixed.
Dear Mr. Park, On 5 December 2011 15:40, Kyungmin Park <kmpark@infradead.org> wrote: [...] >> + /* >> + * If power to lcd and/or lcd interface is controlled using a regulator, >> + * enable or disable the regulator based in the power setting. >> + */ >> + lcd_regulator = regulator_get(plcd->us, "vcc_lcd"); >> + if (IS_ERR(lcd_regulator)) { >> + dev_info(plcd->us, "could not get regulator\n"); >> + goto set_power; >> + } > > Recent regulator discussion. it should be failed instead fall through > gracefully. Ok. But in this case, there could be boards that do not use a regulator for the lcd/display interface. Is it mandatory for fail if regulator is not found. Sorry, I have not read through the regulator discussion. >> + >> + if (lcd_power) { >> + if (plcd->pdata->min_uV || plcd->pdata->max_uV) >> + if (regulator_set_voltage(lcd_regulator, >> + plcd->pdata->min_uV, plcd->pdata->max_uV)) >> + dev_info(plcd->us, >> + "regulator voltage set failed\n"); > Are there case to change the voltage? Doesn't it easy to set the > voltage at board file? I mean don't need to setup at drivers? The constraint 'apply_uV' cannot be expressed with a device tree. So there has be a mechanism to set the required output voltage for a regulator. In case of platform-lcd driver, the power source should be turned off to the lcd/interfaces until the display controller driver is enabled. Powering on the lcd/interfaces at boot time by default would not be ideal. Thanks for your review and comments. Regards, Thomas. > > Thank you, > Kyungmin Park [...]
On Mon, Dec 05, 2011 at 04:06:17PM +0530, Thomas Abraham wrote: > On 5 December 2011 15:40, Kyungmin Park <kmpark@infradead.org> wrote: > > Recent regulator discussion. it should be failed instead fall through > > gracefully. > Ok. But in this case, there could be boards that do not use a > regulator for the lcd/display interface. Is it mandatory for fail if > regulator is not found. Sorry, I have not read through the regulator > discussion. The boards should supply a fixed voltage regulator representing the supply (or use CONFIG_REGULATOR_DUMMY to substitute in a dummy regulator when not needed). Sascha Hauer had some patches to add a flag that could be set to enable this mode on init rather than through Kconfig but they needed a bit of cleanup, not sure what the status is there. > The constraint 'apply_uV' cannot be expressed with a device tree. So > there has be a mechanism to set the required output voltage for a > regulator. It's not an option to not have it with device tree - if there's a single voltage specified apply_uV is assumed.
diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c index 302330a..ffa8108 100644 --- a/drivers/video/backlight/platform_lcd.c +++ b/drivers/video/backlight/platform_lcd.c @@ -17,6 +17,8 @@ #include <linux/backlight.h> #include <linux/lcd.h> #include <linux/slab.h> +#include <linux/regulator/consumer.h> +#include <linux/regulator/machine.h> #include <video/platform_lcd.h> @@ -44,11 +46,38 @@ static int platform_lcd_get_power(struct lcd_device *lcd) static int platform_lcd_set_power(struct lcd_device *lcd, int power) { struct platform_lcd *plcd = to_our_lcd(lcd); + struct regulator *lcd_regulator; int lcd_power = 1; if (power == FB_BLANK_POWERDOWN || plcd->suspended) lcd_power = 0; + /* + * If power to lcd and/or lcd interface is controlled using a regulator, + * enable or disable the regulator based in the power setting. + */ + lcd_regulator = regulator_get(plcd->us, "vcc_lcd"); + if (IS_ERR(lcd_regulator)) { + dev_info(plcd->us, "could not get regulator\n"); + goto set_power; + } + + if (lcd_power) { + if (plcd->pdata->min_uV || plcd->pdata->max_uV) + if (regulator_set_voltage(lcd_regulator, + plcd->pdata->min_uV, plcd->pdata->max_uV)) + dev_info(plcd->us, + "regulator voltage set failed\n"); + + if (regulator_enable(lcd_regulator)) + dev_info(plcd->us, "failed to enable regulator\n"); + } else { + regulator_disable(lcd_regulator); + } + + regulator_put(lcd_regulator); + +set_power: plcd->pdata->set_power(plcd->pdata, lcd_power); plcd->power = power; diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h index ad3bdfe..acd5d21 100644 --- a/include/video/platform_lcd.h +++ b/include/video/platform_lcd.h @@ -14,8 +14,15 @@ struct plat_lcd_data; struct fb_info; +/** + * struct plat_lcd_data - platform data for platform_lcd driver. + * @min_uV: Minimum required voltage output from the regulator. + * @max_uV: Maximum acceptable voltage output from the regulator. + */ struct plat_lcd_data { void (*set_power)(struct plat_lcd_data *, unsigned int power); int (*match_fb)(struct plat_lcd_data *, struct fb_info *); + int min_uV; + int max_uV; };
The power source to the lcd panel or the lcd interface such as lvds transmitters could be controlled by a regulator supply. Add support for enabling/disabling the regulator when switching power to lcd. Two new elements 'min_uV' and 'max_uV' in the platform data are added to allow platform code to specifiy the desired output voltage from the regulator. Cc: Ben Dooks <ben-linux@fluff.org> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- drivers/video/backlight/platform_lcd.c | 29 +++++++++++++++++++++++++++++ include/video/platform_lcd.h | 7 +++++++ 2 files changed, 36 insertions(+), 0 deletions(-)