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 <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.
> 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.
> >