* Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Grant Likely @ 2014-11-13 12:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464A0A2.1080503@redhat.com>
On Thu, Nov 13, 2014 at 12:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11/13/2014 12:02 PM, Grant Likely wrote:
>> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> 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>
>>> --
>>> Changes in v2:
>>> -Add stdout-path to the example code
>>> ---
>>> .../bindings/video/simple-framebuffer.txt | 38 +++++++++++++++++++++-
>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> index 8f35718..8b7ecf6 100644
>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> @@ -4,6 +4,26 @@ 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 (*). The primary display node must be named framebuffer0,
>>> +additional nodes must be called framebuffer1, etc.
>>
>> Thinking more about our conversation yesterday, the preferred name
>> should still be framebuffer@<address>. There is no reason to break
>> with established convention, and as mentioned in my reply on patch #2,
>> using the name is not the preferred way to identify a device node. So,
>> my recommendation is to prefer the name "framebuffer@<addr>", but if
>> it is inconvenient because the address isn't known, then
>> "framebuffer#" is acceptable.
>
> Since we want to use pre-filled simplefb nodes already present in the devicetree,
> and u-boot is the one chosing the framebuffer address we don't know the
> address to put in the dts file beforehand, so framebuffer# it is.
fdt_set_name() is available to U-Boot to rename a node. :-)
Prepopulating with /chosen/framebuffer#, and then renaming to
/chosen/framebuffer@<addr> is perfectly reasonable.
g.
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Hans de Goede @ 2014-11-13 12:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6tra-G3-FV0FJcfXNEVgJNdsR+NvzALM34_XymP93icGg@mail.gmail.com>
Hi,
On 11/13/2014 01:36 PM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 12:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11/13/2014 12:02 PM, Grant Likely wrote:
>>> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> 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>
>>>> --
>>>> Changes in v2:
>>>> -Add stdout-path to the example code
>>>> ---
>>>> .../bindings/video/simple-framebuffer.txt | 38 +++++++++++++++++++++-
>>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> index 8f35718..8b7ecf6 100644
>>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> @@ -4,6 +4,26 @@ 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 (*). The primary display node must be named framebuffer0,
>>>> +additional nodes must be called framebuffer1, etc.
>>>
>>> Thinking more about our conversation yesterday, the preferred name
>>> should still be framebuffer@<address>. There is no reason to break
>>> with established convention, and as mentioned in my reply on patch #2,
>>> using the name is not the preferred way to identify a device node. So,
>>> my recommendation is to prefer the name "framebuffer@<addr>", but if
>>> it is inconvenient because the address isn't known, then
>>> "framebuffer#" is acceptable.
>>
>> Since we want to use pre-filled simplefb nodes already present in the devicetree,
>> and u-boot is the one chosing the framebuffer address we don't know the
>> address to put in the dts file beforehand, so framebuffer# it is.
>
> fdt_set_name() is available to U-Boot to rename a node. :-)
>
> Prepopulating with /chosen/framebuffer#, and then renaming to
> /chosen/framebuffer@<addr> is perfectly reasonable.
Ok, that works for me.
Regards,
Hans
^ permalink raw reply
* [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Hans de Goede @ 2014-11-13 13:34 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 noded 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
---
.../bindings/video/simple-framebuffer.txt | 51 +++++++++++++++++++++-
1 file changed, 49 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..6c7339b 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@5fc00000 {
compatible = "simple-framebuffer";
- reg = <0x1d385000 (1600 * 1200 * 2)>;
+ reg = <0x5fc00000 (4096 * 1024)>;
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 v3 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen
From: Hans de Goede @ 2014-11-13 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-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
---
drivers/video/fbdev/simplefb.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cd96edd..223f230 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",
@@ -385,7 +386,31 @@ 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 *np;
+
+ ret = platform_driver_register(&simplefb_driver);
+ if (ret)
+ return ret;
+
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+
+ return 0;
+}
+
+static void __exit simplefb_exit(void)
+{
+ platform_driver_unregister(&simplefb_driver);
+}
+
+module_init(simplefb_init);
+module_exit(simplefb_exit);
MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.1.0
^ permalink raw reply related
* [PATCH v3 3/4] simplefb: Change simplefb_init from module_init to fs_initcall
From: Hans de Goede @ 2014-11-13 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-1-git-send-email-hdegoede@redhat.com>
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.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
---
drivers/video/fbdev/simplefb.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 223f230..1028a0d 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -409,7 +409,11 @@ static void __exit simplefb_exit(void)
platform_driver_unregister(&simplefb_driver);
}
-module_init(simplefb_init);
+/*
+ * While this can be a module, if builtin it's most likely the console
+ * So let's leave module_exit but move module_init to an earlier place
+ */
+fs_initcall(simplefb_init);
module_exit(simplefb_exit);
MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
--
2.1.0
^ permalink raw reply related
* [PATCH v3 4/4] fbcon: Change fbcon_init from module_init to fs_initcall
From: Hans de Goede @ 2014-11-13 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415885645-24613-1-git-send-email-hdegoede@redhat.com>
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.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/console/fbcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
return 0;
}
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
#ifdef MODULE
--
2.1.0
^ permalink raw reply related
* 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
* 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 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: [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 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: [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: [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: [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: 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: [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: [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: 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: 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: [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 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 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 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 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 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
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