[RFC,v2,2/5] drm/dsi: Try to match non-DT dsi devices
diff mbox

Message ID 1444123482-25579-3-git-send-email-architt@codeaurora.org
State New
Headers show

Commit Message

Archit Taneja Oct. 6, 2015, 9:24 a.m. UTC
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.
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. Matching by comparing driver and
device names isn't the best thing to do. I'd appreciate some suggestions
here.

Other buses (like i2c/spi) perform a non-DT match by comparing the
device name and entries in the driver's id_table. The id_table structs
for different buses are defined in "include/linux/mod_devicetable.h", I
didn't want to touch that for now.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 29 ++++++++++++++++++++++++++++-
 include/drm/drm_mipi_dsi.h     |  8 ++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

Comments

Andrzej Hajda Oct. 30, 2015, 12:42 p.m. UTC | #1
On 10/06/2015 11:24 AM, 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.
> 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. Matching by comparing driver and
> device names isn't the best thing to do. I'd appreciate some suggestions
> here.
>
> Other buses (like i2c/spi) perform a non-DT match by comparing the
> device name and entries in the driver's id_table. The id_table structs
> for different buses are defined in "include/linux/mod_devicetable.h", I
> didn't want to touch that for now.
>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
Beside small comments below.

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 29 ++++++++++++++++++++++++++++-
>  include/drm/drm_mipi_dsi.h     |  8 ++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 245ecfe..46ee515 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -45,9 +45,30 @@
>   * 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;
> +
> +	/* Attempt OF style match */
> +	if (of_driver_match_device(dev, drv))
> +		return 1;
> +
> +	/*
> +	 * Try to compare dsi device and driver names. If this matching approach
> +	 * isn't strong, we'd probably want the dsi drivers to populate the
> +	 * id_table field and use that instead
> +	 */
> +	if (!strcmp(dsi->name, drv->name))
> +		return 1;
> +
> +	return 0;
>  }
>  
>  static const struct dev_pm_ops mipi_dsi_device_pm_ops = {
> @@ -125,6 +146,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->name, sizeof(dsi->name));
>  
>  	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
>  
> @@ -147,6 +169,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.name, sizeof(info.name)) < 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..93dec7b 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
> + * @name: name of the dsi peripheral
>   * @reg: DSI virtual channel assigned to peripheral
>   * @node: pointer to OF device node
>   *
> @@ -148,14 +151,17 @@ enum mipi_dsi_pixel_format {
>   * DSI device
>   */
>  struct mipi_dsi_device_info {
> +	char name[DSI_DEV_NAME_SIZE];
I wonder if it shouldn't be called type as in case of i2c,
but no strong feelings.
>  	u32 reg;
>  	struct device_node *node;
>  };
>  
> +
Empty line.

>  /**
>   * struct mipi_dsi_device - DSI peripheral device
>   * @host: DSI host for this peripheral
>   * @dev: driver model device node for this peripheral
> + * @name: name of the dsi peripheral
>   * @channel: virtual channel assigned to the peripheral
>   * @format: pixel format for video mode
>   * @lanes: number of active data lanes
> @@ -165,6 +171,8 @@ struct mipi_dsi_device {
>  	struct mipi_dsi_host *host;
>  	struct device dev;
>  
> +	char name[DSI_DEV_NAME_SIZE];
> +
This empty line can be also removed.

Regards
Andrzej

>  	unsigned int channel;
>  	unsigned int lanes;
>  	enum mipi_dsi_pixel_format format;
Archit Taneja Nov. 2, 2015, 5:26 a.m. UTC | #2
On 10/30/2015 06:12 PM, Andrzej Hajda wrote:
> On 10/06/2015 11:24 AM, 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.
>> 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. Matching by comparing driver and
>> device names isn't the best thing to do. I'd appreciate some suggestions
>> here.
>>
>> Other buses (like i2c/spi) perform a non-DT match by comparing the
>> device name and entries in the driver's id_table. The id_table structs
>> for different buses are defined in "include/linux/mod_devicetable.h", I
>> didn't want to touch that for now.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> Beside small comments below.
>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
>> ---
>>   drivers/gpu/drm/drm_mipi_dsi.c | 29 ++++++++++++++++++++++++++++-
>>   include/drm/drm_mipi_dsi.h     |  8 ++++++++
>>   2 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 245ecfe..46ee515 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -45,9 +45,30 @@
>>    * 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;
>> +
>> +	/* Attempt OF style match */
>> +	if (of_driver_match_device(dev, drv))
>> +		return 1;
>> +
>> +	/*
>> +	 * Try to compare dsi device and driver names. If this matching approach
>> +	 * isn't strong, we'd probably want the dsi drivers to populate the
>> +	 * id_table field and use that instead
>> +	 */
>> +	if (!strcmp(dsi->name, drv->name))
>> +		return 1;
>> +
>> +	return 0;
>>   }
>>
>>   static const struct dev_pm_ops mipi_dsi_device_pm_ops = {
>> @@ -125,6 +146,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->name, sizeof(dsi->name));
>>
>>   	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
>>
>> @@ -147,6 +169,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.name, sizeof(info.name)) < 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..93dec7b 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
>> + * @name: name of the dsi peripheral
>>    * @reg: DSI virtual channel assigned to peripheral
>>    * @node: pointer to OF device node
>>    *
>> @@ -148,14 +151,17 @@ enum mipi_dsi_pixel_format {
>>    * DSI device
>>    */
>>   struct mipi_dsi_device_info {
>> +	char name[DSI_DEV_NAME_SIZE];
> I wonder if it shouldn't be called type as in case of i2c,
> but no strong feelings.

We could change it to 'type'. It'll be easier for someone to understand
what the dsi core is doing by keeping the same nomenclature as in
i2c-core.

>>   	u32 reg;
>>   	struct device_node *node;
>>   };
>>
>> +
> Empty line.
>
>>   /**
>>    * struct mipi_dsi_device - DSI peripheral device
>>    * @host: DSI host for this peripheral
>>    * @dev: driver model device node for this peripheral
>> + * @name: name of the dsi peripheral
>>    * @channel: virtual channel assigned to the peripheral
>>    * @format: pixel format for video mode
>>    * @lanes: number of active data lanes
>> @@ -165,6 +171,8 @@ struct mipi_dsi_device {
>>   	struct mipi_dsi_host *host;
>>   	struct device dev;
>>
>> +	char name[DSI_DEV_NAME_SIZE];
>> +
> This empty line can be also removed.
>

Will remove these.

Thanks,
Archit

> Regards
> Andrzej
>
>>   	unsigned int channel;
>>   	unsigned int lanes;
>>   	enum mipi_dsi_pixel_format format;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 245ecfe..46ee515 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -45,9 +45,30 @@ 
  * 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;
+
+	/* Attempt OF style match */
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	/*
+	 * Try to compare dsi device and driver names. If this matching approach
+	 * isn't strong, we'd probably want the dsi drivers to populate the
+	 * id_table field and use that instead
+	 */
+	if (!strcmp(dsi->name, drv->name))
+		return 1;
+
+	return 0;
 }
 
 static const struct dev_pm_ops mipi_dsi_device_pm_ops = {
@@ -125,6 +146,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->name, sizeof(dsi->name));
 
 	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
 
@@ -147,6 +169,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.name, sizeof(info.name)) < 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..93dec7b 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
+ * @name: name of the dsi peripheral
  * @reg: DSI virtual channel assigned to peripheral
  * @node: pointer to OF device node
  *
@@ -148,14 +151,17 @@  enum mipi_dsi_pixel_format {
  * DSI device
  */
 struct mipi_dsi_device_info {
+	char name[DSI_DEV_NAME_SIZE];
 	u32 reg;
 	struct device_node *node;
 };
 
+
 /**
  * struct mipi_dsi_device - DSI peripheral device
  * @host: DSI host for this peripheral
  * @dev: driver model device node for this peripheral
+ * @name: name of the dsi peripheral
  * @channel: virtual channel assigned to the peripheral
  * @format: pixel format for video mode
  * @lanes: number of active data lanes
@@ -165,6 +171,8 @@  struct mipi_dsi_device {
 	struct mipi_dsi_host *host;
 	struct device dev;
 
+	char name[DSI_DEV_NAME_SIZE];
+
 	unsigned int channel;
 	unsigned int lanes;
 	enum mipi_dsi_pixel_format format;