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

[Archit Taneja]

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@xxxxxxxxxxx>
> > Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> > ---
> >   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