* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Kishon Vijay Abraham I @ 2013-10-24 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK9yfHxaLsdFGXiCxvs+HpMSuY6xWd=CGPv-YfSkJqWSxE+f-w@mail.gmail.com>
Hi,
On Thursday 24 October 2013 09:12 PM, Sachin Kamat wrote:
> Hi Olof,
>
> On 24 October 2013 20:00, Olof Johansson <olof@lixom.net> wrote:
>> Hi Kishon,
>>
>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>>> index 32e5406..00b3a52 100644
>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>>> exynos_mipi_regulator_enable(dsim);
>>>
>>> /* enable MIPI-DSI PHY. */
>>> - if (dsim->pd->phy_enable)
>>> - dsim->pd->phy_enable(pdev, true);
>>> + phy_power_on(dsim->phy);
>>>
>>> clk_enable(dsim->clock);
>>>
>>
>> This introduces the below with exynos_defconfig:
>>
>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
>> 'exynos_mipi_dsi_blank_mode':
>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
>> variable 'pdev' [-Wunused-variable]
>> struct platform_device *pdev = to_platform_device(dsim->dev);
>>
>
> I have already submitted a patch to fix this [1]
>
> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
Sorry, missed that :-(
Thanks
Kishon
>
>
^ permalink raw reply
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Olof Johansson @ 2013-10-24 17:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK9yfHxaLsdFGXiCxvs+HpMSuY6xWd=CGPv-YfSkJqWSxE+f-w@mail.gmail.com>
On Thu, Oct 24, 2013 at 8:42 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Olof,
>
> On 24 October 2013 20:00, Olof Johansson <olof@lixom.net> wrote:
>> Hi Kishon,
>>
>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>>> index 32e5406..00b3a52 100644
>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>>> exynos_mipi_regulator_enable(dsim);
>>>
>>> /* enable MIPI-DSI PHY. */
>>> - if (dsim->pd->phy_enable)
>>> - dsim->pd->phy_enable(pdev, true);
>>> + phy_power_on(dsim->phy);
>>>
>>> clk_enable(dsim->clock);
>>>
>>
>> This introduces the below with exynos_defconfig:
>>
>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
>> 'exynos_mipi_dsi_blank_mode':
>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
>> variable 'pdev' [-Wunused-variable]
>> struct platform_device *pdev = to_platform_device(dsim->dev);
>>
>
> I have already submitted a patch to fix this [1]
>
> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
Ah, I'm not subscribed to the fbdev list. Next time it might be a good
idea to cc the same lists as the original patch. But thanks for fixing
it!
-Olof
^ permalink raw reply
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Sylwester Nawrocki @ 2013-10-24 21:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52694354.6030603@ti.com>
On 10/24/2013 05:57 PM, Kishon Vijay Abraham I wrote:
> On Thursday 24 October 2013 09:12 PM, Sachin Kamat wrote:
>> On 24 October 2013 20:00, Olof Johansson<olof@lixom.net> wrote:
>>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I<kishon@ti.com> wrote:
>>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>>>> index 32e5406..00b3a52 100644
>>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>>>> exynos_mipi_regulator_enable(dsim);
>>>>
>>>> /* enable MIPI-DSI PHY. */
>>>> - if (dsim->pd->phy_enable)
>>>> - dsim->pd->phy_enable(pdev, true);
>>>> + phy_power_on(dsim->phy);
>>>>
>>>> clk_enable(dsim->clock);
>>>>
>>>
>>> This introduces the below with exynos_defconfig:
>>>
>>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
>>> 'exynos_mipi_dsi_blank_mode':
>>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
>>> variable 'pdev' [-Wunused-variable]
>>> struct platform_device *pdev = to_platform_device(dsim->dev);
Sorry about missing that, I only noticed this warning recently and didn't
get around to submit a patch.
>> I have already submitted a patch to fix this [1]
Thanks a lot guys for fixing this.
>> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
>
> Sorry, missed that :-(
This MIPI DSIM driver is affectively a dead code in the mainline now, once
Exynos become a dt-only platform. I guess it can be deleted for 3.14, once
S5P gets converted to the device tree. The new driver using CDF is basically
a complete rewrite. Or device tree support should be added to that driver,
but I believe it doesn't make sense without CDF.
--
Regards,
Sylwester
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Stephen Warren @ 2013-10-24 22:06 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <3278905.VOXO7bPE7e@avalon>
On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> Hi Stephen,
>
> On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
>> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
>> ...
>>
>>>> As I said, anything that really needs a CDF binding to work
>>>> likely isn't "simple" anymore, therefore a separate driver can
>>>> easily be justified.
>>>
>>> The system as a whole would be more complex, but the panel could be
>>> the same. We can't have two drivers for the same piece of hardware
>>> in the DT world, as there will be a single compatible string and no
>>> way to choose between the drivers (unlike the board code world that
>>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
>>> and live happily with that ever after).
>>
>> That's not true. We can certainly define two different compatible
>> values for a piece of HW if we have to. We can easily control whether
>> they are handled by the same or different drivers in the OS.
>
> From an implementation point of view, sure. But from a conceptual point of
> view, that would make the DT bindings pretty Linux-specific, with a
> description of what the operating system should do instead of a description of
> what the hardware looks like. My understanding is that we've tried pretty hard
> in the past not to open that Pandora's box.
>
> The case I'm mostly concerned about would be two different compatibility
> strings to select whether the device should be handled by a KMS or V4L driver.
> I don't think that's a good idea.
I wouldn't think of the two compatible values as selecting different
specific Linux drivers, but rather they simply describe the HW in
different levels of detail. The fact that if we know a certain level of
detail about the HW means that Linux can and does create a KMS driver
rather than a V4L2 driver seems like a detail that's completely hidden
inside the OS.
^ permalink raw reply
* [PATCH 1/1] video: exynos_mipi_dsi: Unlock the mutex before returning
From: Sachin Kamat @ 2013-10-25 5:24 UTC (permalink / raw)
To: linux-fbdev
Mutex should be unlocked before returning. Fixes mutex lock-unlock
imbalance issue.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
drivers/video/exynos/exynos_mipi_dsi_common.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c b/drivers/video/exynos/exynos_mipi_dsi_common.c
index 520fc9b..649d9e6 100644
--- a/drivers/video/exynos/exynos_mipi_dsi_common.c
+++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
@@ -376,6 +376,7 @@ int exynos_mipi_dsi_rd_data(struct mipi_dsim_device *dsim, unsigned int data_id,
"data id %x is not supported current DSI spec.\n",
data_id);
+ mutex_unlock(&dsim->lock);
return -EINVAL;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/1] video: exynos_mipi_dsi: Remove unused variable
From: Sachin Kamat @ 2013-10-25 5:33 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1382333533-32740-1-git-send-email-sachin.kamat@linaro.org>
On 24 October 2013 21:25, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On Monday 21 October 2013 11:02 AM, Sachin Kamat wrote:
>> 'pdev' is not used anymore (Removed vide commit 7e0be9f9 "video:
>> exynos_mipi_dsim: Use the generic PHY driver"). Remove it and
>> silence the following compilation warning:
>> drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning:
>> unused variable ‘pdev’ [-Wunused-variable]
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
Thanks Kishon.
I believe this patch should go through Greg's tree as your other
patches are lined up there?
--
With warm regards,
Sachin
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-25 8:13 UTC (permalink / raw)
To: Stephen Warren
Cc: Laurent Pinchart, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <526999DA.7080409-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3868 bytes --]
On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > Hi Stephen,
> >
> > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> >> ...
> >>
> >>>> As I said, anything that really needs a CDF binding to work
> >>>> likely isn't "simple" anymore, therefore a separate driver can
> >>>> easily be justified.
> >>>
> >>> The system as a whole would be more complex, but the panel could be
> >>> the same. We can't have two drivers for the same piece of hardware
> >>> in the DT world, as there will be a single compatible string and no
> >>> way to choose between the drivers (unlike the board code world that
> >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> >>> and live happily with that ever after).
> >>
> >> That's not true. We can certainly define two different compatible
> >> values for a piece of HW if we have to. We can easily control whether
> >> they are handled by the same or different drivers in the OS.
> >
> > From an implementation point of view, sure. But from a conceptual point of
> > view, that would make the DT bindings pretty Linux-specific, with a
> > description of what the operating system should do instead of a description of
> > what the hardware looks like. My understanding is that we've tried pretty hard
> > in the past not to open that Pandora's box.
> >
> > The case I'm mostly concerned about would be two different compatibility
> > strings to select whether the device should be handled by a KMS or V4L driver.
> > I don't think that's a good idea.
>
> I wouldn't think of the two compatible values as selecting different
> specific Linux drivers, but rather they simply describe the HW in
> different levels of detail. The fact that if we know a certain level of
> detail about the HW means that Linux can and does create a KMS driver
> rather than a V4L2 driver seems like a detail that's completely hidden
> inside the OS.
I've had a somewhat similar idea the other day but couldn't really put
it into words. Interestingly someone else mentioned a similar concept in
a different thread which I think describes what I had in mind as well.
I was wondering if we couldn't use two compatible values to denote two
interfaces that the device implements. Something along the lines of:
compatible = "vendor,block-name", "encoder";
So a driver could primarily match on "vendor,block-name", but at the
same time it could use the additional information of being required to
implement "encoder" to expose an additonal interface.
I suppose that perhaps something like a device_type property could be
used for that as well, and that might even be the more correct thing to
do.
We already do something similar to make GPIO controllers expose an
interrupt chip by adding an interrupt-controller property. We also use
the gpio-controller property to mark a device node as exposing the GPIO
interface for that matter.
So if a HW block can actually implement two different interfaces, each
of them being optional, then there should be ways to represent that in
DT as well. We already do that for "simpler" HW blocks, so there's no
reason we shouldn't be able to do the same with multimedia components.
If it's really an encoder, though, the problem might be different,
though, since the interface (at a hardware or functional level if you
will) remains the same. But I think in that case it's something that
needs to be figured out internally by the OS. In my opinion, if we are
in a situation where we have two different drivers in two subsystems for
the same device, then we're doing something wrong and it should be fixed
at that level, not by quirking the DT into making a decision for us.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Sylwester Nawrocki @ 2013-10-25 10:54 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, Thierry Reding, Laurent Pinchart, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
devicetree, linux-fbdev, dri-devel, Dave Airlie, linux-media,
Guennadi Liakhovetski
In-Reply-To: <5268FBE3.80000@ti.com>
On 10/24/2013 12:52 PM, Tomi Valkeinen wrote:
> On 24/10/13 13:40, Laurent Pinchart wrote:
>
>>> panel {
>>> remote =<&remote-endpoint>;
>>> common-video-property =<asd>;
>>> };
>>>
>>> panel {
>>> port {
>>> endpoint {
>>> remote =<&remote-endpoint>;
>>> common-video-property =<asd>;
>>> };
>>> };
>>> };
>>
>> Please note that the common video properties would be in the panel node, not
>> in the endpoint node (unless you have specific requirements to do so, which
>> isn't the common case).
>
> Hmm, well, the panel driver must look for its properties either in the
> panel node, or in the endpoint node (I guess it could look them from
> both, but that doesn't sound good).
Presumably the OS could be searching for port node and any endpoint node
inside it first. If that's not found then it could be parsing the panel
node.
Please note that a port node may be required even if there is only one
port, when there are multiple physical bus interfaces, e.g. at an LCD
controller and only one of them is used. The reg property would select
the physical bus interface.
I wonder if a property like #video-port or #video-endpoint could be used
to indicate that a node contains video bus properties. Probably it's too
late to introduce it now and make it a required property for the endpoint
nodes or nodes containing the common video properties.
> If you write the panel driver, and in all your cases the properties work
> fine in the panel node, does that mean they'll work fine with everybody?
It's likely not safe to assume so. In V4L data bus properties are specified
a both the receiver and the transmitter endpoint nodes separately.
> I guess there are different kinds of properties. Something like a
> regulator is obviously property of the panel. But anything related to
> the video itself, like DPI's bus width, or perhaps even something like
> "orientation" if the panel supports such, could need to be in the
> endpoint node.
If we use port/endpoint nodes it all seems clear, the video bus properties
are put in an endpoint node.
But since we are considering a simplified binding all the properties would
be placed in the panel or display controller node.
> But yes, I understand what you mean. With "common-video-property" I
> meant common properties like DPI bus width.
>
>>> If that can be supported in the SW by adding complexity to a few functions,
>>> and it covers practically all the panels, isn't it worth it?
>>>
>>> Note that I have not tried this, so I don't know if there are issues.
>>> It's just a thought. Even if there's need for a endpoint node, perhaps
>>> the port node can be made optional.
>>
>> It can be worth it, as long as we make sure that simplified bindings cover the
>> needs of the generic code.
>>
>> We could assume that, if the port subnode isn't present, the device will have
>> a single port, with a single endpoint. However, isn't the number of endpoints
>
> Right.
>
>> a system property rather than a device property ? If a port of a device is
>
> Yes.
>
>> connected to two remote ports it will require two endpoints. We could select
>> the simplified or full bindings based on the system topology though.
Yes, I guess it's all about the system topology. Any simplified binding
would
work only for very simple configuration like single-output LCD
controller with
single panel attached to it.
> The drivers should not know about simplified/normal bindings. They
> should use CDF DT helper functions to get the port and endpoint
> information. The helper functions would do the assuming.
Yes, anyway all the parsing is supposed to be done within the helpers.
--
Thanks,
Sylwester
^ permalink raw reply
* Re: [PATCHv7][ 1/6] video: imxfb: Introduce regulator support.
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382605103-9595-1-git-send-email-denis@eukrea.com>
On 10:58 Thu 24 Oct , Denis Carikli wrote:
> This commit is based on the following commit by Fabio Estevam:
> 4344429 video: mxsfb: Introduce regulator support
I've too many patch version in my mailbox specially without any description
about what is updated between version can explain this.
>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> drivers/video/imxfb.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
> index 44ee678..4bf3837 100644
> --- a/drivers/video/imxfb.c
> +++ b/drivers/video/imxfb.c
> @@ -28,6 +28,7 @@
> #include <linux/cpufreq.h>
> #include <linux/clk.h>
> #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/dma-mapping.h>
> #include <linux/io.h>
> #include <linux/math64.h>
> @@ -145,6 +146,7 @@ struct imxfb_info {
> struct clk *clk_ipg;
> struct clk *clk_ahb;
> struct clk *clk_per;
> + struct regulator *reg_lcd;
> enum imxfb_type devtype;
> bool enabled;
>
> @@ -563,12 +565,23 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
>
> static void imxfb_enable_controller(struct imxfb_info *fbi)
> {
> + int ret;
>
> if (fbi->enabled)
> return;
>
> pr_debug("Enabling LCD controller\n");
>
> + if (fbi->reg_lcd) {
> + ret = regulator_enable(fbi->reg_lcd);
> + if (ret) {
> + dev_err(&fbi->pdev->dev,
> + "lcd regulator enable failed with error: %d\n",
> + ret);
> + return;
> + }
> + }
> +
> writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
>
> /* panning offset 0 (0 pixel offset) */
> @@ -597,6 +610,8 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
>
> static void imxfb_disable_controller(struct imxfb_info *fbi)
> {
> + int ret;
> +
> if (!fbi->enabled)
> return;
>
> @@ -613,6 +628,14 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
> fbi->enabled = false;
>
> writel(0, fbi->regs + LCDC_RMCR);
> +
> + if (fbi->reg_lcd) {
> + ret = regulator_disable(fbi->reg_lcd);
> + if (ret)
> + dev_err(&fbi->pdev->dev,
> + "lcd regulator disable failed with error: %d\n",
> + ret);
> + }
> }
>
> static int imxfb_blank(int blank, struct fb_info *info)
> @@ -1020,6 +1043,12 @@ static int imxfb_probe(struct platform_device *pdev)
> goto failed_register;
> }
>
> + fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
> + if (IS_ERR(fbi->reg_lcd)) {
> + dev_info(&pdev->dev, "No lcd regulator used.\n");
> + fbi->reg_lcd = NULL;
> + }
> +
> imxfb_enable_controller(fbi);
> fbi->pdev = pdev;
> #ifdef PWMR_BACKLIGHT_AVAILABLE
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/9] backlight: atmel-pwm-bl: fix reported brightness
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:08 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel, stable
In-Reply-To: <1382522143-32072-2-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> The driver supports 16-bit brightness values, but the value returned
> from get_brightness was truncated to eight bits.
>
> Cc: stable@vger.kernel.org
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 66885fb..8aac273 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -70,7 +70,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> - u8 intensity;
> + u32 intensity;
>
> if (pwmbl->pdata->pwm_active_low) {
> intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> @@ -80,7 +80,7 @@ static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> }
>
> - return intensity;
> + return (u16)intensity;
no cast mask it
> }
>
> static int atmel_pwm_bl_init_pwm(struct atmel_pwm_bl *pwmbl)
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 2/9] backlight: atmel-pwm-bl: fix gpio polarity in remove
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:09 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel, stable
In-Reply-To: <1382522143-32072-3-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Make sure to honour gpio polarity also at remove so that the backlight
> is actually disabled on boards with active-low enable pin.
>
> Cc: stable@vger.kernel.org
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 8aac273..3cb0094 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -205,8 +205,10 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
> {
> struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
>
> - if (pwmbl->gpio_on != -1)
> - gpio_set_value(pwmbl->gpio_on, 0);
> + if (pwmbl->gpio_on != -1) {
here we need to use gpio_is_valid
> + gpio_set_value(pwmbl->gpio_on,
> + 0 ^ pwmbl->pdata->on_active_low);
> + }
> pwm_channel_disable(&pwmbl->pwmc);
> pwm_channel_free(&pwmbl->pwmc);
>
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 4/9] backlight: atmel-pwm-bl: clean up probe error handling
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:11 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-5-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Clean up probe error handling by checking parameters before any
> allocations and removing an obsolete error label. Also remove
> unnecessary reset of private gpio number.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 31 ++++++++++++-------------------
> 1 file changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index cc5a5ed..52a8134 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -126,40 +126,33 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> struct atmel_pwm_bl *pwmbl;
> int retval;
>
> + pdata = dev_get_platdata(&pdev->dev);
> + if (!pdata)
> + return -ENODEV;
> +
> + if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
> + pdata->pwm_duty_min > pdata->pwm_duty_max ||
> + pdata->pwm_frequency = 0)
> + return -EINVAL;
> +
> pwmbl = devm_kzalloc(&pdev->dev, sizeof(struct atmel_pwm_bl),
> GFP_KERNEL);
> if (!pwmbl)
> return -ENOMEM;
>
> pwmbl->pdev = pdev;
> -
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - retval = -ENODEV;
> - goto err_free_mem;
> - }
> -
> - if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
> - pdata->pwm_duty_min > pdata->pwm_duty_max ||
> - pdata->pwm_frequency = 0) {
> - retval = -EINVAL;
> - goto err_free_mem;
> - }
> -
> pwmbl->pdata = pdata;
> pwmbl->gpio_on = pdata->gpio_on;
>
> retval = pwm_channel_alloc(pdata->pwm_channel, &pwmbl->pwmc);
> if (retval)
> - goto err_free_mem;
> + return retval;
>
> if (pwmbl->gpio_on != -1) {
gpio_is_valid here
> retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> "gpio_atmel_pwm_bl");
> - if (retval) {
> - pwmbl->gpio_on = -1;
> + if (retval)
> goto err_free_pwm;
> - }
>
> /* Turn display off by default. */
> retval = gpio_direction_output(pwmbl->gpio_on,
> @@ -197,7 +190,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
>
> err_free_pwm:
> pwm_channel_free(&pwmbl->pwmc);
> -err_free_mem:
> +
> return retval;
> }
>
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 5/9] backlight: atmel-pwm-bl: clean up get_intensity
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:12 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-6-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Clean up get_intensity to increase readability.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
this ok
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 52a8134..504061c 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -70,15 +70,14 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> + u32 cdty;
> u32 intensity;
>
> - if (pwmbl->pdata->pwm_active_low) {
> - intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> - pwmbl->pdata->pwm_duty_min;
> - } else {
> - intensity = pwmbl->pdata->pwm_duty_max -
> - pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> - }
> + cdty = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> + if (pwmbl->pdata->pwm_active_low)
> + intensity = cdty - pwmbl->pdata->pwm_duty_min;
> + else
> + intensity = pwmbl->pdata->pwm_duty_max - cdty;
>
> return (u16)intensity;
> }
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 8/9] backlight: atmel-pwm-bl: refactor gpio_on handling
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:13 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-9-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Add helper function to control the gpio_on signal.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
ok
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index ffc30d2..1277e0c 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -26,6 +26,14 @@ struct atmel_pwm_bl {
> int gpio_on;
> };
>
> +static void atmel_pwm_bl_set_gpio_on(struct atmel_pwm_bl *pwmbl, int on)
> +{
> + if (!gpio_is_valid(pwmbl->gpio_on))
> + return;
> +
> + gpio_set_value(pwmbl->gpio_on, on ^ pwmbl->pdata->on_active_low);
> +}
> +
> static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> @@ -48,19 +56,13 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> pwm_duty = pwmbl->pdata->pwm_duty_min;
>
> if (!intensity) {
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 0 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 0);
> pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
> pwm_channel_disable(&pwmbl->pwmc);
> } else {
> pwm_channel_enable(&pwmbl->pwmc);
> pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 1 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 1);
> }
>
> return 0;
> @@ -196,10 +198,7 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
> {
> struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
>
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 0 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 0);
> pwm_channel_disable(&pwmbl->pwmc);
> pwm_channel_free(&pwmbl->pwmc);
>
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 9/9] backlight: atmel-pwm-bl: use gpio_request_one
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:15 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-10-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Use devm_gpio_request_one rather than requesting and setting direction
> in two calls.
this is the same I do not see any advantage
and as I said for ather backligth It's wrong to enable or disable it at probe
as the bootloader might have already enable it for splash screen
Best Regards,
J.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>:w
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 1277e0c..5ea2517 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> const struct atmel_pwm_bl_platform_data *pdata;
> struct backlight_device *bldev;
> struct atmel_pwm_bl *pwmbl;
> + int flags;
> int retval;
>
> pdata = dev_get_platdata(&pdev->dev);
> @@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> return retval;
>
> if (gpio_is_valid(pwmbl->gpio_on)) {
> - retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> - "gpio_atmel_pwm_bl");
> - if (retval)
> - goto err_free_pwm;
> -
> /* Turn display off by default. */
> - retval = gpio_direction_output(pwmbl->gpio_on,
> - 0 ^ pdata->on_active_low);
> + if (pdata->on_active_low)
> + flags = GPIOF_OUT_INIT_HIGH;
> + else
> + flags = GPIOF_OUT_INIT_LOW;
> +
> + retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
> + flags, "gpio_atmel_pwm_bl");
> if (retval)
> goto err_free_pwm;
> }
> --
> 1.8.4
>
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 11:27 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131024114823.GC11296-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 15398 bytes --]
Hi Thierry,
On Thursday 24 October 2013 13:48:24 Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> > > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
> > [snip]
> >
> > > > The point is that the video pipeline must be described in DT. Having a
> > > > per-device way to represent connections would be a nightmare to
> > > > support from an implementation point of view, hence the need for a
> > > > generic way to describe them.
> > >
> > > Okay, so we're back to the need to describe pipelines in DT. At the risk
> > > of sounding selfish: I don't care about pipelines. I just want us to
> > > settle on a way to represent a dumb panel in DT so that it can be
> > > enabled when it needs to. Furthermore I don't have any hardware that
> > > exhibits any of these "advanced" features, so I'm totally unqualified to
> > > work on any of this.
> > >
> > > Can we please try to be a little pragmatic here and solve one problem at
> > > a time? I am aware that solving this for panels may require some amount
> > > of foresight, but let's not try to solve everything at once at the risk
> > > of not getting anything done at all.
> >
> > I won't force you to care about pipelines, and I don't think this is
> > selfish at all :-)
> >
> > What I would like to ensure is that whatever bindings we come up with,
> > they will not preclude us from adding pipeline support when needed (as I'm
> > pretty sure it will be needed at some point).
>
> Sure. I think I've explained elsewhere that I consider this easily
> possible. Other people have said the same thing. If all we do is add
> properties to a binding, then drivers written for an old binding will
> have no problem continuing to work.
>
> I also think that we're mostly being too scared. If ever the next device
> comes around that requires explicit pipeline support to work, how likely
> is it to have the exact same display as prior devices?
The exact same panel, probably low, but a panel supported by the same driver
using the same DT bindings (such as the generic simple panel driver), quite
high.
> > > > > I'm not at all opposed to the idea of CDF or the problems it tries
> > > > > to address. I don't think it necessarily should be a separate
> > > > > framework for reasons explained earlier. But even if it were to end
> > > > > up in a separate framework there's absolutely nothing preventing us
> > > > > from adding a DRM panel driver that hooks into CDF as a "backend" of
> > > > > sorts for panels that require something more complex than the
> > > > > simple-panel binding.
> > > >
> > > > Once again it's not about panel having complex needs, but more about
> > > > using simple panels in complex pipelines. I'm fine with the drm_panel
> > > > infrastructure, what I would like is DT bindings that describe
> > > > connections in a more future-proof way. The rest is fine.
> > >
> > > And I've already said elsewhere that the bindings in their current form
> > > are easily extensible to cater for the needs of CDF.
> >
> > The simple panel bindings do not include any connection information, so we
> > could add that later when needed without having to deprecate, remove or
> > repurpose existing properties.
>
> Yes, I agree.
>
> > The simple panel driver would need to be extended, which isn't much of a
> > problem (except that extending it with CDF support might require changes
> > to the users of the simple panel driver, which I believe won't be happily
> > accepted, but that's a different issue).
>
> That might be true. But that's just the way kernel development works. We
> sometimes require major rework of APIs and we're actually pretty good at
> pulling that off, so I don't worry too much about it.
>
> Also, being the original author of the driver makes me the maintainer,
> and if it should prove to be necessary I'm absolutely willing to add CDF
> support.
Much appreciated :-)
> > My concern is also on the other side. In the patches you've sent the tegra
> > driver uses a custom nvidia,panel property to reference the panel. That
> > would of course not be CDF-compatible, but there's no way around that at
> > the moment if we don't want to keep development of all ARM KMS drivers
> > stalled for the next 6 months.
>
> Well, the nvidia,panel property is part of the display output and it is
> an optional property. So the driver already needs to cope with it being
> absent. If the display output driver is ever extended with CDF support
> then we can just go and use CDF if CDF-specific properties exist, or we
> fall back to nvidia,panel if that doesn't exist.
>
> > It boils down to the question of whether DT should be a stable ABI, and
> > I'm increasingly tempted to say that I don't care. I want to solve
> > issues we have on the display side, the firmware interface isn't my main
> > concern.
>
> I wholeheartedly agree. In my opinion it is much more useful to come up
> with a solution that works now, rather than discussing and arguing
> things to death and not get anything done. If that means that I'll have
> to maintain additional code, then so be it.
>
> > > > > But that's precisely the point. Why would we need to go back from
> > > > > the panel to the display controller? Especially for dumb panels that
> > > > > can't or don't have to be configured in any way. Even if they needed
> > > > > some sort of setup, why can't that be done from the display
> > > > > controller/output.
> > > > >
> > > > > Even given a setup where a DSI controller needs to write some
> > > > > registers in a panel upon initialization, I don't see why the
> > > > > reverse connection needs to be described. The fact alone that an
> > > > > output dereferences a panel's phandle should be enough to connect
> > > > > both of them and have any panel driver use the DSI controller that
> > > > > it's been attached to for the programming.
> > > > >
> > > > > That's very much analogous to how I2C works. There are no
> > > > > connections back to the I2C master from the slaves. Yet each I2C
> > > > > client driver manages to use the services provided by the I2C master
> > > > > to perform transactions on the I2C bus. In a similar way the DSI
> > > > > controller is the bus master that talks to DSI panels. DSI panels
> > > > > don't actively talk to the DSI controller.
> > > >
> > > > It's all about modeling video pipeline graphs in DT. To be able to
> > > > walk the graph we need to describe connections. Not having
> > > > bidirectional information means that we restrict the points at which
> > > > we can start walking the graph, potentially making our life much more
> > > > difficult in the future.
> > > >
> > > > What's so wrong about adding a port node and link information to the
> > > > panel DT node ? It describe what's there: the panel has one input, why
> > > > not make that explicit ?
> > >
> > > What's wrong with it is that there's no way to verify the soundness of
> > > the design by means of a full implementation because we're missing the
> > > majority of the pieces. Just putting the nodes into DT to provide some
> > > imaginary future-proofness isn't helpful either. Without any code that
> > > actually uses them they are useless.
> > >
> > > And again, why should we add them right away (while not needed) when
> > > they can easily be added in a backwards-compatible manner at some later
> > > point when there's actually any use for them and they can actually be
> > > tested in some larger framework?
> >
> > It's the "easily" part I'm not sure about. I doubt we'll ever have any
> > easy to solve DT backward compatibility issue. However, as mentioned
> > above, this shouldn't be a show stopper. I'm thus fine with the way the
> > proposed bindings describe (or rather don't describe) the connection.
> > However, I will then expect your support in the future to implement the
> > "easy" extension of the bindings to support CDF.
>
> Again, I don't expect that to ever happen. But if it ever happens anyway,
> then...
>
> > Do we have a deal ? ;-)
>
> Yes, we do.
Great!
> > > > > > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > > > > > +{
> > > > > > > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > > > > > > + int err;
> > > > > > > > > +
> > > > > > > > > + if (p->enabled)
> > > > > > > > > + return;
> > > > > > > > > +
> > > > > > > > > + err = regulator_enable(p->supply);
> > > > > > > > > + if (err < 0)
> > > > > > > > > + dev_err(panel->dev, "failed to enable supply:
%d\n",
> >
> > err);
> >
> > > > > > > > Is that really a non-fatal error ? Shouldn't the enable
> > > > > > > > operation return an int ?
> > > > > > >
> > > > > > > There's no way to propagate this in DRM, so why go through the
> > > > > > > trouble of returning the error? Furthermore, there's nothing
> > > > > > > that the caller could do to remedy the situation anyway.
> > > > > >
> > > > > > That's a DRM issue, which could be fixed. While the caller can't
> > > > > > remedy the situation, it should at least report the error to the
> > > > > > application instead of silently ignoring it.
> > > > >
> > > > > Perhaps. It's not really relevant to the discussion and can always
> > > > > be fixed in a subsequent patch.
> > > >
> > > > Most things can be fixed by a subsequent patch, that's not a good
> > > > enough reason not to address the known problems before pushing the
> > > > code to mainline (to clarify my point of view, there's no need to fix
> > > > DRM to use the return value before pushing this patch set to mainline,
> > > > but I'd like a v2 with an int return value).
> > >
> > > Why? What's the use of returning an error if you know up front that it
> > > can't be used? This should be changed if or when we "fix" DRM to
> > > propagate errors.
> >
> > Because not doing so now will require us to change (potentially) lots of
> > panel drivers at that time. It's much easier to have each panel driver
> > developer implement the required code in his/her driver than having a
> > single developer refactoring the code later and have to touch all
> > drivers. If your concern is that the error paths won't be testable at the
> > moment, you could easily already add a WARN_ON() to the caller to catch
> > problems.
>
> I don't mind fixing potentially many drivers. The conversion is pretty
> mechanical and therefore easy. We even have tools such as coccinelle to
> help with it. But we've probably spent more time arguing the point than
> it would take to make this simple change, so I'll just go and change the
> return value to an int and return an error.
Thank you.
> Perhaps if I'm in the mood I'll even write up a patch to propagate the
> error further.
>
> > > > > > > > Instead of hardcoding the modes in the driver, which would
> > > > > > > > then require to be updated for every new simple panel model
> > > > > > > > (and we know there are lots of them), why don't you specify
> > > > > > > > the modes in the panel DT node ? The simple panel driver would
> > > > > > > > then become much more generic. It would also allow board
> > > > > > > > designers to tweak h/v sync timings depending on the system
> > > > > > > > requirements.
> > > > > > >
> > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when
> > > > > > > power sequences were designed (and NAKed at the last minute), we
> > > > > > > all decided that the right thing to do would be to use specific
> > > > > > > compatible values for individual panels, because that would
> > > > > > > allow us to encode the power sequencing within the driver. And
> > > > > > > when we already have the panel model encoded in the compatible
> > > > > > > value, we might just as well encode the mode within the driver
> > > > > > > for that panel. Otherwise we'll have to keep adding the same
> > > > > > > mode timings for every board that uses the same panel.
> > > > > > >
> > > > > > > I do agree though that it might be useful to tweak the mode in
> > > > > > > case the default one doesn't work. How about we provide a means
> > > > > > > to override the mode encoded in the driver using one specified
> > > > > > > in the DT? That's obviously a backwards-compatible change, so it
> > > > > > > could be added if or when it becomes necessary.
> > > > > >
> > > > > > I share Tomi's point of view here. Your three panels use the same
> > > > > > power sequence, so I believe we should have a generic panel
> > > > > > compatible string that would use modes described in DT for the
> > > > > > common case. Only if a panel required something more complex which
> > > > > > can't (or which could, but won't for any reason) accurately be
> > > > > > described in DT would you need to extend the driver.
> > > > >
> > > > > I don't see the point quite frankly. You seem to be assuming that
> > > > > every panel will only be used in a single board.
> > > >
> > > > No, but in practice that's often the case, at least for boards with
> > > > .dts files in the mainline kernel.
> > > >
> > > > > However what you're proposing will require the mode timings to be
> > > > > repeated in every board's DT file, if the same panel is ever used on
> > > > > more than a single board.
> > > >
> > > > Is that a problem ? You could #include a .dtsi for the panel that
> > > > would specify the video mode if you want to avoid multiple copies.
> > >
> > > Yes, I don't think it's desirable to duplicate data needlessly in DT. It
> > > also makes the binding more difficult to use. If I know that the panel
> > > is one supported by the simple-panel binding, I can just go and add the
> > > right compatible value without having to bother looking up the mode
> > > timings and duplicating them. That way DT writers get to concern
> > > themselves only with the variable data.
> >
> > I've had a quick chat with Dave Airlie and Hans de Goede yesterday about
> > this. As most panels will use standard timings, Hans proposed adding a
> > timings property that would reference the standard timings the panel uses
> > (this could be a string or integer defined in include/dt-bindings/...).
> > In most case DT would just have a single property to select the timings,
> > and only in more complex cases where the system designer wants to use
> > custom timings would the timings need to be manually defined.
>
> We can do the same thing within the kernel. We already have a list of
> standard EDID/HDMI/CEA display modes, so we could similarly add a new
> list of common display panel modes and make each driver reference that
> instead of defining a custom one.
Sure. My point is that I would like to avoid adding zillions of compatible
properties to the driver, when we could use a single property in the DT
bindings that would specify the timings instead. This would lower the amount
of changes made to the simple panel driver, while keeping DT simple enough (at
least in my opinion).
> And that still enables us to add a property that would allow DT writers
> to override the display mode if they need to.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 11:33 UTC (permalink / raw)
To: Stephen Warren
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <526999DA.7080409-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Hi Stephen,
On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> >> ...
> >>
> >>>> As I said, anything that really needs a CDF binding to work
> >>>> likely isn't "simple" anymore, therefore a separate driver can
> >>>> easily be justified.
> >>>
> >>> The system as a whole would be more complex, but the panel could be
> >>> the same. We can't have two drivers for the same piece of hardware
> >>> in the DT world, as there will be a single compatible string and no
> >>> way to choose between the drivers (unlike the board code world that
> >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> >>> and live happily with that ever after).
> >>
> >> That's not true. We can certainly define two different compatible
> >> values for a piece of HW if we have to. We can easily control whether
> >> they are handled by the same or different drivers in the OS.
> >
> > From an implementation point of view, sure. But from a conceptual point of
> > view, that would make the DT bindings pretty Linux-specific, with a
> > description of what the operating system should do instead of a
> > description of what the hardware looks like. My understanding is that
> > we've tried pretty hard in the past not to open that Pandora's box.
> >
> > The case I'm mostly concerned about would be two different compatibility
> > strings to select whether the device should be handled by a KMS or V4L
> > driver. I don't think that's a good idea.
>
> I wouldn't think of the two compatible values as selecting different
> specific Linux drivers, but rather they simply describe the HW in
> different levels of detail. The fact that if we know a certain level of
> detail about the HW means that Linux can and does create a KMS driver
> rather than a V4L2 driver seems like a detail that's completely hidden
> inside the OS.
I expect the same level of details to be needed on both the KMS and V4L sides.
Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
-kms to different names wouldn't fundamentally change the problem.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH 1/1] backlight: hx8357: Remove redundant of_match_ptr
From: Sachin Kamat @ 2013-10-25 12:11 UTC (permalink / raw)
To: linux-fbdev
'hx8357_dt_ids' is always compiled in. Hence of_match_ptr is
not needed.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/backlight/hx8357.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index c7af8c4..5a4dd33 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -685,7 +685,7 @@ static struct spi_driver hx8357_driver = {
.remove = hx8357_remove,
.driver = {
.name = "hx8357",
- .of_match_table = of_match_ptr(hx8357_dt_ids),
+ .of_match_table = hx8357_dt_ids,
},
};
--
1.7.9.5
^ permalink raw reply related
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-25 12:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1580941.z2CDo8WmRF@avalon>
[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]
On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote:
> Hi Stephen,
>
> On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > >> ...
> > >>
> > >>>> As I said, anything that really needs a CDF binding to work
> > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > >>>> easily be justified.
> > >>>
> > >>> The system as a whole would be more complex, but the panel could be
> > >>> the same. We can't have two drivers for the same piece of hardware
> > >>> in the DT world, as there will be a single compatible string and no
> > >>> way to choose between the drivers (unlike the board code world that
> > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > >>> and live happily with that ever after).
> > >>
> > >> That's not true. We can certainly define two different compatible
> > >> values for a piece of HW if we have to. We can easily control whether
> > >> they are handled by the same or different drivers in the OS.
> > >
> > > From an implementation point of view, sure. But from a conceptual point of
> > > view, that would make the DT bindings pretty Linux-specific, with a
> > > description of what the operating system should do instead of a
> > > description of what the hardware looks like. My understanding is that
> > > we've tried pretty hard in the past not to open that Pandora's box.
> > >
> > > The case I'm mostly concerned about would be two different compatibility
> > > strings to select whether the device should be handled by a KMS or V4L
> > > driver. I don't think that's a good idea.
> >
> > I wouldn't think of the two compatible values as selecting different
> > specific Linux drivers, but rather they simply describe the HW in
> > different levels of detail. The fact that if we know a certain level of
> > detail about the HW means that Linux can and does create a KMS driver
> > rather than a V4L2 driver seems like a detail that's completely hidden
> > inside the OS.
>
> I expect the same level of details to be needed on both the KMS and V4L sides.
> Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
> bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
> and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
> -kms to different names wouldn't fundamentally change the problem.
I think that we're doing something fundamentally wrong if we use the
same device (with the same functionality) in two different subsystems.
If the device is used to display something, shouldn't we be moving the
driver to DRM?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 13:47 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025081314.GC19622-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]
Hi Thierry,
On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > >> ...
> > >>
> > >>>> As I said, anything that really needs a CDF binding to work
> > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > >>>> easily be justified.
> > >>>
> > >>> The system as a whole would be more complex, but the panel could be
> > >>> the same. We can't have two drivers for the same piece of hardware
> > >>> in the DT world, as there will be a single compatible string and no
> > >>> way to choose between the drivers (unlike the board code world that
> > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > >>> and live happily with that ever after).
> > >>
> > >> That's not true. We can certainly define two different compatible
> > >> values for a piece of HW if we have to. We can easily control whether
> > >> they are handled by the same or different drivers in the OS.
> > >
> > > From an implementation point of view, sure. But from a conceptual point
> > > of view, that would make the DT bindings pretty Linux-specific, with a
> > > description of what the operating system should do instead of a
> > > description of what the hardware looks like. My understanding is that
> > > we've tried pretty hard in the past not to open that Pandora's box.
> > >
> > > The case I'm mostly concerned about would be two different compatibility
> > > strings to select whether the device should be handled by a KMS or V4L
> > > driver. I don't think that's a good idea.
> >
> > I wouldn't think of the two compatible values as selecting different
> > specific Linux drivers, but rather they simply describe the HW in
> > different levels of detail. The fact that if we know a certain level of
> > detail about the HW means that Linux can and does create a KMS driver
> > rather than a V4L2 driver seems like a detail that's completely hidden
> > inside the OS.
>
> I've had a somewhat similar idea the other day but couldn't really put
> it into words. Interestingly someone else mentioned a similar concept in
> a different thread which I think describes what I had in mind as well.
>
> I was wondering if we couldn't use two compatible values to denote two
> interfaces that the device implements. Something along the lines of:
>
> compatible = "vendor,block-name", "encoder";
>
> So a driver could primarily match on "vendor,block-name", but at the
> same time it could use the additional information of being required to
> implement "encoder" to expose an additonal interface.
Let's take the hardware architecture described in
http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as
an example. The green blocks are part of the capture pipeline and handled by
the V4L subsystem. The blue blocks are part of the display pipeline and
handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be
controlled by a KMS driver and the second one by a V4L driver.
The two instances are identical, so their DT nodes will show no difference if
we stick to a hardware description only. There would then be no way to bind to
different drivers.
> I suppose that perhaps something like a device_type property could be
> used for that as well, and that might even be the more correct thing to
> do.
>
> We already do something similar to make GPIO controllers expose an
> interrupt chip by adding an interrupt-controller property. We also use
> the gpio-controller property to mark a device node as exposing the GPIO
> interface for that matter.
>
> So if a HW block can actually implement two different interfaces, each
> of them being optional, then there should be ways to represent that in
> DT as well. We already do that for "simpler" HW blocks, so there's no
> reason we shouldn't be able to do the same with multimedia components.
>
> If it's really an encoder, though, the problem might be different,
> though, since the interface (at a hardware or functional level if you
> will) remains the same. But I think in that case it's something that
> needs to be figured out internally by the OS. In my opinion, if we are
> in a situation where we have two different drivers in two subsystems for
> the same device, then we're doing something wrong and it should be fixed
> at that level, not by quirking the DT into making a decision for us.
I tend to agree, and I'd like to be able to share drivers between V4L and KMS.
This is way down the road though.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 13:49 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025122925.GA24720-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]
Hi Thierry,
On Friday 25 October 2013 14:29:26 Thierry Reding wrote:
> On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote:
> > On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > >> ...
> > > >>
> > > >>>> As I said, anything that really needs a CDF binding to work
> > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > >>>> easily be justified.
> > > >>>
> > > >>> The system as a whole would be more complex, but the panel could be
> > > >>> the same. We can't have two drivers for the same piece of hardware
> > > >>> in the DT world, as there will be a single compatible string and no
> > > >>> way to choose between the drivers (unlike the board code world that
> > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > > >>> and live happily with that ever after).
> > > >>
> > > >> That's not true. We can certainly define two different compatible
> > > >> values for a piece of HW if we have to. We can easily control whether
> > > >> they are handled by the same or different drivers in the OS.
> > > >
> > > > From an implementation point of view, sure. But from a conceptual
> > > > point of view, that would make the DT bindings pretty Linux-specific,
> > > > with a description of what the operating system should do instead of a
> > > > description of what the hardware looks like. My understanding is that
> > > > we've tried pretty hard in the past not to open that Pandora's box.
> > > >
> > > > The case I'm mostly concerned about would be two different
> > > > compatibility strings to select whether the device should be handled
> > > > by a KMS or V4L driver. I don't think that's a good idea.
> > >
> > > I wouldn't think of the two compatible values as selecting different
> > > specific Linux drivers, but rather they simply describe the HW in
> > > different levels of detail. The fact that if we know a certain level of
> > > detail about the HW means that Linux can and does create a KMS driver
> > > rather than a V4L2 driver seems like a detail that's completely hidden
> > > inside the OS.
> >
> > I expect the same level of details to be needed on both the KMS and V4L
> > sides. Taking the example of the ADV7511 HDMI transmitter, the only
> > change in the DT bindings between KMS and V4L would be the compatible
> > string. "adi,adv7511-v4l" and "adi,adv7511-kms" is an option that I don't
> > really like. Renaming -v4l and -kms to different names wouldn't
> > fundamentally change the problem.
>
> I think that we're doing something fundamentally wrong if we use the
> same device (with the same functionality) in two different subsystems.
> If the device is used to display something, shouldn't we be moving the
> driver to DRM?
Let's discuss this in reply to the e-mail from this thread in which I mention
the ADV7511 example.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-25 14:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1394972.QJSGNM8CJk@avalon>
[-- Attachment #1: Type: text/plain, Size: 5732 bytes --]
On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > >> ...
> > > >>
> > > >>>> As I said, anything that really needs a CDF binding to work
> > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > >>>> easily be justified.
> > > >>>
> > > >>> The system as a whole would be more complex, but the panel could be
> > > >>> the same. We can't have two drivers for the same piece of hardware
> > > >>> in the DT world, as there will be a single compatible string and no
> > > >>> way to choose between the drivers (unlike the board code world that
> > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > > >>> and live happily with that ever after).
> > > >>
> > > >> That's not true. We can certainly define two different compatible
> > > >> values for a piece of HW if we have to. We can easily control whether
> > > >> they are handled by the same or different drivers in the OS.
> > > >
> > > > From an implementation point of view, sure. But from a conceptual point
> > > > of view, that would make the DT bindings pretty Linux-specific, with a
> > > > description of what the operating system should do instead of a
> > > > description of what the hardware looks like. My understanding is that
> > > > we've tried pretty hard in the past not to open that Pandora's box.
> > > >
> > > > The case I'm mostly concerned about would be two different compatibility
> > > > strings to select whether the device should be handled by a KMS or V4L
> > > > driver. I don't think that's a good idea.
> > >
> > > I wouldn't think of the two compatible values as selecting different
> > > specific Linux drivers, but rather they simply describe the HW in
> > > different levels of detail. The fact that if we know a certain level of
> > > detail about the HW means that Linux can and does create a KMS driver
> > > rather than a V4L2 driver seems like a detail that's completely hidden
> > > inside the OS.
> >
> > I've had a somewhat similar idea the other day but couldn't really put
> > it into words. Interestingly someone else mentioned a similar concept in
> > a different thread which I think describes what I had in mind as well.
> >
> > I was wondering if we couldn't use two compatible values to denote two
> > interfaces that the device implements. Something along the lines of:
> >
> > compatible = "vendor,block-name", "encoder";
> >
> > So a driver could primarily match on "vendor,block-name", but at the
> > same time it could use the additional information of being required to
> > implement "encoder" to expose an additonal interface.
>
> Let's take the hardware architecture described in
> http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as
> an example. The green blocks are part of the capture pipeline and handled by
> the V4L subsystem. The blue blocks are part of the display pipeline and
> handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be
> controlled by a KMS driver and the second one by a V4L driver.
>
> The two instances are identical, so their DT nodes will show no difference if
> we stick to a hardware description only. There would then be no way to bind to
> different drivers.
I don't quite see why some of the green parts couldn't be part of the
display (KMS) pipeline. I thought I remember you say something about
implementing deep pipelining support in one of the more recent talks.
This sounds exactly like a case where this would be useful.
Obviously as long as that work isn't finished that won't help you, but I
think that providing a way to pass a video stream object from V4L2 to
DRM/KMS would be a more proper solution to this problem.
> > I suppose that perhaps something like a device_type property could be
> > used for that as well, and that might even be the more correct thing to
> > do.
> >
> > We already do something similar to make GPIO controllers expose an
> > interrupt chip by adding an interrupt-controller property. We also use
> > the gpio-controller property to mark a device node as exposing the GPIO
> > interface for that matter.
> >
> > So if a HW block can actually implement two different interfaces, each
> > of them being optional, then there should be ways to represent that in
> > DT as well. We already do that for "simpler" HW blocks, so there's no
> > reason we shouldn't be able to do the same with multimedia components.
> >
> > If it's really an encoder, though, the problem might be different,
> > though, since the interface (at a hardware or functional level if you
> > will) remains the same. But I think in that case it's something that
> > needs to be figured out internally by the OS. In my opinion, if we are
> > in a situation where we have two different drivers in two subsystems for
> > the same device, then we're doing something wrong and it should be fixed
> > at that level, not by quirking the DT into making a decision for us.
>
> I tend to agree, and I'd like to be able to share drivers between V4L and KMS.
> This is way down the road though.
My point is that I don't see a need for sharing the drivers if we can
make both V4L2 and DRM/KMS interoperate well enough to cover such use
cases.
Why share the drivers if we can make it work by sharing the data?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 14:22 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025141029.GD1551-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6360 bytes --]
Hi Thierry,
On Friday 25 October 2013 16:10:30 Thierry Reding wrote:
> On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote:
> > On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> > > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > > >> ...
> > > > >>
> > > > >>>> As I said, anything that really needs a CDF binding to work
> > > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > > >>>> easily be justified.
> > > > >>>
> > > > >>> The system as a whole would be more complex, but the panel could
> > > > >>> be the same. We can't have two drivers for the same piece of
> > > > >>> hardware in the DT world, as there will be a single compatible
> > > > >>> string and no way to choose between the drivers (unlike the board
> > > > >>> code world that could set device names to "foo- encoder-v4l2" or
> > > > >>> "foo-encoder-drm" and live happily with that ever after).
> > > > >>
> > > > >> That's not true. We can certainly define two different compatible
> > > > >> values for a piece of HW if we have to. We can easily control
> > > > >> whether they are handled by the same or different drivers in the
> > > > >> OS.
> > > > >
> > > > > From an implementation point of view, sure. But from a conceptual
> > > > > point of view, that would make the DT bindings pretty Linux-
> > > > > specific, with a description of what the operating system should do
> > > > > instead of a description of what the hardware looks like. My
> > > > > understanding is that we've tried pretty hard in the past not to
> > > > > open that Pandora's box.
> > > > >
> > > > > The case I'm mostly concerned about would be two different
> > > > > compatibility strings to select whether the device should be handled
> > > > > by a KMS or V4L driver. I don't think that's a good idea.
> > > >
> > > > I wouldn't think of the two compatible values as selecting different
> > > > specific Linux drivers, but rather they simply describe the HW in
> > > > different levels of detail. The fact that if we know a certain level
> > > > of detail about the HW means that Linux can and does create a KMS
> > > > driver rather than a V4L2 driver seems like a detail that's completely
> > > > hidden inside the OS.
> > >
> > > I've had a somewhat similar idea the other day but couldn't really put
> > > it into words. Interestingly someone else mentioned a similar concept in
> > > a different thread which I think describes what I had in mind as well.
> > >
> > > I was wondering if we couldn't use two compatible values to denote two
> > >
> > > interfaces that the device implements. Something along the lines of:
> > > compatible = "vendor,block-name", "encoder";
> > >
> > > So a driver could primarily match on "vendor,block-name", but at the
> > > same time it could use the additional information of being required to
> > > implement "encoder" to expose an additonal interface.
> >
> > Let's take the hardware architecture described in
> > http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39)
> > as an example. The green blocks are part of the capture pipeline and
> > handled by the V4L subsystem. The blue blocks are part of the display
> > pipeline and handled by the KMS subsystem. One ADV7511 HDMI transmitter
> > instance need to be controlled by a KMS driver and the second one by a
> > V4L driver.
> >
> > The two instances are identical, so their DT nodes will show no difference
> > if we stick to a hardware description only. There would then be no way to
> > bind to different drivers.
>
> I don't quite see why some of the green parts couldn't be part of the
> display (KMS) pipeline.
Because there might be no blue pipeline in the device, just the green one. The
green pipeline is a video capture pipeline and happens to have an HDMI output
with a deep pipeline only, with no frame buffer. We can't create a KMS driver
for that, as there's no frame buffer and no CRTC.
> I thought I remember you say something about implementing deep pipelining
> support in one of the more recent talks. This sounds exactly like a case
> where this would be useful.
>
> Obviously as long as that work isn't finished that won't help you, but I
> think that providing a way to pass a video stream object from V4L2 to
> DRM/KMS would be a more proper solution to this problem.
We need that as well, but it won't solve the problem when no KMS device is
present.
> > > I suppose that perhaps something like a device_type property could be
> > > used for that as well, and that might even be the more correct thing to
> > > do.
> > >
> > > We already do something similar to make GPIO controllers expose an
> > > interrupt chip by adding an interrupt-controller property. We also use
> > > the gpio-controller property to mark a device node as exposing the GPIO
> > > interface for that matter.
> > >
> > > So if a HW block can actually implement two different interfaces, each
> > > of them being optional, then there should be ways to represent that in
> > > DT as well. We already do that for "simpler" HW blocks, so there's no
> > > reason we shouldn't be able to do the same with multimedia components.
> > >
> > > If it's really an encoder, though, the problem might be different,
> > > though, since the interface (at a hardware or functional level if you
> > > will) remains the same. But I think in that case it's something that
> > > needs to be figured out internally by the OS. In my opinion, if we are
> > > in a situation where we have two different drivers in two subsystems for
> > > the same device, then we're doing something wrong and it should be fixed
> > > at that level, not by quirking the DT into making a decision for us.
> >
> > I tend to agree, and I'd like to be able to share drivers between V4L and
> > KMS. This is way down the road though.
>
> My point is that I don't see a need for sharing the drivers if we can
> make both V4L2 and DRM/KMS interoperate well enough to cover such use
> cases.
>
> Why share the drivers if we can make it work by sharing the data?
Would you like to try and merge the two subsystems ? :-)
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Stephen Warren @ 2013-10-25 15:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1580941.z2CDo8WmRF@avalon>
On 10/25/2013 12:33 PM, Laurent Pinchart wrote:
> On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
>> On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
...
>>> The case I'm mostly concerned about would be two different compatibility
>>> strings to select whether the device should be handled by a KMS or V4L
>>> driver. I don't think that's a good idea.
>>
>> I wouldn't think of the two compatible values as selecting different
>> specific Linux drivers, but rather they simply describe the HW in
>> different levels of detail. The fact that if we know a certain level of
>> detail about the HW means that Linux can and does create a KMS driver
>> rather than a V4L2 driver seems like a detail that's completely hidden
>> inside the OS.
>
> I expect the same level of details to be needed on both the KMS and V4L sides.
> Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
> bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
> and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
> -kms to different names wouldn't fundamentally change the problem.
Yes, two compatible values such as "adi,adv7511-v4l" and
"adi,adv7511-kms" sounds like a bad idea.
Rather, shouldn't we have just "adi,adv7511", and the driver for that
should register itself in a way that allows either/both the top-level
DRM or top-level V4L drivers to find it, and make use of its services.
There are plenty of drivers in the kernel already that export services
through two different subsystems, e.g. a GPIO HW module that registers
as both a struct gpio_chip and a struct irq_chip, or both a struct
gpio_chip and a pinctrl device.
^ permalink raw reply
* Re: [PATCHv3][ 3/5] video: mx3fb: Add device tree suport.
From: Sascha Hauer @ 2013-10-26 0:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20131025195040.0CCC3C404DA@trevor.secretlab.ca>
On Fri, Oct 25, 2013 at 08:50:40PM +0100, Grant Likely wrote:
> On Wed, 23 Oct 2013 14:43:47 +0200, Denis Carikli <denis@eukrea.com> wrote:
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: linux-fbdev@vger.kernel.org
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v2->v3:
> > - The device tree bindings were reworked in order to make it look more like the
> > IPUv3 bindings.
> > - The interface_pix_fmt property now looks like the IPUv3 one.
> > ---
> > .../devicetree/bindings/video/fsl,mx3-fb.txt | 35 ++++++
> > drivers/video/Kconfig | 2 +
> > drivers/video/mx3fb.c | 125 +++++++++++++++++---
> > 3 files changed, 147 insertions(+), 15 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> > new file mode 100644
> > index 0000000..0b31374
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> > @@ -0,0 +1,35 @@
> > +Freescale MX3 fb
> > +========
> > +
> > +Required properties:
> > +- compatible: Should be "fsl,mx3fb". compatible chips include the imx31 and the
> > + imx35.
> > +- reg: should be register base and length as documented in the datasheet.
> > +- clocks: Handle to the ipu_gate clock.
> > +
> > +Example:
> > +
> > +lcdc: mx3fb@53fc00b4 {
> > + compatible = "fsl,mx3-fb";
> > + reg = <0x53fc00b4 0x0b>;
> > + clocks = <&clks 55>;
> > +};
>
> This (and some of the other bindings) are trivial, and they are all
> associated with a single SoC. I think it would be better to collect all
> the mx3 bindings into a single file rather than distributing them all
> over the bindings tree.
>
> I started thinking about this after some of the DT conversations in
> Edinburgh this week. Unless there is a high likelyhood of components
> being used separately, I think it is far more useful to collect all the
> bindings for an SoC into a single file. It will certainly reduce a lot
> of the boilerplate that we've been collecting in bindings documentation
> files.
>
> A long time ago I took that approach for the mpc5200 documentation[1].
> Take a look at that organization and let me know what you think.
I don't think this is a good idea. When a new SoC comes out we don't
know which components will be reused on the next SoC. This will cause a
lot of bikeshedding when it actually is reused and then has to be moved.
Also I would find it quite inconsistent if I had to lookup some devices
in a SoC file and most bindings in subsystem specific files. So when
searching for a binding I would first have to know if the hardware is
unique to the SoC or not.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
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