* Re: [GIT PULL] On-demand device probing
From: Mark Brown @ 2015-10-14 9:26 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Rob Herring, Russell King, Greg Kroah-Hartman, Michael Turquette,
Stephen Boyd, Vinod Koul, Dan Williams, Linus Walleij,
Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse, Liam Girdwood,
Felipe Balbi
In-Reply-To: <561E1378.6000906@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 242 bytes --]
On Wed, Oct 14, 2015 at 10:34:00AM +0200, Tomeu Vizoso wrote:
> git+ssh://git.collabora.co.uk/git/user/tomeu/linux.git
> on-demand-probes-for-next
In don't think that's the URL you intended to use (also everything looks
word wrapped here)?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH] pwm-backlight: fix the panel power sequence
From: YH Huang @ 2015-10-14 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1442414556-32381-1-git-send-email-yh.huang@mediatek.com>
Hi all,
If you have any suggestion, please let me know.
Thanks.
Regards,
YH Huang
On Wed, 2015-09-16 at 22:42 +0800, YH Huang wrote:
> In order to match the panel power sequence, disable the enable_gpio
> in the probe function. Also, reorder the code in the power_on and
> power_off function to match the timing.
>
> Signed-off-by: YH Huang <yh.huang@mediatek.com>
> ---
> drivers/video/backlight/pwm_bl.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index eff379b..99eca1e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> if (err < 0)
> dev_err(pb->dev, "failed to enable power supply\n");
>
> + pwm_enable(pb->pwm);
> +
> if (pb->enable_gpio)
> gpiod_set_value(pb->enable_gpio, 1);
>
> - pwm_enable(pb->pwm);
> pb->enabled = true;
> }
>
> @@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> if (!pb->enabled)
> return;
>
> - pwm_config(pb->pwm, 0, pb->period);
> - pwm_disable(pb->pwm);
> -
> if (pb->enable_gpio)
> gpiod_set_value(pb->enable_gpio, 0);
>
> + pwm_config(pb->pwm, 0, pb->period);
> + pwm_disable(pb->pwm);
> +
> regulator_disable(pb->power_supply);
> pb->enabled = false;
> }
> @@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->dev = &pdev->dev;
> pb->enabled = false;
>
> - pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> - GPIOD_OUT_HIGH);
> + pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
> if (IS_ERR(pb->enable_gpio)) {
> ret = PTR_ERR(pb->enable_gpio);
> goto err_alloc;
> @@ -264,6 +264,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> + if (pb->enable_gpio)
> + gpiod_direction_output(pb->enable_gpio, 0);
> +
> pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> if (IS_ERR(pb->power_supply)) {
> ret = PTR_ERR(pb->power_supply);
^ permalink raw reply
* Re: [PATCH RFC 1/2] dt-bindings: simplefb: Support a list of regulator supply properties
From: Mark Brown @ 2015-10-14 10:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <561CADFE.2010609@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]
On Tue, Oct 13, 2015 at 09:08:46AM +0200, Hans de Goede wrote:
> On 13-10-15 04:22, Chen-Yu Tsai wrote:
> >On Tue, Oct 13, 2015 at 1:10 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >>On Tue, Oct 13, 2015 at 01:04:17AM +0800, Chen-Yu Tsai wrote:
> >>>+- num-supplies : The number of regulators used by the framebuffer.
> >>>+- vinN-supply : The N-th (from 0) regulator used by the framebuffer.
> >>I don't see why you need num-supplies. Why not just try probing
> >>vin${N}-supply until such a property isn't present in the DT?
> +1 for this.
Even better would be to just enumerate all the properties on the node
and request anything with a FOO-supply name. That way we can keep
using standard regulator bindings that name the supplies after their
actual names on the device.
> > That's doable. Though I'd add a hard limit on it. Does 16 seem
> > reasonable?
> I would not add a hard limit to the binding, you can use a fixed array in
> the code to make the code simpler. I would say 8 should be sufficient, since
> the limit will only be in the code we can always bump it when we need
> to.
Or just dynamically allocate the array and resize as needed if it starts
to get to be a problem.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/2] simplefb: Add regulator handling support
From: Mark Brown @ 2015-10-14 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <561CAFE8.9080506@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]
On Tue, Oct 13, 2015 at 09:16:56AM +0200, Hans de Goede wrote:
> On 12-10-15 19:04, Chen-Yu Tsai wrote:
> >Now the DT bindings don't support a list of regulators directly, so
> >I'm working around it by having a "num-supplies" property to specify
> >the number of supply properties to check, and name the actual supplies
> >as "vinN-supply".
> Hmm, I can see the need for a "supplies" property with a list of regulators
> in other use-cases (e.g. the generic mmc-pwrseq driver) too. Now as discussed
> we can simply do vin0-supply - vinN-supply properties and be done with it,
> but maybe we need to actually add support for a generic "supplies" property ?
I really don't like having unnamed supplies, or supplies with names that
don't correspond to the schematic names for the physical supplies. It
makes it harder to go between the DT and the schematic and encourages
bad practice on specific chip bindings which should be done properly
since it's harder to tell if the binding is done correctly.
Adding something with the pattern of parallel arrays of phandles and
names properties that got introduced after the regulator bindings were
done also means we need to go and update every single binding using
regulators to document the new properties which is going to be tedious
and require constant policing for a while. I'm also not a big fan of
the pattern from a legibility point of view but that's a separate thing.
> And if not then maybe we need a few generic helper devm helper function which
> takes a node, figures out how much vinN-supply properties there are and returns
> a dynamically allocated array containing references to all the regulators, or
> a PTR_ERR in case of err, at which point the caller is expected to fail the
> probe so that any successfully acquired regulators are released.
I can see it for this sort of simplefb thing but I'm not sure how we'd
discourage drivers for specific hardware from also using the same helper
which then makes it easy to get sloppy board DTs which I'd expect to
lead to hassle down the road as drivers try to use their supplies and
find that actual DTs have things that don't correspond to reality in
them. The nice thing about having drivers name the supplies they're
expecting is that it makes describing the board as it really is much
more the path of least resistance.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/2] simplefb: Add regulator handling support
From: Hans de Goede @ 2015-10-14 11:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20151014105556.GT14956@sirena.org.uk>
Hi,
On 14-10-15 12:55, Mark Brown wrote:
> On Tue, Oct 13, 2015 at 09:16:56AM +0200, Hans de Goede wrote:
>> On 12-10-15 19:04, Chen-Yu Tsai wrote:
>
>>> Now the DT bindings don't support a list of regulators directly, so
>>> I'm working around it by having a "num-supplies" property to specify
>>> the number of supply properties to check, and name the actual supplies
>>> as "vinN-supply".
>
>> Hmm, I can see the need for a "supplies" property with a list of regulators
>> in other use-cases (e.g. the generic mmc-pwrseq driver) too. Now as discussed
>> we can simply do vin0-supply - vinN-supply properties and be done with it,
>> but maybe we need to actually add support for a generic "supplies" property ?
>
> I really don't like having unnamed supplies, or supplies with names that
> don't correspond to the schematic names for the physical supplies. It
> makes it harder to go between the DT and the schematic and encourages
> bad practice on specific chip bindings which should be done properly
> since it's harder to tell if the binding is done correctly.
Ok.
> Adding something with the pattern of parallel arrays of phandles and
> names properties that got introduced after the regulator bindings were
> done also means we need to go and update every single binding using
> regulators to document the new properties which is going to be tedious
> and require constant policing for a while. I'm also not a big fan of
> the pattern from a legibility point of view but that's a separate thing.
Oh no, I was not suggesting to have this replace how we currently do
things, I was merely suggesting allowing to have a supplies list property
for bindings where a list of (unnamed) supplies makes sense like simplefb.
I fully agree that we do not want to see matching a supplies-names prop,
if names are needed the old name-supply schema should be used just like
it is today.
>> And if not then maybe we need a few generic helper devm helper function which
>> takes a node, figures out how much vinN-supply properties there are and returns
>> a dynamically allocated array containing references to all the regulators, or
>> a PTR_ERR in case of err, at which point the caller is expected to fail the
>> probe so that any successfully acquired regulators are released.
>
> I can see it for this sort of simplefb thing but I'm not sure how we'd
> discourage drivers for specific hardware from also using the same helper
> which then makes it easy to get sloppy board DTs which I'd expect to
> lead to hassle down the road as drivers try to use their supplies and
> find that actual DTs have things that don't correspond to reality in
> them. The nice thing about having drivers name the supplies they're
> expecting is that it makes describing the board as it really is much
> more the path of least resistance.
Ok, so as said I see some value in this for generic drivers like
simplefb, mmc-pwrseq, but also the generic ahci-platform, ohci-platform
and ehci-platform drivers, where often it is possible to use the generic
driver (together with a soc specific phy driver) without needing to
introduce new compatibles, as all we need is to specify a phy(s),
bunch of clocks, resets, etc. It would be good IMHO if we could specify
e.g. this is a generic ehci block, which needs this list of supplies
to be enabled (note typically the supplies are tied to the phy, so
maybe not the best example).
I like your idea in your other mail where you suggest to actually
use foo-supply and bar-supply names in the simplefb node, and then have
some code simple iterate over all the properties and check for *-supply
properties, so that the proper, schematic matching names can be used.
But surely if we go this way having a helper for this so that others
can re-use that likely not entirely trivial code is a good idea ?
One user which comes to mind immediately here is the generic mmc-pwrseq
driver.
I agree that we need to be careful to not use a helper like this too
much, but I do believe it will make sense to have it in some rare cases.
We can put a big warning in both the header declaring it and above
the implementation to use it scarcely.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] pwm-backlight: fix the panel power sequence
From: Lee Jones @ 2015-10-14 12:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1444816875.31675.2.camel@mtksdaap41>
On Wed, 14 Oct 2015, YH Huang wrote:
> Hi all,
>
> If you have any suggestion, please let me know.
> Thanks.
Please don't do that.
If you think the patch has fallen through the gaps, please just RESEND
it, like [RESEND vX] $subject.
> On Wed, 2015-09-16 at 22:42 +0800, YH Huang wrote:
> > In order to match the panel power sequence, disable the enable_gpio
> > in the probe function. Also, reorder the code in the power_on and
> > power_off function to match the timing.
> >
> > Signed-off-by: YH Huang <yh.huang@mediatek.com>
> > ---
> > drivers/video/backlight/pwm_bl.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index eff379b..99eca1e 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> > if (err < 0)
> > dev_err(pb->dev, "failed to enable power supply\n");
> >
> > + pwm_enable(pb->pwm);
> > +
> > if (pb->enable_gpio)
> > gpiod_set_value(pb->enable_gpio, 1);
> >
> > - pwm_enable(pb->pwm);
> > pb->enabled = true;
> > }
> >
> > @@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> > if (!pb->enabled)
> > return;
> >
> > - pwm_config(pb->pwm, 0, pb->period);
> > - pwm_disable(pb->pwm);
> > -
> > if (pb->enable_gpio)
> > gpiod_set_value(pb->enable_gpio, 0);
> >
> > + pwm_config(pb->pwm, 0, pb->period);
> > + pwm_disable(pb->pwm);
> > +
> > regulator_disable(pb->power_supply);
> > pb->enabled = false;
> > }
> > @@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > pb->dev = &pdev->dev;
> > pb->enabled = false;
> >
> > - pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> > - GPIOD_OUT_HIGH);
> > + pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
> > if (IS_ERR(pb->enable_gpio)) {
> > ret = PTR_ERR(pb->enable_gpio);
> > goto err_alloc;
> > @@ -264,6 +264,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> > }
> >
> > + if (pb->enable_gpio)
> > + gpiod_direction_output(pb->enable_gpio, 0);
> > +
> > pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> > if (IS_ERR(pb->power_supply)) {
> > ret = PTR_ERR(pb->power_supply);
>
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [RESEND PATCH v2] pwm-backlight: Avoid backlight flicker when probed from DT
From: Philipp Zabel @ 2015-10-14 13:25 UTC (permalink / raw)
To: linux-arm-kernel
If the driver is probed from the device tree, and there is a phandle
property set on it, and the enable GPIO is already configured as output,
and the backlight is currently disabled, keep it disabled.
If all these conditions are met, assume there will be some other driver
that can enable the backlight at the appropriate time.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Also check if the regulator is enabled. If the power supply is disabled,
and a phandle points to it, the backlight should stay powered down.
---
drivers/video/backlight/pwm_bl.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..31afd6d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
struct backlight_properties props;
struct backlight_device *bl;
struct pwm_bl_data *pb;
+ int initial_blank = FB_BLANK_UNBLANK;
+ bool phandle;
int ret;
if (!data) {
@@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}
+ phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
+
+ if (pb->enable_gpio) {
+ /*
+ * If the driver is probed from the device tree and there is a
+ * phandle link pointing to the backlight node, it is safe to
+ * assume that another driver will enable the backlight at the
+ * appropriate time. Therefore, if it is disabled, keep it so.
+ */
+ if (phandle &&
+ gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
+ gpiod_get_value(pb->enable_gpio) = 0)
+ initial_blank = FB_BLANK_POWERDOWN;
+ else
+ gpiod_direction_output(pb->enable_gpio, 1);
+ }
+
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
goto err_alloc;
}
+ if (phandle && !regulator_is_enabled(pb->power_supply))
+ initial_blank = FB_BLANK_POWERDOWN;
+
pb->pwm = devm_pwm_get(&pdev->dev, NULL);
if (IS_ERR(pb->pwm)) {
ret = PTR_ERR(pb->pwm);
@@ -321,6 +343,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}
bl->props.brightness = data->dft_brightness;
+ bl->props.power = initial_blank;
backlight_update_status(bl);
platform_set_drvdata(pdev, bl);
--
2.6.1
^ permalink raw reply related
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Vladimir Zapolskiy @ 2015-10-15 10:45 UTC (permalink / raw)
To: Lee Jones
Cc: Boris Brezillon, Robert Jarzmik, Nicolas Ferre, Thierry Reding,
Jingoo Han, linux-pwm, linux-fbdev
In-Reply-To: <20151013072941.GR17172@x1>
Hi Lee,
Lee,On 13.10.2015 10:29, Lee Jones wrote:
> On Mon, 12 Oct 2015, Vladimir Zapolskiy wrote:
>
>> On 12.10.2015 18:19, Boris Brezillon wrote:
>>> On Mon, 12 Oct 2015 17:19:35 +0300
>>> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:
>>>>
>>>>> Thierry's patch makes sure that EPROBE_DEFER is not returned when the
>>>>> PWM device definition is not found using in the PWM lookup tables or
>>>>> the DT definition,
>>>>
>>>> This is okay, but I'm interested in proper handling of cases other than
>>>> EPROBE_DEFER. EPROBE_DEFER and the related issues are on your balance
>>>> and I'm attempting to avoid interfering with it here :)
>>>
>>> I keep thinking we should fix all platforms using the ->pwm_id pdata
>>> field to attach a PWM device to a PWM backlight instead of trying to
>>> guess when falling back to the legacy API is acceptable...
>>>
>>>>
>>>>> and in this case the pwm_bl code will fallback to
>>>>> the legacy PWM API, which AFAICT is what you're trying to solve.
>>>>
>>>> Fallback must happen exclusively under (IS_ERR(pb->pwm) &&
>>>> PTR_ERR(pb->pwm) != -EPROBE_DEFER && !pdev->dev.of_node) condition IMHO.
>>>>
>>>> Before EPROBE_DEFER appeared on the scene the condition was
>>>> (IS_ERR(pb->pwm) && !pdev->dev.of_node).
>>>>
>>>> So, the question is if my change requires any updates or not from your
>>>> point of view.
>>>
>>> ... but from a functional point of view your patch seems correct.
>>
>> Sounds good, thank you for review.
>
> So should I take this patch, or not?
>
Robert's testing shows no regression, please apply this change on top of
Nicolas' one.
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Lee Jones @ 2015-10-15 11:03 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Thierry Reding, Jingoo Han, Nicolas Ferre, linux-pwm, linux-fbdev
In-Reply-To: <1444652943-19712-1-git-send-email-vladimir_zapolskiy@mentor.com>
On Mon, 12 Oct 2015, Vladimir Zapolskiy wrote:
> Platform PWM backlight data provided by board's device tree should be
> complete enough to successfully request a pwm device using pwm_get()
> API. This change fixes a bug, when an arbitrary (first found) PWM is
> connected to a "pwm-backlight" compatible device, when explicit PWM
> device reference is not given.
>
> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> already describes "pwms" as a required property, instead of blind
> selection of a potentially wrong PWM reject legacy PWM device
> registration request, leave legacy API only for non-dt cases.
>
> Based on initial implementation done by Dmitry Eremin-Solenikov.
>
> Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> The change is based on lee-backlight/for-backlight-next
>
> Changes from v1 to v2:
> * rebased on top of Nicolas' commit
> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
>
> Links to previous discussions of the change:
> * https://patchwork.ozlabs.org/patch/483993/
> * https://patchwork.ozlabs.org/patch/398849/
>
> drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
Applied, thanks.
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index eff379b..ae3c6b6 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> - if (IS_ERR(pb->pwm)) {
> - ret = PTR_ERR(pb->pwm);
> - if (ret = -EPROBE_DEFER)
> - goto err_alloc;
> -
> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
> + && !pdev->dev.of_node) {
> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> pb->legacy = true;
> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> - if (IS_ERR(pb->pwm)) {
> - dev_err(&pdev->dev, "unable to request legacy PWM\n");
> - ret = PTR_ERR(pb->pwm);
> - goto err_alloc;
> - }
> + }
> +
> + if (IS_ERR(pb->pwm)) {
> + ret = PTR_ERR(pb->pwm);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "unable to request PWM\n");
> + goto err_alloc;
> }
>
> dev_dbg(&pdev->dev, "got pwm for backlight\n");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [GIT PULL] On-demand device probing
From: Tomeu Vizoso @ 2015-10-15 11:42 UTC (permalink / raw)
To: Rob Herring
Cc: Russell King, Greg Kroah-Hartman, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Alexandre Courbot,
Thierry Reding, David Airlie, Terje Bergström,
Stephen Warren, Wolfram Sang, Frank Rowand, Grant Likely,
Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse, Liam Girdwood,
Mark Brown, Felipe Balbi
In-Reply-To: <20151014092644.GO14956@sirena.org.uk>
Hi,
this second pull request replaces the last references to device_initcall_sync with late_initcall, as noticed by Frank Rowand.
Also fixes the url of the git repo and the wrapping, as suggested by Mark Brown.
Thanks,
Tomeu
The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f:
Linux 4.3-rc1 (2015-09-12 16:35:56 -0700)
are available in the git repository at:
git://git.collabora.com/git/user/tomeu/linux.git on-demand-probes-for-next
for you to fetch changes up to c074fef5d36e1c27dfdf7474e23c01a1b044ff98:
of/platform: Defer probes of registered devices (2015-10-15 13:25:47 +0200)
----------------------------------------------------------------
Tomeu Vizoso (20):
driver core: handle -EPROBE_DEFER from bus_type.match()
ARM: amba: Move reading of periphid to amba_match()
of/platform: Point to struct device from device node
of: add function to allow probing a device from a OF node
gpio: Probe GPIO drivers on demand
pinctrl: Probe pinctrl devices on demand
regulator: core: Probe regulators on demand
drm: Probe panels on demand
drm/tegra: Probe dpaux devices on demand
i2c: core: Probe i2c adapters and devices on demand
pwm: Probe PWM chip devices on demand
backlight: Probe backlight devices on demand
usb: phy: Probe phy devices on demand
clk: Probe clk providers on demand
pinctrl: Probe pinctrl devices on demand
phy: core: Probe phy providers on demand
dma: of: Probe DMA controllers on demand
power-supply: Probe power supplies on demand
driver core: Allow deferring probes until late init
of/platform: Defer probes of registered devices
drivers/amba/bus.c | 88 +++++++++++++++++++------------------
drivers/base/Kconfig | 18 ++++++++
drivers/base/dd.c | 30 ++++++++++++-
drivers/clk/clk.c | 3 ++
drivers/dma/of-dma.c | 3 ++
drivers/gpio/gpiolib-of.c | 5 +++
drivers/gpu/drm/drm_panel.c | 3 ++
drivers/gpu/drm/tegra/dpaux.c | 3 ++
drivers/i2c/i2c-core.c | 4 ++
drivers/of/device.c | 61 +++++++++++++++++++++++++
drivers/of/platform.c | 30 ++++++++-----
drivers/phy/phy-core.c | 3 ++
drivers/pinctrl/devicetree.c | 3 ++
drivers/power/power_supply_core.c | 3 ++
drivers/pwm/core.c | 3 ++
drivers/regulator/core.c | 2 +
drivers/usb/phy/phy.c | 3 ++
drivers/video/backlight/backlight.c | 3 ++
include/linux/device.h | 4 +-
include/linux/of.h | 1 +
include/linux/of_device.h | 3 ++
21 files changed, 219 insertions(+), 57 deletions(-)
^ permalink raw reply
* Re: [RESEND PATCH v2] pwm-backlight: Avoid backlight flicker when probed from DT
From: Christian Gmeiner @ 2015-10-15 11:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1444829119-4266-1-git-send-email-p.zabel@pengutronix.de>
2015-10-14 15:25 GMT+02:00 Philipp Zabel <p.zabel@pengutronix.de>:
> If the driver is probed from the device tree, and there is a phandle
> property set on it, and the enable GPIO is already configured as output,
> and the backlight is currently disabled, keep it disabled.
> If all these conditions are met, assume there will be some other driver
> that can enable the backlight at the appropriate time.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
> - Also check if the regulator is enabled. If the power supply is disabled,
> and a phandle points to it, the backlight should stay powered down.
> ---
> drivers/video/backlight/pwm_bl.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index eff379b..31afd6d 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> struct backlight_properties props;
> struct backlight_device *bl;
> struct pwm_bl_data *pb;
> + int initial_blank = FB_BLANK_UNBLANK;
> + bool phandle;
> int ret;
>
> if (!data) {
> @@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> + phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
> +
> + if (pb->enable_gpio) {
> + /*
> + * If the driver is probed from the device tree and there is a
> + * phandle link pointing to the backlight node, it is safe to
> + * assume that another driver will enable the backlight at the
> + * appropriate time. Therefore, if it is disabled, keep it so.
> + */
> + if (phandle &&
> + gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
> + gpiod_get_value(pb->enable_gpio) = 0)
> + initial_blank = FB_BLANK_POWERDOWN;
> + else
> + gpiod_direction_output(pb->enable_gpio, 1);
> + }
> +
> pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> if (IS_ERR(pb->power_supply)) {
> ret = PTR_ERR(pb->power_supply);
> goto err_alloc;
> }
>
> + if (phandle && !regulator_is_enabled(pb->power_supply))
> + initial_blank = FB_BLANK_POWERDOWN;
> +
> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> if (IS_ERR(pb->pwm)) {
> ret = PTR_ERR(pb->pwm);
> @@ -321,6 +343,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> bl->props.brightness = data->dft_brightness;
> + bl->props.power = initial_blank;
> backlight_update_status(bl);
>
> platform_set_drvdata(pdev, bl);
> --
> 2.6.1
>
Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
^ permalink raw reply
* Re: [PATCHv4 3/3] devicetree: Add led-backlight binding
From: Tomi Valkeinen @ 2015-10-15 12:17 UTC (permalink / raw)
To: Rob Herring
Cc: Jacek Anaszewski, Jingoo Han, Lee Jones, Linux LED Subsystem,
linux-fbdev@vger.kernel.org, Andrew Lunn,
devicetree@vger.kernel.org
In-Reply-To: <CAL_JsqJ+xYpX86DNHFCOEqV85FRh-Xzh1wg3p6guA0QeWG4+pg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2402 bytes --]
Hi Rob,
On 13/10/15 17:21, Rob Herring wrote:
> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Add DT binding for led-backlight.
>
> Please use get_maintainers.pl.
At some point I got feedback that the DT maintainers don't have time to
look at each individual driver binding, but rely on the subsystem
maintainers to handle them. Maybe I misunderstood that.
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/video/backlight/led-backlight.txt | 30 ++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> new file mode 100644
>> index 000000000000..d4621d7414bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> @@ -0,0 +1,30 @@
>> +led-backlight bindings
>> +
>> +Required properties:
>> + - compatible: "led-backlight"
>> + - leds: phandle to a led OF node [0]
>
> Why do we need 2 levels of LED nodes?
Sorry, didn't get that. What do you mean with 2 levels?
>> + - brightness-levels: Array of distinct LED brightness levels. These
>> + are in the range from 0 to 255, passed to the LED class driver.
>> + - default-brightness-level: the default brightness level (index into the
>> + array defined by the "brightness-levels" property)
>> +
>> +Optional properties:
>> + - power-supply: regulator for supply voltage
>> + - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>> + and disables the backlight (see GPIO binding[1])
>
> Why are all of these not part of the LED node pointed to by leds?
These are for the backlight, not for the LED chip. So "LED" here is a
chip that produces (most likely) a PWM signal, and "backlight" is the
collection of components that use the PWM to produce the backlight
itself, and use the power-supply and gpios.
> Describe the h/w, not what you want for a driver.
I think this describes the HW quite well. The LED chip works fine
without any of the properties here, and these are specific to the
backlight part of the board.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] OMAPDSS: DISPC: Remove boolean comparisons
From: Luis de Bethencourt @ 2015-10-15 12:29 UTC (permalink / raw)
To: linux-kernel
Cc: tomi.valkeinen, plagnioj, laurent.pinchart, linux-omap,
linux-fbdev, Luis de Bethencourt
Boolean tests do not need explicit comparison to true or false.
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
drivers/video/fbdev/omap2/dss/dispc-compat.c | 6 +++---
drivers/video/fbdev/omap2/dss/dispc.c | 6 +++---
drivers/video/fbdev/omap2/dss/manager.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/dispc-compat.c b/drivers/video/fbdev/omap2/dss/dispc-compat.c
index 633c461..0918b3b 100644
--- a/drivers/video/fbdev/omap2/dss/dispc-compat.c
+++ b/drivers/video/fbdev/omap2/dss/dispc-compat.c
@@ -476,7 +476,7 @@ static void dispc_mgr_disable_lcd_out(enum omap_channel channel)
int r;
u32 irq;
- if (dispc_mgr_is_enabled(channel) = false)
+ if (!dispc_mgr_is_enabled(channel))
return;
/*
@@ -524,7 +524,7 @@ static void dispc_mgr_enable_digit_out(void)
int r;
u32 irq_mask;
- if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) = true)
+ if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT))
return;
/*
@@ -562,7 +562,7 @@ static void dispc_mgr_disable_digit_out(void)
u32 irq_mask;
int num_irqs;
- if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) = false)
+ if (!dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT))
return;
/*
diff --git a/drivers/video/fbdev/omap2/dss/dispc.c b/drivers/video/fbdev/omap2/dss/dispc.c
index be716c9..43b0367 100644
--- a/drivers/video/fbdev/omap2/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/dss/dispc.c
@@ -571,7 +571,7 @@ EXPORT_SYMBOL(dispc_mgr_go_busy);
void dispc_mgr_go(enum omap_channel channel)
{
- WARN_ON(dispc_mgr_is_enabled(channel) = false);
+ WARN_ON(!dispc_mgr_is_enabled(channel));
WARN_ON(dispc_mgr_go_busy(channel));
DSSDBG("GO %s\n", mgr_desc[channel].name);
@@ -3220,7 +3220,7 @@ void dispc_mgr_set_timings(enum omap_channel channel,
DSSDBG("hsync %luHz, vsync %luHz\n", ht, vt);
} else {
- if (t.interlace = true)
+ if (t.interlace)
t.y_res /= 2;
}
@@ -3237,7 +3237,7 @@ static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div,
dispc_write_reg(DISPC_DIVISORo(channel),
FLD_VAL(lck_div, 23, 16) | FLD_VAL(pck_div, 7, 0));
- if (dss_has_feature(FEAT_CORE_CLK_DIV) = false &&
+ if (!dss_has_feature(FEAT_CORE_CLK_DIV) &&
channel = OMAP_DSS_CHANNEL_LCD)
dispc.core_clk_rate = dispc_fclk_rate() / lck_div;
}
diff --git a/drivers/video/fbdev/omap2/dss/manager.c b/drivers/video/fbdev/omap2/dss/manager.c
index 1aac9b4..08a67f4 100644
--- a/drivers/video/fbdev/omap2/dss/manager.c
+++ b/drivers/video/fbdev/omap2/dss/manager.c
@@ -210,7 +210,7 @@ static int dss_mgr_check_lcd_config(struct omap_overlay_manager *mgr,
return -EINVAL;
/* fifohandcheck should be used only with stallmode */
- if (stallmode = false && fifohandcheck = true)
+ if (!stallmode && fifohandcheck)
return -EINVAL;
/*
--
2.5.1
^ permalink raw reply related
* Re: [PATCH] OMAPDSS: DISPC: Remove boolean comparisons
From: Laurent Pinchart @ 2015-10-15 13:10 UTC (permalink / raw)
To: Luis de Bethencourt
Cc: linux-kernel, tomi.valkeinen, plagnioj, linux-omap, linux-fbdev
In-Reply-To: <1444912178-15989-1-git-send-email-luisbg@osg.samsung.com>
Hi Luis,
Thank you for the patch.
On Thursday 15 October 2015 13:29:38 Luis de Bethencourt wrote:
> Boolean tests do not need explicit comparison to true or false.
>
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/fbdev/omap2/dss/dispc-compat.c | 6 +++---
> drivers/video/fbdev/omap2/dss/dispc.c | 6 +++---
> drivers/video/fbdev/omap2/dss/manager.c | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/omap2/dss/dispc-compat.c
> b/drivers/video/fbdev/omap2/dss/dispc-compat.c index 633c461..0918b3b
> 100644
> --- a/drivers/video/fbdev/omap2/dss/dispc-compat.c
> +++ b/drivers/video/fbdev/omap2/dss/dispc-compat.c
> @@ -476,7 +476,7 @@ static void dispc_mgr_disable_lcd_out(enum omap_channel
> channel) int r;
> u32 irq;
>
> - if (dispc_mgr_is_enabled(channel) = false)
> + if (!dispc_mgr_is_enabled(channel))
> return;
>
> /*
> @@ -524,7 +524,7 @@ static void dispc_mgr_enable_digit_out(void)
> int r;
> u32 irq_mask;
>
> - if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) = true)
> + if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT))
> return;
>
> /*
> @@ -562,7 +562,7 @@ static void dispc_mgr_disable_digit_out(void)
> u32 irq_mask;
> int num_irqs;
>
> - if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) = false)
> + if (!dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT))
> return;
>
> /*
> diff --git a/drivers/video/fbdev/omap2/dss/dispc.c
> b/drivers/video/fbdev/omap2/dss/dispc.c index be716c9..43b0367 100644
> --- a/drivers/video/fbdev/omap2/dss/dispc.c
> +++ b/drivers/video/fbdev/omap2/dss/dispc.c
> @@ -571,7 +571,7 @@ EXPORT_SYMBOL(dispc_mgr_go_busy);
>
> void dispc_mgr_go(enum omap_channel channel)
> {
> - WARN_ON(dispc_mgr_is_enabled(channel) = false);
> + WARN_ON(!dispc_mgr_is_enabled(channel));
> WARN_ON(dispc_mgr_go_busy(channel));
>
> DSSDBG("GO %s\n", mgr_desc[channel].name);
> @@ -3220,7 +3220,7 @@ void dispc_mgr_set_timings(enum omap_channel channel,
>
> DSSDBG("hsync %luHz, vsync %luHz\n", ht, vt);
> } else {
> - if (t.interlace = true)
> + if (t.interlace)
> t.y_res /= 2;
> }
>
> @@ -3237,7 +3237,7 @@ static void dispc_mgr_set_lcd_divisor(enum
> omap_channel channel, u16 lck_div, dispc_write_reg(DISPC_DIVISORo(channel),
> FLD_VAL(lck_div, 23, 16) | FLD_VAL(pck_div, 7, 0));
>
> - if (dss_has_feature(FEAT_CORE_CLK_DIV) = false &&
> + if (!dss_has_feature(FEAT_CORE_CLK_DIV) &&
> channel = OMAP_DSS_CHANNEL_LCD)
> dispc.core_clk_rate = dispc_fclk_rate() / lck_div;
> }
> diff --git a/drivers/video/fbdev/omap2/dss/manager.c
> b/drivers/video/fbdev/omap2/dss/manager.c index 1aac9b4..08a67f4 100644
> --- a/drivers/video/fbdev/omap2/dss/manager.c
> +++ b/drivers/video/fbdev/omap2/dss/manager.c
> @@ -210,7 +210,7 @@ static int dss_mgr_check_lcd_config(struct
> omap_overlay_manager *mgr, return -EINVAL;
>
> /* fifohandcheck should be used only with stallmode */
> - if (stallmode = false && fifohandcheck = true)
> + if (!stallmode && fifohandcheck)
> return -EINVAL;
>
> /*
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCHv4 3/3] devicetree: Add led-backlight binding
From: Rob Herring @ 2015-10-15 13:46 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jacek Anaszewski, Jingoo Han, Lee Jones, Linux LED Subsystem,
linux-fbdev@vger.kernel.org, Andrew Lunn,
devicetree@vger.kernel.org
In-Reply-To: <561F9961.30008@ti.com>
On Thu, Oct 15, 2015 at 7:17 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 13/10/15 17:21, Rob Herring wrote:
>> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> Add DT binding for led-backlight.
>>
>> Please use get_maintainers.pl.
>
> At some point I got feedback that the DT maintainers don't have time to
> look at each individual driver binding, but rely on the subsystem
> maintainers to handle them. Maybe I misunderstood that.
True, but that doesn't mean to not copy us. If we didn't want to be
copied, we would update MAINTAINERS.
I wouldn't call this one an individual driver either. This is very
much a generic binding which we do want to review.
>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>> .../bindings/video/backlight/led-backlight.txt | 30 ++++++++++++++++++++++
>>> 1 file changed, 30 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> new file mode 100644
>>> index 000000000000..d4621d7414bc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> @@ -0,0 +1,30 @@
>>> +led-backlight bindings
>>> +
>>> +Required properties:
>>> + - compatible: "led-backlight"
>>> + - leds: phandle to a led OF node [0]
>>
>> Why do we need 2 levels of LED nodes?
>
> Sorry, didn't get that. What do you mean with 2 levels?
You have the node the "leds" phandle points to which is the actual LED
device and then this node which is just backlight properties. And then
presumably another phandle in the panel device to point to the
backlight device.
>>> + - brightness-levels: Array of distinct LED brightness levels. These
>>> + are in the range from 0 to 255, passed to the LED class driver.
>>> + - default-brightness-level: the default brightness level (index into the
>>> + array defined by the "brightness-levels" property)
>>> +
>>> +Optional properties:
>>> + - power-supply: regulator for supply voltage
>>> + - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>> + and disables the backlight (see GPIO binding[1])
>>
>> Why are all of these not part of the LED node pointed to by leds?
>
> These are for the backlight, not for the LED chip. So "LED" here is a
> chip that produces (most likely) a PWM signal, and "backlight" is the
> collection of components that use the PWM to produce the backlight
> itself, and use the power-supply and gpios.
Okay, it wasn't clear that leds points to the LED controller node. The
example made it seem as it was the device. We already have a way to
describe LEDs and that is as child nodes of the LED controller node.
Please follow what was done for flash LEDs (leds/common.txt).
What's wrong with the existing pwm-backlight binding in the PWM case?
>
>> Describe the h/w, not what you want for a driver.
>
> I think this describes the HW quite well. The LED chip works fine
> without any of the properties here, and these are specific to the
> backlight part of the board.
A more complete example would be helpful here.
Rob
^ permalink raw reply
* Re: [PATCHv4 3/3] devicetree: Add led-backlight binding
From: Tomi Valkeinen @ 2015-10-15 14:46 UTC (permalink / raw)
To: Rob Herring
Cc: Jacek Anaszewski, Jingoo Han, Lee Jones, Linux LED Subsystem,
linux-fbdev@vger.kernel.org, Andrew Lunn,
devicetree@vger.kernel.org
In-Reply-To: <CAL_Jsq+trQ3jLczxvtzqfpUKBgxaazPdGyjLg+NT2YFva0_xbA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4272 bytes --]
Hi Rob,
On 15/10/15 16:46, Rob Herring wrote:
>> At some point I got feedback that the DT maintainers don't have time to
>> look at each individual driver binding, but rely on the subsystem
>> maintainers to handle them. Maybe I misunderstood that.
>
> True, but that doesn't mean to not copy us. If we didn't want to be
> copied, we would update MAINTAINERS.
Ok.
>>> Why do we need 2 levels of LED nodes?
>>
>> Sorry, didn't get that. What do you mean with 2 levels?
>
> You have the node the "leds" phandle points to which is the actual LED
> device and then this node which is just backlight properties. And then
> presumably another phandle in the panel device to point to the
> backlight device.
Ok, I see what you mean.
Well, I have to say this is far from perfect. I initially pushed for a
PWM driver for the LED chip we use (tlc591xx), which would have allowed
us to use pwm-backlight driver. But Andrew was using the same chip for
more LED-ish use cases, for which a LED driver was more suitable.
But what I think we really should have is a more generic way to
represent output pins, so that GPIOs (well, GPOs really), PWMs and
current controlled outputs would all be done the same way.
It was rather difficult to use the LED driver and LED bindings for this,
as (afaics) they were really never designed to be used for anything else
than for simple LEDs (i.e. a LED connected directly to the LED chip).
The flash support was added later, but that's almost as simple as the
first case.
>> These are for the backlight, not for the LED chip. So "LED" here is a
>> chip that produces (most likely) a PWM signal, and "backlight" is the
>> collection of components that use the PWM to produce the backlight
>> itself, and use the power-supply and gpios.
>
> Okay, it wasn't clear that leds points to the LED controller node. The
No, it doesn't point to the main LED node (the one having 'compatible').
It points to a child node.
> example made it seem as it was the device. We already have a way to
> describe LEDs and that is as child nodes of the LED controller node.
True, but those child nodes are very limited. As I see it, those child
nodes really describe the outputs of the LED chip, not what's on the
other end of the lines.
If on the other end of the lines is a more complex device, we need a
proper device driver for it, with a proper DT node with compatible
property etc.
Now, one could argue that a "backlight" that gets the LED signal from a
LED chip is really just a simple LED. But there are complications:
- Our board needs a GPIO to enable the backlight. I can't say what
exactly the GPIO does as my HW skills don't go far enough, but all this
is after the LED chip. I also see the circuitry using powers, which in
our case happen to be always on so we don't need to enable them explicitly.
- We need a backlight device/driver (because of the Linux SW stack).
So, maybe it would be possible to construct all that in a LED child
node, and the LED driver would create a child device for the nodes which
have 'compatible' property. But then, that would be very different from
pwm-backlight, and the parent-child relationships are usually used to
indicate a control relationship, right?
The led-backlight in these patches is very much similar to pwm-backlight.
> Please follow what was done for flash LEDs (leds/common.txt).
The flash support is quite simple. I'm not sure how I could do the same
for the backlight, as I described above.
> What's wrong with the existing pwm-backlight binding in the PWM case?
Nothing, if there's a PWM driver. But if the LED chip is modelled as a
LED driver, pwm-backlight is out. I think there are two kinds of LED
chips, PWM ones and current-controlling ones. And then there are the PWM
devices which are clearly PWM ones.
>>> Describe the h/w, not what you want for a driver.
>>
>> I think this describes the HW quite well. The LED chip works fine
>> without any of the properties here, and these are specific to the
>> backlight part of the board.
>
> A more complete example would be helpful here.
Of our HW? I can't give the schematics but I hope I described it enough
above.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv4 3/3] devicetree: Add led-backlight binding
From: Rob Herring @ 2015-10-15 18:55 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jacek Anaszewski, Jingoo Han, Lee Jones, Linux LED Subsystem,
linux-fbdev@vger.kernel.org, Andrew Lunn,
devicetree@vger.kernel.org
In-Reply-To: <561FBC4F.6030306@ti.com>
On Thu, Oct 15, 2015 at 9:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 16:46, Rob Herring wrote:
>
>>> At some point I got feedback that the DT maintainers don't have time to
>>> look at each individual driver binding, but rely on the subsystem
>>> maintainers to handle them. Maybe I misunderstood that.
>>
>> True, but that doesn't mean to not copy us. If we didn't want to be
>> copied, we would update MAINTAINERS.
>
> Ok.
>
>>>> Why do we need 2 levels of LED nodes?
>>>
>>> Sorry, didn't get that. What do you mean with 2 levels?
>>
>> You have the node the "leds" phandle points to which is the actual LED
>> device and then this node which is just backlight properties. And then
>> presumably another phandle in the panel device to point to the
>> backlight device.
>
> Ok, I see what you mean.
>
> Well, I have to say this is far from perfect. I initially pushed for a
> PWM driver for the LED chip we use (tlc591xx), which would have allowed
> us to use pwm-backlight driver. But Andrew was using the same chip for
> more LED-ish use cases, for which a LED driver was more suitable.
>
> But what I think we really should have is a more generic way to
> represent output pins, so that GPIOs (well, GPOs really), PWMs and
> current controlled outputs would all be done the same way.
>
> It was rather difficult to use the LED driver and LED bindings for this,
> as (afaics) they were really never designed to be used for anything else
> than for simple LEDs (i.e. a LED connected directly to the LED chip).
> The flash support was added later, but that's almost as simple as the
> first case.
There's still room to extend it though.
>>> These are for the backlight, not for the LED chip. So "LED" here is a
>>> chip that produces (most likely) a PWM signal, and "backlight" is the
>>> collection of components that use the PWM to produce the backlight
>>> itself, and use the power-supply and gpios.
>>
>> Okay, it wasn't clear that leds points to the LED controller node. The
>
> No, it doesn't point to the main LED node (the one having 'compatible').
> It points to a child node.
>
>> example made it seem as it was the device. We already have a way to
>> describe LEDs and that is as child nodes of the LED controller node.
>
> True, but those child nodes are very limited. As I see it, those child
> nodes really describe the outputs of the LED chip, not what's on the
> other end of the lines.
The child nodes are supposed to be the other end. In the flash case,
the properties are constraints on the flash LED (i.e. different flash
LEDs will have different max currents).
>
> If on the other end of the lines is a more complex device, we need a
> proper device driver for it, with a proper DT node with compatible
> property etc.
>
> Now, one could argue that a "backlight" that gets the LED signal from a
> LED chip is really just a simple LED. But there are complications:
I would say backlights are a complex example of LEDs. Of course, there
are backlights not based on LEDs, but we're not talking about those
here.
> - Our board needs a GPIO to enable the backlight. I can't say what
> exactly the GPIO does as my HW skills don't go far enough, but all this
> is after the LED chip. I also see the circuitry using powers, which in
> our case happen to be always on so we don't need to enable them explicitly.
The GPIO is probably controlling a transistor to connect the LED anode
to ground and therefore turn it on. These have nothing to do with
backlights really, but really are common to LEDs. Every LED needs a
supply rail too. This may come for a regulator or directly from an LED
driver IC.
If the flash LED binding doesn't have these, then it is only a matter of time.
>
> - We need a backlight device/driver (because of the Linux SW stack).
From a binding perspective, not my problem. The problem with the
driver needs driving the binding definition is the drivers can change
over time. IIRC there has been some discussion of combining the 2
subsystems in the kernel, so we don't want to create something defined
by current kernel needs.
> So, maybe it would be possible to construct all that in a LED child
> node, and the LED driver would create a child device for the nodes which
> have 'compatible' property. But then, that would be very different from
> pwm-backlight, and the parent-child relationships are usually used to
> indicate a control relationship, right?
This is along the lines I was thinking, but don't see how it is very
different at the binding level. The parent-child relationship is
typically control path or just what is downstream from the parent
device. Some bindings like GPIO and PWM don't follow this, but that is
often because they are just additional sideband interfaces on top of
the main control interface. I think simply making the "backlight" node
from the pwm-backlight binding a child works. We probably need a
different compatible string though (led-backlight is as good as
anything). I also think we should require child nodes to have a
compatible string which we didn't do for flash devices. It's probably
not too late to fix that.
I think there are 2 cases of PWM connection to LEDs to consider. The
PWM is an input to a LED driver chip or the PWM directly controls the
LED (attached to the anode). The current pwm-backlight binding covers
the latter. In the former case, pwms should probably be in the parent
(LED driver IC node). Of course, if the driver IC has no s/w
controllable interface beyond PWM, then it probably doesn't need to be
modeled at all in DT.
> The led-backlight in these patches is very much similar to pwm-backlight.
Yes, I think we're really only debating the structure of nodes.
>
>> Please follow what was done for flash LEDs (leds/common.txt).
>
> The flash support is quite simple. I'm not sure how I could do the same
> for the backlight, as I described above.
>
>> What's wrong with the existing pwm-backlight binding in the PWM case?
>
> Nothing, if there's a PWM driver. But if the LED chip is modelled as a
> LED driver, pwm-backlight is out. I think there are two kinds of LED
> chips, PWM ones and current-controlling ones. And then there are the PWM
> devices which are clearly PWM ones.
>
>>>> Describe the h/w, not what you want for a driver.
>>>
>>> I think this describes the HW quite well. The LED chip works fine
>>> without any of the properties here, and these are specific to the
>>> backlight part of the board.
>>
>> A more complete example would be helpful here.
>
> Of our HW? I can't give the schematics but I hope I described it enough
> above.
I just meant the relationship of all the nodes involved.
Rob
^ permalink raw reply
* [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: YH Huang @ 2015-10-16 1:37 UTC (permalink / raw)
To: linux-arm-kernel
In order to match the panel power sequence, disable the enable_gpio
in the probe function. Also, reorder the code in the power_on and
power_off function to match the timing.
Signed-off-by: YH Huang <yh.huang@mediatek.com>
---
drivers/video/backlight/pwm_bl.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..99eca1e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
if (err < 0)
dev_err(pb->dev, "failed to enable power supply\n");
+ pwm_enable(pb->pwm);
+
if (pb->enable_gpio)
gpiod_set_value(pb->enable_gpio, 1);
- pwm_enable(pb->pwm);
pb->enabled = true;
}
@@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
if (!pb->enabled)
return;
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
-
if (pb->enable_gpio)
gpiod_set_value(pb->enable_gpio, 0);
+ pwm_config(pb->pwm, 0, pb->period);
+ pwm_disable(pb->pwm);
+
regulator_disable(pb->power_supply);
pb->enabled = false;
}
@@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->dev = &pdev->dev;
pb->enabled = false;
- pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
- GPIOD_OUT_HIGH);
+ pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
if (IS_ERR(pb->enable_gpio)) {
ret = PTR_ERR(pb->enable_gpio);
goto err_alloc;
@@ -264,6 +264,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}
+ if (pb->enable_gpio)
+ gpiod_direction_output(pb->enable_gpio, 0);
+
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
--
1.7.9.5
^ permalink raw reply related
* Re: [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: kbuild test robot @ 2015-10-16 2:42 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1444959454-1516-1-git-send-email-yh.huang@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]
Hi YH,
[auto build test ERROR on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/YH-Huang/pwm-backlight-fix-the-panel-power-sequence/20151016-093957
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/video/backlight/pwm_bl.c: In function 'pwm_backlight_probe':
>> drivers/video/backlight/pwm_bl.c:245:20: error: too few arguments to function 'devm_gpiod_get_optional'
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
^
In file included from drivers/video/backlight/pwm_bl.c:13:0:
include/linux/gpio/consumer.h:80:32: note: declared here
struct gpio_desc *__must_check devm_gpiod_get_optional(struct device *dev,
^
vim +/devm_gpiod_get_optional +245 drivers/video/backlight/pwm_bl.c
239 pb->notify_after = data->notify_after;
240 pb->check_fb = data->check_fb;
241 pb->exit = data->exit;
242 pb->dev = &pdev->dev;
243 pb->enabled = false;
244
> 245 pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
246 if (IS_ERR(pb->enable_gpio)) {
247 ret = PTR_ERR(pb->enable_gpio);
248 goto err_alloc;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 51512 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: Lucas Stach @ 2015-10-16 8:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1444959454-1516-1-git-send-email-yh.huang@mediatek.com>
Am Freitag, den 16.10.2015, 09:37 +0800 schrieb YH Huang:
> In order to match the panel power sequence, disable the enable_gpio
> in the probe function. Also, reorder the code in the power_on and
> power_off function to match the timing.
>
You aren't specifying which panels power sequence you are matching here.
Are you sure you aren't breaking other panels with this patch?
Regards,
Lucas
> Signed-off-by: YH Huang <yh.huang@mediatek.com>
> ---
> drivers/video/backlight/pwm_bl.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index eff379b..99eca1e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> if (err < 0)
> dev_err(pb->dev, "failed to enable power supply\n");
>
> + pwm_enable(pb->pwm);
> +
> if (pb->enable_gpio)
> gpiod_set_value(pb->enable_gpio, 1);
>
> - pwm_enable(pb->pwm);
> pb->enabled = true;
> }
>
> @@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> if (!pb->enabled)
> return;
>
> - pwm_config(pb->pwm, 0, pb->period);
> - pwm_disable(pb->pwm);
> -
> if (pb->enable_gpio)
> gpiod_set_value(pb->enable_gpio, 0);
>
> + pwm_config(pb->pwm, 0, pb->period);
> + pwm_disable(pb->pwm);
> +
> regulator_disable(pb->power_supply);
> pb->enabled = false;
> }
> @@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->dev = &pdev->dev;
> pb->enabled = false;
>
> - pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> - GPIOD_OUT_HIGH);
> + pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
> if (IS_ERR(pb->enable_gpio)) {
> ret = PTR_ERR(pb->enable_gpio);
> goto err_alloc;
> @@ -264,6 +264,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> + if (pb->enable_gpio)
> + gpiod_direction_output(pb->enable_gpio, 0);
> +
> pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> if (IS_ERR(pb->power_supply)) {
> ret = PTR_ERR(pb->power_supply);
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: Sascha Hauer @ 2015-10-16 8:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1444959454-1516-1-git-send-email-yh.huang@mediatek.com>
On Fri, Oct 16, 2015 at 09:37:34AM +0800, YH Huang wrote:
> In order to match the panel power sequence, disable the enable_gpio
> in the probe function. Also, reorder the code in the power_on and
> power_off function to match the timing.
> @@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->dev = &pdev->dev;
> pb->enabled = false;
>
> - pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> - GPIOD_OUT_HIGH);
> + pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
Please actually test your patches. This change here won't compile.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: YH Huang @ 2015-10-16 8:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201510161036.Q5q8ayZx%fengguang.wu@intel.com>
On Fri, 2015-10-16 at 10:42 +0800, kbuild test robot wrote:
> Hi YH,
>
> [auto build test ERROR on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
>
> url: https://github.com/0day-ci/linux/commits/YH-Huang/pwm-backlight-fix-the-panel-power-sequence/20151016-093957
> config: i386-allmodconfig (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
> drivers/video/backlight/pwm_bl.c: In function 'pwm_backlight_probe':
> >> drivers/video/backlight/pwm_bl.c:245:20: error: too few arguments to function 'devm_gpiod_get_optional'
> pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
> ^
> In file included from drivers/video/backlight/pwm_bl.c:13:0:
> include/linux/gpio/consumer.h:80:32: note: declared here
> struct gpio_desc *__must_check devm_gpiod_get_optional(struct device *dev,
> ^
>
> vim +/devm_gpiod_get_optional +245 drivers/video/backlight/pwm_bl.c
>
> 239 pb->notify_after = data->notify_after;
> 240 pb->check_fb = data->check_fb;
> 241 pb->exit = data->exit;
> 242 pb->dev = &pdev->dev;
> 243 pb->enabled = false;
> 244
> > 245 pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
> 246 if (IS_ERR(pb->enable_gpio)) {
> 247 ret = PTR_ERR(pb->enable_gpio);
> 248 goto err_alloc;
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
Sorry, I made a mistake.
I will send patch v2 to fix it.
YH Huang
^ permalink raw reply
* Re: [RESEND PATCH] pwm-backlight: fix the panel power sequence
From: YH Huang @ 2015-10-16 8:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20151016083616.GA7858@pengutronix.de>
On Fri, 2015-10-16 at 10:36 +0200, Sascha Hauer wrote:
> On Fri, Oct 16, 2015 at 09:37:34AM +0800, YH Huang wrote:
> > In order to match the panel power sequence, disable the enable_gpio
> > in the probe function. Also, reorder the code in the power_on and
> > power_off function to match the timing.
> > @@ -241,8 +242,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > pb->dev = &pdev->dev;
> > pb->enabled = false;
> >
> > - pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> > - GPIOD_OUT_HIGH);
> > + pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
>
> Please actually test your patches. This change here won't compile.
>
> Sascha
>
I will send patch v2 to fix it.
YH Huang
^ permalink raw reply
* [PATCH v2] pwm-backlight: fix the panel power sequence
From: YH Huang @ 2015-10-16 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In order to match the panel power sequence, disable the enable_gpio
in the probe function. Also, reorder the code in the power_on and
power_off function to match the timing.
Signed-off-by: YH Huang <yh.huang@mediatek.com>
---
Change in v2:
Fix the build error.
---
drivers/video/backlight/pwm_bl.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..d8c9c62 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
if (err < 0)
dev_err(pb->dev, "failed to enable power supply\n");
+ pwm_enable(pb->pwm);
+
if (pb->enable_gpio)
gpiod_set_value(pb->enable_gpio, 1);
- pwm_enable(pb->pwm);
pb->enabled = true;
}
@@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
if (!pb->enabled)
return;
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
-
if (pb->enable_gpio)
gpiod_set_value(pb->enable_gpio, 0);
+ pwm_config(pb->pwm, 0, pb->period);
+ pwm_disable(pb->pwm);
+
regulator_disable(pb->power_supply);
pb->enabled = false;
}
@@ -242,7 +243,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enabled = false;
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
- GPIOD_OUT_HIGH);
+ GPIOD_ASIS);
if (IS_ERR(pb->enable_gpio)) {
ret = PTR_ERR(pb->enable_gpio);
goto err_alloc;
@@ -264,6 +265,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}
+ if (pb->enable_gpio)
+ gpiod_direction_output(pb->enable_gpio, 0);
+
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2] pwm-backlight: fix the panel power sequence
From: Philipp Zabel @ 2015-10-16 9:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1444987060-48202-1-git-send-email-yh.huang@mediatek.com>
Am Freitag, den 16.10.2015, 17:17 +0800 schrieb YH Huang:
> In order to match the panel power sequence, disable the enable_gpio
> in the probe function. Also, reorder the code in the power_on and
> power_off function to match the timing.
Could you also have a look at the "pwm-backlight: Avoid backlight
flicker when probed from DT" patch?
I think in case of a panel left enabled by the bootloader, your patch
will disable the backlight for a short time.
best regards
Philipp
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox