Re: [PATCH v4 3/6] drm/dsi: Try to match non-DT dsi devices

[Archit Taneja]

On 1/21/2016 9:35 PM, Thierry Reding wrote:
> On Thu, Dec 10, 2015 at 06:11:37PM +0530, Archit Taneja wrote:
> > Add a device name field in mipi_dsi_device. This name is different from
> > the actual dev name (which is of the format "hostname.reg"). When the
> > device is created via DT, this name is set to the modalias string.
>
> Why? What's the use of setting this to the modalias string?

There is no use to set it in the DT case. It's just set for the sake
of consistency between the non-DT and DT devices. For now, dsi->name
is just used for device/driver matching for non-DT devices. There's
no harm in setting it to a valid name for DT devices.

> > In the non-DT case, the driver creating the DSI device provides the
> > name by populating a filed in mipi_dsi_device_info.
> >
> > Matching for DT case would be as it was before. For the non-DT case,
> > we compare the device and driver names. Other buses (like i2c/spi)
>
> "I2C" and "SPI", please.
>
> > perform a non-DT match by comparing the device name and entries in the
> > driver's id_table. Such a mechanism isn't used for the dsi bus.
>
> "DSI", please.
>
> > Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_mipi_dsi.c | 25 ++++++++++++++++++++++++-
> >   include/drm/drm_mipi_dsi.h     |  6 ++++++
> >   2 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 9434585..5a46802 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -45,9 +45,26 @@
> >    * subset of the MIPI DCS command set.
> >    */
> >
> > +static const struct device_type mipi_dsi_device_type;
> > +
> >   static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
> >   {
> > -	return of_driver_match_device(dev, drv);
> > +	struct mipi_dsi_device *dsi;
> > +
> > +	if (dev->type == &mipi_dsi_device_type)
> > +		dsi = to_mipi_dsi_device(dev);
> > +	else
> > +		return 0;
>
> I think this check is redundant. I'm not aware of any case where the bus
> ->match() callback is called on a device that isn't on said bus.

You're right. I'll drop this.

> > +	/* attempt OF style match */
> > +	if (of_driver_match_device(dev, drv))
> > +		return 1;
> > +
> > +	/* compare dsi device and driver names */
>
> "DSI", please.
>
> > +	if (!strcmp(dsi->name, drv->name))
> > +		return 1;
> > +
> > +	return 0;
> >   }
> >
> >   static const struct dev_pm_ops mipi_dsi_device_pm_ops = {
> > @@ -125,6 +142,7 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
> >   	dsi->dev.type = &mipi_dsi_device_type;
> >   	dsi->dev.of_node = info->node;
> >   	dsi->channel = info->reg;
> > +	strlcpy(dsi->name, info->type, sizeof(dsi->name));
>
> Don't you need to check info->type != NULL before doing this?

It's not needed with the way struct mipi_dsi_device_info is currently
defined.

> >   	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
> >
> > @@ -148,6 +166,11 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
> >   	int ret;
> >   	u32 reg;
> >
> > +	if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
> > +		dev_err(dev, "modalias failure on %s\n", node->full_name);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >   	ret = of_property_read_u32(node, "reg", &reg);
> >   	if (ret) {
> >   		dev_err(dev, "device node %s has no valid reg property: %d\n",
> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > index 90f4f3c..cb084af 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -139,8 +139,11 @@ enum mipi_dsi_pixel_format {
> >   	MIPI_DSI_FMT_RGB565,
> >   };
> >
> > +#define DSI_DEV_NAME_SIZE		20
> > +
> >   /**
> >    * struct mipi_dsi_device_info - template for creating a mipi_dsi_device
> > + * @type: dsi peripheral chip type
> >    * @reg: DSI virtual channel assigned to peripheral
> >    * @node: pointer to OF device node
> >    *
> > @@ -148,6 +151,7 @@ enum mipi_dsi_pixel_format {
> >    * DSI device
> >    */
> >   struct mipi_dsi_device_info {
> > +	char type[DSI_DEV_NAME_SIZE];
>
> Why limit ourselves to 20 characters? And why even so complicated? Isn't
> the type always static when someone specifies this? Couldn't we simply
> use a const char *name here instead?

In the case where the device is registered via DT, we would need
space allocated for 'type' to copy the modalias string into it.
Having const char *type would make it a bit complicated for the
DT path.

The mipi_dsi_device_info struct was based on
the i2c_board_info/spi_board_info structs, and they have
type/modalias members declared as array of chars. I kind of
followed suit without putting to much thought on member type.

Archit

> Thierry

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