Re: [RFC 1/2] drm/dsi: Create dummy DSI devices

From: Archit Taneja <architt@codeaurora.org>
To: Andrzej Hajda <a.hajda@samsung.com>, dri-devel@lists.freedesktop.org
Cc: linux-arm-msm@vger.kernel.org, treding@nvidia.com,
	inki.dae@samsung.com, linux-kernel@vger.kernel.org,
	airlied@linux.ie, daniel@ffwll.ch, jani.nikula@linux.intel.com
Subject: Re: [RFC 1/2] drm/dsi: Create dummy DSI devices
Date: Wed, 19 Aug 2015 15:00:13 +0530	[thread overview]
Message-ID: <55D44CA5.3070701@codeaurora.org> (raw)
In-Reply-To: <55D439E6.3080604@samsung.com>

Hi,

On 08/19/2015 01:40 PM, Andrzej Hajda wrote:
> On 06/30/2015 07:24 AM, Archit Taneja wrote:
>> We can have devices where the data bus is MIPI DSI, but the control bus
>> is something else (i2c, spi etc). A typical example is i2c controlled
>> encoder bridge chips.
>>
>> Such devices too require passing DSI specific parameters (number of data
>> lanes, DSI mode flags, color format etc) to their DSI host. For a device
>> that isn't 'mipi_dsi_device', there is no way of passing such parameters.
>>
>> Provide the option of creating a dummy DSI device. The main purpose of
>> this would be to attach to a DSI host by calling mipi_dsi_attach, and
>> pass DSI params.
>>
>> Create mipi_dsi_new_dummy for creating a dummy dsi device. The driver
>> calling this needs to be aware of the mipi_dsi_host it wants to attach
>> to, and also the DSI virtual channel the DSI device intends to use.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   drivers/gpu/drm/drm_mipi_dsi.c | 78 ++++++++++++++++++++++++++++++++++++++++--
>>   include/drm/drm_mipi_dsi.h     |  2 ++
>>   2 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 2d5ca8ee..9bfe215 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -47,7 +47,14 @@
>>
>>   static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
>>   {
>> -	return of_driver_match_device(dev, drv);
>> +	if (of_driver_match_device(dev, drv))
>> +		return 1;
>> +
>> +	if (!strcmp(drv->name, "mipi_dsi_dummy") &&
>> +			strstr(dev_name(dev), "dummy_dev"))
>> +		return 1;
>
> Is this kind of fuzzy matching used in other dummy devs? It looks little bit
> scary. You
> can at least replace
>
> strstr(dev_name(dev), "dummy_dev"))
>
> with
>
> strstr(dev_name(dev), ".dummy_dev."))
>
> Anyway, currently it should not break anything, am I right?

I took i2c's dummy dev creation as reference. The i2c_driver struct has
an id_table param, that allows the match function (i2c_device_match) to
not have a special case to check for a dummy device.

We could a 'id_table' entry in mipi_dsi_driver, and a 'name' entry in
mipi_dsi_device. But that would be a bit of an overkill just to support
dummy devices.

I could make the check more thorough by adding a func which does
something similar to 'i2c_verify_client', but I think we would
still need the above string.

I will change "dummy_dev" to ".dummy_dev.". I grepped the kernel for
devices named "dummy_dev", but didn't find anything as such, so it
shouldn't really break anything.

>
>> +
>> +	return 0;
>>   }
>>
>>   static const struct dev_pm_ops mipi_dsi_device_pm_ops = {
>> @@ -171,6 +178,67 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
>>   	return dsi;
>>   }
>>
>> +static int dummy_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dummy_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void dummy_shutdown(struct mipi_dsi_device *dsi)
>> +{
>> +}
>
> I suppose these callbacks are optional, so you can omit them.

Right. I will remove these.

Thanks for the review.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project