* simplefb: add clock handling
From: Luc Verhaegen @ 2014-08-13 7:17 UTC (permalink / raw)
To: linux-arm-kernel
This is needed for the sunxi platform, where the u-boot initialized display
engine gets disabled by the clocks framework if certain clocks are not
claimed. Once these clocks are disabled, register content is lost, and there
is no turning back unless a full display driver is loaded, which kind of
beats the purpose of having simplefb running.
The lack of clock handling should plague more hardware, but so far rpi is the
best known user of simplefb, and its stepmotherly handling of the arm core
has kept these sort of issues from the kernel.
The sunxi u-boot side code can be found here:
https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
Patch 3 might be controversial, as this does not achieve anything real today,
since the status property in dt is only really evaluated when dealing with a
nodes memory. It still seems like a good idea to at least flag this memory or
node as disabled, as we really have no way back when the clocks get disabled.
Luc Verhaegen.
^ permalink raw reply
* [PATCH 1/4] simplefb: formalize pseudo palette handling
From: Luc Verhaegen @ 2014-08-13 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-1-git-send-email-libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 210f3a0..32be590 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -41,6 +41,8 @@ static struct fb_var_screeninfo simplefb_var = {
.vmode = FB_VMODE_NONINTERLACED,
};
+#define PSEUDO_PALETTE_SIZE 16
+
static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
u_int transp, struct fb_info *info)
{
@@ -50,7 +52,7 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
u32 cb = blue >> (16 - info->var.blue.length);
u32 value;
- if (regno >= 16)
+ if (regno >= PSEUDO_PALETTE_SIZE)
return -EINVAL;
value = (cr << info->var.red.offset) |
@@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
+struct simplefb_par {
+ u32 palette[PSEUDO_PALETTE_SIZE];
+};
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
struct simplefb_params params;
struct fb_info *info;
+ struct simplefb_par *par;
struct resource *mem;
if (fb_get_options("simplefb", NULL))
@@ -188,11 +195,13 @@ static int simplefb_probe(struct platform_device *pdev)
return -EINVAL;
}
- info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+ info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
if (!info)
return -ENOMEM;
platform_set_drvdata(pdev, info);
+ par = info->par;
+
info->fix = simplefb_fix;
info->fix.smem_start = mem->start;
info->fix.smem_len = resource_size(mem);
@@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
framebuffer_release(info);
return -ENODEV;
}
- info->pseudo_palette = (void *)(info + 1);
+ info->pseudo_palette = (void *) par->palette;
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
--
1.7.7
^ permalink raw reply related
* [PATCH 2/4] simplefb: add goto error path to probe
From: Luc Verhaegen @ 2014-08-13 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-1-git-send-email-libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 32be590..72a4f20 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->apertures = alloc_apertures(1);
if (!info->apertures) {
- framebuffer_release(info);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto error_fb_release;
}
info->apertures->ranges[0].base = info->fix.smem_start;
info->apertures->ranges[0].size = info->fix.smem_len;
@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->screen_base = ioremap_wc(info->fix.smem_start,
info->fix.smem_len);
if (!info->screen_base) {
- framebuffer_release(info);
- return -ENODEV;
+ ret = -ENODEV;
+ goto error_fb_release;
}
info->pseudo_palette = (void *) par->palette;
@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- iounmap(info->screen_base);
- framebuffer_release(info);
- return ret;
+ goto error_unmap;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
+
+ error_unmap:
+ iounmap(info->screen_base);
+
+ error_fb_release:
+ framebuffer_release(info);
+
+ return ret;
}
static int simplefb_remove(struct platform_device *pdev)
--
1.7.7
^ permalink raw reply related
* [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-1-git-send-email-libv@skynet.be>
The next commit will handle clocks correctly, so that these do not get
automatically disabled on certain SoC simplefb implementations. As a
result, the removal of this simplefb driver, and the release of the
clocks, is rather final, and only a full display driver can work after
this. So, it makes sense to also flag the dt node as disabled, even
though it has no real value today.
Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
drivers/video/fbdev/simplefb.c | 43 ++++++++++++++++++++++++++++++++++++---
1 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 72a4f20..74c4b2a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -138,6 +138,32 @@ static int simplefb_parse_dt(struct platform_device *pdev,
return 0;
}
+/*
+ * Make sure that nothing tries to inadvertedly re-use this node...
+ */
+static int
+simplefb_dt_disable(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct property *property;
+ int ret;
+
+ property = kzalloc(sizeof(struct property), GFP_KERNEL);
+ if (!property)
+ return -ENOMEM;
+
+ property->name = "status";
+ property->value = "disabled";
+ property->length = strlen(property->value) + 1;
+
+ ret = of_update_property(np, property);
+ if (ret)
+ dev_err(&pdev->dev, "%s: failed to update property: %d\n",
+ __func__, ret);
+
+ return ret;
+}
+
static int simplefb_parse_pd(struct platform_device *pdev,
struct simplefb_params *params)
{
@@ -187,17 +213,20 @@ static int simplefb_probe(struct platform_device *pdev)
ret = simplefb_parse_dt(pdev, ¶ms);
if (ret)
- return ret;
+ goto error_dt_disable;
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!mem) {
dev_err(&pdev->dev, "No memory resource\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto error_dt_disable;
}
info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
- if (!info)
- return -ENOMEM;
+ if (!info) {
+ ret = -ENOMEM;
+ goto error_dt_disable;
+ }
platform_set_drvdata(pdev, info);
par = info->par;
@@ -260,6 +289,10 @@ static int simplefb_probe(struct platform_device *pdev)
error_fb_release:
framebuffer_release(info);
+ error_dt_disable:
+ if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
+ simplefb_dt_disable(pdev);
+
return ret;
}
@@ -269,6 +302,8 @@ static int simplefb_remove(struct platform_device *pdev)
unregister_framebuffer(info);
framebuffer_release(info);
+ if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
+ simplefb_dt_disable(pdev);
return 0;
}
--
1.7.7
^ permalink raw reply related
* [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-13 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-1-git-send-email-libv@skynet.be>
This claims and enables clocks listed in the simple framebuffer dt node.
This is needed so that the display engine, in case the required clocks
are known by the kernel code and are described in the dt, will remain
properly enabled.
Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
drivers/video/fbdev/simplefb.c | 103 +++++++++++++++++++++++++++++++++++++++-
1 files changed, 101 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 74c4b2a..481dbfd 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -26,6 +26,7 @@
#include <linux/module.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
+#include <linux/clk-provider.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -191,8 +192,101 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ */
+struct simplefb_clock {
+ struct list_head list;
+ struct clk *clock;
+};
+
+/*
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static void
+simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int clock_count, i;
+
+ INIT_LIST_HEAD(list);
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return;
+
+ clock_count = of_clk_get_parent_count(np);
+ for (i = 0; i < clock_count; i++) {
+ struct simplefb_clock *entry;
+ struct clk *clock = of_clk_get(np, i);
+ int ret;
+
+ if (IS_ERR(clock)) {
+ dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+ __func__, i, PTR_ERR(clock));
+ continue;
+ }
+
+ ret = clk_prepare_enable(clock);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s: failed to enable clock %d: %d\n",
+ __func__, i, ret);
+ clk_put(clock);
+ continue;
+ }
+
+ entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
+ if (!entry) {
+ dev_err(&pdev->dev,
+ "%s: failed to alloc clock %d list entry.\n",
+ __func__, i);
+ clk_disable_unprepare(clock);
+ clk_put(clock);
+ continue;
+ }
+
+ entry->clock = clock;
+ /*
+ * add to the front of the list, so we disable clocks in the
+ * correct order.
+ */
+ list_add(&entry->list, list);
+ }
+}
+
+static void
+simplefb_clocks_destroy(struct list_head *list)
+{
+ struct list_head *pos, *n;
+
+ list_for_each_safe(pos, n, list) {
+ struct simplefb_clock *entry + container_of(pos, struct simplefb_clock, list);
+
+ list_del(&entry->list);
+
+ clk_disable_unprepare(entry->clock);
+ clk_put(entry->clock);
+ kfree(entry);
+ }
+}
+
struct simplefb_par {
u32 palette[PSEUDO_PALETTE_SIZE];
+ struct list_head clock_list[1];
};
static int simplefb_probe(struct platform_device *pdev)
@@ -265,6 +359,8 @@ static int simplefb_probe(struct platform_device *pdev)
}
info->pseudo_palette = (void *) par->palette;
+ simplefb_clocks_init(pdev, par->clock_list);
+
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
info->screen_base);
@@ -276,14 +372,15 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- goto error_unmap;
+ goto error_clocks;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
- error_unmap:
+ error_clocks:
+ simplefb_clocks_destroy(par->clock_list);
iounmap(info->screen_base);
error_fb_release:
@@ -299,8 +396,10 @@ static int simplefb_probe(struct platform_device *pdev)
static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ struct simplefb_par *par = info->par;
unregister_framebuffer(info);
+ simplefb_clocks_destroy(par->clock_list);
framebuffer_release(info);
if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
simplefb_dt_disable(pdev);
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 1/4] simplefb: formalize pseudo palette handling
From: David Herrmann @ 2014-08-13 7:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-2-git-send-email-libv@skynet.be>
Hi
On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> ---
> drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
> 1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 210f3a0..32be590 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -41,6 +41,8 @@ static struct fb_var_screeninfo simplefb_var = {
> .vmode = FB_VMODE_NONINTERLACED,
> };
>
> +#define PSEUDO_PALETTE_SIZE 16
> +
> static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
> u_int transp, struct fb_info *info)
> {
> @@ -50,7 +52,7 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
> u32 cb = blue >> (16 - info->var.blue.length);
> u32 value;
>
> - if (regno >= 16)
> + if (regno >= PSEUDO_PALETTE_SIZE)
> return -EINVAL;
>
> value = (cr << info->var.red.offset) |
> @@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
> return 0;
> }
>
> +struct simplefb_par {
> + u32 palette[PSEUDO_PALETTE_SIZE];
> +};
> +
I'd move that definition to the top of the file.
> static int simplefb_probe(struct platform_device *pdev)
> {
> int ret;
> struct simplefb_params params;
> struct fb_info *info;
> + struct simplefb_par *par;
> struct resource *mem;
>
> if (fb_get_options("simplefb", NULL))
> @@ -188,11 +195,13 @@ static int simplefb_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
> + info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
> if (!info)
> return -ENOMEM;
> platform_set_drvdata(pdev, info);
>
> + par = info->par;
> +
> info->fix = simplefb_fix;
> info->fix.smem_start = mem->start;
> info->fix.smem_len = resource_size(mem);
> @@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
> framebuffer_release(info);
> return -ENODEV;
> }
> - info->pseudo_palette = (void *)(info + 1);
> + info->pseudo_palette = (void *) par->palette;
I think coding-style is this (i.e., no whitespace):
info->pseudo_palette = (void*)par->palette;
Patch is fine with me:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
>
> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
> info->fix.smem_start, info->fix.smem_len,
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] simplefb: add goto error path to probe
From: David Herrmann @ 2014-08-13 7:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-3-git-send-email-libv@skynet.be>
Hi
On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> ---
> drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
> 1 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 32be590..72a4f20 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
>
> info->apertures = alloc_apertures(1);
> if (!info->apertures) {
> - framebuffer_release(info);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto error_fb_release;
> }
> info->apertures->ranges[0].base = info->fix.smem_start;
> info->apertures->ranges[0].size = info->fix.smem_len;
> @@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
> info->screen_base = ioremap_wc(info->fix.smem_start,
> info->fix.smem_len);
> if (!info->screen_base) {
> - framebuffer_release(info);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto error_fb_release;
> }
> info->pseudo_palette = (void *) par->palette;
>
> @@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
> ret = register_framebuffer(info);
> if (ret < 0) {
> dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
> - iounmap(info->screen_base);
> - framebuffer_release(info);
> - return ret;
> + goto error_unmap;
> }
>
> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>
> return 0;
> +
> + error_unmap:
> + iounmap(info->screen_base);
> +
> + error_fb_release:
> + framebuffer_release(info);
> +
> + return ret;
Again, I'd use different coding-style, but I will leave that to
Stephen and Tomi:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> }
>
> static int simplefb_remove(struct platform_device *pdev)
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: simplefb: add clock handling
From: David Herrmann @ 2014-08-13 7:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-1-git-send-email-libv@skynet.be>
Hi
On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> This is needed for the sunxi platform, where the u-boot initialized display
> engine gets disabled by the clocks framework if certain clocks are not
> claimed. Once these clocks are disabled, register content is lost, and there
> is no turning back unless a full display driver is loaded, which kind of
> beats the purpose of having simplefb running.
>
> The lack of clock handling should plague more hardware, but so far rpi is the
> best known user of simplefb, and its stepmotherly handling of the arm core
> has kept these sort of issues from the kernel.
>
> The sunxi u-boot side code can be found here:
> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>
> Patch 3 might be controversial, as this does not achieve anything real today,
> since the status property in dt is only really evaluated when dealing with a
> nodes memory. It still seems like a good idea to at least flag this memory or
> node as disabled, as we really have no way back when the clocks get disabled.
Hans de Goede shortly told me about this and, tbh, I am not very
pleased. Stephen tried to keep simplefd "as simple as possible", your
patch now adds hardware-specific features. To be fair, the patch is
simple and clocks are easy to handle, but I somehow fear we have to
add more and more hardware-support that is required to keep the
framebuffer active. This really defeats the purpose of simplefb.
The biggest question I have, is why do you add the clocks to your DT
at all? The framebuffer is set up by your boot-loader (or maybe
platform code) and should prepare the clocks. I don't see why we add
the clocks to DT now. If they're not added, then no-one will disable
them and simplefb works just fine, right?
Or is there some requirement to make DT a _complete_ hw-description?
Or is there some parent clock which might be used by other drivers and
controls the clock used for your display?
The only reason I see to add the clocks, is to support both, simplefb
*and* a hardware-driver. However, fbdev hand-over is horribly racy and
I'd much rather prefer a solution like sysfb that does proper handover
from primitive firmware-FBs to real hardware-drivers. In that case,
we'd have to figure out how to deal with clocks, but we could do it in
sysfb (which is meant to deal with those issues) with the benefit of
controlling hand-over directly, and allowing hw-dependent features.
My sysfb patches haven't been updated for a while, though, and you
have a working solution here. So I'm not going to NAK this, I
appreciate people working on this. But I'd like to get a discussion
started so we can at least figure out some nicer solution for the
future which might replace your code.
Btw., my current idea was to destroy the platform-devices of EFI/VGA
framebuffers during hand-over (because usually the underlying hw is
shutdown/modified). This automatically unloads simplefb and makes sure
the real hw-driver can be probed without other drivers touching the
hardware in parallel. Iff you unload the hw-driver afterwards, it can
re-create the firmware framebuffer and add a platform-device to make
simplefb load again (in case anyone really needs this).
Looking at your 3rd patch, I wonder whether this works with DT based
machines, or whether we'd have to use the "disable" mechanism you
chose. Any reason destroying the device would not work there? If yes,
I will look into the "disable-idea".
Thanks
David
^ permalink raw reply
* Re: simplefb: add clock handling
From: Luc Verhaegen @ 2014-08-13 8:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4Re79+D5H-pf8pW87NZ5MWz9UFknd5Dk2GKCvhzm5MNSA@mail.gmail.com>
On Wed, Aug 13, 2014 at 09:54:21AM +0200, David Herrmann wrote:
> Hi
>
> Hans de Goede shortly told me about this and, tbh, I am not very
> pleased. Stephen tried to keep simplefd "as simple as possible", your
> patch now adds hardware-specific features. To be fair, the patch is
> simple and clocks are easy to handle, but I somehow fear we have to
> add more and more hardware-support that is required to keep the
> framebuffer active. This really defeats the purpose of simplefb.
SimpleFB is a really deceptive name. If you lie to your ARM core all the
time, and do most things behind its back, like RPi does, then you can
get away with simplefb.
If not, it quickly becomes a lot more complicated, fast. This should've
been called rpibootfb or something.
How does one deal with this dealbreaker issue of clocks? Or memory,
apart from telling the kernel that the fb area is simple not accessible
as normal ram. Or dpms?
> The biggest question I have, is why do you add the clocks to your DT
> at all?
I didn't, someone else did.
> The framebuffer is set up by your boot-loader (or maybe
> platform code) and should prepare the clocks. I don't see why we add
> the clocks to DT now. If they're not added, then no-one will disable
> them and simplefb works just fine, right?
>
> Or is there some requirement to make DT a _complete_ hw-description?
Again, i am not responsible for that part. But my impression is that it
should be absolutely complete, and i have been whining about actual bit
offsets into registers being provided from the dt :(
> Or is there some parent clock which might be used by other drivers and
> controls the clock used for your display?
Not at this point for sunxi, but there will be.
> The only reason I see to add the clocks, is to support both, simplefb
> *and* a hardware-driver. However, fbdev hand-over is horribly racy and
> I'd much rather prefer a solution like sysfb that does proper handover
> from primitive firmware-FBs to real hardware-drivers. In that case,
> we'd have to figure out how to deal with clocks, but we could do it in
> sysfb (which is meant to deal with those issues) with the benefit of
> controlling hand-over directly, and allowing hw-dependent features.
Yes, a KMS driver is being worked on. I was, as a first order
approximation (when i move it to mainline code), going to manually do
some of this handover from simplefb.
> My sysfb patches haven't been updated for a while, though, and you
> have a working solution here. So I'm not going to NAK this, I
> appreciate people working on this. But I'd like to get a discussion
> started so we can at least figure out some nicer solution for the
> future which might replace your code.
So much for this being simple. again :)
> Btw., my current idea was to destroy the platform-devices of EFI/VGA
> framebuffers during hand-over (because usually the underlying hw is
> shutdown/modified). This automatically unloads simplefb and makes sure
> the real hw-driver can be probed without other drivers touching the
> hardware in parallel. Iff you unload the hw-driver afterwards, it can
> re-create the firmware framebuffer and add a platform-device to make
> simplefb load again (in case anyone really needs this).
> Looking at your 3rd patch, I wonder whether this works with DT based
> machines, or whether we'd have to use the "disable" mechanism you
> chose. Any reason destroying the device would not work there? If yes,
> I will look into the "disable-idea".
The clocks we currently claim handle the ahb gating of different display
engines. Disable the gating bit, and all power is lost to the respective
engine, and register content vanishes. So there is absolutely no coming
back from that, apart from starting a proper display driver.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] simplefb: add clock handling
From: Koen Kooi @ 2014-08-13 8:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4Re79+D5H-pf8pW87NZ5MWz9UFknd5Dk2GKCvhzm5MNSA@mail.gmail.com>
Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
> Hi
>
> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> This is needed for the sunxi platform, where the u-boot initialized display
>> engine gets disabled by the clocks framework if certain clocks are not
>> claimed. Once these clocks are disabled, register content is lost, and there
>> is no turning back unless a full display driver is loaded, which kind of
>> beats the purpose of having simplefb running.
>>
>> The lack of clock handling should plague more hardware, but so far rpi is the
>> best known user of simplefb, and its stepmotherly handling of the arm core
>> has kept these sort of issues from the kernel.
>>
>> The sunxi u-boot side code can be found here:
>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>
>> Patch 3 might be controversial, as this does not achieve anything real today,
>> since the status property in dt is only really evaluated when dealing with a
>> nodes memory. It still seems like a good idea to at least flag this memory or
>> node as disabled, as we really have no way back when the clocks get disabled.
>
> Hans de Goede shortly told me about this and, tbh, I am not very
> pleased. Stephen tried to keep simplefd "as simple as possible", your
> patch now adds hardware-specific features. To be fair, the patch is
> simple and clocks are easy to handle, but I somehow fear we have to
> add more and more hardware-support that is required to keep the
> framebuffer active. This really defeats the purpose of simplefb.
>
> The biggest question I have, is why do you add the clocks to your DT
> at all? The framebuffer is set up by your boot-loader (or maybe
> platform code) and should prepare the clocks. I don't see why we add
> the clocks to DT now. If they're not added, then no-one will disable
> them and simplefb works just fine, right?
All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch.
To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work.
regards,
Koen
^ permalink raw reply
* Re: [linux-sunxi] simplefb: add clock handling
From: Hans de Goede @ 2014-08-13 8:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <BFCF4C2C-77ED-4719-903B-921DEB250B7A@dominion.thruhere.net>
Hi,
On 08/13/2014 10:21 AM, Koen Kooi wrote:
>
> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
>
>> Hi
>>
>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> This is needed for the sunxi platform, where the u-boot initialized display
>>> engine gets disabled by the clocks framework if certain clocks are not
>>> claimed. Once these clocks are disabled, register content is lost, and there
>>> is no turning back unless a full display driver is loaded, which kind of
>>> beats the purpose of having simplefb running.
>>>
>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>> has kept these sort of issues from the kernel.
>>>
>>> The sunxi u-boot side code can be found here:
>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>
>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>> since the status property in dt is only really evaluated when dealing with a
>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>> node as disabled, as we really have no way back when the clocks get disabled.
>>
>> Hans de Goede shortly told me about this and, tbh, I am not very
>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>> patch now adds hardware-specific features. To be fair, the patch is
>> simple and clocks are easy to handle, but I somehow fear we have to
>> add more and more hardware-support that is required to keep the
>> framebuffer active. This really defeats the purpose of simplefb.
>>
>> The biggest question I have, is why do you add the clocks to your DT
>> at all? The framebuffer is set up by your boot-loader (or maybe
>> platform code) and should prepare the clocks. I don't see why we add
>> the clocks to DT now. If they're not added, then no-one will disable
>> them and simplefb works just fine, right?
>
> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch.
> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work.
I believe that having some simple generic fb driver will be useful
on non x86, since we don't have vga-console there, and most distros
will build kms drivers as modules. Having the kernel / initrd code being
able to show output (like e.g. missing symbols in the kms drivers) seems
a very useful feature to me.
The way I envision this to work is:
u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
it can show this on the display
kernel takes over using something like simplefb (built into the kernel)
for its initial output / any error messages.
initrd loads kms, kms takes over.
This way we've a way to show error messages during boot at all times.
As we start supporting more ARM htpc boxes out of the box, telling the
user to hook up a serial console (which often involves soldering wires
to some test points) when things don't work really is not a viable
answer.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Grant Likely @ 2014-08-13 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-4-git-send-email-libv@skynet.be>
On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> The next commit will handle clocks correctly, so that these do not get
> automatically disabled on certain SoC simplefb implementations. As a
> result, the removal of this simplefb driver, and the release of the
> clocks, is rather final, and only a full display driver can work after
> this. So, it makes sense to also flag the dt node as disabled, even
> though it has no real value today.
>
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
Please, no.
Drivers should not be modifying the device tree without and
exceptionally good reason for doing so. Drivers are to treat the DT as
immutable.
* the exception is an overlay driver which add new devices to the
kernel. Definitely not the case here.
g.
> ---
> drivers/video/fbdev/simplefb.c | 43 ++++++++++++++++++++++++++++++++++++---
> 1 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 72a4f20..74c4b2a 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -138,6 +138,32 @@ static int simplefb_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +/*
> + * Make sure that nothing tries to inadvertedly re-use this node...
> + */
> +static int
> +simplefb_dt_disable(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct property *property;
> + int ret;
> +
> + property = kzalloc(sizeof(struct property), GFP_KERNEL);
> + if (!property)
> + return -ENOMEM;
> +
> + property->name = "status";
> + property->value = "disabled";
> + property->length = strlen(property->value) + 1;
> +
> + ret = of_update_property(np, property);
> + if (ret)
> + dev_err(&pdev->dev, "%s: failed to update property: %d\n",
> + __func__, ret);
> +
> + return ret;
> +}
> +
> static int simplefb_parse_pd(struct platform_device *pdev,
> struct simplefb_params *params)
> {
> @@ -187,17 +213,20 @@ static int simplefb_probe(struct platform_device *pdev)
> ret = simplefb_parse_dt(pdev, ¶ms);
>
> if (ret)
> - return ret;
> + goto error_dt_disable;
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!mem) {
> dev_err(&pdev->dev, "No memory resource\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto error_dt_disable;
> }
>
> info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
> - if (!info)
> - return -ENOMEM;
> + if (!info) {
> + ret = -ENOMEM;
> + goto error_dt_disable;
> + }
> platform_set_drvdata(pdev, info);
>
> par = info->par;
> @@ -260,6 +289,10 @@ static int simplefb_probe(struct platform_device *pdev)
> error_fb_release:
> framebuffer_release(info);
>
> + error_dt_disable:
> + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
> + simplefb_dt_disable(pdev);
> +
> return ret;
> }
>
> @@ -269,6 +302,8 @@ static int simplefb_remove(struct platform_device *pdev)
>
> unregister_framebuffer(info);
> framebuffer_release(info);
> + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
> + simplefb_dt_disable(pdev);
>
> return 0;
> }
> --
> 1.7.7
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/4] simplefb: formalize pseudo palette handling
From: Geert Uytterhoeven @ 2014-08-13 8:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4ShhaUMT3MpAei4sgo7Nr-jHfD+TpF8dzAVbn58Feroiw@mail.gmail.com>
On Wed, Aug 13, 2014 at 9:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> @@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
>> framebuffer_release(info);
>> return -ENODEV;
>> }
>> - info->pseudo_palette = (void *)(info + 1);
>> + info->pseudo_palette = (void *) par->palette;
>
> I think coding-style is this (i.e., no whitespace):
> info->pseudo_palette = (void*)par->palette;
<casts-are-evil>
Is this cast even needed?
</casts-are-evil>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: David Herrmann @ 2014-08-13 8:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6vaPUa4QvYWBA-QY7MTmJYoeqnPt9Zh5jpky6S9UYbrvQ@mail.gmail.com>
Hi
On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> The next commit will handle clocks correctly, so that these do not get
>> automatically disabled on certain SoC simplefb implementations. As a
>> result, the removal of this simplefb driver, and the release of the
>> clocks, is rather final, and only a full display driver can work after
>> this. So, it makes sense to also flag the dt node as disabled, even
>> though it has no real value today.
>>
>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>
> Please, no.
>
> Drivers should not be modifying the device tree without and
> exceptionally good reason for doing so. Drivers are to treat the DT as
> immutable.
>
> * the exception is an overlay driver which add new devices to the
> kernel. Definitely not the case here.
Why? I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and
create simple-framebuffer devices for them. Those are
valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing
framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB
and thus create a new (or enable the old) platform device.
Or, in a nutshell: A "simple-framebuffer" device is basically a
platform-device for framebuffers. Framebuffers can be created and
destroyed during runtime. The reason we create platform-devices for
them, is to allow dummy drivers to be probed. Real hw-drivers
obviously bind to the parent bus device.
Other ideas are obviously welcome, but so far all of the other ideas
sounded like big hacks (like remove_conflicting_framebuffers() so
far..).
Thanks
David
^ permalink raw reply
* Re: [PATCH 1/4] simplefb: formalize pseudo palette handling
From: David Herrmann @ 2014-08-13 8:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdVROkKvW1VYWbOtecO9sQAudtgS-mJtZb5ZLUCMaZdGAA@mail.gmail.com>
Hi
On Wed, Aug 13, 2014 at 10:46 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Aug 13, 2014 at 9:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> @@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
>>> framebuffer_release(info);
>>> return -ENODEV;
>>> }
>>> - info->pseudo_palette = (void *)(info + 1);
>>> + info->pseudo_palette = (void *) par->palette;
>>
>> I think coding-style is this (i.e., no whitespace):
>> info->pseudo_palette = (void*)par->palette;
>
> <casts-are-evil>
> Is this cast even needed?
> </casts-are-evil>
"pseudo_palette" is "void*", so not at all.
Thanks
David
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Grant Likely @ 2014-08-13 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4ReOB87RX8VhnW1Ln=SK3Tw5V962ujcsfuz-2E9Uc61yQ@mail.gmail.com>
On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> The next commit will handle clocks correctly, so that these do not get
>>> automatically disabled on certain SoC simplefb implementations. As a
>>> result, the removal of this simplefb driver, and the release of the
>>> clocks, is rather final, and only a full display driver can work after
>>> this. So, it makes sense to also flag the dt node as disabled, even
>>> though it has no real value today.
>>>
>>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>>
>> Please, no.
>>
>> Drivers should not be modifying the device tree without and
>> exceptionally good reason for doing so. Drivers are to treat the DT as
>> immutable.
>>
>> * the exception is an overlay driver which add new devices to the
>> kernel. Definitely not the case here.
>
> Why?
The majority of the DT code is based on the assumption of a static
tree. Pantelis has been working on being able to modify it at runtime
with overlays, but he has had to go through a lot of rework because it
is not a trivial task. When you get into modifying the DT, you need to
have a lot more understanding of the side effects to changing the
tree. The DT structure also has a lifecycle that can go beyond the
current lifecycle of the kernel. The kexec tool will extract the
current tree from the kernel, make the appropriate modifications, and
use that to boot the next kernel. Allowing any driver to modify the
tree has side effects beyond just the current kernel.
In this specific case, it will interact badly with the work Pantelis
is doing to make platform devices work with overlays. Modifying the
status property will cause the associated struct device to get removed
in the middle of probing a driver for that device! That will most
likely cause an oops.
Besides, Luc straight out *said*: "...even though it has no real value
today". In what circumstance is that justification for modifying the
tree?
> I think we have exactly that case:
> * DT describes the real hw properly and those parts are immutable
> * Additionally, bootloaders create firmware-framebuffers and
> create simple-framebuffer devices for them. Those are
> valid as long as no driver reconfigured the real hw.
> * Once a real hw-driver loads, it might destroy the existing
> framebuffers, thus, it should also destroy the platform device.
> * If the real hw-driver is unloaded, it might re-create the FB
> and thus create a new (or enable the old) platform device.
>
> Or, in a nutshell: A "simple-framebuffer" device is basically a
> platform-device for framebuffers. Framebuffers can be created and
> destroyed during runtime. The reason we create platform-devices for
> them, is to allow dummy drivers to be probed. Real hw-drivers
> obviously bind to the parent bus device.
>
> Other ideas are obviously welcome, but so far all of the other ideas
> sounded like big hacks (like remove_conflicting_framebuffers() so
> far..).
>
> Thanks
> David
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: David Herrmann @ 2014-08-13 9:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6vQrhd8QOAyGnjRppmuANpT60Ao2+KmkDfF4FNeoZSZgw@mail.gmail.com>
Hi
On Wed, Aug 13, 2014 at 11:23 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>> * the exception is an overlay driver which add new devices to the
>>> kernel. Definitely not the case here.
>>
>> Why?
>
> The majority of the DT code is based on the assumption of a static
> tree. Pantelis has been working on being able to modify it at runtime
> with overlays, but he has had to go through a lot of rework because it
> is not a trivial task. When you get into modifying the DT, you need to
> have a lot more understanding of the side effects to changing the
> tree. The DT structure also has a lifecycle that can go beyond the
> current lifecycle of the kernel. The kexec tool will extract the
> current tree from the kernel, make the appropriate modifications, and
> use that to boot the next kernel. Allowing any driver to modify the
> tree has side effects beyond just the current kernel.
Ok, fair enough. So we leave the DT untouched. That still allows
calling device_add() / device_del() on platform-devices, right?
> In this specific case, it will interact badly with the work Pantelis
> is doing to make platform devices work with overlays. Modifying the
> status property will cause the associated struct device to get removed
> in the middle of probing a driver for that device! That will most
> likely cause an oops.
>
> Besides, Luc straight out *said*: "...even though it has no real value
> today". In what circumstance is that justification for modifying the
> tree?
Sorry, I wasn't clear enough: I'm not arguing in favor of this patch.
I just want to figure out what to do once we implement
hardware-handover for graphics devices on non-x86 (which this series
is kinda preparing for). This patch just reminded me, that we could do
this on a DT level, instead of driver-core level. But I'm fine with
avoiding that, if you warn about complications.
Thanks
David
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 9:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6vQrhd8QOAyGnjRppmuANpT60Ao2+KmkDfF4FNeoZSZgw@mail.gmail.com>
On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>
> The majority of the DT code is based on the assumption of a static
> tree. Pantelis has been working on being able to modify it at runtime
> with overlays, but he has had to go through a lot of rework because it
> is not a trivial task. When you get into modifying the DT, you need to
> have a lot more understanding of the side effects to changing the
> tree. The DT structure also has a lifecycle that can go beyond the
> current lifecycle of the kernel. The kexec tool will extract the
> current tree from the kernel, make the appropriate modifications, and
> use that to boot the next kernel. Allowing any driver to modify the
> tree has side effects beyond just the current kernel.
>
> In this specific case, it will interact badly with the work Pantelis
> is doing to make platform devices work with overlays. Modifying the
> status property will cause the associated struct device to get removed
> in the middle of probing a driver for that device! That will most
> likely cause an oops.
>
> Besides, Luc straight out *said*: "...even though it has no real value
> today". In what circumstance is that justification for modifying the
> tree?
With that sentence i meant that given the current state of things, it
has no real value.
It has no value currently as re-probing simplefb is not going to happen.
But it's not a big leap to turn simplefb into a proper module. Not that
that makes much sense, but that's never stopped anyone.
To me it seemed simple, dt is what drives simplefb, so dt then also
becomes responsible for making sure that simplefb or another driver does
not attempt to blindly use this info again. The way this is implemented
i do not care for in any way, i just knew that i could not do nothing
here, given the catastrophic effect disabling the clocks has on simplefb
on sunxi. Given the discussion that errupted here, i'd say that this
does need some resolution, and altering the dt is going to have to be
part of the solution.
In any case, i will gladly drop this patch, as it is not absolutely
necessary. But it should be very clear that there is no going back on
this dt node after the clocks were released once.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] simplefb: add clock handling
From: Koen Kooi @ 2014-08-13 10:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EB238B.5010404@redhat.com>
Op 13 aug. 2014, om 10:36 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:
> Hi,
>
> On 08/13/2014 10:21 AM, Koen Kooi wrote:
>>
>> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
>>
>>> Hi
>>>
>>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>> This is needed for the sunxi platform, where the u-boot initialized display
>>>> engine gets disabled by the clocks framework if certain clocks are not
>>>> claimed. Once these clocks are disabled, register content is lost, and there
>>>> is no turning back unless a full display driver is loaded, which kind of
>>>> beats the purpose of having simplefb running.
>>>>
>>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>>> has kept these sort of issues from the kernel.
>>>>
>>>> The sunxi u-boot side code can be found here:
>>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>>
>>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>>> since the status property in dt is only really evaluated when dealing with a
>>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>>> node as disabled, as we really have no way back when the clocks get disabled.
>>>
>>> Hans de Goede shortly told me about this and, tbh, I am not very
>>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>>> patch now adds hardware-specific features. To be fair, the patch is
>>> simple and clocks are easy to handle, but I somehow fear we have to
>>> add more and more hardware-support that is required to keep the
>>> framebuffer active. This really defeats the purpose of simplefb.
>>>
>>> The biggest question I have, is why do you add the clocks to your DT
>>> at all? The framebuffer is set up by your boot-loader (or maybe
>>> platform code) and should prepare the clocks. I don't see why we add
>>> the clocks to DT now. If they're not added, then no-one will disable
>>> them and simplefb works just fine, right?
>>
>> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch.
>> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work.
>
> I believe that having some simple generic fb driver will be useful
> on non x86, since we don't have vga-console there, and most distros
> will build kms drivers as modules. Having the kernel / initrd code being
> able to show output (like e.g. missing symbols in the kms drivers) seems
> a very useful feature to me.
>
> The way I envision this to work is:
>
> u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
> it can show this on the display
>
> kernel takes over using something like simplefb (built into the kernel)
> for its initial output / any error messages.
>
> initrd loads kms, kms takes over.
>
> This way we've a way to show error messages during boot at all times.
>
> As we start supporting more ARM htpc boxes out of the box, telling the
> user to hook up a serial console (which often involves soldering wires
> to some test points) when things don't work really is not a viable
> answer.
So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?
Worse, the experience I have with ARM DRM drivers is that they fail horrible when being built as modules, but that's a different problem.
regards,
Koen
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 10:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813094524.GC31326@skynet.be>
On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
> >
> > The majority of the DT code is based on the assumption of a static
> > tree. Pantelis has been working on being able to modify it at runtime
> > with overlays, but he has had to go through a lot of rework because it
> > is not a trivial task. When you get into modifying the DT, you need to
> > have a lot more understanding of the side effects to changing the
> > tree. The DT structure also has a lifecycle that can go beyond the
> > current lifecycle of the kernel. The kexec tool will extract the
> > current tree from the kernel, make the appropriate modifications, and
> > use that to boot the next kernel. Allowing any driver to modify the
> > tree has side effects beyond just the current kernel.
> >
> > In this specific case, it will interact badly with the work Pantelis
> > is doing to make platform devices work with overlays. Modifying the
> > status property will cause the associated struct device to get removed
> > in the middle of probing a driver for that device! That will most
> > likely cause an oops.
> >
> > Besides, Luc straight out *said*: "...even though it has no real value
> > today". In what circumstance is that justification for modifying the
> > tree?
>
> With that sentence i meant that given the current state of things, it
> has no real value.
>
> It has no value currently as re-probing simplefb is not going to happen.
> But it's not a big leap to turn simplefb into a proper module. Not that
> that makes much sense, but that's never stopped anyone.
>
> To me it seemed simple, dt is what drives simplefb, so dt then also
> becomes responsible for making sure that simplefb or another driver does
> not attempt to blindly use this info again. The way this is implemented
> i do not care for in any way, i just knew that i could not do nothing
> here, given the catastrophic effect disabling the clocks has on simplefb
> on sunxi. Given the discussion that errupted here, i'd say that this
> does need some resolution, and altering the dt is going to have to be
> part of the solution.
>
> In any case, i will gladly drop this patch, as it is not absolutely
> necessary. But it should be very clear that there is no going back on
> this dt node after the clocks were released once.
>
> Luc Verhaegen.
What about approaching this from the other end? U-Boot could add a
property named "once-only" or so.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] simplefb: add clock handling
From: David Herrmann @ 2014-08-13 10:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <958E7D39-A034-42AC-A6FF-130609E46967@dominion.thruhere.net>
Hi
On Wed, Aug 13, 2014 at 12:16 PM, Koen Kooi <koen@dominion.thruhere.net> wrote:
> So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?
> Worse, the experience I have with ARM DRM drivers is that they fail horrible when being built as modules, but that's a different problem.
Exactly. But there's no intention to stop building them as modules.
Imagine you build a kernel that's supposed to run on multiple
different platforms (like x86), you really don't want all DRM drivers
built-in. Instead, you load the correct driver during boot-up. To
still provide early graphics access, we use simplefb.
Note that there might be legitimate reasons to make DRM drivers
built-in. But at least general purpose distros avoid that.
Thanks
David
^ permalink raw reply
* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Mark Brown @ 2014-08-13 11:04 UTC (permalink / raw)
To: Jyri Sarha
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <cover.1407503813.git.jsarha@ti.com>
[-- Attachment #1: Type: text/plain, Size: 338 bytes --]
On Fri, Aug 08, 2014 at 04:23:41PM +0300, Jyri Sarha wrote:
> Question to ASoC maintainers:
> Do feel it is Ok to put ASoC code outside sound/soc the way I do
> in these patches (in patch 0006 particularly)?
I'm not thrilled about the idea, it's going to result in breakage during
API updates and more cross tree coordination needs.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] simplefb: add clock handling
From: Hans de Goede @ 2014-08-13 11:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <958E7D39-A034-42AC-A6FF-130609E46967@dominion.thruhere.net>
Hi,
On 08/13/2014 12:16 PM, Koen Kooi wrote:
>
> Op 13 aug. 2014, om 10:36 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:
>
>> Hi,
>>
>> On 08/13/2014 10:21 AM, Koen Kooi wrote:
>>>
>>> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
>>>
>>>> Hi
>>>>
>>>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>> This is needed for the sunxi platform, where the u-boot initialized display
>>>>> engine gets disabled by the clocks framework if certain clocks are not
>>>>> claimed. Once these clocks are disabled, register content is lost, and there
>>>>> is no turning back unless a full display driver is loaded, which kind of
>>>>> beats the purpose of having simplefb running.
>>>>>
>>>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>>>> has kept these sort of issues from the kernel.
>>>>>
>>>>> The sunxi u-boot side code can be found here:
>>>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>>>
>>>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>>>> since the status property in dt is only really evaluated when dealing with a
>>>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>>>> node as disabled, as we really have no way back when the clocks get disabled.
>>>>
>>>> Hans de Goede shortly told me about this and, tbh, I am not very
>>>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>>>> patch now adds hardware-specific features. To be fair, the patch is
>>>> simple and clocks are easy to handle, but I somehow fear we have to
>>>> add more and more hardware-support that is required to keep the
>>>> framebuffer active. This really defeats the purpose of simplefb.
>>>>
>>>> The biggest question I have, is why do you add the clocks to your DT
>>>> at all? The framebuffer is set up by your boot-loader (or maybe
>>>> platform code) and should prepare the clocks. I don't see why we add
>>>> the clocks to DT now. If they're not added, then no-one will disable
>>>> them and simplefb works just fine, right?
>>>
>>> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch.
>>> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work.
>>
>> I believe that having some simple generic fb driver will be useful
>> on non x86, since we don't have vga-console there, and most distros
>> will build kms drivers as modules. Having the kernel / initrd code being
>> able to show output (like e.g. missing symbols in the kms drivers) seems
>> a very useful feature to me.
>>
>> The way I envision this to work is:
>>
>> u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
>> it can show this on the display
>>
>> kernel takes over using something like simplefb (built into the kernel)
>> for its initial output / any error messages.
>>
>> initrd loads kms, kms takes over.
>>
>> This way we've a way to show error messages during boot at all times.
>>
>> As we start supporting more ARM htpc boxes out of the box, telling the
>> user to hook up a serial console (which often involves soldering wires
>> to some test points) when things don't work really is not a viable
>> answer.
>
> So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?
Right, except that that is not going to happen, see David's reply also.
Regards,
Hans
^ permalink raw reply
* [PATCH] backlight: remove .owner field for drivers using module_platform_driver
From: Peter Griffin @ 2014-08-13 12:38 UTC (permalink / raw)
To: linux-arm-kernel
This patch removes the superflous .owner field for drivers which
use the module_platform_driver or platform_driver_register api,
as this is overriden in __platform_driver_register.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
drivers/video/backlight/88pm860x_bl.c | 1 -
drivers/video/backlight/aat2870_bl.c | 1 -
drivers/video/backlight/adp5520_bl.c | 1 -
drivers/video/backlight/as3711_bl.c | 1 -
drivers/video/backlight/da903x_bl.c | 1 -
drivers/video/backlight/da9052_bl.c | 1 -
drivers/video/backlight/ep93xx_bl.c | 1 -
drivers/video/backlight/gpio_backlight.c | 1 -
drivers/video/backlight/lm3533_bl.c | 1 -
drivers/video/backlight/lp8788_bl.c | 1 -
drivers/video/backlight/max8925_bl.c | 1 -
drivers/video/backlight/ot200_bl.c | 1 -
drivers/video/backlight/pandora_bl.c | 1 -
drivers/video/backlight/platform_lcd.c | 1 -
drivers/video/backlight/pwm_bl.c | 1 -
drivers/video/backlight/tps65217_bl.c | 1 -
drivers/video/backlight/wm831x_bl.c | 1 -
17 files changed, 17 deletions(-)
diff --git a/drivers/video/backlight/88pm860x_bl.c b/drivers/video/backlight/88pm860x_bl.c
index 7db5234..6214fc6 100644
--- a/drivers/video/backlight/88pm860x_bl.c
+++ b/drivers/video/backlight/88pm860x_bl.c
@@ -265,7 +265,6 @@ static int pm860x_backlight_probe(struct platform_device *pdev)
static struct platform_driver pm860x_backlight_driver = {
.driver = {
.name = "88pm860x-backlight",
- .owner = THIS_MODULE,
},
.probe = pm860x_backlight_probe,
};
diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
index ec5350f..5e6401b 100644
--- a/drivers/video/backlight/aat2870_bl.c
+++ b/drivers/video/backlight/aat2870_bl.c
@@ -217,7 +217,6 @@ static int aat2870_bl_remove(struct platform_device *pdev)
static struct platform_driver aat2870_bl_driver = {
.driver = {
.name = "aat2870-backlight",
- .owner = THIS_MODULE,
},
.probe = aat2870_bl_probe,
.remove = aat2870_bl_remove,
diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
index f37097a..b56cb3f 100644
--- a/drivers/video/backlight/adp5520_bl.c
+++ b/drivers/video/backlight/adp5520_bl.c
@@ -374,7 +374,6 @@ static SIMPLE_DEV_PM_OPS(adp5520_bl_pm_ops, adp5520_bl_suspend,
static struct platform_driver adp5520_bl_driver = {
.driver = {
.name = "adp5520-backlight",
- .owner = THIS_MODULE,
.pm = &adp5520_bl_pm_ops,
},
.probe = adp5520_bl_probe,
diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
index bb1fc45..734a915 100644
--- a/drivers/video/backlight/as3711_bl.c
+++ b/drivers/video/backlight/as3711_bl.c
@@ -467,7 +467,6 @@ static int as3711_backlight_probe(struct platform_device *pdev)
static struct platform_driver as3711_backlight_driver = {
.driver = {
.name = "as3711-backlight",
- .owner = THIS_MODULE,
},
.probe = as3711_backlight_probe,
};
diff --git a/drivers/video/backlight/da903x_bl.c b/drivers/video/backlight/da903x_bl.c
index 12c5d84..f793738 100644
--- a/drivers/video/backlight/da903x_bl.c
+++ b/drivers/video/backlight/da903x_bl.c
@@ -162,7 +162,6 @@ static int da903x_backlight_probe(struct platform_device *pdev)
static struct platform_driver da903x_backlight_driver = {
.driver = {
.name = "da903x-backlight",
- .owner = THIS_MODULE,
},
.probe = da903x_backlight_probe,
};
diff --git a/drivers/video/backlight/da9052_bl.c b/drivers/video/backlight/da9052_bl.c
index 20d55be..d4bd74bd 100644
--- a/drivers/video/backlight/da9052_bl.c
+++ b/drivers/video/backlight/da9052_bl.c
@@ -173,7 +173,6 @@ static struct platform_driver da9052_wled_driver = {
.id_table = da9052_wled_ids,
.driver = {
.name = "da9052-wled",
- .owner = THIS_MODULE,
},
};
diff --git a/drivers/video/backlight/ep93xx_bl.c b/drivers/video/backlight/ep93xx_bl.c
index 0d1f633..0067931 100644
--- a/drivers/video/backlight/ep93xx_bl.c
+++ b/drivers/video/backlight/ep93xx_bl.c
@@ -128,7 +128,6 @@ static SIMPLE_DEV_PM_OPS(ep93xxbl_pm_ops, ep93xxbl_suspend, ep93xxbl_resume);
static struct platform_driver ep93xxbl_driver = {
.driver = {
.name = "ep93xx-bl",
- .owner = THIS_MODULE,
.pm = &ep93xxbl_pm_ops,
},
.probe = ep93xxbl_probe,
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 1cea688..ff2d540 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -157,7 +157,6 @@ static struct of_device_id gpio_backlight_of_match[] = {
static struct platform_driver gpio_backlight_driver = {
.driver = {
.name = "gpio-backlight",
- .owner = THIS_MODULE,
.of_match_table = of_match_ptr(gpio_backlight_of_match),
},
.probe = gpio_backlight_probe,
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index cff1fbe..0e2337f 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -397,7 +397,6 @@ static void lm3533_bl_shutdown(struct platform_device *pdev)
static struct platform_driver lm3533_bl_driver = {
.driver = {
.name = "lm3533-backlight",
- .owner = THIS_MODULE,
.pm = &lm3533_bl_pm_ops,
},
.probe = lm3533_bl_probe,
diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
index daba34d..a8eeeb4 100644
--- a/drivers/video/backlight/lp8788_bl.c
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -321,7 +321,6 @@ static struct platform_driver lp8788_bl_driver = {
.remove = lp8788_backlight_remove,
.driver = {
.name = LP8788_DEV_BACKLIGHT,
- .owner = THIS_MODULE,
},
};
module_platform_driver(lp8788_bl_driver);
diff --git a/drivers/video/backlight/max8925_bl.c b/drivers/video/backlight/max8925_bl.c
index 66fa08c..7b738d6 100644
--- a/drivers/video/backlight/max8925_bl.c
+++ b/drivers/video/backlight/max8925_bl.c
@@ -197,7 +197,6 @@ static int max8925_backlight_probe(struct platform_device *pdev)
static struct platform_driver max8925_backlight_driver = {
.driver = {
.name = "max8925-backlight",
- .owner = THIS_MODULE,
},
.probe = max8925_backlight_probe,
};
diff --git a/drivers/video/backlight/ot200_bl.c b/drivers/video/backlight/ot200_bl.c
index f5a5202..3acdb9f 100644
--- a/drivers/video/backlight/ot200_bl.c
+++ b/drivers/video/backlight/ot200_bl.c
@@ -152,7 +152,6 @@ static int ot200_backlight_remove(struct platform_device *pdev)
static struct platform_driver ot200_backlight_driver = {
.driver = {
.name = "ot200-backlight",
- .owner = THIS_MODULE,
},
.probe = ot200_backlight_probe,
.remove = ot200_backlight_remove,
diff --git a/drivers/video/backlight/pandora_bl.c b/drivers/video/backlight/pandora_bl.c
index 2098c5d..b515c50 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -148,7 +148,6 @@ static int pandora_backlight_probe(struct platform_device *pdev)
static struct platform_driver pandora_backlight_driver = {
.driver = {
.name = "pandora-backlight",
- .owner = THIS_MODULE,
},
.probe = pandora_backlight_probe,
};
diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c
index c3d2e20..872a3bf 100644
--- a/drivers/video/backlight/platform_lcd.c
+++ b/drivers/video/backlight/platform_lcd.c
@@ -148,7 +148,6 @@ MODULE_DEVICE_TABLE(of, platform_lcd_of_match);
static struct platform_driver platform_lcd_driver = {
.driver = {
.name = "platform-lcd",
- .owner = THIS_MODULE,
.pm = &platform_lcd_pm_ops,
.of_match_table = of_match_ptr(platform_lcd_of_match),
},
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 38ca88b..77ee475 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -398,7 +398,6 @@ static const struct dev_pm_ops pwm_backlight_pm_ops = {
static struct platform_driver pwm_backlight_driver = {
.driver = {
.name = "pwm-backlight",
- .owner = THIS_MODULE,
.pm = &pwm_backlight_pm_ops,
.of_match_table = of_match_ptr(pwm_backlight_of_match),
},
diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
index 595dcf5..c745df3 100644
--- a/drivers/video/backlight/tps65217_bl.c
+++ b/drivers/video/backlight/tps65217_bl.c
@@ -329,7 +329,6 @@ static int tps65217_bl_probe(struct platform_device *pdev)
static struct platform_driver tps65217_bl_driver = {
.probe = tps65217_bl_probe,
.driver = {
- .owner = THIS_MODULE,
.name = "tps65217-bl",
},
};
diff --git a/drivers/video/backlight/wm831x_bl.c b/drivers/video/backlight/wm831x_bl.c
index 8b9455e..6f59616 100644
--- a/drivers/video/backlight/wm831x_bl.c
+++ b/drivers/video/backlight/wm831x_bl.c
@@ -217,7 +217,6 @@ static int wm831x_backlight_probe(struct platform_device *pdev)
static struct platform_driver wm831x_backlight_driver = {
.driver = {
.name = "wm831x-backlight",
- .owner = THIS_MODULE,
},
.probe = wm831x_backlight_probe,
};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Jyri Sarha @ 2014-08-13 12:51 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <20140813110425.GL17528@sirena.org.uk>
On 08/13/2014 02:04 PM, Mark Brown wrote:
> On Fri, Aug 08, 2014 at 04:23:41PM +0300, Jyri Sarha wrote:
>> Question to ASoC maintainers:
>> Do feel it is Ok to put ASoC code outside sound/soc the way I do
>> in these patches (in patch 0006 particularly)?
>
> I'm not thrilled about the idea, it's going to result in breakage during
> API updates and more cross tree coordination needs.
>
I guess then the least bad alternative would be cutting the cpu-dai
driver away from drivers/video/fbdev/omap2/dss/hdmi_audio.c and placing
it under sound/soc/omap and registering it from hdmi_audio.c the same
way as hdmi-audio-codec and asoc-simple-card is now registered.
There would be two devices for a single ip, but at least the ASoC side
driver would receive its resources directly from the HDMI video driver
with necessary tools to synchronize the access.
This approach would work for other HDMI audio situation too, where there
would usually (for tda998x and SiI9022 at least) be a need to register
an i2s codec driver associated with the HDMI encoder. It would probably
be possible to make a single codec driver to work with multiple HDMI
encoders if the API between the codec and endcoder drivers is designed
that in mind.
How does this sound?
Best regards,
Jyri
^ 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