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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

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

-- 
Regards,

Laurent Pinchart