* [PATCH v4 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-1-git-send-email-hdegoede@redhat.com>
Update simplefb to support the new preferred location for simplefb dt nodes
under /chosen.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--
Changes in v2:
-Make name array larger in case we ever encounter more then 10000 framebuffers
Changes in v3:
-Switch to for_each_child_of_node to loop over the nodes under chosen
Changes in v4:
-Wrap the chosen enumeration code in #ifdef CONFIG_OF, as simplefb is used on
non devicetree platforms too
-simplefb can only be built-in, so drop the module_exit function calling
platform_driver_unregister
---
drivers/video/fbdev/simplefb.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 868099a..e381a53 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -27,6 +27,7 @@
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
#include <linux/clk-provider.h>
+#include <linux/of_platform.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -390,7 +391,27 @@ static struct platform_driver simplefb_driver = {
.probe = simplefb_probe,
.remove = simplefb_remove,
};
-module_platform_driver(simplefb_driver);
+
+static int __init simplefb_init(void)
+{
+ int ret;
+ struct device_node __maybe_unused *np;
+
+ ret = platform_driver_register(&simplefb_driver);
+ if (ret)
+ return ret;
+
+#ifdef CONFIG_OF
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+#endif
+
+ return 0;
+}
+
+module_init(simplefb_init);
MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.1.0
^ permalink raw reply related
* [PATCH v4 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.
Also specify when to set the chosen stdout-path property to a simplefb node.
For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.
Last add some advice on how to fill and use simplefb nodes from a firmware
pov.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
--
Changes in v2:
-Add stdout-path to the example code
Changes in v3:
-Specify that the node name must be "framebuffer@<address>"
-Specify that the link to link simplefb node and display hw nodes to one
another for handover goes into the simplefb node, and must be called "display"
-Specify how aliases may be used to tell the OS how to number displays
-Update the example to reflect these changes
Changes in v4:
-Keep the size of the reg property in the example as it was before
---
.../bindings/video/simple-framebuffer.txt | 49 +++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..c548f33 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,31 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
the bootloader, with the assumption that the display hardware has already
been set up to scan out from the memory pointed to by the reg property.
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). Simplefb nodes must be named "framebuffer@<address>".
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then the simplefb node must contain a property called "display", which
+contains a phandle pointing to the primary display hw node, so that the OS
+knows which simplefb to disable when handing over control to a driver for the
+real hardware. The bindings for the hw nodes must specify which node is
+considered the primary node.
+
+It is advised to add display# aliases to help the OS determine how to number
+things. If display# aliases are used, then if the simplefb node contains a
+"display" property then the /aliases/display# path must point to the display
+hw node the "display" property points to, otherwise it must point directly
+to the simplefb node.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
Required properties:
- compatible: "simple-framebuffer"
- reg: Should contain the location and size of the framebuffer memory.
@@ -19,14 +44,36 @@ Optional properties:
are expected to already be configured correctly. The OS must
ensure these clocks are not modified or disabled while the
simple framebuffer remains active.
+- display : phandle pointing to the primary display hardware node
Example:
- framebuffer {
+aliases {
+ display0 = &lcdc0;
+}
+
+chosen {
+ framebuffer0: framebuffer@1d385000 {
compatible = "simple-framebuffer";
reg = <0x1d385000 (1600 * 1200 * 2)>;
width = <1600>;
height = <1200>;
stride = <(1600 * 2)>;
format = "r5g6b5";
+ clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
+ display = <&lcdc0>;
+ };
+ stdout-path = &framebuffer0;
+};
+
+soc@01c00000 {
+ lcdc0: lcdc@1c0c000 {
+ compatible = "allwinner,sun4i-a10-lcdc";
+ ...
};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any compatible
+nodes found under chosen and then check for other compatible nodes.
--
2.1.0
^ permalink raw reply related
* [PATCH v6] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-13 19:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Luc Verhaegen <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>
[hdegoede@redhat.com: Change clks from list to dynamic array]
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
--
Changes in v4:
-change clks from linkedlist to dynamic allocated array
-propagate EPROBE_DEFER up from simplefb_clocks_init to simplefb_probe
changes in v6:
-Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
devicetree platforms too
---
drivers/video/fbdev/simplefb.c | 106 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 105 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cdcf1fe..868099a 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",
@@ -167,8 +168,103 @@ static int simplefb_parse_pd(struct platform_device *pdev,
struct simplefb_par {
u32 palette[PSEUDO_PALETTE_SIZE];
+#ifdef CONFIG_OF
+ int clk_count;
+ struct clk **clks;
+#endif
};
+/*
+ * 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.
+ *
+ * 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 int
+simplefb_clocks_init(struct simplefb_par *par, struct platform_device *pdev)
+{
+#ifdef CONFIG_OF
+ struct device_node *np = pdev->dev.of_node;
+ struct clk *clock;
+ int i, ret;
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return 0;
+
+ par->clk_count = of_clk_get_parent_count(np);
+ if (par->clk_count <= 0)
+ return 0;
+
+ par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
+ if (!par->clks)
+ return -ENOMEM;
+
+ for (i = 0; i < par->clk_count; i++) {
+ clock = of_clk_get(np, i);
+ if (IS_ERR(clock)) {
+ if (PTR_ERR(clock) = -EPROBE_DEFER) {
+ while (--i >= 0) {
+ if (par->clks[i])
+ clk_put(par->clks[i]);
+ }
+ kfree(par->clks);
+ return -EPROBE_DEFER;
+ }
+ dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+ __func__, i, PTR_ERR(clock));
+ continue;
+ }
+ par->clks[i] = clock;
+ }
+
+ for (i = 0; i < par->clk_count; i++) {
+ if (par->clks[i]) {
+ ret = clk_prepare_enable(par->clks[i]);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s: failed to enable clock %d: %d\n",
+ __func__, i, ret);
+ clk_put(par->clks[i]);
+ par->clks[i] = NULL;
+ }
+ }
+ }
+#endif
+ return 0;
+}
+
+static void
+simplefb_clocks_destroy(struct simplefb_par *par)
+{
+#ifdef CONFIG_OF
+ int i;
+
+ if (!par->clks)
+ return;
+
+ for (i = 0; i < par->clk_count; i++) {
+ if (par->clks[i]) {
+ clk_disable_unprepare(par->clks[i]);
+ clk_put(par->clks[i]);
+ }
+ }
+
+ kfree(par->clks);
+#endif
+}
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
@@ -236,6 +332,10 @@ static int simplefb_probe(struct platform_device *pdev)
}
info->pseudo_palette = par->palette;
+ ret = simplefb_clocks_init(par, pdev);
+ if (ret < 0)
+ goto error_unmap;
+
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);
@@ -247,13 +347,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_clocks:
+ simplefb_clocks_destroy(par);
error_unmap:
iounmap(info->screen_base);
error_fb_release:
@@ -264,8 +366,10 @@ error_fb_release:
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);
framebuffer_release(info);
return 0;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register
From: Tony Lindgren @ 2014-11-13 18:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464969F.30708@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> On 12/11/14 17:02, Tony Lindgren wrote:
>
> >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> >> places in the kernel. I wonder if adding a pinmux entry for it could
> >> cause some rather odd problems.
> >
> > They can all use pinctrl-single no problem.
>
> Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> and we have code in mach-omap2 that also touch DEVCONF1, without any
> knowledge (and locking) between those...
Hmm yeah the McBSP clock mux could be racy as the mux register for
McBSP is treated as a clock. This register muxes the clock between
external pin and internal clock. Considering that this should be
selectable at board level as the external clock probably needs to be
used if level shifters are being used, it should be really handled by
pinctrl-single.
The other use for hsmmc.c and pdata-quirks.c for the one time mux for
MMC clock from the MMC clock pin. That can be done with pinctrl-single
from the MMC driver too for DT based booting.
Then we just have the save and restore of the registers for
off-idle.
> So _maybe_ that's not an issue, as the pinmux config we have here is
> fixed, and done once at boot time, and maybe the code in mach-omap2 that
> touch DEVCONF1 is also ran just once and not at the same time as the
> pinmux. But I don't know if that's so.
It seems we could just do a read-only check for McBSP in the clock
code for the mux register, or even completely drop that code from
cclock3xxx_data.c and start using the pinctrl for that mux.
Paul & Tero, got any comments here?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v5 0/5] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-13 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464CC5A.7010508@ti.com>
Hi,
On 11/13/2014 04:20 PM, Tomi Valkeinen wrote:
> On 12/11/14 18:15, Hans de Goede wrote:
>> Hi Tomi,
>>
>> Here is v5 of my simplefb patch-set, this time with an Ack from
>> Grant Likely for the dt-bindings bits, which should end all controversy
>> surrounding that patch.
>>
>> For changes since the original postings please see the per patch changelogs
>> in the patch commit messages.
>>
>> Please merge this into your tree for 3.19.
>
> I applied this and the "dt-bindings: simplefb: Specify node location and
> handoff related properties" v3 series to my test branch, compiled for
> x64 and:
>
> drivers/video/fbdev/simplefb.c: In function ‘simplefb_clocks_init’:
> drivers/video/fbdev/simplefb.c:204:2: error: implicit declaration of
> function ‘of_clk_get_parent_count’ [-Werror=implicit-function-declaration]
> par->clk_count = of_clk_get_parent_count(np);
> ^
> In file included from include/linux/i2c.h:33:0,
> from include/uapi/linux/fb.h:5,
> from include/linux/fb.h:5,
> from drivers/video/fbdev/simplefb.c:24:
> drivers/video/fbdev/simplefb.c: In function ‘simplefb_init’:
> drivers/video/fbdev/simplefb.c:399:25: error: ‘of_chosen’ undeclared
> (first use in this function)
> for_each_child_of_node(of_chosen, np) {
> ^
> include/linux/of.h:736:33: note: in definition of macro
> ‘for_each_child_of_node’
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> ^
> drivers/video/fbdev/simplefb.c:399:25: note: each undeclared identifier
> is reported only once for each function it appears in
> for_each_child_of_node(of_chosen, np) {
> ^
> include/linux/of.h:736:33: note: in definition of macro
> ‘for_each_child_of_node’
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> ^
>
> We need "depends on OF"?
Good catch!
So a quick-grep through the entire kernel sources has thought me that
apparently simplefb is used on non devicetree / of platforms through
platform-device instantiation, using the trick of naming the platform-device the
same as the driver to get the driver to bind.
So the answer to your compile breakage is to make the relevant code bits
be #ifdef CONFIG_OF
I'll respin the offending patch (5/5) in the "simplefb: add clock handling code"
series, and I'll respin the entire second series, as I need to also address
your comment on the example in the bindings there.
I hope to be able to post a re-spun version of both later tonight.
Regards,
Hans
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH 4/7] ARM: dts: sun6i: Add simplefb node
From: Hans de Goede @ 2014-11-13 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20141113164606.GM20972@lukather>
Hi,
On 11/13/2014 05:46 PM, Maxime Ripard wrote:
> Hi,
>
> On Thu, Nov 13, 2014 at 10:31:46AM +0100, Hans de Goede wrote:
>> Add a simplefb template node for u-boot to further fill and activate.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> arch/arm/boot/dts/sun6i-a31.dtsi | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index 543f895..438952e 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -62,6 +62,16 @@
>> ethernet0 = &gmac;
>> };
>>
>> + chosen {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + framebuffer0 {
>> + compatible = "simple-framebuffer";
>> + status = "disabled";
>
> Aren't we suppose to have clocks in there too?
I deliberately left them out as they are not needed on sun6i, once the blocks
are setup ahb access is not necessary, but it seems that the ahb_gate bits
in sun4i / sun5i / sun7i double as a reset. Toggling them on / off / on breaks
the mode, while toggling module clocks on / off / on only causes a glitch.
Since the sun6i has a separate reset controller, the clock gates seem to really
only control ahb access which is not necessary once the mode has been set up.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/7] ARM: dts: sunxi: Add regulator-boot-on property to ahci-5v regulator
From: Maxime Ripard @ 2014-11-13 16:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415871109-28332-1-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 604 bytes --]
On Thu, Nov 13, 2014 at 10:31:43AM +0100, Hans de Goede wrote:
> This avoids it getting briefly turned off between when the regulator getting
> registered and the ahci driver turning it back on, thus avoiding the disk
> going into emergency head park mode.
>
> Cc: Bruno Prémont <bonbons@linux-vserver.org>
> Reported-and-tested-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, and split up the tags.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 7/7] ARM: dts: sun6i: Add support for the status led
From: Maxime Ripard @ 2014-11-13 16:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415871109-28332-7-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 379 bytes --]
On Thu, Nov 13, 2014 at 10:31:49AM +0100, Hans de Goede wrote:
> The Mele M9 / A1000G quad has a blue status led, add support for this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, with a newline after the pnictrl node.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 6/7] ARM: dts: sun6i: Add EHCI support for the M9 board
From: Maxime Ripard @ 2014-11-13 16:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415871109-28332-6-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 639 bytes --]
On Thu, Nov 13, 2014 at 10:31:48AM +0100, Hans de Goede wrote:
> The Mele M9 / A1000G quad uses both usb-ports, one goes to an internal
> usb wifi card, the other to a build-in usb-hub, so neither need their
> OHCI companion controller to be enabled since the are always connected at
> USB-2 speeds.
>
> The controller which is attached to the wifi also does not need a vbus
> regulator.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, with a newline before the pinctrl node.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] ARM: dts: sun6i: Add simplefb node
From: Maxime Ripard @ 2014-11-13 16:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415871109-28332-4-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 923 bytes --]
Hi,
On Thu, Nov 13, 2014 at 10:31:46AM +0100, Hans de Goede wrote:
> Add a simplefb template node for u-boot to further fill and activate.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> arch/arm/boot/dts/sun6i-a31.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 543f895..438952e 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -62,6 +62,16 @@
> ethernet0 = &gmac;
> };
>
> + chosen {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + framebuffer0 {
> + compatible = "simple-framebuffer";
> + status = "disabled";
Aren't we suppose to have clocks in there too?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
From: Tomi Valkeinen @ 2014-11-13 16:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9FD016B8-4EA2-4A0D-B790-6494AB449B31@goldelico.com>
[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
>
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
>
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the „encoder“ category, because we
> would expect something digital or digital to analog „encoding“ which it does not.
That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.
>>> +
>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
>
> Yes, you are right - this is not systematic.
>
> But the problem is that we can’t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().
You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.
>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
>
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.
It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
From: Dr. H. Nikolaus Schaller @ 2014-11-13 16:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54649B43.1000801@ti.com>
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>
> I think it would be better to rename this to encoder-opa362.c. It's not
When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does not.
> encoder as such, but it falls into the same category.
But we can change it.
>
>> include/video/omap-panel-data.h | 12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>>
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>>
>> +config DISPLAY_AMPLIFIER_OPA362
>
> Here also use ENCODER instead.
>
>> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> + help
>> + Driver to enable an external analog TV amplifier (e.g. OPA362)
>> + through a GPIO.
>
> The indentation above seems funny.
>
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.
Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.
>
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>> help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * 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/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> + struct omap_dss_device dssdev;
>> + struct omap_dss_device *in;
>> +
>> + int enable_gpio;
>> +
>> + struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "connect\n");
>> +
>> + if (omapdss_device_is_connected(dssdev))
>> + return -EBUSY;
>> +
>> + r = in->ops.atv->connect(in, dssdev);
>> + if (r)
>> + return r;
>> +
>> + dst->src = dssdev;
>> + dssdev->dst = dst;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> + WARN_ON(!omapdss_device_is_connected(dssdev));
>> + if (!omapdss_device_is_connected(dssdev))
>> + return;
>> +
>> + WARN_ON(dst != dssdev->dst);
>> + if (dst != dssdev->dst)
>> + return;
>> +
>> + dst->src = NULL;
>> + dssdev->dst = NULL;
>> +
>> + in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "enable\n");
>> +
>> + if (!omapdss_device_is_connected(dssdev))
>> + return -ENODEV;
>> +
>> + if (omapdss_device_is_enabled(dssdev))
>> + return 0;
>> +
>> + in->ops.atv->set_timings(in, &ddata->timings);
>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> + in->ops.atv->invert_vid_out_polarity(in, true);
>
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
>
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
>
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
>
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.
Yes, you are right - this is not systematic.
But the problem is that we can’t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().
What it should be is something like
in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())
but that does not exist and I am not sure if you want to introduce it.
So I only see the option that we always use
in->ops.atv->invert_vid_out_polarity(in, true);
and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.
>
>> +
>> + r = in->ops.atv->enable(in);
>> + if (r)
>> + return r;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disable\n");
>> +
>> + if (!omapdss_device_is_enabled(dssdev))
>> + return;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> + in->ops.atv->disable(in);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> + ddata->timings = *timings;
>> + dssdev->panel.timings = *timings;
>> +
>> + in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> + dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> + *timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> + return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> + enum omap_dss_venc_type type)
>> +{
>> + /* we can only drive a COMPOSITE output */
>> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> + bool invert_polarity)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + /* OPA362 inverts the polarity */
>> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> + .connect = opa362_connect,
>> + .disconnect = opa362_disconnect,
>> +
>> + .enable = opa362_enable,
>> + .disable = opa362_disable,
>> +
>> + .check_timings = opa362_check_timings,
>> + .set_timings = opa362_set_timings,
>> + .get_timings = opa362_get_timings,
>> +
>> + .set_type = opa362_set_type,
>> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct amplifier_opa362_platform_data *pdata;
>> + struct omap_dss_device *dssdev, *in;
>> +
>> + pdata = dev_get_platdata(&pdev->dev);
>> +
>> + ddata->enable_gpio = pdata->enable_gpio;
>> +
>> + in = omap_dss_find_output(pdata->source);
>> + if (in = NULL) {
>> + dev_err(&pdev->dev, "Failed to find video source\n");
>> + return -ENODEV;
>> + }
>> +
>> + ddata->in = in;
>> +
>> + dssdev = &ddata->dssdev;
>> + dssdev->name = pdata->name;
>> +
>> + return 0;
>> +}
>
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.
Is there already a decision? I think it should not be done before. And it
does not harm to still have it.
>
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + int gpio;
>> +
>> + gpio = of_get_gpio(node, 0);
>> +
>> + if (gpio_is_valid(gpio) || gpio = -ENOENT) {
>> + ddata->enable_gpio = gpio;
>> + } else {
>> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> + return gpio;
>> + }
>
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.
Ok.
BR,
Nikolaus
^ permalink raw reply
* Re: [PATCH v5 0/5] simplefb: add clock handling code
From: Tomi Valkeinen @ 2014-11-13 16:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6ssdeOnQnem1012h_jFK1wWOD_WwosjFgiAcgBoqyDt9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On 13/11/14 18:05, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 7:38 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Wed, Nov 12, 2014 at 4:15 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi Tomi,
>>>
>>> Here is v5 of my simplefb patch-set, this time with an Ack from
>>> Grant Likely for the dt-bindings bits, which should end all controversy
>>> surrounding that patch.
>>>
>>> For changes since the original postings please see the per patch changelogs
>>> in the patch commit messages.
>>>
>>> Please merge this into your tree for 3.19.
>>
>> Hi Tomi,
>>
>> I'm discussing this with Rob today. Please hold off 24 hours before
>> merging. If you don't hear anything to the contrary by tomorrow then
>> go ahead and merge.
>
> I just talked to Rob. All is good, you can apply.
Ok. I'll apply tomorrow if there are no more comments during the night
for the new series, and the compilation issue has been solved.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate
From: Grant Likely @ 2014-11-13 16:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464D3C7.7060707@ti.com>
On Thu, Nov 13, 2014 at 3:52 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 13/11/14 16:46, Hans de Goede wrote:
>> Hi,
>>
>> On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
>>> On 13/11/14 15:34, Hans de Goede wrote:
>>>
>>>> +chosen {
>>>> + framebuffer0: framebuffer@5fc00000 {
>>>> compatible = "simple-framebuffer";
>>>> - reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>> + reg = <0x5fc00000 (4096 * 1024)>;
>>>
>>> Was there a reason for this change?
>>
>> I changed the node name to match the new bindings text which
>> specifies that the node name must be framebuffer@<address>,
>> while at it I've taken a real world address range.
>>
>> I assume you refer to the bit where the size of the reg property
>> is changed ? That indeed is not really necessary.
>
> Yes, I meant the size of the reg prop. As this is an example, I think it
> should be as clear as possible.
>
> I guess a valid use case here is to set the size to larger-than-needed,
> so that it gets preallocated and you don't need to worry about getting
> the memory later when the system has already been running for a longer
> time. But if that's the case in this example, I think it would be better
> to describe it explicitly.
Besides, we already have a mechanism for that called /reserved-memory.
Any memory used by a framebuffer needs to be described in the reserved
map anyway, and that region can be made larger than the firmware
configured framebuffer.
g.
^ permalink raw reply
* Re: [PATCH v5 0/5] simplefb: add clock handling code
From: Grant Likely @ 2014-11-13 16:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6ua8OF5+SNjgO3Nc5ZSVvaic3OzrFES-5=f+bsj-fvE3g@mail.gmail.com>
On Thu, Nov 13, 2014 at 7:38 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, Nov 12, 2014 at 4:15 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Tomi,
>>
>> Here is v5 of my simplefb patch-set, this time with an Ack from
>> Grant Likely for the dt-bindings bits, which should end all controversy
>> surrounding that patch.
>>
>> For changes since the original postings please see the per patch changelogs
>> in the patch commit messages.
>>
>> Please merge this into your tree for 3.19.
>
> Hi Tomi,
>
> I'm discussing this with Rob today. Please hold off 24 hours before
> merging. If you don't hear anything to the contrary by tomorrow then
> go ahead and merge.
I just talked to Rob. All is good, you can apply.
g.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate
From: Tomi Valkeinen @ 2014-11-13 15:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464C444.4050101@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]
On 13/11/14 16:46, Hans de Goede wrote:
> Hi,
>
> On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
>> On 13/11/14 15:34, Hans de Goede wrote:
>>
>>> +chosen {
>>> + framebuffer0: framebuffer@5fc00000 {
>>> compatible = "simple-framebuffer";
>>> - reg = <0x1d385000 (1600 * 1200 * 2)>;
>>> + reg = <0x5fc00000 (4096 * 1024)>;
>>
>> Was there a reason for this change?
>
> I changed the node name to match the new bindings text which
> specifies that the node name must be framebuffer@<address>,
> while at it I've taken a real world address range.
>
> I assume you refer to the bit where the size of the reg property
> is changed ? That indeed is not really necessary.
Yes, I meant the size of the reg prop. As this is an example, I think it
should be as clear as possible.
I guess a valid use case here is to set the size to larger-than-needed,
so that it gets preallocated and you don't need to worry about getting
the memory later when the system has already been running for a longer
time. But if that's the case in this example, I think it would be better
to describe it explicitly.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [alsa-devel] [PATCH v7.1 00/19] Rework OMAP4+ HDMI audio support
From: Tomi Valkeinen @ 2014-11-13 15:44 UTC (permalink / raw)
To: Jean-Francois Moine
Cc: Mark Brown, Jyri Sarha, peter.ujfalusi, liam.r.girdwood,
alsa-devel, linux-omap, linux-fbdev
In-Reply-To: <20141113160002.7a1157a5@armhf>
[-- Attachment #1: Type: text/plain, Size: 4763 bytes --]
On 13/11/14 17:00, Jean-Francois Moine wrote:
> When the tda998x is not operational, the CODEC knows it and reports an
> error to the audio subsystem on device open. But, once the tda998x has
> been started, it always stays operational, even without HDMI connection.
What does "started" mean here? tda998x device/driver has been probed?
The reason for the above is probably to handle hotplug? I.e. tda998x
needs to be enabled always to detect HPD?
Usually for OMAP, the HPD detection happens outside the HDMI IP. Thus
the HDMI IP is turned fully off when the video is disabled.
I believe tda998x could be used the same way, if the HPD pin is
connected to a SoC GPIO instead of tda998x.
>>> and I saw only a few dependencies between the 2 subsystems:
>>>
>>> - the CODEC must know the transmitter parameters (DAIs - static -,
>>> audio constraints - dynamic -),
>>
>> The video mode (i.e. availability of audio) or the EDID (i.e. the
>> supported audio parameters) may change at any time, so I presume in your
>> series the video side will inform the codec of these changes at any point?
>
> Such changes occur only when the HDMI output device (TV) is replaced by
The user can change the video mode at any time. The audio data packets
are sent in the video blanking intervals, and if those intervals are too
short, the video mode cannot support audio.
If I'm not mistaken, officially only certain video modes defined in the
HDMI spec support audio.
> an other one (manual cable exchange or HDMI switcher). I wonder if
> these changes are correctly treated in the HDMI transmitters, DRM core,
> DRM drivers, X11/wayland...
HPD (which usually equals to cable change, but not always) is handled.
>> a) Always keep the audio device operational, no matter what is the
>> status of the video side. How should this work if the HDMI videomode or
>> the HDMI monitor does not support audio? Is it desirable that the
>> userspace has no idea that the audio is not actually going anywhere?
>>
>> b) Remove the audio device when audio support is not available. This
>> kind of makes sense, as, well, there's no possibility for audio output.
>> But maybe this is not how linux sound devices are supposed to behave?
>>
>> c) Return errors when playback is attempted when audio support is not
>> available. Again, this kind of makes sense, as audio playback is not
>> possible. But maybe this is also not how audio devices generally work.
>>
>> Jyri, were there some other options we discussed?
>>
>> Currently, the OMAP HDMI version does c). It's the easiest solution for us.
>
> Same for me, but checking the audio capability is done on audio streaming
> start only.
Hmm, I understood you have option a)? You said "I even let audio
streaming start when the HDMI cable is
disconnected".
With "audio support is not available" I mean also the case when the
cable is not connected, as then we don't have EDID information, and thus
we don't have a HDMI monitor with audio support on the other end.
So to clarify, our driver only has "audio support available" if:
- we successfully read EDID
- EDID shows it's a HDMI monitor
- EDID shows it has audio support for the format we support (this we
don't actually do yet)
Otherwise we default to DVI, which means no audio.
If, at any point, the situation changes to "no audio support available",
the audio playback is aborted and starting new audio playback fails.
> AFAIK, the HDMI transmitters don't know if the video or audio is
> displayed/played by the external device(s). Only the user knows.
True.
But the HDMI transmitter can know if the audio absolutely cannot be
played, because there's no cable, or because the monitor says it doesn't
support audio, or because the video mode cannot support audio, or
because the HDMI output stream is disabled.
So the question is, do we want/need to inform the userspace about those
situations? Should a HDMI transmitted always look like it can play
audio, even if it's clear it cannot?
Again, I know very little about audio, but I think it would be nice that
if I open the sound configuration window on my desktop, it'd show me
that the HDMI audio device is disabled (because there's no cable).
But perhaps that's a separate feature. We could have HDMI audio device
always there and functional, but the desktop could get the information
about HDMI cable connection some other way (DRM). And if the cable is
not there, it knows that this particular audio device is also out, and
thus doesn't show it (even if it's functional).
> BTW, you are talking about video modes without audio support. What are
> you thinking about?
This I covered above.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/5] simplefb: add clock handling code
From: Tomi Valkeinen @ 2014-11-13 15:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415808952-23549-1-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]
On 12/11/14 18:15, Hans de Goede wrote:
> Hi Tomi,
>
> Here is v5 of my simplefb patch-set, this time with an Ack from
> Grant Likely for the dt-bindings bits, which should end all controversy
> surrounding that patch.
>
> For changes since the original postings please see the per patch changelogs
> in the patch commit messages.
>
> Please merge this into your tree for 3.19.
I applied this and the "dt-bindings: simplefb: Specify node location and
handoff related properties" v3 series to my test branch, compiled for
x64 and:
drivers/video/fbdev/simplefb.c: In function ‘simplefb_clocks_init’:
drivers/video/fbdev/simplefb.c:204:2: error: implicit declaration of
function ‘of_clk_get_parent_count’ [-Werror=implicit-function-declaration]
par->clk_count = of_clk_get_parent_count(np);
^
In file included from include/linux/i2c.h:33:0,
from include/uapi/linux/fb.h:5,
from include/linux/fb.h:5,
from drivers/video/fbdev/simplefb.c:24:
drivers/video/fbdev/simplefb.c: In function ‘simplefb_init’:
drivers/video/fbdev/simplefb.c:399:25: error: ‘of_chosen’ undeclared
(first use in this function)
for_each_child_of_node(of_chosen, np) {
^
include/linux/of.h:736:33: note: in definition of macro
‘for_each_child_of_node’
for (child = of_get_next_child(parent, NULL); child != NULL; \
^
drivers/video/fbdev/simplefb.c:399:25: note: each undeclared identifier
is reported only once for each function it appears in
for_each_child_of_node(of_chosen, np) {
^
include/linux/of.h:736:33: note: in definition of macro
‘for_each_child_of_node’
for (child = of_get_next_child(parent, NULL); child != NULL; \
^
We need "depends on OF"?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [alsa-devel] [PATCH v7.1 00/19] Rework OMAP4+ HDMI audio support
From: Jean-Francois Moine @ 2014-11-13 14:57 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Mark Brown, Jyri Sarha, peter.ujfalusi, liam.r.girdwood,
alsa-devel, linux-omap, linux-fbdev
In-Reply-To: <54648149.9070104@ti.com>
On Thu, 13 Nov 2014 12:00:41 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
[snip]
> Jyri or Peter knows this better, but I think one difference with OMAP
> HDMI case and tda998x is that tda998x is an external encoder, and you
> transfer audio data to it via i2s or spdif, whereas OMAP HDMI is inside
> the SoC, and the HDMI IP gets the audio data via system DMA.
>
> The system DMA transfers are triggered by the HDMI IP, and the HDMI IP
> has to be enabled and properly configured for the DMA ports to function.
>
> So, correct me if I'm wrong, but I think this is the fundamental difference:
>
> In your case, the actual audio playback happens with the i2s or spdif
> side. You can enable the playback at any time, no matter what is the
> status of tda998x. If tda998x is not operational, the audio data will
> just go to /dev/null.
>
> In our case, the actual audio playback happens inside the HDMI block. We
> need the whole HDMI block to be operational and correctly configured for
> (fake or real) playback to be possible.
When the tda998x is not operational, the CODEC knows it and reports an
error to the audio subsystem on device open. But, once the tda998x has
been started, it always stays operational, even without HDMI connection.
> > and I saw only a few dependencies between the 2 subsystems:
> >
> > - the CODEC must know the transmitter parameters (DAIs - static -,
> > audio constraints - dynamic -),
>
> The video mode (i.e. availability of audio) or the EDID (i.e. the
> supported audio parameters) may change at any time, so I presume in your
> series the video side will inform the codec of these changes at any point?
Such changes occur only when the HDMI output device (TV) is replaced by
an other one (manual cable exchange or HDMI switcher). I wonder if
these changes are correctly treated in the HDMI transmitters, DRM core,
DRM drivers, X11/wayland...
So, no, actually, once streaming is started, no information go from the
HDMI transmitter to the CODEC.
> > - the CODEC must alert the transmitter on audio start and stop.
> >
> > I don't think that stopping audio streaming on HDMI disconnection is
>
> And you allow audio playback also if the monitor does not support audio,
> or the video mode does not supprot audio?
Bug in my patch: audio playback will be rejected if there will be no
audio support.
> > useful. I even let audio streaming start when the HDMI cable is
> > disconnected.
>
> Ah, this is actually unclear thing to me: what should the audio side
> behavior be when the HDMI cable is disconnected or the video is blanked?
> I believe the options are:
>
> a) Always keep the audio device operational, no matter what is the
> status of the video side. How should this work if the HDMI videomode or
> the HDMI monitor does not support audio? Is it desirable that the
> userspace has no idea that the audio is not actually going anywhere?
>
> b) Remove the audio device when audio support is not available. This
> kind of makes sense, as, well, there's no possibility for audio output.
> But maybe this is not how linux sound devices are supposed to behave?
>
> c) Return errors when playback is attempted when audio support is not
> available. Again, this kind of makes sense, as audio playback is not
> possible. But maybe this is also not how audio devices generally work.
>
> Jyri, were there some other options we discussed?
>
> Currently, the OMAP HDMI version does c). It's the easiest solution for us.
Same for me, but checking the audio capability is done on audio streaming
start only.
When the HDMI output device is disconnected, the video and audio outputs
don't need to be changed. I often switch off my TV set when I will not
use my computer for a while, letting the computer itself running. When
I get back and switch on back the TV set, video and audio are still
there.
> Option a) is a bit problematic for us, as it requires the HDMI IP side
> to be fully operational, with the video output configured and enabled,
> as that is what drives the audio DMA. If we stop the video, I believe
> the audio DMA will freeze, and it'll lead to timeouts on the audio side.
>
> I haven't tried, but I believe we could program the HDMI IP to some safe
> default video mode if the cable is disconnected, and turn off the actual
> HDMI PHY, so that the audio side could work even if the HDMI stream is
> not going anywhere. I think it would be quite big change to how the HDMI
> driver works at the moment.
>
> But then, if the cable _is_ connected and the video mode is such that it
> cannot not support audio, I wonder if we can still fake the audio
> playback or will the HDMI IP get confused...
AFAIK, the HDMI transmitters don't know if the video or audio is
displayed/played by the external device(s). Only the user knows.
BTW, you are talking about video modes without audio support. What are
you thinking about?
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
^ permalink raw reply
* Re: [alsa-devel] [PATCH v7.1 00/19] Rework OMAP4+ HDMI audio support
From: Jyri Sarha @ 2014-11-13 14:54 UTC (permalink / raw)
To: Tomi Valkeinen, Jean-Francois Moine
Cc: Mark Brown, peter.ujfalusi, liam.r.girdwood, alsa-devel,
linux-omap, linux-fbdev
In-Reply-To: <54648149.9070104@ti.com>
On 11/13/2014 12:00 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 13/11/14 11:17, Jean-Francois Moine wrote:
>> On Thu, 13 Nov 2014 10:05:28 +0200
>> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
...
>> and I saw only a few dependencies between the 2 subsystems:
>>
>> - the CODEC must know the transmitter parameters (DAIs - static -,
>> audio constraints - dynamic -),
>
> The video mode (i.e. availability of audio) or the EDID (i.e. the
> supported audio parameters) may change at any time, so I presume in your
> series the video side will inform the codec of these changes at any point?
>
>> - the CODEC must alert the transmitter on audio start and stop.
>>
>> I don't think that stopping audio streaming on HDMI disconnection is
>
> And you allow audio playback also if the monitor does not support audio,
> or the video mode does not supprot audio?
>
>> useful. I even let audio streaming start when the HDMI cable is
>> disconnected.
>
> Ah, this is actually unclear thing to me: what should the audio side
> behavior be when the HDMI cable is disconnected or the video is blanked?
> I believe the options are:
>
> a) Always keep the audio device operational, no matter what is the
> status of the video side. How should this work if the HDMI videomode or
> the HDMI monitor does not support audio? Is it desirable that the
> userspace has no idea that the audio is not actually going anywhere?
>
> b) Remove the audio device when audio support is not available. This
> kind of makes sense, as, well, there's no possibility for audio output.
> But maybe this is not how linux sound devices are supposed to behave?
>
> c) Return errors when playback is attempted when audio support is not
> available. Again, this kind of makes sense, as audio playback is not
> possible. But maybe this is also not how audio devices generally work.
>
> Jyri, were there some other options we discussed?
>
There was the 4th option: the driver forcing pause on the pcm when the
cable is disconnected or screen is blanked and resuming it automatically
when the picture is back. But I think that is maybe too hackish solution.
Considering the big picture, including the userspace. I think we would
need some way to tell the userspace when the HDMI device is available.
The problem is the same for both a and c cases. Maybe a read only alsa
mixer could be used for that.
Cheers,
Jyri
> Currently, the OMAP HDMI version does c). It's the easiest solution for us.
>
> Option a) is a bit problematic for us, as it requires the HDMI IP side
> to be fully operational, with the video output configured and enabled,
> as that is what drives the audio DMA. If we stop the video, I believe
> the audio DMA will freeze, and it'll lead to timeouts on the audio side.
>
> I haven't tried, but I believe we could program the HDMI IP to some safe
> default video mode if the cable is disconnected, and turn off the actual
> HDMI PHY, so that the audio side could work even if the HDMI stream is
> not going anywhere. I think it would be quite big change to how the HDMI
> driver works at the moment.
>
> But then, if the cable _is_ connected and the video mode is such that it
> cannot not support audio, I wonder if we can still fake the audio
> playback or will the HDMI IP get confused...
>
^ permalink raw reply
* Re: [PATCH v3 3/4] simplefb: Change simplefb_init from module_init to fs_initcall
From: Hans de Goede @ 2014-11-13 14:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464C10A.30709@ti.com>
Hi,
On 11/13/2014 03:32 PM, Tomi Valkeinen wrote:
> On 13/11/14 15:34, Hans de Goede wrote:
>> One of the reasons for having the simplefb nodes in /chosen, and doing
>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>> so that we get a console earlier on.
>>
>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>> a console when fbcon intializes, which is a fs_initcall too.
>
> The above text is slightly confusing, as fbcon initializes at
> module_init time. You only change it in the next patch. I can move the
> fbcon change to be the first patch when applying, if there are no more
> versions sent.
True, feel free to swap them when applying.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate
From: Hans de Goede @ 2014-11-13 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464C026.7030300@ti.com>
Hi,
On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
> On 13/11/14 15:34, Hans de Goede wrote:
>
>> +chosen {
>> + framebuffer0: framebuffer@5fc00000 {
>> compatible = "simple-framebuffer";
>> - reg = <0x1d385000 (1600 * 1200 * 2)>;
>> + reg = <0x5fc00000 (4096 * 1024)>;
>
> Was there a reason for this change?
I changed the node name to match the new bindings text which
specifies that the node name must be framebuffer@<address>,
while at it I've taken a real world address range.
I assume you refer to the bit where the size of the reg property
is changed ? That indeed is not really necessary.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v3 4/4] fbcon: Change fbcon_init from module_init to fs_initcall
From: Tomi Valkeinen @ 2014-11-13 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-4-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 525 bytes --]
On 13/11/14 15:34, Hans de Goede wrote:
> Various fb drivers register themselves before module_init so as to have a
> console as early as possible, this is of little use if fbcon does not initialze
> early too.
>
> Fbcon cannot initialize earlier then fs_initcall, because then the creation
> of /sys/class/graphics/fbcon will fail.
Not related to this series, but makes me wonder how difficult it would
be to get fbcon and simplefb initialize at even earlier stage. Well,
something for the future.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/4] simplefb: Change simplefb_init from module_init to fs_initcall
From: Tomi Valkeinen @ 2014-11-13 14:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-3-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 626 bytes --]
On 13/11/14 15:34, Hans de Goede wrote:
> One of the reasons for having the simplefb nodes in /chosen, and doing
> explicit enumeration of the nodes there, is too allow enumerating them sooner,
> so that we get a console earlier on.
>
> Doing this earlier then fs_initcall is not useful, since the fb only turns into
> a console when fbcon intializes, which is a fs_initcall too.
The above text is slightly confusing, as fbcon initializes at
module_init time. You only change it in the next patch. I can move the
fbcon change to be the first patch when applying, if there are no more
versions sent.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Tomi Valkeinen @ 2014-11-13 14:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-1-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 360 bytes --]
On 13/11/14 15:34, Hans de Goede wrote:
> +chosen {
> + framebuffer0: framebuffer@5fc00000 {
> compatible = "simple-framebuffer";
> - reg = <0x1d385000 (1600 * 1200 * 2)>;
> + reg = <0x5fc00000 (4096 * 1024)>;
Was there a reason for this change? The former version seems much more
obvious to me, especially as this is an example.
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