| Message ID | 1417570752-23633-2-git-send-email-seanpaul@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 12/3/2014 10:39 AM, Sean Paul wrote: > This patch adds a supply regulator to the lp855x platform data to facilitate > powering on/off the 3V rail attached to the controller. > > Cc: Stéphane Marchesin<marcheu@chromium.org> > Cc: Aaron Durbin<adurbin@chromium.org> > Acked-by: Milo Kim<milo.kim@ti.com> > Signed-off-by: Sean Paul<seanpaul@chromium.org> Acked-by: Milo Kim <milo.kim@ti.com> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, December 03, 2014 10:39 AM, Sean Paul wrote: > > This patch adds a supply regulator to the lp855x platform data to facilitate > powering on/off the 3V rail attached to the controller. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Cc: Aaron Durbin <adurbin@chromium.org> > Acked-by: Milo Kim <milo.kim@ti.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> Acked-by: Jingoo Han <jg1.han@samsung.com> Best regards, Jingoo Han > --- > > Changes in v2: > - Add NULL check in lp855x_remove > > > .../devicetree/bindings/video/backlight/lp855x.txt | 2 ++ > drivers/video/backlight/lp855x_bl.c | 18 ++++++++++++++++++ > include/linux/platform_data/lp855x.h | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt > b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > index 96e83a5..0a3ecbc 100644 > --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt > +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > @@ -12,6 +12,7 @@ Optional properties: > - pwm-period: PWM period value. Set only PWM input mode used (u32) > - rom-addr: Register address of ROM area to be updated (u8) > - rom-val: Register value to be updated (u8) > + - power-supply: Regulator which controls the 3V rail > > Example: > > @@ -56,6 +57,7 @@ Example: > backlight@2c { > compatible = "ti,lp8557"; > reg = <0x2c>; > + power-supply = <&backlight_vdd>; > > dev-ctrl = /bits/ 8 <0x41>; > init-brt = /bits/ 8 <0x0a>; > diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c > index 257b3ba..a26d3bb 100644 > --- a/drivers/video/backlight/lp855x_bl.c > +++ b/drivers/video/backlight/lp855x_bl.c > @@ -17,6 +17,7 @@ > #include <linux/of.h> > #include <linux/platform_data/lp855x.h> > #include <linux/pwm.h> > +#include <linux/regulator/consumer.h> > > /* LP8550/1/2/3/6 Registers */ > #define LP855X_BRIGHTNESS_CTRL 0x00 > @@ -383,6 +384,13 @@ static int lp855x_parse_dt(struct lp855x *lp) > pdata->rom_data = &rom[0]; > } > > + pdata->supply = devm_regulator_get(dev, "power"); > + if (IS_ERR(pdata->supply)) { > + if (PTR_ERR(pdata->supply) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + pdata->supply = NULL; > + } > + > lp->pdata = pdata; > > return 0; > @@ -423,6 +431,14 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) > else > lp->mode = REGISTER_BASED; > > + if (lp->pdata->supply) { > + ret = regulator_enable(lp->pdata->supply); > + if (ret < 0) { > + dev_err(&cl->dev, "failed to enable supply: %d\n", ret); > + return ret; > + } > + } > + > i2c_set_clientdata(cl, lp); > > ret = lp855x_configure(lp); > @@ -454,6 +470,8 @@ static int lp855x_remove(struct i2c_client *cl) > > lp->bl->props.brightness = 0; > backlight_update_status(lp->bl); > + if (lp->pdata->supply) > + regulator_disable(lp->pdata->supply); > sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group); > > return 0; > diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h > index 1b2ba24..9c7fd1e 100644 > --- a/include/linux/platform_data/lp855x.h > +++ b/include/linux/platform_data/lp855x.h > @@ -136,6 +136,7 @@ struct lp855x_rom_data { > Only valid when mode is PWM_BASED. > * @size_program : total size of lp855x_rom_data > * @rom_data : list of new eeprom/eprom registers > + * @supply : regulator that supplies 3V input > */ > struct lp855x_platform_data { > const char *name; > @@ -144,6 +145,7 @@ struct lp855x_platform_data { > unsigned int period_ns; > int size_program; > struct lp855x_rom_data *rom_data; > + struct regulator *supply; > }; > > #endif > -- > 2.2.0.rc0.207.ga3a616c -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 2, 2014 at 5:39 PM, Sean Paul <seanpaul@chromium.org> wrote: > This patch adds a supply regulator to the lp855x platform data to facilitate > powering on/off the 3V rail attached to the controller. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Cc: Aaron Durbin <adurbin@chromium.org> > Acked-by: Milo Kim <milo.kim@ti.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> Acked-by: Bryan Wu <cooloney@gmail.com> Thanks, -Bryan > --- > > Changes in v2: > - Add NULL check in lp855x_remove > > > .../devicetree/bindings/video/backlight/lp855x.txt | 2 ++ > drivers/video/backlight/lp855x_bl.c | 18 ++++++++++++++++++ > include/linux/platform_data/lp855x.h | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > index 96e83a5..0a3ecbc 100644 > --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt > +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > @@ -12,6 +12,7 @@ Optional properties: > - pwm-period: PWM period value. Set only PWM input mode used (u32) > - rom-addr: Register address of ROM area to be updated (u8) > - rom-val: Register value to be updated (u8) > + - power-supply: Regulator which controls the 3V rail > > Example: > > @@ -56,6 +57,7 @@ Example: > backlight@2c { > compatible = "ti,lp8557"; > reg = <0x2c>; > + power-supply = <&backlight_vdd>; > > dev-ctrl = /bits/ 8 <0x41>; > init-brt = /bits/ 8 <0x0a>; > diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c > index 257b3ba..a26d3bb 100644 > --- a/drivers/video/backlight/lp855x_bl.c > +++ b/drivers/video/backlight/lp855x_bl.c > @@ -17,6 +17,7 @@ > #include <linux/of.h> > #include <linux/platform_data/lp855x.h> > #include <linux/pwm.h> > +#include <linux/regulator/consumer.h> > > /* LP8550/1/2/3/6 Registers */ > #define LP855X_BRIGHTNESS_CTRL 0x00 > @@ -383,6 +384,13 @@ static int lp855x_parse_dt(struct lp855x *lp) > pdata->rom_data = &rom[0]; > } > > + pdata->supply = devm_regulator_get(dev, "power"); > + if (IS_ERR(pdata->supply)) { > + if (PTR_ERR(pdata->supply) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + pdata->supply = NULL; > + } > + > lp->pdata = pdata; > > return 0; > @@ -423,6 +431,14 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) > else > lp->mode = REGISTER_BASED; > > + if (lp->pdata->supply) { > + ret = regulator_enable(lp->pdata->supply); > + if (ret < 0) { > + dev_err(&cl->dev, "failed to enable supply: %d\n", ret); > + return ret; > + } > + } > + > i2c_set_clientdata(cl, lp); > > ret = lp855x_configure(lp); > @@ -454,6 +470,8 @@ static int lp855x_remove(struct i2c_client *cl) > > lp->bl->props.brightness = 0; > backlight_update_status(lp->bl); > + if (lp->pdata->supply) > + regulator_disable(lp->pdata->supply); > sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group); > > return 0; > diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h > index 1b2ba24..9c7fd1e 100644 > --- a/include/linux/platform_data/lp855x.h > +++ b/include/linux/platform_data/lp855x.h > @@ -136,6 +136,7 @@ struct lp855x_rom_data { > Only valid when mode is PWM_BASED. > * @size_program : total size of lp855x_rom_data > * @rom_data : list of new eeprom/eprom registers > + * @supply : regulator that supplies 3V input > */ > struct lp855x_platform_data { > const char *name; > @@ -144,6 +145,7 @@ struct lp855x_platform_data { > unsigned int period_ns; > int size_program; > struct lp855x_rom_data *rom_data; > + struct regulator *supply; > }; > > #endif > -- > 2.2.0.rc0.207.ga3a616c > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 02 Dec 2014, Sean Paul wrote: > This patch adds a supply regulator to the lp855x platform data to facilitate > powering on/off the 3V rail attached to the controller. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Cc: Aaron Durbin <adurbin@chromium.org> > Acked-by: Milo Kim <milo.kim@ti.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > > Changes in v2: > - Add NULL check in lp855x_remove > > > .../devicetree/bindings/video/backlight/lp855x.txt | 2 ++ > drivers/video/backlight/lp855x_bl.c | 18 ++++++++++++++++++ > include/linux/platform_data/lp855x.h | 2 ++ > 3 files changed, 22 insertions(+) Applied with Acks, thanks. > diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > index 96e83a5..0a3ecbc 100644 > --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt > +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt > @@ -12,6 +12,7 @@ Optional properties: > - pwm-period: PWM period value. Set only PWM input mode used (u32) > - rom-addr: Register address of ROM area to be updated (u8) > - rom-val: Register value to be updated (u8) > + - power-supply: Regulator which controls the 3V rail > > Example: > > @@ -56,6 +57,7 @@ Example: > backlight@2c { > compatible = "ti,lp8557"; > reg = <0x2c>; > + power-supply = <&backlight_vdd>; > > dev-ctrl = /bits/ 8 <0x41>; > init-brt = /bits/ 8 <0x0a>; > diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c > index 257b3ba..a26d3bb 100644 > --- a/drivers/video/backlight/lp855x_bl.c > +++ b/drivers/video/backlight/lp855x_bl.c > @@ -17,6 +17,7 @@ > #include <linux/of.h> > #include <linux/platform_data/lp855x.h> > #include <linux/pwm.h> > +#include <linux/regulator/consumer.h> > > /* LP8550/1/2/3/6 Registers */ > #define LP855X_BRIGHTNESS_CTRL 0x00 > @@ -383,6 +384,13 @@ static int lp855x_parse_dt(struct lp855x *lp) > pdata->rom_data = &rom[0]; > } > > + pdata->supply = devm_regulator_get(dev, "power"); > + if (IS_ERR(pdata->supply)) { > + if (PTR_ERR(pdata->supply) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + pdata->supply = NULL; > + } > + > lp->pdata = pdata; > > return 0; > @@ -423,6 +431,14 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) > else > lp->mode = REGISTER_BASED; > > + if (lp->pdata->supply) { > + ret = regulator_enable(lp->pdata->supply); > + if (ret < 0) { > + dev_err(&cl->dev, "failed to enable supply: %d\n", ret); > + return ret; > + } > + } > + > i2c_set_clientdata(cl, lp); > > ret = lp855x_configure(lp); > @@ -454,6 +470,8 @@ static int lp855x_remove(struct i2c_client *cl) > > lp->bl->props.brightness = 0; > backlight_update_status(lp->bl); > + if (lp->pdata->supply) > + regulator_disable(lp->pdata->supply); > sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group); > > return 0; > diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h > index 1b2ba24..9c7fd1e 100644 > --- a/include/linux/platform_data/lp855x.h > +++ b/include/linux/platform_data/lp855x.h > @@ -136,6 +136,7 @@ struct lp855x_rom_data { > Only valid when mode is PWM_BASED. > * @size_program : total size of lp855x_rom_data > * @rom_data : list of new eeprom/eprom registers > + * @supply : regulator that supplies 3V input > */ > struct lp855x_platform_data { > const char *name; > @@ -144,6 +145,7 @@ struct lp855x_platform_data { > unsigned int period_ns; > int size_program; > struct lp855x_rom_data *rom_data; > + struct regulator *supply; > }; > > #endif
On Tuesday 02 December 2014 17:39:12 Sean Paul wrote: > > .../devicetree/bindings/video/backlight/lp855x.txt | 2 ++ > drivers/video/backlight/lp855x_bl.c | 18 ++++++++++++++++++ > include/linux/platform_data/lp855x.h | 2 ++ > 3 files changed, 22 insertions(+) While your two patches are both correct (and applied already), I took a look at the platform_data header and noticed that all users of this file have been converted to DT a while ago, so it would be nice to clean it up by integrating the linux/platform_data/lp855x.h header file into the drivers/video/backlight/lp855x_bl.c itself. You can then merge struct lp855x_platform_data into lp855x to avoid the extra dynamic allocation and remove all the 'pdata' references in the driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 3, 2014 at 6:11 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 02 December 2014 17:39:12 Sean Paul wrote: >> >> .../devicetree/bindings/video/backlight/lp855x.txt | 2 ++ >> drivers/video/backlight/lp855x_bl.c | 18 ++++++++++++++++++ >> include/linux/platform_data/lp855x.h | 2 ++ >> 3 files changed, 22 insertions(+) > > While your two patches are both correct (and applied already), I took > a look at the platform_data header and noticed that all users of this > file have been converted to DT a while ago, so it would be nice to clean > it up by integrating the linux/platform_data/lp855x.h header file into the > drivers/video/backlight/lp855x_bl.c itself. > > You can then merge struct lp855x_platform_data into lp855x to avoid the > extra dynamic allocation and remove all the 'pdata' references in the > driver. > Hi Arnd, Thanks for the suggestion. Indeed, that will be much cleaner, I'll send a follow-up patch. Sean > Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Good to talk to you again. It's been a long time since I met you in Linaro Connect HK few years ago :) On 12/3/2014 11:11 PM, Arnd Bergmann wrote: > While your two patches are both correct (and applied already), I took > a look at the platform_data header and noticed that all users of this > file have been converted to DT a while ago, so it would be nice to clean > it up by integrating the linux/platform_data/lp855x.h header file into the > drivers/video/backlight/lp855x_bl.c itself. I like this cleanup but need to consider few things. - Platform which does not support the DT In this case, there is no way to configure options like backlight device control mode. Without the DT, only default options are set. To change the settings, the driver modification is required. I don't want to write project/platform dependency code inside the driver. - Backward compatibility Some customers have been using this platform data in their board-*.c (but this *.c has not been mainlined yet). My concern is the backward compatibility issue may happen in their projects. Best regards, Milo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, December 09, 2014 12:04 PM, Kim, Milo wrote: > > Hi Arnd, > > Good to talk to you again. It's been a long time since I met you in > Linaro Connect HK few years ago :) > > On 12/3/2014 11:11 PM, Arnd Bergmann wrote: > > While your two patches are both correct (and applied already), I took > > a look at the platform_data header and noticed that all users of this > > file have been converted to DT a while ago, so it would be nice to clean > > it up by integrating the linux/platform_data/lp855x.h header file into the > > drivers/video/backlight/lp855x_bl.c itself. > > I like this cleanup but need to consider few things. > > - Platform which does not support the DT > In this case, there is no way to configure options like backlight device > control mode. Without the DT, only default options are set. > To change the settings, the driver modification is required. > I don't want to write project/platform dependency code inside the driver. > > - Backward compatibility > Some customers have been using this platform data in their board-*.c > (but this *.c has not been mainlined yet). > My concern is the backward compatibility issue may happen in their projects. I understand this situation. In the case of Exynos DisplayPort driver, a similar issue happened. At that time, the conclusion was that out-of-tree code (not mainlined yet) cannot be considered. In this context, it is valuable to upstream the code to mainline kernel. I think that one of the followings can be considered. 1. Ask customers to upstream their machine code 2. Ask customers to use DT If they don't want to these things, someone (you or customers) will need to make the patch for non-DT support, whenever new kernel is released and is used by projects not supporting DT. Best regards, Jingoo Han > > Best regards, > Milo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 09 December 2014 12:46:15 Jingoo Han wrote: > On Tuesday, December 09, 2014 12:04 PM, Kim, Milo wrote: > > > > Hi Arnd, > > > > Good to talk to you again. It's been a long time since I met you in > > Linaro Connect HK few years ago > > > > On 12/3/2014 11:11 PM, Arnd Bergmann wrote: > > > While your two patches are both correct (and applied already), I took > > > a look at the platform_data header and noticed that all users of this > > > file have been converted to DT a while ago, so it would be nice to clean > > > it up by integrating the linux/platform_data/lp855x.h header file into the > > > drivers/video/backlight/lp855x_bl.c itself. > > > > I like this cleanup but need to consider few things. > > > > - Platform which does not support the DT > > In this case, there is no way to configure options like backlight device > > control mode. Without the DT, only default options are set. > > To change the settings, the driver modification is required. > > I don't want to write project/platform dependency code inside the driver. > > > > - Backward compatibility > > Some customers have been using this platform data in their board-*.c > > (but this *.c has not been mainlined yet). > > My concern is the backward compatibility issue may happen in their projects. > > I understand this situation. In the case of Exynos DisplayPort driver, > a similar issue happened. At that time, the conclusion was that > out-of-tree code (not mainlined yet) cannot be considered. Right. Basically when the only reason to have code in the kernel is to support someone with an out-of-tree patch set, they can be expected to add the extra bit to their patch set, it doesn't add any extra burden to them, and it makes our lives easier. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt index 96e83a5..0a3ecbc 100644 --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt @@ -12,6 +12,7 @@ Optional properties: - pwm-period: PWM period value. Set only PWM input mode used (u32) - rom-addr: Register address of ROM area to be updated (u8) - rom-val: Register value to be updated (u8) + - power-supply: Regulator which controls the 3V rail Example: @@ -56,6 +57,7 @@ Example: backlight@2c { compatible = "ti,lp8557"; reg = <0x2c>; + power-supply = <&backlight_vdd>; dev-ctrl = /bits/ 8 <0x41>; init-brt = /bits/ 8 <0x0a>; diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index 257b3ba..a26d3bb 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c @@ -17,6 +17,7 @@ #include <linux/of.h> #include <linux/platform_data/lp855x.h> #include <linux/pwm.h> +#include <linux/regulator/consumer.h> /* LP8550/1/2/3/6 Registers */ #define LP855X_BRIGHTNESS_CTRL 0x00 @@ -383,6 +384,13 @@ static int lp855x_parse_dt(struct lp855x *lp) pdata->rom_data = &rom[0]; } + pdata->supply = devm_regulator_get(dev, "power"); + if (IS_ERR(pdata->supply)) { + if (PTR_ERR(pdata->supply) == -EPROBE_DEFER) + return -EPROBE_DEFER; + pdata->supply = NULL; + } + lp->pdata = pdata; return 0; @@ -423,6 +431,14 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) else lp->mode = REGISTER_BASED; + if (lp->pdata->supply) { + ret = regulator_enable(lp->pdata->supply); + if (ret < 0) { + dev_err(&cl->dev, "failed to enable supply: %d\n", ret); + return ret; + } + } + i2c_set_clientdata(cl, lp); ret = lp855x_configure(lp); @@ -454,6 +470,8 @@ static int lp855x_remove(struct i2c_client *cl) lp->bl->props.brightness = 0; backlight_update_status(lp->bl); + if (lp->pdata->supply) + regulator_disable(lp->pdata->supply); sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group); return 0; diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h index 1b2ba24..9c7fd1e 100644 --- a/include/linux/platform_data/lp855x.h +++ b/include/linux/platform_data/lp855x.h @@ -136,6 +136,7 @@ struct lp855x_rom_data { Only valid when mode is PWM_BASED. * @size_program : total size of lp855x_rom_data * @rom_data : list of new eeprom/eprom registers + * @supply : regulator that supplies 3V input */ struct lp855x_platform_data { const char *name; @@ -144,6 +145,7 @@ struct lp855x_platform_data { unsigned int period_ns; int size_program; struct lp855x_rom_data *rom_data; + struct regulator *supply; }; #endif