RE: [Linux-stm32] [PATCH] drm/stm: ltdc: Use simple encoder

[Philippe CORNU - foss]

Applied on drm-misc-next.
Many thanks Jagan for your patch and many thanks Thomas & Yannick for your 
reviews & tests.
Philippe :-)


De : Linux-stm32  de la part 
de Yannick FERTRE - foss
Envoyé : jeudi 4 mars 2021 09:49
À : Thomas Zimmermann; Jagan Teki; Yannick FERTRE; Philippe CORNU; Benjamin 
Gaignard; Vincent ABRIOU
Cc : dri-de...@lists.freedesktop.org; linux-amar...@amarulasolutions.com; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
linux-st...@st-md-mailman.stormreply.com
Objet : Re: [Linux-stm32] [PATCH] drm/stm: ltdc: Use simple encoder

Hi Thomas,
I wait a few days before merging it.
Thank you for your help.

Best regards

Yannick


On 3/4/21 9:21 AM, Thomas Zimmermann wrote:
> Hi,
>
> shall I merge this patch?
>
> Am 02.03.21 um 18:57 schrieb Jagan Teki:
>> STM ltdc driver uses an empty implementation for its encoder.
>> Replace the code with the generic simple encoder.
>>
>> Signed-off-by: Jagan Teki 
>> ---
>>   drivers/gpu/drm/stm/ltdc.c | 12 ++--
>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 7812094f93d6..aeeb43524ca0 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -31,6 +31,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>> @@ -1020,14 +1021,6 @@ static int ltdc_crtc_init(struct drm_device
>> *ddev, struct drm_crtc *crtc)
>>   return ret;
>>   }
>> -/*
>> - * DRM_ENCODER
>> - */
>> -
>> -static const struct drm_encoder_funcs ltdc_encoder_funcs = {
>> -.destroy = drm_encoder_cleanup,
>> -};
>> -
>>   static void ltdc_encoder_disable(struct drm_encoder *encoder)
>>   {
>>   struct drm_device *ddev = encoder->dev;
>> @@ -1088,8 +1081,7 @@ static int ltdc_encoder_init(struct drm_device
>> *ddev, struct drm_bridge *bridge)
>>   encoder->possible_crtcs = CRTC_MASK;
>>   encoder->possible_clones = 0;/* No cloning support */
>> -drm_encoder_init(ddev, encoder, _encoder_funcs,
>> - DRM_MODE_ENCODER_DPI, NULL);
>> +drm_simple_encoder_init(ddev, encoder, DRM_MODE_ENCODER_DPI);
>>   drm_encoder_helper_add(encoder, _encoder_helper_funcs);
>>
>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
___
Linux-stm32 mailing list
linux-st...@st-md-mailman.stormreply.com
https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

RE: panel: panel-raydium68200 driver fails to write MIPI DSI init commands

[Philippe CORNU]

Dear Aleksandr,

We use the raydium68200 ic driver in a dsi 720p 2dl panel module mounted on the 
MB1230 board [1], mounted on the STM32MP157 eval board [2].

According to your email, you are using the EDT ETML0500F3DHA panel module, 
probably composed of a raydium68200+a touchscreen+a glass+backlight+ 

Could you please double check if your panel module has the same characteristics 
as the one described in panel-raydium68200.c (pixel clock, blanking values, 
resolutions, number of dsi data lanes, enable & reset gpios, backlight...).

Moreover, maybe your panel embeds a non-volatile ram which contains nice 
default values ("fused" during production) allowing to reduce a lot the panel 
init sequence... allowing then to use panel-simple.c instead of 
panel-raydium68200.c (that could explain why you can see "colors" without 
sending any init sequence).

The issues you encountered may come from (starting with the highest 
probability):
* bad lcd hw vs sw configuration (see description above).
* bad pixel clock frequency, bad blanking values...
* bad dsi internal Rockchip ip programming (pll and clock trees in dt...)

Hope it helps,

[1] https://wiki.st.com/stm32mpu/wiki/MB1230
[2] https://www.st.com/en/evaluation-tools/stm32mp157a-ev1.html

Philippe :-)

-Original Message-
From: aleksandr.o.maka...@gmail.com  
Sent: Saturday, January 16, 2021 12:40
To: Andrzej Hajda ; Neil Armstrong 
; Laurent Pinchart 
; Jonas Karlman ; Jernej 
Skrabec ; David Airlie ; Daniel 
Vetter ; Sam Ravnborg ; Philippe CORNU 
; Antonio BORNEO ; 
 
Subject: drm: panel: panel-raydium68200 driver fails to write MIPI DSI init 
commands

I need to bring up my MIPI DSI 1280x720 EDT ETML0500F3DHA panel on a
RockPro64 V2.1 board.

There is no completely suitable in-tree driver for that panel yet, but for the 
purpose of reproducing the issue that I face, the 
gpu/drm/panel/panel-raydium-rm68200.c can do just fine.

To reproduce:

- Get the same Linux 5.9.14 as on my RockPro64 board (with Armbian
20.11.6 on it)

- Patch the rk3399-rockpro64.dts to add a panel node that is compatible with 
"raydium,rm68200" driver on MIPI interface (rockpro64- rm68200.patch attached)

- Compile and put the resulting rk3399-rockpro64.dtb on the target system. The 
panel driver shall then get probed at next boot.

The kernel log shall contain following errors:

[   10.139957] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.139988] [drm:rm68200_dcs_write_cmd.isra.4
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write failed: -110
[   10.160972] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.161000] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.181929] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.181953] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.202923] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.202947] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.223064] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.223094] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.226104] zram1: detected capacity change from 0 to 52428800
[   10.244027] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.244073] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.265024] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.265064] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.285711] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.285746] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.305926] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.305955] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.326996] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.327039] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110
[   10.348030] dw-mipi-dsi-rockchip ff96.mipi: failed to write
command FIFO
[   10.348074] [drm:rm68200_dcs_write_buf.isra.5
[panel_raydium_rm68200]] *ERROR* MIPI DSI DCS write buffer failed: -110

It's remarkable that if to pull the module panel-rm682000 out and then back in, 
there are no errors mentioned. I can for sure say that those commands become 
effective - I start seeing colourful stripes on the display after. That is, if 
I would send the correct command set to the panel, then it would bring up just 
fine. the panel, then it would bring up just fine.

Re: [PATCH] dt-bindings: display: Add dsi-controller.yaml in DSI controller schemas

[Philippe CORNU]

On 10/3/20 12:59 AM, Rob Herring wrote:
> Some DSI controllers are missing a reference to the recently added
> dsi-controller.yaml schema. Add it and we can drop the duplicate parts.
> 
> Cc: Maxime Ripard 
> Cc: Chen-Yu Tsai 
> Cc: Eric Anholt 
> Cc: Nicolas Saenz Julienne 
> Cc: Florian Fainelli 
> Cc: Ray Jui 
> Cc: Scott Branden 
> Cc: bcm-kernel-feedback-l...@broadcom.com
> Cc: Maxime Coquelin 
> Cc: Alexandre Torgue 
> Cc: "Guido Gúnther" 
> Cc: Robert Chiras 
> Cc: Philippe Cornu 


Hi Rob,
and many thanks for the patch.
For the stm32 part,
Reviewed-by: Philippe Cornu 

Philippe :-)

Re: [PATCH v2] drm/stm: dsi: Use dev_ based logging

[Philippe CORNU]

On 10/13/20 9:56 AM, Yannick Fertre wrote:
> Standardize on the dev_ based logging.
> 
> Signed-off-by: Yannick Fertre 
> ---
> Changes in v2:
>   - restore function dsi_color_from_mipi.
>   - reword commit.
> 
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 55 ++-
>   1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c 
> b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index 164f79ef6269..a5a87c89aa07 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -76,6 +76,7 @@ enum dsi_color {
>   
>   struct dw_mipi_dsi_stm {
>   void __iomem *base;
> + struct device *dev;
>   struct clk *pllref_clk;
>   struct dw_mipi_dsi *dsi;
>   u32 hw_version;
> @@ -110,7 +111,8 @@ static inline void dsi_update_bits(struct dw_mipi_dsi_stm 
> *dsi, u32 reg,
>   dsi_write(dsi, reg, (dsi_read(dsi, reg) & ~mask) | val);
>   }
>   
> -static enum dsi_color dsi_color_from_mipi(enum mipi_dsi_pixel_format fmt)
> +static enum dsi_color dsi_color_from_mipi(struct dw_mipi_dsi_stm *dsi,
> +   enum mipi_dsi_pixel_format fmt)
>   {
>   switch (fmt) {
>   case MIPI_DSI_FMT_RGB888:
> @@ -122,7 +124,7 @@ static enum dsi_color dsi_color_from_mipi(enum 
> mipi_dsi_pixel_format fmt)
>   case MIPI_DSI_FMT_RGB565:
>   return DSI_RGB565_CONF1;
>   default:
> - DRM_DEBUG_DRIVER("MIPI color invalid, so we use rgb888\n");
> + dev_dbg(dsi->dev, "MIPI color invalid, so we use rgb888\n");
>   }
>   return DSI_RGB888;
>   }
> @@ -205,14 +207,14 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
>   ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS,
>SLEEP_US, TIMEOUT_US);
>   if (ret)
> - DRM_DEBUG_DRIVER("!TIMEOUT! waiting REGU, let's continue\n");
> + dev_dbg(dsi->dev, "!TIMEOUT! waiting REGU, let's continue\n");
>   
>   /* Enable the DSI PLL & wait for its lock */
>   dsi_set(dsi, DSI_WRPCR, WRPCR_PLLEN);
>   ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_PLLLS,
>SLEEP_US, TIMEOUT_US);
>   if (ret)
> - DRM_DEBUG_DRIVER("!TIMEOUT! waiting PLL, let's continue\n");
> + dev_dbg(dsi->dev, "!TIMEOUT! waiting PLL, let's continue\n");
>   
>   return 0;
>   }
> @@ -221,7 +223,7 @@ static void dw_mipi_dsi_phy_power_on(void *priv_data)
>   {
>   struct dw_mipi_dsi_stm *dsi = priv_data;
>   
> - DRM_DEBUG_DRIVER("\n");
> + dev_dbg(dsi->dev, "\n");
>   
>   /* Enable the DSI wrapper */
>   dsi_set(dsi, DSI_WCR, WCR_DSIEN);
> @@ -231,7 +233,7 @@ static void dw_mipi_dsi_phy_power_off(void *priv_data)
>   {
>   struct dw_mipi_dsi_stm *dsi = priv_data;
>   
> - DRM_DEBUG_DRIVER("\n");
> + dev_dbg(dsi->dev, "\n");
>   
>   /* Disable the DSI wrapper */
>   dsi_clear(dsi, DSI_WCR, WCR_DSIEN);
> @@ -267,11 +269,11 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct 
> drm_display_mode *mode,
>   
>   if (pll_out_khz > dsi->lane_max_kbps) {
>   pll_out_khz = dsi->lane_max_kbps;
> - DRM_WARN("Warning max phy mbps is used\n");
> + dev_warn(dsi->dev, "Warning max phy mbps is used\n");
>   }
>   if (pll_out_khz < dsi->lane_min_kbps) {
>   pll_out_khz = dsi->lane_min_kbps;
> - DRM_WARN("Warning min phy mbps is used\n");
> + dev_warn(dsi->dev, "Warning min phy mbps is used\n");
>   }
>   
>   /* Compute best pll parameters */
> @@ -281,7 +283,7 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct 
> drm_display_mode *mode,
>   ret = dsi_pll_get_params(dsi, pll_in_khz, pll_out_khz,
>, , );
>   if (ret)
> - DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
> + dev_warn(dsi->dev, "Warning dsi_pll_get_params(): bad 
> params\n");
>   
>   /* Get the adjusted pll out value */
>   pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
> @@ -299,12 +301,12 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct 
> drm_display_mode *mode,
>   
>   /* Select the color coding */
>   dsi_update_bits(dsi, DSI_WCFGR, WCFGR_COLMUX,
> - dsi_color_from_mipi(forma

Re: [PATCH] drm/panel: rm68200: fix mode to 50fps

[Philippe CORNU]

On 9/25/20 4:16 PM, Yannick Fertre wrote:
> Compute new timings to get a framerate of 50fps with a pixel clock
> @54Mhz.
> 
> Signed-off-by: Yannick Fertre 
> ---
>   drivers/gpu/drm/panel/panel-raydium-rm68200.c | 12 ++--
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm68200.c 
> b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
> index 2b9e48b0a491..412c0dbcb2b6 100644
> --- a/drivers/gpu/drm/panel/panel-raydium-rm68200.c
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
> @@ -82,15 +82,15 @@ struct rm68200 {
>   };
>   
>   static const struct drm_display_mode default_mode = {
> - .clock = 52582,
> + .clock = 54000,
>   .hdisplay = 720,
> - .hsync_start = 720 + 38,
> - .hsync_end = 720 + 38 + 8,
> - .htotal = 720 + 38 + 8 + 38,
> + .hsync_start = 720 + 48,
> + .hsync_end = 720 + 48 + 9,
> + .htotal = 720 + 48 + 9 + 48,
>   .vdisplay = 1280,
>   .vsync_start = 1280 + 12,
> - .vsync_end = 1280 + 12 + 4,
> - .vtotal = 1280 + 12 + 4 + 12,
> + .vsync_end = 1280 + 12 + 5,
> + .vtotal = 1280 + 12 + 5 + 12,
>   .flags = 0,
>   .width_mm = 68,
>   .height_mm = 122,
> 

Hi Yannick,
Tested-by: Philippe Cornu 
Thank you,
Philippe

Re: [PATCH] drm/bridge: dw-mipi-dsi: permit configuring the escape clock rate

[Philippe CORNU]

On 9/4/20 2:55 PM, Neil Armstrong wrote:
> The Amlogic D-PHY in the Amlogic AXG SoC Family does support a frequency
> higher than 10MHz for the TX Escape Clock, thus make the target rate
> configurable.
> 
> Signed-off-by: Neil Armstrong 
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 25 +++
>   include/drm/bridge/dw_mipi_dsi.h  |  1 +
>   2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index d580b2aa4ce9..31fc965c66fd 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -562,15 +562,30 @@ static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi)
>   
>   static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
>   {
> + const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops;
> + unsigned int esc_rate; /* in MHz */
> + u32 esc_clk_division;
> + int ret;
> +
>   /*
>* The maximum permitted escape clock is 20MHz and it is derived from
> -  * lanebyteclk, which is running at "lane_mbps / 8".  Thus we want:
> -  *
> -  * (lane_mbps >> 3) / esc_clk_division < 20
> +  * lanebyteclk, which is running at "lane_mbps / 8".
> +  */
> + if (phy_ops->get_esc_clk_rate) {
> + ret = phy_ops->get_esc_clk_rate(dsi->plat_data->priv_data,
> + _rate);
> + if (ret)
> + DRM_DEBUG_DRIVER("Phy get_esc_clk_rate() failed\n");
> + } else
> + esc_rate = 20; /* Default to 20MHz */
> +
> + /*
> +  * We want :
> +  * (lane_mbps >> 3) / esc_clk_division < X
>* which is:
> -  * (lane_mbps >> 3) / 20 > esc_clk_division
> +  * (lane_mbps >> 3) / X > esc_clk_division
>*/
> - u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1;
> + esc_clk_division = (dsi->lane_mbps >> 3) / esc_rate + 1;
>   
>   dsi_write(dsi, DSI_PWR_UP, RESET);
>   
> diff --git a/include/drm/bridge/dw_mipi_dsi.h 
> b/include/drm/bridge/dw_mipi_dsi.h
> index b0e390b3288e..bda8aa7c2280 100644
> --- a/include/drm/bridge/dw_mipi_dsi.h
> +++ b/include/drm/bridge/dw_mipi_dsi.h
> @@ -36,6 +36,7 @@ struct dw_mipi_dsi_phy_ops {
>unsigned int *lane_mbps);
>   int (*get_timing)(void *priv_data, unsigned int lane_mbps,
> struct dw_mipi_dsi_dphy_timing *timing);
> + int (*get_esc_clk_rate)(void *priv_data, unsigned int *esc_clk_rate);
>   };
>   
>   struct dw_mipi_dsi_host_ops {
> 

Hi Neil,
Thank you for the patch

Reviewed-by: Philippe Cornu 

Philippe :-)

Re: [PATCH] drm/bridge/synopsys: dsi: allow sending longer LP commands

[Philippe CORNU]

On 7/1/20 4:31 PM, Yannick Fertre wrote:
> From: Antonio Borneo 
> 
> Current code does not properly computes the max length of LP
> commands that can be send during H or V sync, and rely on static
> values.
> Limiting the max LP length to 4 byte during the V-sync is overly
> conservative.
> 
> Relax the limit and allows longer LP commands (16 bytes) to be
> sent during V-sync.
> 
> Signed-off-by: Antonio Borneo 
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 17 +
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index d580b2aa4ce9..1a24ea648ef8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -360,6 +360,15 @@ static void dw_mipi_message_config(struct dw_mipi_dsi 
> *dsi,
>   bool lpm = msg->flags & MIPI_DSI_MSG_USE_LPM;
>   u32 val = 0;
>   
> + /*
> +  * TODO dw drv improvements
> +  * largest packet sizes during hfp or during vsa/vpb/vfp
> +  * should be computed according to byte lane, lane number and only
> +  * if sending lp cmds in high speed is enable (PHY_TXREQUESTCLKHS)
> +  */
> + dsi_write(dsi, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(16)
> +   | INVACT_LPCMD_TIME(4));
> +
>   if (msg->flags & MIPI_DSI_MSG_REQ_ACK)
>   val |= ACK_RQST_EN;
>   if (lpm)
> @@ -611,14 +620,6 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi 
> *dsi,
>   dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel));
>   dsi_write(dsi, DSI_DPI_COLOR_CODING, color);
>   dsi_write(dsi, DSI_DPI_CFG_POL, val);
> - /*
> -  * TODO dw drv improvements
> -  * largest packet sizes during hfp or during vsa/vpb/vfp
> -  * should be computed according to byte lane, lane number and only
> -  * if sending lp cmds in high speed is enable (PHY_TXREQUESTCLKHS)
> -  */
> - dsi_write(dsi, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4)
> -   | INVACT_LPCMD_TIME(4));
>   }
>   
>   static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
> 

(+ Antonio)

Hi Yannick & Antonio,

Reviewed-by: Philippe Cornu 
Tested-by: Philippe Cornu 

(Tested with the 3 patches named
drm/bridge/synopsys: dsi: allow LP commands in video mode
drm/bridge/synopsys: dsi: allow sending longer LP commands
drm/bridge/synopsys: dsi: add support for non-continuous HS clock
on various dsi bridges + stm32mp157 disco board)

Many thanks
Philippe :-)

Re: [PATCH] drm/bridge/synopsys: dsi: add support for non-continuous HS clock

[Philippe CORNU]

On 7/1/20 9:42 PM, Yannick Fertre wrote:
> From: Antonio Borneo 
> 
> Current code enables the HS clock when video mode is started or to
> send out a HS command, and disables the HS clock to send out a LP
> command. This is not what DSI spec specify.
> 
> Enable HS clock either in command and in video mode.
> Set automatic HS clock management for panels and devices that
> support non-continuous HS clock.
> 
> Signed-off-by: Antonio Borneo 
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 +++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index d580b2aa4ce9..979acaa90d00 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -365,7 +365,6 @@ static void dw_mipi_message_config(struct dw_mipi_dsi 
> *dsi,
>   if (lpm)
>   val |= CMD_MODE_ALL_LP;
>   
> - dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
>   dsi_write(dsi, DSI_CMD_MODE_CFG, val);
>   }
>   
> @@ -541,16 +540,22 @@ static void dw_mipi_dsi_video_mode_config(struct 
> dw_mipi_dsi *dsi)
>   static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi,
>unsigned long mode_flags)
>   {
> + u32 val;
> +
>   dsi_write(dsi, DSI_PWR_UP, RESET);
>   
>   if (mode_flags & MIPI_DSI_MODE_VIDEO) {
>   dsi_write(dsi, DSI_MODE_CFG, ENABLE_VIDEO_MODE);
>   dw_mipi_dsi_video_mode_config(dsi);
> - dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
>   } else {
>   dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
>   }
>   
> + val = PHY_TXREQUESTCLKHS;
> + if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
> + val |= AUTO_CLKLANE_CTRL;
> +     dsi_write(dsi, DSI_LPCLK_CTRL, val);
> +
>   dsi_write(dsi, DSI_PWR_UP, POWERUP);
>   }
>   
> 

(+ Antonio)

Hi Yannick & Antonio,

Reviewed-by: Philippe Cornu 
Tested-by: Philippe Cornu 

(Tested with the 3 patches named
drm/bridge/synopsys: dsi: allow LP commands in video mode
drm/bridge/synopsys: dsi: allow sending longer LP commands
drm/bridge/synopsys: dsi: add support for non-continuous HS clock
on various dsi bridges + stm32mp157 disco board)

Many thanks
Philippe :-)

Re: [PATCH v2] drm/bridge/synopsys: dsi: allow LP commands in video mode

[Philippe CORNU]

On 7/8/20 4:08 PM, Yannick Fertre wrote:
> From: Antonio Borneo 
> 
> Current code only sends LP commands in command mode.
> 
> Allows sending LP commands also in video mode by setting the
> proper flag in DSI_VID_MODE_CFG.
> 
> Signed-off-by: Antonio Borneo 
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 1a24ea648ef8..e9a0f42ff99f 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -89,6 +89,7 @@
>   #define VID_MODE_TYPE_NON_BURST_SYNC_EVENTS 0x1
>   #define VID_MODE_TYPE_BURST 0x2
>   #define VID_MODE_TYPE_MASK  0x3
> +#define ENABLE_LOW_POWER_CMD BIT(15)
>   #define VID_MODE_VPG_ENABLE BIT(16)
>   #define VID_MODE_VPG_HORIZONTAL BIT(24)
>   
> @@ -376,6 +377,13 @@ static void dw_mipi_message_config(struct dw_mipi_dsi 
> *dsi,
>   
>   dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
>   dsi_write(dsi, DSI_CMD_MODE_CFG, val);
> +
> + val = dsi_read(dsi, DSI_VID_MODE_CFG);
> + if (lpm)
> + val |= ENABLE_LOW_POWER_CMD;
> + else
> + val &= ~ENABLE_LOW_POWER_CMD;
> + dsi_write(dsi, DSI_VID_MODE_CFG, val);
>   }
>   
>   static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 
> hdr_val)
> 

(+ Antonio)

Hi Yannick & Antonio,

Reviewed-by: Philippe Cornu 
Tested-by: Philippe Cornu 

(Tested with the 3 patches named
drm/bridge/synopsys: dsi: allow LP commands in video mode
drm/bridge/synopsys: dsi: allow sending longer LP commands
drm/bridge/synopsys: dsi: add support for non-continuous HS clock
on various dsi bridges + stm32mp157 disco board)

Many thanks
Philippe :-)

Re: [PATCH v2] drm/bridge: dw-mipi-dsi.c: Add VPG runtime config through debugfs

[Philippe CORNU]

On 7/8/20 7:08 PM, Angelo Ribeiro wrote:
> Hi,
> 
> Is this patch good to go?
> @dan...@ffwll.ch, @Philippe CORNU
> 
> Was already tested by @Yannick FERTRE
> and @Adrian Pop
> on https://lkml.org/lkml/2020/4/6/691 .
> 
> Thanks,
> Angelo
> 
> From: Yannick
> FERTRE 
> Date: Wed, Jun 24, 2020 at 16:35:04
> 
>> Hello Angelo,
>> thanks for the patch.
>> Tested-by: Yannick Fertre 
>> Tested OK on STM32MP1-DISCO, DSI v1.31
>>
>> Best regards
>>
>>
>> On 4/6/20 3:49 PM, Angelo Ribeiro wrote:
>>> Add support for the video pattern generator (VPG) BER pattern mode and
>>> configuration in runtime.
>>>
>>> This enables using the debugfs interface to manipulate the VPG after
>>> the pipeline is set.
>>> Also, enables the usage of the VPG BER pattern.
>>>
>>> Changes in v2:
>>> - Added VID_MODE_VPG_MODE
>>> - Solved incompatible return type on __get and __set
>>>
>>> Reported-by: kbuild test robot 
>>> Reported-by: Adrian Pop 
>>> Cc: Gustavo Pimentel 
>>> Cc: Joao Pinto 
>>> Cc: Jose Abreu 
>>> Signed-off-by: Angelo Ribeiro 
>>> ---
>>>drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 98 
>>> ---
>>>1 file changed, 90 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> index b18351b..9de3645 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> @@ -91,6 +91,7 @@
>>>#define VID_MODE_TYPE_BURST  0x2
>>>#define VID_MODE_TYPE_MASK   0x3
>>>#define VID_MODE_VPG_ENABLE  BIT(16)
>>> +#define VID_MODE_VPG_MODE  BIT(20)
>>>#define VID_MODE_VPG_HORIZONTAL  BIT(24)
>>>
>>>#define DSI_VID_PKT_SIZE 0x3c
>>> @@ -221,6 +222,21 @@
>>>#define PHY_STATUS_TIMEOUT_US1
>>>#define CMD_PKT_STATUS_TIMEOUT_US2
>>>
>>> +#ifdef CONFIG_DEBUG_FS
>>> +#define VPG_DEFS(name, dsi) \
>>> +   ((void __force *)&((*dsi).vpg_defs.name))
>>> +
>>> +#define REGISTER(name, mask, dsi) \
>>> +   { #name, VPG_DEFS(name, dsi), mask, dsi }
>>> +
>>> +struct debugfs_entries {
>>> +   const char  *name;
>>> +   bool*reg;
>>> +   u32 mask;
>>> +   struct dw_mipi_dsi  *dsi;
>>> +};
>>> +#endif /* CONFIG_DEBUG_FS */
>>> +
>>>struct dw_mipi_dsi {
>>> struct drm_bridge bridge;
>>> struct mipi_dsi_host dsi_host;
>>> @@ -238,9 +254,12 @@ struct dw_mipi_dsi {
>>>
>>>#ifdef CONFIG_DEBUG_FS
>>> struct dentry *debugfs;
>>> -
>>> -   bool vpg;
>>> -   bool vpg_horizontal;
>>> +   struct debugfs_entries *debugfs_vpg;
>>> +   struct {
>>> +   bool vpg;
>>> +   bool vpg_horizontal;
>>> +   bool vpg_ber_pattern;
>>> +   } vpg_defs;
>>>#endif /* CONFIG_DEBUG_FS */
>>>
>>> struct dw_mipi_dsi *master; /* dual-dsi master ptr */
>>> @@ -530,9 +549,11 @@ static void dw_mipi_dsi_video_mode_config(struct 
>>> dw_mipi_dsi *dsi)
>>> val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
>>>
>>>#ifdef CONFIG_DEBUG_FS
>>> -   if (dsi->vpg) {
>>> +   if (dsi->vpg_defs.vpg) {
>>> val |= VID_MODE_VPG_ENABLE;
>>> -   val |= dsi->vpg_horizontal ? VID_MODE_VPG_HORIZONTAL : 0;
>>> +   val |= dsi->vpg_defs.vpg_horizontal ?
>>> +  VID_MODE_VPG_HORIZONTAL : 0;
>>> +   val |= dsi->vpg_defs.vpg_ber_pattern ? VID_MODE_VPG_MODE : 0;
>>> }
>>>#endif /* CONFIG_DEBUG_FS */
>>>
>>> @@ -961,6 +982,68 @@ static const struct drm_bridge_funcs 
>>> dw_mipi_dsi_bridge_funcs = {
>>>
>>>#ifdef CONFIG_DEBUG_FS
>>>
>>> +int dw_mipi_dsi_debugfs_write(void *data, u64 val)
>>> +{
>>> +   struct debugfs_entries *vpg = data;
>>> +   struct dw_mipi_dsi *dsi;
>>> +   u32 mode_cfg;
>>> +
>>> +   if (!vpg)
>>> +

Re: [PATCH] drm/stm: ltdc: remove call of pm-runtime functions

[Philippe CORNU]

On 7/1/20 2:04 PM, Yannick Fertre wrote:
> It is not necessary to suspend or stop the ltdc clocks
> to modify the pixel clock.
> 
> Signed-off-by: Yannick Fertre 
> ---
>   drivers/gpu/drm/stm/ltdc.c | 16 
>   1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 3f590d916e91..6e28f707092f 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -506,15 +506,7 @@ static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
>struct drm_display_mode *adjusted_mode)
>   {
>   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> - struct drm_device *ddev = crtc->dev;
>   int rate = mode->clock * 1000;
> - bool runtime_active;
> - int ret;
> -
> - runtime_active = pm_runtime_active(ddev->dev);
> -
> - if (runtime_active)
> - pm_runtime_put_sync(ddev->dev);
>   
>   if (clk_set_rate(ldev->pixel_clk, rate) < 0) {
>   DRM_ERROR("Cannot set rate (%dHz) for pixel clk\n", rate);
> @@ -523,14 +515,6 @@ static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
>   
>   adjusted_mode->clock = clk_get_rate(ldev->pixel_clk) / 1000;
>   
> - if (runtime_active) {
> - ret = pm_runtime_get_sync(ddev->dev);
> - if (ret) {
> - DRM_ERROR("Failed to fixup mode, cannot get sync\n");
> - return false;
> - }
> - }
> -
>   DRM_DEBUG_DRIVER("requested clock %dkHz, adjusted clock %dkHz\n",
>mode->clock, adjusted_mode->clock);
>   
> 
Hi Yannick,
Many thanks for your patch,
Acked-by: Philippe Cornu 
Philippe :-)

Re: [Linux-stm32] [PATCH v8 08/10] drm: stm: dw-mipi-dsi: let the bridge handle the HW version check

[Philippe CORNU]

Hi Adrian,
and thank you very much for the patchset.
Thank you also for having tested it on STM32F769 and STM32MP1.
Sorry for the late response, Yannick and I will review it as soon as 
possible and we will keep you posted.
Note: Do not hesitate to put us in copy for the next version 
(philippe.co...@st.com, yannick.fer...@st.com)
Regards,
Philippe :-)


On 4/27/20 10:19 AM, Adrian Ratiu wrote:
> The stm mipi-dsi platform driver added a version test in
> commit fa6251a747b7 ("drm/stm: dsi: check hardware version")
> so that HW revisions other than v1.3x get rejected. The rockchip
> driver had no such check and just assumed register layouts are
> v1.3x compatible.
> 
> Having such tests was a good idea because only v130/v131 layouts
> were supported at the time, however since adding multiple layout
> support in the bridge, the version is automatically checked for
> all drivers, compatible layouts get picked and unsupported HW is
> automatically rejected by the bridge, so there's no use keeping
> the test in the stm driver.
> 
> The main reason prompting this change is that the stm driver
> test immediately disabled the peripheral clock after reading
> the version, making the bridge read version 0x0 immediately
> after in its own probe(), so we move the clock disabling after
> the bridge does the version test.
> 
> Tested on STM32F769 and STM32MP1.
> 
> Cc: linux-st...@st-md-mailman.stormreply.com
> Reported-by: Adrian Pop 
> Tested-by: Adrian Pop 
> Tested-by: Arnaud Ferraris 
> Signed-off-by: Adrian Ratiu 
> ---
> New in v6.
> ---
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++-
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c 
> b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index 2e1f2664495d0..7218e405d7e2b 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -402,15 +402,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device 
> *pdev)
>   goto err_dsi_probe;
>   }
>   
> - dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> - clk_disable_unprepare(pclk);
> -
> - if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
> - ret = -ENODEV;
> - DRM_ERROR("bad dsi hardware version\n");
> - goto err_dsi_probe;
> - }
> -
>   dw_mipi_dsi_stm_plat_data.base = dsi->base;
>   dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>   
> @@ -423,6 +414,9 @@ static int dw_mipi_dsi_stm_probe(struct platform_device 
> *pdev)
>   goto err_dsi_probe;
>   }
>   
> + dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> + clk_disable_unprepare(pclk);
> +
>   return 0;
>   
>   err_dsi_probe:
> 

Re: [PATCH] drm/stm: ltdc: add pinctrl for DPI encoder mode

[Philippe CORNU]

Hi Yannick,

On 8/2/19 4:47 PM, Yannick Fertré wrote:
> The implementation of functions encoder_enable and encoder_disable
> make possible to control the pinctrl according to the encoder type.
> The pinctrl must be activated only if the encoder type is DPI.
> This helps to move the DPI-related pinctrl configuration from
> all the panel or bridge to the LTDC dt node.
> 
> Signed-off-by: Yannick Fertré 
> ---
>   drivers/gpu/drm/stm/ltdc.c | 35 +++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 3ab4fbf..1c4fde0 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -15,6 +15,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -1040,6 +1041,36 @@ static const struct drm_encoder_funcs 
> ltdc_encoder_funcs = {
>   .destroy = drm_encoder_cleanup,
>   };
>   
> +static void ltdc_encoder_disable(struct drm_encoder *encoder)
> +{
> + struct drm_device *ddev = encoder->dev;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + /* Set to sleep state the pinctrl whatever type of encoder */
> + pinctrl_pm_select_sleep_state(ddev->dev);
> +}
> +
> +static void ltdc_encoder_enable(struct drm_encoder *encoder)
> +{
> + struct drm_device *ddev = encoder->dev;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + /*
> +  * Set to default state the pinctrl only with DPI type.
> +  * Others types like DSI, don't need pinctrl due to
> +  * internal bridge (the signals do not come out of the chipset).
> +  */
> + if (encoder->encoder_type == DRM_MODE_ENCODER_DPI)
> + pinctrl_pm_select_default_state(ddev->dev);
> +}
> +
> +static const struct drm_encoder_helper_funcs ltdc_encoder_helper_funcs = {
> + .disable = ltdc_encoder_disable,
> + .enable = ltdc_encoder_enable,
> +};
> +
>   static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge 
> *bridge)
>   {
>   struct drm_encoder *encoder;
> @@ -1055,6 +1086,8 @@ static int ltdc_encoder_init(struct drm_device *ddev, 
> struct drm_bridge *bridge)
>   drm_encoder_init(ddev, encoder, _encoder_funcs,
>DRM_MODE_ENCODER_DPI, NULL);
>   
> + drm_encoder_helper_add(encoder, _encoder_helper_funcs);
> +
>   ret = drm_bridge_attach(encoder, bridge, NULL);
>   if (ret) {
>   drm_encoder_cleanup(encoder);
> @@ -1280,6 +1313,8 @@ int ltdc_load(struct drm_device *ddev)
>   
>   clk_disable_unprepare(ldev->pixel_clk);
>   
> + pinctrl_pm_select_sleep_state(ddev->dev);
> +

Reviewed-by: Philippe Cornu 

Thanks
Philippe :)

>   pm_runtime_enable(ddev->dev);
>   
>   return 0;
> 

Re: [PATCH] ARM: dts: stm32: move ltdc pinctrl on stm32mp157a dk1 board

[Philippe CORNU]

Hi Yannick,

On 8/2/19 4:08 PM, Yannick Fertré wrote:
> The ltdc pinctrl must be in the display controller node and
> not in the peripheral node (hdmi bridge).
> 
> Signed-off-by: Yannick Fertré 
> ---
>   arch/arm/boot/dts/stm32mp157a-dk1.dts | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/stm32mp157a-dk1.dts 
> b/arch/arm/boot/dts/stm32mp157a-dk1.dts
> index f3f0e37..1285cfc 100644
> --- a/arch/arm/boot/dts/stm32mp157a-dk1.dts
> +++ b/arch/arm/boot/dts/stm32mp157a-dk1.dts
> @@ -99,9 +99,6 @@
>   reset-gpios = < 10 GPIO_ACTIVE_LOW>;
>   interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>   interrupt-parent = <>;
> - pinctrl-names = "default", "sleep";
> - pinctrl-0 = <_pins_a>;
> - pinctrl-1 = <_pins_sleep_a>;
>   status = "okay";
>   
>   ports {
> @@ -276,6 +273,9 @@
>   };
>   
>{
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <_pins_a>;
> + pinctrl-1 = <_pins_sleep_a>;

Reviewed-by: Philippe Cornu 

Thanks
Philippe :)

>   status = "okay";
>   
>   port {
> 

Re: [PATCH] drm/bridge: sii902x: add audio graph card support

[Philippe CORNU]

Hi Olivier,
and many thanks for your patch.
Good to have the audio graph card support, looks ok.
Reviewed-by: Philippe Cornu 
Philippe :-)

On 7/3/19 10:04 AM, Olivier Moysan wrote:
> Implement get_dai_id callback of audio HDMI codec
> to support ASoC audio graph card.
> HDMI audio output has to be connected to sii902x port 3.
> get_dai_id callback maps this port to ASoC DAI index 0.
> 
> Signed-off-by: Olivier Moysan 
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 23 +++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
> b/drivers/gpu/drm/bridge/sii902x.c
> index dd7aa466b280..daf9ef3cd817 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -158,6 +158,8 @@
>   
>   #define SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS  500
>   
> +#define SII902X_AUDIO_PORT_INDEX 3
> +
>   struct sii902x {
>   struct i2c_client *i2c;
>   struct regmap *regmap;
> @@ -690,11 +692,32 @@ static int sii902x_audio_get_eld(struct device *dev, 
> void *data,
>   return 0;
>   }
>   
> +static int sii902x_audio_get_dai_id(struct snd_soc_component *component,
> + struct device_node *endpoint)
> +{
> + struct of_endpoint of_ep;
> + int ret;
> +
> + ret = of_graph_parse_endpoint(endpoint, _ep);
> + if (ret < 0)
> + return ret;
> +
> + /*
> +  * HDMI sound should be located at reg = <3>
> +  * Return expected DAI index 0.
> +  */
> + if (of_ep.port == SII902X_AUDIO_PORT_INDEX)
> + return 0;
> +
> + return -EINVAL;
> +}
> +
>   static const struct hdmi_codec_ops sii902x_audio_codec_ops = {
>   .hw_params = sii902x_audio_hw_params,
>   .audio_shutdown = sii902x_audio_shutdown,
>   .digital_mute = sii902x_audio_digital_mute,
>   .get_eld = sii902x_audio_get_eld,
> + .get_dai_id = sii902x_audio_get_dai_id,
>   };
>   
>   static int sii902x_audio_codec_init(struct sii902x *sii902x,
> 

Re: [PATCH 2/3] dt-bindings: display: sii902x: Change audio mclk binding

[Philippe CORNU]

Hi Olivier,
and many thanks for your patch.
I have double checked in the sil9022/24 datasheet and you are right:

"As an option, the original MCLK signal used to strobe the I2S signals 
out from the sourcing chip can be used. If the internal PLL is used, 
then an external MCLK input is not required."

So, even if #sound-dai-cells is there, this is not mandatory to have MCLK.

Reviewed-by: Philippe Cornu 
Philippe :-)


On 7/2/19 5:47 PM, Olivier Moysan wrote:
> As stated in SiL9022/24 datasheet, master clock is not required for I2S.
> Make mclk property optional in DT bindings.
> 
> Fixes: 3f18021f43a3 ("dt-bindings: display: sii902x: Add HDMI audio bindings")
> 
> Signed-off-by: Olivier Moysan 
> ---
>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt 
> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> index 2df44b7d3821..6e14e087c0d0 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> @@ -26,9 +26,8 @@ Optional properties:
>   - clocks: phandle and clock specifier for each clock listed in
>  the clock-names property
>   - clock-names: "mclk"
> -Describes SII902x MCLK input. MCLK is used to produce
> -HDMI audio CTS values. This property is required if
> -"#sound-dai-cells"-property is present. This property follows
> +Describes SII902x MCLK input. MCLK can be used to produce
> +HDMI audio CTS values. This property follows
>  Documentation/devicetree/bindings/clock/clock-bindings.txt
>  consumer binding.
>   
> 

Re: [PATCH v2] drm/stm: ltdc: remove clk_round_rate comment

[Philippe CORNU]

Dear Yannick,

Acked-by: Philippe Cornu 

Thank you,

Philippe :-)

On 5/13/19 3:15 PM, Yannick Fertré wrote:
> Clk_round_rate returns rounded clock without changing
> the hardware in any way.
> This function couldn't replace set_rate/get_rate calls.
> Todo comment has been removed & a new log inserted.
> 
> Signed-off-by: Yannick Fertré 
> ---
> Changes in v2:
>   - Clk_enable & clk_disable are needed for the SOC STM32F7
>(not for STM32MP1). I return this part of the patch to make sure the
>driver is compatible with all SOC STM32.
> 
>   drivers/gpu/drm/stm/ltdc.c | 8 +++-
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 97912e2..1104e78 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -507,11 +507,6 @@ static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
>   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>   int rate = mode->clock * 1000;
> 
> - /*
> -  * TODO clk_round_rate() does not work yet. When ready, it can
> -  * be used instead of clk_set_rate() then clk_get_rate().
> -  */
> -
>   clk_disable(ldev->pixel_clk);
>   if (clk_set_rate(ldev->pixel_clk, rate) < 0) {
>   DRM_ERROR("Cannot set rate (%dHz) for pixel clk\n", rate);
> @@ -521,6 +516,9 @@ static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
> 
>   adjusted_mode->clock = clk_get_rate(ldev->pixel_clk) / 1000;
> 
> + DRM_DEBUG_DRIVER("requested clock %dkHz, adjusted clock %dkHz\n",
> +  mode->clock, adjusted_mode->clock);
> +
>   return true;
>   }
> 
> --
> 2.7.4
> 

Re: [PATCH] drm/bridge: checkpatch strict minor updates

[Philippe CORNU]

Hi,
Applied on drm-misc-next.
Note: patch subject has been renamed "drm/bridge: spelling and coding 
style minor fixes" to comply with checkpatch (my bad ;-), hope it is 
better, Many thanks,
Philippe :-)

On 05/16/2018 11:43 AM, Daniel Vetter wrote:
> On Tue, May 15, 2018 at 10:37:36PM +0200, Philippe Cornu wrote:
>> Minor fixes detected with "scripts/checkpatch.pl --strict"
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>> Detected when merging "drm: clarify adjusted_mode documentation for bridges"
> 
> Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>>
>>   include/drm/drm_bridge.h | 10 +-
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 9917651a7fdd..70131ab57e8f 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -97,7 +97,7 @@ struct drm_bridge_funcs {
>>  /**
>>   * @mode_fixup:
>>   *
>> - * This callback is used to validate and adjust a mode. The paramater
>> + * This callback is used to validate and adjust a mode. The parameter
>>   * mode is the display mode that should be fed to the next element in
>>   * the display chain, either the final _connector or the next
>>   * _bridge. The parameter adjusted_mode is the input mode the bridge
>> @@ -301,15 +301,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>struct drm_bridge *previous);
>>   
>>   bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>> -const struct drm_display_mode *mode,
>> -struct drm_display_mode *adjusted_mode);
>> +   const struct drm_display_mode *mode,
>> +   struct drm_display_mode *adjusted_mode);
>>   enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>> const struct drm_display_mode *mode);
>>   void drm_bridge_disable(struct drm_bridge *bridge);
>>   void drm_bridge_post_disable(struct drm_bridge *bridge);
>>   void drm_bridge_mode_set(struct drm_bridge *bridge,
>> -struct drm_display_mode *mode,
>> -struct drm_display_mode *adjusted_mode);
>> + struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode);
>>   void drm_bridge_pre_enable(struct drm_bridge *bridge);
>>   void drm_bridge_enable(struct drm_bridge *bridge);
>>   
>> -- 
>> 2.15.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drm/bridge: checkpatch strict minor updates

[Philippe CORNU]

Hi,
Applied on drm-misc-next.
Note: patch subject has been renamed "drm/bridge: spelling and coding 
style minor fixes" to comply with checkpatch (my bad ;-), hope it is 
better, Many thanks,
Philippe :-)

On 05/16/2018 11:43 AM, Daniel Vetter wrote:
> On Tue, May 15, 2018 at 10:37:36PM +0200, Philippe Cornu wrote:
>> Minor fixes detected with "scripts/checkpatch.pl --strict"
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>> Detected when merging "drm: clarify adjusted_mode documentation for bridges"
> 
> Acked-by: Daniel Vetter 
>>
>>   include/drm/drm_bridge.h | 10 +-
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 9917651a7fdd..70131ab57e8f 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -97,7 +97,7 @@ struct drm_bridge_funcs {
>>  /**
>>   * @mode_fixup:
>>   *
>> - * This callback is used to validate and adjust a mode. The paramater
>> + * This callback is used to validate and adjust a mode. The parameter
>>   * mode is the display mode that should be fed to the next element in
>>   * the display chain, either the final _connector or the next
>>   * _bridge. The parameter adjusted_mode is the input mode the bridge
>> @@ -301,15 +301,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>struct drm_bridge *previous);
>>   
>>   bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>> -const struct drm_display_mode *mode,
>> -struct drm_display_mode *adjusted_mode);
>> +   const struct drm_display_mode *mode,
>> +   struct drm_display_mode *adjusted_mode);
>>   enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>> const struct drm_display_mode *mode);
>>   void drm_bridge_disable(struct drm_bridge *bridge);
>>   void drm_bridge_post_disable(struct drm_bridge *bridge);
>>   void drm_bridge_mode_set(struct drm_bridge *bridge,
>> -struct drm_display_mode *mode,
>> -struct drm_display_mode *adjusted_mode);
>> + struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode);
>>   void drm_bridge_pre_enable(struct drm_bridge *bridge);
>>   void drm_bridge_enable(struct drm_bridge *bridge);
>>   
>> -- 
>> 2.15.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[PATCH] drm/bridge: checkpatch strict minor updates

[Philippe Cornu]

Minor fixes detected with "scripts/checkpatch.pl --strict"

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
Detected when merging "drm: clarify adjusted_mode documentation for bridges"

 include/drm/drm_bridge.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 9917651a7fdd..70131ab57e8f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -97,7 +97,7 @@ struct drm_bridge_funcs {
/**
 * @mode_fixup:
 *
-* This callback is used to validate and adjust a mode. The paramater
+* This callback is used to validate and adjust a mode. The parameter
 * mode is the display mode that should be fed to the next element in
 * the display chain, either the final _connector or the next
 * _bridge. The parameter adjusted_mode is the input mode the bridge
@@ -301,15 +301,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
  struct drm_bridge *previous);
 
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
-   const struct drm_display_mode *mode,
-   struct drm_display_mode *adjusted_mode);
+  const struct drm_display_mode *mode,
+  struct drm_display_mode *adjusted_mode);
 enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
   const struct drm_display_mode *mode);
 void drm_bridge_disable(struct drm_bridge *bridge);
 void drm_bridge_post_disable(struct drm_bridge *bridge);
 void drm_bridge_mode_set(struct drm_bridge *bridge,
-   struct drm_display_mode *mode,
-   struct drm_display_mode *adjusted_mode);
+struct drm_display_mode *mode,
+struct drm_display_mode *adjusted_mode);
 void drm_bridge_pre_enable(struct drm_bridge *bridge);
 void drm_bridge_enable(struct drm_bridge *bridge);
 
-- 
2.15.1

[PATCH] drm/bridge: checkpatch strict minor updates

[Philippe Cornu]

Minor fixes detected with "scripts/checkpatch.pl --strict"

Signed-off-by: Philippe Cornu 
---
Detected when merging "drm: clarify adjusted_mode documentation for bridges"

 include/drm/drm_bridge.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 9917651a7fdd..70131ab57e8f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -97,7 +97,7 @@ struct drm_bridge_funcs {
/**
 * @mode_fixup:
 *
-* This callback is used to validate and adjust a mode. The paramater
+* This callback is used to validate and adjust a mode. The parameter
 * mode is the display mode that should be fed to the next element in
 * the display chain, either the final _connector or the next
 * _bridge. The parameter adjusted_mode is the input mode the bridge
@@ -301,15 +301,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
  struct drm_bridge *previous);
 
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
-   const struct drm_display_mode *mode,
-   struct drm_display_mode *adjusted_mode);
+  const struct drm_display_mode *mode,
+  struct drm_display_mode *adjusted_mode);
 enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
   const struct drm_display_mode *mode);
 void drm_bridge_disable(struct drm_bridge *bridge);
 void drm_bridge_post_disable(struct drm_bridge *bridge);
 void drm_bridge_mode_set(struct drm_bridge *bridge,
-   struct drm_display_mode *mode,
-   struct drm_display_mode *adjusted_mode);
+struct drm_display_mode *mode,
+struct drm_display_mode *adjusted_mode);
 void drm_bridge_pre_enable(struct drm_bridge *bridge);
 void drm_bridge_enable(struct drm_bridge *bridge);
 
-- 
2.15.1

Re: [PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe CORNU]

Hi,

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/19/2018 07:00 PM, Archit Taneja wrote:
> 
> 
> On Thursday 19 April 2018 09:20 PM, Philippe CORNU wrote:
>> Hi Archit & Andrzej,
>>
>> May I ask you please a short review of this documentation update.
>> Many thanks
>> Philippe :-)
>>
>> On 04/09/2018 05:24 PM, Philippe Cornu wrote:
>>> This patch clarifies the adjusted_mode documentation
>>> for bridges.
>>>
> 
> Reviewed-by: Archit Taneja <arch...@codeaurora.org>
> 
>>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
>>> ---
>>> This patch follows discussions in:
>>> - "drm: clarify adjusted_mode for a bridge connected to a crtc"
>>>  https://patchwork.freedesktop.org/patch/206801/
>>> - "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
>>>  https://patchwork.freedesktop.org/patch/215449/
>>>
>>> include/drm/drm_bridge.h | 16 
>>> include/drm/drm_crtc.h   | 11 +++
>>> 2 files changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index 3270fec46979..7d632c6a9214 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -178,6 +178,22 @@ struct drm_bridge_funcs {
>>>  * then this would be _encoder_helper_funcs.mode_set. The 
>>> display
>>>  * pipe (i.e.  clocks and timing signals) is off when this 
>>> function is
>>>  * called.
>>> +*
>>> +* The adjusted_mode parameter corresponds to the mode output by the 
>>> CRTC
>>> +* for the first bridge in the chain. It can be different from the mode
>>> +* parameter that contains the desired mode for the connector at the end
>>> +* of the bridges chain, for instance when the first bridge in the chain
>>> +* performs scaling. The adjusted mode is mostly useful for the first
>>> +* bridge in the chain and is likely irrelevant for the other bridges.
>>> +*
>>> +* For atomic drivers the adjusted_mode is the mode stored in
>>> +* _crtc_state.adjusted_mode.
>>> +*
>>> +* NOTE:
>>> +*
>>> +* If a need arises to store and access modes adjusted for other 
>>> locations
>>> +* than the connection between the CRTC and the first bridge, the DRM
>>> +* framework will have to be extended with DRM bridge states.
>>>  */
>>> void (*mode_set)(struct drm_bridge *bridge,
>>>  struct drm_display_mode *mode,
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index a2d81d2907a9..65f749a9e9d3 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -134,10 +134,13 @@ struct drm_crtc_state {
>>>  *
>>>  * Internal display timings which can be used by the driver to 
>>> handle
>>>  * differences between the mode requested by userspace in @mode 
>>> and what
>>> -* is actually programmed into the hardware. It is purely driver
>>> -* implementation defined what exactly this adjusted mode means. Usually
>>> -* it is used to store the hardware display timings used between the
>>> -* CRTC and encoder blocks.
>>> +* is actually programmed into the hardware.
>>> +*
>>> +* For drivers using drm_bridge, this stores the hardware display 
>>> timings
>>> +* used between the CRTC and the first bridge. For other drivers, the
>>> +* meaning of the adjusted_mode field is purely driver implementation
>>> +* defined information, and will usually be used to store the hardware
>>> +* display timings used between the CRTC and encoder blocks.
>>>  */
>>> struct drm_display_mode adjusted_mode;
>>> 

Re: [PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe CORNU]

Hi,

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/19/2018 07:00 PM, Archit Taneja wrote:
> 
> 
> On Thursday 19 April 2018 09:20 PM, Philippe CORNU wrote:
>> Hi Archit & Andrzej,
>>
>> May I ask you please a short review of this documentation update.
>> Many thanks
>> Philippe :-)
>>
>> On 04/09/2018 05:24 PM, Philippe Cornu wrote:
>>> This patch clarifies the adjusted_mode documentation
>>> for bridges.
>>>
> 
> Reviewed-by: Archit Taneja 
> 
>>> Signed-off-by: Philippe Cornu 
>>> Signed-off-by: Laurent Pinchart 
>>> ---
>>> This patch follows discussions in:
>>> - "drm: clarify adjusted_mode for a bridge connected to a crtc"
>>>  https://patchwork.freedesktop.org/patch/206801/
>>> - "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
>>>  https://patchwork.freedesktop.org/patch/215449/
>>>
>>> include/drm/drm_bridge.h | 16 
>>> include/drm/drm_crtc.h   | 11 +++
>>> 2 files changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index 3270fec46979..7d632c6a9214 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -178,6 +178,22 @@ struct drm_bridge_funcs {
>>>  * then this would be _encoder_helper_funcs.mode_set. The 
>>> display
>>>  * pipe (i.e.  clocks and timing signals) is off when this 
>>> function is
>>>  * called.
>>> +*
>>> +* The adjusted_mode parameter corresponds to the mode output by the 
>>> CRTC
>>> +* for the first bridge in the chain. It can be different from the mode
>>> +* parameter that contains the desired mode for the connector at the end
>>> +* of the bridges chain, for instance when the first bridge in the chain
>>> +* performs scaling. The adjusted mode is mostly useful for the first
>>> +* bridge in the chain and is likely irrelevant for the other bridges.
>>> +*
>>> +* For atomic drivers the adjusted_mode is the mode stored in
>>> +* _crtc_state.adjusted_mode.
>>> +*
>>> +* NOTE:
>>> +*
>>> +* If a need arises to store and access modes adjusted for other 
>>> locations
>>> +* than the connection between the CRTC and the first bridge, the DRM
>>> +* framework will have to be extended with DRM bridge states.
>>>  */
>>> void (*mode_set)(struct drm_bridge *bridge,
>>>  struct drm_display_mode *mode,
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index a2d81d2907a9..65f749a9e9d3 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -134,10 +134,13 @@ struct drm_crtc_state {
>>>  *
>>>  * Internal display timings which can be used by the driver to 
>>> handle
>>>  * differences between the mode requested by userspace in @mode 
>>> and what
>>> -* is actually programmed into the hardware. It is purely driver
>>> -* implementation defined what exactly this adjusted mode means. Usually
>>> -* it is used to store the hardware display timings used between the
>>> -* CRTC and encoder blocks.
>>> +* is actually programmed into the hardware.
>>> +*
>>> +* For drivers using drm_bridge, this stores the hardware display 
>>> timings
>>> +* used between the CRTC and the first bridge. For other drivers, the
>>> +* meaning of the adjusted_mode field is purely driver implementation
>>> +* defined information, and will usually be used to store the hardware
>>> +* display timings used between the CRTC and encoder blocks.
>>>  */
>>> struct drm_display_mode adjusted_mode;
>>> 

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Andrzej,

On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> On 14.05.2018 11:38, Philippe CORNU wrote:
>> Hi Laurent, Archit, Andrzej & Yannick,
>>
>> Do you have any comments on this v2 driver part?
>> (more details regarding v1/v2 differences in the cover letter
>> https://www.spinics.net/lists/dri-devel/msg174137.html)
>>
>> Thank you,
>> Philippe :-)
>>
>>
>> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>>> Add the optional power supplies using the description found in
>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>
>>> The sii902x input IOs are not "io safe" so it is important to
>>> enable/disable voltage regulators during probe/remove phases to
>>> avoid damages.
> 
> What exactly does it mean? Ie I understand that the chip has some
> limitations, but why enabling/disabling regulators in probe/remove
> should solve it?

thank you for your comment.

And sorry for the "bad" explanation in the 2nd paragraph about the fact 
that inputs are not "io safe". I added this 2nd paragraph in v2 
following a good comment from Laurent on adding the management of the 
regulators outside the probe/remove for a better power consumption 
management (enable/disable regulators only when the ic is used for 
displaying something for instance...). But after a deeper analysis, I 
realized that the only way to improve the power consumption is to 
implement & test the sii902x various sleep modes, that is out-of-scope 
of this small patch and also out-of-scope of my test board I use on 
which the sii902x bridge ic power consumption is very low compare to the 
rest of the system...

I will remove this "explanation" in v3 as it creates confusion.

Many thanks,
Philippe :-)

> 
> Regards
> Andrzej
> 
>>>
>>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>>> ---
>>>drivers/gpu/drm/bridge/sii902x.c | 38 
>>> ++
>>>1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
>>> b/drivers/gpu/drm/bridge/sii902x.c
>>> index 60373d7eb220..c367d7b91ade 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -24,6 +24,7 @@
>>>#include 
>>>#include 
>>>#include 
>>> +#include 
>>>
>>>#include 
>>>#include 
>>> @@ -86,6 +87,7 @@ struct sii902x {
>>> struct drm_bridge bridge;
>>> struct drm_connector connector;
>>> struct gpio_desc *reset_gpio;
>>> +   struct regulator_bulk_data supplies[2];
>>>};
>>>
>>>static inline struct sii902x *bridge_to_sii902x(struct drm_bridge 
>>> *bridge)
>>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>> return PTR_ERR(sii902x->reset_gpio);
>>> }
>>>
>>> +   sii902x->supplies[0].supply = "iovcc";
>>> +   sii902x->supplies[1].supply = "vcc12";
>>> +   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>>> + sii902x->supplies);
>>> +   if (ret) {
>>> +   dev_err(dev, "Failed to get power supplies: %d\n", ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>>> +   sii902x->supplies);
>>> +   if (ret) {
>>> +   dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   usleep_range(1, 2);
>>> +
>>> sii902x_reset(sii902x);
>>>
>>> ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>> if (ret)
>>> -   return ret;
>>> +   goto err_disable_regulator;
>>>
>>> ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>>, 4);
>>> if (ret) {
>>> dev_err(dev, "regmap_read failed %d\n", ret);
>>> -   return ret;
>>> +   goto err_disable_regulator;
>>> }
>>>
>>> if (chipid[0] != 0xb0) {
>>> dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>> chipid[0]);
>>> -   r

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Andrzej,

On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> On 14.05.2018 11:38, Philippe CORNU wrote:
>> Hi Laurent, Archit, Andrzej & Yannick,
>>
>> Do you have any comments on this v2 driver part?
>> (more details regarding v1/v2 differences in the cover letter
>> https://www.spinics.net/lists/dri-devel/msg174137.html)
>>
>> Thank you,
>> Philippe :-)
>>
>>
>> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>>> Add the optional power supplies using the description found in
>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>
>>> The sii902x input IOs are not "io safe" so it is important to
>>> enable/disable voltage regulators during probe/remove phases to
>>> avoid damages.
> 
> What exactly does it mean? Ie I understand that the chip has some
> limitations, but why enabling/disabling regulators in probe/remove
> should solve it?

thank you for your comment.

And sorry for the "bad" explanation in the 2nd paragraph about the fact 
that inputs are not "io safe". I added this 2nd paragraph in v2 
following a good comment from Laurent on adding the management of the 
regulators outside the probe/remove for a better power consumption 
management (enable/disable regulators only when the ic is used for 
displaying something for instance...). But after a deeper analysis, I 
realized that the only way to improve the power consumption is to 
implement & test the sii902x various sleep modes, that is out-of-scope 
of this small patch and also out-of-scope of my test board I use on 
which the sii902x bridge ic power consumption is very low compare to the 
rest of the system...

I will remove this "explanation" in v3 as it creates confusion.

Many thanks,
Philippe :-)

> 
> Regards
> Andrzej
> 
>>>
>>> Signed-off-by: Philippe Cornu 
>>> ---
>>>drivers/gpu/drm/bridge/sii902x.c | 38 
>>> ++
>>>1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
>>> b/drivers/gpu/drm/bridge/sii902x.c
>>> index 60373d7eb220..c367d7b91ade 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -24,6 +24,7 @@
>>>#include 
>>>#include 
>>>#include 
>>> +#include 
>>>
>>>#include 
>>>#include 
>>> @@ -86,6 +87,7 @@ struct sii902x {
>>> struct drm_bridge bridge;
>>> struct drm_connector connector;
>>> struct gpio_desc *reset_gpio;
>>> +   struct regulator_bulk_data supplies[2];
>>>};
>>>
>>>static inline struct sii902x *bridge_to_sii902x(struct drm_bridge 
>>> *bridge)
>>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>> return PTR_ERR(sii902x->reset_gpio);
>>> }
>>>
>>> +   sii902x->supplies[0].supply = "iovcc";
>>> +   sii902x->supplies[1].supply = "vcc12";
>>> +   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>>> + sii902x->supplies);
>>> +   if (ret) {
>>> +   dev_err(dev, "Failed to get power supplies: %d\n", ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>>> +   sii902x->supplies);
>>> +   if (ret) {
>>> +   dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   usleep_range(1, 2);
>>> +
>>> sii902x_reset(sii902x);
>>>
>>> ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>> if (ret)
>>> -   return ret;
>>> +   goto err_disable_regulator;
>>>
>>> ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>>, 4);
>>> if (ret) {
>>> dev_err(dev, "regmap_read failed %d\n", ret);
>>> -   return ret;
>>> +   goto err_disable_regulator;
>>> }
>>>
>>> if (chipid[0] != 0xb0) {
>>> dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>> chipid[0]);
>>> -   return -EINVAL;
>>> +

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent, Archit, Andrzej & Yannick,

Do you have any comments on this v2 driver part?
(more details regarding v1/v2 differences in the cover letter 
https://www.spinics.net/lists/dri-devel/msg174137.html)

Thank you,
Philippe :-)


On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> Add the optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> The sii902x input IOs are not "io safe" so it is important to
> enable/disable voltage regulators during probe/remove phases to
> avoid damages.
> 
> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 38 ++
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
> b/drivers/gpu/drm/bridge/sii902x.c
> index 60373d7eb220..c367d7b91ade 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -24,6 +24,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   
>   #include 
>   #include 
> @@ -86,6 +87,7 @@ struct sii902x {
>   struct drm_bridge bridge;
>   struct drm_connector connector;
>   struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[2];
>   };
>   
>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>   return PTR_ERR(sii902x->reset_gpio);
>   }
>   
> + sii902x->supplies[0].supply = "iovcc";
> + sii902x->supplies[1].supply = "vcc12";
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
> +   sii902x->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to get power supplies: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
> + sii902x->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to enable power supplies: %d\n", ret);
> + return ret;
> + }
> +
> + usleep_range(1, 2);
> +
>   sii902x_reset(sii902x);
>   
>   ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>   if (ret)
> - return ret;
> + goto err_disable_regulator;
>   
>   ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>  , 4);
>   if (ret) {
>   dev_err(dev, "regmap_read failed %d\n", ret);
> - return ret;
> + goto err_disable_regulator;
>   }
>   
>   if (chipid[0] != 0xb0) {
>   dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>   chipid[0]);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_disable_regulator;
>   }
>   
>   /* Clear all pending interrupts */
> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>   IRQF_ONESHOT, dev_name(dev),
>   sii902x);
>   if (ret)
> - return ret;
> + goto err_disable_regulator;
>   }
>   
>   sii902x->bridge.funcs = _bridge_funcs;
> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>   i2c_set_clientdata(client, sii902x);
>   
>   return 0;
> +
> +err_disable_regulator:
> + regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +sii902x->supplies);
> +
> + return ret;
>   }
>   
>   static int sii902x_remove(struct i2c_client *client)
> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>   
>   drm_bridge_remove(>bridge);
>   
> + regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +sii902x->supplies);
> +
>   return 0;
>   }
>   
> 

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent, Archit, Andrzej & Yannick,

Do you have any comments on this v2 driver part?
(more details regarding v1/v2 differences in the cover letter 
https://www.spinics.net/lists/dri-devel/msg174137.html)

Thank you,
Philippe :-)


On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> Add the optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> The sii902x input IOs are not "io safe" so it is important to
> enable/disable voltage regulators during probe/remove phases to
> avoid damages.
> 
> Signed-off-by: Philippe Cornu 
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 38 ++
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
> b/drivers/gpu/drm/bridge/sii902x.c
> index 60373d7eb220..c367d7b91ade 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -24,6 +24,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   
>   #include 
>   #include 
> @@ -86,6 +87,7 @@ struct sii902x {
>   struct drm_bridge bridge;
>   struct drm_connector connector;
>   struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[2];
>   };
>   
>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>   return PTR_ERR(sii902x->reset_gpio);
>   }
>   
> + sii902x->supplies[0].supply = "iovcc";
> + sii902x->supplies[1].supply = "vcc12";
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
> +   sii902x->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to get power supplies: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
> + sii902x->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to enable power supplies: %d\n", ret);
> + return ret;
> + }
> +
> + usleep_range(1, 2);
> +
>   sii902x_reset(sii902x);
>   
>   ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>   if (ret)
> - return ret;
> + goto err_disable_regulator;
>   
>   ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>  , 4);
>   if (ret) {
>   dev_err(dev, "regmap_read failed %d\n", ret);
> - return ret;
> + goto err_disable_regulator;
>   }
>   
>   if (chipid[0] != 0xb0) {
>   dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>   chipid[0]);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_disable_regulator;
>   }
>   
>   /* Clear all pending interrupts */
> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>   IRQF_ONESHOT, dev_name(dev),
>   sii902x);
>   if (ret)
> - return ret;
> + goto err_disable_regulator;
>   }
>   
>   sii902x->bridge.funcs = _bridge_funcs;
> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>   i2c_set_clientdata(client, sii902x);
>   
>   return 0;
> +
> +err_disable_regulator:
> + regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +sii902x->supplies);
> +
> + return ret;
>   }
>   
>   static int sii902x_remove(struct i2c_client *client)
> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>   
>   drm_bridge_remove(>bridge);
>   
> + regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +sii902x->supplies);
> +
>   return 0;
>   }
>   
> 

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Rob & Laurent :)

On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
>> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
>>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
>>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
>>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>>>>>> Add optional power supplies using the description found in
>>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>>>
>>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>>>>>> (digital core) and avcc12 (TMDS analog) are derived because according
>>>>>> to this data sheet:
>>>>>> "cvcc12 and avcc12 can be derived from the same power source"
>>>>>
>>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
>>>>> ?
>>>>
>>>> Laurent,
>>>> Many thanks Laurent for your comment, I understood the merge of the two
>>>> 1v2 power supplies but missed the "mandatory" part... maybe because this
>>>> patch (with optional power supplies) already got the reviewed-by from
>>>> Rob, I thought the discussion thread you pointed out was applicable
>>>> "only" to totally new driver documentation.
>>>>
>>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
>>>> power supplies as mandatory instead of optional properties but I would
>>>> like to be sure this is applicable to both old and new bindings doc : )
>>>
>>> We obviously need to retain backward compatibility, so on the driver side
>>> you need to treat those power supplies as optional. From a DT bindings
>>> point of view, however, I think they should be mandatory for new DT.
>>
>> We don't really have a way to describe these 3 conditions (required for
>> all, optional for all, and required for new). So generally we make
>> additions optional. The exception sometimes is if we update all the dts
>> files.
> 
> Can't we just make it mandatory in the bindings, as long as we treat it as
> optional in drivers ?
> 

How to progress on this patch? Do you have any suggestions?

Many thanks for your help,
Philippe :-)

>>>> Rob,
>>>> could you please confirm these power supply properties should be
>>>> "mandatory"? if yes, should we then modify other optional properties like
>>>> the reset-gpios too in the future?
>>>
>>> The GPIOs properties are different in my opinion, as there's no
>>> requirement to connect for instance the reset pin to a GPIO controllable
>>> by the SoC. The pin could be hardwired to VCC, or connected to a system
>>> reset that is automatically managed without SoC intervention. The power
>>> supplies, however, are mandatory, in the sense that the chip will not work
>>> if you leave the power supplies unconnected.
>>
>> DT only needs to describe what matters to s/w. If a regulator is
>> fixed and you don't need to know its voltage (or other read-only
>> parameters), then there's not much point in putting it in DT.
>>
>> I'd probably base this more at a platform level and you either use
>> regulator binding or you don't. It's perfectly valid that you want to do
>> things like regulator setup, pin ctrl and muxing setup, etc. all in
>> firmware and the OS doesn't touch any of that.
>>
>> That's all a big can of worms which we shouldn't solve on this 2 line
>> change. I think this change is fine as-is, so:
>>
>> Reviewed-by: Rob Herring <r...@kernel.org>
> 

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Rob & Laurent :)

On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
>> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
>>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
>>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
>>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>>>>>> Add optional power supplies using the description found in
>>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>>>
>>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>>>>>> (digital core) and avcc12 (TMDS analog) are derived because according
>>>>>> to this data sheet:
>>>>>> "cvcc12 and avcc12 can be derived from the same power source"
>>>>>
>>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
>>>>> ?
>>>>
>>>> Laurent,
>>>> Many thanks Laurent for your comment, I understood the merge of the two
>>>> 1v2 power supplies but missed the "mandatory" part... maybe because this
>>>> patch (with optional power supplies) already got the reviewed-by from
>>>> Rob, I thought the discussion thread you pointed out was applicable
>>>> "only" to totally new driver documentation.
>>>>
>>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
>>>> power supplies as mandatory instead of optional properties but I would
>>>> like to be sure this is applicable to both old and new bindings doc : )
>>>
>>> We obviously need to retain backward compatibility, so on the driver side
>>> you need to treat those power supplies as optional. From a DT bindings
>>> point of view, however, I think they should be mandatory for new DT.
>>
>> We don't really have a way to describe these 3 conditions (required for
>> all, optional for all, and required for new). So generally we make
>> additions optional. The exception sometimes is if we update all the dts
>> files.
> 
> Can't we just make it mandatory in the bindings, as long as we treat it as
> optional in drivers ?
> 

How to progress on this patch? Do you have any suggestions?

Many thanks for your help,
Philippe :-)

>>>> Rob,
>>>> could you please confirm these power supply properties should be
>>>> "mandatory"? if yes, should we then modify other optional properties like
>>>> the reset-gpios too in the future?
>>>
>>> The GPIOs properties are different in my opinion, as there's no
>>> requirement to connect for instance the reset pin to a GPIO controllable
>>> by the SoC. The pin could be hardwired to VCC, or connected to a system
>>> reset that is automatically managed without SoC intervention. The power
>>> supplies, however, are mandatory, in the sense that the chip will not work
>>> if you leave the power supplies unconnected.
>>
>> DT only needs to describe what matters to s/w. If a regulator is
>> fixed and you don't need to know its voltage (or other read-only
>> parameters), then there's not much point in putting it in DT.
>>
>> I'd probably base this more at a platform level and you either use
>> regulator binding or you don't. It's perfectly valid that you want to do
>> things like regulator setup, pin ctrl and muxing setup, etc. all in
>> firmware and the OS doesn't touch any of that.
>>
>> That's all a big can of worms which we shouldn't solve on this 2 line
>> change. I think this change is fine as-is, so:
>>
>> Reviewed-by: Rob Herring 
> 

Re: [PATCH] drm/stm: ltdc: fix warnings in ltdc_plane_create()

[Philippe CORNU]

On 04/25/2018 09:13 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré <yannick.fer...@st.com>
> 

Applied on drm-misc-next.
Many thanks,
Philippe :-)

> 
> On 04/19/2018 03:28 PM, Philippe Cornu wrote:
>> "make C=1" returns 2 warnings in ltdc_plane_create()
>> ("Using plain integer as NULL pointer"). This patch
>> fixes them.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 4 ++--
>>1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 616191fe98ae..d997a6014d6c 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -860,13 +860,13 @@ static struct drm_plane *ltdc_plane_create(struct 
>> drm_device *ddev,
>>
>>  plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
>>  if (!plane)
>> -return 0;
>> +return NULL;
>>
>>  ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
>> _plane_funcs, formats, nb_fmt,
>> NULL, type, NULL);
>>  if (ret < 0)
>> -return 0;
>> +return NULL;
>>
>>  drm_plane_helper_add(plane, _plane_helper_funcs);
>>

Re: [PATCH] drm/stm: ltdc: fix warnings in ltdc_plane_create()

[Philippe CORNU]

On 04/25/2018 09:13 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré 
> 

Applied on drm-misc-next.
Many thanks,
Philippe :-)

> 
> On 04/19/2018 03:28 PM, Philippe Cornu wrote:
>> "make C=1" returns 2 warnings in ltdc_plane_create()
>> ("Using plain integer as NULL pointer"). This patch
>> fixes them.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 4 ++--
>>1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 616191fe98ae..d997a6014d6c 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -860,13 +860,13 @@ static struct drm_plane *ltdc_plane_create(struct 
>> drm_device *ddev,
>>
>>  plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
>>  if (!plane)
>> -return 0;
>> +return NULL;
>>
>>  ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
>> _plane_funcs, formats, nb_fmt,
>> NULL, type, NULL);
>>  if (ret < 0)
>> -return 0;
>> +return NULL;
>>
>>  drm_plane_helper_add(plane, _plane_helper_funcs);
>>

Re: [PATCH] drm/stm: ltdc: add mode_valid()

[Philippe CORNU]

On 04/25/2018 09:12 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré <yannick.fer...@st.com>
> 

Applied on drm-misc-next.
Many thanks,
Philippe :-)

> On 04/17/2018 01:40 PM, Philippe Cornu wrote:
>> Add mode_valid() function to filter modes according to available
>> pll clock values and "preferred" modes. It is particularly
>> useful for hdmi modes that require precise pixel clocks.
>>
>> Note that "preferred" modes are always accepted:
>> - this is important for panels because panel clock tolerances are
>> bigger than hdmi ones and there is no reason to not accept them
>> (the fps may vary a little but it is not a problem).
>> - the hdmi preferred mode will be accepted too, but userland will
>> be able to use others hdmi "valid" modes if necessary.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 38 ++
>>1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 014cef8cef37..616191fe98ae 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -445,6 +445,43 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc 
>> *crtc,
>>  reg_set(ldev->regs, LTDC_SRCR, SRCR_IMR);
>>}
>>
>> +#define CLK_TOLERANCE_HZ 50
>> +
>> +static enum drm_mode_status
>> +ltdc_crtc_mode_valid(struct drm_crtc *crtc,
>> + const struct drm_display_mode *mode)
>> +{
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>> +int target = mode->clock * 1000;
>> +int target_min = target - CLK_TOLERANCE_HZ;
>> +int target_max = target + CLK_TOLERANCE_HZ;
>> +int result;
>> +
>> +/*
>> + * Accept all "preferred" modes:
>> + * - this is important for panels because panel clock tolerances are
>> + *   bigger than hdmi ones and there is no reason to not accept them
>> + *   (the fps may vary a little but it is not a problem).
>> + * - the hdmi preferred mode will be accepted too, but userland will
>> + *   be able to use others hdmi "valid" modes if necessary.
>> + */
>> +if (mode->type & DRM_MODE_TYPE_PREFERRED)
>> +return MODE_OK;
>> +
>> +result = clk_round_rate(ldev->pixel_clk, target);
>> +
>> +DRM_DEBUG_DRIVER("clk rate target %d, available %d\n", target, result);
>> +
>> +/*
>> + * Filter modes according to the clock value, particularly useful for
>> + * hdmi modes that require precise pixel clocks.
>> + */
>> +if (result < target_min || result > target_max)
>> +return MODE_CLOCK_RANGE;
>> +
>> +return MODE_OK;
>> +}
>> +
>>static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
>>   const struct drm_display_mode *mode,
>>   struct drm_display_mode *adjusted_mode)
>> @@ -559,6 +596,7 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
>>}
>>
>>static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
>> +.mode_valid = ltdc_crtc_mode_valid,
>>  .mode_fixup = ltdc_crtc_mode_fixup,
>>  .mode_set_nofb = ltdc_crtc_mode_set_nofb,
>>  .atomic_flush = ltdc_crtc_atomic_flush,

Re: [PATCH] drm/stm: ltdc: add mode_valid()

[Philippe CORNU]

On 04/25/2018 09:12 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré 
> 

Applied on drm-misc-next.
Many thanks,
Philippe :-)

> On 04/17/2018 01:40 PM, Philippe Cornu wrote:
>> Add mode_valid() function to filter modes according to available
>> pll clock values and "preferred" modes. It is particularly
>> useful for hdmi modes that require precise pixel clocks.
>>
>> Note that "preferred" modes are always accepted:
>> - this is important for panels because panel clock tolerances are
>> bigger than hdmi ones and there is no reason to not accept them
>> (the fps may vary a little but it is not a problem).
>> - the hdmi preferred mode will be accepted too, but userland will
>> be able to use others hdmi "valid" modes if necessary.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 38 ++
>>1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 014cef8cef37..616191fe98ae 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -445,6 +445,43 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc 
>> *crtc,
>>  reg_set(ldev->regs, LTDC_SRCR, SRCR_IMR);
>>}
>>
>> +#define CLK_TOLERANCE_HZ 50
>> +
>> +static enum drm_mode_status
>> +ltdc_crtc_mode_valid(struct drm_crtc *crtc,
>> + const struct drm_display_mode *mode)
>> +{
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>> +int target = mode->clock * 1000;
>> +int target_min = target - CLK_TOLERANCE_HZ;
>> +int target_max = target + CLK_TOLERANCE_HZ;
>> +int result;
>> +
>> +/*
>> + * Accept all "preferred" modes:
>> + * - this is important for panels because panel clock tolerances are
>> + *   bigger than hdmi ones and there is no reason to not accept them
>> + *   (the fps may vary a little but it is not a problem).
>> + * - the hdmi preferred mode will be accepted too, but userland will
>> + *   be able to use others hdmi "valid" modes if necessary.
>> + */
>> +if (mode->type & DRM_MODE_TYPE_PREFERRED)
>> +return MODE_OK;
>> +
>> +result = clk_round_rate(ldev->pixel_clk, target);
>> +
>> +DRM_DEBUG_DRIVER("clk rate target %d, available %d\n", target, result);
>> +
>> +/*
>> + * Filter modes according to the clock value, particularly useful for
>> + * hdmi modes that require precise pixel clocks.
>> + */
>> +if (result < target_min || result > target_max)
>> +return MODE_CLOCK_RANGE;
>> +
>> +return MODE_OK;
>> +}
>> +
>>static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
>>   const struct drm_display_mode *mode,
>>   struct drm_display_mode *adjusted_mode)
>> @@ -559,6 +596,7 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
>>}
>>
>>static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
>> +.mode_valid = ltdc_crtc_mode_valid,
>>  .mode_fixup = ltdc_crtc_mode_fixup,
>>  .mode_set_nofb = ltdc_crtc_mode_set_nofb,
>>  .atomic_flush = ltdc_crtc_atomic_flush,

Re: [PATCH] drm/stm: ltdc: fix deferred endpoint management

[Philippe CORNU]

On 04/25/2018 09:12 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré <yannick.fer...@st.com>

Applied on drm-misc-next.
Many thanks,
Philippe :-)


> 
> 
> On 04/17/2018 01:34 PM, Philippe Cornu wrote:
>> When a driver related to one of the endpoints is deferred
>> due to probe dependencies (i2c, spi...) but the other one
>> is ready, ltdc probe continues and the deferred driver
>> will never be probed again.
>>
>> The fix consists in waiting for all deferred endpoints before
>> continuing the ltdc probe.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 11 +--
>>1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index e3121d9e4230..014cef8cef37 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -987,14 +987,13 @@ int ltdc_load(struct drm_device *ddev)
>>[i]);
>>
>>  /*
>> - * If at least one endpoint is ready, continue probing,
>> - * else if at least one endpoint is -EPROBE_DEFER and
>> - * there is no previous ready endpoints, defer probing.
>> + * If at least one endpoint is -EPROBE_DEFER, defer probing,
>> + * else if at least one endpoint is ready, continue probing.
>>   */
>> -if (!ret)
>> +if (ret == -EPROBE_DEFER)
>> +return ret;
>> +else if (!ret)
>>  endpoint_not_ready = 0;
>> -else if (ret == -EPROBE_DEFER && endpoint_not_ready)
>> -endpoint_not_ready = -EPROBE_DEFER;
>>  }
>>
>>  if (endpoint_not_ready)

Re: [PATCH] drm/stm: ltdc: fix deferred endpoint management

[Philippe CORNU]

On 04/25/2018 09:12 AM, Yannick FERTRE wrote:
> Hi Philippe,
> 
> Reviewed-by: Yannick Fertré 

Applied on drm-misc-next.
Many thanks,
Philippe :-)


> 
> 
> On 04/17/2018 01:34 PM, Philippe Cornu wrote:
>> When a driver related to one of the endpoints is deferred
>> due to probe dependencies (i2c, spi...) but the other one
>> is ready, ltdc probe continues and the deferred driver
>> will never be probed again.
>>
>> The fix consists in waiting for all deferred endpoints before
>> continuing the ltdc probe.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 11 +--
>>1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index e3121d9e4230..014cef8cef37 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -987,14 +987,13 @@ int ltdc_load(struct drm_device *ddev)
>>[i]);
>>
>>  /*
>> - * If at least one endpoint is ready, continue probing,
>> - * else if at least one endpoint is -EPROBE_DEFER and
>> - * there is no previous ready endpoints, defer probing.
>> + * If at least one endpoint is -EPROBE_DEFER, defer probing,
>> + * else if at least one endpoint is ready, continue probing.
>>   */
>> -if (!ret)
>> +if (ret == -EPROBE_DEFER)
>> +return ret;
>> +else if (!ret)
>>  endpoint_not_ready = 0;
>> -else if (ret == -EPROBE_DEFER && endpoint_not_ready)
>> -endpoint_not_ready = -EPROBE_DEFER;
>>  }
>>
>>  if (endpoint_not_ready)

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent & Rob :-)

On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>> Add optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>> (digital core) and avcc12 (TMDS analog) are derived because according
>> to this data sheet:
>> "cvcc12 and avcc12 can be derived from the same power source"
> 
> Shouldn't the power supplies be mandatory, as explained by Mark in https://
> lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?
> 

Laurent,
Many thanks Laurent for your comment, I understood the merge of the two 
1v2 power supplies but missed the "mandatory" part... maybe because this 
patch (with optional power supplies) already got the reviewed-by from 
Rob, I thought the discussion thread you pointed out was applicable 
"only" to totally new driver documentation.

So, on my side, as a "new user" of sii902x IC, no problem to put these 
power supplies as mandatory instead of optional properties but I would 
like to be sure this is applicable to both old and new bindings doc : )

Rob,
could you please confirm these power supply properties should be 
"mandatory"?
if yes, should we then modify other optional properties like the 
reset-gpios too in the future?

Big thanks to both of you,
Philippe :-)

>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..9fb41fc9af51 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,8 @@ Optional properties:
>>  - interrupts-extended or interrupt-parent + interrupts: describe
>>the interrupt line used to inform the host about hotplug events.
>>  - reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
>>
>>   Optional subnodes:
>>  - video input: this subnode can contain a video input port node
> 

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent & Rob :-)

On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>> Add optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>> (digital core) and avcc12 (TMDS analog) are derived because according
>> to this data sheet:
>> "cvcc12 and avcc12 can be derived from the same power source"
> 
> Shouldn't the power supplies be mandatory, as explained by Mark in https://
> lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?
> 

Laurent,
Many thanks Laurent for your comment, I understood the merge of the two 
1v2 power supplies but missed the "mandatory" part... maybe because this 
patch (with optional power supplies) already got the reviewed-by from 
Rob, I thought the discussion thread you pointed out was applicable 
"only" to totally new driver documentation.

So, on my side, as a "new user" of sii902x IC, no problem to put these 
power supplies as mandatory instead of optional properties but I would 
like to be sure this is applicable to both old and new bindings doc : )

Rob,
could you please confirm these power supply properties should be 
"mandatory"?
if yes, should we then modify other optional properties like the 
reset-gpios too in the future?

Big thanks to both of you,
Philippe :-)

>> Signed-off-by: Philippe Cornu 
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..9fb41fc9af51 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,8 @@ Optional properties:
>>  - interrupts-extended or interrupt-parent + interrupts: describe
>>the interrupt line used to inform the host about hotplug events.
>>  - reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
>>
>>   Optional subnodes:
>>  - video input: this subnode can contain a video input port node
> 

[PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

The sii902x input IOs are not "io safe" so it is important to
enable/disable voltage regulators during probe/remove phases to
avoid damages.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 38 ++
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..c367d7b91ade 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -86,6 +87,7 @@ struct sii902x {
struct drm_bridge bridge;
struct drm_connector connector;
struct gpio_desc *reset_gpio;
+   struct regulator_bulk_data supplies[2];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
return PTR_ERR(sii902x->reset_gpio);
}
 
+   sii902x->supplies[0].supply = "iovcc";
+   sii902x->supplies[1].supply = "vcc12";
+   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+ sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "Failed to get power supplies: %d\n", ret);
+   return ret;
+   }
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+   sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "Failed to enable power supplies: %d\n", ret);
+   return ret;
+   }
+
+   usleep_range(1, 2);
+
sii902x_reset(sii902x);
 
ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
if (ret)
-   return ret;
+   goto err_disable_regulator;
 
ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
   , 4);
if (ret) {
dev_err(dev, "regmap_read failed %d\n", ret);
-   return ret;
+   goto err_disable_regulator;
}
 
if (chipid[0] != 0xb0) {
dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
chipid[0]);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto err_disable_regulator;
}
 
/* Clear all pending interrupts */
@@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
IRQF_ONESHOT, dev_name(dev),
sii902x);
if (ret)
-   return ret;
+   goto err_disable_regulator;
}
 
sii902x->bridge.funcs = _bridge_funcs;
@@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
i2c_set_clientdata(client, sii902x);
 
return 0;
+
+err_disable_regulator:
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
+   return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
 
drm_bridge_remove(>bridge);
 
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
return 0;
 }
 
-- 
2.15.1

[PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

The sii902x input IOs are not "io safe" so it is important to
enable/disable voltage regulators during probe/remove phases to
avoid damages.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/bridge/sii902x.c | 38 ++
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..c367d7b91ade 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -86,6 +87,7 @@ struct sii902x {
struct drm_bridge bridge;
struct drm_connector connector;
struct gpio_desc *reset_gpio;
+   struct regulator_bulk_data supplies[2];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
return PTR_ERR(sii902x->reset_gpio);
}
 
+   sii902x->supplies[0].supply = "iovcc";
+   sii902x->supplies[1].supply = "vcc12";
+   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+ sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "Failed to get power supplies: %d\n", ret);
+   return ret;
+   }
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+   sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "Failed to enable power supplies: %d\n", ret);
+   return ret;
+   }
+
+   usleep_range(1, 2);
+
sii902x_reset(sii902x);
 
ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
if (ret)
-   return ret;
+   goto err_disable_regulator;
 
ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
   , 4);
if (ret) {
dev_err(dev, "regmap_read failed %d\n", ret);
-   return ret;
+   goto err_disable_regulator;
}
 
if (chipid[0] != 0xb0) {
dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
chipid[0]);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto err_disable_regulator;
}
 
/* Clear all pending interrupts */
@@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
IRQF_ONESHOT, dev_name(dev),
sii902x);
if (ret)
-   return ret;
+   goto err_disable_regulator;
}
 
sii902x->bridge.funcs = _bridge_funcs;
@@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
i2c_set_clientdata(client, sii902x);
 
return 0;
+
+err_disable_regulator:
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
+   return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
 
drm_bridge_remove(>bridge);
 
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
return 0;
 }
 
-- 
2.15.1

[PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

There is a single 1v2 supply voltage named vcc12 from which cvcc12
(digital core) and avcc12 (TMDS analog) are derived because according
to this data sheet:
"cvcc12 and avcc12 can be derived from the same power source"

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt 
b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..9fb41fc9af51 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,8 @@ Optional properties:
- interrupts-extended or interrupt-parent + interrupts: describe
  the interrupt line used to inform the host about hotplug events.
- reset-gpios: OF device-tree gpio specification for RST_N pin.
+   - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+   - vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
 
 Optional subnodes:
- video input: this subnode can contain a video input port node
-- 
2.15.1

[PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

There is a single 1v2 supply voltage named vcc12 from which cvcc12
(digital core) and avcc12 (TMDS analog) are derived because according
to this data sheet:
"cvcc12 and avcc12 can be derived from the same power source"

Signed-off-by: Philippe Cornu 
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt 
b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..9fb41fc9af51 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,8 @@ Optional properties:
- interrupts-extended or interrupt-parent + interrupts: describe
  the interrupt line used to inform the host about hotplug events.
- reset-gpios: OF device-tree gpio specification for RST_N pin.
+   - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+   - vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
 
 Optional subnodes:
- video input: this subnode can contain a video input port node
-- 
2.15.1

[PATCH v2 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds optional power supplies to the sii902x
drm bridge driver.

Version 2:
- merge avcc12 & cvcc12 to a single vcc12 supply as suggested by
  Laurent Pinchart (see discussion details in
  https://patchwork.freedesktop.org/patch/216058/)
- improve error messages following Laurent Pinchart comments.
- note about power consumption: as sii902x input IOs are not "io safe",
  it is important to enable/disable voltage regulators during
  probe/remove phases to avoid damages. Then, the only way to improve
  the power consumption is to add the sii902x standy mode. My actual
  platform is not an handheld device so power consumption is not
  critical. But in the future, it could be nice to add the standby
  mode, paying attention to wake-up events (hdmi cable plug)...

Version 1:
- Initial commit

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  2 ++
 drivers/gpu/drm/bridge/sii902x.c   | 38 +++---
 2 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH v2 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds optional power supplies to the sii902x
drm bridge driver.

Version 2:
- merge avcc12 & cvcc12 to a single vcc12 supply as suggested by
  Laurent Pinchart (see discussion details in
  https://patchwork.freedesktop.org/patch/216058/)
- improve error messages following Laurent Pinchart comments.
- note about power consumption: as sii902x input IOs are not "io safe",
  it is important to enable/disable voltage regulators during
  probe/remove phases to avoid damages. Then, the only way to improve
  the power consumption is to add the sii902x standy mode. My actual
  platform is not an handheld device so power consumption is not
  critical. But in the future, it could be nice to add the standby
  mode, paying attention to wake-up events (hdmi cable plug)...

Version 1:
- Initial commit

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  2 ++
 drivers/gpu/drm/bridge/sii902x.c   | 38 +++---
 2 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH 0/4] drm/panel: otm8009a: backlight fixes & improvements

[Philippe Cornu]

This patch serie fixes 2 backlight issues and adds the new
backlight api support.

Philippe Cornu (4):
  drm/panel: otm8009a: fix backlight updates
  drm/panel: otm8009a: fix glitches by moving backlight enable to
otm8009a_enable()
  drm/panel: otm8009a: no message if probe success
  drm/panel: otm8009a: use new backlight api

 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 58 
 1 file changed, 30 insertions(+), 28 deletions(-)

-- 
2.15.1

[PATCH 0/4] drm/panel: otm8009a: backlight fixes & improvements

[Philippe Cornu]

This patch serie fixes 2 backlight issues and adds the new
backlight api support.

Philippe Cornu (4):
  drm/panel: otm8009a: fix backlight updates
  drm/panel: otm8009a: fix glitches by moving backlight enable to
otm8009a_enable()
  drm/panel: otm8009a: no message if probe success
  drm/panel: otm8009a: use new backlight api

 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 58 
 1 file changed, 30 insertions(+), 28 deletions(-)

-- 
2.15.1

[PATCH 1/4] drm/panel: otm8009a: fix backlight updates

[Philippe Cornu]

Backlight updates was not working anymore since the good
implementation of the dsi lpm mode in the dsi host driver.
After a longer analysis, the backlight updates in dsi video
mode require the dsi hs mode.
Note: it is important to keep the dsi lpm mode for the rest
of the driver as init sequence, sleep in/out... dsi commands
work in lp mode.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 90f1ae4af93c..0fd2e0144d2b 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -98,6 +98,20 @@ static void otm8009a_dcs_write_buf(struct otm8009a *ctx, 
const void *data,
DRM_WARN("mipi dsi dcs write buffer failed\n");
 }
 
+static void otm8009a_dcs_write_buf_hs(struct otm8009a *ctx, const void *data,
+ size_t len)
+{
+   struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+   /* data will be sent in dsi hs mode (ie. no lpm) */
+   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+   otm8009a_dcs_write_buf(ctx, data, len);
+
+   /* restore back the dsi lpm mode */
+   dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+}
+
 #define dcs_write_seq(ctx, seq...) \
 ({ \
static const u8 d[] = { seq };  \
@@ -387,7 +401,7 @@ static int otm8009a_backlight_update_status(struct 
backlight_device *bd)
 */
data[0] = MIPI_DCS_SET_DISPLAY_BRIGHTNESS;
data[1] = bd->props.brightness;
-   otm8009a_dcs_write_buf(ctx, data, ARRAY_SIZE(data));
+   otm8009a_dcs_write_buf_hs(ctx, data, ARRAY_SIZE(data));
 
/* set Brightness Control & Backlight on */
data[1] = 0x24;
@@ -399,7 +413,7 @@ static int otm8009a_backlight_update_status(struct 
backlight_device *bd)
 
/* Update Brightness Control & Backlight */
data[0] = MIPI_DCS_WRITE_CONTROL_DISPLAY;
-   otm8009a_dcs_write_buf(ctx, data, ARRAY_SIZE(data));
+   otm8009a_dcs_write_buf_hs(ctx, data, ARRAY_SIZE(data));
 
return 0;
 }
-- 
2.15.1

[PATCH 3/4] drm/panel: otm8009a: no message if probe success

[Philippe Cornu]

Remove the message in case of probe success. This comes from
a suggestion followed in the recent integration of the
raydium rm68200 panel.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index de4a16d5275c..4c638b7b9943 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -14,8 +14,6 @@
 #include 
 #include 
 
-#define DRV_NAME "orisetech_otm8009a"
-
 #define OTM8009A_BACKLIGHT_DEFAULT 240
 #define OTM8009A_BACKLIGHT_MAX 255
 
@@ -461,7 +459,7 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
ctx->panel.dev = dev;
ctx->panel.funcs = _drm_funcs;
 
-   ctx->bl_dev = backlight_device_register(DRV_NAME "_backlight", dev, ctx,
+   ctx->bl_dev = backlight_device_register(dev_name(dev), dev, ctx,
_backlight_ops, NULL);
if (IS_ERR(ctx->bl_dev)) {
dev_err(dev, "failed to register backlight device\n");
@@ -483,11 +481,6 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
return ret;
}
 
-   DRM_INFO(DRV_NAME "_panel %ux%u@%u %ubpp dsi %udl - ready\n",
-default_mode.hdisplay, default_mode.vdisplay,
-default_mode.vrefresh,
-mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
-
return 0;
 }
 
@@ -513,7 +506,7 @@ static struct mipi_dsi_driver orisetech_otm8009a_driver = {
.probe  = otm8009a_probe,
.remove = otm8009a_remove,
.driver = {
-   .name = DRV_NAME "_panel",
+   .name = "panel-orisetech-otm8009a",
.of_match_table = orisetech_otm8009a_of_match,
},
 };
-- 
2.15.1

[PATCH 3/4] drm/panel: otm8009a: no message if probe success

[Philippe Cornu]

Remove the message in case of probe success. This comes from
a suggestion followed in the recent integration of the
raydium rm68200 panel.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index de4a16d5275c..4c638b7b9943 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -14,8 +14,6 @@
 #include 
 #include 
 
-#define DRV_NAME "orisetech_otm8009a"
-
 #define OTM8009A_BACKLIGHT_DEFAULT 240
 #define OTM8009A_BACKLIGHT_MAX 255
 
@@ -461,7 +459,7 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
ctx->panel.dev = dev;
ctx->panel.funcs = _drm_funcs;
 
-   ctx->bl_dev = backlight_device_register(DRV_NAME "_backlight", dev, ctx,
+   ctx->bl_dev = backlight_device_register(dev_name(dev), dev, ctx,
_backlight_ops, NULL);
if (IS_ERR(ctx->bl_dev)) {
dev_err(dev, "failed to register backlight device\n");
@@ -483,11 +481,6 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
return ret;
}
 
-   DRM_INFO(DRV_NAME "_panel %ux%u@%u %ubpp dsi %udl - ready\n",
-default_mode.hdisplay, default_mode.vdisplay,
-default_mode.vrefresh,
-mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
-
return 0;
 }
 
@@ -513,7 +506,7 @@ static struct mipi_dsi_driver orisetech_otm8009a_driver = {
.probe  = otm8009a_probe,
.remove = otm8009a_remove,
.driver = {
-   .name = DRV_NAME "_panel",
+   .name = "panel-orisetech-otm8009a",
.of_match_table = orisetech_otm8009a_of_match,
},
 };
-- 
2.15.1

[PATCH 1/4] drm/panel: otm8009a: fix backlight updates

[Philippe Cornu]

Backlight updates was not working anymore since the good
implementation of the dsi lpm mode in the dsi host driver.
After a longer analysis, the backlight updates in dsi video
mode require the dsi hs mode.
Note: it is important to keep the dsi lpm mode for the rest
of the driver as init sequence, sleep in/out... dsi commands
work in lp mode.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 90f1ae4af93c..0fd2e0144d2b 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -98,6 +98,20 @@ static void otm8009a_dcs_write_buf(struct otm8009a *ctx, 
const void *data,
DRM_WARN("mipi dsi dcs write buffer failed\n");
 }
 
+static void otm8009a_dcs_write_buf_hs(struct otm8009a *ctx, const void *data,
+ size_t len)
+{
+   struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+   /* data will be sent in dsi hs mode (ie. no lpm) */
+   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+   otm8009a_dcs_write_buf(ctx, data, len);
+
+   /* restore back the dsi lpm mode */
+   dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+}
+
 #define dcs_write_seq(ctx, seq...) \
 ({ \
static const u8 d[] = { seq };  \
@@ -387,7 +401,7 @@ static int otm8009a_backlight_update_status(struct 
backlight_device *bd)
 */
data[0] = MIPI_DCS_SET_DISPLAY_BRIGHTNESS;
data[1] = bd->props.brightness;
-   otm8009a_dcs_write_buf(ctx, data, ARRAY_SIZE(data));
+   otm8009a_dcs_write_buf_hs(ctx, data, ARRAY_SIZE(data));
 
/* set Brightness Control & Backlight on */
data[1] = 0x24;
@@ -399,7 +413,7 @@ static int otm8009a_backlight_update_status(struct 
backlight_device *bd)
 
/* Update Brightness Control & Backlight */
data[0] = MIPI_DCS_WRITE_CONTROL_DISPLAY;
-   otm8009a_dcs_write_buf(ctx, data, ARRAY_SIZE(data));
+   otm8009a_dcs_write_buf_hs(ctx, data, ARRAY_SIZE(data));
 
return 0;
 }
-- 
2.15.1

[PATCH 4/4] drm/panel: otm8009a: use new backlight api

[Philippe Cornu]

Use the new backlight api.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 26 
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 4c638b7b9943..c2a71bd17e08 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -260,11 +260,7 @@ static int otm8009a_disable(struct drm_panel *panel)
if (!ctx->enabled)
return 0; /* This is not an issue so we return 0 here */
 
-   /* Power off the backlight. Note: end-user still controls brightness */
-   ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
-   ret = backlight_update_status(ctx->bl_dev);
-   if (ret)
-   return ret;
+   backlight_disable(ctx->bl_dev);
 
ret = mipi_dsi_dcs_set_display_off(dsi);
if (ret)
@@ -338,12 +334,7 @@ static int otm8009a_enable(struct drm_panel *panel)
if (ctx->enabled)
return 0;
 
-   /*
-* Power on the backlight. Note: end-user still controls brightness
-* Note: ctx->prepared must be true before updating the backlight.
-*/
-   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ctx->bl_dev);
+   backlight_enable(ctx->bl_dev);
 
ctx->enabled = true;
 
@@ -459,11 +450,14 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
ctx->panel.dev = dev;
ctx->panel.funcs = _drm_funcs;
 
-   ctx->bl_dev = backlight_device_register(dev_name(dev), dev, ctx,
-   _backlight_ops, NULL);
+   ctx->bl_dev = devm_backlight_device_register(dev, dev_name(dev),
+dsi->host->dev, ctx,
+_backlight_ops,
+NULL);
if (IS_ERR(ctx->bl_dev)) {
-   dev_err(dev, "failed to register backlight device\n");
-   return PTR_ERR(ctx->bl_dev);
+   ret = PTR_ERR(ctx->bl_dev);
+   dev_err(dev, "failed to register backlight %d\n", ret);
+   return ret;
}
 
ctx->bl_dev->props.max_brightness = OTM8009A_BACKLIGHT_MAX;
@@ -491,8 +485,6 @@ static int otm8009a_remove(struct mipi_dsi_device *dsi)
mipi_dsi_detach(dsi);
drm_panel_remove(>panel);
 
-   backlight_device_unregister(ctx->bl_dev);
-
return 0;
 }
 
-- 
2.15.1

[PATCH 2/4] drm/panel: otm8009a: fix glitches by moving backlight enable to otm8009a_enable()

[Philippe Cornu]

The backlight 1st update was in the otm8009a_prepare() function
for a bad reason: backlight was not working in video mode and the
otm8009a_prepare() is in command mode for the init sequence. As the
backlight is now fixed (no lpm), it is good to put it back in the
otm8009a_enable() function, avoiding also image glitches visible
on some "slow" devices.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 0fd2e0144d2b..de4a16d5275c 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -330,13 +330,6 @@ static int otm8009a_prepare(struct drm_panel *panel)
 
ctx->prepared = true;
 
-   /*
-* Power on the backlight. Note: end-user still controls brightness
-* Note: ctx->prepared must be true before updating the backlight.
-*/
-   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ctx->bl_dev);
-
return 0;
 }
 
@@ -344,6 +337,16 @@ static int otm8009a_enable(struct drm_panel *panel)
 {
struct otm8009a *ctx = panel_to_otm8009a(panel);
 
+   if (ctx->enabled)
+   return 0;
+
+   /*
+* Power on the backlight. Note: end-user still controls brightness
+* Note: ctx->prepared must be true before updating the backlight.
+*/
+   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
+   backlight_update_status(ctx->bl_dev);
+
ctx->enabled = true;
 
return 0;
-- 
2.15.1

[PATCH 4/4] drm/panel: otm8009a: use new backlight api

[Philippe Cornu]

Use the new backlight api.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 26 
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 4c638b7b9943..c2a71bd17e08 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -260,11 +260,7 @@ static int otm8009a_disable(struct drm_panel *panel)
if (!ctx->enabled)
return 0; /* This is not an issue so we return 0 here */
 
-   /* Power off the backlight. Note: end-user still controls brightness */
-   ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
-   ret = backlight_update_status(ctx->bl_dev);
-   if (ret)
-   return ret;
+   backlight_disable(ctx->bl_dev);
 
ret = mipi_dsi_dcs_set_display_off(dsi);
if (ret)
@@ -338,12 +334,7 @@ static int otm8009a_enable(struct drm_panel *panel)
if (ctx->enabled)
return 0;
 
-   /*
-* Power on the backlight. Note: end-user still controls brightness
-* Note: ctx->prepared must be true before updating the backlight.
-*/
-   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ctx->bl_dev);
+   backlight_enable(ctx->bl_dev);
 
ctx->enabled = true;
 
@@ -459,11 +450,14 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
ctx->panel.dev = dev;
ctx->panel.funcs = _drm_funcs;
 
-   ctx->bl_dev = backlight_device_register(dev_name(dev), dev, ctx,
-   _backlight_ops, NULL);
+   ctx->bl_dev = devm_backlight_device_register(dev, dev_name(dev),
+dsi->host->dev, ctx,
+_backlight_ops,
+NULL);
if (IS_ERR(ctx->bl_dev)) {
-   dev_err(dev, "failed to register backlight device\n");
-   return PTR_ERR(ctx->bl_dev);
+   ret = PTR_ERR(ctx->bl_dev);
+   dev_err(dev, "failed to register backlight %d\n", ret);
+   return ret;
}
 
ctx->bl_dev->props.max_brightness = OTM8009A_BACKLIGHT_MAX;
@@ -491,8 +485,6 @@ static int otm8009a_remove(struct mipi_dsi_device *dsi)
mipi_dsi_detach(dsi);
drm_panel_remove(>panel);
 
-   backlight_device_unregister(ctx->bl_dev);
-
return 0;
 }
 
-- 
2.15.1

[PATCH 2/4] drm/panel: otm8009a: fix glitches by moving backlight enable to otm8009a_enable()

[Philippe Cornu]

The backlight 1st update was in the otm8009a_prepare() function
for a bad reason: backlight was not working in video mode and the
otm8009a_prepare() is in command mode for the init sequence. As the
backlight is now fixed (no lpm), it is good to put it back in the
otm8009a_enable() function, avoiding also image glitches visible
on some "slow" devices.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 0fd2e0144d2b..de4a16d5275c 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -330,13 +330,6 @@ static int otm8009a_prepare(struct drm_panel *panel)
 
ctx->prepared = true;
 
-   /*
-* Power on the backlight. Note: end-user still controls brightness
-* Note: ctx->prepared must be true before updating the backlight.
-*/
-   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ctx->bl_dev);
-
return 0;
 }
 
@@ -344,6 +337,16 @@ static int otm8009a_enable(struct drm_panel *panel)
 {
struct otm8009a *ctx = panel_to_otm8009a(panel);
 
+   if (ctx->enabled)
+   return 0;
+
+   /*
+* Power on the backlight. Note: end-user still controls brightness
+* Note: ctx->prepared must be true before updating the backlight.
+*/
+   ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
+   backlight_update_status(ctx->bl_dev);
+
ctx->enabled = true;
 
return 0;
-- 
2.15.1

Re: [PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe CORNU]

Hi Archit & Andrzej,

May I ask you please a short review of this documentation update.
Many thanks
Philippe :-)

On 04/09/2018 05:24 PM, Philippe Cornu wrote:
> This patch clarifies the adjusted_mode documentation
> for bridges.
> 
> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> ---
> This patch follows discussions in:
> - "drm: clarify adjusted_mode for a bridge connected to a crtc"
>https://patchwork.freedesktop.org/patch/206801/
> - "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
>https://patchwork.freedesktop.org/patch/215449/
> 
>   include/drm/drm_bridge.h | 16 
>   include/drm/drm_crtc.h   | 11 +++
>   2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..7d632c6a9214 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -178,6 +178,22 @@ struct drm_bridge_funcs {
>* then this would be _encoder_helper_funcs.mode_set. The display
>* pipe (i.e.  clocks and timing signals) is off when this function is
>* called.
> +  *
> +  * The adjusted_mode parameter corresponds to the mode output by the 
> CRTC
> +  * for the first bridge in the chain. It can be different from the mode
> +  * parameter that contains the desired mode for the connector at the end
> +  * of the bridges chain, for instance when the first bridge in the chain
> +  * performs scaling. The adjusted mode is mostly useful for the first
> +  * bridge in the chain and is likely irrelevant for the other bridges.
> +  *
> +  * For atomic drivers the adjusted_mode is the mode stored in
> +  * _crtc_state.adjusted_mode.
> +  *
> +  * NOTE:
> +  *
> +  * If a need arises to store and access modes adjusted for other 
> locations
> +  * than the connection between the CRTC and the first bridge, the DRM
> +  * framework will have to be extended with DRM bridge states.
>*/
>   void (*mode_set)(struct drm_bridge *bridge,
>struct drm_display_mode *mode,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a2d81d2907a9..65f749a9e9d3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -134,10 +134,13 @@ struct drm_crtc_state {
>*
>* Internal display timings which can be used by the driver to handle
>* differences between the mode requested by userspace in @mode and what
> -  * is actually programmed into the hardware. It is purely driver
> -  * implementation defined what exactly this adjusted mode means. Usually
> -  * it is used to store the hardware display timings used between the
> -  * CRTC and encoder blocks.
> +  * is actually programmed into the hardware.
> +  *
> +  * For drivers using drm_bridge, this stores the hardware display 
> timings
> +  * used between the CRTC and the first bridge. For other drivers, the
> +  * meaning of the adjusted_mode field is purely driver implementation
> +  * defined information, and will usually be used to store the hardware
> +  * display timings used between the CRTC and encoder blocks.
>*/
>   struct drm_display_mode adjusted_mode;
>   
> 

Re: [PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe CORNU]

Hi Archit & Andrzej,

May I ask you please a short review of this documentation update.
Many thanks
Philippe :-)

On 04/09/2018 05:24 PM, Philippe Cornu wrote:
> This patch clarifies the adjusted_mode documentation
> for bridges.
> 
> Signed-off-by: Philippe Cornu 
> Signed-off-by: Laurent Pinchart 
> ---
> This patch follows discussions in:
> - "drm: clarify adjusted_mode for a bridge connected to a crtc"
>https://patchwork.freedesktop.org/patch/206801/
> - "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
>https://patchwork.freedesktop.org/patch/215449/
> 
>   include/drm/drm_bridge.h | 16 
>   include/drm/drm_crtc.h   | 11 +++
>   2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..7d632c6a9214 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -178,6 +178,22 @@ struct drm_bridge_funcs {
>* then this would be _encoder_helper_funcs.mode_set. The display
>* pipe (i.e.  clocks and timing signals) is off when this function is
>* called.
> +  *
> +  * The adjusted_mode parameter corresponds to the mode output by the 
> CRTC
> +  * for the first bridge in the chain. It can be different from the mode
> +  * parameter that contains the desired mode for the connector at the end
> +  * of the bridges chain, for instance when the first bridge in the chain
> +  * performs scaling. The adjusted mode is mostly useful for the first
> +  * bridge in the chain and is likely irrelevant for the other bridges.
> +  *
> +  * For atomic drivers the adjusted_mode is the mode stored in
> +  * _crtc_state.adjusted_mode.
> +  *
> +  * NOTE:
> +  *
> +  * If a need arises to store and access modes adjusted for other 
> locations
> +  * than the connection between the CRTC and the first bridge, the DRM
> +  * framework will have to be extended with DRM bridge states.
>*/
>   void (*mode_set)(struct drm_bridge *bridge,
>struct drm_display_mode *mode,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a2d81d2907a9..65f749a9e9d3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -134,10 +134,13 @@ struct drm_crtc_state {
>*
>* Internal display timings which can be used by the driver to handle
>* differences between the mode requested by userspace in @mode and what
> -  * is actually programmed into the hardware. It is purely driver
> -  * implementation defined what exactly this adjusted mode means. Usually
> -  * it is used to store the hardware display timings used between the
> -  * CRTC and encoder blocks.
> +  * is actually programmed into the hardware.
> +  *
> +  * For drivers using drm_bridge, this stores the hardware display 
> timings
> +  * used between the CRTC and the first bridge. For other drivers, the
> +  * meaning of the adjusted_mode field is purely driver implementation
> +  * defined information, and will usually be used to store the hardware
> +  * display timings used between the CRTC and encoder blocks.
>*/
>   struct drm_display_mode adjusted_mode;
>   
> 

[PATCH] drm/stm: ltdc: fix warnings in ltdc_plane_create()

[Philippe Cornu]

"make C=1" returns 2 warnings in ltdc_plane_create()
("Using plain integer as NULL pointer"). This patch
fixes them.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 616191fe98ae..d997a6014d6c 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -860,13 +860,13 @@ static struct drm_plane *ltdc_plane_create(struct 
drm_device *ddev,
 
plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
if (!plane)
-   return 0;
+   return NULL;
 
ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
   _plane_funcs, formats, nb_fmt,
   NULL, type, NULL);
if (ret < 0)
-   return 0;
+   return NULL;
 
drm_plane_helper_add(plane, _plane_helper_funcs);
 
-- 
2.15.1

[PATCH] drm/stm: ltdc: fix warnings in ltdc_plane_create()

[Philippe Cornu]

"make C=1" returns 2 warnings in ltdc_plane_create()
("Using plain integer as NULL pointer"). This patch
fixes them.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/stm/ltdc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 616191fe98ae..d997a6014d6c 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -860,13 +860,13 @@ static struct drm_plane *ltdc_plane_create(struct 
drm_device *ddev,
 
plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
if (!plane)
-   return 0;
+   return NULL;
 
ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
   _plane_funcs, formats, nb_fmt,
   NULL, type, NULL);
if (ret < 0)
-   return 0;
+   return NULL;
 
drm_plane_helper_add(plane, _plane_helper_funcs);
 
-- 
2.15.1

Re: [PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent,


On 04/19/2018 01:09 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> On Thursday, 19 April 2018 12:31:15 EEST Philippe CORNU wrote:
>> On 04/19/2018 10:11 AM, Laurent Pinchart wrote:
>>> On Tuesday, 10 April 2018 08:19:26 EEST Philippe Cornu wrote:
>>>
>>>> Add the 3 optional power supplies using the exact description
>>>> found in the document named
>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>
>>>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>>>> ---
>>>>
>>>>Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
>>>>1 file changed, 3 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>>>> 56a3e68ccb80..cf53678fe574 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> @@ -8,6 +8,9 @@ Optional properties:
>>>>- interrupts-extended or interrupt-parent + interrupts: describe
>>>>  the interrupt line used to inform the host about hotplug 
>>>> events.
>>>>- reset-gpios: OF device-tree gpio specification for RST_N pin.
>>>> +  - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>>>> +  - avcc12-supply: TMDS analog supply voltage (1.2V).
>>>> +  - cvcc12-supply: Digital core supply voltage (1.2V).
>>>
>>> It seems that the AVCC12 and CVCC12 power supplies are usually derived
>>> from the same source. How about starting with one DT property for both,
>>> and adding a second one later if needed ?
>>
>> Well, I do not know what is the best. Here I took the description from
>> the documentation, and to allow all possible board configurations, I
>> added these supplies as "optional" properties: if there is only one 1v2
>> regulator on the board, the dt will contain only avcc12 or cvcc12 and
>> everything will work fine (we will have a dummy regulator for the
>> missing optional 1v2 reg), if both regulators are there for any reasons
>> (stability, noise, whatever...) then both entries will be in the dt.
>>
>> If you confirm you prefer a single 1v2 supply (named for instance
>> "vcc12-supply") then I will do :-)
> 
> Please see https://lists.freedesktop.org/archives/dri-devel/2018-April/
> 172400.html (and the messages that lead to it) and https://
> lists.freedesktop.org/archives/dri-devel/2018-March/170763.html.
> 

Thanks for this discussion thread.

On my side, I found "CVCC12 and AVCC12 can be derived from the same 
power source" written with a small font (august 2016 datasheet p13) so 
then your advice is clearly what we have to do :-)
I will add this info in v2 too.
Thank you,
Philippe :-)

>>>>Optional subnodes:
>>>>- video input: this subnode can contain a video input port node
> 

Re: [PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent,


On 04/19/2018 01:09 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> On Thursday, 19 April 2018 12:31:15 EEST Philippe CORNU wrote:
>> On 04/19/2018 10:11 AM, Laurent Pinchart wrote:
>>> On Tuesday, 10 April 2018 08:19:26 EEST Philippe Cornu wrote:
>>>
>>>> Add the 3 optional power supplies using the exact description
>>>> found in the document named
>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>
>>>> Signed-off-by: Philippe Cornu 
>>>> ---
>>>>
>>>>Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
>>>>1 file changed, 3 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>>>> 56a3e68ccb80..cf53678fe574 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>>>> @@ -8,6 +8,9 @@ Optional properties:
>>>>- interrupts-extended or interrupt-parent + interrupts: describe
>>>>  the interrupt line used to inform the host about hotplug 
>>>> events.
>>>>- reset-gpios: OF device-tree gpio specification for RST_N pin.
>>>> +  - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>>>> +  - avcc12-supply: TMDS analog supply voltage (1.2V).
>>>> +  - cvcc12-supply: Digital core supply voltage (1.2V).
>>>
>>> It seems that the AVCC12 and CVCC12 power supplies are usually derived
>>> from the same source. How about starting with one DT property for both,
>>> and adding a second one later if needed ?
>>
>> Well, I do not know what is the best. Here I took the description from
>> the documentation, and to allow all possible board configurations, I
>> added these supplies as "optional" properties: if there is only one 1v2
>> regulator on the board, the dt will contain only avcc12 or cvcc12 and
>> everything will work fine (we will have a dummy regulator for the
>> missing optional 1v2 reg), if both regulators are there for any reasons
>> (stability, noise, whatever...) then both entries will be in the dt.
>>
>> If you confirm you prefer a single 1v2 supply (named for instance
>> "vcc12-supply") then I will do :-)
> 
> Please see https://lists.freedesktop.org/archives/dri-devel/2018-April/
> 172400.html (and the messages that lead to it) and https://
> lists.freedesktop.org/archives/dri-devel/2018-March/170763.html.
> 

Thanks for this discussion thread.

On my side, I found "CVCC12 and AVCC12 can be derived from the same 
power source" written with a small font (august 2016 datasheet p13) so 
then your advice is clearly what we have to do :-)
I will add this info in v2 too.
Thank you,
Philippe :-)

>>>>Optional subnodes:
>>>>- video input: this subnode can contain a video input port node
> 

Re: [PATCH v2] drm/bridge/synopsys: dsi: Adopt SPDX identifiers

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 02/08/2018 05:46 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Thursday, 8 February 2018 16:58:05 EET Philippe Cornu wrote:
>> Add SPDX identifiers to the Synopsys DesignWare MIPI DSI
>> host controller driver.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
>> ---
>> Changes in v2: Update to "GPL-2.0+" following comments from Laurent
>> Pinchart, Benjamin Gaignard & Philippe Ombredanne.
>>
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 +-
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index
>> 7bac101c285c..99f0e4f51716 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> @@ -1,12 +1,8 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>>   /*
>>* Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>>* Copyright (C) STMicroelectronics SA 2017
>>*
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation; either version 2 of the License, or
>> - * (at your option) any later version.
>> - *
>>* Modified by Philippe Cornu <philippe.co...@st.com>
>>* This generic Synopsys DesignWare MIPI DSI host driver is based on the
>>* Rockchip version from rockchip/dw-mipi-dsi.c with phy & bridge APIs.
> 
> 

Re: [PATCH v2] drm/bridge/synopsys: dsi: Adopt SPDX identifiers

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 02/08/2018 05:46 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Thursday, 8 February 2018 16:58:05 EET Philippe Cornu wrote:
>> Add SPDX identifiers to the Synopsys DesignWare MIPI DSI
>> host controller driver.
>>
>> Signed-off-by: Philippe Cornu 
> 
> Reviewed-by: Laurent Pinchart 
> 
>> ---
>> Changes in v2: Update to "GPL-2.0+" following comments from Laurent
>> Pinchart, Benjamin Gaignard & Philippe Ombredanne.
>>
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 +-
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index
>> 7bac101c285c..99f0e4f51716 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> @@ -1,12 +1,8 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>>   /*
>>* Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>>* Copyright (C) STMicroelectronics SA 2017
>>*
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation; either version 2 of the License, or
>> - * (at your option) any later version.
>> - *
>>* Modified by Philippe Cornu 
>>* This generic Synopsys DesignWare MIPI DSI host driver is based on the
>>* Rockchip version from rockchip/dw-mipi-dsi.c with phy & bridge APIs.
> 
> 

Re: [PATCH] drm/stm: ltdc: fix warning in ltdc_crtc_update_clut()

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:18 AM, Yannick FERTRE wrote:
> Reviewed-by: yannick fertre <yannick.fer...@st.com>
> 
> 
> On 04/10/2018 03:53 PM, Philippe Cornu wrote:
>> Fix the warning
>> "warn: variable dereferenced before check 'crtc' (see line 390)"
>> by removing unnecessary checks as ltdc_crtc_update_clut() is
>> only called from ltdc_crtc_atomic_flush() where crtc and
>> crtc->state are not NULL.
>>
>> Many thanks to Dan Carpenter for the bug report
>> https://lists.freedesktop.org/archives/dri-devel/2018-February/166918.html
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 3 ---
>>1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 061d2b6e5157..e3121d9e4230 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -392,9 +392,6 @@ static void ltdc_crtc_update_clut(struct drm_crtc *crtc)
>>  u32 val;
>>  int i;
>>
>> -if (!crtc || !crtc->state)
>> -return;
>> -
>>  if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut)
>>  return;
>>

Re: [PATCH] drm/stm: ltdc: fix warning in ltdc_crtc_update_clut()

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:18 AM, Yannick FERTRE wrote:
> Reviewed-by: yannick fertre 
> 
> 
> On 04/10/2018 03:53 PM, Philippe Cornu wrote:
>> Fix the warning
>> "warn: variable dereferenced before check 'crtc' (see line 390)"
>> by removing unnecessary checks as ltdc_crtc_update_clut() is
>> only called from ltdc_crtc_atomic_flush() where crtc and
>> crtc->state are not NULL.
>>
>> Many thanks to Dan Carpenter for the bug report
>> https://lists.freedesktop.org/archives/dri-devel/2018-February/166918.html
>>
>> Signed-off-by: Philippe Cornu 
>> Reported-by: Dan Carpenter 
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 3 ---
>>1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 061d2b6e5157..e3121d9e4230 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -392,9 +392,6 @@ static void ltdc_crtc_update_clut(struct drm_crtc *crtc)
>>  u32 val;
>>  int i;
>>
>> -if (!crtc || !crtc->state)
>> -return;
>> -
>>  if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut)
>>  return;
>>

Re: [PATCH] drm/stm: move enable/disable_vblank to crtc

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:07 AM, Vincent ABRIOU wrote:
> Hi Philippe,
> 
> Patch looks good to me.
> 
> Reviewed-by: Vincent Abriou <vincent.abr...@st.com>
> 
> On 04/07/2018 11:29 PM, Philippe Cornu wrote:
>> enable/disable_vblank() functions at drm_driver level
>> are deprecated. Move them to the ltdc drm_crtc_funcs
>> structure.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>drivers/gpu/drm/stm/drv.c  |  2 --
>>drivers/gpu/drm/stm/ltdc.c | 10 ++
>>drivers/gpu/drm/stm/ltdc.h |  2 --
>>3 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>> index 9ab00a87f7cc..8698e08313e1 100644
>> --- a/drivers/gpu/drm/stm/drv.c
>> +++ b/drivers/gpu/drm/stm/drv.c
>> @@ -72,8 +72,6 @@ static struct drm_driver drv_driver = {
>>  .gem_prime_vmap = drm_gem_cma_prime_vmap,
>>  .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>>  .gem_prime_mmap = drm_gem_cma_prime_mmap,
>> -.enable_vblank = ltdc_crtc_enable_vblank,
>> -.disable_vblank = ltdc_crtc_disable_vblank,
>>};
>>
>>static int drv_load(struct drm_device *ddev)
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 1a3277e483d5..2b745cfc9000 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -569,9 +569,9 @@ static const struct drm_crtc_helper_funcs 
>> ltdc_crtc_helper_funcs = {
>>  .atomic_disable = ltdc_crtc_atomic_disable,
>>};
>>
>> -int ltdc_crtc_enable_vblank(struct drm_device *ddev, unsigned int pipe)
>> +static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
>>{
>> -struct ltdc_device *ldev = ddev->dev_private;
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>>
>>  DRM_DEBUG_DRIVER("\n");
>>  reg_set(ldev->regs, LTDC_IER, IER_LIE);
>> @@ -579,9 +579,9 @@ int ltdc_crtc_enable_vblank(struct drm_device *ddev, 
>> unsigned int pipe)
>>  return 0;
>>}
>>
>> -void ltdc_crtc_disable_vblank(struct drm_device *ddev, unsigned int pipe)
>> +static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
>>{
>> -struct ltdc_device *ldev = ddev->dev_private;
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>>
>>  DRM_DEBUG_DRIVER("\n");
>>  reg_clear(ldev->regs, LTDC_IER, IER_LIE);
>> @@ -594,6 +594,8 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>>  .reset = drm_atomic_helper_crtc_reset,
>>  .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>  .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +.enable_vblank = ltdc_crtc_enable_vblank,
>> +.disable_vblank = ltdc_crtc_disable_vblank,
>>  .gamma_set = drm_atomic_helper_legacy_gamma_set,
>>};
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
>> index edb268129c54..61a80d00bc3b 100644
>> --- a/drivers/gpu/drm/stm/ltdc.h
>> +++ b/drivers/gpu/drm/stm/ltdc.h
>> @@ -29,8 +29,6 @@ struct ltdc_device {
>>  u32 irq_status;
>>};
>>
>> -int ltdc_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
>> -void ltdc_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
>>int ltdc_load(struct drm_device *ddev);
>>void ltdc_unload(struct drm_device *ddev);
>>

Re: [PATCH] drm/stm: ltdc: add user update info in plane print state

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:14 AM, Vincent ABRIOU wrote:
> Hi Philippe,
> 
> Reviewed-by: Vincent Abriou <vincent.abr...@st.com>
> 
> On 04/07/2018 11:35 PM, Philippe Cornu wrote:
>> This patch adds the user update information in
>> frames-per-second into the drm debugfs plane state.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 22 ++
>>drivers/gpu/drm/stm/ltdc.h |  8 
>>2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 2b745cfc9000..061d2b6e5157 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -729,6 +729,8 @@ static void ltdc_plane_atomic_update(struct drm_plane 
>> *plane,
>>  reg_update_bits(ldev->regs, LTDC_L1CR + lofs,
>>  LXCR_LEN | LXCR_CLUTEN, val);
>>
>> +ldev->plane_fpsi[plane->index].counter++;
>> +
>>  mutex_lock(>err_lock);
>>  if (ldev->error_status & ISR_FUIF) {
>>  DRM_DEBUG_DRIVER("Fifo underrun\n");
>> @@ -754,6 +756,25 @@ static void ltdc_plane_atomic_disable(struct drm_plane 
>> *plane,
>>   oldstate->crtc->base.id, plane->base.id);
>>}
>>
>> +static void ltdc_plane_atomic_print_state(struct drm_printer *p,
>> +  const struct drm_plane_state *state)
>> +{
>> +struct drm_plane *plane = state->plane;
>> +struct ltdc_device *ldev = plane_to_ltdc(plane);
>> +struct fps_info *fpsi = >plane_fpsi[plane->index];
>> +int ms_since_last;
>> +ktime_t now;
>> +
>> +now = ktime_get();
>> +ms_since_last = ktime_to_ms(ktime_sub(now, fpsi->last_timestamp));
>> +
>> +drm_printf(p, "\tuser_updates=%dfps\n",
>> +   DIV_ROUND_CLOSEST(fpsi->counter * 1000, ms_since_last));
>> +
>> +fpsi->last_timestamp = now;
>> +fpsi->counter = 0;
>> +}
>> +
>>static const struct drm_plane_funcs ltdc_plane_funcs = {
>>  .update_plane = drm_atomic_helper_update_plane,
>>  .disable_plane = drm_atomic_helper_disable_plane,
>> @@ -761,6 +782,7 @@ static const struct drm_plane_funcs ltdc_plane_funcs = {
>>  .reset = drm_atomic_helper_plane_reset,
>>  .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>  .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> +.atomic_print_state = ltdc_plane_atomic_print_state,
>>};
>>
>>static const struct drm_plane_helper_funcs ltdc_plane_helper_funcs = {
>> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
>> index 61a80d00bc3b..1e16d6afb0d2 100644
>> --- a/drivers/gpu/drm/stm/ltdc.h
>> +++ b/drivers/gpu/drm/stm/ltdc.h
>> @@ -20,6 +20,13 @@ struct ltdc_caps {
>>  bool non_alpha_only_l1; /* non-native no-alpha formats on layer 1 */
>>};
>>
>> +#define LTDC_MAX_LAYER  4
>> +
>> +struct fps_info {
>> +unsigned int counter;
>> +ktime_t last_timestamp;
>> +};
>> +
>>struct ltdc_device {
>>  void __iomem *regs;
>>  struct clk *pixel_clk;  /* lcd pixel clock */
>> @@ -27,6 +34,7 @@ struct ltdc_device {
>>  struct ltdc_caps caps;
>>  u32 error_status;
>>  u32 irq_status;
>> +struct fps_info plane_fpsi[LTDC_MAX_LAYER];
>>};
>>
>>int ltdc_load(struct drm_device *ddev);

Re: [PATCH] drm/stm: move enable/disable_vblank to crtc

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:07 AM, Vincent ABRIOU wrote:
> Hi Philippe,
> 
> Patch looks good to me.
> 
> Reviewed-by: Vincent Abriou 
> 
> On 04/07/2018 11:29 PM, Philippe Cornu wrote:
>> enable/disable_vblank() functions at drm_driver level
>> are deprecated. Move them to the ltdc drm_crtc_funcs
>> structure.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>drivers/gpu/drm/stm/drv.c  |  2 --
>>drivers/gpu/drm/stm/ltdc.c | 10 ++
>>drivers/gpu/drm/stm/ltdc.h |  2 --
>>3 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>> index 9ab00a87f7cc..8698e08313e1 100644
>> --- a/drivers/gpu/drm/stm/drv.c
>> +++ b/drivers/gpu/drm/stm/drv.c
>> @@ -72,8 +72,6 @@ static struct drm_driver drv_driver = {
>>  .gem_prime_vmap = drm_gem_cma_prime_vmap,
>>  .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>>  .gem_prime_mmap = drm_gem_cma_prime_mmap,
>> -.enable_vblank = ltdc_crtc_enable_vblank,
>> -.disable_vblank = ltdc_crtc_disable_vblank,
>>};
>>
>>static int drv_load(struct drm_device *ddev)
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 1a3277e483d5..2b745cfc9000 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -569,9 +569,9 @@ static const struct drm_crtc_helper_funcs 
>> ltdc_crtc_helper_funcs = {
>>  .atomic_disable = ltdc_crtc_atomic_disable,
>>};
>>
>> -int ltdc_crtc_enable_vblank(struct drm_device *ddev, unsigned int pipe)
>> +static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
>>{
>> -struct ltdc_device *ldev = ddev->dev_private;
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>>
>>  DRM_DEBUG_DRIVER("\n");
>>  reg_set(ldev->regs, LTDC_IER, IER_LIE);
>> @@ -579,9 +579,9 @@ int ltdc_crtc_enable_vblank(struct drm_device *ddev, 
>> unsigned int pipe)
>>  return 0;
>>}
>>
>> -void ltdc_crtc_disable_vblank(struct drm_device *ddev, unsigned int pipe)
>> +static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
>>{
>> -struct ltdc_device *ldev = ddev->dev_private;
>> +struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>>
>>  DRM_DEBUG_DRIVER("\n");
>>  reg_clear(ldev->regs, LTDC_IER, IER_LIE);
>> @@ -594,6 +594,8 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>>  .reset = drm_atomic_helper_crtc_reset,
>>  .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>  .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +.enable_vblank = ltdc_crtc_enable_vblank,
>> +.disable_vblank = ltdc_crtc_disable_vblank,
>>  .gamma_set = drm_atomic_helper_legacy_gamma_set,
>>};
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
>> index edb268129c54..61a80d00bc3b 100644
>> --- a/drivers/gpu/drm/stm/ltdc.h
>> +++ b/drivers/gpu/drm/stm/ltdc.h
>> @@ -29,8 +29,6 @@ struct ltdc_device {
>>  u32 irq_status;
>>};
>>
>> -int ltdc_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
>> -void ltdc_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
>>int ltdc_load(struct drm_device *ddev);
>>void ltdc_unload(struct drm_device *ddev);
>>

Re: [PATCH] drm/stm: ltdc: add user update info in plane print state

[Philippe CORNU]

Applied on drm-misc-next.
Many thanks,
Philippe :-)

On 04/16/2018 11:14 AM, Vincent ABRIOU wrote:
> Hi Philippe,
> 
> Reviewed-by: Vincent Abriou 
> 
> On 04/07/2018 11:35 PM, Philippe Cornu wrote:
>> This patch adds the user update information in
>> frames-per-second into the drm debugfs plane state.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>drivers/gpu/drm/stm/ltdc.c | 22 ++
>>drivers/gpu/drm/stm/ltdc.h |  8 
>>2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>> index 2b745cfc9000..061d2b6e5157 100644
>> --- a/drivers/gpu/drm/stm/ltdc.c
>> +++ b/drivers/gpu/drm/stm/ltdc.c
>> @@ -729,6 +729,8 @@ static void ltdc_plane_atomic_update(struct drm_plane 
>> *plane,
>>  reg_update_bits(ldev->regs, LTDC_L1CR + lofs,
>>  LXCR_LEN | LXCR_CLUTEN, val);
>>
>> +ldev->plane_fpsi[plane->index].counter++;
>> +
>>  mutex_lock(>err_lock);
>>  if (ldev->error_status & ISR_FUIF) {
>>  DRM_DEBUG_DRIVER("Fifo underrun\n");
>> @@ -754,6 +756,25 @@ static void ltdc_plane_atomic_disable(struct drm_plane 
>> *plane,
>>   oldstate->crtc->base.id, plane->base.id);
>>}
>>
>> +static void ltdc_plane_atomic_print_state(struct drm_printer *p,
>> +  const struct drm_plane_state *state)
>> +{
>> +struct drm_plane *plane = state->plane;
>> +struct ltdc_device *ldev = plane_to_ltdc(plane);
>> +struct fps_info *fpsi = >plane_fpsi[plane->index];
>> +int ms_since_last;
>> +ktime_t now;
>> +
>> +now = ktime_get();
>> +ms_since_last = ktime_to_ms(ktime_sub(now, fpsi->last_timestamp));
>> +
>> +drm_printf(p, "\tuser_updates=%dfps\n",
>> +   DIV_ROUND_CLOSEST(fpsi->counter * 1000, ms_since_last));
>> +
>> +fpsi->last_timestamp = now;
>> +fpsi->counter = 0;
>> +}
>> +
>>static const struct drm_plane_funcs ltdc_plane_funcs = {
>>  .update_plane = drm_atomic_helper_update_plane,
>>  .disable_plane = drm_atomic_helper_disable_plane,
>> @@ -761,6 +782,7 @@ static const struct drm_plane_funcs ltdc_plane_funcs = {
>>  .reset = drm_atomic_helper_plane_reset,
>>  .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>  .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> +.atomic_print_state = ltdc_plane_atomic_print_state,
>>};
>>
>>static const struct drm_plane_helper_funcs ltdc_plane_helper_funcs = {
>> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
>> index 61a80d00bc3b..1e16d6afb0d2 100644
>> --- a/drivers/gpu/drm/stm/ltdc.h
>> +++ b/drivers/gpu/drm/stm/ltdc.h
>> @@ -20,6 +20,13 @@ struct ltdc_caps {
>>  bool non_alpha_only_l1; /* non-native no-alpha formats on layer 1 */
>>};
>>
>> +#define LTDC_MAX_LAYER  4
>> +
>> +struct fps_info {
>> +unsigned int counter;
>> +ktime_t last_timestamp;
>> +};
>> +
>>struct ltdc_device {
>>  void __iomem *regs;
>>  struct clk *pixel_clk;  /* lcd pixel clock */
>> @@ -27,6 +34,7 @@ struct ltdc_device {
>>  struct ltdc_caps caps;
>>  u32 error_status;
>>  u32 irq_status;
>> +struct fps_info plane_fpsi[LTDC_MAX_LAYER];
>>};
>>
>>int ltdc_load(struct drm_device *ddev);

Re: [PATCH 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent :-)

On 04/19/2018 10:20 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Tuesday, 10 April 2018 08:19:27 EEST Philippe Cornu wrote:
>> Add the 3 optional power supplies using the exact description
>> found in the document named
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>> drivers/gpu/drm/bridge/sii902x.c | 39 +
>> 1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c
>> b/drivers/gpu/drm/bridge/sii902x.c index 60373d7eb220..e17ba6db1ec8 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -24,6 +24,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>
>>   #include 
>>   #include 
>> @@ -86,6 +87,7 @@ struct sii902x {
>>  struct drm_bridge bridge;
>>  struct drm_connector connector;
>>  struct gpio_desc *reset_gpio;
>> +struct regulator_bulk_data supplies[3];
>>   };
>>
>>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>> @@ -392,23 +394,43 @@ static int sii902x_probe(struct i2c_client *client,
>>  return PTR_ERR(sii902x->reset_gpio);
>>  }
>>
>> +sii902x->supplies[0].supply = "iovcc";
>> +sii902x->supplies[1].supply = "avcc12";
>> +sii902x->supplies[2].supply = "cvcc12";
>> +ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>> +  sii902x->supplies);
>> +if (ret) {
>> +dev_err(dev, "regulator_bulk_get failed\n");
> 
> Maybe "failed to get power supplies" to be a bit more explicit ? And while at
> it, printing the value of ret too ?
> 

good point, I will do that in v2


>> +return ret;
>> +}
>> +
>> +ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>> +sii902x->supplies);
>> +if (ret) {
>> +dev_err(dev, "regulator_bulk_enable failed\n");
> 
> Same here ?
> 

agreed

>> +return ret;
>> +}
>> +
>> +usleep_range(1, 2);
>> +
>>  sii902x_reset(sii902x);
>>
>>  ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>  if (ret)
>> -return ret;
>> +goto err_disable_regulator;
>>
>>  ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>> , 4);
>>  if (ret) {
>>  dev_err(dev, "regmap_read failed %d\n", ret);
>> -return ret;
>> +goto err_disable_regulator;
>>  }
>>
>>  if (chipid[0] != 0xb0) {
>>  dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>  chipid[0]);
>> -return -EINVAL;
>> +ret = -EINVAL;
>> +goto err_disable_regulator;
>>  }
>>
>>  /* Clear all pending interrupts */
>> @@ -424,7 +446,7 @@ static int sii902x_probe(struct i2c_client *client,
>>  IRQF_ONESHOT, dev_name(dev),
>>  sii902x);
>>  if (ret)
>> -return ret;
>> +goto err_disable_regulator;
>>  }
>>
>>  sii902x->bridge.funcs = _bridge_funcs;
>> @@ -434,6 +456,12 @@ static int sii902x_probe(struct i2c_client *client,
>>  i2c_set_clientdata(client, sii902x);
>>
>>  return 0;
>> +
>> +err_disable_regulator:
>> +regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +   sii902x->supplies);
>> +
>> +return ret;
>>   }
>>
>>   static int sii902x_remove(struct i2c_client *client)
>> @@ -443,6 +471,9 @@ static int sii902x_remove(struct i2c_client *client)
>>
>>  drm_bridge_remove(>bridge);
>>
>> +regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +   sii902x->supplies);
>> +
> 
> While this seems functionally correct, would it be useful to only enable power
> supplies when needed to save power ?
> 

that is a good point. I do not know well (yet) this bridge. Maybe I can 
add a 3rd patch with bridge pre_enable() and post_disable() containing 
reset & supplies management. Or I can put reset in bridge 
enable() & disable() but it could be a little messy.

Any opinion/advice?
Many thanks,
Philippe :-)

>>  return 0;
>>   }
> 

Re: [PATCH 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent :-)

On 04/19/2018 10:20 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Tuesday, 10 April 2018 08:19:27 EEST Philippe Cornu wrote:
>> Add the 3 optional power supplies using the exact description
>> found in the document named
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>> drivers/gpu/drm/bridge/sii902x.c | 39 +
>> 1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c
>> b/drivers/gpu/drm/bridge/sii902x.c index 60373d7eb220..e17ba6db1ec8 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -24,6 +24,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>
>>   #include 
>>   #include 
>> @@ -86,6 +87,7 @@ struct sii902x {
>>  struct drm_bridge bridge;
>>  struct drm_connector connector;
>>  struct gpio_desc *reset_gpio;
>> +struct regulator_bulk_data supplies[3];
>>   };
>>
>>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>> @@ -392,23 +394,43 @@ static int sii902x_probe(struct i2c_client *client,
>>  return PTR_ERR(sii902x->reset_gpio);
>>  }
>>
>> +sii902x->supplies[0].supply = "iovcc";
>> +sii902x->supplies[1].supply = "avcc12";
>> +sii902x->supplies[2].supply = "cvcc12";
>> +ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>> +  sii902x->supplies);
>> +if (ret) {
>> +dev_err(dev, "regulator_bulk_get failed\n");
> 
> Maybe "failed to get power supplies" to be a bit more explicit ? And while at
> it, printing the value of ret too ?
> 

good point, I will do that in v2


>> +return ret;
>> +}
>> +
>> +ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>> +sii902x->supplies);
>> +if (ret) {
>> +dev_err(dev, "regulator_bulk_enable failed\n");
> 
> Same here ?
> 

agreed

>> +return ret;
>> +}
>> +
>> +usleep_range(1, 2);
>> +
>>  sii902x_reset(sii902x);
>>
>>  ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>  if (ret)
>> -return ret;
>> +goto err_disable_regulator;
>>
>>  ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>> , 4);
>>  if (ret) {
>>  dev_err(dev, "regmap_read failed %d\n", ret);
>> -return ret;
>> +goto err_disable_regulator;
>>  }
>>
>>  if (chipid[0] != 0xb0) {
>>  dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>  chipid[0]);
>> -return -EINVAL;
>> +ret = -EINVAL;
>> +goto err_disable_regulator;
>>  }
>>
>>  /* Clear all pending interrupts */
>> @@ -424,7 +446,7 @@ static int sii902x_probe(struct i2c_client *client,
>>  IRQF_ONESHOT, dev_name(dev),
>>  sii902x);
>>  if (ret)
>> -return ret;
>> +goto err_disable_regulator;
>>  }
>>
>>  sii902x->bridge.funcs = _bridge_funcs;
>> @@ -434,6 +456,12 @@ static int sii902x_probe(struct i2c_client *client,
>>  i2c_set_clientdata(client, sii902x);
>>
>>  return 0;
>> +
>> +err_disable_regulator:
>> +regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +   sii902x->supplies);
>> +
>> +return ret;
>>   }
>>
>>   static int sii902x_remove(struct i2c_client *client)
>> @@ -443,6 +471,9 @@ static int sii902x_remove(struct i2c_client *client)
>>
>>  drm_bridge_remove(>bridge);
>>
>> +regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +   sii902x->supplies);
>> +
> 
> While this seems functionally correct, would it be useful to only enable power
> supplies when needed to save power ?
> 

that is a good point. I do not know well (yet) this bridge. Maybe I can 
add a 3rd patch with bridge pre_enable() and post_disable() containing 
reset & supplies management. Or I can put reset in bridge 
enable() & disable() but it could be a little messy.

Any opinion/advice?
Many thanks,
Philippe :-)

>>  return 0;
>>   }
> 

Re: [PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent : )

On 04/19/2018 10:11 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Tuesday, 10 April 2018 08:19:26 EEST Philippe Cornu wrote:
>> Add the 3 optional power supplies using the exact description
>> found in the document named
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..cf53678fe574 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,9 @@ Optional properties:
>>  - interrupts-extended or interrupt-parent + interrupts: describe
>>the interrupt line used to inform the host about hotplug events.
>>  - reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +- avcc12-supply: TMDS analog supply voltage (1.2V).
>> +- cvcc12-supply: Digital core supply voltage (1.2V).
> 
> It seems that the AVCC12 and CVCC12 power supplies are usually derived from
> the same source. How about starting with one DT property for both, and adding
> a second one later if needed ?
> 

Well, I do not know what is the best. Here I took the description from 
the documentation, and to allow all possible board configurations, I 
added these supplies as "optional" properties: if there is only one 1v2 
regulator on the board, the dt will contain only avcc12 or cvcc12 and 
everything will work fine (we will have a dummy regulator for the 
missing optional 1v2 reg), if both regulators are there for any reasons 
(stability, noise, whatever...) then both entries will be in the dt.

If you confirm you prefer a single 1v2 supply (named for instance 
"vcc12-supply") then I will do :-)

Many thanks,
Philippe


>>   Optional subnodes:
>>  - video input: this subnode can contain a video input port node
> 

Re: [PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent : )

On 04/19/2018 10:11 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Tuesday, 10 April 2018 08:19:26 EEST Philippe Cornu wrote:
>> Add the 3 optional power supplies using the exact description
>> found in the document named
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..cf53678fe574 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,9 @@ Optional properties:
>>  - interrupts-extended or interrupt-parent + interrupts: describe
>>the interrupt line used to inform the host about hotplug events.
>>  - reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +- avcc12-supply: TMDS analog supply voltage (1.2V).
>> +- cvcc12-supply: Digital core supply voltage (1.2V).
> 
> It seems that the AVCC12 and CVCC12 power supplies are usually derived from
> the same source. How about starting with one DT property for both, and adding
> a second one later if needed ?
> 

Well, I do not know what is the best. Here I took the description from 
the documentation, and to allow all possible board configurations, I 
added these supplies as "optional" properties: if there is only one 1v2 
regulator on the board, the dt will contain only avcc12 or cvcc12 and 
everything will work fine (we will have a dummy regulator for the 
missing optional 1v2 reg), if both regulators are there for any reasons 
(stability, noise, whatever...) then both entries will be in the dt.

If you confirm you prefer a single 1v2 supply (named for instance 
"vcc12-supply") then I will do :-)

Many thanks,
Philippe


>>   Optional subnodes:
>>  - video input: this subnode can contain a video input port node
> 

[PATCH] drm/stm: ltdc: add mode_valid()

[Philippe Cornu]

Add mode_valid() function to filter modes according to available
pll clock values and "preferred" modes. It is particularly
useful for hdmi modes that require precise pixel clocks.

Note that "preferred" modes are always accepted:
- this is important for panels because panel clock tolerances are
  bigger than hdmi ones and there is no reason to not accept them
  (the fps may vary a little but it is not a problem).
- the hdmi preferred mode will be accepted too, but userland will
  be able to use others hdmi "valid" modes if necessary.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 014cef8cef37..616191fe98ae 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -445,6 +445,43 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc *crtc,
reg_set(ldev->regs, LTDC_SRCR, SRCR_IMR);
 }
 
+#define CLK_TOLERANCE_HZ 50
+
+static enum drm_mode_status
+ltdc_crtc_mode_valid(struct drm_crtc *crtc,
+const struct drm_display_mode *mode)
+{
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+   int target = mode->clock * 1000;
+   int target_min = target - CLK_TOLERANCE_HZ;
+   int target_max = target + CLK_TOLERANCE_HZ;
+   int result;
+
+   /*
+* Accept all "preferred" modes:
+* - this is important for panels because panel clock tolerances are
+*   bigger than hdmi ones and there is no reason to not accept them
+*   (the fps may vary a little but it is not a problem).
+* - the hdmi preferred mode will be accepted too, but userland will
+*   be able to use others hdmi "valid" modes if necessary.
+*/
+   if (mode->type & DRM_MODE_TYPE_PREFERRED)
+   return MODE_OK;
+
+   result = clk_round_rate(ldev->pixel_clk, target);
+
+   DRM_DEBUG_DRIVER("clk rate target %d, available %d\n", target, result);
+
+   /*
+* Filter modes according to the clock value, particularly useful for
+* hdmi modes that require precise pixel clocks.
+*/
+   if (result < target_min || result > target_max)
+   return MODE_CLOCK_RANGE;
+
+   return MODE_OK;
+}
+
 static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
 const struct drm_display_mode *mode,
 struct drm_display_mode *adjusted_mode)
@@ -559,6 +596,7 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
+   .mode_valid = ltdc_crtc_mode_valid,
.mode_fixup = ltdc_crtc_mode_fixup,
.mode_set_nofb = ltdc_crtc_mode_set_nofb,
.atomic_flush = ltdc_crtc_atomic_flush,
-- 
2.15.1

[PATCH] drm/stm: ltdc: add mode_valid()

[Philippe Cornu]

Add mode_valid() function to filter modes according to available
pll clock values and "preferred" modes. It is particularly
useful for hdmi modes that require precise pixel clocks.

Note that "preferred" modes are always accepted:
- this is important for panels because panel clock tolerances are
  bigger than hdmi ones and there is no reason to not accept them
  (the fps may vary a little but it is not a problem).
- the hdmi preferred mode will be accepted too, but userland will
  be able to use others hdmi "valid" modes if necessary.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/stm/ltdc.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 014cef8cef37..616191fe98ae 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -445,6 +445,43 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc *crtc,
reg_set(ldev->regs, LTDC_SRCR, SRCR_IMR);
 }
 
+#define CLK_TOLERANCE_HZ 50
+
+static enum drm_mode_status
+ltdc_crtc_mode_valid(struct drm_crtc *crtc,
+const struct drm_display_mode *mode)
+{
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+   int target = mode->clock * 1000;
+   int target_min = target - CLK_TOLERANCE_HZ;
+   int target_max = target + CLK_TOLERANCE_HZ;
+   int result;
+
+   /*
+* Accept all "preferred" modes:
+* - this is important for panels because panel clock tolerances are
+*   bigger than hdmi ones and there is no reason to not accept them
+*   (the fps may vary a little but it is not a problem).
+* - the hdmi preferred mode will be accepted too, but userland will
+*   be able to use others hdmi "valid" modes if necessary.
+*/
+   if (mode->type & DRM_MODE_TYPE_PREFERRED)
+   return MODE_OK;
+
+   result = clk_round_rate(ldev->pixel_clk, target);
+
+   DRM_DEBUG_DRIVER("clk rate target %d, available %d\n", target, result);
+
+   /*
+* Filter modes according to the clock value, particularly useful for
+* hdmi modes that require precise pixel clocks.
+*/
+   if (result < target_min || result > target_max)
+   return MODE_CLOCK_RANGE;
+
+   return MODE_OK;
+}
+
 static bool ltdc_crtc_mode_fixup(struct drm_crtc *crtc,
 const struct drm_display_mode *mode,
 struct drm_display_mode *adjusted_mode)
@@ -559,6 +596,7 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
+   .mode_valid = ltdc_crtc_mode_valid,
.mode_fixup = ltdc_crtc_mode_fixup,
.mode_set_nofb = ltdc_crtc_mode_set_nofb,
.atomic_flush = ltdc_crtc_atomic_flush,
-- 
2.15.1

[PATCH] drm/stm: ltdc: fix deferred endpoint management

[Philippe Cornu]

When a driver related to one of the endpoints is deferred
due to probe dependencies (i2c, spi...) but the other one
is ready, ltdc probe continues and the deferred driver
will never be probed again.

The fix consists in waiting for all deferred endpoints before
continuing the ltdc probe.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index e3121d9e4230..014cef8cef37 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -987,14 +987,13 @@ int ltdc_load(struct drm_device *ddev)
  [i]);
 
/*
-* If at least one endpoint is ready, continue probing,
-* else if at least one endpoint is -EPROBE_DEFER and
-* there is no previous ready endpoints, defer probing.
+* If at least one endpoint is -EPROBE_DEFER, defer probing,
+* else if at least one endpoint is ready, continue probing.
 */
-   if (!ret)
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   else if (!ret)
endpoint_not_ready = 0;
-   else if (ret == -EPROBE_DEFER && endpoint_not_ready)
-   endpoint_not_ready = -EPROBE_DEFER;
}
 
if (endpoint_not_ready)
-- 
2.15.1

[PATCH] drm/stm: ltdc: fix deferred endpoint management

[Philippe Cornu]

When a driver related to one of the endpoints is deferred
due to probe dependencies (i2c, spi...) but the other one
is ready, ltdc probe continues and the deferred driver
will never be probed again.

The fix consists in waiting for all deferred endpoints before
continuing the ltdc probe.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/stm/ltdc.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index e3121d9e4230..014cef8cef37 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -987,14 +987,13 @@ int ltdc_load(struct drm_device *ddev)
  [i]);
 
/*
-* If at least one endpoint is ready, continue probing,
-* else if at least one endpoint is -EPROBE_DEFER and
-* there is no previous ready endpoints, defer probing.
+* If at least one endpoint is -EPROBE_DEFER, defer probing,
+* else if at least one endpoint is ready, continue probing.
 */
-   if (!ret)
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   else if (!ret)
endpoint_not_ready = 0;
-   else if (ret == -EPROBE_DEFER && endpoint_not_ready)
-   endpoint_not_ready = -EPROBE_DEFER;
}
 
if (endpoint_not_ready)
-- 
2.15.1

[PATCH] drm/stm: ltdc: fix warning in ltdc_crtc_update_clut()

[Philippe Cornu]

Fix the warning
"warn: variable dereferenced before check 'crtc' (see line 390)"
by removing unnecessary checks as ltdc_crtc_update_clut() is
only called from ltdc_crtc_atomic_flush() where crtc and
crtc->state are not NULL.

Many thanks to Dan Carpenter for the bug report
https://lists.freedesktop.org/archives/dri-devel/2018-February/166918.html

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
---
 drivers/gpu/drm/stm/ltdc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 061d2b6e5157..e3121d9e4230 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -392,9 +392,6 @@ static void ltdc_crtc_update_clut(struct drm_crtc *crtc)
u32 val;
int i;
 
-   if (!crtc || !crtc->state)
-   return;
-
if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut)
return;
 
-- 
2.15.1

[PATCH] drm/stm: ltdc: fix warning in ltdc_crtc_update_clut()

[Philippe Cornu]

Fix the warning
"warn: variable dereferenced before check 'crtc' (see line 390)"
by removing unnecessary checks as ltdc_crtc_update_clut() is
only called from ltdc_crtc_atomic_flush() where crtc and
crtc->state are not NULL.

Many thanks to Dan Carpenter for the bug report
https://lists.freedesktop.org/archives/dri-devel/2018-February/166918.html

Signed-off-by: Philippe Cornu 
Reported-by: Dan Carpenter 
---
 drivers/gpu/drm/stm/ltdc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 061d2b6e5157..e3121d9e4230 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -392,9 +392,6 @@ static void ltdc_crtc_update_clut(struct drm_crtc *crtc)
u32 val;
int i;
 
-   if (!crtc || !crtc->state)
-   return;
-
if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut)
return;
 
-- 
2.15.1

[PATCH 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the 3 optional power supplies using the exact description
found in the document named
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 39 +++
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..e17ba6db1ec8 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -86,6 +87,7 @@ struct sii902x {
struct drm_bridge bridge;
struct drm_connector connector;
struct gpio_desc *reset_gpio;
+   struct regulator_bulk_data supplies[3];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,43 @@ static int sii902x_probe(struct i2c_client *client,
return PTR_ERR(sii902x->reset_gpio);
}
 
+   sii902x->supplies[0].supply = "iovcc";
+   sii902x->supplies[1].supply = "avcc12";
+   sii902x->supplies[2].supply = "cvcc12";
+   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+ sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "regulator_bulk_get failed\n");
+   return ret;
+   }
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+   sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "regulator_bulk_enable failed\n");
+   return ret;
+   }
+
+   usleep_range(1, 2);
+
sii902x_reset(sii902x);
 
ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
if (ret)
-   return ret;
+   goto err_disable_regulator;
 
ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
   , 4);
if (ret) {
dev_err(dev, "regmap_read failed %d\n", ret);
-   return ret;
+   goto err_disable_regulator;
}
 
if (chipid[0] != 0xb0) {
dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
chipid[0]);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto err_disable_regulator;
}
 
/* Clear all pending interrupts */
@@ -424,7 +446,7 @@ static int sii902x_probe(struct i2c_client *client,
IRQF_ONESHOT, dev_name(dev),
sii902x);
if (ret)
-   return ret;
+   goto err_disable_regulator;
}
 
sii902x->bridge.funcs = _bridge_funcs;
@@ -434,6 +456,12 @@ static int sii902x_probe(struct i2c_client *client,
i2c_set_clientdata(client, sii902x);
 
return 0;
+
+err_disable_regulator:
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
+   return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +471,9 @@ static int sii902x_remove(struct i2c_client *client)
 
drm_bridge_remove(>bridge);
 
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
return 0;
 }
 
-- 
2.15.1

[PATCH 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the 3 optional power supplies using the exact description
found in the document named
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/bridge/sii902x.c | 39 +++
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..e17ba6db1ec8 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -86,6 +87,7 @@ struct sii902x {
struct drm_bridge bridge;
struct drm_connector connector;
struct gpio_desc *reset_gpio;
+   struct regulator_bulk_data supplies[3];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,43 @@ static int sii902x_probe(struct i2c_client *client,
return PTR_ERR(sii902x->reset_gpio);
}
 
+   sii902x->supplies[0].supply = "iovcc";
+   sii902x->supplies[1].supply = "avcc12";
+   sii902x->supplies[2].supply = "cvcc12";
+   ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+ sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "regulator_bulk_get failed\n");
+   return ret;
+   }
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+   sii902x->supplies);
+   if (ret) {
+   dev_err(dev, "regulator_bulk_enable failed\n");
+   return ret;
+   }
+
+   usleep_range(1, 2);
+
sii902x_reset(sii902x);
 
ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
if (ret)
-   return ret;
+   goto err_disable_regulator;
 
ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
   , 4);
if (ret) {
dev_err(dev, "regmap_read failed %d\n", ret);
-   return ret;
+   goto err_disable_regulator;
}
 
if (chipid[0] != 0xb0) {
dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
chipid[0]);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto err_disable_regulator;
}
 
/* Clear all pending interrupts */
@@ -424,7 +446,7 @@ static int sii902x_probe(struct i2c_client *client,
IRQF_ONESHOT, dev_name(dev),
sii902x);
if (ret)
-   return ret;
+   goto err_disable_regulator;
}
 
sii902x->bridge.funcs = _bridge_funcs;
@@ -434,6 +456,12 @@ static int sii902x_probe(struct i2c_client *client,
i2c_set_clientdata(client, sii902x);
 
return 0;
+
+err_disable_regulator:
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
+   return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +471,9 @@ static int sii902x_remove(struct i2c_client *client)
 
drm_bridge_remove(>bridge);
 
+   regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+  sii902x->supplies);
+
return 0;
 }
 
-- 
2.15.1

[PATCH 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds the 3 optional power supplies to the sii902x
drm bridge driver.

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  3 ++
 drivers/gpu/drm/bridge/sii902x.c   | 39 +++---
 2 files changed, 38 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds the 3 optional power supplies to the sii902x
drm bridge driver.

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  3 ++
 drivers/gpu/drm/bridge/sii902x.c   | 39 +++---
 2 files changed, 38 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the 3 optional power supplies using the exact description
found in the document named
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt 
b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..cf53678fe574 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,9 @@ Optional properties:
- interrupts-extended or interrupt-parent + interrupts: describe
  the interrupt line used to inform the host about hotplug events.
- reset-gpios: OF device-tree gpio specification for RST_N pin.
+   - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+   - avcc12-supply: TMDS analog supply voltage (1.2V).
+   - cvcc12-supply: Digital core supply voltage (1.2V).
 
 Optional subnodes:
- video input: this subnode can contain a video input port node
-- 
2.15.1

[PATCH 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the 3 optional power supplies using the exact description
found in the document named
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

Signed-off-by: Philippe Cornu 
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt 
b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..cf53678fe574 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,9 @@ Optional properties:
- interrupts-extended or interrupt-parent + interrupts: describe
  the interrupt line used to inform the host about hotplug events.
- reset-gpios: OF device-tree gpio specification for RST_N pin.
+   - iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+   - avcc12-supply: TMDS analog supply voltage (1.2V).
+   - cvcc12-supply: Digital core supply voltage (1.2V).
 
 Optional subnodes:
- video input: this subnode can contain a video input port node
-- 
2.15.1

[PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe Cornu]

This patch clarifies the adjusted_mode documentation
for bridges.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
---
This patch follows discussions in:
- "drm: clarify adjusted_mode for a bridge connected to a crtc"
  https://patchwork.freedesktop.org/patch/206801/
- "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
  https://patchwork.freedesktop.org/patch/215449/

 include/drm/drm_bridge.h | 16 
 include/drm/drm_crtc.h   | 11 +++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7d632c6a9214 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -178,6 +178,22 @@ struct drm_bridge_funcs {
 * then this would be _encoder_helper_funcs.mode_set. The display
 * pipe (i.e.  clocks and timing signals) is off when this function is
 * called.
+*
+* The adjusted_mode parameter corresponds to the mode output by the 
CRTC
+* for the first bridge in the chain. It can be different from the mode
+* parameter that contains the desired mode for the connector at the end
+* of the bridges chain, for instance when the first bridge in the chain
+* performs scaling. The adjusted mode is mostly useful for the first
+* bridge in the chain and is likely irrelevant for the other bridges.
+*
+* For atomic drivers the adjusted_mode is the mode stored in
+* _crtc_state.adjusted_mode.
+*
+* NOTE:
+*
+* If a need arises to store and access modes adjusted for other 
locations
+* than the connection between the CRTC and the first bridge, the DRM
+* framework will have to be extended with DRM bridge states.
 */
void (*mode_set)(struct drm_bridge *bridge,
 struct drm_display_mode *mode,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a2d81d2907a9..65f749a9e9d3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -134,10 +134,13 @@ struct drm_crtc_state {
 *
 * Internal display timings which can be used by the driver to handle
 * differences between the mode requested by userspace in @mode and what
-* is actually programmed into the hardware. It is purely driver
-* implementation defined what exactly this adjusted mode means. Usually
-* it is used to store the hardware display timings used between the
-* CRTC and encoder blocks.
+* is actually programmed into the hardware.
+*
+* For drivers using drm_bridge, this stores the hardware display 
timings
+* used between the CRTC and the first bridge. For other drivers, the
+* meaning of the adjusted_mode field is purely driver implementation
+* defined information, and will usually be used to store the hardware
+* display timings used between the CRTC and encoder blocks.
 */
struct drm_display_mode adjusted_mode;
 
-- 
2.15.1

[PATCH] drm: clarify adjusted_mode documentation for bridges

[Philippe Cornu]

This patch clarifies the adjusted_mode documentation
for bridges.

Signed-off-by: Philippe Cornu 
Signed-off-by: Laurent Pinchart 
---
This patch follows discussions in:
- "drm: clarify adjusted_mode for a bridge connected to a crtc"
  https://patchwork.freedesktop.org/patch/206801/
- "drm: bridge: Constify mode arguments to bridge .mode_set() operation"
  https://patchwork.freedesktop.org/patch/215449/

 include/drm/drm_bridge.h | 16 
 include/drm/drm_crtc.h   | 11 +++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7d632c6a9214 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -178,6 +178,22 @@ struct drm_bridge_funcs {
 * then this would be _encoder_helper_funcs.mode_set. The display
 * pipe (i.e.  clocks and timing signals) is off when this function is
 * called.
+*
+* The adjusted_mode parameter corresponds to the mode output by the 
CRTC
+* for the first bridge in the chain. It can be different from the mode
+* parameter that contains the desired mode for the connector at the end
+* of the bridges chain, for instance when the first bridge in the chain
+* performs scaling. The adjusted mode is mostly useful for the first
+* bridge in the chain and is likely irrelevant for the other bridges.
+*
+* For atomic drivers the adjusted_mode is the mode stored in
+* _crtc_state.adjusted_mode.
+*
+* NOTE:
+*
+* If a need arises to store and access modes adjusted for other 
locations
+* than the connection between the CRTC and the first bridge, the DRM
+* framework will have to be extended with DRM bridge states.
 */
void (*mode_set)(struct drm_bridge *bridge,
 struct drm_display_mode *mode,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a2d81d2907a9..65f749a9e9d3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -134,10 +134,13 @@ struct drm_crtc_state {
 *
 * Internal display timings which can be used by the driver to handle
 * differences between the mode requested by userspace in @mode and what
-* is actually programmed into the hardware. It is purely driver
-* implementation defined what exactly this adjusted mode means. Usually
-* it is used to store the hardware display timings used between the
-* CRTC and encoder blocks.
+* is actually programmed into the hardware.
+*
+* For drivers using drm_bridge, this stores the hardware display 
timings
+* used between the CRTC and the first bridge. For other drivers, the
+* meaning of the adjusted_mode field is purely driver implementation
+* defined information, and will usually be used to store the hardware
+* display timings used between the CRTC and encoder blocks.
 */
struct drm_display_mode adjusted_mode;
 
-- 
2.15.1

[PATCH] drm/stm: ltdc: add user update info in plane print state

[Philippe Cornu]

This patch adds the user update information in
frames-per-second into the drm debugfs plane state.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 22 ++
 drivers/gpu/drm/stm/ltdc.h |  8 
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 2b745cfc9000..061d2b6e5157 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -729,6 +729,8 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
reg_update_bits(ldev->regs, LTDC_L1CR + lofs,
LXCR_LEN | LXCR_CLUTEN, val);
 
+   ldev->plane_fpsi[plane->index].counter++;
+
mutex_lock(>err_lock);
if (ldev->error_status & ISR_FUIF) {
DRM_DEBUG_DRIVER("Fifo underrun\n");
@@ -754,6 +756,25 @@ static void ltdc_plane_atomic_disable(struct drm_plane 
*plane,
 oldstate->crtc->base.id, plane->base.id);
 }
 
+static void ltdc_plane_atomic_print_state(struct drm_printer *p,
+ const struct drm_plane_state *state)
+{
+   struct drm_plane *plane = state->plane;
+   struct ltdc_device *ldev = plane_to_ltdc(plane);
+   struct fps_info *fpsi = >plane_fpsi[plane->index];
+   int ms_since_last;
+   ktime_t now;
+
+   now = ktime_get();
+   ms_since_last = ktime_to_ms(ktime_sub(now, fpsi->last_timestamp));
+
+   drm_printf(p, "\tuser_updates=%dfps\n",
+  DIV_ROUND_CLOSEST(fpsi->counter * 1000, ms_since_last));
+
+   fpsi->last_timestamp = now;
+   fpsi->counter = 0;
+}
+
 static const struct drm_plane_funcs ltdc_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
@@ -761,6 +782,7 @@ static const struct drm_plane_funcs ltdc_plane_funcs = {
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+   .atomic_print_state = ltdc_plane_atomic_print_state,
 };
 
 static const struct drm_plane_helper_funcs ltdc_plane_helper_funcs = {
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index 61a80d00bc3b..1e16d6afb0d2 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -20,6 +20,13 @@ struct ltdc_caps {
bool non_alpha_only_l1; /* non-native no-alpha formats on layer 1 */
 };
 
+#define LTDC_MAX_LAYER 4
+
+struct fps_info {
+   unsigned int counter;
+   ktime_t last_timestamp;
+};
+
 struct ltdc_device {
void __iomem *regs;
struct clk *pixel_clk;  /* lcd pixel clock */
@@ -27,6 +34,7 @@ struct ltdc_device {
struct ltdc_caps caps;
u32 error_status;
u32 irq_status;
+   struct fps_info plane_fpsi[LTDC_MAX_LAYER];
 };
 
 int ltdc_load(struct drm_device *ddev);
-- 
2.15.1

[PATCH] drm/stm: ltdc: add user update info in plane print state

[Philippe Cornu]

This patch adds the user update information in
frames-per-second into the drm debugfs plane state.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/stm/ltdc.c | 22 ++
 drivers/gpu/drm/stm/ltdc.h |  8 
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 2b745cfc9000..061d2b6e5157 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -729,6 +729,8 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
reg_update_bits(ldev->regs, LTDC_L1CR + lofs,
LXCR_LEN | LXCR_CLUTEN, val);
 
+   ldev->plane_fpsi[plane->index].counter++;
+
mutex_lock(>err_lock);
if (ldev->error_status & ISR_FUIF) {
DRM_DEBUG_DRIVER("Fifo underrun\n");
@@ -754,6 +756,25 @@ static void ltdc_plane_atomic_disable(struct drm_plane 
*plane,
 oldstate->crtc->base.id, plane->base.id);
 }
 
+static void ltdc_plane_atomic_print_state(struct drm_printer *p,
+ const struct drm_plane_state *state)
+{
+   struct drm_plane *plane = state->plane;
+   struct ltdc_device *ldev = plane_to_ltdc(plane);
+   struct fps_info *fpsi = >plane_fpsi[plane->index];
+   int ms_since_last;
+   ktime_t now;
+
+   now = ktime_get();
+   ms_since_last = ktime_to_ms(ktime_sub(now, fpsi->last_timestamp));
+
+   drm_printf(p, "\tuser_updates=%dfps\n",
+  DIV_ROUND_CLOSEST(fpsi->counter * 1000, ms_since_last));
+
+   fpsi->last_timestamp = now;
+   fpsi->counter = 0;
+}
+
 static const struct drm_plane_funcs ltdc_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
@@ -761,6 +782,7 @@ static const struct drm_plane_funcs ltdc_plane_funcs = {
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+   .atomic_print_state = ltdc_plane_atomic_print_state,
 };
 
 static const struct drm_plane_helper_funcs ltdc_plane_helper_funcs = {
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index 61a80d00bc3b..1e16d6afb0d2 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -20,6 +20,13 @@ struct ltdc_caps {
bool non_alpha_only_l1; /* non-native no-alpha formats on layer 1 */
 };
 
+#define LTDC_MAX_LAYER 4
+
+struct fps_info {
+   unsigned int counter;
+   ktime_t last_timestamp;
+};
+
 struct ltdc_device {
void __iomem *regs;
struct clk *pixel_clk;  /* lcd pixel clock */
@@ -27,6 +34,7 @@ struct ltdc_device {
struct ltdc_caps caps;
u32 error_status;
u32 irq_status;
+   struct fps_info plane_fpsi[LTDC_MAX_LAYER];
 };
 
 int ltdc_load(struct drm_device *ddev);
-- 
2.15.1

[PATCH] drm/stm: move enable/disable_vblank to crtc

[Philippe Cornu]

enable/disable_vblank() functions at drm_driver level
are deprecated. Move them to the ltdc drm_crtc_funcs
structure.

Signed-off-by: Philippe Cornu <philippe.co...@st.com>
---
 drivers/gpu/drm/stm/drv.c  |  2 --
 drivers/gpu/drm/stm/ltdc.c | 10 ++
 drivers/gpu/drm/stm/ltdc.h |  2 --
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 9ab00a87f7cc..8698e08313e1 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -72,8 +72,6 @@ static struct drm_driver drv_driver = {
.gem_prime_vmap = drm_gem_cma_prime_vmap,
.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
.gem_prime_mmap = drm_gem_cma_prime_mmap,
-   .enable_vblank = ltdc_crtc_enable_vblank,
-   .disable_vblank = ltdc_crtc_disable_vblank,
 };
 
 static int drv_load(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 1a3277e483d5..2b745cfc9000 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -569,9 +569,9 @@ static const struct drm_crtc_helper_funcs 
ltdc_crtc_helper_funcs = {
.atomic_disable = ltdc_crtc_atomic_disable,
 };
 
-int ltdc_crtc_enable_vblank(struct drm_device *ddev, unsigned int pipe)
+static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
 {
-   struct ltdc_device *ldev = ddev->dev_private;
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
 
DRM_DEBUG_DRIVER("\n");
reg_set(ldev->regs, LTDC_IER, IER_LIE);
@@ -579,9 +579,9 @@ int ltdc_crtc_enable_vblank(struct drm_device *ddev, 
unsigned int pipe)
return 0;
 }
 
-void ltdc_crtc_disable_vblank(struct drm_device *ddev, unsigned int pipe)
+static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
 {
-   struct ltdc_device *ldev = ddev->dev_private;
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
 
DRM_DEBUG_DRIVER("\n");
reg_clear(ldev->regs, LTDC_IER, IER_LIE);
@@ -594,6 +594,8 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
.reset = drm_atomic_helper_crtc_reset,
.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+   .enable_vblank = ltdc_crtc_enable_vblank,
+   .disable_vblank = ltdc_crtc_disable_vblank,
.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index edb268129c54..61a80d00bc3b 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -29,8 +29,6 @@ struct ltdc_device {
u32 irq_status;
 };
 
-int ltdc_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
-void ltdc_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
 int ltdc_load(struct drm_device *ddev);
 void ltdc_unload(struct drm_device *ddev);
 
-- 
2.15.1

[PATCH] drm/stm: move enable/disable_vblank to crtc

[Philippe Cornu]

enable/disable_vblank() functions at drm_driver level
are deprecated. Move them to the ltdc drm_crtc_funcs
structure.

Signed-off-by: Philippe Cornu 
---
 drivers/gpu/drm/stm/drv.c  |  2 --
 drivers/gpu/drm/stm/ltdc.c | 10 ++
 drivers/gpu/drm/stm/ltdc.h |  2 --
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 9ab00a87f7cc..8698e08313e1 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -72,8 +72,6 @@ static struct drm_driver drv_driver = {
.gem_prime_vmap = drm_gem_cma_prime_vmap,
.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
.gem_prime_mmap = drm_gem_cma_prime_mmap,
-   .enable_vblank = ltdc_crtc_enable_vblank,
-   .disable_vblank = ltdc_crtc_disable_vblank,
 };
 
 static int drv_load(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 1a3277e483d5..2b745cfc9000 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -569,9 +569,9 @@ static const struct drm_crtc_helper_funcs 
ltdc_crtc_helper_funcs = {
.atomic_disable = ltdc_crtc_atomic_disable,
 };
 
-int ltdc_crtc_enable_vblank(struct drm_device *ddev, unsigned int pipe)
+static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
 {
-   struct ltdc_device *ldev = ddev->dev_private;
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
 
DRM_DEBUG_DRIVER("\n");
reg_set(ldev->regs, LTDC_IER, IER_LIE);
@@ -579,9 +579,9 @@ int ltdc_crtc_enable_vblank(struct drm_device *ddev, 
unsigned int pipe)
return 0;
 }
 
-void ltdc_crtc_disable_vblank(struct drm_device *ddev, unsigned int pipe)
+static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
 {
-   struct ltdc_device *ldev = ddev->dev_private;
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
 
DRM_DEBUG_DRIVER("\n");
reg_clear(ldev->regs, LTDC_IER, IER_LIE);
@@ -594,6 +594,8 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
.reset = drm_atomic_helper_crtc_reset,
.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+   .enable_vblank = ltdc_crtc_enable_vblank,
+   .disable_vblank = ltdc_crtc_disable_vblank,
.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index edb268129c54..61a80d00bc3b 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -29,8 +29,6 @@ struct ltdc_device {
u32 irq_status;
 };
 
-int ltdc_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
-void ltdc_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
 int ltdc_load(struct drm_device *ddev);
 void ltdc_unload(struct drm_device *ddev);
 
-- 
2.15.1

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

Hi Laurent,

On 04/06/2018 04:53 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Monday, 26 February 2018 14:16:04 EEST Philippe Cornu wrote:
>> This patch clarifies the adjusted_mode documentation
>> for a bridge directly connected to a crtc.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>
>>   include/drm/drm_bridge.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..b5f3c070467c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>   * pipeline has been called already. If the bridge is the first element
>>   * then this would be _encoder_helper_funcs.mode_set. The display
>>   * pipe (i.e.  clocks and timing signals) is off when this function is
>> - * called.
>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>> + * parameter is the one defined in _crtc_state.adjusted_mode.
> 
> Unless I'm mistaken this will always be the mode stored in
> _crtc_state.adjusted_mode (at least for atomic drivers), regardless of
> whether the bridge is the first in the chain (connected to the CRTC) or not.
> What is important to document is that we have a single adjusted_mode for the
> whole chain of bridges, and that it corresponds to the mode output by the CRTC
> for the first bridge. Bridges further in the chain can look at that mode,
> although there will probably be nothing of interest to them there.
> 
> How about the following text ?
> 
>  /**
>   * @mode_set:
>   *
>   * This callback should set the given mode on the bridge. It is called
>   * after the @mode_set callback for the preceding element in the display
>   * pipeline has been called already. If the bridge is the first element
>   * then this would be _encoder_helper_funcs.mode_set. The display
>   * pipe (i.e.  clocks and timing signals) is off when this function is
>   * called.
>   *
>   * The adjusted_mode parameter corresponds to the mode output by the CRTC
>   * for the first bridge in the chain. It can be different from the mode
>   * parameter that contains the desired mode for the connector at the end
>   * of the bridges chain, for instance when the first bridge in the chain
>   * performs scaling. The adjusted mode is mostly useful for the first
>   * bridge in the chain and is likely irrelevant for the other bridges.
>   *
>   * For atomic drivers the adjusted_mode is the mode stored in
>   * _crtc_state.adjusted_mode.
>   *
>   * NOTE:
>   *
>   * If a need arises to store and access modes adjusted for other 
> locations
>   * than the connection between the CRTC and the first bridge, the DRM
>   * framework will have to be extended with DRM bridge states.
>*/
> 
> Then I think we should also update the documentation of
> drm_crtc_state.adjusted_mode accordingly:
> 
>  /*
>   * @adjusted_mode:
>   *
>   * Internal display timings which can be used by the driver to handle
>   * differences between the mode requested by userspace in @mode and what
>   * is actually programmed into the hardware.
>   *
>   * For drivers using drm_bridge, this store the hardware display timings
>   * used between the CRTC and the first bridge. For other drivers, the
>   * meaning of the adjusted_mode field is purely driver implementation
>   * defined information, and will usually be used to store the hardware
>   * display timings used between the CRTC and encoder blocks.
>   */
> 

Your proposal is very clear and understandable. I will make a new patch 
version based on it.
Big thanks
Philippe :-)

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

Hi Laurent,

On 04/06/2018 04:53 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Monday, 26 February 2018 14:16:04 EEST Philippe Cornu wrote:
>> This patch clarifies the adjusted_mode documentation
>> for a bridge directly connected to a crtc.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>
>>   include/drm/drm_bridge.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..b5f3c070467c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>   * pipeline has been called already. If the bridge is the first element
>>   * then this would be _encoder_helper_funcs.mode_set. The display
>>   * pipe (i.e.  clocks and timing signals) is off when this function is
>> - * called.
>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>> + * parameter is the one defined in _crtc_state.adjusted_mode.
> 
> Unless I'm mistaken this will always be the mode stored in
> _crtc_state.adjusted_mode (at least for atomic drivers), regardless of
> whether the bridge is the first in the chain (connected to the CRTC) or not.
> What is important to document is that we have a single adjusted_mode for the
> whole chain of bridges, and that it corresponds to the mode output by the CRTC
> for the first bridge. Bridges further in the chain can look at that mode,
> although there will probably be nothing of interest to them there.
> 
> How about the following text ?
> 
>  /**
>   * @mode_set:
>   *
>   * This callback should set the given mode on the bridge. It is called
>   * after the @mode_set callback for the preceding element in the display
>   * pipeline has been called already. If the bridge is the first element
>   * then this would be _encoder_helper_funcs.mode_set. The display
>   * pipe (i.e.  clocks and timing signals) is off when this function is
>   * called.
>   *
>   * The adjusted_mode parameter corresponds to the mode output by the CRTC
>   * for the first bridge in the chain. It can be different from the mode
>   * parameter that contains the desired mode for the connector at the end
>   * of the bridges chain, for instance when the first bridge in the chain
>   * performs scaling. The adjusted mode is mostly useful for the first
>   * bridge in the chain and is likely irrelevant for the other bridges.
>   *
>   * For atomic drivers the adjusted_mode is the mode stored in
>   * _crtc_state.adjusted_mode.
>   *
>   * NOTE:
>   *
>   * If a need arises to store and access modes adjusted for other 
> locations
>   * than the connection between the CRTC and the first bridge, the DRM
>   * framework will have to be extended with DRM bridge states.
>*/
> 
> Then I think we should also update the documentation of
> drm_crtc_state.adjusted_mode accordingly:
> 
>  /*
>   * @adjusted_mode:
>   *
>   * Internal display timings which can be used by the driver to handle
>   * differences between the mode requested by userspace in @mode and what
>   * is actually programmed into the hardware.
>   *
>   * For drivers using drm_bridge, this store the hardware display timings
>   * used between the CRTC and the first bridge. For other drivers, the
>   * meaning of the adjusted_mode field is purely driver implementation
>   * defined information, and will usually be used to store the hardware
>   * display timings used between the CRTC and encoder blocks.
>   */
> 

Your proposal is very clear and understandable. I will make a new patch 
version based on it.
Big thanks
Philippe :-)

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

On 03/29/2018 09:39 AM, Daniel Vetter wrote:
> On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU <philippe.co...@st.com> wrote:
>> Hi Daniel,
>>
>> On 03/27/2018 05:51 PM, Daniel Vetter wrote:
>>> On Mon, Feb 26, 2018 at 01:16:04PM +0100, Philippe Cornu wrote:
>>>> This patch clarifies the adjusted_mode documentation
>>>> for a bridge directly connected to a crtc.
>>>>
>>>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>>>> ---
>>>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>>
>> Many thanks for the review.
>>
>>>
>>> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
>>> commit rights, but you seem to not use that. And with 18 patches in the
>>> 4.17 cycle you're the top contributor who's not pushing his own patches.
>>>
>>> What's the hold-up? Tools don't work, or something else? I really think
>>> regular contributors should just push their own stuff themselves (after
>>> appropriate review ofc).
>>> -Daniel
>>>
>>
>> I still consider myself a drm “beginner”, my first patch in drm was last
>> summer so less than 1 year ago.
> 
> You're doing great patches, and at least for drm-misc you're the top
> contributor who doesn't push stuff himself. You're definitely ready to
> do so!
> 
>> However, thank you for your encouraging return, I will immediately study
>> the matter (dim...) and prepare myself.
> 
> Yes please. And for any questions please ask on #dri-devel on
> freenode, we're all happy to help out.
> -Daniel

Hi Daniel & Benjamin,

I managed to push a first patch on drm-misc-next :-)

No particular difficulties with dim installation, main encountered 
issues are "corporate proxies" & ubuntu package updates...

So this email is mostly to thank you Benjamin for your good advice and 
support when using dim.

Big thanks,
Philippe :-)

> 
>>
>> Thank you,
>> Philippe :-)
>>
>>>>
>>>>include/drm/drm_bridge.h | 3 ++-
>>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index 3270fec46979..b5f3c070467c 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>>>* pipeline has been called already. If the bridge is the first 
>>>> element
>>>>* then this would be _encoder_helper_funcs.mode_set. The display
>>>>* pipe (i.e.  clocks and timing signals) is off when this function 
>>>> is
>>>> - * called.
>>>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>>>> + * parameter is the one defined in _crtc_state.adjusted_mode.
>>>>*/
>>>>   void (*mode_set)(struct drm_bridge *bridge,
>>>>struct drm_display_mode *mode,
>>>> --
>>>> 2.15.1
>>>>
>>>> ___
>>>> dri-devel mailing list
>>>> dri-de...@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

On 03/29/2018 09:39 AM, Daniel Vetter wrote:
> On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU  wrote:
>> Hi Daniel,
>>
>> On 03/27/2018 05:51 PM, Daniel Vetter wrote:
>>> On Mon, Feb 26, 2018 at 01:16:04PM +0100, Philippe Cornu wrote:
>>>> This patch clarifies the adjusted_mode documentation
>>>> for a bridge directly connected to a crtc.
>>>>
>>>> Signed-off-by: Philippe Cornu 
>>>> ---
>>>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>>
>>> Reviewed-by: Daniel Vetter 
>>
>> Many thanks for the review.
>>
>>>
>>> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
>>> commit rights, but you seem to not use that. And with 18 patches in the
>>> 4.17 cycle you're the top contributor who's not pushing his own patches.
>>>
>>> What's the hold-up? Tools don't work, or something else? I really think
>>> regular contributors should just push their own stuff themselves (after
>>> appropriate review ofc).
>>> -Daniel
>>>
>>
>> I still consider myself a drm “beginner”, my first patch in drm was last
>> summer so less than 1 year ago.
> 
> You're doing great patches, and at least for drm-misc you're the top
> contributor who doesn't push stuff himself. You're definitely ready to
> do so!
> 
>> However, thank you for your encouraging return, I will immediately study
>> the matter (dim...) and prepare myself.
> 
> Yes please. And for any questions please ask on #dri-devel on
> freenode, we're all happy to help out.
> -Daniel

Hi Daniel & Benjamin,

I managed to push a first patch on drm-misc-next :-)

No particular difficulties with dim installation, main encountered 
issues are "corporate proxies" & ubuntu package updates...

So this email is mostly to thank you Benjamin for your good advice and 
support when using dim.

Big thanks,
Philippe :-)

> 
>>
>> Thank you,
>> Philippe :-)
>>
>>>>
>>>>include/drm/drm_bridge.h | 3 ++-
>>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index 3270fec46979..b5f3c070467c 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>>>* pipeline has been called already. If the bridge is the first 
>>>> element
>>>>* then this would be _encoder_helper_funcs.mode_set. The display
>>>>* pipe (i.e.  clocks and timing signals) is off when this function 
>>>> is
>>>> - * called.
>>>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>>>> + * parameter is the one defined in _crtc_state.adjusted_mode.
>>>>*/
>>>>   void (*mode_set)(struct drm_bridge *bridge,
>>>>struct drm_display_mode *mode,
>>>> --
>>>> 2.15.1
>>>>
>>>> ___
>>>> dri-devel mailing list
>>>> dri-de...@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

Re: [PATCH] drm/sti: Depend on OF rather than selecting it

[Philippe CORNU]

On 04/05/2018 01:05 PM, Benjamin Gaignard wrote:
> 2018-04-03 7:34 GMT+02:00 Oliver O'Halloran :
>> Commit cc6b741c6f63 ("drm: sti: remove useless fields from vtg
>> structure") reworked some code inside of this driver and made it select
>> CONFIG_OF. This results in the entire OF layer being enabled when
>> building an allmodconfig on ia64. OF on ia64 is completely unsupported
>> so this isn't a great state of affairs.
>>
>> The 0day robot noticed a link-time failure on ia64 caused by
>> using of_node_to_nid() in an otherwise unrelated driver. The
>> generic fallback for of_node_to_nid() only exists when:
>>
>>  defined(CONFIG_OF) && defined(CONFIG_NUMA) == false
>>
>> Since CONFIG_NUMA is usually selected for IA64 we get the link failure.
>> Fix this by making the driver depend on OF rather than selecting it,
>> odds are that was the original intent.
>>
>> Link: https://lists.01.org/pipermail/kbuild-all/2018-March/045172.html
>> Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
>> Cc: Benjamin Gaignard 
>> Cc: Vincent Abriou 
>> Cc: David Airlie 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-i...@vger.kernel.org
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Oliver O'Halloran 
> 
> Reviewed-by: Benjamin Gaignard 

Applied on drm-misc-next

Many thanks for your patch,
Philippe :-)

> 
>> ---
>> Cc`ed to stable since the ia64 guys might want it backported. I'm not
>> bothered if it just goes into mainline.
>> ---
>>   drivers/gpu/drm/sti/Kconfig | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
>> index cca4b3c9aeb5..1963cc1b1cc5 100644
>> --- a/drivers/gpu/drm/sti/Kconfig
>> +++ b/drivers/gpu/drm/sti/Kconfig
>> @@ -1,6 +1,6 @@
>>   config DRM_STI
>>  tristate "DRM Support for STMicroelectronics SoC stiH4xx Series"
>> -   depends on DRM && (ARCH_STI || ARCH_MULTIPLATFORM)
>> +   depends on OF && DRM && (ARCH_STI || ARCH_MULTIPLATFORM)
>>  select RESET_CONTROLLER
>>  select DRM_KMS_HELPER
>>  select DRM_GEM_CMA_HELPER
>> @@ -8,6 +8,5 @@ config DRM_STI
>>  select DRM_PANEL
>>  select FW_LOADER
>>  select SND_SOC_HDMI_CODEC if SND_SOC
>> -   select OF
>>  help
>>Choose this option to enable DRM on STM stiH4xx chipset
>> --
>> 2.9.5
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drm/sti: Depend on OF rather than selecting it

[Philippe CORNU]

On 04/05/2018 01:05 PM, Benjamin Gaignard wrote:
> 2018-04-03 7:34 GMT+02:00 Oliver O'Halloran :
>> Commit cc6b741c6f63 ("drm: sti: remove useless fields from vtg
>> structure") reworked some code inside of this driver and made it select
>> CONFIG_OF. This results in the entire OF layer being enabled when
>> building an allmodconfig on ia64. OF on ia64 is completely unsupported
>> so this isn't a great state of affairs.
>>
>> The 0day robot noticed a link-time failure on ia64 caused by
>> using of_node_to_nid() in an otherwise unrelated driver. The
>> generic fallback for of_node_to_nid() only exists when:
>>
>>  defined(CONFIG_OF) && defined(CONFIG_NUMA) == false
>>
>> Since CONFIG_NUMA is usually selected for IA64 we get the link failure.
>> Fix this by making the driver depend on OF rather than selecting it,
>> odds are that was the original intent.
>>
>> Link: https://lists.01.org/pipermail/kbuild-all/2018-March/045172.html
>> Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
>> Cc: Benjamin Gaignard 
>> Cc: Vincent Abriou 
>> Cc: David Airlie 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-i...@vger.kernel.org
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Oliver O'Halloran 
> 
> Reviewed-by: Benjamin Gaignard 

Applied on drm-misc-next

Many thanks for your patch,
Philippe :-)

> 
>> ---
>> Cc`ed to stable since the ia64 guys might want it backported. I'm not
>> bothered if it just goes into mainline.
>> ---
>>   drivers/gpu/drm/sti/Kconfig | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
>> index cca4b3c9aeb5..1963cc1b1cc5 100644
>> --- a/drivers/gpu/drm/sti/Kconfig
>> +++ b/drivers/gpu/drm/sti/Kconfig
>> @@ -1,6 +1,6 @@
>>   config DRM_STI
>>  tristate "DRM Support for STMicroelectronics SoC stiH4xx Series"
>> -   depends on DRM && (ARCH_STI || ARCH_MULTIPLATFORM)
>> +   depends on OF && DRM && (ARCH_STI || ARCH_MULTIPLATFORM)
>>  select RESET_CONTROLLER
>>  select DRM_KMS_HELPER
>>  select DRM_GEM_CMA_HELPER
>> @@ -8,6 +8,5 @@ config DRM_STI
>>  select DRM_PANEL
>>  select FW_LOADER
>>  select SND_SOC_HDMI_CODEC if SND_SOC
>> -   select OF
>>  help
>>Choose this option to enable DRM on STM stiH4xx chipset
>> --
>> 2.9.5
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

Hi Daniel,

On 03/27/2018 05:51 PM, Daniel Vetter wrote:
> On Mon, Feb 26, 2018 at 01:16:04PM +0100, Philippe Cornu wrote:
>> This patch clarifies the adjusted_mode documentation
>> for a bridge directly connected to a crtc.
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
> 
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>

Many thanks for the review.

> 
> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
> commit rights, but you seem to not use that. And with 18 patches in the
> 4.17 cycle you're the top contributor who's not pushing his own patches.
> 
> What's the hold-up? Tools don't work, or something else? I really think
> regular contributors should just push their own stuff themselves (after
> appropriate review ofc).
> -Daniel
> 

I still consider myself a drm “beginner”, my first patch in drm was last 
summer so less than 1 year ago.
However, thank you for your encouraging return, I will immediately study 
the matter (dim...) and prepare myself.

Thank you,
Philippe :-)

>>
>>   include/drm/drm_bridge.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..b5f3c070467c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>   * pipeline has been called already. If the bridge is the first element
>>   * then this would be _encoder_helper_funcs.mode_set. The display
>>   * pipe (i.e.  clocks and timing signals) is off when this function is
>> - * called.
>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>> + * parameter is the one defined in _crtc_state.adjusted_mode.
>>   */
>>  void (*mode_set)(struct drm_bridge *bridge,
>>   struct drm_display_mode *mode,
>> -- 
>> 2.15.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe CORNU]

Hi Daniel,

On 03/27/2018 05:51 PM, Daniel Vetter wrote:
> On Mon, Feb 26, 2018 at 01:16:04PM +0100, Philippe Cornu wrote:
>> This patch clarifies the adjusted_mode documentation
>> for a bridge directly connected to a crtc.
>>
>> Signed-off-by: Philippe Cornu 
>> ---
>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
> 
> Reviewed-by: Daniel Vetter 

Many thanks for the review.

> 
> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
> commit rights, but you seem to not use that. And with 18 patches in the
> 4.17 cycle you're the top contributor who's not pushing his own patches.
> 
> What's the hold-up? Tools don't work, or something else? I really think
> regular contributors should just push their own stuff themselves (after
> appropriate review ofc).
> -Daniel
> 

I still consider myself a drm “beginner”, my first patch in drm was last 
summer so less than 1 year ago.
However, thank you for your encouraging return, I will immediately study 
the matter (dim...) and prepare myself.

Thank you,
Philippe :-)

>>
>>   include/drm/drm_bridge.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..b5f3c070467c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -177,7 +177,8 @@ struct drm_bridge_funcs {
>>   * pipeline has been called already. If the bridge is the first element
>>   * then this would be _encoder_helper_funcs.mode_set. The display
>>   * pipe (i.e.  clocks and timing signals) is off when this function is
>> - * called.
>> + * called. If the bridge is connected to the crtc, the adjusted_mode
>> + * parameter is the one defined in _crtc_state.adjusted_mode.
>>   */
>>  void (*mode_set)(struct drm_bridge *bridge,
>>   struct drm_display_mode *mode,
>> -- 
>> 2.15.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH v2 0/2] drm/panel: Add support for Raydium RM68200 panel

[Philippe CORNU]

Hi Thierry,

On 03/12/2018 09:04 AM, Thierry Reding wrote:
> On Fri, Mar 02, 2018 at 04:32:20PM +0100, Philippe Cornu wrote:
>> The Raydium Semiconductor Corporation RM68200 is a 5.5" 720x1280
>> TFT LCD panel connected using a MIPI-DSI video interface.
>>
>> Version 2:
>> - Add Rob Herring Reviewed-by on dt-bindings.
>> - Update Kconfig & driver thanks to Thierry Reding comments: no more
>>DRV_NAME, DRM_WARN_ONCE instead of DRV_NAME where applicable, use
>>backlight_enable/disable() & devm_of_find_backlight(), no extra
>>gpio reset to 0, no more msg if successful, use RM68200 instead of
>>rm68200 where necessary.
>>
>> Version 1:
>> - Initial commit
>>
>> Philippe Cornu (2):
>>dt-bindings/display/panel: Add support for Raydium rm68200 dsi panel
>>drm/panel: Add support for Raydium RM68200 panel driver
>>
>>   .../bindings/display/panel/raydium,rm68200.txt |  25 ++
>>   drivers/gpu/drm/panel/Kconfig  |   8 +
>>   drivers/gpu/drm/panel/Makefile |   1 +
>>   drivers/gpu/drm/panel/panel-raydium-rm68200.c  | 437 
>> +
>>   4 files changed, 471 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/panel/raydium,rm68200.txt
>>   create mode 100755 drivers/gpu/drm/panel/panel-raydium-rm68200.c
> 
> Applied, thanks. There were a few tiny things I wasn't entirely happy
> about, but I fixed those up myself.
> 
> Most notable was the use of DRM_WARN_ONCE(), which is bad in this case
> because it won't show any subsequent errors, ever again. I think in this
> case the proper solution is DRM_ERROR_RATELIMITED(), or erroring out on
> the first error, assuming that subsequent operations won't succeed
> either. I went with the rate limitation.

Big thank you for your nice cleanups and reworks, I will follow those in 
the future :-)

I take the opportunity to thank you also for the merge of the 2 otm8009a 
patches.

Moreover, if you think I need to update otm8009a driver with the same 
reworks (include order, DRM_ERROR_RATELIMITED, error msg, remove msg if 
successfull, struct data alignment...), please tell me :)
Note: I plan to update it soon with new backlight_enable/disable api...

Thank you again,
Philippe :-)

> 
> Thierry
> 

Re: [PATCH v2 0/2] drm/panel: Add support for Raydium RM68200 panel

[Philippe CORNU]

Hi Thierry,

On 03/12/2018 09:04 AM, Thierry Reding wrote:
> On Fri, Mar 02, 2018 at 04:32:20PM +0100, Philippe Cornu wrote:
>> The Raydium Semiconductor Corporation RM68200 is a 5.5" 720x1280
>> TFT LCD panel connected using a MIPI-DSI video interface.
>>
>> Version 2:
>> - Add Rob Herring Reviewed-by on dt-bindings.
>> - Update Kconfig & driver thanks to Thierry Reding comments: no more
>>DRV_NAME, DRM_WARN_ONCE instead of DRV_NAME where applicable, use
>>backlight_enable/disable() & devm_of_find_backlight(), no extra
>>gpio reset to 0, no more msg if successful, use RM68200 instead of
>>rm68200 where necessary.
>>
>> Version 1:
>> - Initial commit
>>
>> Philippe Cornu (2):
>>dt-bindings/display/panel: Add support for Raydium rm68200 dsi panel
>>drm/panel: Add support for Raydium RM68200 panel driver
>>
>>   .../bindings/display/panel/raydium,rm68200.txt |  25 ++
>>   drivers/gpu/drm/panel/Kconfig  |   8 +
>>   drivers/gpu/drm/panel/Makefile |   1 +
>>   drivers/gpu/drm/panel/panel-raydium-rm68200.c  | 437 
>> +
>>   4 files changed, 471 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/panel/raydium,rm68200.txt
>>   create mode 100755 drivers/gpu/drm/panel/panel-raydium-rm68200.c
> 
> Applied, thanks. There were a few tiny things I wasn't entirely happy
> about, but I fixed those up myself.
> 
> Most notable was the use of DRM_WARN_ONCE(), which is bad in this case
> because it won't show any subsequent errors, ever again. I think in this
> case the proper solution is DRM_ERROR_RATELIMITED(), or erroring out on
> the first error, assuming that subsequent operations won't succeed
> either. I went with the rate limitation.

Big thank you for your nice cleanups and reworks, I will follow those in 
the future :-)

I take the opportunity to thank you also for the merge of the 2 otm8009a 
patches.

Moreover, if you think I need to update otm8009a driver with the same 
reworks (include order, DRM_ERROR_RATELIMITED, error msg, remove msg if 
successfull, struct data alignment...), please tell me :)
Note: I plan to update it soon with new backlight_enable/disable api...

Thank you again,
Philippe :-)

> 
> Thierry
> 

Re: [PATCH v1 2/2] drm/panel: Add support for Raydium rm68200 panel driver

[Philippe CORNU]

Hi Thierry,

A big thank you for your code review and comments.
It helped me to improve the driver and to send a v2.

Philippe :-)

On 02/28/2018 08:16 PM, Thierry Reding wrote:
> On Thu, Feb 08, 2018 at 03:30:26PM +0100, Philippe Cornu wrote:
>> This patch adds Raydium Semiconductor Corporation rm68200
>> 5.5" 720x1280 TFT LCD panel driver (MIPI-DSI video mode).
>>
>> Signed-off-by: Philippe Cornu <philippe.co...@st.com>
>> ---
>>   drivers/gpu/drm/panel/Kconfig |   8 +
>>   drivers/gpu/drm/panel/Makefile|   1 +
>>   drivers/gpu/drm/panel/panel-raydium-rm68200.c | 464 
>> ++
>>   3 files changed, 473 insertions(+)
>>   create mode 100755 drivers/gpu/drm/panel/panel-raydium-rm68200.c
>>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 988048ebcc22..08d99dd46765 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -108,6 +108,14 @@ config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
>>Pi 7" Touchscreen.  To compile this driver as a module,
>>choose M here.
>>   
>> +config DRM_PANEL_RAYDIUM_RM68200
>> +tristate "Raydium rm68200 720x1280 dsi 2dl video mode panel"
> 
> What's 2dl? Either this is something already implied by the RM68200
> model or if there are multiple variants of the RM68200 you'll probably
> want to ensure that's reflected in the compatible string.
> 
>> +depends on OF
>> +depends on DRM_MIPI_DSI
>> +help
>> +  Say Y here if you want to enable support for Raydium rm68200
>> +  720x1280 dsi 2dl video mode panel
>> +
>>   config DRM_PANEL_SAMSUNG_S6E3HA2
>>  tristate "Samsung S6E3HA2 DSI video mode panel"
>>  depends on OF
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 3d2a88d0e965..f26efc11d746 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>>   obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
>>   obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
>> panel-panasonic-vvx10f034n00.o
>>   obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += 
>> panel-raspberrypi-touchscreen.o
>> +obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
>>   obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
>>   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
>>   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
>> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm68200.c 
>> b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
>> new file mode 100755
>> index ..f3e15873d05a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
>> @@ -0,0 +1,464 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics SA 2017
>> + *
>> + * Authors: Philippe Cornu <philippe.co...@st.com>
>> + *  Yannick Fertre <yannick.fer...@st.com>
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define DRV_NAME "raydium_rm68200"
> 
> Not needed, see below.
> 
>> +
>> +/*** Manufacturer Command Set ***/
>> +#define MCS_CMD_MODE_SW 0xFE /* CMD Mode Switch */
>> +#define MCS_CMD1_UCS0x00 /* User Command Set (UCS = CMD1) */
>> +#define MCS_CMD2_P0 0x01 /* Manufacture Command Set Page0 (CMD2 P0) */
>> +#define MCS_CMD2_P1 0x02 /* Manufacture Command Set Page1 (CMD2 P1) */
>> +#define MCS_CMD2_P2 0x03 /* Manufacture Command Set Page2 (CMD2 P2) */
>> +#define MCS_CMD2_P3 0x04 /* Manufacture Command Set Page3 (CMD2 P3) */
>> +
>> +/* CMD2 P0 commands (Display Options and Power) */
>> +#define MCS_STBCTR  0x12 /* TE1 Output Setting Zig-Zag Connection */
>> +#define MCS_SGOPCTR 0x16 /* Source Bias Current */
>> +#define MCS_SDCTR   0x1A /* Source Output Delay Time */
>> +#define MCS_INVCTR  0x1B /* Inversion Type */
>> +#define MCS_EXT_PWR_IC  0x24 /* External PWR IC Control */
>> +#define MCS_SETAVDD 0x27 /* PFM Control for AVDD Output */
>> +#define MCS_SETAVEE 0x29 /* PFM Control for AVEE Output */
>> +#define MCS_BT2CTR  0x2B /* DDVDL Charge Pump Control */
>> +#define MCS_BT3CTR  0x2F /* VGH Charge Pump Control */
>> +#define MCS_BT4CTR  0x34 /* VGL Charge Pump Control */
>