On 27.02.2018 17:38, Daniel Thompson wrote:

On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:

quoted

On 27.02.2018 12:54, Daniel Thompson wrote:

quoted

On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:

quoted

On 26.02.2018 11:57, Jani Nikula wrote:

quoted

On Thu, 22 Feb 2018, Daniel Thompson [off-list ref] wrote:

quoted

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:

quoted

Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
were adapted to this change.

Signed-off-by: Claudiu Beznea <redacted>
---
 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.

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

quoted

quoted

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

quoted

Daniel.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx