* [PATCHv2 1/3] leds: Add of_led_get() and led_put()
From: Tomi Valkeinen @ 2015-09-08 11:19 UTC (permalink / raw)
To: Jacek Anaszewski, Jingoo Han, Lee Jones, linux-leds, linux-fbdev
Cc: Andrew Lunn, Tomi Valkeinen
In-Reply-To: <1441711176-4258-1-git-send-email-tomi.valkeinen@ti.com>
This patch adds basic support for a kernel driver to get a LED device.
This will be used by the led-backlight driver.
Only OF version is implemented for now, and the behavior is similar to
PWM's of_pwm_get() and pwm_put().
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/leds/Makefile | 6 +++-
drivers/leds/led-class.c | 13 +++++++-
drivers/leds/led-of.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/leds/leds.h | 1 +
include/linux/leds-of.h | 26 +++++++++++++++
include/linux/leds.h | 2 ++
6 files changed, 128 insertions(+), 2 deletions(-)
create mode 100644 drivers/leds/led-of.c
create mode 100644 include/linux/leds-of.h
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a2f513..6fd22e411810 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,7 +1,11 @@
# LED Core
obj-$(CONFIG_NEW_LEDS) += led-core.o
-obj-$(CONFIG_LEDS_CLASS) += led-class.o
+
+obj-$(CONFIG_LEDS_CLASS) += led-class-objs.o
+led-class-objs-y := led-class.o
+led-class-objs-$(CONFIG_OF) += led-of.o
+
obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index beabfbc6f7cd..1234f9dc3537 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -22,7 +22,7 @@
#include <linux/timer.h>
#include "leds.h"
-static struct class *leds_class;
+struct class *leds_class;
static ssize_t brightness_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -216,6 +216,17 @@ static int led_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
+/**
+ * led_put() - release a LED device, reserved with led_get()
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+ put_device(led_cdev->dev);
+ module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);
+
static int match_name(struct device *dev, const void *data)
{
if (!dev_name(dev))
diff --git a/drivers/leds/led-of.c b/drivers/leds/led-of.c
new file mode 100644
index 000000000000..32631682be07
--- /dev/null
+++ b/drivers/leds/led-of.c
@@ -0,0 +1,82 @@
+/*
+ * LED Class Core OF support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/leds.h>
+#include <linux/of.h>
+#include <linux/leds-of.h>
+#include <linux/module.h>
+
+#include "leds.h"
+
+/* find OF node for the given led_cdev */
+static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
+{
+ struct device *led_dev = led_cdev->dev;
+ struct device_node *child;
+
+ for_each_child_of_node(led_dev->parent->of_node, child) {
+ if (of_property_match_string(child, "label", led_cdev->name) = 0)
+ return child;
+ }
+
+ return NULL;
+}
+
+static int led_match_led_node(struct device *led_dev, const void *data)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
+ const struct device_node *target_node = data;
+ struct device_node *led_node;
+
+ led_node = find_led_of_node(led_cdev);
+ if (!led_node)
+ return 0;
+
+ of_node_put(led_node);
+
+ return led_node = target_node ? 1 : 0;
+}
+
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ *
+ * The caller must use led_put() to release the device after use.
+ */
+struct led_classdev *of_led_get(struct device_node *np)
+{
+ struct device *led_dev;
+ struct led_classdev *led_cdev;
+ struct device_node *led_node;
+
+ led_node = of_parse_phandle(np, "leds", 0);
+ if (!led_node)
+ return ERR_PTR(-ENODEV);
+
+ led_dev = class_find_device(leds_class, NULL, led_node,
+ led_match_led_node);
+
+ of_node_put(led_node);
+
+ if (!led_dev) {
+ pr_err("failed to find led device for node %s, deferring probe\n",
+ of_node_full_name(led_node));
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ led_cdev = dev_get_drvdata(led_dev);
+
+ if (!try_module_get(led_cdev->dev->parent->driver->owner))
+ return ERR_PTR(-ENODEV);
+
+ return led_cdev;
+}
+EXPORT_SYMBOL_GPL(of_led_get);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bc89d7ace2c4..ccc3abb417d4 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -46,6 +46,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
void led_stop_software_blink(struct led_classdev *led_cdev);
+extern struct class *leds_class;
extern struct rw_semaphore leds_list_lock;
extern struct list_head leds_list;
diff --git a/include/linux/leds-of.h b/include/linux/leds-of.h
new file mode 100644
index 000000000000..7e8e64bd9811
--- /dev/null
+++ b/include/linux/leds-of.h
@@ -0,0 +1,26 @@
+/*
+ * OF support for leds
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_LEDS_OF_H_INCLUDED
+#define __LINUX_LEDS_OF_H_INCLUDED
+
+#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_LEDS_CLASS)
+
+extern struct led_classdev *of_led_get(struct device_node *np);
+
+#else
+
+static inline struct led_classdev *of_led_get(struct device_node *np)
+{
+ return -ENODEV;
+}
+
+#endif
+
+#endif /* __LINUX_LEDS_OF_H_INCLUDED */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..0fce71a06d68 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -113,6 +113,8 @@ extern void devm_led_classdev_unregister(struct device *parent,
extern void led_classdev_suspend(struct led_classdev *led_cdev);
extern void led_classdev_resume(struct led_classdev *led_cdev);
+extern void led_put(struct led_classdev *led_cdev);
+
/**
* led_blink_set - set blinking with software fallback
* @led_cdev: the LED to start blinking
--
2.1.4
^ permalink raw reply related
* [PATCHv2 0/3] backlight: led-backlight driver
From: Tomi Valkeinen @ 2015-09-08 11:19 UTC (permalink / raw)
To: Jacek Anaszewski, Jingoo Han, Lee Jones, linux-leds, linux-fbdev
Cc: Andrew Lunn, Tomi Valkeinen
This series aims to add a led-backlight driver, similar to pwm-backlight, but
using a LED class device underneath.
LED framework has no support for DT or getting a LED class driver from another
kernel driver, so I added minimal functionality to led-class to get
led-backlight working.
Changes to v1:
- Split LED OF parts into separate .h and .c files
- Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
out.
- Improved error prints and comments a bit
- Added put_device() into led_put(), as the device was gotten from
class_find_device() which requires a put_device() call.
Tomi
Tomi Valkeinen (3):
leds: Add of_led_get() and led_put()
backlight: add led-backlight driver
devicetree: Add led-backlight binding
.../bindings/video/backlight/led-backlight.txt | 30 +++
drivers/leds/Makefile | 6 +-
drivers/leds/led-class.c | 13 +-
drivers/leds/led-of.c | 82 +++++++
drivers/leds/leds.h | 1 +
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/led_bl.c | 235 +++++++++++++++++++++
include/linux/leds-of.h | 26 +++
include/linux/leds.h | 2 +
10 files changed, 401 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
create mode 100644 drivers/leds/led-of.c
create mode 100644 drivers/video/backlight/led_bl.c
create mode 100644 include/linux/leds-of.h
--
2.1.4
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Jacek Anaszewski @ 2015-09-08 10:57 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55EEBB3F.4040908@ti.com>
On 09/08/2015 12:41 PM, Tomi Valkeinen wrote:
>
> On 08/09/15 12:21, Jacek Anaszewski wrote:
>
>>> The "static struct class *leds_class" from led-class.c, in one way or
>>> another. of_led_get() needs to go through the led devices from the class.
>>>
>>> For now I just removed the "static" from it, so that I can use it from
>>> of.c.
>>
>> I think we can go in this direction. I've skimmed through existing
>> class drivers and found similar examples (e.g. tty_class, rtc_class).
>
> Yep.
>
>>> Sorry, I didn't get that one. How does the backlight driver's
>>> depend/select affect this?
>>
>> OK, I confused something here. Backlight driver should depend on
>> LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
>> It should also depend on OF. In case of this driver the no-ops would be
>> only for the purpose of making the kernel image smaller, as the driver
>> probe will fail without them anyway. Nevertheless, there might be added
>> other drivers in the future, using of_led_get{put} API which would like
>> to make some decisions basing on whether LEDS_CLASS or/and OF are
>> turned on. No-ops would be of use then.
>
> I agree.
>
>>> What do you mean with "waste"?
>>
>> I used 'waste' because we would be wasting time here for the call which
>> can be avoided at no cost. Of course if compiler will decide to inline
>> it.
>>
>>> In this case there's no need to get more performance by inlining
>>
>> Why so?
>
> Reserving and freeing resources are rarely hot paths. The functions in
> question are usually called in a driver's probe and remove. Saving a few
> CPU cycles there doesn't really matter, so I think readability is the
> important part here.
OK, I agree.
>>> and inlining forces the users of led_put to include module.h to compile.
>>
>> We could include module.h from leds.h.
>
> Yes we can. And I can do that in my patch. I don't agree with the
> solution but neither do I really have a problem with it =).
Please go ahead as you originally planned, i.e. without inlining.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Tomi Valkeinen @ 2015-09-08 10:41 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55EEA8B2.3040808@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
On 08/09/15 12:21, Jacek Anaszewski wrote:
>> The "static struct class *leds_class" from led-class.c, in one way or
>> another. of_led_get() needs to go through the led devices from the class.
>>
>> For now I just removed the "static" from it, so that I can use it from
>> of.c.
>
> I think we can go in this direction. I've skimmed through existing
> class drivers and found similar examples (e.g. tty_class, rtc_class).
Yep.
>> Sorry, I didn't get that one. How does the backlight driver's
>> depend/select affect this?
>
> OK, I confused something here. Backlight driver should depend on
> LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
> It should also depend on OF. In case of this driver the no-ops would be
> only for the purpose of making the kernel image smaller, as the driver
> probe will fail without them anyway. Nevertheless, there might be added
> other drivers in the future, using of_led_get{put} API which would like
> to make some decisions basing on whether LEDS_CLASS or/and OF are
> turned on. No-ops would be of use then.
I agree.
>> What do you mean with "waste"?
>
> I used 'waste' because we would be wasting time here for the call which
> can be avoided at no cost. Of course if compiler will decide to inline
> it.
>
>> In this case there's no need to get more performance by inlining
>
> Why so?
Reserving and freeing resources are rarely hot paths. The functions in
question are usually called in a driver's probe and remove. Saving a few
CPU cycles there doesn't really matter, so I think readability is the
important part here.
>> and inlining forces the users of led_put to include module.h to compile.
>
> We could include module.h from leds.h.
Yes we can. And I can do that in my patch. I don't agree with the
solution but neither do I really have a problem with it =).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Jacek Anaszewski @ 2015-09-08 9:21 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55EE89CF.8060308@ti.com>
On 09/08/2015 09:10 AM, Tomi Valkeinen wrote:
>
> On 07/09/15 17:11, Jacek Anaszewski wrote:
>
>>>> Thanks for the patch. Generally, I'd prefer to add files
>>>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>>>> there. Those functions' names should begin with "of_". Please provide
>>>
>>> Ok, I'll do that. I do need to export something from led-class in that
>>> case, so that the of.c gets hold of 'leds_class' pointer, either
>>> directly or indirectly.
>>
>> What exactly do you need to export?
>
> The "static struct class *leds_class" from led-class.c, in one way or
> another. of_led_get() needs to go through the led devices from the class.
>
> For now I just removed the "static" from it, so that I can use it from of.c.
I think we can go in this direction. I've skimmed through existing
class drivers and found similar examples (e.g. tty_class, rtc_class).
>>>> also no-op versions of the functions to address configurations when
>>>> CONFIG_OF isn't enabled. I have also few comments below.
>>>
>>> Yep. No-ops for the purpose of making the kernel image smaller?
>>
>> No, for the case when support for LED subsystem is disabled in the
>> kernel config. I'd rather backlight driver depended on LED subsystem
>> than selected it.
>
> Sorry, I didn't get that one. How does the backlight driver's
> depend/select affect this?
OK, I confused something here. Backlight driver should depend on
LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
It should also depend on OF. In case of this driver the no-ops would be
only for the purpose of making the kernel image smaller, as the driver
probe will fail without them anyway. Nevertheless, there might be added
other drivers in the future, using of_led_get{put} API which would like
to make some decisions basing on whether LEDS_CLASS or/and OF are
turned on. No-ops would be of use then.
>>>> Please move it to include/linux/leds.h, make static inline and provide
>>>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>>>
>>> Ok. Why do you want it as static inline in the leds.h?
>>
>> This function contains only one instruction, why should we waste
>> a function call for it?
>
> I like to keep code in .c files and leave .h files for declarations, and
> only in special cases have inline code in .h files.
>
> What do you mean with "waste"?
I used 'waste' because we would be wasting time here for the call which
can be avoided at no cost. Of course if compiler will decide to inline
it.
> In this case there's no need to get more performance by inlining
Why so?
> (which
> anyway may not help and needs to be studied separately for every case),
There is the tradeoff. Readability vs performance.
> with inlining the resulting kernel image is larger,
That's why we inline only small functions.
> and inlining forces the users of led_put to include module.h to compile.
We could include module.h from leds.h.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v4 0/22] On-demand device probing
From: Tomeu Vizoso @ 2015-09-08 7:30 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel@vger.kernel.org, Rob Herring, Stephen Warren,
Javier Martinez Canillas, Mark Brown, Thierry Reding,
Rafael J. Wysocki, linux-arm-kernel@lists.infradead.org,
Dmitry Torokhov, devicetree@vger.kernel.org, Linus Walleij,
linux-acpi@vger.kernel.org, Arnd Bergmann,
linux-fbdev@vger.kernel.org, Linux USB List, Felipe Balbi,
Linux PWM List, Terje Bergström
In-Reply-To: <CAL_Jsq+E31JaPcp5ELS_ztSqgSrK7U4OzB9xNcNz01Ayrrm+Yg@mail.gmail.com>
On 7 September 2015 at 22:50, Rob Herring <robherring2@gmail.com> wrote:
> On Mon, Sep 7, 2015 at 7:23 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> Hello,
>>
>> I have a problem with the panel on my Tegra Chromebook taking longer
>> than expected to be ready during boot (Stéphane Marchesin reported what
>> is basically the same issue in [0]), and have looked into ordered
>> probing as a better way of solving this than moving nodes around in the
>> DT or playing with initcall levels and linking order.
>>
>> While reading the thread [1] that Alexander Holler started with his
>> series to make probing order deterministic, it occurred to me that it
>> should be possible to achieve the same by probing devices as they are
>> referenced by other devices.
>>
>> This basically reuses the information that is already implicit in the
>> probe() implementations, saving us from refactoring existing drivers or
>> adding information to DTBs.
>>
>> During review of v1 of this series Linus Walleij suggested that it
>> should be the device driver core to make sure that dependencies are
>> ready before probing a device. I gave this idea a try [2] but Mark Brown
>> pointed out to the logic duplication between the resource acquisition
>> and dependency discovery code paths (though I think it's fairly minor).
>>
>> To address that code duplication I experimented with Arnd's devm_probe
>> [3] concept of having drivers declare their dependencies instead of
>> acquiring them during probe, and while it worked [4], I don't think we
>> end up winning anything when compared to just probing devices on-demand
>> from resource getters.
>>
>> One remaining objection is to the "sprinkling" of calls to
>> of_device_probe() in the resource getters of each subsystem, but I think
>> it's the right thing to do given that the storage of resources is
>> currently subsystem-specific.
>>
>> We could avoid the above by moving resource storage into the core, but I
>> don't think there's a compelling case for that.
>>
>> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
>> OMAP SoCs, and these patches were enough to eliminate all the deferred
>> probes (except one in PandaBoard because omap_dma_system doesn't have a
>> firmware node as of yet).
>>
>> Have submitted a branch [5] with only these patches on top of thursday's
>> linux-next to kernelci.org and I don't see any issues that could be
>> caused by them. For some reason it currently has more passes than the
>> version of -next it's based on!
>>
>> With this series I get the kernel to output to the panel in 0.5s,
>> instead of 2.8s.
>>
>> Regards,
>>
>> Tomeu
>>
>> [0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
>>
>> [1] https://lkml.org/lkml/2014/5/12/452
>>
>> [2] https://lkml.org/lkml/2015/6/17/305
>>
>> [3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
>>
>> [4] https://lkml.org/lkml/2015/7/21/441a
>>
>> [5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v6
>>
>> [6] http://kernelci.org/boot/all/job/collabora/kernel/v4.2-11902-g25d80c927f8b/
>>
>> [7] http://kernelci.org/boot/all/job/next/kernel/next-20150903/
>>
>> Changes in v4:
>> - Added bus.pre_probe callback so the probes of Primecell devices can be
>> deferred if their device IDs cannot be yet read because of the clock
>> driver not having probed when they are registered. Maybe this goes
>> overboard and the matching information should be in the DT if there is
>> one.
>
> Seems overboard to me or at least a separate problem.
It's a separate problem but this was preventing the series from
working on a few boards.
> Most clocks have
> to be setup before the driver model simply because timers depend on
> clocks usually.
Yes, but in this case the apb clocks for the primecell devices are
implemented in a normal platform driver (vexpress_osc_driver), instead
of using CLK_OF_DECLARE.
Regards,
Tomeu
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Tomi Valkeinen @ 2015-09-08 7:10 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55ED9B01.5030003@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]
On 07/09/15 17:11, Jacek Anaszewski wrote:
>>> Thanks for the patch. Generally, I'd prefer to add files
>>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>>> there. Those functions' names should begin with "of_". Please provide
>>
>> Ok, I'll do that. I do need to export something from led-class in that
>> case, so that the of.c gets hold of 'leds_class' pointer, either
>> directly or indirectly.
>
> What exactly do you need to export?
The "static struct class *leds_class" from led-class.c, in one way or
another. of_led_get() needs to go through the led devices from the class.
For now I just removed the "static" from it, so that I can use it from of.c.
>>> also no-op versions of the functions to address configurations when
>>> CONFIG_OF isn't enabled. I have also few comments below.
>>
>> Yep. No-ops for the purpose of making the kernel image smaller?
>
> No, for the case when support for LED subsystem is disabled in the
> kernel config. I'd rather backlight driver depended on LED subsystem
> than selected it.
Sorry, I didn't get that one. How does the backlight driver's
depend/select affect this?
>>> Please move it to include/linux/leds.h, make static inline and provide
>>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>>
>> Ok. Why do you want it as static inline in the leds.h?
>
> This function contains only one instruction, why should we waste
> a function call for it?
I like to keep code in .c files and leave .h files for declarations, and
only in special cases have inline code in .h files.
What do you mean with "waste"?
In this case there's no need to get more performance by inlining (which
anyway may not help and needs to be studied separately for every case),
with inlining the resulting kernel image is larger, and inlining forces
the users of led_put to include module.h to compile.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/22] On-demand device probing
From: Rob Herring @ 2015-09-07 20:50 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel@vger.kernel.org, Rob Herring, Stephen Warren,
Javier Martinez Canillas, Mark Brown, Thierry Reding,
Rafael J. Wysocki, linux-arm-kernel@lists.infradead.org,
Dmitry Torokhov, devicetree@vger.kernel.org, Linus Walleij,
linux-acpi@vger.kernel.org, Arnd Bergmann,
linux-fbdev@vger.kernel.org, Linux USB List, Felipe Balbi,
Linux PWM List, Terje Bergström
In-Reply-To: <1441628627-5143-1-git-send-email-tomeu.vizoso@collabora.com>
On Mon, Sep 7, 2015 at 7:23 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> Hello,
>
> I have a problem with the panel on my Tegra Chromebook taking longer
> than expected to be ready during boot (Stéphane Marchesin reported what
> is basically the same issue in [0]), and have looked into ordered
> probing as a better way of solving this than moving nodes around in the
> DT or playing with initcall levels and linking order.
>
> While reading the thread [1] that Alexander Holler started with his
> series to make probing order deterministic, it occurred to me that it
> should be possible to achieve the same by probing devices as they are
> referenced by other devices.
>
> This basically reuses the information that is already implicit in the
> probe() implementations, saving us from refactoring existing drivers or
> adding information to DTBs.
>
> During review of v1 of this series Linus Walleij suggested that it
> should be the device driver core to make sure that dependencies are
> ready before probing a device. I gave this idea a try [2] but Mark Brown
> pointed out to the logic duplication between the resource acquisition
> and dependency discovery code paths (though I think it's fairly minor).
>
> To address that code duplication I experimented with Arnd's devm_probe
> [3] concept of having drivers declare their dependencies instead of
> acquiring them during probe, and while it worked [4], I don't think we
> end up winning anything when compared to just probing devices on-demand
> from resource getters.
>
> One remaining objection is to the "sprinkling" of calls to
> of_device_probe() in the resource getters of each subsystem, but I think
> it's the right thing to do given that the storage of resources is
> currently subsystem-specific.
>
> We could avoid the above by moving resource storage into the core, but I
> don't think there's a compelling case for that.
>
> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
> OMAP SoCs, and these patches were enough to eliminate all the deferred
> probes (except one in PandaBoard because omap_dma_system doesn't have a
> firmware node as of yet).
>
> Have submitted a branch [5] with only these patches on top of thursday's
> linux-next to kernelci.org and I don't see any issues that could be
> caused by them. For some reason it currently has more passes than the
> version of -next it's based on!
>
> With this series I get the kernel to output to the panel in 0.5s,
> instead of 2.8s.
>
> Regards,
>
> Tomeu
>
> [0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
>
> [1] https://lkml.org/lkml/2014/5/12/452
>
> [2] https://lkml.org/lkml/2015/6/17/305
>
> [3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
>
> [4] https://lkml.org/lkml/2015/7/21/441a
>
> [5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v6
>
> [6] http://kernelci.org/boot/all/job/collabora/kernel/v4.2-11902-g25d80c927f8b/
>
> [7] http://kernelci.org/boot/all/job/next/kernel/next-20150903/
>
> Changes in v4:
> - Added bus.pre_probe callback so the probes of Primecell devices can be
> deferred if their device IDs cannot be yet read because of the clock
> driver not having probed when they are registered. Maybe this goes
> overboard and the matching information should be in the DT if there is
> one.
Seems overboard to me or at least a separate problem. Most clocks have
to be setup before the driver model simply because timers depend on
clocks usually.
Rob
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Jacek Anaszewski @ 2015-09-07 14:11 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55ED848F.2060906@ti.com>
On 09/07/2015 02:35 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 25/08/15 16:25, Jacek Anaszewski wrote:
>> Hi Tomi,
>>
>> Thanks for the patch. Generally, I'd prefer to add files
>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>> there. Those functions' names should begin with "of_". Please provide
>
> Ok, I'll do that. I do need to export something from led-class in that
> case, so that the of.c gets hold of 'leds_class' pointer, either
> directly or indirectly.
What exactly do you need to export?
>
>> also no-op versions of the functions to address configurations when
>> CONFIG_OF isn't enabled. I have also few comments below.
>
> Yep. No-ops for the purpose of making the kernel image smaller?
No, for the case when support for LED subsystem is disabled in the
kernel config. I'd rather backlight driver depended on LED subsystem
than selected it.
> I do
> think the current code compiles and works fine with CONFIG_OF disabled
> (although I have to say I don't remember if I actually tested it).
>
>>> +struct led_classdev *of_led_get(struct device_node *np)
>>> +{
>>> + struct device *led_dev;
>>> + struct led_classdev *led_cdev;
>>> + struct device_node *led_node;
>>> +
>>> + led_node = of_parse_phandle(np, "leds", 0);
>>> + if (!led_node)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + led_dev = class_find_device(leds_class, NULL, led_node,
>>> + led_match_led_node);
>>
>> Single of_node_put(led_node) here will do.
>
> Right.
>
>>> + if (!led_dev) {
>>> + of_node_put(led_node);
>>> + return ERR_PTR(-EPROBE_DEFER);
>>> + }
>>> +
>>> + of_node_put(led_node);
>>
>>> + led_cdev = dev_get_drvdata(led_dev);
>>> +pinctrl/pinctrl.h"
>>> + if (!try_module_get(led_cdev->dev->parent->driver->owner))
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + return led_cdev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_led_get);
>>> +/**
>>> + * led_put() - release a LED device
>>> + * @led_cdev: LED device
>>> + */
>>> +void led_put(struct led_classdev *led_cdev)
>>> +{
>>> + module_put(led_cdev->dev->parent->driver->owner);
>>> +}
>>> +EXPORT_SYMBOL_GPL(led_put);
>>
>> Please move it to include/linux/leds.h, make static inline and provide
>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>
> Ok. Why do you want it as static inline in the leds.h?
This function contains only one instruction, why should we waste
a function call for it?
> I usually like to
> keep the matching functions (get and put here) in the same place.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Jacek Anaszewski @ 2015-09-07 14:10 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55ED8E99.9060004@ti.com>
On 09/07/2015 03:18 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 25/08/15 16:25, Jacek Anaszewski wrote:
>> Hi Tomi,
>>
>> Thanks for the patch. Generally, I'd prefer to add files
>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>> there. Those functions' names should begin with "of_". Please provide
>
> So I presume leds/of.c should be linked together with led-class.c.
> Afaics, that means I need to either rename the resulting led-class.ko or
> led-class.c, so that I can do it in the Makefile. Any preferences?
Let's rename led-class.ko to led-class-objs.ko.
> Alternatively I could create a led-of.ko, but that doesn't feel right.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Tomi Valkeinen @ 2015-09-07 13:18 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55DC6CB9.5060301@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 572 bytes --]
Hi,
On 25/08/15 16:25, Jacek Anaszewski wrote:
> Hi Tomi,
>
> Thanks for the patch. Generally, I'd prefer to add files
> drivers/leds/of.c and include/linux/of_leds.h and put related functions
> there. Those functions' names should begin with "of_". Please provide
So I presume leds/of.c should be linked together with led-class.c.
Afaics, that means I need to either rename the resulting led-class.ko or
led-class.c, so that I can do it in the Makefile. Any preferences?
Alternatively I could create a led-of.ko, but that doesn't feel right.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] leds: Add of_led_get() and led_put()
From: Tomi Valkeinen @ 2015-09-07 12:35 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Jingoo Han, Lee Jones, linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <55DC6CB9.5060301@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]
Hi,
On 25/08/15 16:25, Jacek Anaszewski wrote:
> Hi Tomi,
>
> Thanks for the patch. Generally, I'd prefer to add files
> drivers/leds/of.c and include/linux/of_leds.h and put related functions
> there. Those functions' names should begin with "of_". Please provide
Ok, I'll do that. I do need to export something from led-class in that
case, so that the of.c gets hold of 'leds_class' pointer, either
directly or indirectly.
> also no-op versions of the functions to address configurations when
> CONFIG_OF isn't enabled. I have also few comments below.
Yep. No-ops for the purpose of making the kernel image smaller? I do
think the current code compiles and works fine with CONFIG_OF disabled
(although I have to say I don't remember if I actually tested it).
>> +struct led_classdev *of_led_get(struct device_node *np)
>> +{
>> + struct device *led_dev;
>> + struct led_classdev *led_cdev;
>> + struct device_node *led_node;
>> +
>> + led_node = of_parse_phandle(np, "leds", 0);
>> + if (!led_node)
>> + return ERR_PTR(-ENODEV);
>> +
>> + led_dev = class_find_device(leds_class, NULL, led_node,
>> + led_match_led_node);
>
> Single of_node_put(led_node) here will do.
Right.
>> + if (!led_dev) {
>> + of_node_put(led_node);
>> + return ERR_PTR(-EPROBE_DEFER);
>> + }
>> +
>> + of_node_put(led_node);
>
>> + led_cdev = dev_get_drvdata(led_dev);
>> +
>> + if (!try_module_get(led_cdev->dev->parent->driver->owner))
>> + return ERR_PTR(-ENODEV);
>> +
>> + return led_cdev;
>> +}
>> +EXPORT_SYMBOL_GPL(of_led_get);
>> +/**
>> + * led_put() - release a LED device
>> + * @led_cdev: LED device
>> + */
>> +void led_put(struct led_classdev *led_cdev)
>> +{
>> + module_put(led_cdev->dev->parent->driver->owner);
>> +}
>> +EXPORT_SYMBOL_GPL(led_put);
>
> Please move it to include/linux/leds.h, make static inline and provide
> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
Ok. Why do you want it as static inline in the leds.h? I usually like to
keep the matching functions (get and put here) in the same place.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH v4 13/22] backlight: Probe backlight devices on demand
From: Tomeu Vizoso @ 2015-09-07 12:23 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rob Herring, Stephen Warren, Javier Martinez Canillas, Mark Brown,
Thierry Reding, Rafael J. Wysocki,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Dmitry Torokhov, devicetree-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Tomeu Vizoso,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Tomi Valkeinen, Jingoo Han,
Jean-Christophe Plagniol-Villard, Lee Jones
In-Reply-To: <1441628627-5143-1-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
When looking up a backlight device through its OF node, probe it if it
hasn't already.
The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/video/backlight/backlight.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b17a4d8..9bcdc16eacdf 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/fb.h>
#include <linux/slab.h>
+#include <linux/of_device.h>
#ifdef CONFIG_PMAC_BACKLIGHT
#include <asm/backlight.h>
@@ -559,6 +560,8 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
{
struct device *dev;
+ of_device_probe(node);
+
dev = class_find_device(backlight_class, NULL, node, of_parent_match);
return dev ? to_backlight_device(dev) : NULL;
--
2.4.3
^ permalink raw reply related
* [PATCH v4 0/22] On-demand device probing
From: Tomeu Vizoso @ 2015-09-07 12:23 UTC (permalink / raw)
To: linux-kernel
Cc: Rob Herring, Stephen Warren, Javier Martinez Canillas, Mark Brown,
Thierry Reding, Rafael J. Wysocki, linux-arm-kernel,
Dmitry Torokhov, devicetree, Linus Walleij, linux-acpi,
Arnd Bergmann, Tomeu Vizoso, linux-fbdev, linux-usb, Felipe Balbi,
linux-pwm, Terje Bergström, Greg Kroah-Hartman, Jingoo Han,
David Airlie, Michael Turquette, linux-clk, dmaengine
Hello,
I have a problem with the panel on my Tegra Chromebook taking longer
than expected to be ready during boot (Stéphane Marchesin reported what
is basically the same issue in [0]), and have looked into ordered
probing as a better way of solving this than moving nodes around in the
DT or playing with initcall levels and linking order.
While reading the thread [1] that Alexander Holler started with his
series to make probing order deterministic, it occurred to me that it
should be possible to achieve the same by probing devices as they are
referenced by other devices.
This basically reuses the information that is already implicit in the
probe() implementations, saving us from refactoring existing drivers or
adding information to DTBs.
During review of v1 of this series Linus Walleij suggested that it
should be the device driver core to make sure that dependencies are
ready before probing a device. I gave this idea a try [2] but Mark Brown
pointed out to the logic duplication between the resource acquisition
and dependency discovery code paths (though I think it's fairly minor).
To address that code duplication I experimented with Arnd's devm_probe
[3] concept of having drivers declare their dependencies instead of
acquiring them during probe, and while it worked [4], I don't think we
end up winning anything when compared to just probing devices on-demand
from resource getters.
One remaining objection is to the "sprinkling" of calls to
of_device_probe() in the resource getters of each subsystem, but I think
it's the right thing to do given that the storage of resources is
currently subsystem-specific.
We could avoid the above by moving resource storage into the core, but I
don't think there's a compelling case for that.
I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
OMAP SoCs, and these patches were enough to eliminate all the deferred
probes (except one in PandaBoard because omap_dma_system doesn't have a
firmware node as of yet).
Have submitted a branch [5] with only these patches on top of thursday's
linux-next to kernelci.org and I don't see any issues that could be
caused by them. For some reason it currently has more passes than the
version of -next it's based on!
With this series I get the kernel to output to the panel in 0.5s,
instead of 2.8s.
Regards,
Tomeu
[0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
[1] https://lkml.org/lkml/2014/5/12/452
[2] https://lkml.org/lkml/2015/6/17/305
[3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
[4] https://lkml.org/lkml/2015/7/21/441a
[5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v6
[6] http://kernelci.org/boot/all/job/collabora/kernel/v4.2-11902-g25d80c927f8b/
[7] http://kernelci.org/boot/all/job/next/kernel/next-20150903/
Changes in v4:
- Added bus.pre_probe callback so the probes of Primecell devices can be
deferred if their device IDs cannot be yet read because of the clock
driver not having probed when they are registered. Maybe this goes
overboard and the matching information should be in the DT if there is
one.
- Rename of_platform_probe to of_device_probe
- Use device_node.device instead of device_node.platform_dev
- Take a reference to the regulator's device to prevent dangling
pointers
- Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
machines with initcalls that depend on devices probing at a given time.
- Start processing deferred probes in device_initcall_sync
- Also defer probes of AMBA devices registered from the DT as they can
also request resources.
Changes in v3:
- Set and use device_node.platform_dev instead of reversing the logic to
find the platform device that encloses a device node.
- Drop the fwnode API to probe firmware nodes and add OF-only API for
now. I think this same scheme could be used for machines with ACPI,
but I haven't been able to find one that had to defer its probes because
of the device probe order.
- Avoid unlocking the regulator device's mutex if we don't have a device
Changes in v2:
- Acquire regulator device lock before returning from regulator_dev_lookup()
Tomeu Vizoso (22):
driver core: Add pre_probe callback to bus_type
ARM: amba: Move reading of periphid to pre_probe()
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
gpio: Probe pinctrl devices on demand
regulator: core: Reduce critical area in _regulator_get
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
driver core: Start processing deferred probes earlier
of/platform: Defer probes of registered devices
drivers/amba/bus.c | 78 ++++++++++++++++++-------------------
drivers/base/Kconfig | 18 +++++++++
drivers/base/dd.c | 35 ++++++++++++++++-
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 | 58 +++++++++++++++++++++++++++
drivers/of/platform.c | 26 +++++++------
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 | 58 ++++++++++++++++++---------
drivers/usb/phy/phy.c | 3 ++
drivers/video/backlight/backlight.c | 3 ++
include/linux/device.h | 6 +++
include/linux/of.h | 1 +
include/linux/of_device.h | 3 ++
21 files changed, 251 insertions(+), 71 deletions(-)
--
2.4.3
^ permalink raw reply
* [GIT PULL] fbdev changes for 4.3
From: Tomi Valkeinen @ 2015-09-07 12:19 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fbdev, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]
Hi Linus,
Please pull fbdev changes for 4.3. Not much this time.
The following changes since commit 2c6625cd545bdd66acff14f3394865d43920a5c7:
Linux 4.2-rc7 (2015-08-16 16:34:13 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-4.3
for you to fetch changes up to 57817e619a215588739f3f644986c78b586b541b:
video: fbdev: atmel_lcdfb: remove useless include (2015-09-02 14:24:51 +0300)
----------------------------------------------------------------
fbdev changes for 4.3
* Minor fixes and cleanups
----------------------------------------------------------------
Alexandre Belloni (1):
video: fbdev: atmel_lcdfb: remove useless include
Alexey Klimov (1):
fbdev: udlfb: remove unneeded initialization in few places
Dan Carpenter (1):
fbdev: fix snprintf() limit in show_bl_curve()
Geert Uytterhoeven (1):
fbdev: Allow compile test of GPIO consumers if !GPIOLIB
Julia Lawall (1):
fbdev: ssd1307fb: fix error return code
Krzysztof Kozlowski (3):
video: fbdev: Drop owner assignment from i2c_driver
video: fbdev: Drop owner assignment from platform_driver
video: fbdev: s3c-fb: Constify platform_device_id
Marcin Chojnacki (1):
fbdev: remove unnecessary memset in vfb
Nicolai Stange (1):
framebuffer: disable vgacon on microblaze arch
Tomi Valkeinen (2):
fbdev: fix cea_modes array size
video: fbdev: atmel: fix warning for const return value
Vaishali Thakkar (1):
video: fbdev: pxa168fb: Use devm_clk_get
drivers/video/console/Kconfig | 2 +-
drivers/video/fbdev/Kconfig | 2 +-
drivers/video/fbdev/atmel_lcdfb.c | 3 +--
drivers/video/fbdev/core/fbmon.c | 4 ++--
drivers/video/fbdev/core/fbsysfs.c | 2 +-
drivers/video/fbdev/core/modedb.c | 2 +-
drivers/video/fbdev/omap2/displays-new/encoder-opa362.c | 1 -
drivers/video/fbdev/pxa168fb.c | 14 ++++----------
drivers/video/fbdev/s3c-fb.c | 2 +-
drivers/video/fbdev/ssd1307fb.c | 6 +++---
drivers/video/fbdev/udlfb.c | 10 ++++------
drivers/video/fbdev/vfb.c | 17 ++++++++---------
include/linux/fb.h | 2 +-
13 files changed, 28 insertions(+), 39 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv2] staging: sm750fb: fix improper typedef usage
From: Greg Kroah-Hartman @ 2015-09-05 15:51 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20150831014253.GB10267@brian_archtop.etown.edu>
On Sat, Sep 05, 2015 at 09:17:36AM -0400, Brian Stottler wrote:
> On Thu, Sep 03, 2015 at 06:17:10PM -0700, Greg Kroah-Hartman wrote:
> > Please do this one typedef at a time to make this easier to be able to
> > review.
>
> To clarify, are you looking for entirely separate patches for each
> typedef fix
Yes.
^ permalink raw reply
* Re: [PATCHv2] staging: sm750fb: fix improper typedef usage
From: Brian Stottler @ 2015-09-05 13:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20150831014253.GB10267@brian_archtop.etown.edu>
On Thu, Sep 03, 2015 at 06:17:10PM -0700, Greg Kroah-Hartman wrote:
> Please do this one typedef at a time to make this easier to be able to
> review.
To clarify, are you looking for entirely separate patches for each
typedef fix, or for a more cleanly divided multi-part patch?
Thanks,
Brian Stottler
^ permalink raw reply
* Re: [PATCH 3/3] devicetree: Add led-backlight binding
From: Jacek Anaszewski @ 2015-09-04 15:03 UTC (permalink / raw)
To: Rob Herring
Cc: Tomi Valkeinen, Jingoo Han, Lee Jones, Linux LED Subsystem,
linux-fbdev@vger.kernel.org, Andrew Lunn,
devicetree@vger.kernel.org
In-Reply-To: <CAL_JsqK=QiNk=ZEqJ=T6_rHbjnHq7UY9=ZtWg1HLoU=qdSn8Gg@mail.gmail.com>
On 09/01/2015 01:12 AM, Rob Herring wrote:
> On Wed, Aug 26, 2015 at 4:56 AM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>> On 08/26/2015 11:11 AM, Tomi Valkeinen wrote:
>>>
>>>
>>>
>>> On 26/08/15 10:07, Jacek Anaszewski wrote:
>>>>
>>>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 25/08/15 16:39, Jacek Anaszewski wrote:
>>>>>
>>>>>>> +Example:
>>>>>>> +
>>>>>>> + backlight {
>>>>>>> + compatible = "led-backlight";
>>>>>>> + leds = <&backlight_led>;
>>>>>>> +
>>>>>>> + brightness-levels = <0 4 8 16 32 64 128 255>;
>>>>>>
>>>>>>
>>>>>> brightness level is not a suitable unit for describing LED brightness
>>>>>> in a Device Tree, as it is not a physical unit. We have
>>>>>> led-max-microamp
>>>>>> property for this, expressed in microamperes, please refer to [0] from
>>>>>> linux-next.
>>>>>
>>>>>
>>>>> Hmm, ok, but what should the driver do with microamperes? As far as I
>>>>> see, "enum led_brightness" (which is between 0-255) is used to set the
>>>>> brightness to LEDs. I don't see any function accepting microamperes.
>>>>
>>>>
>>>> This is implementation detail. You can convert microamperes to
>>>> enum led_brightness in the driver. Please refer to the discussion [1].
>>>
>>>
>>> The led_set_brightness() takes "enum led_brightness", so I don't
>>> understand what this driver would do with the microampere value. It
>>> could, of course, do an arbitrary conversion, say, direct mapping of the
>>> mA value to brightness, but that would just confuse things further.
>>
>>
>> OK, I was looking at the problem from LED-centric perspective. Indeed,
>> backlight subsystem has no other way to pass brightness to the LED
>> subsystem than in the form of levels. However, the last word belongs
>> to DT maintainer in this matter.
>>
>> Cc'ing devicetree@vger.kernel.org.
>
> I don't have a simple answer for you...
>
> There was a similar discussion for pm8941-wled and
> "default-brightness-level" units[1]. The conclusion was it should be
> units matching the h/w so that there is no conversion between
> bootloader and OS units to h/w units. That principle probably applies
> here.
>
> If the brightness levels are non-linear, then you need a translation
> from percent to h/w level. What's needed here for h/w levels depends
> on whether the brightness control is PWM, current control or both. For
> PWM, units of the PWM control makes sense. For current control, units
> of microamps probably makes sense. I don't know what you do with both,
> but I have seen that h/w (FSL PMICs).
>
> This all certainly needs some more work on defining some common
> binding. We already have some bindings for backlights with the LED and
> PWM bindings (perhaps incomplete?). Do we need another way here? This
> also introduces possibility of multiple ways to define GPIO controlled
> backlights: gpio -> gpio-leds -> led-backlight or gpio ->
> gpio-backlight. We don't want that...
>
> This problem is not really specific at all to backlights, but applies
> to all LEDs. Some LEDs you may not have control beyond on/off or
> really care about fine-grained control of level, but they are really
> no different. The main unique thing about backlights is what display
> are they associated with.
Some time ago I was trying to add common LEDs DT properties that would
have defined brightness in levels, but other people insisted on
microamperes [1]. The discussion was focused on flash LEDs, where
currents are significantly greater and there is risk of hardware
damage in case underrated LED is connected [2]. Having the property in
microamperes removes the need for consulting documentation to check
current value for given brightness level.
As it was mentioned earlier in this thread, this approach is not
suitable for PWM driven leds, and they have their own bindings,
which allow for describing brightness in levels. Probably we
should add some note to the common LEDs DT bindings that would
state explicitly that such exceptions are allowed.
In case of backlights which are built upon LED class devices we could
stick to brightness levels, as a LED class driver will assert
brightness level to the leds-max-microamp value anyway.
[1] http://www.spinics.net/lists/linux-leds/msg03416.html
[2] http://patchwork.ozlabs.org/patch/456622/
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCHv2] staging: sm750fb: fix improper typedef usage
From: Greg Kroah-Hartman @ 2015-09-04 1:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20150831014253.GB10267@brian_archtop.etown.edu>
On Sun, Aug 30, 2015 at 09:42:53PM -0400, Brian Stottler wrote:
> Fix typedef usage which does not comply with kernel style guidelines.
Please do this one typedef at a time to make this easier to be able to
review.
Also, don't keep "_t" at the end of structure and enum names, it doesn't
make sense anymore, right?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] uvesafb: make scaling configurable on Nvidia cards
From: Mikulas Patocka @ 2015-09-02 21:23 UTC (permalink / raw)
To: Michal Januszewski
Cc: linux-fbdev, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-kernel
In-Reply-To: <alpine.LRH.2.02.1505161334150.19574@file01.intranet.prod.int.rdu2.redhat.com>
[ I sent this some times ago, but didn't get any response ]
Nvidia cards have a BIOS function 0x4f14 that allows to set flat panel
scaling. This patch adds a module parameter "scaling" that uses this
function to set the scaling. By default, the parameter is -1, so that the
driver doesn't attempt to call the scaling function.
This patch is useful when using the binary Nvidia graphics driver - in
that case, the console may be only in text mode or VESA mode. By default,
the video card does scaling that degrades font quality and changes aspect
ratio. This patch makes it possible to turn off the scaling and improve
font quality on the console.
The allowed values depend on VESA BIOS. On my card, the following values
are allowed:
-1 - do not change the scaling
0 - scale to full screen
1, 2 - don't scale
3 - scale and preserve aspect ratio
4 - scale with black border around
Example use:
echo 1 >/sys/module/uvesafb/parameters/scaling; fbset 1280x1024-60 -depth 32 -a
- this sets unscaled 1280x1024 video mode that has much sharper font than
the scaled modes.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Index: linux-4.0.9/drivers/video/fbdev/uvesafb.c
=================================--- linux-4.0.9.orig/drivers/video/fbdev/uvesafb.c
+++ linux-4.0.9/drivers/video/fbdev/uvesafb.c
@@ -52,6 +52,7 @@ static u16 maxclk; /* maximum pixel clo
static u16 maxvf; /* maximum vertical frequency */
static u16 maxhf; /* maximum horizontal frequency */
static u16 vbemode; /* force use of a specific VBE mode */
+static short scaling = -1;
static char *mode_option;
static u8 dac_width = 6;
@@ -1245,7 +1246,22 @@ static int uvesafb_set_par(struct fb_inf
task = uvesafb_prep();
if (!task)
return -ENOMEM;
+
+ if (scaling > 0) {
+ /*
+ * We must first reset scaling state with 0. This is workaround
+ * for some supposed BIOS bug - without this, scaling mode 4
+ * could not be set.
+ */
+ uvesafb_reset(task);
+ task->t.regs.eax = 0x4f14;
+ task->t.regs.ebx = 0x0102;
+ task->t.regs.ecx = 0;
+ uvesafb_exec(task);
+ }
+
setmode:
+ uvesafb_reset(task);
task->t.regs.eax = 0x4f02;
task->t.regs.ebx = mode->mode_id | 0x4000; /* use LFB */
@@ -1296,7 +1312,6 @@ setmode:
printk(KERN_WARNING "uvesafb: mode switch failed "
"(eax=0x%x, err=%d). Trying again with "
"default timings.\n", task->t.regs.eax, err);
- uvesafb_reset(task);
kfree(crtc);
crtc = NULL;
info->var.pixclock = 0;
@@ -1330,6 +1345,14 @@ setmode:
FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
info->fix.line_length = mode->bytes_per_scan_line;
+ if (scaling >= 0) {
+ uvesafb_reset(task);
+ task->t.regs.eax = 0x4f14;
+ task->t.regs.ebx = 0x0102;
+ task->t.regs.ecx = scaling;
+ uvesafb_exec(task);
+ }
+
out:
kfree(crtc);
uvesafb_free(task);
@@ -2019,6 +2042,9 @@ MODULE_PARM_DESC(vbemode,
"VBE mode number to set, overrides the 'mode' option");
module_param_string(v86d, v86d_path, PATH_MAX, 0660);
MODULE_PARM_DESC(v86d, "Path to the v86d userspace helper.");
+module_param(scaling, short, 0660);
+MODULE_PARM_DESC(scaling,
+ "Scaling option for nvidia (0 - scaled, 1 - unscaled centered, 2 - unscaled in left corner");
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Michal Januszewski <spock@gentoo.org>");
Index: linux-4.0.9/Documentation/fb/uvesafb.txt
=================================--- linux-4.0.9.orig/Documentation/fb/uvesafb.txt
+++ linux-4.0.9/Documentation/fb/uvesafb.txt
@@ -128,6 +128,14 @@ v86d:path
need to use it and have uvesafb built into the kernel, use
uvesafb.v86d="path".
+scaling:n
+ Set scaling on Nvidia cards. Possible values depend on card's BIOS.
+ -1 - do not change the scaling
+ 0 - scale to full screen
+ 1, 2 - don't scale
+ 3 - scale and preserve aspect ratio
+ 4 - scale with black border around
+
Additionally, the following parameters may be provided. They all override the
EDID-provided values and BIOS defaults. Refer to your monitor's specs to get
the correct values for maxhf, maxvf and maxclk for your hardware.
^ permalink raw reply
* Re: [PATCH v2 1/3] staging: sm7xxfb: move sm712fb out of staging
From: Sudip Mukherjee @ 2015-09-02 12:48 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jean-Christophe Plagniol-Villard, Jonathan Corbet,
Greg Kroah-Hartman, linux-kernel, linux-fbdev, linux-doc, devel
In-Reply-To: <55E6E45B.5030500@ti.com>
On Wed, Sep 02, 2015 at 02:58:19PM +0300, Tomi Valkeinen wrote:
> On 01/09/15 16:55, Sudip Mukherjee wrote:
> > On Tue, Sep 01, 2015 at 04:27:24PM +0300, Tomi Valkeinen wrote:
> >> On 18/07/15 07:08, Sudip Mukherjee wrote:
> >>> Now since all cleanups are done and the code is ready to be merged lets
> >>> move it out of staging into fbdev location.
<snip>
> > Some replies inline and remaining I will fix and send patches to you.
>
> Wouldn't the time be better spent on the DRM driver?
>
> This driver will be obsolete immediately when there's a DRM driver for
> this device, and then it'll be yet another obsoleted fbdev driver we
> need to maintain.
Now I am getting confused. :(
Since this has already been merged I guess we need to maintain it now.
So then should I fix the things you pointed out or should i instead
give more priority to the DRM driver and fix these things later?
And, just to inform you, there are two more fbdev drivers in staging,
staging/sm750fb and staging/fbtft. And the ultimate goal of any driver
in staging is to move out staging into the main part of the kernel. And
I expect sm750fb to be ready for moving before 4.6 merge window.
If you don't want any more fbdev drivers to be added then maybe you can
have a talk with Greg about this. He is already in the cc.
regards
sudip
^ permalink raw reply
* Re: [PATCH 3/4] [resend #2] fb_ddc: Allow I2C adapters without SCL read capability
From: Ondrej Zary @ 2015-09-02 12:04 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Krzysztof Helt, linux-fbdev, Kernel development list
In-Reply-To: <55E6DE7D.9060700@ti.com>
On Wednesday 02 September 2015, Tomi Valkeinen wrote:
> On 24/08/15 22:37, Ondrej Zary wrote:
> > i2c-algo-bit allows I2C adapters without SCL read capability to work but
> > fb_ddc_read fails to work on them.
> >
> > Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>
> > ---
> > drivers/video/fbdev/core/fb_ddc.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fb_ddc.c
> > b/drivers/video/fbdev/core/fb_ddc.c index 94322cc..22c694a 100644
> > --- a/drivers/video/fbdev/core/fb_ddc.c
> > +++ b/drivers/video/fbdev/core/fb_ddc.c
> > @@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter
> > *adapter) algo_data->setscl(algo_data->data, 1);
> > for (j = 0; j < 5; j++) {
> > msleep(10);
> > - if (algo_data->getscl(algo_data->data))
> > + if (algo_data->getscl &&
> > + algo_data->getscl(algo_data->data))
> > break;
> > }
> > - if (j = 5)
> > + if (algo_data->getscl && j = 5)
> > continue;
> >
> > algo_data->setsda(algo_data->data, 0);
> > @@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
> > algo_data->setscl(algo_data->data, 1);
> > for (j = 0; j < 10; j++) {
> > msleep(10);
> > - if (algo_data->getscl(algo_data->data))
> > + if (algo_data->getscl &&
> > + algo_data->getscl(algo_data->data))
> > break;
> > }
>
> Aren't both of those loops pointless if there's no getscl?
They're reduced to delays - don't know how much critical they are.
--
Ondrej Zary
^ permalink raw reply
* Re: [PATCH 4/4] [resend #2] tridentfb: Add DDC support
From: Ondrej Zary @ 2015-09-02 12:04 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Krzysztof Helt, linux-fbdev, Kernel development list
In-Reply-To: <55E6DDC3.3030709@ti.com>
On Wednesday 02 September 2015, Tomi Valkeinen wrote:
> Hi,
>
> On 24/08/15 22:37, Ondrej Zary wrote:
> > Add DDC support for Trident cards.
> >
> > Tested on TGUI9440, TGUI9680, 3DImage 9750, Blade3D 9880 and Blade XP.
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> > drivers/video/fbdev/Kconfig | 9 ++
> > drivers/video/fbdev/tridentfb.c | 192
> > ++++++++++++++++++++++++++++++++++++++- 2 files changed, 196
> > insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> > index 2d98de5..e2531b8 100644
> > --- a/drivers/video/fbdev/Kconfig
> > +++ b/drivers/video/fbdev/Kconfig
> > @@ -1680,6 +1680,15 @@ config FB_TRIDENT
> > To compile this driver as a module, choose M here: the
> > module will be called tridentfb.
> >
> > +config FB_TRIDENT_DDC
> > + bool "DDC for Trident support"
> > + depends on FB_TRIDENT
> > + select FB_DDC
> > + select FB_MODE_HELPERS
> > + default y
> > + help
> > + Say Y here if you want DDC support for your Trident graphics card.
> > +
>
> Why would somebody not want this enabled? Is there some drawback if it's
> enabled?
It's probably an useless config option but many fbdev drivers have it. I have
no problem removing it.
--
Ondrej Zary
^ permalink raw reply
* Re: [PATCH v2 1/3] staging: sm7xxfb: move sm712fb out of staging
From: Tomi Valkeinen @ 2015-09-02 11:58 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Jean-Christophe Plagniol-Villard, Jonathan Corbet,
Greg Kroah-Hartman, linux-kernel, linux-fbdev, linux-doc, devel
In-Reply-To: <20150901135514.GB15833@sudip-pc>
[-- Attachment #1: Type: text/plain, Size: 3049 bytes --]
On 01/09/15 16:55, Sudip Mukherjee wrote:
> On Tue, Sep 01, 2015 at 04:27:24PM +0300, Tomi Valkeinen wrote:
>>
>>
>> On 18/07/15 07:08, Sudip Mukherjee wrote:
>>> Now since all cleanups are done and the code is ready to be merged lets
>>> move it out of staging into fbdev location.
>>
>> Have you considered writing a DRM driver for this? I'm not happy at all
>> adding new fbdev drivers, as the DRM framework is much better,
>> supported, and continuously improved. With fbdev you end up with things
>> like module parameters used to define video modes etc, which is just ugly.
> Yes, I am working on a DRM driver, but since these are all voluntary
> work it is taking time. And Greg has already merged it.
>>
>> Anyway, some comments below.
> Some replies inline and remaining I will fix and send patches to you.
Wouldn't the time be better spent on the DRM driver?
This driver will be obsolete immediately when there's a DRM driver for
this device, and then it'll be yet another obsoleted fbdev driver we
need to maintain.
>>> +static const struct vesa_mode vesa_mode_table[] = {
>>> + {"0x301", 640, 480, 8},
>>> + {"0x303", 800, 600, 8},
>>> + {"0x305", 1024, 768, 8},
>>> + {"0x307", 1280, 1024, 8},
>>> +
>>> + {"0x311", 640, 480, 16},
>>> + {"0x314", 800, 600, 16},
>>> + {"0x317", 1024, 768, 16},
>>> + {"0x31A", 1280, 1024, 16},
>>> +
>>> + {"0x312", 640, 480, 24},
>>> + {"0x315", 800, 600, 24},
>>> + {"0x318", 1024, 768, 24},
>>> + {"0x31B", 1280, 1024, 24},
>>> +};
>>
>> We have "vesa_modes" in include/linux/fb.h. What is the above table for?
> The resolutions that are supported along with the kernel boot parameter
> to point to the resolution to boot with.
Why does the user need to give such hex values? Why not modes according
to Documentation/fb/modedb.txt?
>>> +
>>> +/**********************************************************************
>>> + SM712 Mode table.
>>> + **********************************************************************/
>>> +static const struct modeinit vgamode[] = {
>>> + {
> <snip>
>>> + { /* Init_CR90_CRA7 */
>>> + 0x55, 0xD9, 0x5D, 0xE1, 0x86, 0x1B, 0x8E, 0x26,
>>> + 0xDA, 0x8D, 0xDE, 0x94, 0x00, 0x00, 0x18, 0x00,
>>> + 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x15, 0x03,
>>> + },
>>> + },
>>> +};
>>
>> What are these tables above for?
> Different register settings based on the display resolution. Do you want
> me to do anything with these vgamode table and the vesa_mode_table?
The vgamode table looks quite horrible. It's unmaintainable, and with a
quick look it seems to have lots of repetition. Large blocks of the data
for different modes are the same.
I don't know what the register there are, but I'd imagine you could
write generic functions like, say, "set_timings", which takes normal
linux videomode struct and writes those settings to the registers.
There should be no such bulk-write register tables in a proper driver,
except in some very special cases.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] [resend #2] fb_ddc: Allow I2C adapters without SCL read capability
From: Tomi Valkeinen @ 2015-09-02 11:33 UTC (permalink / raw)
To: Ondrej Zary, Krzysztof Helt; +Cc: linux-fbdev, Kernel development list
In-Reply-To: <1440445048-24694-3-git-send-email-linux@rainbow-software.org>
[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]
On 24/08/15 22:37, Ondrej Zary wrote:
> i2c-algo-bit allows I2C adapters without SCL read capability to work but
> fb_ddc_read fails to work on them.
>
> Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>
> ---
> drivers/video/fbdev/core/fb_ddc.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_ddc.c b/drivers/video/fbdev/core/fb_ddc.c
> index 94322cc..22c694a 100644
> --- a/drivers/video/fbdev/core/fb_ddc.c
> +++ b/drivers/video/fbdev/core/fb_ddc.c
> @@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
> algo_data->setscl(algo_data->data, 1);
> for (j = 0; j < 5; j++) {
> msleep(10);
> - if (algo_data->getscl(algo_data->data))
> + if (algo_data->getscl &&
> + algo_data->getscl(algo_data->data))
> break;
> }
> - if (j == 5)
> + if (algo_data->getscl && j == 5)
> continue;
>
> algo_data->setsda(algo_data->data, 0);
> @@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
> algo_data->setscl(algo_data->data, 1);
> for (j = 0; j < 10; j++) {
> msleep(10);
> - if (algo_data->getscl(algo_data->data))
> + if (algo_data->getscl &&
> + algo_data->getscl(algo_data->data))
> break;
> }
Aren't both of those loops pointless if there's no getscl?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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