Re: [PATCH] drm/exynos: add mic_bypass option for exynos542x fimd

[Inki Dae <inki.dae@samsung.com>]

Hi Chanho,

There was a missing one so below is my review again.

2016년 02월 11일 22:59에 Chanho Park 이(가) 쓴 글:
> Hi Inki,
> 
> On Thu, Feb 11, 2016 at 7:37 PM, Inki Dae <inki.dae@samsung.com> wrote:
>> Hi Chanho,
>>
>> 2016년 01월 30일 22:58에 Chanho Park 이(가) 쓴 글:
>>> From: Chanho Park <chanho61.park@samsung.com>
>>>
>>> This patch adds a mic_bypass option to bypass the mic
>>> from display out path. The mic(Mobile image compressor) compresses
>>> RGB data from fimd and send the compressed data to the mipi dsi.
>>> The bypass option can be founded from system register and the bit
>>> of the option is 11.
>>>
>>> Cc: Inki Dae <inki.dae@samsung.com>
>>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
>>> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
>>> ---
>>>  .../devicetree/bindings/display/exynos/samsung-fimd.txt    |  2 ++
>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c                   | 14 ++++++++++++++
>>>  2 files changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>> index 27c3ce0..7f90c4a 100644
>>> --- a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>> +++ b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>> @@ -45,6 +45,8 @@ Optional Properties:
>>>               Can be used in case timings cannot be provided otherwise
>>>               or to override timings provided by the panel.
>>>  - samsung,sysreg: handle to syscon used to control the system registers
>>> +- samsung,mic-bypass: bypass mic(mobile image compressor) from display path.

mic-bypass is not really common property for fimd family so it's not correct for fimd device node has mic-bypass property.

>>> +                   This option is only available since exynos5420.
>>>  - i80-if-timings: timing configuration for lcd i80 interface support.
>>>    - cs-setup: clock cycles for the active period of address signal is enabled
>>>                until chip select is enabled.
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index 70194d0..4fb2952 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -94,6 +94,7 @@ struct fimd_driver_data {
>>>       unsigned int lcdblk_offset;
>>>       unsigned int lcdblk_vt_shift;
>>>       unsigned int lcdblk_bypass_shift;
>>> +     unsigned int lcdblk_mic_bypass_shift;
>>>
>>>       unsigned int has_shadowcon:1;
>>>       unsigned int has_clksel:1;
>>> @@ -140,6 +141,7 @@ static struct fimd_driver_data exynos5_fimd_driver_data = {
>>>       .lcdblk_offset = 0x214,
>>>       .lcdblk_vt_shift = 24,
>>>       .lcdblk_bypass_shift = 15,
>>> +     .lcdblk_mic_bypass_shift = 11,
>>>       .has_shadowcon = 1,
>>>       .has_vidoutcon = 1,
>>>       .has_vtsel = 1,
>>> @@ -162,6 +164,7 @@ struct fimd_context {
>>>       u32                             i80ifcon;
>>>       bool                            i80_if;
>>>       bool                            suspended;
>>> +     bool                            mic_bypass;
>>>       int                             pipe;
>>>       wait_queue_head_t               wait_vsync_queue;
>>>       atomic_t                        wait_vsync_event;
>>> @@ -461,6 +464,14 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
>>>               return;
>>>       }
>>>
>>> +     if (ctx->mic_bypass && ctx->sysreg && regmap_update_bits(ctx->sysreg,
>>> +                             driver_data->lcdblk_offset,
>>> +                             0x1 << driver_data->lcdblk_mic_bypass_shift,
>>> +                             0x1 << driver_data->lcdblk_mic_bypass_shift)) {
>>> +             DRM_ERROR("Failed to update sysreg for bypass mic.\n");
>>> +             return;
>>> +     }
>>
>> It'd better to consider mic path also because mic bypass bit of lcdblk could be true by bootloader. In this case, fimd driver wouldn't do anything if mic_bypass property isn't declared but the mic_bypass bit of lcdblk register is still set to 1.
> 
> Actually, I wanted to set the bit on kernel side even though it's not
> assigned from bootloader.
> If the bootloader already set the bit, that means mic will be by-pass
> and we don't care about it from kernel side.
> The option is useful when I want to skip the mic on the kernel side.
> 
>>
>> For this, I think you could check lcdblk_mic_pypass_shift to identify whether mic path is supported or not like below,
>> if (ctx->lcdblk_mic_bypass_shift) {
> 
> It causes all exynos5 fimd driver skips the mic from display path.
> How about below patch instead of this?
> 
> +       if (of_property_read_bool(dev->of_node, "samsung,mic-bypass") &&
> +           ctx->driver_data->lcdblk_mic_bypass_shift)
> +               ctx->mic_bypass = true;

So let's bypass mic path in default like lcdblk_bypass,
	/* set mic bypass selection */
	if (ctx->has_mic_bypass && ctx->sysreg && regmap_update_bits(ctx->sysreg,
				driver_data->lcdblk_offset,
				0x1 << driver_data->lcdblk_mic_bypass_shift,
				0x1 << driver_data->lcdblk_mic_bypass_shift)) {
		DRM_ERROR("Failed to update sysreg for mic bypass setting.\n");
		return;
	}

For this, I added a new member, has_mic_by_pass, which means that lcdblk register has mic_bypass bit like has_vtsel.

We could consider display path such as mic and mDNIe later using of_graph concept below,
	Documentation/devicetree/bindings/graph.txt

Thanks,
Inki Dae

> 
> Best Regards,
> Chanho Park
> 
>>         if (ctx->sysreg && regmap_update_bits(ctx->sysreg,
>>                                         driver_data->lcdblk_offset,
>>                                         ctx->mic_bypass << driver_data->lcdblk_mic_bypass_shift,
>>                                         ctx->mic_bypass << driver_data->lcdblk_mic_bypass_shift)) {
>>                 ...
>>         }
>> }
>>
>> Thanks,
>> Inki Dae
>>
>>> +
>>>       /* setup horizontal and vertical display size. */
>>>       val = VIDTCON2_LINEVAL(mode->vdisplay - 1) |
>>>              VIDTCON2_HOZVAL(mode->hdisplay - 1) |
>>> @@ -1014,6 +1025,9 @@ static int fimd_probe(struct platform_device *pdev)
>>>       if (of_property_read_bool(dev->of_node, "samsung,invert-vclk"))
>>>               ctx->vidcon1 |= VIDCON1_INV_VCLK;
>>>
>>> +     if (of_property_read_bool(dev->of_node, "samsung,mic-bypass"))
>>> +             ctx->mic_bypass = true;
>>> +
>>>       i80_if_timings = of_get_child_by_name(dev->of_node, "i80-if-timings");
>>>       if (i80_if_timings) {
>>>               u32 val;
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>