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 <daniel.thompson@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>> ---
> >>>  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, &caps);
> >>> +	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.