[PATCH] drm/exynos: Use platform_register/unregister_drivers()

[Thierry Reding]

From: Thierry Reding <tred...@nvidia.com>

Replace the driver-specific implementations with the ones implemented in
the core.

Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 42 +++--
 1 file changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 2c6019d6a205..3152bca62127 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -678,54 +678,28 @@ static int exynos_drm_register_devices(void)
return 0;
 }
 
-static void exynos_drm_unregister_drivers(struct platform_driver * const *drv,
- int count)
-{
-   while (--count >= 0)
-   platform_driver_unregister(drv[count]);
-}
-
-static int exynos_drm_register_drivers(struct platform_driver * const *drv,
-  int count)
-{
-   int i, ret;
-
-   for (i = 0; i < count; ++i) {
-   ret = platform_driver_register(drv[i]);
-   if (!ret)
-   continue;
-
-   while (--i >= 0)
-   platform_driver_unregister(drv[i]);
-
-   return ret;
-   }
-
-   return 0;
-}
-
 static inline int exynos_drm_register_kms_drivers(void)
 {
-   return exynos_drm_register_drivers(exynos_drm_kms_drivers,
-   ARRAY_SIZE(exynos_drm_kms_drivers));
+   return platform_register_drivers(exynos_drm_kms_drivers,
+ARRAY_SIZE(exynos_drm_kms_drivers));
 }
 
 static inline int exynos_drm_register_non_kms_drivers(void)
 {
-   return exynos_drm_register_drivers(exynos_drm_non_kms_drivers,
-   ARRAY_SIZE(exynos_drm_non_kms_drivers));
+   return platform_register_drivers(exynos_drm_non_kms_drivers,
+
ARRAY_SIZE(exynos_drm_non_kms_drivers));
 }
 
 static inline void exynos_drm_unregister_kms_drivers(void)
 {
-   exynos_drm_unregister_drivers(exynos_drm_kms_drivers,
-   ARRAY_SIZE(exynos_drm_kms_drivers));
+   platform_unregister_drivers(exynos_drm_kms_drivers,
+   ARRAY_SIZE(exynos_drm_kms_drivers));
 }
 
 static inline void exynos_drm_unregister_non_kms_drivers(void)
 {
-   exynos_drm_unregister_drivers(exynos_drm_non_kms_drivers,
-   ARRAY_SIZE(exynos_drm_non_kms_drivers));
+   platform_unregister_drivers(exynos_drm_non_kms_drivers,
+   ARRAY_SIZE(exynos_drm_non_kms_drivers));
 }
 
 static int exynos_drm_init(void)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Thierry Reding]

On Sun, Nov 22, 2015 at 09:13:17AM +0900, Krzysztof Kozlowski wrote:
> 2015-11-22 3:14 GMT+09:00 Anand Moon :
> > Hi Krzysztof,
> >
> > On 21 November 2015 at 18:37, Krzysztof Kozlowski
> >  wrote:
> >> 2015-11-21 21:11 GMT+09:00 Anand Moon :
> >>> hi Krzysztof,
> >>>
> >>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
> >>>  wrote:
>  2015-11-21 18:40 GMT+09:00 Anand Moon :
> > hi Krzysztof,
> >
> > On 21 November 2015 at 09:56, Krzysztof Kozlowski 
> > 
> > wrote:
> >>
> >> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
> >> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
> >> > (pwm: Set enable state properly on failed call to enable)
> >> > introduce double lock of mutex on pwm_enable
> >> >
> >> > Changes fix the following debug lock warning.
> >> >
> >> > [2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
> >> > __mutex_lock_slowpath+0x3bc/0x404()
> >> > [2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
> >>
> >> Hi Anand!
> >>
> >> Yeah, I am hitting this as well. Annoying. However your description is
> >> inaccurate. Double lock of mutex does not cause warnings of sleeping or
> >> locking in atomic context (if you build with debug sleep atomic you 
> >> will
> >> see different warning). More over you are partially reverting the 
> >> commit
> >> d1cd21427747 ("pwm: Set enable state properly on failed call to 
> >> enable")
> >> without proper explanation:
> >> 1. why this locking is inappropriate;
> >> 2. why it is safe to remove the mutex_lock().
> >>
> >> Please find the real problem, fix the real problem and throughly 
> >> explain
> >> the real issue.
> >>
> >> I was suspecting some weird changes in timers. For !irqsafe timer I
> >> think it is safe to use mutexes... but apparently not. Maybe a work
> >> should be scheduled from function called by timer?
> >>
> >> Warning with debug atomic sleep:
> >> [   16.047517] BUG: sleeping function called from invalid context at
> >> ../kernel/locking/mutex.c:617
> >> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: 
> >> swapper/0
> >> [   16.061402] INFO: lockdep is turned off.
> >> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
> >> [   16.070524]
> >> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> >> 4.4.0-rc1-00040-g28c429565d4f #290
> >> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> [   16.086215] [] (unwind_backtrace) from []
> >> (show_stack+0x10/0x14)
> >> [   16.093938] [] (show_stack) from []
> >> (dump_stack+0x70/0xbc)
> >> [   16.101122] [] (dump_stack) from []
> >> (mutex_lock_nested+0x24/0x474)
> >> [   16.109009] [] (mutex_lock_nested) from []
> >> (pwm_enable+0x20/0x7c)
> >> [   16.116799] [] (pwm_enable) from []
> >> (led_heartbeat_function+0xdc/0x140)
> >> [   16.125119] [] (led_heartbeat_function) from []
> >> (call_timer_fn+0x6c/0xf4)
> >> [   16.133606] [] (call_timer_fn) from []
> >> (run_timer_softirq+0x1a8/0x230)
> >> [   16.141846] [] (run_timer_softirq) from []
> >> (__do_softirq+0x134/0x2c0)
> >> [   16.149982] [] (__do_softirq) from []
> >> (irq_exit+0xd0/0x114)
> >> [   16.157255] [] (irq_exit) from []
> >> (__handle_domain_irq+0x70/0xe4)
> >> [   16.165056] [] (__handle_domain_irq) from []
> >> (gic_handle_irq+0x4c/0x94)
> >> [   16.173376] [] (gic_handle_irq) from []
> >> (__irq_svc+0x58/0x98)
> >> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
> >> [   16.185823] df40:
> >>c0010dcc 
> >> [   16.193997] df60: c08cdfa8  c05f57c4  c08ca520
> >> c08ce4bc c08c7364 c08ce4b4
> >> [   16.202141] df80: c09121ca  0001 c08cdfa8 c0010dcc
> >> c0010dd0 600f0013 
> >> [   16.210291] [] (__irq_svc) from []
> >> (arch_cpu_idle+0x20/0x3c)
> >> [   16.217661] [] (arch_cpu_idle) from []
> >> (cpu_startup_entry+0x17c/0x26c)
> >> [   16.225899] [] (cpu_startup_entry) from []
> >> (start_kernel+0x368/0x3d0)
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> >> > [2.701737] Modules linked in:
> >> > [2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 
> >> > 4.4.0-rc1-xu4f
> >> > #24
> >> > [2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> > [2.701787] [] (unwind_backtrace) from []
> >> > (show_stack+0x10/0x14)
> >> > [2.701808] [] (show_stack) from []
> >> > (dump_stack+0x84/0xc4)
> >> > [2.701824] [] (dump_stack) from []
> >> > (warn_slowpath_common+0x80/0xb0)
> >> > [2.701836] [] 

Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active

[Thierry Reding]

On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace
> direct cross-driver call with drm mode). The whole atomic update
> was failing if the hdmi display is not present/active. Add a test
> to only run atomic_check() if the CRTC is active.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b3ba27f..1d3ca0a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
>  {
>   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>  
> + if (!state->active)
> + return 0;
> +
>   if (exynos_crtc->ops->atomic_check)
>   return exynos_crtc->ops->atomic_check(exynos_crtc, state);
>  

This looks like something that the core should be doing.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 2/6] ARM: s3c24xx: rx1950: Use PWM lookup table

[Thierry Reding]

On Wed, Oct 07, 2015 at 10:35:54AM +0900, Krzysztof Kozlowski wrote:
> On 05.10.2015 21:47, Thierry Reding wrote:
> > Use a PWM lookup table to provide the PWM to the pwm-backlight device.
> > The driver has a legacy code path that is required only because boards
> > still use the legacy method of requesting PWMs by global ID. Replacing
> > these usages allows that legacy fallback to be removed.
> > 
> > Cc: Kukjin Kim <kg...@kernel.org>
> > Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
> > Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c 
> > b/arch/arm/mach-s3c24xx/mach-rx1950.c
> > index 1d35ff375a01..774c982a7b7e 100644
> > --- a/arch/arm/mach-s3c24xx/mach-rx1950.c
> > +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
> > @@ -375,6 +375,11 @@ static struct s3c2410fb_mach_info rx1950_lcd_cfg = {
> >  
> >  };
> >  
> > +static struct pwm_lookup rx1950_pwm_lookup[] = {
> > +   PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight.0", NULL, 48000,
> 
> Why the dev_id is "pwm-backlight.0"? Created platform device name is
> "pwm-backlight".

"pwm-backlight" is only the base name. The platform bus code will use
the platform_device.id field and append it to the name as . unless
it is -1 in which case it will be skipped. So in the rx1950_backlight
device the .id field isn't initialized at all, so it will be zeroed
out and hence the device name will become "pwm-backlight.0".

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 5/6] ARM: s3c64xx: hmt: Use PWM lookup table

[Thierry Reding]

On Wed, Oct 07, 2015 at 10:37:42AM +0900, Krzysztof Kozlowski wrote:
> On 05.10.2015 21:47, Thierry Reding wrote:
> > Use a PWM lookup table to provide the PWM to the pwm-backlight device.
> > The driver has a legacy code path that is required only because boards
> > still use the legacy method of requesting PWMs by global ID. Replacing
> > these usages allows that legacy fallback to be removed.
> > 
> > Cc: Kukjin Kim <kg...@kernel.org>
> > Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
> > Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
> > ---
> >  arch/arm/mach-s3c64xx/mach-hmt.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c 
> > b/arch/arm/mach-s3c64xx/mach-hmt.c
> > index e4b087c58ee6..816b39d1e6d1 100644
> > --- a/arch/arm/mach-s3c64xx/mach-hmt.c
> > +++ b/arch/arm/mach-s3c64xx/mach-hmt.c
> > @@ -19,6 +19,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -73,6 +74,11 @@ static struct s3c2410_uartcfg hmt_uartcfgs[] __initdata 
> > = {
> > },
> >  };
> >  
> > +static struct pwm_lookup hmt_pwm_lookup[] = {
> > +   PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", NULL,
> 
> Same questions as in patch 2 - why suffix ".0" for "pwm-backlight"?

For the same reason that I explained in patch 2. Not sure if the .id = 0
was really supposed to be. For most devices it would probably make sense
to initialize it to -1 because they typically only have a single
backlight. But given that userspace might be using the name to control
the backlight via sysfs it's probably not a good idea to go and change
that.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 0/6] ARM: s3cxxxx: Use PWM lookup table

[Thierry Reding]

On Tue, Oct 06, 2015 at 09:28:50AM +0900, Krzysztof Kozlowski wrote:
> On 05.10.2015 21:47, Thierry Reding wrote:
> > Back when the PWM framework was introduced the concept of PWM lookup
> > tables was added to allow board code to register a table of PWM devices
> > and their association with consumers. The goal is to allow drivers to
> > use a unified method to request PWM devices. At the same time, since no
> > boards were exposing these tables, fallback code was kept in drivers to
> > allow old code to remain functional. In order to get rid of the legacy
> > fallback code, legacy users need to be updated to register PWM lookup
> > tables.
> > 
> > This series converts all s3c24xx and s3c64xx boards that hook up a pwm-
> > backlight device to use the new PWM lookup tables. All patches have been
> > compile-tested, but I don't have access to any of these boards, so I
> > couldn't verify that they really work.
> 
> Unfortunately I can't verify them neither. I don't have these boards. In
> that case I would prefer approach "if it ain't broken, don't touch
> it"... unless someone tests the patches of course.

Backlight on these boards is likely to be currently broken anyway. In
fact this series is part of an effort to restore backlight support for
these legacy boards (I sent out a similar series for PXA boards).

We're going to need to have this series merged so that we can remove the
legacy code that's being a lot of trouble maintaining. If this series
ends up breaking anything, I'd be happy to help fix things up.

Thierry


signature.asc
Description: PGP signature

[PATCH 6/6] ARM: s3c64xx: smartq: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c64xx/mach-smartq.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c 
b/arch/arm/mach-s3c64xx/mach-smartq.c
index b3d13537a7f0..7b8a3699795c 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -139,6 +140,11 @@ static struct platform_device smartq_usb_otg_vbus_dev = {
.dev.platform_data  = _usb_otg_vbus_pdata,
 };
 
+static struct pwm_lookup smartq_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", NULL,
+  10 / (1000 * 20), PWM_POLARITY_NORMAL),
+};
+
 static int smartq_bl_init(struct device *dev)
 {
 s3c_gpio_cfgpin(S3C64XX_GPF(15), S3C_GPIO_SFN(2));
@@ -147,10 +153,8 @@ static int smartq_bl_init(struct device *dev)
 }
 
 static struct platform_pwm_backlight_data smartq_backlight_data = {
-   .pwm_id = 1,
.max_brightness = 1000,
.dft_brightness = 600,
-   .pwm_period_ns  = 10 / (1000 * 20),
.enable_gpio= -1,
.init   = smartq_bl_init,
 };
@@ -396,5 +400,6 @@ void __init smartq_machine_init(void)
WARN_ON(smartq_usb_host_init());
WARN_ON(smartq_wifi_init());
 
+   pwm_add_table(smartq_pwm_lookup, ARRAY_SIZE(smartq_pwm_lookup));
platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
 }
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] ARM: s3c64xx: hmt: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c64xx/mach-hmt.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c
index e4b087c58ee6..816b39d1e6d1 100644
--- a/arch/arm/mach-s3c64xx/mach-hmt.c
+++ b/arch/arm/mach-s3c64xx/mach-hmt.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -73,6 +74,11 @@ static struct s3c2410_uartcfg hmt_uartcfgs[] __initdata = {
},
 };
 
+static struct pwm_lookup hmt_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", NULL,
+  10 / (100 * 256 * 20), PWM_POLARITY_NORMAL),
+};
+
 static int hmt_bl_init(struct device *dev)
 {
int ret;
@@ -110,10 +116,8 @@ static void hmt_bl_exit(struct device *dev)
 }
 
 static struct platform_pwm_backlight_data hmt_backlight_data = {
-   .pwm_id = 1,
.max_brightness = 100 * 256,
.dft_brightness = 40 * 256,
-   .pwm_period_ns  = 10 / (100 * 256 * 20),
.enable_gpio= -1,
.init   = hmt_bl_init,
.notify = hmt_bl_notify,
@@ -268,6 +272,7 @@ static void __init hmt_machine_init(void)
gpio_request(S3C64XX_GPF(13), "usb power");
gpio_direction_output(S3C64XX_GPF(13), 1);
 
+   pwm_add_table(hmt_pwm_lookup, ARRAY_SIZE(hmt_pwm_lookup));
platform_add_devices(hmt_devices, ARRAY_SIZE(hmt_devices));
 }
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] ARM: s3cxxxx: Use PWM lookup table

[Thierry Reding]

Back when the PWM framework was introduced the concept of PWM lookup
tables was added to allow board code to register a table of PWM devices
and their association with consumers. The goal is to allow drivers to
use a unified method to request PWM devices. At the same time, since no
boards were exposing these tables, fallback code was kept in drivers to
allow old code to remain functional. In order to get rid of the legacy
fallback code, legacy users need to be updated to register PWM lookup
tables.

This series converts all s3c24xx and s3c64xx boards that hook up a pwm-
backlight device to use the new PWM lookup tables. All patches have been
compile-tested, but I don't have access to any of these boards, so I
couldn't verify that they really work.

Thierry

Thierry Reding (6):
  ARM: s3c24xx: h1940: Use PWM lookup table
  ARM: s3c24xx: rx1950: Use PWM lookup table
  ARM: s3c64xx: smdk6410: Use PWM lookup table
  ARM: s3c64xx: crag6410: Use PWM lookup table
  ARM: s3c64xx: hmt: Use PWM lookup table
  ARM: s3c64xx: smartq: Use PWM lookup table

 arch/arm/mach-s3c24xx/mach-h1940.c| 10 +++---
 arch/arm/mach-s3c24xx/mach-rx1950.c   |  8 ++--
 arch/arm/mach-s3c64xx/dev-backlight.c |  4 
 arch/arm/mach-s3c64xx/mach-crag6410.c |  9 +++--
 arch/arm/mach-s3c64xx/mach-hmt.c  |  9 +++--
 arch/arm/mach-s3c64xx/mach-smartq.c   |  9 +++--
 arch/arm/mach-s3c64xx/mach-smdk6410.c |  8 +++-
 7 files changed, 41 insertions(+), 16 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] ARM: s3c24xx: h1940: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c24xx/mach-h1940.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c 
b/arch/arm/mach-s3c24xx/mach-h1940.c
index d40d4f5244c6..9f54300df4b3 100644
--- a/arch/arm/mach-s3c24xx/mach-h1940.c
+++ b/arch/arm/mach-s3c24xx/mach-h1940.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -469,6 +470,11 @@ static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = 
{
.ocr_avail = MMC_VDD_32_33,
 };
 
+static struct pwm_lookup h1940_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight", NULL, 36296,
+  PWM_POLARITY_NORMAL),
+};
+
 static int h1940_backlight_init(struct device *dev)
 {
gpio_request(S3C2410_GPB(0), "Backlight");
@@ -503,11 +509,8 @@ static void h1940_backlight_exit(struct device *dev)
 
 
 static struct platform_pwm_backlight_data backlight_data = {
-   .pwm_id = 0,
.max_brightness = 100,
.dft_brightness = 50,
-   /* tcnt = 0x31 */
-   .pwm_period_ns  = 36296,
.enable_gpio= -1,
.init   = h1940_backlight_init,
.notify = h1940_backlight_notify,
@@ -725,6 +728,7 @@ static void __init h1940_init(void)
gpio_request(H1940_LATCH_SD_POWER, "SD power");
gpio_direction_output(H1940_LATCH_SD_POWER, 0);
 
+   pwm_add_table(h1940_pwm_lookup, ARRAY_SIZE(h1940_pwm_lookup));
platform_add_devices(h1940_devices, ARRAY_SIZE(h1940_devices));
 
gpio_request(S3C2410_GPA(1), "Red LED blink");
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] ARM: s3c64xx: crag6410: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c64xx/mach-crag6410.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c 
b/arch/arm/mach-s3c64xx/mach-crag6410.c
index 65c426bc45f7..d13aa3f9bac4 100644
--- a/arch/arm/mach-s3c64xx/mach-crag6410.c
+++ b/arch/arm/mach-s3c64xx/mach-crag6410.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -108,11 +109,14 @@ static struct s3c2410_uartcfg crag6410_uartcfgs[] 
__initdata = {
},
 };
 
+static struct pwm_lookup crag6410_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight", NULL, 10,
+  PWM_POLARITY_NORMAL),
+};
+
 static struct platform_pwm_backlight_data crag6410_backlight_data = {
-   .pwm_id = 0,
.max_brightness = 1000,
.dft_brightness = 600,
-   .pwm_period_ns  = 10,   /* about 1kHz */
.enable_gpio= -1,
 };
 
@@ -843,6 +847,7 @@ static void __init crag6410_machine_init(void)
samsung_keypad_set_platdata(_keypad_data);
s3c64xx_spi0_set_platdata(NULL, 0, 2);
 
+   pwm_add_table(crag6410_pwm_lookup, ARRAY_SIZE(crag6410_pwm_lookup));
platform_add_devices(crag6410_devices, ARRAY_SIZE(crag6410_devices));
 
gpio_led_register_device(-1, _leds_pdata);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] ARM: s3c64xx: smdk6410: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c64xx/dev-backlight.c | 4 
 arch/arm/mach-s3c64xx/mach-smdk6410.c | 8 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/dev-backlight.c 
b/arch/arm/mach-s3c64xx/dev-backlight.c
index 38c323e68e3f..e62e789f9aee 100644
--- a/arch/arm/mach-s3c64xx/dev-backlight.c
+++ b/arch/arm/mach-s3c64xx/dev-backlight.c
@@ -69,7 +69,6 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data 
__initdata = {
.plat_data = {
.max_brightness = 255,
.dft_brightness = 255,
-   .pwm_period_ns  = 78770,
.enable_gpio= -1,
.init   = samsung_bl_init,
.exit   = samsung_bl_exit,
@@ -111,7 +110,6 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info 
*gpio_info,
samsung_bl_data = _bl_drvdata->plat_data;
 
/* Copy board specific data provided by user */
-   samsung_bl_data->pwm_id = bl_data->pwm_id;
samsung_bl_device->dev.parent = _device_pwm.dev;
 
if (bl_data->max_brightness)
@@ -120,8 +118,6 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info 
*gpio_info,
samsung_bl_data->dft_brightness = bl_data->dft_brightness;
if (bl_data->lth_brightness)
samsung_bl_data->lth_brightness = bl_data->lth_brightness;
-   if (bl_data->pwm_period_ns)
-   samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns;
if (bl_data->enable_gpio >= 0)
samsung_bl_data->enable_gpio = bl_data->enable_gpio;
if (bl_data->init)
diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c 
b/arch/arm/mach-s3c64xx/mach-smdk6410.c
index d590b88bd8a8..2722800d5c11 100644
--- a/arch/arm/mach-s3c64xx/mach-smdk6410.c
+++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -623,8 +624,12 @@ static struct samsung_bl_gpio_info smdk6410_bl_gpio_info = 
{
.func = S3C_GPIO_SFN(2),
 };
 
+static struct pwm_lookup smdk6410_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", NULL, 78770,
+  PWM_POLARITY_NORMAL),
+};
+
 static struct platform_pwm_backlight_data smdk6410_bl_data = {
-   .pwm_id = 1,
.enable_gpio = -1,
 };
 
@@ -695,6 +700,7 @@ static void __init smdk6410_machine_init(void)
 
platform_add_devices(smdk6410_devices, ARRAY_SIZE(smdk6410_devices));
 
+   pwm_add_table(smdk6410_pwm_lookup, ARRAY_SIZE(smdk6410_pwm_lookup));
samsung_bl_set(_bl_gpio_info, _bl_data);
 }
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] ARM: s3c24xx: rx1950: Use PWM lookup table

[Thierry Reding]

Use a PWM lookup table to provide the PWM to the pwm-backlight device.
The driver has a legacy code path that is required only because boards
still use the legacy method of requesting PWMs by global ID. Replacing
these usages allows that legacy fallback to be removed.

Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c 
b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 1d35ff375a01..774c982a7b7e 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -375,6 +375,11 @@ static struct s3c2410fb_mach_info rx1950_lcd_cfg = {
 
 };
 
+static struct pwm_lookup rx1950_pwm_lookup[] = {
+   PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight.0", NULL, 48000,
+  PWM_POLARITY_NORMAL),
+};
+
 static struct pwm_device *lcd_pwm;
 
 static void rx1950_lcd_power(int enable)
@@ -520,10 +525,8 @@ static int rx1950_backlight_notify(struct device *dev, int 
brightness)
 }
 
 static struct platform_pwm_backlight_data rx1950_backlight_data = {
-   .pwm_id = 0,
.max_brightness = 24,
.dft_brightness = 4,
-   .pwm_period_ns = 48000,
.enable_gpio = -1,
.init = rx1950_backlight_init,
.notify = rx1950_backlight_notify,
@@ -792,6 +795,7 @@ static void __init rx1950_init_machine(void)
gpio_direction_output(S3C2410_GPA(4), 0);
gpio_direction_output(S3C2410_GPJ(6), 0);
 
+   pwm_add_table(rx1950_pwm_lookup, ARRAY_SIZE(rx1950_pwm_lookup));
platform_add_devices(rx1950_devices, ARRAY_SIZE(rx1950_devices));
 
i2c_register_board_info(0, rx1950_i2c_devices,
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] drm/exynos: Suspend/resume is unused if !PM

[Thierry Reding]

From: Thierry Reding <tred...@nvidia.com>

Protect the suspend and resume callbacks with an #ifdef CONFIG_PM_SLEEP
guard to avoid "defined but not used" warnings.

Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index f0a5839bd226..e07a0fe16432 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -304,6 +304,7 @@ int exynos_atomic_commit(struct drm_device *dev, struct 
drm_atomic_state *state,
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
 static int exynos_drm_suspend(struct drm_device *dev, pm_message_t state)
 {
struct drm_connector *connector;
@@ -340,6 +341,7 @@ static int exynos_drm_resume(struct drm_device *dev)
 
return 0;
 }
+#endif
 
 static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
 {
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] drm/exynos: fimc: Clock control is unused if !PM

[Thierry Reding]

From: Thierry Reding <tred...@nvidia.com>

Protect the fimc_clk_ctrl() function with an #ifdef CONFIG_PM guard to
avoid "defined but not used" warnings.

Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 36 
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 2a652359af64..dd3a5e6d58c8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1206,23 +1206,6 @@ static struct exynos_drm_ipp_ops fimc_dst_ops = {
.set_addr = fimc_dst_set_addr,
 };
 
-static int fimc_clk_ctrl(struct fimc_context *ctx, bool enable)
-{
-   DRM_DEBUG_KMS("enable[%d]\n", enable);
-
-   if (enable) {
-   clk_prepare_enable(ctx->clocks[FIMC_CLK_GATE]);
-   clk_prepare_enable(ctx->clocks[FIMC_CLK_WB_A]);
-   ctx->suspended = false;
-   } else {
-   clk_disable_unprepare(ctx->clocks[FIMC_CLK_GATE]);
-   clk_disable_unprepare(ctx->clocks[FIMC_CLK_WB_A]);
-   ctx->suspended = true;
-   }
-
-   return 0;
-}
-
 static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
 {
struct fimc_context *ctx = dev_id;
@@ -1780,6 +1763,24 @@ static int fimc_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM
+static int fimc_clk_ctrl(struct fimc_context *ctx, bool enable)
+{
+   DRM_DEBUG_KMS("enable[%d]\n", enable);
+
+   if (enable) {
+   clk_prepare_enable(ctx->clocks[FIMC_CLK_GATE]);
+   clk_prepare_enable(ctx->clocks[FIMC_CLK_WB_A]);
+   ctx->suspended = false;
+   } else {
+   clk_disable_unprepare(ctx->clocks[FIMC_CLK_GATE]);
+   clk_disable_unprepare(ctx->clocks[FIMC_CLK_WB_A]);
+   ctx->suspended = true;
+   }
+
+   return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int fimc_suspend(struct device *dev)
 {
@@ -1806,7 +1807,6 @@ static int fimc_resume(struct device *dev)
 }
 #endif
 
-#ifdef CONFIG_PM
 static int fimc_runtime_suspend(struct device *dev)
 {
struct fimc_context *ctx = get_fimc_context(dev);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] drm/exynos: rotator: Clock control is unused if !PM

[Thierry Reding]

From: Thierry Reding <tred...@nvidia.com>

Protect the rotator_clk_crtl() function with an #ifdef CONFIG_PM guard
to avoid "defined but not used" warnings.

Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c 
b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 425e70625388..2f5c118f4c8e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -786,6 +786,7 @@ static int rotator_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM
 static int rotator_clk_crtl(struct rot_context *rot, bool enable)
 {
if (enable) {
@@ -822,7 +823,6 @@ static int rotator_resume(struct device *dev)
 }
 #endif
 
-#ifdef CONFIG_PM
 static int rotator_runtime_suspend(struct device *dev)
 {
struct rot_context *rot = dev_get_drvdata(dev);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver

[Thierry Reding]

On Mon, Sep 21, 2015 at 06:27:40PM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> Thanks for your suggest :)
> 
> On 09/21/2015 05:15 PM, Thierry Reding wrote:
> >On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote:
> >>Hi Heiko,
> >>
> >>On 09/02/2015 10:15 AM, Yakir Yang wrote:
> >>>Hi Heiko,
> >>>
> >>>在 09/02/2015 05:47 AM, Heiko Stuebner 写道:
> >>>>Hi Yakir,
> >>>>
> >>>>Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang:
> >>>>>The Samsung Exynos eDP controller and Rockchip RK3288 eDP
> >>>>>controller
> >>>>>share the same IP, so a lot of parts can be re-used. I split the common
> >>>>>code into bridge directory, then rk3288 and exynos only need to keep
> >>>>>some platform code. Cause I can't find the exact IP name of exynos dp
> >>>>>controller, so I decide to name dp core driver with "analogix" which I
> >>>>>find in rk3288 eDP TRM ;)
> >>>>>
> >>>>>Beyond that, there are three light registers setting differents bewteen
> >>>>>exynos and rk3288.
> >>>>>1. RK3288 have five special pll resigters which not indicata in exynos
> >>>>>dp controller.
> >>>>>2. The address of DP_PHY_PD(dp phy power manager register) are
> >>>>>different
> >>>>>between rk3288 and exynos.
> >>>>>3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp
> >>>>>debug
> >>>>>register).
> >>>>>
> >>>>>I have verified this series on two kinds of rockchip platform board,
> >>>>>one
> >>>>>is rk3288 sdk board which connect with a 2K display port monitor, the
> >>>>>other
> >>>>>is google jerry chromebook which connect with a eDP screen
> >>>>>"cnm,n116bgeea2",
> >>>>>both of them works rightlly.
> >>>>it looks like during the rebase something did go wrong and I found some
> >>>>issues
> >>>>I mentioned in the replies to individual patches.
> >>>>
> >>>>I did prepare a branch based on mainline [0] with both the old and the
> >>>>new edp
> >>>>driver - rk3288_veyron_defconfig build both drivers into the image.
> >>>>
> >>>>While the old driver still works, I wasn't able to make the new one work
> >>>>yet
> >>>>... the drm core does find the connector, but not that anything is
> >>>>connected
> >>>>to it. I'll try to dig deeper tomorrow, but maybe you'll see anything
> >>>>interesting before then.
> >>>Many thanks for your comment and debug, I would rebase on your
> >>>"edp-with-veyron" branch and fix the broken, make sure v6 would
> >>>work rightly at least in your side and my side.
> >>Just like we talk off line, I guess there are two tricky questions which
> >>make analogix_dp just crash/failed on rockchip platform:
> >>
> >>-  One is how to reach a agreement with the common way to register
> >>connector. There would be a conflict with Exynos & IMX & Rockchip.
> >>  On analogix_dp thread, Exynos want to register connector when that
> >>connector is ready.
> >>  On dw_hdmi thread, IMX want to register connector when all component 
> >> is
> >>already.
> >>  So Exynos & IMX & Rockchip should reach a common way to register
> >>connector to fix this issue.
> >>
> >>-  The other is atomic API.
> >>   The rockchip drm haven't implemented the atomic API, but the original
> >>exynos_dp have used the atomic API on connector helper function. That's why
> >>analogix_dp just keep crash on your side.
> >There's really no reason not to convert Rockchip to atomic. It will have
> >to happen eventually anyway.
> 
> Do agree on this point, and I see Tomasz Figa have done some WIP
> works on implementing the atomic_commit, maybe would upstream
> in further.(https://chromium-review.googlesource.com/#/c/284560/1)
> 
> 
> >That said, there's another option that would allow you to side-step both
> >of the above problems at the same time. If you turn the common code into
> >a helper library that should give you enough flexibility to integrate it
> >into all existing users. For example you could leave out

Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver

[Thierry Reding]

On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote:
> Hi Heiko,
> 
> On 09/02/2015 10:15 AM, Yakir Yang wrote:
> >Hi Heiko,
> >
> >在 09/02/2015 05:47 AM, Heiko Stuebner 写道:
> >>Hi Yakir,
> >>
> >>Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang:
> >>>The Samsung Exynos eDP controller and Rockchip RK3288 eDP
> >>>controller
> >>>share the same IP, so a lot of parts can be re-used. I split the common
> >>>code into bridge directory, then rk3288 and exynos only need to keep
> >>>some platform code. Cause I can't find the exact IP name of exynos dp
> >>>controller, so I decide to name dp core driver with "analogix" which I
> >>>find in rk3288 eDP TRM ;)
> >>>
> >>>Beyond that, there are three light registers setting differents bewteen
> >>>exynos and rk3288.
> >>>1. RK3288 have five special pll resigters which not indicata in exynos
> >>>dp controller.
> >>>2. The address of DP_PHY_PD(dp phy power manager register) are
> >>>different
> >>>between rk3288 and exynos.
> >>>3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp
> >>>debug
> >>>register).
> >>>
> >>>I have verified this series on two kinds of rockchip platform board,
> >>>one
> >>>is rk3288 sdk board which connect with a 2K display port monitor, the
> >>>other
> >>>is google jerry chromebook which connect with a eDP screen
> >>>"cnm,n116bgeea2",
> >>>both of them works rightlly.
> >>it looks like during the rebase something did go wrong and I found some
> >>issues
> >>I mentioned in the replies to individual patches.
> >>
> >>I did prepare a branch based on mainline [0] with both the old and the
> >>new edp
> >>driver - rk3288_veyron_defconfig build both drivers into the image.
> >>
> >>While the old driver still works, I wasn't able to make the new one work
> >>yet
> >>... the drm core does find the connector, but not that anything is
> >>connected
> >>to it. I'll try to dig deeper tomorrow, but maybe you'll see anything
> >>interesting before then.
> >
> >Many thanks for your comment and debug, I would rebase on your
> >"edp-with-veyron" branch and fix the broken, make sure v6 would
> >work rightly at least in your side and my side.
> 
> Just like we talk off line, I guess there are two tricky questions which
> make analogix_dp just crash/failed on rockchip platform:
> 
> -  One is how to reach a agreement with the common way to register
> connector. There would be a conflict with Exynos & IMX & Rockchip.
>  On analogix_dp thread, Exynos want to register connector when that
> connector is ready.
>  On dw_hdmi thread, IMX want to register connector when all component is
> already.
>  So Exynos & IMX & Rockchip should reach a common way to register
> connector to fix this issue.
> 
> -  The other is atomic API.
>   The rockchip drm haven't implemented the atomic API, but the original
> exynos_dp have used the atomic API on connector helper function. That's why
> analogix_dp just keep crash on your side.

There's really no reason not to convert Rockchip to atomic. It will have
to happen eventually anyway.

That said, there's another option that would allow you to side-step both
of the above problems at the same time. If you turn the common code into
a helper library that should give you enough flexibility to integrate it
into all existing users. For example you could leave out the connector
registration and let the drivers do that. Similarly since the helpers
are only hooked up at registration time you could probably find a way to
share the low-level code but again leave it up to the drivers to glue it
all together at registration time (drivers could wrap the low-level code
with atomic or non-atomic callbacks).

This option may also have the benefit of loosening the coupling between
DRM drivers and the helper code for this IP, which may be handy in case
the drivers diverge again in the future, or ease transitions to new API.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Thierry Reding]

On Sun, Sep 06, 2015 at 11:59:08AM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> 在 09/03/2015 05:04 PM, Thierry Reding 写道:
> >On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote:
> >>Hi Rob,
> >>
> >>在 09/03/2015 04:17 AM, Rob Herring 写道:
> >>>On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <y...@rock-chips.com> wrote:
> >>>>Some edp screen do not have hpd signal, so we can't just return
> >>>>failed when hpd plug in detect failed.
> >>>This is a property of the panel (or connector perhaps), so this
> >>>property should be located there. At least, it is a common issue and
> >>>not specific to this chip. We could have an HDMI connector and failed
> >>>to hook up HPD for example. A connector node is also where hpd-gpios
> >>>should be located instead (and are already defined by
> >>>../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> >>>binding, too.
> >>Yep, I agree with your front point, it is a property of panel, not specific
> >>to eDP controller, so this code should handle in connector logic.
> > From your description it sounds more like this is in fact a property of
> >the panel. Or maybe I should say "quirk". If the panel doesn't generate
> >the HPD signal, then that should be a property of the panel, not the
> >connector. The eDP specification mandates that connectors have a HPD
> >signal, though it allows the "HPD conductor in the connector cable" to
> >be omitted if not used by the source. I'd consider the cable to belong
> >to the panel rather than the connector, so absence of HPD, either
> >because the cable doesn't have the conductor or because the panel does
> >not generate the signal, should be a quirk of the panel.
> >
> >That said you could have a panel that supports HPD connected via a cable
> >that doesn't transmit it, so this would be a per-board variant and hence
> >should be a device tree property rather than hard-coded in some panel
> >driver.
> >
> >Conversely, if the panel isn't capable of generating an HPD signal, then
> >I don't think it would be appropriate to make it a DT property. It would
> >be better to hard-code it in the driver, lest someone forget to set the
> >property in DT and get stuck with a device that isn't operational.
> 
> Oh, you're right, if it's a cable quirk, then DT property would be okay, if
> it
> is a problem of panel, then maybe hard-code in driver would be better.
> 
> After look up for the document of panel "innolux,n116bge", I haven't see
> any description of hot plug signal, and even not found in PIN ASSIGNMENT.
> So I believe it's a panel problem, that's to say it should handle in panel
> driver.

The datasheet that I have for that panel lists HPD as pin 17. Also I
used to have a setup with that panel and I distinctly remember hotplug
working just fine. Perhaps this is an issue with a specific variant of
the panel? Or perhaps this is indeed a problem with the cable that's
connecting the panel to the board. It could be one of those cases where
they left out the HPD conductor to save money.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Thierry Reding]

On Fri, Sep 04, 2015 at 11:20:03AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 03, 2015 at 11:04:40AM +0200, Thierry Reding wrote:
> > Conversely, if the panel isn't capable of generating an HPD signal, then
> > I don't think it would be appropriate to make it a DT property. It would
> > be better to hard-code it in the driver, lest someone forget to set the
> > property in DT and get stuck with a device that isn't operational.
> 
> There is another way to deal with this: DRM supports the idea of connector
> forcing - where you can force the connector to think that it's connected
> or disconnected.

Yes, this could work well for RGB/LVDS or DSI connectors perhaps. For
eDP there is added complexity because the HPD interrupt function is also
used to signal loss of link integrity. That is, after receiving an HPD
interrupt you are supposed to retrain the link (or at least check the
link status to see if the interrupt cause is loss of integrity). While
the eDP specification makes HPD optional, it also says that if HPD isn't
available, then the source must use polling to monitor link integrity
instead.

DRM does provide a mechanism for that as well. You can set the
connector's ->polled field to DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT and have the core actively poll for the
connector status (i.e. call ->detect() every 100 ms).

I think use of polling would be more appropriate in case of eDP.

> One of the problems is that not many ARM DRM drivers implement it - maybe
> it should be a requirement for code to be accepted? :)

I suspect that many drivers may roll their own. In fact I'm guilty of
that myself. On Tegra we have a default implementation for outputs which
will default to the state of an HPD GPIO where available and fall back
to "always connected" for outputs that have a panel connected. Outputs
that have a separate mechanism to signal hotplug detection (such as DP)
simply use a custom ->detect() implementation.

The overhead of rolling one's own is almost zero and connector forcing
has the disadvantage of being available via sysfs and debugfs, so the
default set by drivers could be overwritten by users at runtime with no
easy way back.

Given the above I'm not sure enforcing connector forcing would be
beneficial.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir

[Thierry Reding]

On Fri, Sep 04, 2015 at 11:29:30PM +0200, Heiko Stuebner wrote:
> Am Freitag, 4. September 2015, 16:06:02 schrieb Rob Herring:
> > On Tue, Sep 1, 2015 at 3:46 PM, Heiko Stuebner  wrote:
> > > Am Dienstag, 1. September 2015, 13:49:58 schrieb Yakir Yang:
> > >> Split the dp core driver from exynos directory to bridge
> > >> directory, and rename the core driver to analogix_dp_*,
> > >> leave the platform code to analogix_dp-exynos.
> > >> 
> > >> Signed-off-by: Yakir Yang 
> > > 
> > > [...]
> > > 
> > >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c similarity index 50%
> > >> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >> index bed0252..7d62f22 100644
> > >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > 
> > > [...]
> > > 
> > >>   connector->polled = DRM_CONNECTOR_POLL_HPD;
> > >>   
> > >>   ret = drm_connector_init(dp->drm_dev, connector,
> > >> 
> > >> -  _dp_connector_funcs,
> > >> +  _dp_connector_funcs,
> > >> 
> > >>DRM_MODE_CONNECTOR_eDP);
> > >>   
> > >>   if (ret) {
> > >>   
> > >>   DRM_ERROR("Failed to initialize connector with drm\n");
> > >>   return ret;
> > >>   
> > >>   }
> > >> 
> > >> - drm_connector_helper_add(connector,
> > >> _dp_connector_helper_funcs); +
> > >> drm_connector_helper_add(connector,
> > >> +  _dp_connector_helper_funcs);
> > >> 
> > >>   drm_connector_register(connector);
> > > 
> > > this should only run on exynos, as we're doing all our connector
> > > registration in the core driver after all components are bound, so I
> > > guess something like> 
> > > the following is needed:
> > >if (dp->plat_data && dp->plat_data->dev_type == EXYNOS_DP)
> > >
> > >drm_connector_register(connector);
> > 
> > Yuck!
> > 
> > Surely there is a better way. From what I've seen of DRM, I have no
> > doubt this is needed, but really a better solution is needed. Surely
> > there can be a more generic way for the driver to determine if it
> > should handle the connector or not. This seems like a common problem
> > including one I have seen. What I'm working on has onchip DSI encoder
> > -> ADV7533 -> HDMI. The DSI encoder can also have a direct attached
> > panel. So I have to check for a bridge in the encoder driver and only
> > register the connector for the panel if a bridge is not attached.
> 
> I'm also only a part-time drm meddler, so things may be inaccurate.
> 
> This conditional is not meant to prevent the registration of the dp 
> connector, 
> only delay it for non-exynos drms. The connector registration does publish it 
> to userspace, so like i.MX we're doing that for all connectors at the same 
> time after all components are bound - to also make x11 happy. Exynos on the 
> other hand seems to register its connectors individually and I'm not sure if 
> they would be willing to change that.
> 
> see d3007dabeff4 ("drm/rockchip: register all connectors after bind") or 
> simply rockchip_drm_drv.c line 178.
> 
> and e355e7dd607b ("imx-drm: delay publishing sysfs connector entries")

There really shouldn't be a reason for both drivers to behave
differently in this case. Gustavo has done a lot of work on cleaning up
the Exynos driver lately, so perhaps you should sync up with him to see
if this is something that he can integrate with his changes.

If you can't reach an agreement and Exynos must keep the exact behaviour
as it has now, then perhaps a better option would be to have the
Analogix core driver not register the connector but rather leave that up
to users of the helpers. Then you don't have to clutter the core driver
with this type of platform-specific data.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Thierry Reding]

On Thu, Sep 03, 2015 at 04:55:59PM -0500, Rob Herring wrote:
> On Thu, Sep 3, 2015 at 3:47 AM, Thierry Reding <tred...@nvidia.com> wrote:
> > On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote:
> > [...]
> >> Are there any eDP panels which don't have EDID and need panel details in 
> >> DT?
> >
> > Most panels need information other than EDID. They typically have some
> > requirements regarding the power up sequence that aren't to be found
> > anywhere in EDID or detectable by some other mechanism. A decision was
> > therefore made a long time ago to require panels to be listed in DT with
> > a specific compatible string. That way all of these details can be
> > stashed away in drivers that know how to deal with these kinds of
> > details.
> 
> I guess I was being hopeful that eDP was improving that situation.

Unfortunately not. eDP helps with a number of things (DPCD and link
training take care of a lot of the issues), but it doesn't provide a
solution for everything.

To my knowledge in most cases the power up sequence isn't strictly
necessary to get the panel to operate. But there have been instances
where this is absolutely required if you want to avoid visual artifacts
as you set a mode. A lot of panels that I've come across require
receiving a couple of frames before they guarantee that something will
actually be displayed on the screen. So if your code is too quickly
enabling backlight, and your backlight doesn't happen to have just the
right amount of ramp-up time you might end up seeing garbage on the
screen for a couple of frames.

It would be great if somebody came up with, say, an EDID extension to
describe these requirements, but I'm not sure if even that would give
us panels that could be driven with a generic driver. Often the power
supplies or reset/enable signals are very different across panels. So
describing it all generically would become messy rather quickly.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting

[Thierry Reding]

On Wed, Sep 02, 2015 at 06:02:25PM +0800, Yakir Yang wrote:
> 在 2015/9/2 16:34, Thierry Reding 写道:
[...]
> >At the very least your code must compile when applied against a recent
> >upstream tree. I would also expect you to make sure the code works at
> >runtime, though, contrary to build testing, not everybody will be able
> >to verify that you've actually done so. It is ultimately your platform
> >maintainer's (i.e. Heiko's) responsibility to ensure that because they
> >will get to deal with user complaints if people can't run an upstream
> >kernel on the devices.
> 
> Oh, first time to know this rule. So I should work on Heiko's github
> kernel branch at the first time to start send upstream.

It's usually not necessary to rebase on a specific platform tree. Most
platform trees should feed into linux-next anyway, so linux-next would
be the appropriate base in almost all cases.

Note, though, that that's only true if you expect somebody else to merge
your code. The reason is that whoever will end up applying your patches
will likely apply to a tree that feeds into linux-next, and that way you
both end up having roughly the same base.

On the other hand if you are a maintainer yourself you should be keeping
a branch based on the latest -rc1. That's especially important if your
tree feeds into linux-next, because basing on linux-next will break very
horribly that way.

So for this particular case I would expect either Mark or Inki to apply
these patches when they're ready. Their trees should be based on the
latest -rc1. At least the Exynos DRM tree feeds into linux-next, so you
should be fine if you use linux-next as a base.

Mark, have you ever considered having your tree added to linux-next?

I'm beginning to think that we need to make that a requirement for all
DRM drivers so that we can resolve integration issues early on rather
than Dave having to deal with them when he pulls code in.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Thierry Reding]

On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote:
[...]
> Are there any eDP panels which don't have EDID and need panel details in DT?

Most panels need information other than EDID. They typically have some
requirements regarding the power up sequence that aren't to be found
anywhere in EDID or detectable by some other mechanism. A decision was
therefore made a long time ago to require panels to be listed in DT with
a specific compatible string. That way all of these details can be
stashed away in drivers that know how to deal with these kinds of
details.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Thierry Reding]

On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote:
> Hi Rob,
> 
> 在 09/03/2015 04:17 AM, Rob Herring 写道:
> >On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang  wrote:
> >>Some edp screen do not have hpd signal, so we can't just return
> >>failed when hpd plug in detect failed.
> >This is a property of the panel (or connector perhaps), so this
> >property should be located there. At least, it is a common issue and
> >not specific to this chip. We could have an HDMI connector and failed
> >to hook up HPD for example. A connector node is also where hpd-gpios
> >should be located instead (and are already defined by
> >../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> >binding, too.
> 
> Yep, I agree with your front point, it is a property of panel, not specific
> to eDP controller, so this code should handle in connector logic.

From your description it sounds more like this is in fact a property of
the panel. Or maybe I should say "quirk". If the panel doesn't generate
the HPD signal, then that should be a property of the panel, not the
connector. The eDP specification mandates that connectors have a HPD
signal, though it allows the "HPD conductor in the connector cable" to
be omitted if not used by the source. I'd consider the cable to belong
to the panel rather than the connector, so absence of HPD, either
because the cable doesn't have the conductor or because the panel does
not generate the signal, should be a quirk of the panel.

That said you could have a panel that supports HPD connected via a cable
that doesn't transmit it, so this would be a per-board variant and hence
should be a device tree property rather than hard-coded in some panel
driver.

Conversely, if the panel isn't capable of generating an HPD signal, then
I don't think it would be appropriate to make it a DT property. It would
be better to hard-code it in the driver, lest someone forget to set the
property in DT and get stuck with a device that isn't operational.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting

[Thierry Reding]

On Wed, Sep 02, 2015 at 10:06:36AM +0800, Yakir Yang wrote:
> 在 09/02/2015 05:00 AM, Heiko Stuebner 写道:
> >Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
[...]
> >>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
> >>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>@@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
> >>{
> >>
> >>  int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> >>  int connector_type,
> >>- int out_mode)
> >>+ int bpc, int color)
> >>  {
> >>struct vop *vop = to_vop(crtc);
> >>-
> >>vop->connector_type = connector_type;
> >>vop->connector_out_mode = out_mode;
> >this line should probably go away, as the source var "out_mode" does not 
> >exist
> >in the function params any more, making the compilation break, and is set
> >below anyway.
> 
> Sorry for the failed, there are must be a problem when I backport those code
> to chrome-3.14 to verify.
> 
> Thanks a lot, I would update next version soon.

*sigh*

I get the feeling that you're going about upstreaming the wrong way. If
you post patches to upstream mailing lists and you expect people to
apply those patches, then your target is the upstream kernel. So you
should be basing all of your work on top of a recent release candidate
directly from Linus or some recent version of linux-next.

Your current approach is already making people waste time trying to
build the patches and fail because you've been testing on something
other than mainline or linux-next.

At the very least your code must compile when applied against a recent
upstream tree. I would also expect you to make sure the code works at
runtime, though, contrary to build testing, not everybody will be able
to verify that you've actually done so. It is ultimately your platform
maintainer's (i.e. Heiko's) responsibility to ensure that because they
will get to deal with user complaints if people can't run an upstream
kernel on the devices.

I realize that the upstream kernel isn't what's going to end up in
products later on, but that doesn't change the fact that you're
submitting code for inclusion in the mainline tree. If you need to
backport code to some ChromeOS tree, then that should be done after
you've verified that things build and run upstream. Doing so makes life
a lot easier for your upstream maintainers, and that in turn makes it
more likely to get your code merged.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Tue, Aug 25, 2015 at 10:03:52PM +0800, Yakir Yang wrote:
 Hi Thierry,
 
 在 2015/8/25 17:58, Thierry Reding 写道:
 On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
 [...]
 +   -analogix,color-space:
 +   input video data format.
 +   COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
 I don't think DT is an appropriate place to set this. To my knowledge
 this depends on the display and/or mode, so I don't think hard-coding
 it here is the right thing to do.
 
 Yeah, same question with my previous reply ;)

I don't have an answer for you, unfortunately. But like I said,
hard-coding isn't going to work. What if, for example, you set this to a
fixed value and then you connect a monitor that doesn't support the
specific one you set?

You cited code from dw_hdmi.c earlier, it looks like it might be correct
even though it doesn't cite a reference for why this was done. Perhaps
someone on this thread, or someone involved with dw_hdmi can answer
where that code came from.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote:
 Hi Thierry  Rob,
 
 在 2015/8/25 21:27, Rob Herring 写道:
 On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding tred...@nvidia.com wrote:
 On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
 On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang y...@rock-chips.com wrote:
 [...]
 +   -analogix,link-rate:
 +   max link rate supported by the eDP controller.
 +   LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 
 0x0A,
 +   LINK_RATE_5_40GBPS = 0x14
 Same here. I'd rather see something like link-rate-mbps and use the
 actual rate.
 There is no need whatsoever to hard-code this in DT. (e)DP provides the
 means to detect what rate the link supports and the specification
 provides guidance on how to select an appropriate one.
 Good, even better.
 
 I do think we still need keep this DT prop yet.
 
 I think drm_dp_help.c could get the panel max link-rate and lane-count,
 but it's not enough, we still need knew the eDP controller max link-rate
 and lane-count.
 
 Let me show the exact example that happened in my side. When I connect
 my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
 from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller
 link-rate
 to 5.4Gbps, the DP TV just broken, do not light up normally.
 
 This reason why TV broken is the max link-rate which support by RK3288 eDP
 controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:
 
 *Compliant with DisplayPortTM Specification, Version 1.2.
 Compliant with eDPTM Specification, Version 1.3.
 HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
 Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
 *
 **
 
 
 Beside I haven't found there are some registers would indicate the eDP
 controller
 max link-rate and lane-count, so this is why I still instance that we need
 this DT
 prop to indicata Max rate controller support.
 
 So, I wish you could agree with me on this point.

Your driver should know what link rates it supports and restrict itself
to use those. This is implied by the compatible string and doesn't need
to be duplicated into device tree.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
 On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
  On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang y...@rock-chips.com wrote:
   +   -analogix,color-depth:
   +   number of bits per colour component.
   +   COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 
   = 3
 
  This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
  drop the vendor prefix.
 
  Please think about this some more.  What does color-depth mean?  Does it
  mean the number of bits per colour _component_, or does it mean the total
  number of bits to represent a particular colour.  It's confusing as it
  stands.
 
 Then component-color-bpp perhaps?

There should be no need to have this in DT at all. The BPC is a property
of the attached panel and it should come from the panel (either the
panel driver or parsed from EDID if available).

  When we adopted the graph bindings for iMX DRM, I thought exactly at that
  time it would be nice if this could become the standard for binding DRM
  components together but I don't have the authority from either the DT
  perspective or the DRM perspective to mandate that.  Neither does anyone
  else.  That's the _real_ problem here.
 
  I've seen several DRM bindings go by which don't use the of-graph stuff,
  which means that they'll never be compatible with generic components
  which do use the of-graph stuff.
 
 It goes beyond bindings IMO. The use of the component framework or not
 has been at the whim of driver writers as well. It is either used or
 private APIs are created. I'm using components and my need for it
 boils down to passing the struct drm_device pointer to the encoder.
 Other components like panels and bridges have different ways to attach
 to the DRM driver.

I certainly support unification, but it needs to be reasonable. There
are cases where a different structure for the binding work better than
another and I think this always needs to be evaluated on a case by case
basis.

Because of that I think it makes sense to make all these framework bits
opt-in, otherwise we could easily end up in a situation where drivers
have to be rearchitected (or even DT bindings altered!) in order to be
able to reuse code.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
[...]
 + -analogix,color-space:
 + input video data format.
 + COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2

I don't think DT is an appropriate place to set this. To my knowledge
this depends on the display and/or mode, so I don't think hard-coding
it here is the right thing to do.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
 On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
  On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
   It goes beyond bindings IMO. The use of the component framework or not
   has been at the whim of driver writers as well. It is either used or
   private APIs are created. I'm using components and my need for it
   boils down to passing the struct drm_device pointer to the encoder.
   Other components like panels and bridges have different ways to attach
   to the DRM driver.
  
  I certainly support unification, but it needs to be reasonable. There
  are cases where a different structure for the binding work better than
  another and I think this always needs to be evaluated on a case by case
  basis.
 
 It can't be a case-by-case basis.
 
 The TDA998x encoder/connector is going to be component only.  This is
 a generic chip, which can be attached to the output of any parallel
 RGB+sync+clock bus.  In other words, it could appear anywhere.
 
 Are you really saying that we need to support multiple schemes of
 attaching the driver to DRM?  That's totally insane IMHO.

No, what I'm saying is that we should have a single scheme, but one
that doesn't put any restrictions on what kind of DT binding you use or
how your driver is architected.

 The problem with the drm_encoder_slave stuff is that you can't sanely
 attach of-nodes to the drm-created i2c device.  Yes, you can parse
 them from the DT file as a sub-node of the upper device, but that
 then goes against the principle of the I2C bindings, which is to
 list the I2C devices as a child below the I2C adapter node.  If you
 try and put the DT node there, then the OF code will create the I2C
 device for you, and the drm_encoder_slave stuff won't have the
 control it needs to communicate through the wrapped i2c_driver
 stuff.
 
 So, tda998x is going component-only, as that's the _only_ sane solution
 for it.

Has anyone ever considered turning it into a DRM bridge driver? I had
always envisioned component/master to be primarily useful to glue
together various SoC components to form one componentized device. Now
if tda998x is an I2C slave it is external to the SoC (auxiliary), so
in my opinion much better off as a bridge driver.

Bridge drivers don't come with any of the disadvantages that the
drm_encoder_slave stuff has. They are regular drivers that are probed
via their parent busses (I2C, platform, SPI, ...) and hook into DRM via
an abstract interface. The DT aspect is taken care of automatically
because they get instantiated by their parent bus like any other device.

 Now, what happens when some other DRM driver wants to use the tda998x
 driver, and its bindings are not compatible with the component helpers?
 They're pretty much stuck up the creek without a paddle.

I'm sure that will be very helpful response for whoever's going to end
up having to deal with that situation.

 Case by case doesn't work unless you're talking about truely isolated
 hardware where no one shares anything.

There are two different things here. The inter-driver interface, which,
in my opinion, it makes a lot of sense to standardize. Like I mentioned
above I think it unwise to make this interface depend upon a framework
or the firmware description such as DT in order to avoid unnecessary
restrictions. The second, orthogonal, issue, is the DT bindings. Those
I think should absolutely be designed case by case and select whatever
most accurately describes the hardware.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote:
 Hi Thierry,
 
 在 2015/8/25 17:12, Thierry Reding 写道:
 On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
 On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
 On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
 On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang y...@rock-chips.com wrote:
 +   -analogix,color-depth:
 +   number of bits per colour component.
 +   COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 
 = 3
 This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
 drop the vendor prefix.
 Please think about this some more.  What does color-depth mean?  Does it
 mean the number of bits per colour _component_, or does it mean the total
 number of bits to represent a particular colour.  It's confusing as it
 stands.
 Then component-color-bpp perhaps?
 There should be no need to have this in DT at all. The BPC is a property
 of the attached panel and it should come from the panel (either the
 panel driver or parsed from EDID if available).
 
 Actually I have send an email about this one to you in version 2, just past
 from that email:
 
 samsung,color_space and samsung,color-depth
 
 The drm_display_info's color_formats and bpc indicate the monitor display
 ability, but
 the edp driver could not take it as input video format directly.
 
 For example, with my DP TV I would found RGB444  YCRCB422   YCRCB444
 support in drm_display_info.color_formats and 16bit bpc support, but RK3288
 crtc
 driver could only output RGB  ITU formats, so finally analogix_dp-rockchip
 driver
 config crtc to RGBaaa 10bpc mode.
 
 In this sutiation, the analogix_dp core driver would pazzled by the
 drm_display_info,
 can't chose the right color_space and bpc.
 
 And this is the place that confused me, wish you could give some ideas about
 this one :-)

Your display driver should choose whatever it is capable of outputting.
If the display reports that it can do 16 bits-per-color, but your
display driver can't do it, then it should choose a configuration that
it supports. Similarily for the color encodings. If you can't generate
YCrCb444 with your hardware, then it's the driver's job to know about
that and select the next appropriate configuration.

But hard-coding this is not the right solution because the value in DT
may end up conflicting with what the display reports.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Thierry Reding]

On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
 On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang y...@rock-chips.com wrote:
[...]
  +   -analogix,link-rate:
  +   max link rate supported by the eDP controller.
  +   LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
  +   LINK_RATE_5_40GBPS = 0x14
 
 Same here. I'd rather see something like link-rate-mbps and use the
 actual rate.

There is no need whatsoever to hard-code this in DT. (e)DP provides the
means to detect what rate the link supports and the specification
provides guidance on how to select an appropriate one.

 
  +   -analogix,lane-count:
  +   max number of lanes supported by the eDP contoller.
  +   LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
 
 And drop the vendor prefix here.

Same as for the link rate.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver

[Thierry Reding]

On Fri, Aug 21, 2015 at 08:24:16PM +0900, Jingoo Han wrote:
 On 2015. 8. 21., at PM 7:01, Yakir Yang y...@rock-chips.com wrote:
  
  Hi Jingoo,
  
  在 2015/8/21 16:20, Jingoo Han 写道:
  On 2015. 8. 19., at PM 11:48, Yakir Yang y...@rock-chips.com wrote:
  
  .
  
  .../bindings/video/analogix_dp-rockchip.txt|   83 ++
  .../devicetree/bindings/video/exynos_dp.txt|   51 +-
  arch/arm/boot/dts/exynos5250-arndale.dts   |   10 +-
  arch/arm/boot/dts/exynos5250-smdk5250.dts  |   10 +-
  arch/arm/boot/dts/exynos5250-snow.dts  |   12 +-
  arch/arm/boot/dts/exynos5250-spring.dts|   12 +-
  arch/arm/boot/dts/exynos5420-peach-pit.dts |   12 +-
  arch/arm/boot/dts/exynos5420-smdk5420.dts  |   10 +-
  arch/arm/boot/dts/exynos5800-peach-pi.dts  |   12 +-
  drivers/gpu/drm/bridge/Kconfig |5 +
  drivers/gpu/drm/bridge/Makefile|1 +
  drivers/gpu/drm/bridge/analogix_dp_core.c  | 1382 
  +++
  drivers/gpu/drm/bridge/analogix_dp_core.h  |  286 
  drivers/gpu/drm/bridge/analogix_dp_reg.c   | 1294 
  ++
  .../exynos_dp_reg.h = bridge/analogix_dp_reg.h}   |  270 ++--
  drivers/gpu/drm/exynos/Kconfig |5 +-
  drivers/gpu/drm/exynos/Makefile|2 +-
  drivers/gpu/drm/exynos/analogix_dp-exynos.c|  347 +
  Would you change this file name to exynos_dp.c?
  
  Sorry, I don't think so  ;(
  
  I think IP_name+Soc_name would be better in this re-use case.
 
 So? Is there the naming rule such as IP_name+SoC_name?
 
  Beside I see
  there are lots of driver named with this format in kernel, such as dw_hdmi 
   dw_mmc
 
 Please look at other dw cases.
 For example, look at dw_pcie.
 
 drivers/pci/host/
 pcie-designware.c
 pci-spear13xx.c
 pci-exynos.c
 
 In this case, pci-spear13xx.c and pci-exynos.c do not use IP_name+SoC_name, 
 even though these are dw IPs.
 
 Also, naming consistency is more important.
 Now, Exynos DRM files are using exynos_drm_ prefix.
 
 drivers/gpu/drm/exynos/
 exynos_drm_buf.c
 exynos_drm_core.c
 
 
 However, analogix_dp-exynos.c looks very inconsistent.
 
 If there is no strict naming rule, please use exynos_dp.c
 or exynos_drm_dp.c. 

Exynos DRM maintainers get to pick their filenames, so Yakir, please
rename as Jingoo suggested.

Even if you didn't the first thing that would go into the Exynos DRM
driver tree after this is merged is a rename patch anyway.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH V3] Watchdog: Fix parent of watchdog_devices

[Thierry Reding]

On Wed, Aug 19, 2015 at 08:58:24AM +0530, Pratyush Anand wrote:
[...]
 diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
 index 30451ea46902..7f97cdd53f29 100644
 --- a/drivers/watchdog/tegra_wdt.c
 +++ b/drivers/watchdog/tegra_wdt.c
 @@ -218,6 +218,7 @@ static int tegra_wdt_probe(struct platform_device *pdev)
   wdd-ops = tegra_wdt_ops;
   wdd-min_timeout = MIN_WDT_TIMEOUT;
   wdd-max_timeout = MAX_WDT_TIMEOUT;
 + wdd-parent = pdev-dev;
  
   watchdog_set_drvdata(wdd, wdt);
  

Acked-by: Thierry Reding tred...@nvidia.com


signature.asc
Description: PGP signature

Re: [PATCH 2/2] clk: Convert __clk_get_name(hw-clk) to clk_hw_get_name(hw)

[Thierry Reding]

On Wed, Aug 12, 2015 at 04:12:41PM -0700, Stephen Boyd wrote:
 Use the provider based method to get a clock's name so that we
 can get rid of the clk member in struct clk_hw one day. Mostly
 converted with the following coccinelle script.
 
 @@
 struct clk_hw *E;
 @@
 
 -__clk_get_name(E-clk)
 +clk_hw_get_name(E)
 
[...]
  drivers/clk/tegra/clk-pll.c  |  8 

Acked-by: Thierry Reding tred...@nvidia.com


signature.asc
Description: PGP signature

Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

[Thierry Reding]

On Sat, Aug 08, 2015 at 11:54:38AM +0800, Yakir Yang wrote:
[...]
 edp: edp@ff97 {
[...]
 hsync-active-high = 0;
 vsync-active-high = 0;
 interlaced = 0;

These look like they should come from the display mode definition (EDID)
rather than device tree.

 samsung,color-space = 0;
 samsung,dynamic-range = 0;
 samsung,ycbcr-coeff = 0;

I think these should also come from EDID, though I'm not sure if we
store this in internal data structures yet.

 samsung,color-depth = 1;

This is probably drm_display_info.bpc.

 samsung,link-rate = 0x0a;
 samsung,lane-count = 1;

And these should really be derived from values in the DPCD and adjusted
(if necessary) during link training.

Why would you ever want to hard-code the above?

 +   dp-clk_24m = devm_clk_get(dev, clk_dp_24m);
 Same here, maybe dp_24m.
 Like my previous reply. And actually as those two clocks all have
 a common prefix SCLK in rk3288 clock tree, I thinkt we can name
 them to sclk_dp  sclk_dp_24m, is it okay ?

I don't think there's a need for these common prefixes. The names here
are identifiers in the context of the IP block, so any SoC-specific
prefixes are irrelevant. Also they do appear, in DT and in code, in the
context of clocks already, so sclk_ or clk_ is completely redundant
in these names.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

[Thierry Reding]

On Mon, Aug 10, 2015 at 08:59:44PM +0800, Yakir Yang wrote:
 Hi Thierry,
 
 在 2015/8/10 18:00, Thierry Reding 写道:
 On Sat, Aug 08, 2015 at 11:54:38AM +0800, Yakir Yang wrote:
 [...]
  edp: edp@ff97 {
 [...]
  hsync-active-high = 0;
  vsync-active-high = 0;
  interlaced = 0;
 These look like they should come from the display mode definition (EDID)
 rather than device tree.
 
 I do think so, those numbers can parse from struct drm_mode. But I haven't
 send those changes yet, cause I want to merge the split analogix_dp first,
 and
 then send some patches to improve it. If you think it's better to imptoved
 those
 now, I would like to do it , please let me know ;)
 
  samsung,color-space = 0;
  samsung,dynamic-range = 0;
  samsung,ycbcr-coeff = 0;
 I think these should also come from EDID, though I'm not sure if we
 store this in internal data structures yet.
 
 Same to previous reply
 
  samsung,color-depth = 1;
 This is probably drm_display_info.bpc.
 
 Same to previous reply
 
  samsung,link-rate = 0x0a;
  samsung,lane-count = 1;
 And these should really be derived from values in the DPCD and adjusted
 (if necessary) during link training.
 
 Why would you ever want to hard-code the above?
 
 Yes, I do meet the problem that my eDP screen need lane-count to 4, but my
 DP TV need lane-count to 1. Just like previous reply, if you think I should
 improved
 them in this series, I would rather to do it.

The problem with these is that if you keep them in for your initial
submission, you can never (or only under extreme pain) remove them.
Anything in DTB needs to be effectively supported forever.

Also since these don't make sense to hard-code, just improve the code
and get rid of the need for these DT properties. Mind you that you still
need to keep the code to parse them, because presumably Exynos relies on
them. But depending on how you split up the driver you might be able to
restrict these compatibility hacks to Exynos and not carry them forward
into your new driver.

 + dp-clk_24m = devm_clk_get(dev, clk_dp_24m);
 Same here, maybe dp_24m.
 Like my previous reply. And actually as those two clocks all have
 a common prefix SCLK in rk3288 clock tree, I thinkt we can name
 them to sclk_dp  sclk_dp_24m, is it okay ?
 I don't think there's a need for these common prefixes. The names here
 are identifiers in the context of the IP block, so any SoC-specific
 prefixes are irrelevant. Also they do appear, in DT and in code, in the
 context of clocks already, so sclk_ or clk_ is completely redundant
 in these names.
 
 The sclk_dp  sclk_dp_24m is not IP common ask, it's only exist in RK3288
 SoC (Like exynos
 only got one dp clock), and actually I add this to rockchip platform dp
 driver not analogix
 dp driver. So I think it's okay to add some platform some common prefixes.
 
 And I got a better idea for those clock. sclk_dp  sclk_dp_24m is
 provided for the eDP phy,
 and I just take Heiko suggest that add an new phy-rockchip-dp.c driver, so
 it's better to move
 those clock to phy driver, and rename them to dp-phy  dp-phy-24m.

I agree that dealing with these in a PHY driver sounds like the better
option. However, I still think that the dp-phy prefix is redundant. The
names are in a per-driver scope, so dp-phy is implied by the device
tree binding and driver already. You could simply use shorter names such
as phy and 24m for example.

Also note that the clock provider will already have the proper names for
these, so the clock tree will end up showing the provider names. The
names in the binding are merely the consumer names.

Thierry


signature.asc
Description: PGP signature

[PATCH 2/4] drm/bridge: Add vendor prefixes

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Use vendor prefixes for Kconfig symbols and filenames. This should make
it easier to identify the various bridge drivers and to organize the
directory.

While at it, rename dw_hdmi.[ch] to dw-hdmi.[ch] for consistency.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/configs/exynos_defconfig|  4 ++--
 arch/arm/configs/multi_v7_defconfig  |  4 ++--
 drivers/gpu/drm/bridge/Kconfig   | 10 +-
 drivers/gpu/drm/bridge/Makefile  |  6 +++---
 drivers/gpu/drm/bridge/{dw_hdmi.c = dw-hdmi.c}  |  2 +-
 drivers/gpu/drm/bridge/{dw_hdmi.h = dw-hdmi.h}  |  0
 drivers/gpu/drm/bridge/{ptn3460.c = nxp-ptn3460.c}  |  0
 drivers/gpu/drm/bridge/{ps8622.c = parade-ps8622.c} |  0
 8 files changed, 13 insertions(+), 13 deletions(-)
 rename drivers/gpu/drm/bridge/{dw_hdmi.c = dw-hdmi.c} (99%)
 rename drivers/gpu/drm/bridge/{dw_hdmi.h = dw-hdmi.h} (100%)
 rename drivers/gpu/drm/bridge/{ptn3460.c = nxp-ptn3460.c} (100%)
 rename drivers/gpu/drm/bridge/{ps8622.c = parade-ps8622.c} (100%)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 67965cedeb69..099438e19f05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -125,8 +125,8 @@ CONFIG_REGULATOR_S2MPS11=y
 CONFIG_REGULATOR_S5M8767=y
 CONFIG_REGULATOR_TPS65090=y
 CONFIG_DRM=y
-CONFIG_DRM_PTN3460=y
-CONFIG_DRM_PS8622=y
+CONFIG_DRM_NXP_PTN3460=y
+CONFIG_DRM_PARADE_PS8622=y
 CONFIG_DRM_EXYNOS=y
 CONFIG_DRM_EXYNOS_FIMD=y
 CONFIG_DRM_EXYNOS_DSI=y
diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index fb587b2330ad..29be77726ec8 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -438,8 +438,8 @@ CONFIG_VIDEO_RENESAS_VSP1=m
 CONFIG_VIDEO_ADV7180=m
 CONFIG_VIDEO_ML86V7667=m
 CONFIG_DRM=y
-CONFIG_DRM_PTN3460=m
-CONFIG_DRM_PS8622=m
+CONFIG_DRM_NXP_PTN3460=m
+CONFIG_DRM_PARADE_PS8622=m
 CONFIG_DRM_EXYNOS=m
 CONFIG_DRM_EXYNOS_DSI=y
 CONFIG_DRM_EXYNOS_FIMD=y
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index acef3223772c..adac3250684b 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -3,16 +3,16 @@ config DRM_DW_HDMI
depends on DRM
select DRM_KMS_HELPER
 
-config DRM_PTN3460
-   tristate PTN3460 DP/LVDS bridge
+config DRM_NXP_PTN3460
+   tristate NXP PTN3460 DP/LVDS bridge
depends on DRM
depends on OF
select DRM_KMS_HELPER
select DRM_PANEL
---help---
- ptn3460 eDP-LVDS bridge chip driver.
+ NXP PTN3460 eDP-LVDS bridge chip driver.
 
-config DRM_PS8622
+config DRM_PARADE_PS8622
tristate Parade eDP/LVDS bridge
depends on DRM
depends on OF
@@ -21,4 +21,4 @@ config DRM_PS8622
select BACKLIGHT_LCD_SUPPORT
select BACKLIGHT_CLASS_DEVICE
---help---
- parade eDP-LVDS bridge chip driver.
+ Parade eDP-LVDS bridge chip driver.
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 8dfebd984370..fe490e2a7f25 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,5 +1,5 @@
 ccflags-y := -Iinclude/drm
 
-obj-$(CONFIG_DRM_PS8622) += ps8622.o
-obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
-obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
+obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
+obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
+obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
similarity index 99%
rename from drivers/gpu/drm/bridge/dw_hdmi.c
rename to drivers/gpu/drm/bridge/dw-hdmi.c
index 816d104ca4da..8bb00b70702a 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -26,7 +26,7 @@
 #include drm/drm_encoder_slave.h
 #include drm/bridge/dw_hdmi.h
 
-#include dw_hdmi.h
+#include dw-hdmi.h
 
 #define HDMI_EDID_LEN  512
 
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
similarity index 100%
rename from drivers/gpu/drm/bridge/dw_hdmi.h
rename to drivers/gpu/drm/bridge/dw-hdmi.h
diff --git a/drivers/gpu/drm/bridge/ptn3460.c 
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
similarity index 100%
rename from drivers/gpu/drm/bridge/ptn3460.c
rename to drivers/gpu/drm/bridge/nxp-ptn3460.c
diff --git a/drivers/gpu/drm/bridge/ps8622.c 
b/drivers/gpu/drm/bridge/parade-ps8622.c
similarity index 100%
rename from drivers/gpu/drm/bridge/ps8622.c
rename to drivers/gpu/drm/bridge/parade-ps8622.c
-- 
2.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] ARM: exynos: Remove PTN3460 dependency

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Now that the PTN3460 driver has been rewritten as a proper I2C driver
and there is infrastructure to hook up the bridge with a DRM device, it
is no longer necessary to have this dependency to ensure the correct
build mode.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/exynos/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 43003c4ad80b..df0b61a60501 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -56,7 +56,7 @@ config DRM_EXYNOS_DSI
 
 config DRM_EXYNOS_DP
bool EXYNOS DRM DP driver support
-   depends on DRM_EXYNOS  (DRM_EXYNOS_FIMD || DRM_EXYNOS7_DECON)  
(DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS)
+   depends on DRM_EXYNOS  (DRM_EXYNOS_FIMD || DRM_EXYNOS7_DECON)
default DRM_EXYNOS
select DRM_PANEL
help
-- 
2.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] drm/bridge: Put Kconfig entries in a separate menu

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Put the Kconfig entries for bridge drivers into a separate menu so that
they are automatically grouped and don't clutter up the top-level menu.

While at it, move the bridge menu towards the end of the top-level menu
where the panel menu is already located.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/Kconfig|  4 ++--
 drivers/gpu/drm/bridge/Kconfig | 14 +++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 06ae5008c5ed..86191586340f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -99,8 +99,6 @@ config DRM_KMS_CMA_HELPER
 
 source drivers/gpu/drm/i2c/Kconfig
 
-source drivers/gpu/drm/bridge/Kconfig
-
 config DRM_TDFX
tristate 3dfx Banshee/Voodoo3+
depends on DRM  PCI
@@ -255,6 +253,8 @@ source drivers/gpu/drm/tegra/Kconfig
 
 source drivers/gpu/drm/panel/Kconfig
 
+source drivers/gpu/drm/bridge/Kconfig
+
 source drivers/gpu/drm/sti/Kconfig
 
 source drivers/gpu/drm/amd/amdkfd/Kconfig
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index adac3250684b..2de52a53a803 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -1,11 +1,18 @@
+config DRM_BRIDGE
+   def_bool y
+   depends on DRM
+   help
+ Bridge registration and lookup framework.
+
+menu Display Interface Bridges
+   depends on DRM  DRM_BRIDGE
+
 config DRM_DW_HDMI
tristate
-   depends on DRM
select DRM_KMS_HELPER
 
 config DRM_NXP_PTN3460
tristate NXP PTN3460 DP/LVDS bridge
-   depends on DRM
depends on OF
select DRM_KMS_HELPER
select DRM_PANEL
@@ -14,7 +21,6 @@ config DRM_NXP_PTN3460
 
 config DRM_PARADE_PS8622
tristate Parade eDP/LVDS bridge
-   depends on DRM
depends on OF
select DRM_PANEL
select DRM_KMS_HELPER
@@ -22,3 +28,5 @@ config DRM_PARADE_PS8622
select BACKLIGHT_CLASS_DEVICE
---help---
  Parade eDP-LVDS bridge chip driver.
+
+endmenu
-- 
2.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] drm/panel: Add Samsung prefix to panel drivers

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

The likelihood of getting a large number of panel drivers from different
vendors is quite high. Add a prefix to the two existing Samsung panel
drivers to set a guideline for future patch submissions. Using vendor
prefixes consistently should allow a cleaner organization of the tree.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/configs/exynos_defconfig | 2 +-
 arch/arm/configs/multi_v7_defconfig   | 2 +-
 drivers/gpu/drm/panel/Kconfig | 8 
 drivers/gpu/drm/panel/Makefile| 4 ++--
 drivers/gpu/drm/panel/{panel-ld9040.c = panel-samsung-ld9040.c}  | 2 +-
 .../gpu/drm/panel/{panel-s6e8aa0.c = panel-samsung-s6e8aa0.c}| 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)
 rename drivers/gpu/drm/panel/{panel-ld9040.c = panel-samsung-ld9040.c} (99%)
 rename drivers/gpu/drm/panel/{panel-s6e8aa0.c = panel-samsung-s6e8aa0.c} (99%)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 099438e19f05..354124c3ed2c 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -132,7 +132,7 @@ CONFIG_DRM_EXYNOS_FIMD=y
 CONFIG_DRM_EXYNOS_DSI=y
 CONFIG_DRM_EXYNOS_HDMI=y
 CONFIG_DRM_PANEL_SIMPLE=y
-CONFIG_DRM_PANEL_S6E8AA0=y
+CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0=y
 CONFIG_FB_SIMPLE=y
 CONFIG_EXYNOS_VIDEO=y
 CONFIG_EXYNOS_MIPI_DSI=y
diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 29be77726ec8..6ba9608798ab 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -446,7 +446,7 @@ CONFIG_DRM_EXYNOS_FIMD=y
 CONFIG_DRM_EXYNOS_HDMI=y
 CONFIG_DRM_RCAR_DU=m
 CONFIG_DRM_TEGRA=y
-CONFIG_DRM_PANEL_S6E8AA0=m
+CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0=m
 CONFIG_DRM_PANEL_SIMPLE=y
 CONFIG_FB_ARMCLCD=y
 CONFIG_FB_WM8505=y
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6d64c7bb908b..5be25d9282e3 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -18,13 +18,13 @@ config DRM_PANEL_SIMPLE
  that it can be automatically turned off when the panel goes into a
  low power state.
 
-config DRM_PANEL_LD9040
-   tristate LD9040 RGB/SPI panel
+config DRM_PANEL_SAMSUNG_LD9040
+   tristate Samsung LD9040 RGB/SPI panel
depends on OF  SPI
select VIDEOMODE_HELPERS
 
-config DRM_PANEL_S6E8AA0
-   tristate S6E8AA0 DSI video mode panel
+config DRM_PANEL_SAMSUNG_S6E8AA0
+   tristate Samsung S6E8AA0 DSI video mode panel
depends on OF
select DRM_MIPI_DSI
select VIDEOMODE_HELPERS
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4b2a0430804b..8026ce5d18b5 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
-obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
-obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
diff --git a/drivers/gpu/drm/panel/panel-ld9040.c 
b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
similarity index 99%
rename from drivers/gpu/drm/panel/panel-ld9040.c
rename to drivers/gpu/drm/panel/panel-samsung-ld9040.c
index bba3501c5821..fcb827003a8c 100644
--- a/drivers/gpu/drm/panel/panel-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
@@ -378,7 +378,7 @@ static struct spi_driver ld9040_driver = {
.probe = ld9040_probe,
.remove = ld9040_remove,
.driver = {
-   .name = ld9040,
+   .name = panel-samsung-ld9040,
.owner = THIS_MODULE,
.of_match_table = ld9040_of_match,
},
diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
similarity index 99%
rename from drivers/gpu/drm/panel/panel-s6e8aa0.c
rename to drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
index 9e79bc43b047..6b11e75ff645 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
@@ -1052,7 +1052,7 @@ static struct mipi_dsi_driver s6e8aa0_driver = {
.probe = s6e8aa0_probe,
.remove = s6e8aa0_remove,
.driver = {
-   .name = panel_s6e8aa0,
+   .name = panel-samsung-s6e8aa0,
.of_match_table = s6e8aa0_of_match,
},
 };
-- 
2.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pwm: samsung: Use MODULE_DEVICE_TABLE() to include OF modalias

[Thierry Reding]

On Thu, May 14, 2015 at 02:32:31AM +0200, Javier Martinez Canillas wrote:
 If the pwm-samsung driver is built as a module, modalias information is
 not filled so the module is not autoloaded. Use the MODULE_DEVICE_TABLE()
 macro to export the OF device ID so the module contains that information.
 
 Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
 ---
  drivers/pwm/pwm-samsung.c | 1 +
  1 file changed, 1 insertion(+)

Applied, thanks.

Thierry


pgpOELXi33qbM.pgp
Description: PGP signature

Re: [PATCH 4/8] clk: tegra: Fix duplicate const for parent names

[Thierry Reding]

On Thu, Apr 09, 2015 at 12:07:59PM +0200, Krzysztof Kozlowski wrote:
 2015-04-09 12:00 GMT+02:00 Thierry Reding thierry.red...@gmail.com:
  On Wed, Apr 08, 2015 at 03:22:15PM +0200, Krzysztof Kozlowski wrote:
  Replace duplicated const keyword for 'emc_parent_clk_names' with proper
  array of const pointers to const strings.
 
  Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
  ---
   drivers/clk/tegra/clk-emc.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  This would probably better go in via the Tegra tree since the patch that
  contains this has only made it to linux-next.
 
  Stephen, Mike, any objections to me taking this?
 
 Applying this without the change for const-ness of parent_names (patch
 by Sascha Hauer sent before mine [1]) would introduce a warning -
 assign of const to non-const. Any idea to solve it? Immutable branch?

Right, I had missed that. Immutable branch would work, though perhaps
it'd be easier to just defer this until after v4.1-rc1. The warning
shouldn't happen if we leave out this single patch and apply it later
on, right?

Alternatively the whole series could be deferred until after v4.1-rc1.

Thierry


pgp_USze9Cb6U.pgp
Description: PGP signature

Re: [PATCH 4/8] clk: tegra: Fix duplicate const for parent names

[Thierry Reding]

On Wed, Apr 08, 2015 at 03:22:15PM +0200, Krzysztof Kozlowski wrote:
 Replace duplicated const keyword for 'emc_parent_clk_names' with proper
 array of const pointers to const strings.
 
 Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
 ---
  drivers/clk/tegra/clk-emc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

This would probably better go in via the Tegra tree since the patch that
contains this has only made it to linux-next.

Stephen, Mike, any objections to me taking this?

Thierry

 diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
 index 615da43a508d..637798c3cc59 100644
 --- a/drivers/clk/tegra/clk-emc.c
 +++ b/drivers/clk/tegra/clk-emc.c
 @@ -45,7 +45,7 @@
  #define CLK_SOURCE_EMC_EMC_2X_CLK_SRC(x) (((x)  
 CLK_SOURCE_EMC_EMC_2X_CLK_SRC_MASK)  \
 CLK_SOURCE_EMC_EMC_2X_CLK_SRC_SHIFT)
  
 -static const char const *emc_parent_clk_names[] = {
 +static const char * const emc_parent_clk_names[] = {
   pll_m, pll_c, pll_p, clk_m, pll_m_ud,
   pll_c2, pll_c3, pll_c_ud
  };
 -- 
 1.9.1
 


pgpAcNw2v0Ri3.pgp
Description: PGP signature

Re: [PATCH 09/16] iommu/tegra-smmu: Make use of domain_alloc and domain_free

[Thierry Reding]

On Thu, Mar 26, 2015 at 01:43:12PM +0100, Joerg Roedel wrote:
 From: Joerg Roedel jroe...@suse.de
 
 Implement domain_alloc and domain_free iommu-ops as a
 replacement for domain_init/domain_destroy.
 
 Signed-off-by: Joerg Roedel jroe...@suse.de
 ---
  drivers/iommu/tegra-smmu.c | 41 +++--
  1 file changed, 23 insertions(+), 18 deletions(-)

Acked-by: Thierry Reding tred...@nvidia.com


pgpIvQf6VeP3T.pgp
Description: PGP signature

Re: [PATCH 00/16 v2] iommu: Move domain allocation into drivers

[Thierry Reding]

On Thu, Mar 26, 2015 at 01:43:03PM +0100, Joerg Roedel wrote:
 Changes v1-v2:
 
   * Rebased to v4.0-rc5
   * Converted domain-types to a bit-field
 
 Hi,
 
 here is patch-set to replace the existing domain_init and
 domain_destroy iommu-ops with the new domain_alloc and
 domain_free callbacks
 
 The new callbacks move the allocation of iommu domains into
 the iommu driver, allowing them to put a generic
 iommu_domain struct into their own domain struct. This makes
 domain handling in the drivers more cache efficient and
 prepares the introduction of default domains in another
 patch-set.
 
 While at it, this patch-set also introduces domain types.
 These are internal to the iommu core code for now, there are
 three of them:
 
   * DMA-API domains
   * Identity mapped domains
   * Domains unmanaged by the iommu-core, used for
 iommu-api so that the users can create their own
 mappings
 
 The patches have been compile tested for x86, ARM and PPC
 and runtime tested on x86 (Intel VT-d and AMD IOMMU).
 
 Please review.
 
 Thanks,
 
   Joerg
 
 Joerg Roedel (15):
   iommu: Introduce domain_alloc and domain_free iommu_ops
   iommu: Introduce iommu domain types
   iommu/amd: Make use of domain_alloc and domain_free
   iommu/vt-d: Make use of domain_alloc and domain_free
   iommu/omap: Make use of domain_alloc and domain_free
   iommu/arm-smmu: Make use of domain_alloc and domain_free
   iommu/exynos: Make use of domain_alloc and domain_free
   iommu/tegra-smmu: Make use of domain_alloc and domain_free
   iommu/tegra-gart: Make use of domain_alloc and domain_free
   iommu/msm: Make use of domain_alloc and domain_free
   iommu/shmobile: Make use of domain_alloc and domain_free
   iommu/ipmmu-vmsa: Make use of domain_alloc and domain_free
   iommu/rockchip: Make use of domain_alloc and domain_free
   iommu/fsl: Make use of domain_alloc and domain_free
   iommu: Remove domain_init and domain_free iommu_ops
 
  drivers/iommu/amd_iommu.c   | 84 +--
  drivers/iommu/amd_iommu_types.h |  7 ++--
  drivers/iommu/arm-smmu.c| 46 +-
  drivers/iommu/exynos-iommu.c| 87 
 ++---
  drivers/iommu/fsl_pamu_domain.c | 60 +++-
  drivers/iommu/fsl_pamu_domain.h |  2 +-
  drivers/iommu/intel-iommu.c | 48 +--
  drivers/iommu/iommu.c   | 20 ++
  drivers/iommu/ipmmu-vmsa.c  | 43 +++-
  drivers/iommu/msm_iommu.c   | 73 +-
  drivers/iommu/omap-iommu.c  | 49 +--
  drivers/iommu/rockchip-iommu.c  | 40 +++
  drivers/iommu/shmobile-iommu.c  | 40 +++
  drivers/iommu/tegra-gart.c  | 67 +--
  drivers/iommu/tegra-smmu.c  | 41 ++-
  include/linux/iommu.h   | 17 ++--
  16 files changed, 407 insertions(+), 317 deletions(-)

The core and Tegra bits:

Tested-by: Thierry Reding tred...@nvidia.com


pgpld3hZekpiI.pgp
Description: PGP signature

Re: [PATCH 10/16] iommu/tegra-gart: Make use of domain_alloc and domain_free

[Thierry Reding]

On Thu, Mar 26, 2015 at 01:43:13PM +0100, Joerg Roedel wrote:
 From: Joerg Roedel jroe...@suse.de
 
 Implement domain_alloc and domain_free iommu-ops as a
 replacement for domain_init/domain_destroy.
 
 Signed-off-by: Joerg Roedel jroe...@suse.de
 ---
  drivers/iommu/tegra-gart.c | 67 
 +++---
  1 file changed, 46 insertions(+), 21 deletions(-)

Acked-by: Thierry Reding tred...@nvidia.com


pgpqhT7suGaYx.pgp
Description: PGP signature

Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow

[Thierry Reding]

On Fri, Jan 30, 2015 at 10:37:19AM -0500, Rob Clark wrote:
 On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote:
  Currently, third party bridge drivers(ptn3460) are dependent
  on the corresponding encoder driver init, since bridge driver
  needs a drm_device pointer to finish drm initializations.
  The encoder driver passes the drm_device pointer to the
  bridge driver. Because of this dependency, third party drivers
  like ptn3460 doesn't adhere to the driver model.
 
  In this patch, we reframe the bridge registration framework
  so that bridge initialization is split into 2 steps, and
  bridge registration happens independent of drm flow:
  --Step 1: gather all the bridge settings independent of drm and
add the bridge onto a global list of bridges.
  --Step 2: when the encoder driver is probed, call drm_bridge_attach
for the corresponding bridge so that the bridge receives
drm_device pointer and continues with connector and other
drm initializations.
 
  The old set of bridge helpers are removed, and a set of new helpers
  are added to accomplish the 2 step initialization.
 
  The bridge devices register themselves onto global list of bridges
  when they get probed by calling drm_bridge_add.
 
  The parent encoder driver waits till the bridge is available
  in the lookup table(by calling of_drm_find_bridge) and then
  continues with its initialization.
 
  The encoder driver should also call drm_bridge_attach to pass
  on the drm_device to the bridge object.
 
  drm_bridge_attach inturn calls bridge-funcs-attach so that
  bridge can continue with drm related initializations.
 
 ok, so I probably should have had a closer look at this before it
 landed in drm-next, so if it is too late to revert (and deal w/
 untangling subsequent patches that depend on this) some of these
 issues we'll just have to fix with follow-on patches.
 
 1) needs headerdoc for new fxns in drm_bridge.c, and needs to be added
 in drm.tmpl

drm_panel.c is missing kerneldoc as well, though I have a local patch to
add it. If nobody else steps up I'll take this.

 2) as far as I can tell, the new approach to cleaning up bridge
 objects is to just let them leak !?!

With this series bridges are no longer part of the DRM device and it's
the driver that provides them that needs to free them (in -remove()).
It's not a completely perfect solution yet, but with the registry patch
that I proposed a while back all the remaining issues should go away.
Now if I could get anybody to look at that patch...

Thierry


pgpaoaZZoKn_V.pgp
Description: PGP signature

Re: [PATCH V9 12/14] drm/bridge: Add i2c based driver for ps8622/ps8625 bridge

[Thierry Reding]

On Thu, Jan 29, 2015 at 08:12:20PM +0530, Ajay kumar wrote:
 Hi Thierry,
 
 I think you forgot to take this patch!
 Can you check this?

Yes, I missed it somehow. It didn't build for me after applying it now,
but I fixed that up (and a few sparse warnings along with it). I'll send
out another pull request shortly.

Thierry


pgpRXBmwWcgjN.pgp
Description: PGP signature

Re: [PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support

[Thierry Reding]

On Mon, Jan 19, 2015 at 11:27:52AM +0100, Javier Martinez Canillas wrote:
 Hello Thierry,
 
 On 01/05/2015 02:50 PM, Thierry Reding wrote:
  On Fri, Jan 02, 2015 at 01:10:14PM +, Daniel Stone wrote:
  
   Ajay's series don't apply cleanly anymore because it has been a while 
   since
   he posted it but he can rebase on top of 3.19-rc1 once it is released and
   re-resend.
  
  
  Do you have any plans to rebase this so it's ready for merging?
  
  Thierry, Daniel, Dave - whose tree would this be best to merge through?
  
  The plan is for me to take the bridge patches through the drm/panel
  tree. I'm going to look at these patches again later this week but from
  a very quick peek there don't seem to be any major issues left.
 
 
 I know you probably are very busy but it would be great if you can take a look
 to this series to avoid another kernel release to be missed since we are 
 already
 at v3.19-rc5.
 
 Only this version was posted 2 months ago and the first version of the series
 was sent on May, 2014 so it has been on the list for almost a year now...
 
 Tomi and Laurent had already agreed with the DT binging so I think that we can
 already merge these and if there are any remaining issues, those can be fixed
 during the 3.20 -rc cycle.

I see only a single Tested-by on this series and that seems to be with a
bunch of out-of-tree patches applied on top. Does this now actually work
applied on top of upstream? Also it seems like many people actually want
this to get merged but there's been no Reviewed-bys and only a single
Tested-by? Is that as good as it's going to get?

Also I think the last update was that Ajay was going to resend this
based on v3.19-rc1 or something. Is that still going to happen?
Otherwise I can probably try to apply as-is, but not sure how well it
will.

Thierry


pgpDHQpUnOVrn.pgp
Description: PGP signature

Re: [Resend][PATCH v2 2/3] drm/panel: add s6e63j0x03 LCD panel driver

[Thierry Reding]

On Tue, Jan 06, 2015 at 12:28:22AM +0900, Inki Dae wrote:
[...]
  Original Message 
 Subject: [PATCH v2 2/3] drm/panel: add s6e63j0x03 LCD panel driver
 Date: Tue, 09 Dec 2014 18:29:05 +0900
 From: Hyungwon Hwang human.hw...@samsung.com
 To: dri-de...@lists.freedesktop.org
 CC: airl...@linux.ie, devicet...@vger.kernel.org, robh...@kernel.org,
 pawel.m...@arm.com, mark.rutl...@arm.com, ijc+devicet...@hellion.org.uk,
 ga...@codeaurora.org, linux-samsung-soc@vger.kernel.org,
 kyungmin.p...@samsung.com, inki@samsung.com, a.ha...@samsung.com,
 kgene@samsung.com, thierry.red...@gmail.com, Hyungwon Hwang
 human.hw...@samsung.com
 
 From: Inki Dae inki@samsung.com
 
 This patch adds MIPI-DSI based S6E63J0X03 AMOLED LCD panel driver
 which uses mipi_dsi bus to communicate with panel. The panel has
 320×320 resolution in 1.63-inch physical panel. This panel is used in
 Samsung Galaxy Gear 2.
 
 Signed-off-by: Inki Dae inki@samsung.com
 Signed-off-by: Hyungwon Hwang human.hw...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 Changes for v2:
 - Change the gamma table to 2-dimensional array
 - Change the way to make index for brightness
 - Make command functions to an array so that it can be called simply
 - Change command id for reading device ID
 - Change the way to handle the error condition
 - Remove power variable, and use the same name variable in bl_dev
 - Add the state FB_BLANK_NORMAL to represent the state which the panel
 is working but blanked
 - Miscellaneous changes to increase the readability and follow the
   coding-style standard
 
  drivers/gpu/drm/panel/Kconfig|   6 +
  drivers/gpu/drm/panel/Makefile   |   1 +
  drivers/gpu/drm/panel/panel-s6e63j0x03.c | 549
 +++
  3 files changed, 556 insertions(+)
  create mode 100644 drivers/gpu/drm/panel/panel-s6e63j0x03.c

I thought I had mentioned this before, but this is missing device tree
binding documentation.

Thierry


pgpvxq7glonOX.pgp
Description: PGP signature

Re: [PATCH] i2c: exynos5: Move initialization code to subsys_initcall()

[Thierry Reding]

On Tue, Jan 13, 2015 at 02:13:51PM +0200, Tomi Valkeinen wrote:
 On 12/01/15 10:43, Joonyoung Shim wrote:
  +Cc Tomi Valkeinen,
  
  Hi Uwe,
  
  On 01/12/2015 04:50 PM, Uwe Kleine-König wrote:
  Hello,
 
  On Mon, Jan 12, 2015 at 11:53:02AM +0900, Joonyoung Shim wrote:
  This is required in order to ensure that core system devices such as
  voltage regulators attached via I2C are available early in boot.
  Deferred probing isn't an option? If so I suggest adding the reasoning
  in a comment to stop the next person converting it to that.
  (And if not, please fix accordingly to use deferred probing.)
 
  
  I couldn't get penguin logo since the commit 92b004d (video/logo:
  prevent use of logos after they have been freed) and i just tried old
  way because i missed the flow to move to deferred probing.
  
  Fb driver probe seems to be ran between fb_logo_late_init late_initcall
  and the freeing of the logos.
  
  Any ideas?
 
 Thierry mentioned on IRC that he encountered the same issue. And I
 encountered it also.
 
 So... I'd rather not revert the fix, as it's quite a nasty one, and it
 happens while console lock is held, so it looks like the machine just
 froze. But I don't know how it could be improved with the current kernel.
 
 We could make the logos non-initdata, but I don't much like that option.
 Or we could perhaps implement some new way to catch the freeing of initdata.
 
 Any other ideas?

I think we could still make the logos non-initdata based on a Kconfig
symbol. Another option might be to copy the logo data to memory that's
not automatically freed after init, so that we get better control over
when it is released. I tried tracing the various parts that would need
this data, but couldn't find any place after which it isn't needed
anymore. Specifically it is code that can be executed on every console
switch, so we can't really get rid of it at any sensible time.

I'd argue that if it's needed at every VT switch where the framebuffer
console is activated, then we really can't get rid of it at all. Or we
don't display the logo at every switch and can free the backing memory
right after the first switch for example.

Thierry


pgpXFoIadIHjm.pgp
Description: PGP signature

Re: [PATCH v2 06/21] ARM: tegra: remove old LIC support

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:41PM +, Marc Zyngier wrote:
[...]
 diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
[...]
  void __init tegra_init_irq(void)
  {
 - int i;
 - void __iomem *distbase;
 -
 - if (of_find_matching_node(NULL, tegra_ictlr_match))
 - goto skip_extn_setup;
 -
 - tegra_legacy_irq_syscore_init();
 -
 - distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
 - num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR)  0x1f;
 -
 - if (num_ictlrs  ARRAY_SIZE(ictlr_reg_base)) {
 - WARN(1, Too many (%d) interrupt controllers found. Maximum is 
 %d.,
 - num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
 - num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
 - }
 -
 - for (i = 0; i  num_ictlrs; i++) {
 - void __iomem *ictlr = ictlr_reg_base[i];
 - writel(~0, ictlr + ICTLR_CPU_IER_CLR);
 - writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
 - }
 -
 - gic_arch_extn.irq_ack = tegra_ack;
 - gic_arch_extn.irq_eoi = tegra_eoi;
 - gic_arch_extn.irq_mask = tegra_mask;
 - gic_arch_extn.irq_unmask = tegra_unmask;
 - gic_arch_extn.irq_retrigger = tegra_retrigger;
 - gic_arch_extn.irq_set_wake = tegra_set_wake;
 - gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 + if (!of_find_matching_node(NULL, tegra_ictlr_match))
 + pr_warn(Outdated DT detected, suspend/resume will NOT work\n);

I'm not very happy about the ABI breakage here, but I also realize that
we need this change to properly describe the hardware. To make it more
obvious that people really should update their DTBs, maybe turn this
into a WARN()?

 -skip_extn_setup:
   tegra114_gic_cpu_pm_registration();

I'm not intimately familiar with the GIC, but is this really SoC
specific? Doesn't anybody else need this? Comparing to the GIC spec the
write of 0x1e0 to the GIC_CPU_CTRL register (which I assume corresponds
to GICC_CTLR in the spec), this simply disables the IRQ and FIQ bypass
signals for both group 0 and group 1.

Thierry


pgpVZ3oRmbaxp.pgp
Description: PGP signature

Re: [PATCH v2 01/21] ARM: tegra: irq: nuke leftovers from non-DT support

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:36PM +, Marc Zyngier wrote:
 The GIC is now always initialized from DT on tegra, and there is
 no point in keeping non-DT init code.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/mach-tegra/irq.c | 8 
  1 file changed, 8 deletions(-)

Acked-by: Thierry Reding tred...@nvidia.com


pgpwOEMBfpvYA.pgp
Description: PGP signature

Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:37PM +, Marc Zyngier wrote:
 Tegra's LIC (Legacy Interrupt Controller) has been so far only
 supported as a weird extension of the GIC, which is not exactly
 pretty.
 
 The stacked irq domain framework fits this pretty well, and allows

Nit: s/irq/IRQ/

 the LIC code to be turned into a standalone irqchip. In the process,
 make the driver DT aware, something that was sorely missing from
 the mach-tegra implementation.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  drivers/irqchip/Makefile|   1 +
  drivers/irqchip/irq-tegra.c | 335 
 
  2 files changed, 336 insertions(+)
  create mode 100644 drivers/irqchip/irq-tegra.c

This matches largely what I have in a local patch (modulo the stacked
domains vs. gic_arch_extn). A few comments below.

 diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
 index 9516a32..59f34be 100644
 --- a/drivers/irqchip/Makefile
 +++ b/drivers/irqchip/Makefile
 @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_HIP04)  += irq-hip04.o
  obj-$(CONFIG_ARCH_MMP)   += irq-mmp.o
  obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
  obj-$(CONFIG_ARCH_MXS)   += irq-mxs.o
 +obj-$(CONFIG_ARCH_TEGRA) += irq-tegra.o
  obj-$(CONFIG_ARCH_S3C24XX)   += irq-s3c24xx.o

Should these be sorted alphabetically?

 diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
[...]
 +#define TEGRA_MAX_NUM_ICTLRS 5
 +
 +static int num_ictlrs;

This could be unsigned int.

 +struct tegra_ictlr_info {
 + void __iomem *ictlr_reg_base[TEGRA_MAX_NUM_ICTLRS];

Maybe only reg_base or base. The ictlr_ prefix is redundant.

 +#ifdef CONFIG_PM_SLEEP
 + u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
 + u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
 + u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
 + u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
 +
 + u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];

Same here.

 +#endif
 +};
 +
 +static struct tegra_ictlr_info *tegra_ictlr_info;

This variable name could be shorter, say lic, to make some of the code
below more terse.

 +static int tegra_ictlr_suspend(void)
 +{
 + unsigned long flags;
 + int i;

This could be unsigned int, too.

[...]
 +static void tegra_ictlr_resume(void)
 +{
 + unsigned long flags;
 + int i;

And here.

 +static struct syscore_ops tegra_ictlr_syscore_ops = {
 + .suspend= tegra_ictlr_suspend,
 + .resume = tegra_ictlr_resume,
 +};
 +
 +static int tegra_ictlr_syscore_init(void)
 +{
 + register_syscore_ops(tegra_ictlr_syscore_ops);
 +
 + return 0;
 +}
 +#else
 +#define tegra_set_wake   NULL
 +static inline void tegra_ictlr_syscore_init(void) {}
 +#endif

Both prototypes for tegra_ictlr_syscore_init() should match here. Since
it never fails, returning void for both should be fine.

 +static struct irq_chip tegra_ictlr_chip = {
 + .name   = ICTLR,

Maybe name it LIC since that's the more common name for it?

 +static int tegra_ictlr_domain_xlate(struct irq_domain *domain,
 + struct device_node *controller,
 + const u32 *intspec,
 + unsigned int intsize,
 + unsigned long *out_hwirq,
 + unsigned int *out_type)
 +{
 + if (domain-of_node != controller)
 + return -EINVAL; /* Shouldn't happen, really... */
 + if (intsize != 3)
 + return -EINVAL; /* Not GIC compliant */
 + if (intspec[0] != 0)

Maybe use GIC_SPI from dt-bindings/interrupt-controller/arm-gic.h here?
To match the DTS content?

 +static int tegra_ictlr_domain_alloc(struct irq_domain *domain,
 + unsigned int virq,
 + unsigned int nr_irqs, void *data)
 +{
 + struct of_phandle_args *args = data;
 + struct of_phandle_args parent_args;
 + struct tegra_ictlr_info *info = domain-host_data;
 + irq_hw_number_t hwirq;
 + int i;

unsigned int?

 +
 + if (args-args_count != 3)
 + return -EINVAL; /* Not GIC compliant */
 + if (args-args[0] != 0)

GIC_SPI?

 + hwirq = args-args[1];
 + if (hwirq = (num_ictlrs * 32))
 + return -EINVAL; /* Can't deal with this */

Not sure if that comment is necessary here. It's fairly obvious why this
is an error. But it doesn't hurt, so if you prefer to leave it, that's
fine with me.

 +
 + for (i = 0; i  nr_irqs; i++) {
 + int ictlr = (hwirq + i) / 32;

irq_hw_number_t? Or unsigned int?

 +
 + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
 +   tegra_ictlr_chip,
 +   info-ictlr_reg_base[ictlr]);
 + }
 +
 + parent_args = *args;
 + parent_args.np = domain-parent-of_node;
 + return irq_domain_alloc_irqs_parent(domain, 

Re: [PATCH v2 05/21] DT: tegra: add binding for the legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:40PM +, Marc Zyngier wrote:
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  .../interrupt-controller/nvidia,tegra-ictlr.txt| 39 
 ++
  1 file changed, 39 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 
 diff --git 
 a/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
  
 b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 new file mode 100644
 index 000..44fd873
 --- /dev/null
 +++ 
 b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 @@ -0,0 +1,39 @@
 +NVIDIA Legacy Interrupt Controller
 +
 +All Tegra SoCs contain a legacy interrupt controller that routes
 +interrupts to the GIC, and also serves as a wakeup source. It is also
 +refered to as ictlr, hence the name of the binding.
 +
 +The HW block exposes a number of frames, each implementing a set of 32
 +interrupts.

I don't think I've ever seen them referred to as frames. They are
technically separate instances of the same controller. Maybe:

The HW block exposes a number of controllers, ...

?

 +
 +Reguired properties:
 +
 +- compatible : should contain at least nvidia,tegra-ictlr.

As previously discussed I think this should be something along the lines
of:

should be one of:
- nvidia,tegra20-ictlr
- nvidia,tegra30-ictlr

Or similar. In the past, we've used nvidia,tegrachip-foo to wildcard
the compatible string so that we don't have to modify the documentation
for every new chip. The above has the disadvantage that it omits that we
should always provide a most specific compatible string, too, so maybe
something like the following would be even better:

should be: nvidia,tegrachip-ictlr. The LIC on subsequent
SoCs remained backwards-compatible with Tegra30, so on Tegra
generations later than Tegra30 the compatible value should
include nvidia,tegra30-ictlr.

 +- reg : Specifies base physical address and size of the registers.
 +  Each frame must be described separately.

Each controller must be described separately.? Also maybe mention that
this Tegra20 has 4 of them, whereas Tegra30 and later have 5? That way
people will know how many entries are required.

Thierry


pgpfswePwdlE0.pgp
Description: PGP signature

Re: [PATCH v2 04/21] ARM: tegra: update DTs to expose legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:39PM +, Marc Zyngier wrote:
 Describe the legacy interrupt controller in every tegra DTSI files,
 and make it the parent of most interrupts.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/boot/dts/tegra114.dtsi | 16 +++-
  arch/arm/boot/dts/tegra124.dtsi | 16 +++-
  arch/arm/boot/dts/tegra20.dtsi  | 15 ++-
  arch/arm/boot/dts/tegra30.dtsi  | 16 +++-
  4 files changed, 59 insertions(+), 4 deletions(-)
 
 diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
 index 4296b53..f70bed0 100644
 --- a/arch/arm/boot/dts/tegra114.dtsi
 +++ b/arch/arm/boot/dts/tegra114.dtsi
 @@ -8,7 +8,7 @@
  
  / {
   compatible = nvidia,tegra114;
 - interrupt-parent = gic;
 + interrupt-parent = ictlr;

Maybe name the label lic because that's the more common name for this
controller? Same for the other DTSI files.

 @@ -134,6 +134,19 @@
 0x50046000 0x2000;
   interrupts = GIC_PPI 9
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH);
 + interrupt-parent = gic;

Is this allowed? It makes the GIC its own parent. I guess we need it to
stop a loop from GIC - LIC - GIC, but it doesn't look quite right.

 + };
 +
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra114-ictlr, nvidia,tegra-ictlr;

As previously discussed, I think the second string should be
nvidia,tegra30-ictlr.

 + reg = 0x60004000 64,

I think the first entry should be 256 bytes long since there's another
block of 192 bytes of registers that's part of the interrupt controller,
albeit maybe not related to the functionality of the interrupt chip. But
they're still part of the same hardware block.

 +   0x60004100 64,

According to the TRM there are 4 more registers in this block, so this
should be 80 in size.

 +   0x60004200 64,
 +   0x60004300 64,
 +   0x60004400 64;

I'd prefer all of these to be hexadecimal.

 + interrupt-controller;
 + #interrupt-cells = 3;
 + interrupt-parent = gic;
   };
  
   timer@60005000 {
 @@ -766,5 +779,6 @@
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW),
   GIC_PPI 10
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW);
 + interrupt-parent = gic;

Why does this get to have a non-default parent?

   };
  };
 diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
[...]
 @@ -190,6 +191,18 @@
   status = disabled;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra124-ictlr, nvidia,tegra-ictlr;

Same as for Tegra114.

 + reg = 0x0 0x60004000 0x0 0x40,
 +   0x0 0x60004100 0x0 0x40,
 +   0x0 0x60004200 0x0 0x40,
 +   0x0 0x60004300 0x0 0x40,
 +   0x0 0x60004400 0x0 0x40;

According to the TRM, entries 1-4 should be 0x100 bytes. Even the first
entry could be 0x100 bytes long since there are additional registers in
there. While they may not be directly relevant to the interrupt chip, I
think it would make sense to include them here because they are part of
the same hardware block.

 diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
 index 8acf5d8..ab2f004 100644
 --- a/arch/arm/boot/dts/tegra20.dtsi
 +++ b/arch/arm/boot/dts/tegra20.dtsi
 @@ -7,7 +7,7 @@
  
  / {
   compatible = nvidia,tegra20;
 - interrupt-parent = intc;
 + interrupt-parent = ictlr;

I wonder if we shouldn't rename the intc label to gic for consistency.
Could be in a follow-up patch though, and something that I can easily do
after this patch set.

  
   host1x@5000 {
   compatible = nvidia,tegra20-host1x, simple-bus;
 @@ -142,6 +142,7 @@
  
   timer@50004600 {
   compatible = arm,cortex-a9-twd-timer;
 + interrupt-parent = intc;
   reg = 0x50040600 0x20;
   interrupts = GIC_PPI 13
   (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH);
 @@ -154,6 +155,7 @@
  0x50040100 0x0100;
   interrupt-controller;
   #interrupt-cells = 3;
 + interrupt-parent = intc;
   };
  
   cache-controller@50043000 {
 @@ -165,6 +167,17 @@
   cache-level = 2;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra20-ictlr, nvidia,tegra-ictlr;

As discussed previously, I think the second compatible should be
dropped.

 + reg = 0x60004000 64,
 +   0x60004100 64,
 +   0x60004200 64,
 +   0x60004300 64;

Same comments as for Tegra114, except the quinary controller which
doesn't exist on Tegra20.

 + 

Re: [PATCH v2 2/3] drm/panel: add s6e63j0x03 LCD panel driver

[Thierry Reding]

On Wed, Dec 31, 2014 at 07:41:43PM +0900, Inki Dae wrote:
 Hi Thierry,
 
 Ping~.
 
 Or is it ok to pick up this patch to my tree, exynos-drm-next? It
 doesn't seem to care for a long time.

I don't seem to have a copy of the v2 2/3 patch. All I found in my inbox
is the v2 0/3 cover-letter. Please resend with me in Cc so that I can
properly review (and apply) the patch.

Thierry


pgpFd56LSEeeQ.pgp
Description: PGP signature

Re: [PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support

[Thierry Reding]

On Fri, Jan 02, 2015 at 01:10:14PM +, Daniel Stone wrote:
 Hi Ajay,
 
 On 17 December 2014 at 09:31, Javier Martinez Canillas 
 javier.marti...@collabora.co.uk wrote:
 
  On 12/16/2014 12:37 AM, Laurent Pinchart wrote:
   You asked Ajay to change his series to use the video port and enpoints
  DT
   bindings instead of phandles, could you please review his latest
  version?
  
   I guess is now too late for 3.19 since we are in the middle of the merge
   window but it would be great if this series can at least made it to
  3.20.
  
   I don't have time to review the series in details right now, but I'm
  happy
   with the DT bindings, and have no big issue with the rest of the
  patches. I
   don't really like the of_drm_find_bridge() concept introduced in 03/14
  but I
   won't nack it given lack of time to implement an alternative proposal.
  It's an
   internal API, it can always be reworked later anyway.
 
  Thanks a lot for taking the time to look at the DT bindings, then I guess
  that the series are finally ready to be merged?
 
  Ajay's series don't apply cleanly anymore because it has been a while since
  he posted it but he can rebase on top of 3.19-rc1 once it is released and
  re-resend.
 
 
 Do you have any plans to rebase this so it's ready for merging?
 
 Thierry, Daniel, Dave - whose tree would this be best to merge through?

The plan is for me to take the bridge patches through the drm/panel
tree. I'm going to look at these patches again later this week but from
a very quick peek there don't seem to be any major issues left.

Thierry


pgpBXPr2gyzrt.pgp
Description: PGP signature

[PATCH 01/13] drm/irq: Remove negative CRTC index special-case

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

The drm_send_vblank_event() function treats negative CRTC indices as
meaning that a driver doesn't have proper VBLANK handling. This is the
only place where DRM needs negative CRTC indices, so in order to enable
subsequent cleanup, remove this special case and replace it by the more
obvious check for whether or not VBLANK support was initialized.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/drm_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 75647e7f012b..a24658162284 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -934,7 +934,7 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc,
struct timeval now;
unsigned int seq;
 
-   if (crtc = 0) {
+   if (dev-num_crtcs  0) {
seq = drm_vblank_count_and_time(dev, crtc, now);
} else {
seq = 0;
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/13] drm/irq: Move some prototypes to drm_crtc.h

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

The new prototypes that deal with struct drm_crtc * directly are better
located in include/drm/drm_crtc.h along with the other functions that
deal with CRTCs.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 include/drm/drmP.h | 17 -
 include/drm/drm_crtc.h | 15 +++
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 7baef1dff5f3..4809b6f8be8b 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1069,27 +1069,16 @@ extern int drm_vblank_init(struct drm_device *dev, 
unsigned int num_crtcs);
 extern int drm_wait_vblank(struct drm_device *dev, void *data,
   struct drm_file *filp);
 extern u32 drm_vblank_count(struct drm_device *dev, int pipe);
-extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
 extern u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
 struct timeval *vblanktime);
-extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
- struct timeval *vblanktime);
 extern void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
  struct drm_pending_vblank_event *e);
-extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
-  struct drm_pending_vblank_event *e);
 extern bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
-extern bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
 extern int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
 extern void drm_vblank_put(struct drm_device *dev, unsigned int pipe);
-extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
-extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
 extern void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
-extern void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, unsigned int pipe);
 extern void drm_vblank_on(struct drm_device *dev, unsigned int pipe);
-extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
-extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
 
 extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
@@ -1098,8 +1087,6 @@ extern int drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
 unsigned flags,
 const struct drm_crtc *refcrtc,
 const struct drm_display_mode 
*mode);
-extern void drm_calc_timestamping_constants(struct drm_crtc *crtc,
-   const struct drm_display_mode 
*mode);
 
 /**
  * drm_crtc_vblank_waitqueue - get vblank waitqueue for the CRTC
@@ -1107,6 +1094,10 @@ extern void drm_calc_timestamping_constants(struct 
drm_crtc *crtc,
  *
  * This function returns a pointer to the vblank waitqueue for the CRTC.
  * Drivers can use this to implement vblank waits using wait_event()  co.
+ *
+ * XXX: Move this to include/drm/drm_crtc.h once per-CRTC VBLANK data has
+ * moved into struct drm_crtc. It can't currently be moved because drm_crtc.h
+ * doesn't know the definition of struct drm_device.
  */
 static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc 
*crtc)
 {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ea4dc4cc49c6..c1b639f55401 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1147,6 +1147,21 @@ extern int drm_crtc_init_with_planes(struct drm_device 
*dev,
 extern void drm_crtc_cleanup(struct drm_crtc *crtc);
 extern unsigned int drm_crtc_index(struct drm_crtc *crtc);
 
+extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
+extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ struct timeval *vblanktime);
+extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
+  struct drm_pending_vblank_event *e);
+extern bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
+extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
+extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
+extern void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
+extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
+extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
+
+extern void drm_calc_timestamping_constants(struct drm_crtc *crtc,
+   const struct drm_display_mode 
*mode);
+
 /**
  * drm_crtc_mask - find the mask of a registered CRTC
  * @crtc: CRTC to find mask for
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/13] drm/imx: Make pipe number unsigned

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

There's no reason whatsoever why this should ever be negative.

Cc: Philipp Zabel p.za...@pengutronix.de
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
 drivers/gpu/drm/imx/imx-drm.h  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c 
b/drivers/gpu/drm/imx/imx-drm-core.c
index b250130debc8..c52eccc58a95 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -44,7 +44,7 @@ struct imx_drm_device {
 
 struct imx_drm_crtc {
struct drm_crtc *crtc;
-   int pipe;
+   unsigned intpipe;
struct imx_drm_crtc_helper_funcsimx_drm_helper_funcs;
struct device_node  *port;
 };
@@ -52,7 +52,7 @@ struct imx_drm_crtc {
 static int legacyfb_depth = 16;
 module_param(legacyfb_depth, int, 0444);
 
-int imx_drm_crtc_id(struct imx_drm_crtc *crtc)
+unsigned int imx_drm_crtc_id(struct imx_drm_crtc *crtc)
 {
return crtc-pipe;
 }
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index 7453ae00c412..a15b80319a6a 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -12,7 +12,7 @@ struct drm_framebuffer;
 struct imx_drm_crtc;
 struct platform_device;
 
-int imx_drm_crtc_id(struct imx_drm_crtc *crtc);
+unsigned int imx_drm_crtc_id(struct imx_drm_crtc *crtc);
 
 struct imx_drm_crtc_helper_funcs {
int (*enable_vblank)(struct drm_crtc *crtc);
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/13] drm/irq: Make pipe unsigned and name consistent

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Name all references to the pipe number (CRTC index) consistently to make
it easier to distinguish which is a pipe number and which is a pointer
to struct drm_crtc.

While at it also make all references to the pipe number unsigned because
there is no longer any reason why it should ever be negative.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/drm_irq.c | 319 +++---
 include/drm/drmP.h|  34 ++---
 2 files changed, 179 insertions(+), 174 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index cb207e047505..9c32a1d7aec2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -43,8 +43,8 @@
 #include linux/export.h
 
 /* Access macro for slots in vblank timestamp ringbuffer. */
-#define vblanktimestamp(dev, crtc, count) \
-   ((dev)-vblank[crtc].time[(count) % DRM_VBLANKTIME_RBSIZE])
+#define vblanktimestamp(dev, pipe, count) \
+   ((dev)-vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE])
 
 /* Retry timestamp calculation up to 3 times to satisfy
  * drm_timestamp_precision before giving up.
@@ -57,7 +57,7 @@
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100
 
 static bool
-drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
+drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
  struct timeval *tvblank, unsigned flags);
 
 static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
@@ -77,7 +77,7 @@ module_param_named(timestamp_monotonic, 
drm_timestamp_monotonic, int, 0600);
 /**
  * drm_update_vblank_count - update the master vblank counter
  * @dev: DRM device
- * @crtc: counter to update
+ * @pipe: counter to update
  *
  * Call back into the driver to update the appropriate vblank counter
  * (specified by @crtc).  Deal with wraparound, if it occurred, and
@@ -90,9 +90,9 @@ module_param_named(timestamp_monotonic, 
drm_timestamp_monotonic, int, 0600);
  * Note: caller must hold dev-vbl_lock since this reads  writes
  * device vblank fields.
  */
-static void drm_update_vblank_count(struct drm_device *dev, int crtc)
+static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe)
 {
-   struct drm_vblank_crtc *vblank = dev-vblank[crtc];
+   struct drm_vblank_crtc *vblank = dev-vblank[pipe];
u32 cur_vblank, diff, tslot;
bool rc;
struct timeval t_vblank;
@@ -110,21 +110,21 @@ static void drm_update_vblank_count(struct drm_device 
*dev, int crtc)
 * corresponding vblank timestamp.
 */
do {
-   cur_vblank = dev-driver-get_vblank_counter(dev, crtc);
-   rc = drm_get_last_vbltimestamp(dev, crtc, t_vblank, 0);
-   } while (cur_vblank != dev-driver-get_vblank_counter(dev, crtc));
+   cur_vblank = dev-driver-get_vblank_counter(dev, pipe);
+   rc = drm_get_last_vbltimestamp(dev, pipe, t_vblank, 0);
+   } while (cur_vblank != dev-driver-get_vblank_counter(dev, pipe));
 
/* Deal with counter wrap */
diff = cur_vblank - vblank-last;
if (cur_vblank  vblank-last) {
diff += dev-max_vblank_count;
 
-   DRM_DEBUG(last_vblank[%d]=0x%x, cur_vblank=0x%x = 
diff=0x%x\n,
- crtc, vblank-last, cur_vblank, diff);
+   DRM_DEBUG(last_vblank[%u]=0x%x, cur_vblank=0x%x = 
diff=0x%x\n,
+ pipe, vblank-last, cur_vblank, diff);
}
 
-   DRM_DEBUG(updating vblank count on crtc %d, missed %d\n,
- crtc, diff);
+   DRM_DEBUG(updating vblank count on crtc %u, missed %d\n,
+ pipe, diff);
 
if (diff == 0)
return;
@@ -135,7 +135,7 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
 */
if (rc) {
tslot = atomic_read(vblank-count) + diff;
-   vblanktimestamp(dev, crtc, tslot) = t_vblank;
+   vblanktimestamp(dev, pipe, tslot) = t_vblank;
}
 
smp_mb__before_atomic();
@@ -149,9 +149,9 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
  * are preserved, even if there are any spurious vblank irq's after
  * disable.
  */
-static void vblank_disable_and_save(struct drm_device *dev, int crtc)
+static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 {
-   struct drm_vblank_crtc *vblank = dev-vblank[crtc];
+   struct drm_vblank_crtc *vblank = dev-vblank[pipe];
unsigned long irqflags;
u32 vblcount;
s64 diff_ns;
@@ -179,13 +179,13 @@ static void vblank_disable_and_save(struct drm_device 
*dev, int crtc)
 * vblank interrupt is disabled.
 */
if (!vblank-enabled 
-   drm_get_last_vbltimestamp(dev, crtc, tvblank, 0)) {
-   drm_update_vblank_count(dev, crtc);
+   drm_get_last_vbltimestamp(dev, pipe, tvblank

[PATCH 10/13] drm/irq: Add drm_crtc_vblank_count_and_time()

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

This function is the KMS native variant of drm_vblank_count_and_time().
It takes a struct drm_crtc * instead of a struct drm_device * and an
index of the CRTC.

Eventually the goal is to access vblank data through the CRTC only so
that the per-CRTC data can be moved to struct drm_crtc.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/drm_irq.c | 23 +++
 include/drm/drmP.h|  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 9c32a1d7aec2..b34900a8e172 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -877,6 +877,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_count);
  * vblank events since the system was booted, including lost events due to
  * modesetting activity. Returns corresponding system timestamp of the time
  * of the vblank interval that corresponds to the current vblank counter value.
+ *
+ * This is the legacy version of drm_crtc_vblank_count_and_time().
  */
 u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  struct timeval *vblanktime)
@@ -902,6 +904,27 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, 
unsigned int pipe,
 }
 EXPORT_SYMBOL(drm_vblank_count_and_time);
 
+/**
+ * drm_crtc_vblank_count_and_time - retrieve cooked vblank counter value
+ * and the system timestamp corresponding to that vblank counter value
+ * @crtc: which counter to retrieve
+ * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
+ *
+ * Fetches the cooked vblank count value that represents the number of
+ * vblank events since the system was booted, including lost events due to
+ * modesetting activity. Returns corresponding system timestamp of the time
+ * of the vblank interval that corresponds to the current vblank counter value.
+ *
+ * This is the native KMS version of drm_vblank_count_and_time().
+ */
+u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+  struct timeval *vblanktime)
+{
+   return drm_vblank_count_and_time(crtc-dev, drm_crtc_index(crtc),
+vblanktime);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
+
 static void send_vblank_event(struct drm_device *dev,
struct drm_pending_vblank_event *e,
unsigned long seq, struct timeval *now)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index de3d3f7091fb..7baef1dff5f3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1072,6 +1072,8 @@ extern u32 drm_vblank_count(struct drm_device *dev, int 
pipe);
 extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
 extern u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
 struct timeval *vblanktime);
+extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ struct timeval *vblanktime);
 extern void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
  struct drm_pending_vblank_event *e);
 extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/13] drm/sti: Store correct CRTC index in events

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

A negative pipe causes a special case to be triggered for drivers that
don't have proper VBLANK support. STi does support VBLANKs, so there is
no need for the fallback code.

Cc: Benjamin Gaignard benjamin.gaign...@linaro.org
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/sti/sti_drm_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_drm_crtc.c 
b/drivers/gpu/drm/sti/sti_drm_crtc.c
index e6f6ef7c4866..13bfe7bcb167 100644
--- a/drivers/gpu/drm/sti/sti_drm_crtc.c
+++ b/drivers/gpu/drm/sti/sti_drm_crtc.c
@@ -332,7 +332,7 @@ int sti_drm_crtc_vblank_cb(struct notifier_block *nb,
 
spin_lock_irqsave(drm_dev-event_lock, flags);
if (compo-mixer[*crtc]-pending_event) {
-   drm_send_vblank_event(drm_dev, -1,
+   drm_send_vblank_event(drm_dev, *crtc,
compo-mixer[*crtc]-pending_event);
drm_vblank_put(drm_dev, *crtc);
compo-mixer[*crtc]-pending_event = NULL;
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/13] drm/bochs: Store correct CRTC index in events

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Previously a negative pipe caused a special case to be triggered for
drivers that didn't have proper VBLANK support. The trigger for this
special case is now independent of the pipe, so the correct CRTC index
can now be stored in events.

Cc: Gerd Hoffmann kra...@redhat.com
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/bochs/bochs_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 26bcd03a8cb6..c219c1de3722 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -113,13 +113,14 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
struct bochs_device *bochs =
container_of(crtc, struct bochs_device, crtc);
struct drm_framebuffer *old_fb = crtc-primary-fb;
+   unsigned int pipe = drm_crtc_index(crtc);
unsigned long irqflags;
 
crtc-primary-fb = fb;
bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
if (event) {
spin_lock_irqsave(bochs-dev-event_lock, irqflags);
-   drm_send_vblank_event(bochs-dev, -1, event);
+   drm_send_vblank_event(bochs-dev, pipe, event);
spin_unlock_irqrestore(bochs-dev-event_lock, irqflags);
}
return 0;
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/13] drm/rockchip: Store correct CRTC index in events

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

A negative pipe causes a special case to be triggered for drivers that
don't have proper VBLANK support. Rockchip does support VBLANKs, so
there is no need for the fallback code.

Cc: Mark Yao mark@rock-chips.com
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index e7ca25b3fb38..999616d216ac 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -958,7 +958,7 @@ static void vop_win_state_complete(struct vop_win *vop_win,
 
if (state-event) {
spin_lock_irqsave(drm-event_lock, flags);
-   drm_send_vblank_event(drm, -1, state-event);
+   drm_send_vblank_event(drm, vop-pipe, state-event);
spin_unlock_irqrestore(drm-event_lock, flags);
}
 
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/13] drm/imx: Store correct CRTC index in events

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

A negative pipe causes a special case to be triggered for drivers that
don't have proper VBLANK support. i.MX does support VBLANKs, so there is
no need for the fallback code.

Cc: Philipp Zabel p.za...@pengutronix.de
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index ebee59cb96d8..df3e8b6101b3 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -209,7 +209,8 @@ static void ipu_crtc_handle_pageflip(struct ipu_crtc 
*ipu_crtc)
 
spin_lock_irqsave(drm-event_lock, flags);
if (ipu_crtc-page_flip_event)
-   drm_send_vblank_event(drm, -1, ipu_crtc-page_flip_event);
+   drm_send_vblank_event(drm, imx_drm_crtc_id(ipu_crtc-imx_crtc),
+ ipu_crtc-page_flip_event);
ipu_crtc-page_flip_event = NULL;
imx_drm_crtc_vblank_put(ipu_crtc-imx_crtc);
spin_unlock_irqrestore(drm-event_lock, flags);
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/13] drm/irq: Check for valid VBLANK before dereference

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

When accessing the array of per-CRTC VBLANK structures we must always
check that the index into the array is valid before dereferencing to
avoid crashing.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/drm_irq.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index a24658162284..cb207e047505 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1070,10 +1070,10 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 {
struct drm_vblank_crtc *vblank = dev-vblank[crtc];
 
-   if (WARN_ON(atomic_read(vblank-refcount) == 0))
+   if (WARN_ON(crtc = dev-num_crtcs))
return;
 
-   if (WARN_ON(crtc = dev-num_crtcs))
+   if (WARN_ON(atomic_read(vblank-refcount) == 0))
return;
 
/* Last user schedules interrupt disable */
@@ -1356,6 +1356,9 @@ void drm_vblank_post_modeset(struct drm_device *dev, int 
crtc)
if (!dev-num_crtcs)
return;
 
+   if (WARN_ON(crtc = dev-num_crtcs))
+   return;
+
if (vblank-inmodeset) {
spin_lock_irqsave(dev-vbl_lock, irqflags);
dev-vblank_disable_allowed = true;
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/13] drm/exynos: Store correct CRTC index in events

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

A negative pipe causes a special case to be triggered for drivers that
don't have proper VBLANK support. Exynos does support VBLANKs, so there
is no need for the fallback code.

Cc: Inki Dae inki@samsung.com
Cc: Joonyoung Shim jy0922.s...@samsung.com
Cc: Seung-Woo Kim sw0312@samsung.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 7615bb1b76de..f20fa537945c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -537,7 +537,7 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device 
*dev, int pipe)
continue;
 
list_del(e-base.link);
-   drm_send_vblank_event(dev, -1, e);
+   drm_send_vblank_event(dev, pipe, e);
drm_vblank_put(dev, pipe);
atomic_set(exynos_crtc-pending_flip, 0);
wake_up(exynos_crtc-pending_flip_queue);
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/13] drm/irq: Document return values more consistently

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

Some of the functions are documented inconsistently. Add Returns:
sections where missing and use consistent style to describe the return
value.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/drm_irq.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b34900a8e172..ef5d993f06ee 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -991,6 +991,9 @@ EXPORT_SYMBOL(drm_crtc_send_vblank_event);
  * drm_vblank_enable - enable the vblank interrupt on a CRTC
  * @dev: DRM device
  * @pipe: CRTC index
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
  */
 static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 {
@@ -1035,7 +1038,7 @@ static int drm_vblank_enable(struct drm_device *dev, 
unsigned int pipe)
  * This is the legacy version of drm_crtc_vblank_get().
  *
  * Returns:
- * Zero on success, nonzero on failure.
+ * Zero on success or a negative error code on failure.
  */
 int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 {
@@ -1072,7 +1075,7 @@ EXPORT_SYMBOL(drm_vblank_get);
  * This is the native kms version of drm_vblank_off().
  *
  * Returns:
- * Zero on success, nonzero on failure.
+ * Zero on success or a negative error code on failure.
  */
 int drm_crtc_vblank_get(struct drm_crtc *crtc)
 {
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/13] drm/irq: Expel legacy API

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

These legacy functions all operate on the struct drm_device * and an
index to the CRTC that they should access. This is bad because it
requires keeping track of a global data structures to resolve the index
to CRTC object lookup. In order to get rid of this global data new APIs
have been introduced that operate directly on these objects. Currently
the new functions still operate on the old data, but the goal is to
eventually move that data into struct drm_crtc. In order to start
conversion of drivers to the new API, move the old API away.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/Makefile |2 +-
 drivers/gpu/drm/drm_internal.h   |2 +
 drivers/gpu/drm/drm_irq.c| 1121 +
 drivers/gpu/drm/drm_irq_legacy.c | 1144 ++
 4 files changed, 1165 insertions(+), 1104 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_irq_legacy.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 66e40398b3d3..c0d93beda612 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -5,7 +5,7 @@
 ccflags-y := -Iinclude/drm
 
 drm-y   := drm_auth.o drm_bufs.o drm_cache.o \
-   drm_context.o drm_dma.o \
+   drm_context.o drm_dma.o drm_irq_legacy.o \
drm_fops.o drm_gem.o drm_ioctl.o drm_irq.o \
drm_lock.o drm_memory.o drm_drv.o drm_vm.o \
drm_agpsupport.o drm_scatter.o drm_pci.o \
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 12a61d706827..61d83b581c8b 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -22,7 +22,9 @@
  */
 
 /* drm_irq.c */
+extern unsigned int drm_timestamp_precision;
 extern unsigned int drm_timestamp_monotonic;
+extern int drm_vblank_offdelay;
 
 /* drm_fops.c */
 extern struct mutex drm_global_mutex;
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index ef5d993f06ee..37b536c57cd2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -32,15 +32,13 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
-#include drm/drmP.h
-#include drm_trace.h
-#include drm_internal.h
-
+#include linux/export.h
 #include linux/interrupt.h   /* For task queue support */
 #include linux/slab.h
 
-#include linux/vgaarb.h
-#include linux/export.h
+#include drm/drmP.h
+
+#include drm_trace.h
 
 /* Access macro for slots in vblank timestamp ringbuffer. */
 #define vblanktimestamp(dev, pipe, count) \
@@ -51,16 +49,7 @@
  */
 #define DRM_TIMESTAMP_MAXRETRIES 3
 
-/* Threshold in nanoseconds for detection of redundant
- * vblank irq in drm_handle_vblank(). 1 msec should be ok.
- */
-#define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100
-
-static bool
-drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
- struct timeval *tvblank, unsigned flags);
-
-static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
+unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 
 /*
  * Default to use monotonic timestamps for wait-for-vblank and page-flip
@@ -68,492 +57,13 @@ static unsigned int drm_timestamp_precision = 20;  /* 
Default to 20 usecs. */
  */
 unsigned int drm_timestamp_monotonic = 1;
 
-static int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */
+int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */
 
 module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
 module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 
0600);
 module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
 
 /**
- * drm_update_vblank_count - update the master vblank counter
- * @dev: DRM device
- * @pipe: counter to update
- *
- * Call back into the driver to update the appropriate vblank counter
- * (specified by @crtc).  Deal with wraparound, if it occurred, and
- * update the last read value so we can deal with wraparound on the next
- * call if necessary.
- *
- * Only necessary when going from off-on, to account for frames we
- * didn't get an interrupt for.
- *
- * Note: caller must hold dev-vbl_lock since this reads  writes
- * device vblank fields.
- */
-static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe)
-{
-   struct drm_vblank_crtc *vblank = dev-vblank[pipe];
-   u32 cur_vblank, diff, tslot;
-   bool rc;
-   struct timeval t_vblank;
-
-   /*
-* Interrupts were disabled prior to this call, so deal with counter
-* wrap if needed.
-* NOTE!  It's possible we lost a full dev-max_vblank_count events
-* here if the register is small or we had vblank interrupts off for
-* a long time.
-*
-* We repeat the hardware vblank counter  timestamp query until
-* we get consistent results. This to prevent races between gpu
-* updating its hardware counter

Re: drm: exynos: mixer: fix using usleep() in atomic context

[Thierry Reding]

On Mon, Dec 01, 2014 at 05:16:17PM +0100, Tobias Jakobi wrote:
 On 2014-12-01 16:54, Thierry Reding wrote:
 On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjak...@math.uni-bielefeld.de
 wrote:
 From: Tomasz Stanislawski t.stanisl...@samsung.com
 
 This patch fixes calling usleep_range() after taking reg_slock
 using spin_lock_irqsave(). The mdelay() is used instead.
 Waiting in atomic context is not the best idea in general.
 Hopefully, waiting occurs only when Video Processor fails
 to reset correctly.
 
 Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
 b/drivers/gpu/drm/exynos/exynos_mixer.c
 index a41c84e..cc7 100644
 --- a/drivers/gpu/drm/exynos/exynos_mixer.c
 +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
 @@ -632,7 +632,7 @@ static void vp_win_reset(struct mixer_context *ctx)
 /* waiting until VP_SRESET_PROCESSING is 0 */
 if (~vp_reg_read(res, VP_SRESET)  VP_SRESET_PROCESSING)
 break;
 -   usleep_range(1, 12000);
 +   mdelay(10);
 }
 WARN(tries == 0, failed to reset Video Processor\n);
  }
 
 I can't see a reason why you would need to hold the lock around this
 code. Perhaps a better way to fix this would be to drop the lock before
 calling vp_win_reset()?
 
 Thierry
 
 Hmm, I'm pretty new to spinlocks (only have worked with the usual mutex
 stuff in userspace), but wouldn't that mean that it is then possible for
 mixer_win_reset to execute while a (previous) vp_win_reset is still running?

Indeed it would. I didn't look properly. Looking more closely it seems
the call stack for this looks something like:

vp_win_reset()
mixer_win_reset()
mixer_poweron()
mixer_dpms()
exynos_drm_crtc_dpms()

Which can then be called from two places:

exynos_drm_crtc_commit()
drm_crtc_helper_set_mode()
drm_crtc_helper_set_config()

drm_helper_connector_dpms()
drm_crtc_helper_set_config()

drm_crtc_helper_set_config() itself must be called with the all modeset
locks held, so I don't see a way how vp_win_reset() could be called
concurrently.

Anyway, even if you're still concerned about concurrent accesses to the
register you'd better lock this section with a mutex to avoid excessive
spinning. In fact I think a better option would be to extend the mutex
in mixer_poweron() to encompass the whole function. This currently looks
broken because one process could go to sleep in pm_runtime_get_sync() or
clk_prepare_enable() and another process start running mixer_poweron()
concurrently, getting to the second mutex_lock() sooner than the first
process. So the lock being dropped between checking for ctx-powered and
setting it doesn't actually prevent a race.

Then again, nobody seems to have cared so far...

Thierry


pgps2444GZvNK.pgp
Description: PGP signature

Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Thierry Reding]

On Sun, Dec 14, 2014 at 02:45:36PM +0200, Laurent Pinchart wrote:
 Hi Marek,
 
 Thank you for the patch.
 
 On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
  This patch introduces IOMMU_OF_DECLARE-based initialization to the
  driver, which replaces subsys_initcall-based procedure.
  exynos_iommu_of_setup ensures that each sysmmu controller is probed
  before its master device.
  
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
   drivers/iommu/exynos-iommu.c | 28 +++-
   1 file changed, 27 insertions(+), 1 deletion(-)
  
  diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
  index cd28dc09db39..88f9afe641a0 100644
  --- a/drivers/iommu/exynos-iommu.c
  +++ b/drivers/iommu/exynos-iommu.c
 
 [snip]
 
  @@ -1125,4 +1134,21 @@ err_reg_driver:
  kmem_cache_destroy(lv2table_kmem_cache);
  return ret;
   }
  -subsys_initcall(exynos_iommu_init);
  +
  +static int __init exynos_iommu_of_setup(struct device_node *np)
  +{
  +   struct platform_device *pdev;
  +
  +   if (!init_done)
  +   exynos_iommu_init();
  +
  +   pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
  +   if (IS_ERR(pdev))
  +   return PTR_ERR(pdev);
 
 If we end up having to create the IOMMU platform devices from within the 
 drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a 
 workaround 
 to me. I wonder whether it wouldn't then be better to let the driver core 
 instantiate the IOMMU platform device from DT as for all other devices, and 
 use device notifiers to defer probe of the bus masters until the required 
 IOMMU(s) are registered.

Notifiers don't work very well for this. Notifier blocks are supposed to
return a very limited number of values, so sneaking in a -EPROBE_DEFER
isn't likely to work out very well.

This was in fact one of Hiroshi's proposals over a year ago and got
refused because of those reasons. The next solution was to introduce a
function, not very much unlike the of_iommu_configure() that would be
called in the core prior to calling into the driver's -probe() callback
so that it could handle this at probe time (as opposed to device
creation time). That way the core can easily defer probe if the IOMMU is
not there yet. At the same time it can simply use the driver model
without requiring per-architecture hacks or workarounds.

Note that there is really no need for any of this configuration or
initialization to happen at device creation time. Drivers won't be able
to use the IOMMU or DMA APIs until their .probe(), so handling this any
earlier is completely unnecessary.

Thierry


pgpi5R9GeakFb.pgp
Description: PGP signature

Re: [PATCH 2/3] drm/panel: add s6e63j0x03 LCD Panel driver

[Thierry Reding]

On Fri, Nov 28, 2014 at 08:39:50PM +0900, Inki Dae wrote:
 This patch adds MIPI-DSI based S6E63J0X03 AMOLED LCD Panel driver
 which uses mipi_dsi bus to communicate with Panel.

s/Panel/panel/. Also I prefer to have more information in the commit
message than this. Physical size and resolution would be good. Also a
small comment as to which device this panel can be found in would be
good.

 +config DRM_PANEL_S6E63J0X03
 + tristate S6E63J0X03 DSI video mode panel
 + depends on OF
 + select DRM_MIPI_DSI
 + select VIDEOMODE_HELPERS
 +
  endmenu
 diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
 index 8b92921..7f36dc2 100644
 --- a/drivers/gpu/drm/panel/Makefile
 +++ b/drivers/gpu/drm/panel/Makefile
 @@ -1,3 +1,4 @@
  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
 +obj-$(CONFIG_DRM_PANEL_S6E63J0X03) += panel-s6e63j0x03.o
 diff --git a/drivers/gpu/drm/panel/panel-s6e63j0x03.c 
 b/drivers/gpu/drm/panel/panel-s6e63j0x03.c
[...]
 +struct s6e63j0x03 {
 + struct device *dev;
 + struct drm_panel panel;
 + struct backlight_device *bl_dev;
 +
 + struct regulator_bulk_data supplies[2];
 + struct gpio_desc *reset_gpio;
 + u32 power_on_delay;
 + u32 power_off_delay;
 + u32 reset_delay;
 + u32 init_delay;
 + bool flip_horizontal;
 + bool flip_vertical;
 + struct videomode vm;

Specifying the video mode in DT should be optional and only used to
override the default mode provided by the driver.

 + u32 width_mm;
 + u32 height_mm;

There's no need for these to be sized types. unsigned int will do just
fine.

 + int power;

What's the difference between this and bl_dev-properties.power?

 +
 + /* This field is tested by functions directly accessing DSI bus before
 +  * transfer, transfer is skipped if it is set. In case of transfer
 +  * failure or unexpected response the field is set to error value.
 +  * Such construct allows to eliminate many checks in higher level
 +  * functions.
 +  */

The format of this comment is wrong, it should be:

/*
 * ...
 */

 + int error;
 +};
 +
 +static const unsigned char GAMMA_10[] = {
...

These array names should not be all-caps.

 +static const unsigned char *gamma_tbl[MAX_GAMMA_CNT] = {
 + GAMMA_10,
 + GAMMA_30,
 + GAMMA_60,
 + GAMMA_90,
 + GAMMA_120,
 + GAMMA_150,
 + GAMMA_200,
 + GAMMA_200,
 + GAMMA_240,
 + GAMMA_300,
 + GAMMA_300
 +};

Why not just do this using a two-dimensional array?

 +
 +static inline struct s6e63j0x03 *panel_to_s6e63j0x03(struct drm_panel *panel)
 +{
 + return container_of(panel, struct s6e63j0x03, panel);
 +}
 +
 +static int s6e63j0x03_clear_error(struct s6e63j0x03 *ctx)
 +{
 + int ret = ctx-error;
 +
 + ctx-error = 0;
 + return ret;
 +}

The only place where you ever call this ignores the return value.

 +
 +static void s6e63j0x03_dcs_write(struct s6e63j0x03 *ctx, const void *data, 
 size_t len)
 +{
 + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx-dev);
 + int ret;
 +
 + if (ctx-error  0)
 + return;
 +
 + ret = mipi_dsi_dcs_write(dsi, data, len);
 + if (ret  0) {
 + dev_err(ctx-dev, error %d writing dcs seq: %*ph\n, ret, len,
 + data);
 + ctx-error = ret;
 + }
 +}

The only reason for these wrappers seems to be the ctx-error handling.
But that error handling is flawed because it silently ignores errors.
The only location where you check for errors is in .prepare(), but you
only do so after all commands have been executed, so you loose all
context about what exactly went wrong.

I'd prefer if the driver just did straightforward checking of all
commands that could possibly fail.

 +static void s6e63j0x03_set_maximum_return_packet_size(struct s6e63j0x03 *ctx,
 + unsigned int size)
 +{
 + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx-dev);
 + const struct mipi_dsi_host_ops *ops = dsi-host-ops;
 +
 + if (ops  ops-transfer) {
 + unsigned char buf[] = {size, 0};
 + struct mipi_dsi_msg msg = {
 + .channel = dsi-channel,
 + .type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
 + .tx_len = sizeof(buf),
 + .tx_buf = buf
 + };
 + int ret;
 +
 + ret = ops-transfer(dsi-host, msg);
 + if (ret  0) {
 + dev_err(ctx-dev, failed to transfer message.\n);
 + ctx-error = ret;
 + }
 + }
 +}

Use mipi_dsi_set_maximum_return_packet_size().

 +static void s6e63j0x03_set_sequence(struct s6e63j0x03 *ctx)

Can you find a better name for this? Also this is really only called by
-prepare() below, so why not just 

Re: drm: exynos: mixer: fix using usleep() in atomic context

[Thierry Reding]

On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjak...@math.uni-bielefeld.de wrote:
 From: Tomasz Stanislawski t.stanisl...@samsung.com
 
 This patch fixes calling usleep_range() after taking reg_slock
 using spin_lock_irqsave(). The mdelay() is used instead.
 Waiting in atomic context is not the best idea in general.
 Hopefully, waiting occurs only when Video Processor fails
 to reset correctly.
 
 Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
 b/drivers/gpu/drm/exynos/exynos_mixer.c
 index a41c84e..cc7 100644
 --- a/drivers/gpu/drm/exynos/exynos_mixer.c
 +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
 @@ -632,7 +632,7 @@ static void vp_win_reset(struct mixer_context *ctx)
   /* waiting until VP_SRESET_PROCESSING is 0 */
   if (~vp_reg_read(res, VP_SRESET)  VP_SRESET_PROCESSING)
   break;
 - usleep_range(1, 12000);
 + mdelay(10);
   }
   WARN(tries == 0, failed to reset Video Processor\n);
  }

I can't see a reason why you would need to hold the lock around this
code. Perhaps a better way to fix this would be to drop the lock before
calling vp_win_reset()?

Thierry


pgp5SPPtXnkFq.pgp
Description: PGP signature

Re: [PATCH V2 RESEND] arm: dts: Exynos5: Use pmu_system_controller phandle for dp phy

[Thierry Reding]

On Mon, Nov 24, 2014 at 11:11:23AM +0530, Vivek Gautam wrote:
 DP PHY now require pmu-system-controller to handle PMU register
 to control PHY's power isolation. Adding the same to dp-phy
 node.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 Reviewed-by: Jingoo Han jg1@samsung.com
 Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
 Cc: Kukjin Kim kg...@kernel.org
 ---
  arch/arm/boot/dts/exynos5250.dtsi |2 +-
  arch/arm/boot/dts/exynos5420.dtsi |4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/arch/arm/boot/dts/exynos5250.dtsi 
 b/arch/arm/boot/dts/exynos5250.dtsi
 index 0a588b4..bebd099 100644
 --- a/arch/arm/boot/dts/exynos5250.dtsi
 +++ b/arch/arm/boot/dts/exynos5250.dtsi
 @@ -732,7 +732,7 @@
  
   dp_phy: video-phy@10040720 {
   compatible = samsung,exynos5250-dp-video-phy;
 - reg = 0x10040720 4;
 + samsung,pmu-syscon = pmu_system_controller;
   #phy-cells = 0;
   };
  
 diff --git a/arch/arm/boot/dts/exynos5420.dtsi 
 b/arch/arm/boot/dts/exynos5420.dtsi
 index 8617a03..1353a09 100644
 --- a/arch/arm/boot/dts/exynos5420.dtsi
 +++ b/arch/arm/boot/dts/exynos5420.dtsi
 @@ -503,8 +503,8 @@
   };
  
   dp_phy: video-phy@10040728 {
 - compatible = samsung,exynos5250-dp-video-phy;
 - reg = 0x10040728 4;
 + compatible = samsung,exynos5420-dp-video-phy;
 + samsung,pmu-syscon = pmu_system_controller;
   #phy-cells = 0;
   };
  

It seems like these nodes have been in the Linux tree since 3.12 and
3.17, respectively and these changes break backwards-compatibility. Has
anyone thought about the possible consequences?

Although, looking more closely it seems like this isn't the first time
that backwards-compatibility was broken in these files, so perhaps
nobody cares...

Thierry


pgpeEV_fqr4kQ.pgp
Description: PGP signature

Re: [PATCH V2 RESEND] arm: dts: Exynos5: Use pmu_system_controller phandle for dp phy

[Thierry Reding]

On Mon, Nov 24, 2014 at 04:17:18PM +0530, Vivek Gautam wrote:
 Hi,
 
 
 On Mon, Nov 24, 2014 at 4:02 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Mon, Nov 24, 2014 at 11:11:23AM +0530, Vivek Gautam wrote:
  DP PHY now require pmu-system-controller to handle PMU register
  to control PHY's power isolation. Adding the same to dp-phy
  node.
 
  Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
  Reviewed-by: Jingoo Han jg1@samsung.com
  Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
  Cc: Kukjin Kim kg...@kernel.org
  ---
   arch/arm/boot/dts/exynos5250.dtsi |2 +-
   arch/arm/boot/dts/exynos5420.dtsi |4 ++--
   2 files changed, 3 insertions(+), 3 deletions(-)
 
  diff --git a/arch/arm/boot/dts/exynos5250.dtsi 
  b/arch/arm/boot/dts/exynos5250.dtsi
  index 0a588b4..bebd099 100644
  --- a/arch/arm/boot/dts/exynos5250.dtsi
  +++ b/arch/arm/boot/dts/exynos5250.dtsi
  @@ -732,7 +732,7 @@
 
dp_phy: video-phy@10040720 {
compatible = samsung,exynos5250-dp-video-phy;
  - reg = 0x10040720 4;
  + samsung,pmu-syscon = pmu_system_controller;
#phy-cells = 0;
};
 
  diff --git a/arch/arm/boot/dts/exynos5420.dtsi 
  b/arch/arm/boot/dts/exynos5420.dtsi
  index 8617a03..1353a09 100644
  --- a/arch/arm/boot/dts/exynos5420.dtsi
  +++ b/arch/arm/boot/dts/exynos5420.dtsi
  @@ -503,8 +503,8 @@
};
 
dp_phy: video-phy@10040728 {
  - compatible = samsung,exynos5250-dp-video-phy;
  - reg = 0x10040728 4;
  + compatible = samsung,exynos5420-dp-video-phy;
  + samsung,pmu-syscon = pmu_system_controller;
#phy-cells = 0;
};
 
 
  It seems like these nodes have been in the Linux tree since 3.12 and
  3.17, respectively and these changes break backwards-compatibility. Has
  anyone thought about the possible consequences?
 
 Sorry for my ignorance, but i have a doubt.
 If the bindings and device node both are being changed in the same kernel
 version (as fixes),
 so that the stable will have both; then the only scenerio of backward
 compatibility comes when kernel is upgraded but not dtbs.

Correct.

 Does such upgradation makes sense for distros ?

Yes. Back at the time a decision was made that device trees need to be
stable ABI because eventually they'd be shipped with the device rather
than the distribution. As such it may not at all be possible to update
them (they could be in some sort of ROM).

For that reason new kernels need to keep working with old DTBs unless an
argument can be made that would justify breaking things. I don't think I
have ever seen anyone win such an argument. One of the rare exceptions
that I know of is if you can prove that a given hardware block has never
been used in an upstream kernel, then changing the DTB in backwards-
incompatible ways would be okay because you wouldn't be breaking things
for existing users.

Thierry


pgpMb7BlOEiAw.pgp
Description: PGP signature

Re: [PATCH] drm/panel: simple: Add AUO B116XW03 panel support

[Thierry Reding]

On Mon, Sep 01, 2014 at 03:40:02PM +0530, Ajay Kumar wrote:
 The AUO B116XW03 is a 11.6 HD TFT LCD panel connecting to a LVDS
 interface and with an integrated LED backlight unit.
 
 This panel is used on the Samsung Chromebook(XE303C12).
 
 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
 ---
  .../devicetree/bindings/panel/auo,b116xw03.txt |7 ++
  drivers/gpu/drm/panel/panel-simple.c   |   25 
 
  2 files changed, 32 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/panel/auo,b116xw03.txt

Applied after adding the missing .bpc field in the panel descriptor. I
set it to 6 according to the datasheet.

Thierry


pgpExRCIQ_YtS.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
 On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
  On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
   That makes the entire thing a bit non-trivial, which is why I think it
   would be better as some generic helper. Which then gets embedded or
   instantiated for specific cases, like dtdrm_panel or dtdrm_bridge.
   Or maybe even acpidrm_bridge, who knows ;-)
  
  I worry a little about type safety. How will this generic helper know
  what registry to look in for? Or conversely, if all these resources are
  added to a single registry how do you know that they're of the correct
  type? Failing to ensure this could cause situations where you're asking
  for a panel and get a bridge in return because you've wrongly wired it
  up in device tree for example.
  
  But perhaps if both the registry and the device parts are turned into
  helpers we could still have a single core implementation and then
  instantiate that for each type, something roughly like this:
  
  struct registry {
  struct list_head list;
  struct mutex lock;
  };
  
  struct registry_record {
  struct list_head list;
  struct module *owner;
  struct kref *ref;
  
  struct device *dev;
  };
  
  int registry_add(struct registry *registry, struct registry_record 
  *record)
  {
  ...
  try_module_get(record-owner);
  ...
  }
  
  struct registry_record *registry_find_by_of_node(struct registry 
  *registry,
   struct device_node *np)
  {
  ...
  kref_get(...);
  ...
  }
  
  That way it should be possible to embed these into other structures,
  like so:
  
  struct drm_panel {
  struct registry_record record;
  ...
  };
  
  static struct registry drm_panels;
  
  int drm_panel_add(struct drm_panel *panel)
  {
  return registry_add(drm_panels, panel-record);
  }
  
  struct drm_panel *of_drm_panel_find(struct device_node *np)
  {
  struct registry_record *record;
  
  record = registry_find_by_of_node(drm_panels, np);
  
  return container_of(record, struct drm_panel, record);
  }
  
  Is that what you had in mind?
 
 Yeah I've thought that we should instantiate using macros even, so that we
 have per-type registries. So you'd smash the usual set of
 DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
 a (name, key, value) tripled. For the example here(of_drm_panel, struct
 device_node *, struct drm_panel *) or similar. I might be hand-waving over
 a few too many details though ;-)

Okay, I'll take a stab at this and see if I can convert DRM panel to it.

Thierry


pgpUw0Hut_eXQ.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
 On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
  On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
   On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
 On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org 
 wrote:
  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
* @driver_private: pointer to the bridge driver's internal 
  context
*/
   struct drm_bridge {
  - struct drm_device *dev;
  + struct device *dev;
 
  Please don't rename the -dev pointer into drm. Because _all_ the 
  other
  drm structures still call it -dev. Also, can't we use struct 
  device_node
  here like we do in the of helpers Russell added? See 7e435aad38083
 
 
  I think this is modeled after the naming in drm_panel, FWIW. 
  However,
  seems reasonable to keep the device_node instead.
 
 Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
 drm_crtc drop the struct device and go directly to a struct
 device_node. Since we don't really need the sturct device, the only
 thing we care about is the of_node. For added bonus wrap an #ifdef
 CONFIG_OF around all the various struct device_node in drm_foo.h.
 Should be all fairly simple to pull off with cocci.
 
 Thierry?

The struct device * is in DRM panel because there's nothing device tree
specific about the concept. Having a struct device_node * instead would
indicate that it can only be used with a device tree, whereas the
framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we
should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having
access to the proper device and its name. Also you get access to the
device_node * via dev-of_node anyway. I don't see any advantage in
switching to just a struct device_node *, only disadvantages.
   
   Well the idea is to make the lookup key specific, and conditional on
   #CONFIG_OF. If there's going to be another neat way to enumerate platform
   devices then I think we should add that, too. Or maybe we should have a
   void *platform_data or so.
   
   The reason I really don't want a struct device * in core drm structures is
   that two releases down the road people will have found tons of really
   great ways to abuse them and re-create a midlayer. DRM core really should
   only care about the sw objects and not be hw specific at all. Heck there's
   not even an requirement to have any piece of actual hw, you could write a
   completely fake drm driver (for e.g. testing like the new v4l driver).
   
   Tbh I wonder a bit why we even have this registery embedded into the core
   drm objects. Essentially the only thing you're doing is a list that maps
   some platform specific key onto some subsystem specific driver structure
   or fails the lookup. So instead of putting all these low-level details
   into drm core structures can't we just have a generic hashtable/list for
   this, plus some static inline helpers that cast the void * you get into
   the one you want?
   
   I also get the feeling that this really should be in the driver core (like
   the component helpers), and that we should think a lot harder about
   lifetimes and refcounting (see my other reply on that).
  
  Yes, that sounds very useful indeed. Also see my reply to yours. =)
 
 Just replying here with some of the irc discussions we've had. Since
 drm_bridge/panel isn't a core drm interface object exposed to userspace
 it's less critical. I still think that wasting a few brain cycles to have
 a clear separation between the abstract interface object and how to bind
 and unbind the pieces together is worthwhile, even though the cost when
 getting it wrong is much less severe than in the case of a mandatory piece
 of core infrastructure.
 
 I think in general the recent armsoc motivated drm infrastructure lacks a
 bit in attention to these details. E.g. the cma helpers are built into the
 drm.ko module, but clearly are auxilliary library code. So they should be
 pulled out and the headers clean up, so that we have a clear separation
 between core and helpers. Otherwise someone will sooner or later screw up
 and insert a helper depency into the core, and then we've started with the
 midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
 to have separate headers from the core modeset header drm_crtc.h.

DRM panel does have a separate header. It's still built into the core
DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o
entry in the makefile. At the time it didn't seem worth to add a
completely separate

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
 On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
  On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
   On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org 
wrote:
@@ -660,8 +662,11 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
- struct drm_device *dev;
+ struct device *dev;
   
Please don't rename the -dev pointer into drm. Because _all_ the 
other
drm structures still call it -dev. Also, can't we use struct 
device_node
here like we do in the of helpers Russell added? See 7e435aad38083
   
   
I think this is modeled after the naming in drm_panel, FWIW. However,
seems reasonable to keep the device_node instead.
   
Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
drm_crtc drop the struct device and go directly to a struct
device_node. Since we don't really need the sturct device, the only
thing we care about is the of_node. For added bonus wrap an #ifdef
CONFIG_OF around all the various struct device_node in drm_foo.h.
Should be all fairly simple to pull off with cocci.
   
Thierry?
   
   Looking at the of_drm_find_panel function I actually wonder how that
   works - the drm_panel doesn't really need to stick around afaics.
   After all panel_list is global so some other driver can unload.
   Russell's of support for possible_crtcs code works differently since
   it only looks at per-drm_device lists.
  
  I don't understand. Panels are global resources that get registered by
  some driver that isn't tied to the DRM device until attached. It can't
  be in a per-DRM device list, because it's external to the device.
  
  And yes, they can go away when a driver is unloaded, though it's not the
  typical use-case.
  
   This bridge code here though suffers from the same. So to me this
   looks rather fishy.
  
  Well, this version of the DRM bridge support was written to be close to
  DRM panel, so it isn't really surprising that it's similar =), but like
  I said, I don't really understand what you think is wrong with it.
 
 You have a mutex to protect the global list of bridges/panels. But if you
 do a lookup you don't grab a reference count on the panel, so the moment
 you drop the list mutex the panel/bridge can go away.
 
 Yes usually you don't unload a driver on a soc but soc isn't everything
 and designing new stuff to not be hotunplug save isn't great either.

Yeah, I certainly agree that adding proper reference counting would be a
good thing. I think perhaps we could just take a reference on the struct
device * to prevent it from disappearing.

Although perhaps I misunderstand what you mean by go away.

Thierry


pgpIpdWtkOLY6.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
 On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
  On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
   On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
 On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch 
 wrote:
  On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org 
  wrote:
  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
* @driver_private: pointer to the bridge driver's internal 
  context
*/
   struct drm_bridge {
  - struct drm_device *dev;
  + struct device *dev;
 
  Please don't rename the -dev pointer into drm. Because _all_ the 
  other
  drm structures still call it -dev. Also, can't we use struct 
  device_node
  here like we do in the of helpers Russell added? See 7e435aad38083
 
 
  I think this is modeled after the naming in drm_panel, FWIW. 
  However,
  seems reasonable to keep the device_node instead.
 
  Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like 
  with
  drm_crtc drop the struct device and go directly to a struct
  device_node. Since we don't really need the sturct device, the only
  thing we care about is the of_node. For added bonus wrap an #ifdef
  CONFIG_OF around all the various struct device_node in drm_foo.h.
  Should be all fairly simple to pull off with cocci.
 
  Thierry?
 
 Looking at the of_drm_find_panel function I actually wonder how that
 works - the drm_panel doesn't really need to stick around afaics.
 After all panel_list is global so some other driver can unload.
 Russell's of support for possible_crtcs code works differently since
 it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by
some driver that isn't tied to the DRM device until attached. It can't
be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the
typical use-case.

 This bridge code here though suffers from the same. So to me this
 looks rather fishy.

Well, this version of the DRM bridge support was written to be close to
DRM panel, so it isn't really surprising that it's similar =), but like
I said, I don't really understand what you think is wrong with it.
   
   You have a mutex to protect the global list of bridges/panels. But if you
   do a lookup you don't grab a reference count on the panel, so the moment
   you drop the list mutex the panel/bridge can go away.
   
   Yes usually you don't unload a driver on a soc but soc isn't everything
   and designing new stuff to not be hotunplug save isn't great either.
  
  Yeah, I certainly agree that adding proper reference counting would be a
  good thing. I think perhaps we could just take a reference on the struct
  device * to prevent it from disappearing.
  
  Although perhaps I misunderstand what you mean by go away.
 
 I meant the drm_panel/bridge could go away any second, since nothing
 prevents a module unload of the panel/bridge driver. So in theory you
 could get the unregister call right after you've done the lookup. Which
 means the bridge/panel pointer you've just returned is freed memory.

Ah yes, I see now.

 I think we nee try_get_module for the code and kref on the actual data
 structures.

Agreed, that should do the trick. We'd probably need some sort of logic
to also make operations return something like -ENODEV when the
underlying device has disappeared. I think David had introduced
something similar for DRM device not so long ago?

 That makes the entire thing a bit non-trivial, which is why I think it
 would be better as some generic helper. Which then gets embedded or
 instantiated for specific cases, like dtdrm_panel or dtdrm_bridge.
 Or maybe even acpidrm_bridge, who knows ;-)

I worry a little about type safety. How will this generic helper know
what registry to look in for? Or conversely, if all these resources are
added to a single registry how do you know that they're of the correct
type? Failing to ensure this could cause situations where you're asking
for a panel and get a bridge in return because you've wrongly wired it
up in device tree for example.

But perhaps if both the registry and the device parts are turned into
helpers we could still have a single core implementation and then
instantiate that for each type, something roughly like this:

struct registry {
struct list_head list;
struct mutex lock;
};

struct registry_record {
struct list_head list;
struct module *owner;
struct kref *ref;

struct device *dev

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
 On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
  On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
   On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
@@ -660,8 +662,11 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
- struct drm_device *dev;
+ struct device *dev;
   
Please don't rename the -dev pointer into drm. Because _all_ the other
drm structures still call it -dev. Also, can't we use struct 
device_node
here like we do in the of helpers Russell added? See 7e435aad38083
   
   
I think this is modeled after the naming in drm_panel, FWIW. However,
seems reasonable to keep the device_node instead.
   
   Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
   drm_crtc drop the struct device and go directly to a struct
   device_node. Since we don't really need the sturct device, the only
   thing we care about is the of_node. For added bonus wrap an #ifdef
   CONFIG_OF around all the various struct device_node in drm_foo.h.
   Should be all fairly simple to pull off with cocci.
   
   Thierry?
  
  The struct device * is in DRM panel because there's nothing device tree
  specific about the concept. Having a struct device_node * instead would
  indicate that it can only be used with a device tree, whereas the
  framework doesn't care the tiniest bit what type of device we have.
  
  While the trend clearly is to use more device tree, I don't think we
  should make it impossible for anybody else to use these frameworks.
  
  There are other advantages to keeping a struct device *, like having
  access to the proper device and its name. Also you get access to the
  device_node * via dev-of_node anyway. I don't see any advantage in
  switching to just a struct device_node *, only disadvantages.
 
 Well the idea is to make the lookup key specific, and conditional on
 #CONFIG_OF. If there's going to be another neat way to enumerate platform
 devices then I think we should add that, too. Or maybe we should have a
 void *platform_data or so.
 
 The reason I really don't want a struct device * in core drm structures is
 that two releases down the road people will have found tons of really
 great ways to abuse them and re-create a midlayer. DRM core really should
 only care about the sw objects and not be hw specific at all. Heck there's
 not even an requirement to have any piece of actual hw, you could write a
 completely fake drm driver (for e.g. testing like the new v4l driver).
 
 Tbh I wonder a bit why we even have this registery embedded into the core
 drm objects. Essentially the only thing you're doing is a list that maps
 some platform specific key onto some subsystem specific driver structure
 or fails the lookup. So instead of putting all these low-level details
 into drm core structures can't we just have a generic hashtable/list for
 this, plus some static inline helpers that cast the void * you get into
 the one you want?
 
 I also get the feeling that this really should be in the driver core (like
 the component helpers), and that we should think a lot harder about
 lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

Thierry


pgpi3cwpmOupr.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
 On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
* @driver_private: pointer to the bridge driver's internal context
*/
   struct drm_bridge {
  - struct drm_device *dev;
  + struct device *dev;
 
  Please don't rename the -dev pointer into drm. Because _all_ the other
  drm structures still call it -dev. Also, can't we use struct device_node
  here like we do in the of helpers Russell added? See 7e435aad38083
 
 
  I think this is modeled after the naming in drm_panel, FWIW. However,
  seems reasonable to keep the device_node instead.
 
 Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
 drm_crtc drop the struct device and go directly to a struct
 device_node. Since we don't really need the sturct device, the only
 thing we care about is the of_node. For added bonus wrap an #ifdef
 CONFIG_OF around all the various struct device_node in drm_foo.h.
 Should be all fairly simple to pull off with cocci.
 
 Thierry?

The struct device * is in DRM panel because there's nothing device tree
specific about the concept. Having a struct device_node * instead would
indicate that it can only be used with a device tree, whereas the
framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we
should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having
access to the proper device and its name. Also you get access to the
device_node * via dev-of_node anyway. I don't see any advantage in
switching to just a struct device_node *, only disadvantages.

Thierry


pgpSvaKUtJAL7.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
 On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
  On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
* @driver_private: pointer to the bridge driver's internal context
*/
   struct drm_bridge {
  - struct drm_device *dev;
  + struct device *dev;
 
  Please don't rename the -dev pointer into drm. Because _all_ the other
  drm structures still call it -dev. Also, can't we use struct device_node
  here like we do in the of helpers Russell added? See 7e435aad38083
 
 
  I think this is modeled after the naming in drm_panel, FWIW. However,
  seems reasonable to keep the device_node instead.
 
  Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
  drm_crtc drop the struct device and go directly to a struct
  device_node. Since we don't really need the sturct device, the only
  thing we care about is the of_node. For added bonus wrap an #ifdef
  CONFIG_OF around all the various struct device_node in drm_foo.h.
  Should be all fairly simple to pull off with cocci.
 
  Thierry?
 
 Looking at the of_drm_find_panel function I actually wonder how that
 works - the drm_panel doesn't really need to stick around afaics.
 After all panel_list is global so some other driver can unload.
 Russell's of support for possible_crtcs code works differently since
 it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by
some driver that isn't tied to the DRM device until attached. It can't
be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the
typical use-case.

 This bridge code here though suffers from the same. So to me this
 looks rather fishy.

Well, this version of the DRM bridge support was written to be close to
DRM panel, so it isn't really surprising that it's similar =), but like
I said, I don't really understand what you think is wrong with it.

 It doesn't help that we have drm_of.[hc] around but not all the of
 code is in there. Adding Russell too.

DRM panel and DRM bridge aren't just OF helpers. They can be used with
whatever type of device you want.

Thierry


pgpwbadwzK4yQ.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
 On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
  On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
[...]
  Hm, if you do this can you pls also update drm_panel accordingly? It
  shouldn't be a lot of fuzz and would make things around drm+dt more
  consistent.
  Are you talking about using struct device_node instead of struct device?
  I guess you have misplaced the comment under the wrong section!
 
 Yeah, that should have been one up ;-)

Like I said earlier, I don't think dropping struct device * in favour of
struct device_node * is a good idea.

  If you want to document drm_bridge then I recomment to sprinkle proper
  kerneldoc over drm_bridge.c and pull it all into the drm DocBook
  template. That way all the drm documentation is in one place. I've
  done that for drm_crtc.h in an unrelated patch series (but based upon
  a branch with your patch here included) and there's struct drm_bridge*
  in there. Hence why I've noticed.
  Can you send a link for that?
  And, is there any problem if the doc comes later?
 
 Since quite a while we've asked for the kerneldoc polish as part of
 each drm core patch series. It's just that drm_bridge/panel kinda have
 flown under the radar of the people usually asking for docs ;-)

FWIW, there's some kerneldoc in include/drm/drm_panel.h but I guess I
could write up something more complete and integrate it into DocBook.

Thierry


pgpJWY8ji2fm8.pgp
Description: PGP signature

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
 On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
  On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
   On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
  [...]
   Hm, if you do this can you pls also update drm_panel accordingly? It
   shouldn't be a lot of fuzz and would make things around drm+dt more
   consistent.
   Are you talking about using struct device_node instead of struct device?
   I guess you have misplaced the comment under the wrong section!
 
  Yeah, that should have been one up ;-)
 
  Like I said earlier, I don't think dropping struct device * in favour of
  struct device_node * is a good idea.
 I am not sure about drm_panel.
 But, I am not really doing anything with the struct device pointer in
 case of bridge.
 So, just wondering if it is really needed?

I think it's useful to have it just to send the right message. DRM panel
and DRM bridge aren't specific to device tree. They are completely
generic and can work with any type of device, whether it was
instantiated from the device tree or some other infrastructure. Dropping
struct device * will make it effectively useless on anything but DT. I
don't think we should strive for that, even if only DT-enabled platforms
currently use them.

Thierry


pgpWDDu8jRmCM.pgp
Description: PGP signature

[PATCH 02/15] drm/dsi: Constify mipi_dsi_msg

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

struct mipi_dsi_msg is a read-only structure, drivers should never need
to modify it. Make this explicit by making all references to the struct
const.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +-
 include/drm/drm_mipi_dsi.h  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 24741d8758e8..c5f3c76bfac3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1236,7 +1236,7 @@ static bool exynos_dsi_is_short_dsi_type(u8 type)
 }
 
 static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host,
-  struct mipi_dsi_msg *msg)
+   const struct mipi_dsi_msg *msg)
 {
struct exynos_dsi *dsi = host_to_dsi(host);
struct exynos_dsi_transfer xfer;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index ccc3869e137b..836cc2b677d0 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -56,7 +56,7 @@ struct mipi_dsi_host_ops {
int (*detach)(struct mipi_dsi_host *host,
  struct mipi_dsi_device *dsi);
ssize_t (*transfer)(struct mipi_dsi_host *host,
-   struct mipi_dsi_msg *msg);
+   const struct mipi_dsi_msg *msg);
 };
 
 /**
-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix vblank handling during dpms off

[Thierry Reding]

On Thu, Oct 09, 2014 at 09:52:58AM +0100, Russell King - ARM Linux wrote:
 On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote:
  But there might be another issue, which is that calls to  
  drm_vblank_get() will return -EINVAL if invoked between drm_blank_off  
  and drm_blank_on. Is this really the desired behavior? Can it at least  
  happen? If so, how are drivers supposed to react to this situation?
 
 I've not yet seen the commit which causes this problem, but I hope
 that drm_wait_vblank() isn't affected by this.  In current mainline,
 drm_vblank_get() is used inside drm_wait_vblank(), which is called as
 a result of userspace calling DRM_IOCTL_WAIT_VBLANK.
 
 So, what is the effect of this change on user applications making use
 of the vblank wait ioctl - and is that change intended?

There's no effect on user applications if the driver behaves properly.
As far as I can tell, every driver that calls drm_vblank_off() but not
drm_vblank_on() will break. You can easily test this by running libdrm
modetest -s ... -v, which instead of toggling between the test pattern
and an all-gray framebuffer will switch to the gray one once and then
hang.

I guess that was probably not intended, but according to the new rules
all these drivers have now become buggy. So before merging this patch I
think we need to fix existing drivers to avoid regressions.

Thierry


pgpbuEXgiTUPk.pgp
Description: PGP signature

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Tue, Oct 07, 2014 at 05:49:24PM +0300, Laurent Pinchart wrote:
 Hi Ajay,
 
 On Tuesday 07 October 2014 16:06:55 Ajay kumar wrote:
  On Tue, Oct 7, 2014 at 4:00 PM, Tomi Valkeinen wrote:
   On 20/09/14 14:22, Ajay kumar wrote:
   Well, I am okay with using video ports to describe the relationship
   between the encoder, bridge and the panel.
   But, its just that I need to make use of 2 functions when phandle
   does it using just one function ;)
   -panel_node = of_parse_phandle(dev-of_node, panel, 0)
   +   endpoint = of_graph_get_next_endpoint(dev-of_node, NULL);
   +   if (!endpoint)
   +   return -EPROBE_DEFER;
   +
   +   panel_node = of_graph_get_remote_port_parent(endpoint);
   +   if (!panel_node)
   +   return -EPROBE_DEFER;
   
   
   If nobody else has objections over using of_graph functions instead
   of phandles, I can respin this patchset by making use of video ports.
   
   The discussion did digress somewhat.
   
   As a clarification, I'm in no way nack'ing this series because it
   doesn't use the graphs for video connections. I don't see the simple
   phandle bindings used here as broken as such.
  
  Well, I am okay with any approach you guys decide on. I desperately want
  this to get this in since it has been floating around for quite sometime.
  The more we drag this, the more rework for me since the number of platforms
  using bridge support is increasing daily!
 
 I won't nack this patch either. I'm however concerned that we'll run straight 
 into the wall if we don't come up with an agreement on a standard way to 
 describe connections in DT for display devices, which is why I would prefer 
 the ps8622 bindings to use OF graph to describe connections.

I think there's not really an easy way out here. It's pretty bold trying
to come up with a common way to describe bridges when we have only a
single one (and a single use-case at that). The worst that can happen is
that we need to change the binding at some point, in which case we may
have to special-case early drivers, but I really don't think that's as
much of an issue as everybody seems to think.

This series has been floating around for months because we're being
overly prudent to accept a binding that /may/ turn out to not be generic
enough.

Thierry


pgpVFLkLDYrm9.pgp
Description: PGP signature

Re: Unable to boot mainline on snow chromebook since 3.15

[Thierry Reding]

On Mon, Sep 29, 2014 at 02:12:43PM +0100, Grant Likely wrote:
 On Mon, Sep 29, 2014 at 1:57 PM, Thierry Reding thierry.red...@gmail.com 
 wrote:
  Though there are two cases: one is to use simplefb as a means to have
  early boot messages on a graphical display (and optionally hand off to a
  real driver). The other is to use simplefb as the only framebuffer
  driver until a proper driver has been implemented. The latter would have
  the disadvantage of not allowing unused resources from being garbage
  collected at all. Then again, I don't think power consumption is going
  to be a very big issue on hardware where no proper display driver is
  available.
 
 When simplefb is the only framebuffer to get a platform working, it is
 reasonable to have a placeholder driver that grabs the resources and
 nothing else. When a real driver is implemented, and merged, the
 placeholder driver should drop compatibility with the device node at
 the same time.

You mean the device node for the real device should be compatible with
simplefb? One problem I see with that is that there may be multiple
dummy drivers for different pieces of hardware, all of them binding to
the simplefb compatible and conflicting.

Also this assumes that a device tree node exists for the device. One of
the reasons for using simplefb is so that you don't have to write that
device tree node and its binding yet.

Presumably, though, if the firmware already knows what resources are
needed and generate them at runtime it should be possible to come up
with a static device tree node, too.

Thierry


pgpv2On4Gx01K.pgp
Description: PGP signature

Re: Unable to boot mainline on snow chromebook since 3.15

[Thierry Reding]

On Wed, Sep 10, 2014 at 03:56:16PM +0100, Grant Likely wrote:
 On Wed, Sep 10, 2014 at 3:31 PM, Mark Brown broo...@kernel.org wrote:
  On Wed, Sep 10, 2014 at 06:06:46AM -0700, Olof Johansson wrote:
  On Mon, Sep 8, 2014 at 12:40 PM, Grant Likely grant.lik...@secretlab.ca 
  wrote:
 
   Well, lets see... We've got a real user complaining about a platform
   that used to work on mainline, and no longer does. The only loophole
   for ignoring breakage is if there nobody cares that it is broken. That
   currently isn't the case. So even though it's based on a patch that
   has DO NOT SUBMIT in large friendly letters on the front cover, it
   doesn't change the situation that mainline has a regression.
 
  Yeah, I'm with you on this Grant, it doesn't matter what the patch is
  labelled as.
 
  One way to deal with this could be to add a quirk at boot time --
  looking for the simplefb and if found, modifies the regulators to keep
  them on. That'd go in the kernel, not in firmware.
 
  Well, we should also be fixing simplefb to manage the resources it uses
  though that doesn't clean up after the broken DTs that are currently
  deployed.
 
  As well as the regulators we'll also need to fix the clocks.  If we're
  going to start adding these fixups perhaps we want to consider having a
  wrapper stage that deals with rewriting DTs prior to trying to use them?
  I'm not sure if it makes much difference but there's overlap with other
  tools like the ATAGs conversion wrapper and building separately would
  let the fixup code run early without directly going into the early init
  code (which seems a bit scary).
 
  Much better would have been if the DRM changes worked when they
  landed, so that the migration form simplefb to drm was invisible to
  the user. Or at least, to get them working ASAP since they're still
  broken. :(
 
  As far as I can tell the problem here is coming from the decision to
  have simplefb use resources without knowing about them - can we agree
  that this is a bad idea?
 
 No, I don't think we can... there is a certain amount of firmware got
 things working for us, and we're going to use it for a while that is
 absolutely reasonable. simplefb is a good example, but there are
 certainly others.
 
 I /do/ think it would be better for the simplefb data to get embedded
 or linked into the node of the graphics controller so that it can be
 torn down appropriately, and we need a rule for how long boot-state
 can be considered valid so that a proper driver can either reserve the
 resources for a given SoC, or do a full handoff from the simplefb.
 Even without that though, we need to be able to handle the case of an
 anonymous simplefb node with no regulator information. If that means
 the default simplefb behaviour is to inhibit runtime pm on all
 resources until a real driver show up, then that might just be what we
 need to do.
 
 Two things should probably be changed from the current setup. 1)
 simplefb shouldn't be a platform driver. It is a boot thing that
 handles initial state from the graphics chip. By implementing it as a
 platform driver, it prevents the real driver from binding to the real
 device if the simplefb data embedded into it. 2) make sure that an SoC
 driver can protect the needed resources before they are automatically
 disabled. Either by putting them in an earlier initcall, or handling
 it in the subsystem code. I don't know enough about the regulator and
 clock runtime PM to know what the best way to do this is.

I posted a patch[0] earlier to do this for the clock framework in that
other thread. The idea is that shim drivers for these types of firmware
devices can tell the various subsystems that they might need resources
that aren't explicitly requested. The current implementation simply uses
the existing infrastructure already present for the clk_ignore_unused
command-line argument and allows drivers to declare this requirement. It
also allows these drivers to retire the request once they've properly
handed off to the real driver.

Something similar could be done other frameworks.

One of the objections to that in the other thread is that it won't
prevent clocks from being disabled if some other driver was using those
same clocks and doing a clk_enable()/clk_disable() on them. But quite
frankly I don't think that's something we need to worry about.

Though there are two cases: one is to use simplefb as a means to have
early boot messages on a graphical display (and optionally hand off to a
real driver). The other is to use simplefb as the only framebuffer
driver until a proper driver has been implemented. The latter would have
the disadvantage of not allowing unused resources from being garbage
collected at all. Then again, I don't think power consumption is going
to be a very big issue on hardware where no proper display driver is
available.

Thierry

[0]: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291295.html


pgpCIkL4sbREV.pgp

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Wed, Sep 24, 2014 at 12:08:37PM +0300, Tomi Valkeinen wrote:
 On 23/09/14 17:58, Thierry Reding wrote:
 
  But if a panel driver controls its video source, it makes sense for the
  panel driver to get its video source in its probe, and that happens
  easiest if the panel has a link to the video source.
  
  That's an orthogonal problem. You speak about the link in software here,
  whereas the phandles only represent the link in the description of
  hardware. Now DT describes hardware from the perspective of the CPU, so
  it's sort of like a cave that you're trying to explore. You start at the
  top with the address bus, from where a couple of tunnels lead to the
  various peripherals on the address bus. A display controller might have
  another tunnel to a panel, etc.
 
 We don't do that for GPIOs, regulators, etc. either. And for video data
 there are no address buses. Yes, for DSI and similar we have a control
 bus, but that is modeled differently than the video data path.

GPIOs and regulators are just auxiliary devices that some other device
needs to operate. For instance if you want to know how to operate the
GPIO or regulator, the same still applies. You start from the CPU and
look up the GPIO controller and then the index of the GPIO within that
controller. For regulators you'd typically go and look for an I2C bus
that has a PMIC, which will expose the regulator.

Now for a device such as a display panel, what you want to do is control
the display panel. If part of how you control it involves toggling a
GPIO or a regulator, then you get access to those via phandles. And then
controlling the GPIO and regulator becomes the same as above. So it's a
matter of compositing devices in that case.

For the video data you use the phandles to model the path that video
data takes, with all the devices in that path chained together. So while
both approaches use the same mechanism (phandle) to describe the
relationships, the nature of the relationships is quite different.

 Now, I'm fine with starting from the CPU, going outwards. I agree that
 it's probably better to model it in the direction of the data stream,
 even if that would make the SW a bit more complex.
 
 However, I do think it's not as clear as you make it sound, especially
 if we take cameras/sensors into account as Laurent explained elsewhere
 in this thread.

How are cameras different? The CPU wants to capture video data from the
camera, so it needs to go look for a video capture device, which in turn
needs to involve a sensor.

It isn't so much about following the data stream itself, but rather the
connections between devices that the CPU needs to involve in order to
initiate the data flow.

  If you go the other way around, how do you detect how things connect?
  Where do you get the information about the panel so you can trace back
  to the origin?
 
 When the panel driver probes, it registers itself as a panel and gets
 its video source. Similarly a bridge in between gets its video source,
 which often would be the SoC, i.e. the origin.

That sounds backwards to me. The device tree serves the purpose of
supplementing missing information that can't be probed if hardware is
too stupid. I guess that's one of the primary reasons for structuring it
the way we do, from the CPU point of view, because it allows the CPU to
probe via the device tree.

Probing is always done downstream, so you'd start by looking at some
type of bus interface and query it for what devices are present on the
bus. Take for example PCI: the CPU only needs to know how to access the
host bridge and will then probe devices behind each of the ports on that
bridge. Some of those devices will be bridges, too, so it will continue
to probe down the hierarchy.

Without DT you don't have a means to know that there was a panel before
you've actually gone and probed your whole hierarchy and found a GPU
with some sort of output that a panel can be connected to. I think it
makes a lot of sense to describe things in the same way in DT.

Thierry


pgp7IAwUqAD6G.pgp
Description: PGP signature

Re: [PATCH v7 2/5] ARM: add AFTR mode support to firmware do_idle method

[Thierry Reding]

On Thu, Sep 25, 2014 at 05:23:32PM +0900, Kukjin Kim wrote:
 On 09/24/14 21:24, Bartlomiej Zolnierkiewicz wrote:
 On some platforms (i.e. EXYNOS ones) more than one idle mode is
 available and we need to distinguish them in firmware do_idle method.
 
 Add mode parameter to do_idle firmware method and AFTR mode support
 to EXYNOS do_idle implementation.
 
 This change is a preparation for adding secure firmware support to
 EXYNOS cpuidle driver.
 
 This patch shouldn't cause any functionality changes.
 
 Signed-off-by: Bartlomiej Zolnierkiewiczb.zolnier...@samsung.com
 Acked-by: Kyungmin Parkkyungmin.p...@samsung.com
 ---
 v7:
 - fixed build on Tegra
 - updated summary line
 
   arch/arm/include/asm/firmware.h|  2 +-
   arch/arm/mach-exynos/common.h  |  5 +
   arch/arm/mach-exynos/firmware.c| 10 --
   arch/arm/mach-tegra/cpuidle-tegra114.c |  2 +-
   4 files changed, 15 insertions(+), 4 deletions(-)
 
 diff --git a/arch/arm/include/asm/firmware.h 
 b/arch/arm/include/asm/firmware.h
 index 5904f59..89aefe1 100644
 --- a/arch/arm/include/asm/firmware.h
 +++ b/arch/arm/include/asm/firmware.h
 @@ -28,7 +28,7 @@ struct firmware_ops {
  /*
   * Enters CPU idle mode
   */
 -int (*do_idle)(void);
 +int (*do_idle)(unsigned long mode);
  /*
   * Sets boot address of specified physical CPU
   */
 diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
 index c218200..2d830df 100644
 --- a/arch/arm/mach-exynos/common.h
 +++ b/arch/arm/mach-exynos/common.h
 @@ -119,6 +119,11 @@ extern void __iomem *sysram_base_addr;
   extern void __iomem *pmu_base_addr;
   void exynos_sysram_init(void);
 
 +enum {
 +FW_DO_IDLE_SLEEP,
 +FW_DO_IDLE_AFTR,
 +};
 +
   void exynos_firmware_init(void);
 
   extern u32 exynos_get_eint_wake_mask(void);
 diff --git a/arch/arm/mach-exynos/firmware.c 
 b/arch/arm/mach-exynos/firmware.c
 index f5e626d..e57b7c3 100644
 --- a/arch/arm/mach-exynos/firmware.c
 +++ b/arch/arm/mach-exynos/firmware.c
 @@ -28,9 +28,15 @@
   #define EXYNOS_BOOT_ADDR   0x8
   #define EXYNOS_BOOT_FLAG   0xc
 
 -static int exynos_do_idle(void)
 +static int exynos_do_idle(unsigned long mode)
   {
 -exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
 +switch (mode) {
 +case FW_DO_IDLE_AFTR:
 +exynos_smc(SMC_CMD_CPU0AFTR, 0, 0, 0);
 +break;
 +case FW_DO_IDLE_SLEEP:
 +exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
 +}
  return 0;
   }
 
 diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c 
 b/arch/arm/mach-tegra/cpuidle-tegra114.c
 index e3ebdce..425b6c8 100644
 --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
 +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
 @@ -49,7 +49,7 @@ static int tegra114_idle_power_down(struct cpuidle_device 
 *dev,
  call_firmware_op(prepare_idle);
 
  /* Do suspend by ourselves if the firmware does not implement it */
 -if (call_firmware_op(do_idle) == -ENOSYS)
 +if (call_firmware_op(do_idle, 0) == -ENOSYS)
  cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
 
  clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,dev-cpu);
 
 + Thierry(using nvidia.com e-mail address)
 
 Hi Thierry,
 
 For supporting AFTR mode in call_firmware_op(do_idle), need to modify tegra
 stuff as you can see. The functionality will be same with before. Is it OK
 to you? If you have no objection, I'd like to take this series into samsung
 tree.

I'm only aware of any devices using TrustedFirmware on Tegra, which is
what the above code handles. The implementation for TrustedFirmware does
not implement do_idle() (yet?) so I don't think this patch would cause
any damage.

Acked-by: Thierry Reding tred...@nvidia.com


pgp07lS6Zp04I.pgp
Description: PGP signature

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Mon, Sep 22, 2014 at 05:23:25PM +0300, Tomi Valkeinen wrote:
 On 22/09/14 11:06, Thierry Reding wrote:
 
  Why do we need a complex graph when it can be handled using a simple 
  phandle?
 
  Maybe in your case you can handle it with simple phandle. Can you
  guarantee that it's enough for everyone, on all platforms?
  
  Nobody can guarantee that. An interesting effect that DT ABI stability
  has had is that people now try to come up with overly generic concepts
  to describe devices in an attempt to make them more future-proof. I
  don't think we're doing ourselves a favour with that approach.
 
 I kind of agree. However, there are boards that need a more complex
 approach than just a simple phandle. They are nothing new.
 
 So while it may be true that this specific encoder has never been used
 in such a complex way, and there's a chance that it never will, my
 comments are not really about this particular encoder.
 
 What I want is a standard way to describe the video components. If we
 have a standard complex one (video graphs) and a standard simple one
 (simple phandles), it's ok for me.

I certainly agree that it's useful to have standard ways to describe at
least various aspects. For example I think it would be useful to add
standard properties for a bridge's connections, such as bridge or
panel to allow bridge chaining and attaching them to panels.

But I think that should be the end of it. Mandating anything other than
that will just complicate things and limit what people can do in the
binding.

One of the disadvantages of the video graph bindings is that they are
overly vague in that they don't carry information about what type a
device is. Bindings then have to require additional meta-data, at which
point it's become far easier to describe things with a custom property
that already provides context.

  The point of the ports/endpoint graph is to also support more
  complicated scenarios.
  
  But the scenario will always remain the same for this bridge. There will
  always be an input signal and a translation to some output signal along
  with some parameters to control how that translation happens. More
  complicated scenarios will likely need to be represented differently at
  a higher level than the bridge.
 
 Yes, there's always one active input and one output for this bridge.
 What the video graphs would bring is to have the possibility to have
 multiple inputs and outputs, of which a single ones could be active at a
 time. The different inputs and outputs could even have different
 settings required (say, first output requires this bit set, but when
 using second output that bit must be cleared).

As discussed elsewhere this should be handled at a different level then.
DT should describe the hardware and this particular bridge device simply
doesn't have a means to connect more than a single input or more than a
single output.

Thierry


pgpaIH1mnFBkx.pgp
Description: PGP signature

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Mon, Sep 22, 2014 at 05:04:54PM +0300, Tomi Valkeinen wrote:
 On 22/09/14 10:54, Thierry Reding wrote:
 
  I wish all new display component bindings would use the video
  ports/endpoints to describe the connections. It will be very difficult
  to improve the display driver model later if we're missing such critical
  pieces from the DT bindings.
  
  I disagree. Why would we want to burden all devices with a bloated
 
 I agree that the .dts becomes more bloated with video graph.
 
  binding and drivers with parsing a complex graph when it's not even
 
 Hopefully not all drivers will need to do the parsing themselves. If
 it's common stuff, I don't see why we can't have helpers to do the work.
 
  known that it will be necessary? Evidently this device works fine
  using the current binding. Just because there are bindings to describe
 
 Well, I can write almost any kind of bindings, and then evidently my
 device would work. For me, on my board.

Well, that's the whole problem with DT. For many devices we only have a
single setup to test against. And even when we have several they often
are derived from each other. But the alternative would be to defer
(possibly indefinitely) merging support for a device until a second,
wildly different setup shows up. That's completely unreasonable and we
need to start somewhere.

The same issue exists whether you use the video graph or not. But like I
said elsewhere, I think the key to making this work is by keeping the DT
simple and making sure we can properly use the devices at the OS level
so that we only need to make sure we have the right connections in the
DT.

  ports in a generic way doesn't mean it has to be applied everywhere.
  After all the concept of ports/endpoints applies to non-video devices
  too, yet we don't require the bindings for those devices to add ports
  or endpoints nodes.
 
 I guess non-video devices haven't had need for those. I have had lots of
 boards with video setup that cannot be represented with simple phandles.
 I'm not sure if I have just been unlucky or what, but my understand is
 that other people have encountered such boards also. Usually the
 problems encountered there have been circumvented with some hacky video
 driver for that specific board, or maybe a static configuration handled
 by the boot loader.

I have yet to encounter such a setup. Can you point me at a DTS for one
such setup? I do remember a couple of hypothetical cases being discussed
at one time or another, but I haven't seen any actual DTS content where
this was needed.

  Also it won't be very difficult to extend the binding in a backwards
  compatible way if that becomes necessary.
 
 I don't know about that. More or less all the cases I've encountered
 where a binding has not been good from the start have been all but not
 very difficult.
 
 Do we have a standard way of representing the video pipeline with simple
 phandles? Or does everyone just do their own version? If there's no
 standard way, it sounds it'll be a mess to support in the future.

It doesn't matter all that much whether the representation is standard.
phandles should simply point to the next element in the pipeline and the
OS abstractions should be good enough to handle the details about how to
chain the elements.

While it's entirely possible to represent that using a generic graph, in
most cases (at least the trivial ones) I think that's completely
redundant. If you have a pipeline of two (single input/output) elements
where video flows from the first to the second, then all you need is a
phandle from the first element to the second element. Everything else
can easily be derived. The input for the second device will be the first
implicitly. No need to explicitly describe that in DT. Doing it
explicitly would be redundant.

Thierry


pgpe0i7PzXiB0.pgp
Description: PGP signature

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Tue, Sep 23, 2014 at 11:41:33AM +0530, Ajay kumar wrote:
 On Tue, Sep 23, 2014 at 11:25 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Tue, Sep 23, 2014 at 03:00:37AM +0300, Laurent Pinchart wrote:
  On Monday 22 September 2014 13:35:15 Thierry Reding wrote:
   On Mon, Sep 22, 2014 at 04:53:22PM +0530, Ajay kumar wrote:
On Mon, Sep 22, 2014 at 4:11 PM, Thierry Reding wrote:
 On Mon, Sep 22, 2014 at 02:01:38PM +0530, Ajay kumar wrote:
 On Mon, Sep 22, 2014 at 1:40 PM, Thierry Reding wrote:
  On Thu, Sep 18, 2014 at 11:20:40AM +0530, Ajay kumar wrote:
  On Wed, Sep 17, 2014 at 9:52 PM, Tomi Valkeinen wrote:
   On 17/09/14 17:29, Ajay kumar wrote:
   On Wed, Sep 17, 2014 at 5:22 PM, Tomi Valkeinen wrote:
   On 27/08/14 17:39, Ajay Kumar wrote:
   Add documentation for DT properties supported by 
   ps8622/ps8625
   eDP-LVDS converter.
  
   Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
   ---
  
.../devicetree/bindings/video/bridge/ps8622.txt|   20
 1 file changed, 20 insertions(+)
create mode 100644
Documentation/devicetree/bindings/video/bridge/ps8622.txt
  
   diff --git
   a/Documentation/devicetree/bindings/video/bridge/ps8622.txt
   b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
   new file mode 100644
   index 000..0ec8172
   --- /dev/null
   +++ 
   b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
   @@ -0,0 +1,20 @@
   +ps8622-bridge bindings
   +
   +Required properties:
   + - compatible: parade,ps8622 or parade,ps8625
   + - reg: first i2c address of the bridge
   + - sleep-gpios: OF device-tree gpio specification for 
   PD_
   pin.
   + - reset-gpios: OF device-tree gpio specification for 
   RST_
   pin.
   +
   +Optional properties:
   + - lane-count: number of DP lanes to use
   + - use-external-pwm: backlight will be controlled by an
   external PWM
  
   What does this mean? That the backlight support from ps8625 
   is
   not used? If so, maybe disable-pwm or something?
  
   use-external-pwm or disable-bridge-pwm would be better.
  
   Well, the properties are about the bridge. use-external-pwm
   means that the bridge uses an external PWM, which, if I 
   understood
   correctly, is not what the property is about.
  
   disable-bridge-pwm is ok, but the bridge there is extra. 
   The
   properties are about the bridge, so it's implicit.
 
  Ok. I will use disable-pwm.
 
  Why is this even necessary? According to the datasheet this device
  has circuitry for backlight control. If so, I'd expect it to 
  expose
  either a backlight device or a PWM device. That way unless 
  somebody
  is using the backlight/PWM exposed by the bridge the bridge can
  simply disable PWM.

 The driver does expose a backlight device.
 And, the decision(whether to expose a backlight device or not) is 
 made
 based on the DT flag use-external-pwm.
 This was discussed before, and you suggested to use the boolean
 property, refer to this link:
 http://lists.freedesktop.org/archives/dri-devel/2014-July/065048.html

 I think you misunderstood what I said, or maybe I didn't explain 
 clearly
 what I meant. If the PS8622 provides a backlight there's nothing 
 wrong
 with always exposing it. The bridge itself isn't going to be using 
 the
 backlight anyway. Rather the panel itself should be using the 
 backlight
 device exposed by PS8622 or some separate backlight device.

 To illustrate by an example:
 ps8622: ... {

 compatible = parade,ps8622;
 ...

 };

 panel {

 ...
 backlight = ps8622;
 ...

 };
   
No, if ps8622 backlight control is used, we need not specify the 
backlight
phandle for the panel driver. Somehow, ps8622 internal circuitry keeps
the bootup glitch free :)
Backlight control and panel controls can be separate then.
  
   But they shouldn't. Your panel driver should always be the one to
   control backlight. How else is the bridge supposed to know when to turn
   backlight on or off?
  
 What you did in v6 of this series was look up a backlight device and
 then not use it. That seemed unnecessary. Looking at v6 again the 
 reason
 for getting a phandle to the backlight was so that the device itself 
 did
 not expose its own backlight controlling circuitry if an external one
 was being used. But since the bridge has no business controlling the
 backlight, having the backlight phandle in the bridge is not correct

Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties

[Thierry Reding]

On Tue, Sep 23, 2014 at 09:24:12AM +0200, Andrzej Hajda wrote:
 Hi Thierry, Tomi,
 
 On 09/23/2014 08:04 AM, Thierry Reding wrote:
  On Mon, Sep 22, 2014 at 05:23:25PM +0300, Tomi Valkeinen wrote:
  On 22/09/14 11:06, Thierry Reding wrote:
 
  Why do we need a complex graph when it can be handled using a simple 
  phandle?
 
  Maybe in your case you can handle it with simple phandle. Can you
  guarantee that it's enough for everyone, on all platforms?
 
  Nobody can guarantee that. An interesting effect that DT ABI stability
  has had is that people now try to come up with overly generic concepts
  to describe devices in an attempt to make them more future-proof. I
  don't think we're doing ourselves a favour with that approach.
 
  I kind of agree. However, there are boards that need a more complex
  approach than just a simple phandle. They are nothing new.
 
  So while it may be true that this specific encoder has never been used
  in such a complex way, and there's a chance that it never will, my
  comments are not really about this particular encoder.
 
  What I want is a standard way to describe the video components. If we
  have a standard complex one (video graphs) and a standard simple one
  (simple phandles), it's ok for me.
  
  I certainly agree that it's useful to have standard ways to describe at
  least various aspects. For example I think it would be useful to add
  standard properties for a bridge's connections, such as bridge or
  panel to allow bridge chaining and attaching them to panels.
  
  But I think that should be the end of it. Mandating anything other than
  that will just complicate things and limit what people can do in the
  binding.
  
  One of the disadvantages of the video graph bindings is that they are
  overly vague in that they don't carry information about what type a
  device is. Bindings then have to require additional meta-data, at which
  point it's become far easier to describe things with a custom property
  that already provides context.
 
 I think it is opposite, graph bindings should describe only the link and
 certainly not what type of device is behind the endpoint. The fact we
 may need that information is a disadvantage of Linux frameworks and
 should be corrected in Linux, not by adding Linux specific properties to
 DT. For example display controller should not care where its output goes
 to: panel, encoder, bridge, whatever. It should care only if the
 parameters of the link are agreed with the receiver.

Well, a display controller is never going to attach to a panel directly.
But I agree that it would be nice to unify bridges and encoders more. It
should be possible to make encoder always a bridge (or perhaps even
replace encoders with bridges altogether). Then once you're out of the
DRM device everything would be a bridge until you get to a panel.

I think we discussed this a while back in the context of bridge
chaining.

Thierry


pgpor5We3x5KM.pgp
Description: PGP signature