Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 28.02.2018 22:04, Jani Nikula wrote:
> On Wed, 28 Feb 2018, Thierry Reding  wrote:
>> Anyone that needs something other than normal mode should use the new
>> atomic PWM API.
> 
> At the risk of revealing my true ignorance, what is the new atomic PWM
> API? Where? Examples of how one would convert old code over to the new
> API?
As far as I know, the old PWM core code uses config(), set_polarity(),
enable(), disable() methods of driver, registered as pwm_ops:
struct pwm_ops {

int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);

void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);

int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,

  int duty_ns, int period_ns);

int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,

enum pwm_polarity polarity);

int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,

   struct pwm_capture *result, unsigned long timeout);

int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);

void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);

int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,

 struct pwm_state *state);

void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,

  struct pwm_state *state);

#ifdef CONFIG_DEBUG_FS

void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);

#endif

struct module *owner;

};


to do settings on hardware. In order to so settings on a PWM the users
should have been follow the below steps:
->config()
->set_polarity()
->enable()
Moreover, if the PWM was previously enabled it should have been first
disable and then to follow the above steps in order to apply a new settings
on hardware.
The driver should have been provide, at probe, all the above function:
->config(), ->set_polarity(), ->disable(), ->enable(), function that were
used by PWM core.

Now, having atomic PWM, the driver should provide one function to PWM core,
which is ->apply() function. Every PWM has a state associated, which keeps
the period, duty cycle, polarity and enable/disable status. The driver's
->apply() function takes as argument the state that should be applied and
it takes care of applying this new state directly without asking user to
call ->disable(), then ->config()/->set_polarity(), then ->enable() to
apply new hardware settings.

The PWM consumer could set a new state for PWM it uses, using
pwm_apply_state(pwm, new_state);

Regarding the models to switch on atomic PWM, on the controller side you
can check for drivers that registers apply function at probe time.
Regarding the PWM users, you can look for pwm_apply_state()
(drivers/hwmon/pwm-fan.c or drivers/input/misc/pwm-beeper.c are some examples).

Thierry, please correct me if I'm wrong.

Thank you,
Claudiu Beznea

> 
> BR,
> Jani.
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 28.02.2018 22:04, Jani Nikula wrote:
> On Wed, 28 Feb 2018, Thierry Reding  wrote:
>> Anyone that needs something other than normal mode should use the new
>> atomic PWM API.
> 
> At the risk of revealing my true ignorance, what is the new atomic PWM
> API? Where? Examples of how one would convert old code over to the new
> API?
As far as I know, the old PWM core code uses config(), set_polarity(),
enable(), disable() methods of driver, registered as pwm_ops:
struct pwm_ops {

int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);

void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);

int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,

  int duty_ns, int period_ns);

int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,

enum pwm_polarity polarity);

int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,

   struct pwm_capture *result, unsigned long timeout);

int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);

void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);

int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,

 struct pwm_state *state);

void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,

  struct pwm_state *state);

#ifdef CONFIG_DEBUG_FS

void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);

#endif

struct module *owner;

};


to do settings on hardware. In order to so settings on a PWM the users
should have been follow the below steps:
->config()
->set_polarity()
->enable()
Moreover, if the PWM was previously enabled it should have been first
disable and then to follow the above steps in order to apply a new settings
on hardware.
The driver should have been provide, at probe, all the above function:
->config(), ->set_polarity(), ->disable(), ->enable(), function that were
used by PWM core.

Now, having atomic PWM, the driver should provide one function to PWM core,
which is ->apply() function. Every PWM has a state associated, which keeps
the period, duty cycle, polarity and enable/disable status. The driver's
->apply() function takes as argument the state that should be applied and
it takes care of applying this new state directly without asking user to
call ->disable(), then ->config()/->set_polarity(), then ->enable() to
apply new hardware settings.

The PWM consumer could set a new state for PWM it uses, using
pwm_apply_state(pwm, new_state);

Regarding the models to switch on atomic PWM, on the controller side you
can check for drivers that registers apply function at probe time.
Regarding the PWM users, you can look for pwm_apply_state()
(drivers/hwmon/pwm-fan.c or drivers/input/misc/pwm-beeper.c are some examples).

Thierry, please correct me if I'm wrong.

Thank you,
Claudiu Beznea

> 
> BR,
> Jani.
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 28.02.2018 21:44, Thierry Reding wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
> 
> I don't think it makes sense to leak mode support into the legacy API.
> The pwm_config() function is considered legacy
I missed this aspect.

 and should eventually go
> away. As such it doesn't make sense to integrate a new feature such as
> PWM modes into it. 
Agree.

All users of pwm_config() assume normal mode, and
> that's what pwm_config() should provide.
Agree.

> 
> Anyone that needs something other than normal mode should use the new
> atomic PWM API.
Agree.

> 
> Thierry
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 28.02.2018 21:44, Thierry Reding wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
> 
> I don't think it makes sense to leak mode support into the legacy API.
> The pwm_config() function is considered legacy
I missed this aspect.

 and should eventually go
> away. As such it doesn't make sense to integrate a new feature such as
> PWM modes into it. 
Agree.

All users of pwm_config() assume normal mode, and
> that's what pwm_config() should provide.
Agree.

> 
> Anyone that needs something other than normal mode should use the new
> atomic PWM API.
Agree.

> 
> Thierry
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Jani Nikula]

On Wed, 28 Feb 2018, Thierry Reding  wrote:
> Anyone that needs something other than normal mode should use the new
> atomic PWM API.

At the risk of revealing my true ignorance, what is the new atomic PWM
API? Where? Examples of how one would convert old code over to the new
API?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Jani Nikula]

On Wed, 28 Feb 2018, Thierry Reding  wrote:
> Anyone that needs something other than normal mode should use the new
> atomic PWM API.

At the risk of revealing my true ignorance, what is the new atomic PWM
API? Where? Examples of how one would convert old code over to the new
API?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Thierry Reding]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)

I don't think it makes sense to leak mode support into the legacy API.
The pwm_config() function is considered legacy and should eventually go
away. As such it doesn't make sense to integrate a new feature such as
PWM modes into it. All users of pwm_config() assume normal mode, and
that's what pwm_config() should provide.

Anyone that needs something other than normal mode should use the new
atomic PWM API.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Thierry Reding]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)

I don't think it makes sense to leak mode support into the legacy API.
The pwm_config() function is considered legacy and should eventually go
away. As such it doesn't make sense to integrate a new feature such as
PWM modes into it. All users of pwm_config() assume normal mode, and
that's what pwm_config() should provide.

Anyone that needs something other than normal mode should use the new
atomic PWM API.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 17:38, Daniel Thompson wrote:
> On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
>> On 27.02.2018 12:54, Daniel Thompson wrote:
>>> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
 On 26.02.2018 11:57, Jani Nikula wrote:
> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>>> Add PWM mode to pwm_config() function. The drivers which uses 
>>> pwm_config()
>>> were adapted to this change.
>>>
>>> Signed-off-by: Claudiu Beznea 
>>> ---
>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>>  drivers/bus/ts-nbus.c|  2 +-
>>>  drivers/clk/clk-pwm.c|  3 ++-
>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>>  drivers/leds/leds-pwm.c  |  5 -
>>>  drivers/media/rc/ir-rx51.c   |  5 -
>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>>  include/linux/pwm.h  |  6 --
>>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>>> b/drivers/video/backlight/lm3630a_bl.c
>>> index 2030a6b77a09..696fa25dafd2 100644
>>> --- a/drivers/video/backlight/lm3630a_bl.c
>>> +++ b/drivers/video/backlight/lm3630a_bl.c
>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>>> *pchip, int br, int br_max)
>>>  {
>>> unsigned int period = pchip->pdata->pwm_period;
>>> unsigned int duty = br * period / br_max;
>>> +   struct pwm_caps caps = { };
>>>  
>>> -   pwm_config(pchip->pwmd, duty, period);
>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>>
>> Well... I admit I've only really looked at the patches that impact 
>> backlight but dispersing this really odd looking bit twiddling 
>> throughout the kernel doesn't strike me a great API design.
>>
>> IMHO callers should not be required to find the first set bit in
>> some specially crafted set of capability bits simply to get sane 
>> default behaviour.
>
> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> error prone.

 Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
 OK
 from your side?

 Or, what about using a function like pwm_mode_first() to get the first 
 supported
 mode by PWM channel?

 Or, would you prefer to solve this inside pwm_config() function, let's 
 say, in
 case an invalid mode is passed as argument, to let pwm_config() to choose 
 the
 first available PWM mode for PWM channel passed as argument?
>>>
>>> What is it that actually needs solving?
>>>
>>> If a driver requests normal mode and the PWM driver cannot support it
>>> why not just return an error an move on.
>> Because, simply, I wasn't aware of what these PWM client drivers needs for.
> 
> I'm afraid you have confused me here.
> 
> Didn't you just *add* the whole concept of PWM caps with your patches?
> How could any existing call site expect anything except normal mode.
> Until now there has been no possiblity to request anything else.
Agree. And agree I was confusing in previous email, sorry about that. And
agree that there was nothing before and everything should work with PWM
normal mode.

When I choose to have BIT(ffs(caps.modes)) instead of PWM_MODE(NORMAL) I
was thinking at having these pwm_config() calls working all the time having
in mind that in future the PWM controllers that these drivers use, might
change in terms of PWM supported modes.

Thank you,
Claudiu Beznea

> 
> 
>>> Put another way, what is the use case for secretly adopting a mode the
>>> caller didn't want? Under what circumstances is this a good thing?
>> No one... But I wasn't aware of what the PWM clients needs for from their PWM
>> controllers. At this moment having BIT(ffs(caps.modes)) instead of
>> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
>> explicitly
>> registered PWM caps will use PWM normal mode.
>>
>> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
>> from
>> your side.
>>
>> Thank you,
>> Claudiu Beznea
>>>
>>>
>>> Daniel.
>>>
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 17:38, Daniel Thompson wrote:
> On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
>> On 27.02.2018 12:54, Daniel Thompson wrote:
>>> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
 On 26.02.2018 11:57, Jani Nikula wrote:
> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>>> Add PWM mode to pwm_config() function. The drivers which uses 
>>> pwm_config()
>>> were adapted to this change.
>>>
>>> Signed-off-by: Claudiu Beznea 
>>> ---
>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>>  drivers/bus/ts-nbus.c|  2 +-
>>>  drivers/clk/clk-pwm.c|  3 ++-
>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>>  drivers/leds/leds-pwm.c  |  5 -
>>>  drivers/media/rc/ir-rx51.c   |  5 -
>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>>  include/linux/pwm.h  |  6 --
>>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>>> b/drivers/video/backlight/lm3630a_bl.c
>>> index 2030a6b77a09..696fa25dafd2 100644
>>> --- a/drivers/video/backlight/lm3630a_bl.c
>>> +++ b/drivers/video/backlight/lm3630a_bl.c
>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>>> *pchip, int br, int br_max)
>>>  {
>>> unsigned int period = pchip->pdata->pwm_period;
>>> unsigned int duty = br * period / br_max;
>>> +   struct pwm_caps caps = { };
>>>  
>>> -   pwm_config(pchip->pwmd, duty, period);
>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>>
>> Well... I admit I've only really looked at the patches that impact 
>> backlight but dispersing this really odd looking bit twiddling 
>> throughout the kernel doesn't strike me a great API design.
>>
>> IMHO callers should not be required to find the first set bit in
>> some specially crafted set of capability bits simply to get sane 
>> default behaviour.
>
> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> error prone.

 Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
 OK
 from your side?

 Or, what about using a function like pwm_mode_first() to get the first 
 supported
 mode by PWM channel?

 Or, would you prefer to solve this inside pwm_config() function, let's 
 say, in
 case an invalid mode is passed as argument, to let pwm_config() to choose 
 the
 first available PWM mode for PWM channel passed as argument?
>>>
>>> What is it that actually needs solving?
>>>
>>> If a driver requests normal mode and the PWM driver cannot support it
>>> why not just return an error an move on.
>> Because, simply, I wasn't aware of what these PWM client drivers needs for.
> 
> I'm afraid you have confused me here.
> 
> Didn't you just *add* the whole concept of PWM caps with your patches?
> How could any existing call site expect anything except normal mode.
> Until now there has been no possiblity to request anything else.
Agree. And agree I was confusing in previous email, sorry about that. And
agree that there was nothing before and everything should work with PWM
normal mode.

When I choose to have BIT(ffs(caps.modes)) instead of PWM_MODE(NORMAL) I
was thinking at having these pwm_config() calls working all the time having
in mind that in future the PWM controllers that these drivers use, might
change in terms of PWM supported modes.

Thank you,
Claudiu Beznea

> 
> 
>>> Put another way, what is the use case for secretly adopting a mode the
>>> caller didn't want? Under what circumstances is this a good thing?
>> No one... But I wasn't aware of what the PWM clients needs for from their PWM
>> controllers. At this moment having BIT(ffs(caps.modes)) instead of
>> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
>> explicitly
>> registered PWM caps will use PWM normal mode.
>>
>> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
>> from
>> your side.
>>
>> Thank you,
>> Claudiu Beznea
>>>
>>>
>>> Daniel.
>>>
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
> On 27.02.2018 12:54, Daniel Thompson wrote:
> > On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> >> On 26.02.2018 11:57, Jani Nikula wrote:
> >>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>  On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> > Add PWM mode to pwm_config() function. The drivers which uses 
> > pwm_config()
> > were adapted to this change.
> >
> > Signed-off-by: Claudiu Beznea 
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >  drivers/bus/ts-nbus.c|  2 +-
> >  drivers/clk/clk-pwm.c|  3 ++-
> >  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >  drivers/hwmon/pwm-fan.c  |  2 +-
> >  drivers/input/misc/max77693-haptic.c |  2 +-
> >  drivers/input/misc/max8997_haptic.c  |  6 +-
> >  drivers/leds/leds-pwm.c  |  5 -
> >  drivers/media/rc/ir-rx51.c   |  5 -
> >  drivers/media/rc/pwm-ir-tx.c |  5 -
> >  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >  drivers/video/backlight/lp8788_bl.c  |  5 -
> >  drivers/video/backlight/pwm_bl.c | 11 +--
> >  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >  include/linux/pwm.h  |  6 --
> >  16 files changed, 70 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/video/backlight/lm3630a_bl.c 
> > b/drivers/video/backlight/lm3630a_bl.c
> > index 2030a6b77a09..696fa25dafd2 100644
> > --- a/drivers/video/backlight/lm3630a_bl.c
> > +++ b/drivers/video/backlight/lm3630a_bl.c
> > @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> > *pchip, int br, int br_max)
> >  {
> > unsigned int period = pchip->pdata->pwm_period;
> > unsigned int duty = br * period / br_max;
> > +   struct pwm_caps caps = { };
> >  
> > -   pwm_config(pchip->pwmd, duty, period);
> > +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> > +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> 
>  Well... I admit I've only really looked at the patches that impact 
>  backlight but dispersing this really odd looking bit twiddling 
>  throughout the kernel doesn't strike me a great API design.
> 
>  IMHO callers should not be required to find the first set bit in
>  some specially crafted set of capability bits simply to get sane 
>  default behaviour.
> >>>
> >>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> >>> error prone.
> >>
> >> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
> >> OK
> >> from your side?
> >>
> >> Or, what about using a function like pwm_mode_first() to get the first 
> >> supported
> >> mode by PWM channel?
> >>
> >> Or, would you prefer to solve this inside pwm_config() function, let's 
> >> say, in
> >> case an invalid mode is passed as argument, to let pwm_config() to choose 
> >> the
> >> first available PWM mode for PWM channel passed as argument?
> > 
> > What is it that actually needs solving?
> > 
> > If a driver requests normal mode and the PWM driver cannot support it
> > why not just return an error an move on.
> Because, simply, I wasn't aware of what these PWM client drivers needs for.

I'm afraid you have confused me here.

Didn't you just *add* the whole concept of PWM caps with your patches?
How could any existing call site expect anything except normal mode.
Until now there has been no possiblity to request anything else.


> > Put another way, what is the use case for secretly adopting a mode the
> > caller didn't want? Under what circumstances is this a good thing?
> No one... But I wasn't aware of what the PWM clients needs for from their PWM
> controllers. At this moment having BIT(ffs(caps.modes)) instead of
> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
> explicitly
> registered PWM caps will use PWM normal mode.
> 
> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
> from
> your side.
> 
> Thank you,
> Claudiu Beznea
> > 
> > 
> > Daniel.
> > 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
> On 27.02.2018 12:54, Daniel Thompson wrote:
> > On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> >> On 26.02.2018 11:57, Jani Nikula wrote:
> >>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>  On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> > Add PWM mode to pwm_config() function. The drivers which uses 
> > pwm_config()
> > were adapted to this change.
> >
> > Signed-off-by: Claudiu Beznea 
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >  drivers/bus/ts-nbus.c|  2 +-
> >  drivers/clk/clk-pwm.c|  3 ++-
> >  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >  drivers/hwmon/pwm-fan.c  |  2 +-
> >  drivers/input/misc/max77693-haptic.c |  2 +-
> >  drivers/input/misc/max8997_haptic.c  |  6 +-
> >  drivers/leds/leds-pwm.c  |  5 -
> >  drivers/media/rc/ir-rx51.c   |  5 -
> >  drivers/media/rc/pwm-ir-tx.c |  5 -
> >  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >  drivers/video/backlight/lp8788_bl.c  |  5 -
> >  drivers/video/backlight/pwm_bl.c | 11 +--
> >  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >  include/linux/pwm.h  |  6 --
> >  16 files changed, 70 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/video/backlight/lm3630a_bl.c 
> > b/drivers/video/backlight/lm3630a_bl.c
> > index 2030a6b77a09..696fa25dafd2 100644
> > --- a/drivers/video/backlight/lm3630a_bl.c
> > +++ b/drivers/video/backlight/lm3630a_bl.c
> > @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> > *pchip, int br, int br_max)
> >  {
> > unsigned int period = pchip->pdata->pwm_period;
> > unsigned int duty = br * period / br_max;
> > +   struct pwm_caps caps = { };
> >  
> > -   pwm_config(pchip->pwmd, duty, period);
> > +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> > +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> 
>  Well... I admit I've only really looked at the patches that impact 
>  backlight but dispersing this really odd looking bit twiddling 
>  throughout the kernel doesn't strike me a great API design.
> 
>  IMHO callers should not be required to find the first set bit in
>  some specially crafted set of capability bits simply to get sane 
>  default behaviour.
> >>>
> >>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> >>> error prone.
> >>
> >> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
> >> OK
> >> from your side?
> >>
> >> Or, what about using a function like pwm_mode_first() to get the first 
> >> supported
> >> mode by PWM channel?
> >>
> >> Or, would you prefer to solve this inside pwm_config() function, let's 
> >> say, in
> >> case an invalid mode is passed as argument, to let pwm_config() to choose 
> >> the
> >> first available PWM mode for PWM channel passed as argument?
> > 
> > What is it that actually needs solving?
> > 
> > If a driver requests normal mode and the PWM driver cannot support it
> > why not just return an error an move on.
> Because, simply, I wasn't aware of what these PWM client drivers needs for.

I'm afraid you have confused me here.

Didn't you just *add* the whole concept of PWM caps with your patches?
How could any existing call site expect anything except normal mode.
Until now there has been no possiblity to request anything else.


> > Put another way, what is the use case for secretly adopting a mode the
> > caller didn't want? Under what circumstances is this a good thing?
> No one... But I wasn't aware of what the PWM clients needs for from their PWM
> controllers. At this moment having BIT(ffs(caps.modes)) instead of
> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
> explicitly
> registered PWM caps will use PWM normal mode.
> 
> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
> from
> your side.
> 
> Thank you,
> Claudiu Beznea
> > 
> > 
> > Daniel.
> > 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 12:54, Daniel Thompson wrote:
> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
>> On 26.02.2018 11:57, Jani Nikula wrote:
>>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
 On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
>
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> *pchip, int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

 Well... I admit I've only really looked at the patches that impact 
 backlight but dispersing this really odd looking bit twiddling 
 throughout the kernel doesn't strike me a great API design.

 IMHO callers should not be required to find the first set bit in
 some specially crafted set of capability bits simply to get sane 
 default behaviour.
>>>
>>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
>>> error prone.
>>
>> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
>> from your side?
>>
>> Or, what about using a function like pwm_mode_first() to get the first 
>> supported
>> mode by PWM channel?
>>
>> Or, would you prefer to solve this inside pwm_config() function, let's say, 
>> in
>> case an invalid mode is passed as argument, to let pwm_config() to choose the
>> first available PWM mode for PWM channel passed as argument?
> 
> What is it that actually needs solving?
> 
> If a driver requests normal mode and the PWM driver cannot support it
> why not just return an error an move on.
Because, simply, I wasn't aware of what these PWM client drivers needs for.

> 
> Put another way, what is the use case for secretly adopting a mode the
> caller didn't want? Under what circumstances is this a good thing?
No one... But I wasn't aware of what the PWM clients needs for from their PWM
controllers. At this moment having BIT(ffs(caps.modes)) instead of
PWM_MODE(NORMAL) is mostly the same since all the driver that has not explicitly
registered PWM caps will use PWM normal mode.

I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK from
your side.

Thank you,
Claudiu Beznea
> 
> 
> Daniel.
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 12:54, Daniel Thompson wrote:
> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
>> On 26.02.2018 11:57, Jani Nikula wrote:
>>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
 On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
>
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> *pchip, int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

 Well... I admit I've only really looked at the patches that impact 
 backlight but dispersing this really odd looking bit twiddling 
 throughout the kernel doesn't strike me a great API design.

 IMHO callers should not be required to find the first set bit in
 some specially crafted set of capability bits simply to get sane 
 default behaviour.
>>>
>>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
>>> error prone.
>>
>> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
>> from your side?
>>
>> Or, what about using a function like pwm_mode_first() to get the first 
>> supported
>> mode by PWM channel?
>>
>> Or, would you prefer to solve this inside pwm_config() function, let's say, 
>> in
>> case an invalid mode is passed as argument, to let pwm_config() to choose the
>> first available PWM mode for PWM channel passed as argument?
> 
> What is it that actually needs solving?
> 
> If a driver requests normal mode and the PWM driver cannot support it
> why not just return an error an move on.
Because, simply, I wasn't aware of what these PWM client drivers needs for.

> 
> Put another way, what is the use case for secretly adopting a mode the
> caller didn't want? Under what circumstances is this a good thing?
No one... But I wasn't aware of what the PWM clients needs for from their PWM
controllers. At this moment having BIT(ffs(caps.modes)) instead of
PWM_MODE(NORMAL) is mostly the same since all the driver that has not explicitly
registered PWM caps will use PWM normal mode.

I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK from
your side.

Thank you,
Claudiu Beznea
> 
> 
> Daniel.
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> On 26.02.2018 11:57, Jani Nikula wrote:
> > On Thu, 22 Feb 2018, Daniel Thompson  wrote:
> >> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> >>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> >>> were adapted to this change.
> >>>
> >>> Signed-off-by: Claudiu Beznea 
> >>> ---
> >>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >>>  drivers/bus/ts-nbus.c|  2 +-
> >>>  drivers/clk/clk-pwm.c|  3 ++-
> >>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >>>  drivers/hwmon/pwm-fan.c  |  2 +-
> >>>  drivers/input/misc/max77693-haptic.c |  2 +-
> >>>  drivers/input/misc/max8997_haptic.c  |  6 +-
> >>>  drivers/leds/leds-pwm.c  |  5 -
> >>>  drivers/media/rc/ir-rx51.c   |  5 -
> >>>  drivers/media/rc/pwm-ir-tx.c |  5 -
> >>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >>>  drivers/video/backlight/lp8788_bl.c  |  5 -
> >>>  drivers/video/backlight/pwm_bl.c | 11 +--
> >>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >>>  include/linux/pwm.h  |  6 --
> >>>  16 files changed, 70 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> >>> b/drivers/video/backlight/lm3630a_bl.c
> >>> index 2030a6b77a09..696fa25dafd2 100644
> >>> --- a/drivers/video/backlight/lm3630a_bl.c
> >>> +++ b/drivers/video/backlight/lm3630a_bl.c
> >>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> >>> *pchip, int br, int br_max)
> >>>  {
> >>>   unsigned int period = pchip->pdata->pwm_period;
> >>>   unsigned int duty = br * period / br_max;
> >>> + struct pwm_caps caps = { };
> >>>  
> >>> - pwm_config(pchip->pwmd, duty, period);
> >>> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> >>> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> >>
> >> Well... I admit I've only really looked at the patches that impact 
> >> backlight but dispersing this really odd looking bit twiddling 
> >> throughout the kernel doesn't strike me a great API design.
> >>
> >> IMHO callers should not be required to find the first set bit in
> >> some specially crafted set of capability bits simply to get sane 
> >> default behaviour.
> > 
> > Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> > error prone.
> 
> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
> from your side?
>
> Or, what about using a function like pwm_mode_first() to get the first 
> supported
> mode by PWM channel?
> 
> Or, would you prefer to solve this inside pwm_config() function, let's say, in
> case an invalid mode is passed as argument, to let pwm_config() to choose the
> first available PWM mode for PWM channel passed as argument?

What is it that actually needs solving?

If a driver requests normal mode and the PWM driver cannot support it
why not just return an error an move on.

Put another way, what is the use case for secretly adopting a mode the
caller didn't want? Under what circumstances is this a good thing?


Daniel.

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> On 26.02.2018 11:57, Jani Nikula wrote:
> > On Thu, 22 Feb 2018, Daniel Thompson  wrote:
> >> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> >>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> >>> were adapted to this change.
> >>>
> >>> Signed-off-by: Claudiu Beznea 
> >>> ---
> >>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >>>  drivers/bus/ts-nbus.c|  2 +-
> >>>  drivers/clk/clk-pwm.c|  3 ++-
> >>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >>>  drivers/hwmon/pwm-fan.c  |  2 +-
> >>>  drivers/input/misc/max77693-haptic.c |  2 +-
> >>>  drivers/input/misc/max8997_haptic.c  |  6 +-
> >>>  drivers/leds/leds-pwm.c  |  5 -
> >>>  drivers/media/rc/ir-rx51.c   |  5 -
> >>>  drivers/media/rc/pwm-ir-tx.c |  5 -
> >>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >>>  drivers/video/backlight/lp8788_bl.c  |  5 -
> >>>  drivers/video/backlight/pwm_bl.c | 11 +--
> >>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >>>  include/linux/pwm.h  |  6 --
> >>>  16 files changed, 70 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> >>> b/drivers/video/backlight/lm3630a_bl.c
> >>> index 2030a6b77a09..696fa25dafd2 100644
> >>> --- a/drivers/video/backlight/lm3630a_bl.c
> >>> +++ b/drivers/video/backlight/lm3630a_bl.c
> >>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> >>> *pchip, int br, int br_max)
> >>>  {
> >>>   unsigned int period = pchip->pdata->pwm_period;
> >>>   unsigned int duty = br * period / br_max;
> >>> + struct pwm_caps caps = { };
> >>>  
> >>> - pwm_config(pchip->pwmd, duty, period);
> >>> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> >>> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> >>
> >> Well... I admit I've only really looked at the patches that impact 
> >> backlight but dispersing this really odd looking bit twiddling 
> >> throughout the kernel doesn't strike me a great API design.
> >>
> >> IMHO callers should not be required to find the first set bit in
> >> some specially crafted set of capability bits simply to get sane 
> >> default behaviour.
> > 
> > Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> > error prone.
> 
> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
> from your side?
>
> Or, what about using a function like pwm_mode_first() to get the first 
> supported
> mode by PWM channel?
> 
> Or, would you prefer to solve this inside pwm_config() function, let's say, in
> case an invalid mode is passed as argument, to let pwm_config() to choose the
> first available PWM mode for PWM channel passed as argument?

What is it that actually needs solving?

If a driver requests normal mode and the PWM driver cannot support it
why not just return an error an move on.

Put another way, what is the use case for secretly adopting a mode the
caller didn't want? Under what circumstances is this a good thing?


Daniel.

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 26.02.2018 11:57, Jani Nikula wrote:
> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>>> were adapted to this change.
>>>
>>> Signed-off-by: Claudiu Beznea 
>>> ---
>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>>  drivers/bus/ts-nbus.c|  2 +-
>>>  drivers/clk/clk-pwm.c|  3 ++-
>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>>  drivers/leds/leds-pwm.c  |  5 -
>>>  drivers/media/rc/ir-rx51.c   |  5 -
>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>>  include/linux/pwm.h  |  6 --
>>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>>> b/drivers/video/backlight/lm3630a_bl.c
>>> index 2030a6b77a09..696fa25dafd2 100644
>>> --- a/drivers/video/backlight/lm3630a_bl.c
>>> +++ b/drivers/video/backlight/lm3630a_bl.c
>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>>> *pchip, int br, int br_max)
>>>  {
>>> unsigned int period = pchip->pdata->pwm_period;
>>> unsigned int duty = br * period / br_max;
>>> +   struct pwm_caps caps = { };
>>>  
>>> -   pwm_config(pchip->pwmd, duty, period);
>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>>
>> Well... I admit I've only really looked at the patches that impact 
>> backlight but dispersing this really odd looking bit twiddling 
>> throughout the kernel doesn't strike me a great API design.
>>
>> IMHO callers should not be required to find the first set bit in
>> some specially crafted set of capability bits simply to get sane 
>> default behaviour.
> 
> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> error prone.

Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
from your side?

Or, what about using a function like pwm_mode_first() to get the first supported
mode by PWM channel?

Or, would you prefer to solve this inside pwm_config() function, let's say, in
case an invalid mode is passed as argument, to let pwm_config() to choose the
first available PWM mode for PWM channel passed as argument?

Thank you,
Claudiu Beznea

> 
> BR,
> Jani.
> 
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 26.02.2018 11:57, Jani Nikula wrote:
> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>>> were adapted to this change.
>>>
>>> Signed-off-by: Claudiu Beznea 
>>> ---
>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>>  drivers/bus/ts-nbus.c|  2 +-
>>>  drivers/clk/clk-pwm.c|  3 ++-
>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>>  drivers/leds/leds-pwm.c  |  5 -
>>>  drivers/media/rc/ir-rx51.c   |  5 -
>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>>  include/linux/pwm.h  |  6 --
>>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>>> b/drivers/video/backlight/lm3630a_bl.c
>>> index 2030a6b77a09..696fa25dafd2 100644
>>> --- a/drivers/video/backlight/lm3630a_bl.c
>>> +++ b/drivers/video/backlight/lm3630a_bl.c
>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>>> *pchip, int br, int br_max)
>>>  {
>>> unsigned int period = pchip->pdata->pwm_period;
>>> unsigned int duty = br * period / br_max;
>>> +   struct pwm_caps caps = { };
>>>  
>>> -   pwm_config(pchip->pwmd, duty, period);
>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>>
>> Well... I admit I've only really looked at the patches that impact 
>> backlight but dispersing this really odd looking bit twiddling 
>> throughout the kernel doesn't strike me a great API design.
>>
>> IMHO callers should not be required to find the first set bit in
>> some specially crafted set of capability bits simply to get sane 
>> default behaviour.
> 
> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> error prone.

Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
from your side?

Or, what about using a function like pwm_mode_first() to get the first supported
mode by PWM channel?

Or, would you prefer to solve this inside pwm_config() function, let's say, in
case an invalid mode is passed as argument, to let pwm_config() to choose the
first available PWM mode for PWM channel passed as argument?

Thank you,
Claudiu Beznea

> 
> BR,
> Jani.
> 
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Jani Nikula]

On Thu, 22 Feb 2018, Daniel Thompson  wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>> 
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>> b/drivers/video/backlight/lm3630a_bl.c
>> index 2030a6b77a09..696fa25dafd2 100644
>> --- a/drivers/video/backlight/lm3630a_bl.c
>> +++ b/drivers/video/backlight/lm3630a_bl.c
>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>> *pchip, int br, int br_max)
>>  {
>>  unsigned int period = pchip->pdata->pwm_period;
>>  unsigned int duty = br * period / br_max;
>> +struct pwm_caps caps = { };
>>  
>> -pwm_config(pchip->pwmd, duty, period);
>> +pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>> +pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>
> Well... I admit I've only really looked at the patches that impact 
> backlight but dispersing this really odd looking bit twiddling 
> throughout the kernel doesn't strike me a great API design.
>
> IMHO callers should not be required to find the first set bit in
> some specially crafted set of capability bits simply to get sane 
> default behaviour.

Agreed. IMHO the regular use case becomes rather tedious, ugly, and
error prone.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Jani Nikula]

On Thu, 22 Feb 2018, Daniel Thompson  wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>> 
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>> b/drivers/video/backlight/lm3630a_bl.c
>> index 2030a6b77a09..696fa25dafd2 100644
>> --- a/drivers/video/backlight/lm3630a_bl.c
>> +++ b/drivers/video/backlight/lm3630a_bl.c
>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>> *pchip, int br, int br_max)
>>  {
>>  unsigned int period = pchip->pdata->pwm_period;
>>  unsigned int duty = br * period / br_max;
>> +struct pwm_caps caps = { };
>>  
>> -pwm_config(pchip->pwmd, duty, period);
>> +pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>> +pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>
> Well... I admit I've only really looked at the patches that impact 
> backlight but dispersing this really odd looking bit twiddling 
> throughout the kernel doesn't strike me a great API design.
>
> IMHO callers should not be required to find the first set bit in
> some specially crafted set of capability bits simply to get sane 
> default behaviour.

Agreed. IMHO the regular use case becomes rather tedious, ugly, and
error prone.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 22.02.2018 15:01, Sean Young wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
> 
> -snip-
> 
>> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
>> index 49265f02e772..a971b02ea021 100644
>> --- a/drivers/media/rc/ir-rx51.c
>> +++ b/drivers/media/rc/ir-rx51.c
>> @@ -55,10 +55,13 @@ static int init_timing_params(struct ir_rx51 *ir_rx51)
>>  {
>>  struct pwm_device *pwm = ir_rx51->pwm;
>>  int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq);
>> +struct pwm_caps caps = { };
>>  
>>  duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100);
>>  
>> -pwm_config(pwm, duty, period);
>> +pwm_get_caps(pwm->chip, pwm, );
>> +
>> +pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  
>>  return 0;
>>  }
>> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
>> index 27d0f5837a76..c630e1b450a3 100644
>> --- a/drivers/media/rc/pwm-ir-tx.c
>> +++ b/drivers/media/rc/pwm-ir-tx.c
>> @@ -61,6 +61,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
>> *txbuf,
>>  {
>>  struct pwm_ir *pwm_ir = dev->priv;
>>  struct pwm_device *pwm = pwm_ir->pwm;
>> +struct pwm_caps caps = { };
>>  int i, duty, period;
>>  ktime_t edge;
>>  long delta;
>> @@ -68,7 +69,9 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
>> *txbuf,
>>  period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>>  duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
>>  
>> -pwm_config(pwm, duty, period);
>> +pwm_get_caps(pwm->chip, pwm, );
>> +
>> +pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  
>>  edge = ktime_get();
>>  
> 
> The two PWM rc-core drivers need PWM_MODE(NORMAL), not the first available
> mode that the device supports. If mode normal is not supported, then probe
> should fail.
OK, thank you for your inputs. I will address this in next version.


Thank you,
Claudiu Beznea
> 
> 
> Sean
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 22.02.2018 15:01, Sean Young wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
> 
> -snip-
> 
>> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
>> index 49265f02e772..a971b02ea021 100644
>> --- a/drivers/media/rc/ir-rx51.c
>> +++ b/drivers/media/rc/ir-rx51.c
>> @@ -55,10 +55,13 @@ static int init_timing_params(struct ir_rx51 *ir_rx51)
>>  {
>>  struct pwm_device *pwm = ir_rx51->pwm;
>>  int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq);
>> +struct pwm_caps caps = { };
>>  
>>  duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100);
>>  
>> -pwm_config(pwm, duty, period);
>> +pwm_get_caps(pwm->chip, pwm, );
>> +
>> +pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  
>>  return 0;
>>  }
>> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
>> index 27d0f5837a76..c630e1b450a3 100644
>> --- a/drivers/media/rc/pwm-ir-tx.c
>> +++ b/drivers/media/rc/pwm-ir-tx.c
>> @@ -61,6 +61,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
>> *txbuf,
>>  {
>>  struct pwm_ir *pwm_ir = dev->priv;
>>  struct pwm_device *pwm = pwm_ir->pwm;
>> +struct pwm_caps caps = { };
>>  int i, duty, period;
>>  ktime_t edge;
>>  long delta;
>> @@ -68,7 +69,9 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
>> *txbuf,
>>  period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>>  duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
>>  
>> -pwm_config(pwm, duty, period);
>> +pwm_get_caps(pwm->chip, pwm, );
>> +
>> +pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  
>>  edge = ktime_get();
>>  
> 
> The two PWM rc-core drivers need PWM_MODE(NORMAL), not the first available
> mode that the device supports. If mode normal is not supported, then probe
> should fail.
OK, thank you for your inputs. I will address this in next version.


Thank you,
Claudiu Beznea
> 
> 
> Sean
> 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 22.02.2018 14:33, Daniel Thompson wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>> b/drivers/video/backlight/lm3630a_bl.c
>> index 2030a6b77a09..696fa25dafd2 100644
>> --- a/drivers/video/backlight/lm3630a_bl.c
>> +++ b/drivers/video/backlight/lm3630a_bl.c
>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>> *pchip, int br, int br_max)
>>  {
>>  unsigned int period = pchip->pdata->pwm_period;
>>  unsigned int duty = br * period / br_max;
>> +struct pwm_caps caps = { };
>>  
>> -pwm_config(pchip->pwmd, duty, period);
>> +pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>> +pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> 
> Well... I admit I've only really looked at the patches that impact 
> backlight but dispersing this really odd looking bit twiddling 
> throughout the kernel doesn't strike me a great API design.>
> IMHO callers should not be required to find the first set bit in
> some specially crafted set of capability bits simply to get sane 
> default behaviour.
Thank you for your inputs. I will try to have a fix for this in next version.

The idea with this was to locate first available PWM mode of PWM channel. If the
driver hasn't registered any PWM capabilities related ops the default
capabilities will include only PWM normal mode. In case the PWM channel will
register different capabilities taking the first available mode from caps.modes
and passing it as argument to pwm_config() will ensure the pwm_apply_state()
will not fail.

Thank you,
Claudiu Beznea

> 
> 
> Daniel.
> 
> 
> 
> 
>>  if (duty)
>>  pwm_enable(pchip->pwmd);
>>  else
>> diff --git a/drivers/video/backlight/lp855x_bl.c 
>> b/drivers/video/backlight/lp855x_bl.c
>> index 939f057836e1..3d274c604862 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
>> int max_br)
>>  unsigned int period = lp->pdata->period_ns;
>>  unsigned int duty = br * period / max_br;
>>  struct pwm_device *pwm;
>> +struct pwm_caps caps = { };
>>  
>>  /* request pwm device with the consumer name */
>>  if (!lp->pwm) {
>> @@ -256,7 +257,8 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
>> int max_br)
>>  pwm_apply_args(pwm);
>>  }
>>  
>> -pwm_config(lp->pwm, duty, period);
>> +pwm_get_caps(lp->pwm->chip, lp->pwm, );
>> +pwm_config(lp->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  if (duty)
>>  pwm_enable(lp->pwm);
>>  else
>> diff --git a/drivers/video/backlight/lp8788_bl.c 
>> b/drivers/video/backlight/lp8788_bl.c
>> index cf869ec90cce..06de3163650d 100644
>> --- a/drivers/video/backlight/lp8788_bl.c
>> +++ b/drivers/video/backlight/lp8788_bl.c
>> @@ -128,6 +128,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int 
>> br, int max_br)
>>  unsigned int duty;
>>  struct device *dev;
>>  struct pwm_device *pwm;
>> +struct pwm_caps caps = { };
>>  
>>  if (!bl->pdata)
>>  return;
>> @@ -153,7 +154,9 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int 
>> br, int max_br)
>>  pwm_apply_args(pwm);
>>  }
>>  
>> -pwm_config(bl->pwm, duty, period);
>> +pwm_get_caps(bl->pwm->chip, bl->pwm, );
>> +
>> +pwm_config(bl->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  if (duty)
>>  pwm_enable(bl->pwm);
>>  else
>> diff --git a/drivers/video/backlight/pwm_bl.c 
>> b/drivers/video/backlight/pwm_bl.c
>> index 1c2289ddd555..706a9ab053a7 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -63,10 +63,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
>> *pb, int brightness)
>>  
>>  static void 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 22.02.2018 14:33, Daniel Thompson wrote:
> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
>> were adapted to this change.
>>
>> Signed-off-by: Claudiu Beznea 
>> ---
>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>  drivers/bus/ts-nbus.c|  2 +-
>>  drivers/clk/clk-pwm.c|  3 ++-
>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>  drivers/leds/leds-pwm.c  |  5 -
>>  drivers/media/rc/ir-rx51.c   |  5 -
>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>  include/linux/pwm.h  |  6 --
>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>> b/drivers/video/backlight/lm3630a_bl.c
>> index 2030a6b77a09..696fa25dafd2 100644
>> --- a/drivers/video/backlight/lm3630a_bl.c
>> +++ b/drivers/video/backlight/lm3630a_bl.c
>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>> *pchip, int br, int br_max)
>>  {
>>  unsigned int period = pchip->pdata->pwm_period;
>>  unsigned int duty = br * period / br_max;
>> +struct pwm_caps caps = { };
>>  
>> -pwm_config(pchip->pwmd, duty, period);
>> +pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>> +pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> 
> Well... I admit I've only really looked at the patches that impact 
> backlight but dispersing this really odd looking bit twiddling 
> throughout the kernel doesn't strike me a great API design.>
> IMHO callers should not be required to find the first set bit in
> some specially crafted set of capability bits simply to get sane 
> default behaviour.
Thank you for your inputs. I will try to have a fix for this in next version.

The idea with this was to locate first available PWM mode of PWM channel. If the
driver hasn't registered any PWM capabilities related ops the default
capabilities will include only PWM normal mode. In case the PWM channel will
register different capabilities taking the first available mode from caps.modes
and passing it as argument to pwm_config() will ensure the pwm_apply_state()
will not fail.

Thank you,
Claudiu Beznea

> 
> 
> Daniel.
> 
> 
> 
> 
>>  if (duty)
>>  pwm_enable(pchip->pwmd);
>>  else
>> diff --git a/drivers/video/backlight/lp855x_bl.c 
>> b/drivers/video/backlight/lp855x_bl.c
>> index 939f057836e1..3d274c604862 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
>> int max_br)
>>  unsigned int period = lp->pdata->period_ns;
>>  unsigned int duty = br * period / max_br;
>>  struct pwm_device *pwm;
>> +struct pwm_caps caps = { };
>>  
>>  /* request pwm device with the consumer name */
>>  if (!lp->pwm) {
>> @@ -256,7 +257,8 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
>> int max_br)
>>  pwm_apply_args(pwm);
>>  }
>>  
>> -pwm_config(lp->pwm, duty, period);
>> +pwm_get_caps(lp->pwm->chip, lp->pwm, );
>> +pwm_config(lp->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  if (duty)
>>  pwm_enable(lp->pwm);
>>  else
>> diff --git a/drivers/video/backlight/lp8788_bl.c 
>> b/drivers/video/backlight/lp8788_bl.c
>> index cf869ec90cce..06de3163650d 100644
>> --- a/drivers/video/backlight/lp8788_bl.c
>> +++ b/drivers/video/backlight/lp8788_bl.c
>> @@ -128,6 +128,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int 
>> br, int max_br)
>>  unsigned int duty;
>>  struct device *dev;
>>  struct pwm_device *pwm;
>> +struct pwm_caps caps = { };
>>  
>>  if (!bl->pdata)
>>  return;
>> @@ -153,7 +154,9 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int 
>> br, int max_br)
>>  pwm_apply_args(pwm);
>>  }
>>  
>> -pwm_config(bl->pwm, duty, period);
>> +pwm_get_caps(bl->pwm->chip, bl->pwm, );
>> +
>> +pwm_config(bl->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>>  if (duty)
>>  pwm_enable(bl->pwm);
>>  else
>> diff --git a/drivers/video/backlight/pwm_bl.c 
>> b/drivers/video/backlight/pwm_bl.c
>> index 1c2289ddd555..706a9ab053a7 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -63,10 +63,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
>> *pb, int brightness)
>>  
>>  static void pwm_backlight_power_off(struct 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Sean Young]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---

-snip-

> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> index 49265f02e772..a971b02ea021 100644
> --- a/drivers/media/rc/ir-rx51.c
> +++ b/drivers/media/rc/ir-rx51.c
> @@ -55,10 +55,13 @@ static int init_timing_params(struct ir_rx51 *ir_rx51)
>  {
>   struct pwm_device *pwm = ir_rx51->pwm;
>   int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq);
> + struct pwm_caps caps = { };
>  
>   duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100);
>  
> - pwm_config(pwm, duty, period);
> + pwm_get_caps(pwm->chip, pwm, );
> +
> + pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>  
>   return 0;
>  }
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index 27d0f5837a76..c630e1b450a3 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -61,6 +61,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
> *txbuf,
>  {
>   struct pwm_ir *pwm_ir = dev->priv;
>   struct pwm_device *pwm = pwm_ir->pwm;
> + struct pwm_caps caps = { };
>   int i, duty, period;
>   ktime_t edge;
>   long delta;
> @@ -68,7 +69,9 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
> *txbuf,
>   period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>   duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
>  
> - pwm_config(pwm, duty, period);
> + pwm_get_caps(pwm->chip, pwm, );
> +
> + pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>  
>   edge = ktime_get();
>  

The two PWM rc-core drivers need PWM_MODE(NORMAL), not the first available
mode that the device supports. If mode normal is not supported, then probe
should fail.


Sean

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Sean Young]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---

-snip-

> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> index 49265f02e772..a971b02ea021 100644
> --- a/drivers/media/rc/ir-rx51.c
> +++ b/drivers/media/rc/ir-rx51.c
> @@ -55,10 +55,13 @@ static int init_timing_params(struct ir_rx51 *ir_rx51)
>  {
>   struct pwm_device *pwm = ir_rx51->pwm;
>   int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq);
> + struct pwm_caps caps = { };
>  
>   duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100);
>  
> - pwm_config(pwm, duty, period);
> + pwm_get_caps(pwm->chip, pwm, );
> +
> + pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>  
>   return 0;
>  }
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index 27d0f5837a76..c630e1b450a3 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -61,6 +61,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
> *txbuf,
>  {
>   struct pwm_ir *pwm_ir = dev->priv;
>   struct pwm_device *pwm = pwm_ir->pwm;
> + struct pwm_caps caps = { };
>   int i, duty, period;
>   ktime_t edge;
>   long delta;
> @@ -68,7 +69,9 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int 
> *txbuf,
>   period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>   duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
>  
> - pwm_config(pwm, duty, period);
> + pwm_get_caps(pwm->chip, pwm, );
> +
> + pwm_config(pwm, duty, period, BIT(ffs(caps.modes) - 1));
>  
>   edge = ktime_get();
>  

The two PWM rc-core drivers need PWM_MODE(NORMAL), not the first available
mode that the device supports. If mode normal is not supported, then probe
should fail.


Sean

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, 
> int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

Well... I admit I've only really looked at the patches that impact 
backlight but dispersing this really odd looking bit twiddling 
throughout the kernel doesn't strike me a great API design.

IMHO callers should not be required to find the first set bit in
some specially crafted set of capability bits simply to get sane 
default behaviour.


Daniel.




>   if (duty)
>   pwm_enable(pchip->pwmd);
>   else
> diff --git a/drivers/video/backlight/lp855x_bl.c 
> b/drivers/video/backlight/lp855x_bl.c
> index 939f057836e1..3d274c604862 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   unsigned int period = lp->pdata->period_ns;
>   unsigned int duty = br * period / max_br;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   /* request pwm device with the consumer name */
>   if (!lp->pwm) {
> @@ -256,7 +257,8 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(lp->pwm, duty, period);
> + pwm_get_caps(lp->pwm->chip, lp->pwm, );
> + pwm_config(lp->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(lp->pwm);
>   else
> diff --git a/drivers/video/backlight/lp8788_bl.c 
> b/drivers/video/backlight/lp8788_bl.c
> index cf869ec90cce..06de3163650d 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -128,6 +128,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   unsigned int duty;
>   struct device *dev;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   if (!bl->pdata)
>   return;
> @@ -153,7 +154,9 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(bl->pwm, duty, period);
> + pwm_get_caps(bl->pwm->chip, bl->pwm, );
> +
> + pwm_config(bl->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(bl->pwm);
>   else
> diff --git a/drivers/video/backlight/pwm_bl.c 
> b/drivers/video/backlight/pwm_bl.c
> index 1c2289ddd555..706a9ab053a7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -63,10 +63,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
> *pb, int brightness)
>  
>  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  {
> + struct pwm_caps caps = { };
> +
>   if (!pb->enabled)
>   return;
>  
> - pwm_config(pb->pwm, 0, pb->period);
> + pwm_get_caps(pb->pwm->chip, pb->pwm, );
> +
> + pwm_config(pb->pwm, 0, pb->period, BIT(ffs(caps.modes) - 1));
>   pwm_disable(pb->pwm);
>  
>   if (pb->enable_gpio)
> @@ -96,6 +100,7 @@ static int pwm_backlight_update_status(struct 
> backlight_device *bl)
>  {
>   struct pwm_bl_data *pb = bl_get_data(bl);
>   int brightness = bl->props.brightness;
> + struct pwm_caps caps = { };
>   int duty_cycle;
>  
>   if 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, 
> int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

Well... I admit I've only really looked at the patches that impact 
backlight but dispersing this really odd looking bit twiddling 
throughout the kernel doesn't strike me a great API design.

IMHO callers should not be required to find the first set bit in
some specially crafted set of capability bits simply to get sane 
default behaviour.


Daniel.




>   if (duty)
>   pwm_enable(pchip->pwmd);
>   else
> diff --git a/drivers/video/backlight/lp855x_bl.c 
> b/drivers/video/backlight/lp855x_bl.c
> index 939f057836e1..3d274c604862 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   unsigned int period = lp->pdata->period_ns;
>   unsigned int duty = br * period / max_br;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   /* request pwm device with the consumer name */
>   if (!lp->pwm) {
> @@ -256,7 +257,8 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(lp->pwm, duty, period);
> + pwm_get_caps(lp->pwm->chip, lp->pwm, );
> + pwm_config(lp->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(lp->pwm);
>   else
> diff --git a/drivers/video/backlight/lp8788_bl.c 
> b/drivers/video/backlight/lp8788_bl.c
> index cf869ec90cce..06de3163650d 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -128,6 +128,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   unsigned int duty;
>   struct device *dev;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   if (!bl->pdata)
>   return;
> @@ -153,7 +154,9 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(bl->pwm, duty, period);
> + pwm_get_caps(bl->pwm->chip, bl->pwm, );
> +
> + pwm_config(bl->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(bl->pwm);
>   else
> diff --git a/drivers/video/backlight/pwm_bl.c 
> b/drivers/video/backlight/pwm_bl.c
> index 1c2289ddd555..706a9ab053a7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -63,10 +63,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
> *pb, int brightness)
>  
>  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  {
> + struct pwm_caps caps = { };
> +
>   if (!pb->enabled)
>   return;
>  
> - pwm_config(pb->pwm, 0, pb->period);
> + pwm_get_caps(pb->pwm->chip, pb->pwm, );
> +
> + pwm_config(pb->pwm, 0, pb->period, BIT(ffs(caps.modes) - 1));
>   pwm_disable(pb->pwm);
>  
>   if (pb->enable_gpio)
> @@ -96,6 +100,7 @@ static int pwm_backlight_update_status(struct 
> backlight_device *bl)
>  {
>   struct pwm_bl_data *pb = bl_get_data(bl);
>   int brightness = bl->props.brightness;
> + struct pwm_caps caps = { };
>   int duty_cycle;
>  
>   if (bl->props.power !=