| Message ID | 1379972467-11243-6-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/23/2013 03:41 PM, Thierry Reding wrote: > The GPIO API defines 0 as being a valid GPIO number, so this field needs > to be initialized explicitly. > static void __init smdkv210_map_io(void) > @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = { > .max_brightness = 255, > .dft_brightness = 255, > .pwm_period_ns = 78770, > + .enable_gpio = -1, > .init = samsung_bl_init, > .exit = samsung_bl_exit, > }, > @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, > samsung_bl_data->lth_brightness = bl_data->lth_brightness; > if (bl_data->pwm_period_ns) > samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns; > + if (bl_data->enable_gpio) > + samsung_bl_data->enable_gpio = bl_data->enable_gpio; > + if (bl_data->enable_gpio_flags) > + samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags; Won't this cause the core pwm_bl driver to request/manipulate the GPIO, whereas this driver already does that inside the samsung_bl_init/exit callbacks? I think you either need to adjust those callbacks, or not set the new standard GPIO property in samsung_bl_data.
On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote: > On 09/23/2013 03:41 PM, Thierry Reding wrote: > > The GPIO API defines 0 as being a valid GPIO number, so this field needs > > to be initialized explicitly. > > > static void __init smdkv210_map_io(void) > > > @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = { > > .max_brightness = 255, > > .dft_brightness = 255, > > .pwm_period_ns = 78770, > > + .enable_gpio = -1, > > .init = samsung_bl_init, > > .exit = samsung_bl_exit, > > }, > > @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, > > samsung_bl_data->lth_brightness = bl_data->lth_brightness; > > if (bl_data->pwm_period_ns) > > samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns; > > + if (bl_data->enable_gpio) > > + samsung_bl_data->enable_gpio = bl_data->enable_gpio; > > + if (bl_data->enable_gpio_flags) > > + samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags; > > Won't this cause the core pwm_bl driver to request/manipulate the GPIO, > whereas this driver already does that inside the samsung_bl_init/exit > callbacks? I think you either need to adjust those callbacks, or not set > the new standard GPIO property in samsung_bl_data. I don't think so. The samsung_bl_data is a copy of samsung_dfl_bl_data augmented by board-specific settings. So in fact copying these values here is essential to allow boards to override the enable_gpio and flags fields. Currently no board sets the enable_gpio to a valid GPIO so it's all still handled by the callbacks only. Thierry
On 10/01/2013 02:43 PM, Thierry Reding wrote: > On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote: >> On 09/23/2013 03:41 PM, Thierry Reding wrote: >>> The GPIO API defines 0 as being a valid GPIO number, so this >>> field needs to be initialized explicitly. >> >>> static void __init smdkv210_map_io(void) >> >>> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata >>> samsung_dfl_bl_data __initdata = { .max_brightness = 255, >>> .dft_brightness = 255, .pwm_period_ns = 78770, + .enable_gpio >>> = -1, .init = samsung_bl_init, .exit = >>> samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init >>> samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, >>> samsung_bl_data->lth_brightness = bl_data->lth_brightness; if >>> (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns = >>> bl_data->pwm_period_ns; + if (bl_data->enable_gpio) + >>> samsung_bl_data->enable_gpio = bl_data->enable_gpio; + if >>> (bl_data->enable_gpio_flags) + >>> samsung_bl_data->enable_gpio_flags = >>> bl_data->enable_gpio_flags; >> >> Won't this cause the core pwm_bl driver to request/manipulate the >> GPIO, whereas this driver already does that inside the >> samsung_bl_init/exit callbacks? I think you either need to adjust >> those callbacks, or not set the new standard GPIO property in >> samsung_bl_data. > > I don't think so. The samsung_bl_data is a copy of > samsung_dfl_bl_data augmented by board-specific settings. So in > fact copying these values here is essential to allow boards to > override the enable_gpio and flags fields. Currently no board sets > the enable_gpio to a valid GPIO so it's all still handled by the > callbacks only. Oh yes, you're right. I was confusing the new enable_gpio field in pwm_bl's platform data with some other field in a custom data structure. One minor point though: >>> + if (bl_data->enable_gpio) + samsung_bl_data->enable_gpio = >>> bl_data->enable_gpio; That assumes that enable_gpio==0 means "none", whereas you've gone to great pains in the rest of the series to allow 0 to be a valid GPIO ID. right now, the default value of samsung_bl_data->enable_gpio is -1, and if !bl_data->enable_gpio, that value won't be propagated across.
On Tue, Oct 01, 2013 at 02:58:22PM -0600, Stephen Warren wrote: > On 10/01/2013 02:43 PM, Thierry Reding wrote: > > On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote: > >> On 09/23/2013 03:41 PM, Thierry Reding wrote: > >>> The GPIO API defines 0 as being a valid GPIO number, so this > >>> field needs to be initialized explicitly. > >> > >>> static void __init smdkv210_map_io(void) > >> > >>> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata > >>> samsung_dfl_bl_data __initdata = { .max_brightness = 255, > >>> .dft_brightness = 255, .pwm_period_ns = 78770, + .enable_gpio > >>> = -1, .init = samsung_bl_init, .exit = > >>> samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init > >>> samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, > >>> samsung_bl_data->lth_brightness = bl_data->lth_brightness; if > >>> (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns = > >>> bl_data->pwm_period_ns; + if (bl_data->enable_gpio) + > >>> samsung_bl_data->enable_gpio = bl_data->enable_gpio; + if > >>> (bl_data->enable_gpio_flags) + > >>> samsung_bl_data->enable_gpio_flags = > >>> bl_data->enable_gpio_flags; > >> > >> Won't this cause the core pwm_bl driver to request/manipulate the > >> GPIO, whereas this driver already does that inside the > >> samsung_bl_init/exit callbacks? I think you either need to adjust > >> those callbacks, or not set the new standard GPIO property in > >> samsung_bl_data. > > > > I don't think so. The samsung_bl_data is a copy of > > samsung_dfl_bl_data augmented by board-specific settings. So in > > fact copying these values here is essential to allow boards to > > override the enable_gpio and flags fields. Currently no board sets > > the enable_gpio to a valid GPIO so it's all still handled by the > > callbacks only. > > Oh yes, you're right. I was confusing the new enable_gpio field in > pwm_bl's platform data with some other field in a custom data structure. > > One minor point though: > > >>> + if (bl_data->enable_gpio) + samsung_bl_data->enable_gpio = > >>> bl_data->enable_gpio; > > That assumes that enable_gpio==0 means "none", whereas you've gone to > great pains in the rest of the series to allow 0 to be a valid GPIO > ID. right now, the default value of samsung_bl_data->enable_gpio is > -1, and if !bl_data->enable_gpio, that value won't be propagated across. Right, that check should now be: if (bl_data->enable_gpio >= 0) Well, it depends. It would be possible for the default to specify a valid GPIO and for a board to override it with -1 (and provide a set of corresponding callbacks). In that case the right thing to do here would be not to check at all. Then again, I don't think that will ever happen, because no fixed GPIO will ever be a good default. So changing to >= 0 instead of != 0 should work fine. Again, starting with 3.13 this should become a lot easier to handle since the GPIO subsystem will gain functionality to use a per-board lookup table, similarly to what the regulator and PWM subsystems do. Once that's in place I plan to make another pass over all users of the pwm-backlight driver and replace the enable_gpio field with a GPIO lookup table, so that the driver can uniformly request them using a simple gpiod_get(). Thierry
diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c index 74dd479..952b6a0 100644 --- a/arch/arm/mach-s3c24xx/mach-h1940.c +++ b/arch/arm/mach-s3c24xx/mach-h1940.c @@ -504,6 +504,7 @@ static struct platform_pwm_backlight_data backlight_data = { .dft_brightness = 50, /* tcnt = 0x31 */ .pwm_period_ns = 36296, + .enable_gpio = -1, .init = h1940_backlight_init, .notify = h1940_backlight_notify, .exit = h1940_backlight_exit, diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index 206b1f7..034b7fe 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -522,6 +522,7 @@ static struct platform_pwm_backlight_data rx1950_backlight_data = { .max_brightness = 24, .dft_brightness = 4, .pwm_period_ns = 48000, + .enable_gpio = -1, .init = rx1950_backlight_init, .notify = rx1950_backlight_notify, .exit = rx1950_backlight_exit, diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c b/arch/arm/mach-s3c64xx/mach-crag6410.c index 1a911df..b319bf9 100644 --- a/arch/arm/mach-s3c64xx/mach-crag6410.c +++ b/arch/arm/mach-s3c64xx/mach-crag6410.c @@ -114,6 +114,7 @@ static struct platform_pwm_backlight_data crag6410_backlight_data = { .max_brightness = 1000, .dft_brightness = 600, .pwm_period_ns = 100000, /* about 1kHz */ + .enable_gpio = -1, }; static struct platform_device crag6410_backlight_device = { diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c index e806404..614a03a 100644 --- a/arch/arm/mach-s3c64xx/mach-hmt.c +++ b/arch/arm/mach-s3c64xx/mach-hmt.c @@ -114,6 +114,7 @@ static struct platform_pwm_backlight_data hmt_backlight_data = { .max_brightness = 100 * 256, .dft_brightness = 40 * 256, .pwm_period_ns = 1000000000 / (100 * 256 * 20), + .enable_gpio = -1, .init = hmt_bl_init, .notify = hmt_bl_notify, .exit = hmt_bl_exit, diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c index 0f47237..a6b338f 100644 --- a/arch/arm/mach-s3c64xx/mach-smartq.c +++ b/arch/arm/mach-s3c64xx/mach-smartq.c @@ -151,6 +151,7 @@ static struct platform_pwm_backlight_data smartq_backlight_data = { .max_brightness = 1000, .dft_brightness = 600, .pwm_period_ns = 1000000000 / (1000 * 20), + .enable_gpio = -1, .init = smartq_bl_init, }; diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c b/arch/arm/mach-s3c64xx/mach-smdk6410.c index 2a7b32c..d5ea938 100644 --- a/arch/arm/mach-s3c64xx/mach-smdk6410.c +++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c @@ -625,6 +625,7 @@ static struct samsung_bl_gpio_info smdk6410_bl_gpio_info = { static struct platform_pwm_backlight_data smdk6410_bl_data = { .pwm_id = 1, + .enable_gpio = -1, }; static struct s3c_hsotg_plat smdk6410_hsotg_pdata; diff --git a/arch/arm/mach-s5p64x0/mach-smdk6440.c b/arch/arm/mach-s5p64x0/mach-smdk6440.c index 0b00304..9efdcc0 100644 --- a/arch/arm/mach-s5p64x0/mach-smdk6440.c +++ b/arch/arm/mach-s5p64x0/mach-smdk6440.c @@ -223,6 +223,7 @@ static struct samsung_bl_gpio_info smdk6440_bl_gpio_info = { static struct platform_pwm_backlight_data smdk6440_bl_data = { .pwm_id = 1, + .enable_gpio = -1, }; static void __init smdk6440_map_io(void) diff --git a/arch/arm/mach-s5p64x0/mach-smdk6450.c b/arch/arm/mach-s5p64x0/mach-smdk6450.c index 5949296..c3cacc0 100644 --- a/arch/arm/mach-s5p64x0/mach-smdk6450.c +++ b/arch/arm/mach-s5p64x0/mach-smdk6450.c @@ -242,6 +242,7 @@ static struct samsung_bl_gpio_info smdk6450_bl_gpio_info = { static struct platform_pwm_backlight_data smdk6450_bl_data = { .pwm_id = 1, + .enable_gpio = -1, }; static void __init smdk6450_map_io(void) diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c b/arch/arm/mach-s5pc100/mach-smdkc100.c index 7c57a22..9e256b9 100644 --- a/arch/arm/mach-s5pc100/mach-smdkc100.c +++ b/arch/arm/mach-s5pc100/mach-smdkc100.c @@ -216,6 +216,7 @@ static struct samsung_bl_gpio_info smdkc100_bl_gpio_info = { static struct platform_pwm_backlight_data smdkc100_bl_data = { .pwm_id = 0, + .enable_gpio = -1, }; static void __init smdkc100_map_io(void) diff --git a/arch/arm/mach-s5pv210/mach-smdkv210.c b/arch/arm/mach-s5pv210/mach-smdkv210.c index 6d72bb99..f52cc15 100644 --- a/arch/arm/mach-s5pv210/mach-smdkv210.c +++ b/arch/arm/mach-s5pv210/mach-smdkv210.c @@ -279,6 +279,7 @@ static struct samsung_bl_gpio_info smdkv210_bl_gpio_info = { static struct platform_pwm_backlight_data smdkv210_bl_data = { .pwm_id = 3, .pwm_period_ns = 1000, + .enable_gpio = -1, }; static void __init smdkv210_map_io(void) diff --git a/arch/arm/plat-samsung/dev-backlight.c b/arch/arm/plat-samsung/dev-backlight.c index d51f956..80e7450 100644 --- a/arch/arm/plat-samsung/dev-backlight.c +++ b/arch/arm/plat-samsung/dev-backlight.c @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = { .max_brightness = 255, .dft_brightness = 255, .pwm_period_ns = 78770, + .enable_gpio = -1, .init = samsung_bl_init, .exit = samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, samsung_bl_data->lth_brightness = bl_data->lth_brightness; if (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns; + if (bl_data->enable_gpio) + samsung_bl_data->enable_gpio = bl_data->enable_gpio; + if (bl_data->enable_gpio_flags) + samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags; if (bl_data->init) samsung_bl_data->init = bl_data->init; if (bl_data->notify)
The GPIO API defines 0 as being a valid GPIO number, so this field needs to be initialized explicitly. Signed-off-by: Thierry Reding <treding@nvidia.com> --- arch/arm/mach-s3c24xx/mach-h1940.c | 1 + arch/arm/mach-s3c24xx/mach-rx1950.c | 1 + arch/arm/mach-s3c64xx/mach-crag6410.c | 1 + arch/arm/mach-s3c64xx/mach-hmt.c | 1 + arch/arm/mach-s3c64xx/mach-smartq.c | 1 + arch/arm/mach-s3c64xx/mach-smdk6410.c | 1 + arch/arm/mach-s5p64x0/mach-smdk6440.c | 1 + arch/arm/mach-s5p64x0/mach-smdk6450.c | 1 + arch/arm/mach-s5pc100/mach-smdkc100.c | 1 + arch/arm/mach-s5pv210/mach-smdkv210.c | 1 + arch/arm/plat-samsung/dev-backlight.c | 5 +++++ 11 files changed, 15 insertions(+)