Re: [PATCH v3 4/7] drm/i2c: adv7511: Create a MIPI DSI device

[Archit Taneja <architt@codeaurora.org>]

On 05/10/2016 02:08 AM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Tuesday 03 May 2016 12:27:38 Archit Taneja wrote:
>> On 04/22/2016 10:40 AM, Archit Taneja wrote:
>>> On 04/22/2016 03:59 AM, Laurent Pinchart wrote:
>>>> On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote:
>>>>> In order to pass DSI specific parameters to the DSI host, we need the
>>>>> driver to create a mipi_dsi_device DSI device that attaches to the
>>>>> host.
>>>>>
>>>>> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI
>>>>> device using this host. Finally, attach this device to the DSI host.
>>>>>
>>>>> Populate DT parameters (number of data lanes for now) that are required
>>>>> for DSI RX to work correctly. Hardcode few other parameters (rgb,
>>>>> embedded_sync) for now.
>>>>>
>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>>
>>>> This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the
>>>> kernel won't link. As the ADV7511 doesn't require DSI, could you make it
>>>> optional with conditional compilation to avoid pulling in dead code ?
>>>
>>> You're right. The driver's Kconfig should at least select DRM_MIPI_DSI
>>> in the current state to make sure we don't break build.
>>>
>>> Do you suggest we create another config option for ADV7533, which
>>> selects DRM_MIPI_DSI and builds the ADV7533 parts? Or did you mean
>>> something else?
>>
>> Do you have any suggestions for this point? For the next revision, I've
>> just selected DRM_MIPI_DSI in the Kconfig to ensure build doesn't break.
>>
>> For this driver, to conditionally compile DRM_MIPI_DSI, it essentially
>> means we allow conditional support for ADV7533. I would imagine us
>> #ifdef-ing out the ADV7533 compatible string in the of_match_table. Is
>> this something we want to do?
>
> If it's not much trouble I think that would be useful to avoid bloating the
> kernel with unused features, yes. You might want to add stub functions to
> include/drm/drm_mipi_dsi.h when CONFIG_DRM_MIPI_DSI is not defined to avoid
> sprinkling the driver with lots of #ifdefs.

Yeah, I can do this. Now that there isn't a chance for it to get in the
4.7 merge window, there's plenty time.

Archit

>
>>>>> ---
>>>>>
>>>>> drivers/gpu/drm/i2c/adv7511.c | 130 +++++++++++++++++++++++++++++++++---
>>>>>   1 file changed, 123 insertions(+), 7 deletions(-)
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel