All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: Lee Jones <lee.jones@linaro.org>,
	Daniel Thompson <daniel.thompson@linaro.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	dri-devel@lists.freedesktop.org, linux-leds@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree
Date: Tue, 11 Jun 2019 21:58:59 +0000	[thread overview]
Message-ID: <20190611215859.GF137143@google.com> (raw)
In-Reply-To: <20190611101843.GD7526@amd>

Hi Pavel,

On Tue, Jun 11, 2019 at 12:18:43PM +0200, Pavel Machek wrote:
> On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> > 
> > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> 
> I don't think this one is suitable for stable. I'm pretty sure the
> heuristics works well for many boards, and you just replaced it with
> another heuristics ("256").

whether the patch is suitable for stable/upstream is certainly
debatable, in any case I'd argue the current heuristic is bogus and
works by accident or at a cost:

nlevels = period / hweight(period)

w/ period = 131071 ns  (0x1FFFF)

  nlevels = 131071 / 17 = 7710

w/ period = 131072 ns (0x20000)

  nlevels = 131072 / 1 = 131072

and some PWMs use significantly higher periods like 1 ms or 10 ms.

WARNING: multiple messages have this Message-ID (diff)
From: Matthias Kaehlcke <mka@chromium.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: Lee Jones <lee.jones@linaro.org>,
	Daniel Thompson <daniel.thompson@linaro.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	dri-devel@lists.freedesktop.org, linux-leds@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree
Date: Tue, 11 Jun 2019 14:58:59 -0700	[thread overview]
Message-ID: <20190611215859.GF137143@google.com> (raw)
In-Reply-To: <20190611101843.GD7526@amd>

Hi Pavel,

On Tue, Jun 11, 2019 at 12:18:43PM +0200, Pavel Machek wrote:
> On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> > 
> > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> 
> I don't think this one is suitable for stable. I'm pretty sure the
> heuristics works well for many boards, and you just replaced it with
> another heuristics ("256").

whether the patch is suitable for stable/upstream is certainly
debatable, in any case I'd argue the current heuristic is bogus and
works by accident or at a cost:

nlevels = period / hweight(period)

w/ period = 131071 ns  (0x1FFFF)

  nlevels = 131071 / 17 = 7710

w/ period = 131072 ns (0x20000)

  nlevels = 131072 / 1 = 131072

and some PWMs use significantly higher periods like 1 ms or 10 ms.

  reply	other threads:[~2019-06-11 21:58 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-28 11:16 [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property Enric Balletbo i Serra
2017-06-28 11:16 ` Enric Balletbo i Serra
2017-06-28 11:16 ` [PATCH 2/2] pwm-backlight: Add support for " Enric Balletbo i Serra
2017-06-28 11:16   ` Enric Balletbo i Serra
2017-06-28 13:16 ` [PATCH 1/2] dt-bindings: pwm-backlight: Add " Daniel Thompson
2017-06-28 13:16   ` Daniel Thompson
2017-06-28 13:30 ` Pavel Machek
2017-06-28 13:30   ` Pavel Machek
2017-06-28 14:15   ` Enric Balletbo Serra
2017-06-28 14:15     ` Enric Balletbo Serra
2017-06-29 13:07     ` Daniel Thompson
2017-06-29 13:07       ` Daniel Thompson
2019-06-10 23:37 ` [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property Matthias Kaehlcke
2019-06-10 23:37   ` Matthias Kaehlcke
2019-06-10 23:37   ` [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree Matthias Kaehlcke
2019-06-10 23:37     ` Matthias Kaehlcke
2019-06-11 10:18     ` Pavel Machek
2019-06-11 10:18       ` Pavel Machek
2019-06-11 10:18       ` Pavel Machek
2019-06-11 21:58       ` Matthias Kaehlcke [this message]
2019-06-11 21:58         ` Matthias Kaehlcke
2019-06-11 15:33     ` Daniel Thompson
2019-06-11 15:33       ` Daniel Thompson
2019-06-11 15:33       ` Daniel Thompson
2019-06-11 17:01       ` Matthias Kaehlcke
2019-06-11 17:01         ` Matthias Kaehlcke
2019-06-11 10:14   ` [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property Pavel Machek
2019-06-11 10:14     ` Pavel Machek
2019-06-11 10:28   ` Thierry Reding
2019-06-11 10:28     ` Thierry Reding
2019-06-11 21:38     ` Matthias Kaehlcke
2019-06-11 21:38       ` Matthias Kaehlcke
2019-06-11 20:02   ` Jacek Anaszewski
2019-06-11 20:02     ` Jacek Anaszewski
2019-06-11 22:11     ` Matthias Kaehlcke
2019-06-11 22:11       ` Matthias Kaehlcke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190611215859.GF137143@google.com \
    --to=mka@chromium.org \
    --cc=b.zolnierkie@samsung.com \
    --cc=briannorris@chromium.org \
    --cc=daniel.thompson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=enric.balletbo@collabora.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=jingoohan1@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.