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

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 <claudiu.beznea@microchip.com>
>> ---
>>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <thierry.reding@gmail.com> 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.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html