Re: [PATCH v4 2/6] drm/dsi: Refactor device creation

[Archit Taneja <architt@codeaurora.org>]

On 1/21/2016 9:16 PM, Thierry Reding wrote:
> On Thu, Dec 10, 2015 at 06:11:36PM +0530, Archit Taneja wrote:
>> Simplify the mipi dsi device creation process. device_initialize and
>
> "MIPI" and "DSI", please.

Sure, I'll replace with these and in the other patches.

>
>> device_add don't need to be called separately when creating
>> mipi_dsi_device's. Use device_register instead to simplify things.
>>
>> Create a helper function mipi_dsi_device_new which takes in struct
>> mipi_dsi_device_info and mipi_dsi_host. It clubs the functions
>> mipi_dsi_device_alloc and mipi_dsi_device_add into one.
>>
>> mipi_dsi_device_info acts as a template to populate the dsi device
>> information. This is populated by of_mipi_dsi_device_add and passed to
>> mipi_dsi_device_new.
>>
>> Later on, we'll provide mipi_dsi_device_new as a standalone way to create
>> a dsi device not available via DT.
>>
>> The new device creation process tries to closely follow what's been done
>> in i2c_new_device in i2c-core.
>>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   drivers/gpu/drm/drm_mipi_dsi.c | 61 +++++++++++++++++-------------------------
>>   include/drm/drm_mipi_dsi.h     | 15 +++++++++++
>>   2 files changed, 40 insertions(+), 36 deletions(-)
>
> To be honest, I'm not sure I like this. If you want to have a simpler
> helper, why not implement it using the lower-level helpers. Really the
> only thing you're doing here is add a high-level helper that takes an
> info struct, whereas previously the same would be done by storing the
> info directly in the structure between allocation and addition of the
> device.
>
> Initially the implementation was following that of platform devices, I
> see no reason to deviate from that. What you want here can easily be

I don't see why we need to call device_initialize and device_add
separately for DSI devices. From my (limited) understanding, we should
call these separately if we want to take a reference (using 
get_device()), or set up some private data before the bus's
notifier kicks in.

Since the main purpose of the series is not to simplify the device
creation code, I can drop this.

> done by something like:
>
> 	struct mipi_dsi_device *
> 	mipi_dsi_device_register_full(struct mipi_dsi_host *host,
> 				      const struct mipi_dsi_device_info *info)
> 	{
> 		struct mipi_dsi_device *dsi;
>
> 		dsi = mipi_dsi_device_alloc(host);
> 		if (IS_ERR(dsi))
> 			return dsi;
>
> 		dsi->dev.of_node = info->node;
> 		dsi->channel = info->channel;
>
> 		err = mipi_dsi_device_add(dsi);
> 		if (err < 0) {
> 			...
> 		}
>
> 		return dsi;
> 	}
>
> Thierry
>

This does look less intrusive. I'll consider switching to this.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel