| Message ID | 20181026184157.16371-2-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] pwm: Add new helper to initialize a pwm_state variable with defaults | expand |
On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > The legacy PWM API should be removed in the long run, so convert a user > to the atomic PWM API. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> I see this depends on first patch so I assume this will go through pwm tree. Best regards, Krzysztof
On Mon, Oct 29, 2018 at 11:46:15AM +0100, Krzysztof Kozlowski wrote: > On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > The legacy PWM API should be removed in the long run, so convert a user > > to the atomic PWM API. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> > I see this depends on first patch so I assume this will go through pwm tree. Right, I assume so, too. Unfortunately I didn't hear anything back from Thierry about this because there seems to be a mail reception problem on his end. @Thierry: Could you find the problem? It would be great to get some feedback from you regarding these patches. There are more pwm users that could benefit from this change, but I first want some feedback about this before investing more time here. Best regards Uwe
On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote: > The legacy PWM API should be removed in the long run, so convert a user > to the atomic PWM API. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c > index 7f5a18fa305b..5c4459f9a5f7 100644 > --- a/arch/arm/mach-s3c24xx/mach-rx1950.c > +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c > @@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = { > PWM_POLARITY_NORMAL), > }; > > +static struct pwm_state lcd_pwm_state; You shouldn't need this. The whole point of the atomic API is that the PWM carries its state, so the proper way to make a change is to query the current state, make any required modifications and then apply the new state. > static struct pwm_device *lcd_pwm; > > static void rx1950_lcd_power(int enable) > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable) > > /* GPB1->OUTPUT, GPB1->0 */ > gpio_direction_output(S3C2410_GPB(1), 0); > - pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); > - pwm_disable(lcd_pwm); > + lcd_pwm_state.duty_cycle = 0; > + lcd_pwm_state.enabled = false; > + pwm_apply_state(lcd_pwm, &lcd_pwm_state); The correct way to do this would be: struct pwm_state state; pwm_get_state(lcd_pwm, &state); state.enabled = false; state.duty_cycle = 0; pwm_apply_state(lcd_pwm, &state); > > /* GPC0->0, GPC10->0 */ > gpio_direction_output(S3C2410_GPC(0), 0); > gpio_direction_output(S3C2410_GPC(10), 0); > } else { > - pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD); > - pwm_enable(lcd_pwm); > + lcd_pwm_state.duty_cycle = LCD_PWM_DUTY; > + lcd_pwm_state.enabled = true; > + pwm_apply_state(lcd_pwm, &lcd_pwm_state); Similarily here. > > gpio_direction_output(S3C2410_GPC(0), 1); > gpio_direction_output(S3C2410_GPC(5), 1); > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev) > return PTR_ERR(lcd_pwm); > } > > - /* > - * FIXME: pwm_apply_args() should be removed when switching to > - * the atomic PWM API. > - */ > - pwm_apply_args(lcd_pwm); > + pwm_get_state_default(lcd_pwm, &lcd_pwm_state); > + lcd_pwm_state.period = LCD_PWM_PERIOD; This is wrong, though it's probably because the comment is also confusing. There should be nothing wrong with using pwm_apply_args() in this case. While at it, I think the conversion should also be include replacing the call to pwm_request() by pwm_get(). There's already a PWM lookup table in the RX1950 board code, so pwm_get() would be able to use that. Note that for some reason that table contains a period that is different from LCD_PWM_PERIOD, so I think that should also be addressed. Basically all the information other than duty cycle should be coming from either DT or a lookup table. Thierry
Hello Thierry, On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote: > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote: > > +static struct pwm_state lcd_pwm_state; > > You shouldn't need this. The whole point of the atomic API is that the > PWM carries its state, so the proper way to make a change is to query > the current state, make any required modifications and then apply the > new state. I thought the whole point of the atomic API is to be atomic :-) > > static struct pwm_device *lcd_pwm; > > > > static void rx1950_lcd_power(int enable) > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable) > > > > /* GPB1->OUTPUT, GPB1->0 */ > > gpio_direction_output(S3C2410_GPB(1), 0); > > - pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); > > - pwm_disable(lcd_pwm); > > + lcd_pwm_state.duty_cycle = 0; > > + lcd_pwm_state.enabled = false; > > + pwm_apply_state(lcd_pwm, &lcd_pwm_state); > > The correct way to do this would be: > > struct pwm_state state; > > pwm_get_state(lcd_pwm, &state); > state.enabled = false; > state.duty_cycle = 0; > pwm_apply_state(lcd_pwm, &state); The difference here is that with my approach the rx1950 driver caches the intended pwm_state saving a few cycles for repeatedly copying the pwm cache to a stack variable at the cost of some memory. I like my approach a little better but I'm not willing to argue about that one and can give in. > > gpio_direction_output(S3C2410_GPC(0), 1); > > gpio_direction_output(S3C2410_GPC(5), 1); > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev) > > return PTR_ERR(lcd_pwm); > > } > > > > - /* > > - * FIXME: pwm_apply_args() should be removed when switching to > > - * the atomic PWM API. > > - */ > > - pwm_apply_args(lcd_pwm); > > + pwm_get_state_default(lcd_pwm, &lcd_pwm_state); > > + lcd_pwm_state.period = LCD_PWM_PERIOD; > > This is wrong, though it's probably because the comment is also > confusing. There should be nothing wrong with using pwm_apply_args() in > this case. In my eyes it is wrong because it results in a call to the backend driver's apply callback to get the default setting just to fix the configuration and apply that in the code to follow. > While at it, I think the conversion should also be include replacing the > call to pwm_request() by pwm_get(). There's already a PWM lookup table > in the RX1950 board code, so pwm_get() would be able to use that. Note > that for some reason that table contains a period that is different from > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all > the information other than duty cycle should be coming from either DT or > a lookup table. Yeah, I noticed that, too. It's not entirely clear to me how to do that yet. So I thought to care about getting rid of the legacy usage of pwm_config and pwm_apply_args first. Best regards Uwe
On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote: > > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote: > > > +static struct pwm_state lcd_pwm_state; > > > > You shouldn't need this. The whole point of the atomic API is that the > > PWM carries its state, so the proper way to make a change is to query > > the current state, make any required modifications and then apply the > > new state. > > I thought the whole point of the atomic API is to be atomic :-) > > > > static struct pwm_device *lcd_pwm; > > > > > > static void rx1950_lcd_power(int enable) > > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable) > > > > > > /* GPB1->OUTPUT, GPB1->0 */ > > > gpio_direction_output(S3C2410_GPB(1), 0); > > > - pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); > > > - pwm_disable(lcd_pwm); > > > + lcd_pwm_state.duty_cycle = 0; > > > + lcd_pwm_state.enabled = false; > > > + pwm_apply_state(lcd_pwm, &lcd_pwm_state); > > > > The correct way to do this would be: > > > > struct pwm_state state; > > > > pwm_get_state(lcd_pwm, &state); > > state.enabled = false; > > state.duty_cycle = 0; > > pwm_apply_state(lcd_pwm, &state); > > The difference here is that with my approach the rx1950 driver caches > the intended pwm_state saving a few cycles for repeatedly copying the > pwm cache to a stack variable at the cost of some memory. I like my > approach a little better but I'm not willing to argue about that one and > can give in. It's a redundant copy of the PWM's internal state and not guaranteed to remain in sync with the PWM hardware state. You also require a global variable, which is usually a bad idea. And we're not repeatedly copying data. We occasionally do. These operations are typically only executed once or twice per boot, so hardly anything that needs to be optimized for speed. > > > gpio_direction_output(S3C2410_GPC(0), 1); > > > gpio_direction_output(S3C2410_GPC(5), 1); > > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev) > > > return PTR_ERR(lcd_pwm); > > > } > > > > > > - /* > > > - * FIXME: pwm_apply_args() should be removed when switching to > > > - * the atomic PWM API. > > > - */ > > > - pwm_apply_args(lcd_pwm); > > > + pwm_get_state_default(lcd_pwm, &lcd_pwm_state); > > > + lcd_pwm_state.period = LCD_PWM_PERIOD; > > > > This is wrong, though it's probably because the comment is also > > confusing. There should be nothing wrong with using pwm_apply_args() in > > this case. > > In my eyes it is wrong because it results in a call to the backend > driver's apply callback to get the default setting just to fix the > configuration and apply that in the code to follow. Okay, I see. But there's already pwm_init_state() which pretty much does what you do here, right? Thinking about it some more, I wonder if we shouldn't be more consistent about state handling here. So the reason why pwm_apply_args() exists is to program the PWM with a known state if the PWM doesn't support hardware readout. Since most drivers don't support hardware readout, we need this to at some specific point synchronize the hardware and internal states. In retrospect I'm not sure that's necessary, because there's more and more evidence that we don't want to touch a PWM configuration until a consumer explicitly says so. There's also the slight problem that the pwm_apply_args() is really only necessary if the PWM driver doesn't support hardware readout, but that's not something that consumers are aware of or should worry about. In light of that I think perhaps a better solution would be to basically apply the PWM arguments to the internal state at request time. We can't do it earlier because we don't know the arguments before the PWM is requested. So I think at request time we could do something like this: if (chip->ops->get_state) chip->ops->get_state(chip, pwm, &pwm->state); else pwm_init_state(pwm, &pwm->state); That way drivers always get either the current state or the "default" state that was configured via platform-specific means (DT or lookup). > > While at it, I think the conversion should also be include replacing the > > call to pwm_request() by pwm_get(). There's already a PWM lookup table > > in the RX1950 board code, so pwm_get() would be able to use that. Note > > that for some reason that table contains a period that is different from > > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all > > the information other than duty cycle should be coming from either DT or > > a lookup table. > > Yeah, I noticed that, too. It's not entirely clear to me how to do that > yet. So I thought to care about getting rid of the legacy usage of > pwm_config and pwm_apply_args first. So I think I got the backlight PWM confused with the LCD PWM. The former is what we have in the PWM lookup table and which controls the backlight brightness. The latter is what we don't have in the table and which is requested using the legacy pwm_request() function. That also explains why there's a difference between the period in the PWM lookup table for the backlight PWM and the LCD_PWM_PERIOD macro. Adding support for pwm_get() should be as simple ad adding an entry to rx1950_pwm_lookup[], such as this: PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960, PWM_POLARITY_NORMAL), Then in rx1950_backlight_init(), which is passed the device structure that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can be called like this: lcd_pwm = pwm_get(dev, "lcd"); The matching code should then be able to find the second entry as the best match and return the correct one. Thierry
Hello Thierry, On Thu, Nov 15, 2018 at 05:15:16PM +0100, Thierry Reding wrote: > On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-König wrote: > > On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote: > > > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote: > > > > +static struct pwm_state lcd_pwm_state; > > > > static struct pwm_device *lcd_pwm; > > > > > > > > static void rx1950_lcd_power(int enable) > > > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable) > > > > > > > > /* GPB1->OUTPUT, GPB1->0 */ > > > > gpio_direction_output(S3C2410_GPB(1), 0); > > > > - pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); > > > > - pwm_disable(lcd_pwm); > > > > + lcd_pwm_state.duty_cycle = 0; > > > > + lcd_pwm_state.enabled = false; > > > > + pwm_apply_state(lcd_pwm, &lcd_pwm_state); > > > > > > The correct way to do this would be: > > > > > > struct pwm_state state; > > > > > > pwm_get_state(lcd_pwm, &state); > > > state.enabled = false; > > > state.duty_cycle = 0; > > > pwm_apply_state(lcd_pwm, &state); > > > > The difference here is that with my approach the rx1950 driver caches > > the intended pwm_state saving a few cycles for repeatedly copying the > > pwm cache to a stack variable at the cost of some memory. I like my > > approach a little better but I'm not willing to argue about that one and > > can give in. > > It's a redundant copy of the PWM's internal state That the pwm keeps a copy of this is an implementation detail of the pwm framework that shouldn't matter to its consumers. > and not guaranteed to remain in sync with the PWM hardware state. What should happen that the driver's pwm_state and the hardware state diverge? > You also require a global variable, which is usually a bad idea. Right, but that's attributable to the fact, that the struct pwm_device is a global variable, too. > And we're not repeatedly copying data. We occasionally do. These > operations are typically only executed once or twice per boot, so > hardly anything that needs to be optimized for speed. > > > > > gpio_direction_output(S3C2410_GPC(0), 1); > > > > gpio_direction_output(S3C2410_GPC(5), 1); > > > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev) > > > > return PTR_ERR(lcd_pwm); > > > > } > > > > > > > > - /* > > > > - * FIXME: pwm_apply_args() should be removed when switching to > > > > - * the atomic PWM API. > > > > - */ > > > > - pwm_apply_args(lcd_pwm); > > > > + pwm_get_state_default(lcd_pwm, &lcd_pwm_state); > > > > + lcd_pwm_state.period = LCD_PWM_PERIOD; > > > > > > This is wrong, though it's probably because the comment is also > > > confusing. There should be nothing wrong with using pwm_apply_args() in > > > this case. > > > > In my eyes it is wrong because it results in a call to the backend > > driver's apply callback to get the default setting just to fix the > > configuration and apply that in the code to follow. > > Okay, I see. But there's already pwm_init_state() which pretty much does > what you do here, right? > > Thinking about it some more, I wonder if we shouldn't be more consistent > about state handling here. So the reason why pwm_apply_args() exists is > to program the PWM with a known state if the PWM doesn't support > hardware readout. Since most drivers don't support hardware readout, we > need this to at some specific point synchronize the hardware and > internal states. > > In retrospect I'm not sure that's necessary, because there's more and > more evidence that we don't want to touch a PWM configuration until a > consumer explicitly says so. There's also the slight problem that the > pwm_apply_args() is really only necessary if the PWM driver doesn't > support hardware readout, but that's not something that consumers are > aware of or should worry about. I consider your words as a light in the tunnel. Maybe we will reach a conclusion eventually. > In light of that I think perhaps a better solution would be to basically > apply the PWM arguments to the internal state at request time. We can't > do it earlier because we don't know the arguments before the PWM is > requested. So I think at request time we could do something like this: > > if (chip->ops->get_state) > chip->ops->get_state(chip, pwm, &pwm->state); > else > pwm_init_state(pwm, &pwm->state); > > That way drivers always get either the current state or the "default" > state that was configured via platform-specific means (DT or lookup). I think a gap in this reasoning is that if the DT dictates a period of say 1 ms and the hardware is configured to something else, the consumer should still work with the 1 ms value. > > > While at it, I think the conversion should also be include replacing the > > > call to pwm_request() by pwm_get(). There's already a PWM lookup table > > > in the RX1950 board code, so pwm_get() would be able to use that. Note > > > that for some reason that table contains a period that is different from > > > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all > > > the information other than duty cycle should be coming from either DT or > > > a lookup table. > > > > Yeah, I noticed that, too. It's not entirely clear to me how to do that > > yet. So I thought to care about getting rid of the legacy usage of > > pwm_config and pwm_apply_args first. > > So I think I got the backlight PWM confused with the LCD PWM. The former > is what we have in the PWM lookup table and which controls the backlight > brightness. The latter is what we don't have in the table and which is > requested using the legacy pwm_request() function. That also explains > why there's a difference between the period in the PWM lookup table for > the backlight PWM and the LCD_PWM_PERIOD macro. > > Adding support for pwm_get() should be as simple ad adding an entry to > rx1950_pwm_lookup[], such as this: > > PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960, > PWM_POLARITY_NORMAL), > > Then in rx1950_backlight_init(), which is passed the device structure > that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can > be called like this: > > lcd_pwm = pwm_get(dev, "lcd"); > > The matching code should then be able to find the second entry as the > best match and return the correct one. Does this lookup belong to the board file or to a more general place near the provider of the "samsung-pwm" devices? Best regards Uwe
diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index 7f5a18fa305b..5c4459f9a5f7 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = { PWM_POLARITY_NORMAL), }; +static struct pwm_state lcd_pwm_state; static struct pwm_device *lcd_pwm; static void rx1950_lcd_power(int enable) @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable) /* GPB1->OUTPUT, GPB1->0 */ gpio_direction_output(S3C2410_GPB(1), 0); - pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); - pwm_disable(lcd_pwm); + lcd_pwm_state.duty_cycle = 0; + lcd_pwm_state.enabled = false; + pwm_apply_state(lcd_pwm, &lcd_pwm_state); /* GPC0->0, GPC10->0 */ gpio_direction_output(S3C2410_GPC(0), 0); gpio_direction_output(S3C2410_GPC(10), 0); } else { - pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD); - pwm_enable(lcd_pwm); + lcd_pwm_state.duty_cycle = LCD_PWM_DUTY; + lcd_pwm_state.enabled = true; + pwm_apply_state(lcd_pwm, &lcd_pwm_state); gpio_direction_output(S3C2410_GPC(0), 1); gpio_direction_output(S3C2410_GPC(5), 1); @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev) return PTR_ERR(lcd_pwm); } - /* - * FIXME: pwm_apply_args() should be removed when switching to - * the atomic PWM API. - */ - pwm_apply_args(lcd_pwm); + pwm_get_state_default(lcd_pwm, &lcd_pwm_state); + lcd_pwm_state.period = LCD_PWM_PERIOD; rx1950_lcd_power(1); rx1950_bl_power(1);
The legacy PWM API should be removed in the long run, so convert a user to the atomic PWM API. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)