diff mbox

[16/33] OMAPDSS: acx565akm panel: handle gpios in panel driver

Message ID 1360765345-19312-17-git-send-email-archit@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

archit taneja Feb. 13, 2013, 2:22 p.m. UTC
The acx565akm panel driver leaves gpio configurations to the platform_enable
and disable calls in the platform's board file. These should happen in the panel
driver itself.

Create a platform data struct for the panel, this contains the reset gpio number
used by the panel driver, this struct will be passed to the panel driver as
platform data. The driver will request and configure the reset gpio rather than
leaving it to platform callbacks in board files.

This will help in removing the need for the panel drivers to have platform
related callbacks.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/displays/panel-acx565akm.c |   48 ++++++++++++++++--------
 include/video/omap-panel-data.h                |    8 ++++
 2 files changed, 41 insertions(+), 15 deletions(-)

Comments

Aaro Koskinen Feb. 13, 2013, 5:29 p.m. UTC | #1
Hi,

On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
> +static struct panel_acx565akm_data *get_panel_data(struct omap_dss_device *dssdev)
> +{
> +	return (struct panel_acx565akm_data *) dssdev->data;
> +}
> +
>  static int acx_panel_probe(struct omap_dss_device *dssdev)
>  {
>  	int r;
>  	struct acx565akm_device *md = &acx_dev;
> +	struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);

Why the get_panel_data function is needed, isn't the cast unnecessary?

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
archit taneja Feb. 14, 2013, 6:51 a.m. UTC | #2
On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
> Hi,
>
> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>> +static struct panel_acx565akm_data *get_panel_data(struct omap_dss_device *dssdev)
>> +{
>> +	return (struct panel_acx565akm_data *) dssdev->data;
>> +}
>> +
>>   static int acx_panel_probe(struct omap_dss_device *dssdev)
>>   {
>>   	int r;
>>   	struct acx565akm_device *md = &acx_dev;
>> +	struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>
> Why the get_panel_data function is needed, isn't the cast unnecessary?

the 'data' member of omap_dss_device has the type 'void *', we need to 
cast it to access the panel_acx565akm_data struct pointer.

Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Feb. 14, 2013, 6:58 a.m. UTC | #3
On 2013-02-14 08:51, Archit Taneja wrote:
> On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
>> Hi,
>>
>> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>>> +static struct panel_acx565akm_data *get_panel_data(struct
>>> omap_dss_device *dssdev)
>>> +{
>>> +    return (struct panel_acx565akm_data *) dssdev->data;
>>> +}
>>> +
>>>   static int acx_panel_probe(struct omap_dss_device *dssdev)
>>>   {
>>>       int r;
>>>       struct acx565akm_device *md = &acx_dev;
>>> +    struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>>
>> Why the get_panel_data function is needed, isn't the cast unnecessary?
> 
> the 'data' member of omap_dss_device has the type 'void *', we need to
> cast it to access the panel_acx565akm_data struct pointer.

You don't need an explicit cast to assign a void pointer to a pointer to
something else (or vice versa, I think).

I remember us having similar constructs in some other panel drivers
also. I think they are unnecessary also.

 Tomi
archit taneja Feb. 14, 2013, 7:08 a.m. UTC | #4
On Thursday 14 February 2013 12:28 PM, Tomi Valkeinen wrote:
> On 2013-02-14 08:51, Archit Taneja wrote:
>> On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
>>> Hi,
>>>
>>> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>>>> +static struct panel_acx565akm_data *get_panel_data(struct
>>>> omap_dss_device *dssdev)
>>>> +{
>>>> +    return (struct panel_acx565akm_data *) dssdev->data;
>>>> +}
>>>> +
>>>>    static int acx_panel_probe(struct omap_dss_device *dssdev)
>>>>    {
>>>>        int r;
>>>>        struct acx565akm_device *md = &acx_dev;
>>>> +    struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>>>
>>> Why the get_panel_data function is needed, isn't the cast unnecessary?
>>
>> the 'data' member of omap_dss_device has the type 'void *', we need to
>> cast it to access the panel_acx565akm_data struct pointer.
>
> You don't need an explicit cast to assign a void pointer to a pointer to
> something else (or vice versa, I think).
>
> I remember us having similar constructs in some other panel drivers
> also. I think they are unnecessary also.

Ah okay, I'll take care of it.

Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
archit taneja Feb. 18, 2013, 7:33 a.m. UTC | #5
On Thursday 14 February 2013 12:28 PM, Tomi Valkeinen wrote:
> On 2013-02-14 08:51, Archit Taneja wrote:
>> On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
>>> Hi,
>>>
>>> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>>>> +static struct panel_acx565akm_data *get_panel_data(struct
>>>> omap_dss_device *dssdev)
>>>> +{
>>>> +    return (struct panel_acx565akm_data *) dssdev->data;
>>>> +}
>>>> +
>>>>    static int acx_panel_probe(struct omap_dss_device *dssdev)
>>>>    {
>>>>        int r;
>>>>        struct acx565akm_device *md = &acx_dev;
>>>> +    struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>>>
>>> Why the get_panel_data function is needed, isn't the cast unnecessary?
>>
>> the 'data' member of omap_dss_device has the type 'void *', we need to
>> cast it to access the panel_acx565akm_data struct pointer.
>
> You don't need an explicit cast to assign a void pointer to a pointer to
> something else (or vice versa, I think).
>
> I remember us having similar constructs in some other panel drivers
> also. I think they are unnecessary also.

I was considering keeping the get_panel_data() funcs in the panel 
drivers though. This way, whenever the way of retrieving platform data 
changes because of DT or CDF or something else, we would just need to 
modify the get_panel_data func.

Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Feb. 18, 2013, 8:11 a.m. UTC | #6
On 2013-02-18 09:33, Archit Taneja wrote:
> On Thursday 14 February 2013 12:28 PM, Tomi Valkeinen wrote:
>> On 2013-02-14 08:51, Archit Taneja wrote:
>>> On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
>>>> Hi,
>>>>
>>>> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>>>>> +static struct panel_acx565akm_data *get_panel_data(struct
>>>>> omap_dss_device *dssdev)
>>>>> +{
>>>>> +    return (struct panel_acx565akm_data *) dssdev->data;
>>>>> +}
>>>>> +
>>>>>    static int acx_panel_probe(struct omap_dss_device *dssdev)
>>>>>    {
>>>>>        int r;
>>>>>        struct acx565akm_device *md = &acx_dev;
>>>>> +    struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>>>>
>>>> Why the get_panel_data function is needed, isn't the cast unnecessary?
>>>
>>> the 'data' member of omap_dss_device has the type 'void *', we need to
>>> cast it to access the panel_acx565akm_data struct pointer.
>>
>> You don't need an explicit cast to assign a void pointer to a pointer to
>> something else (or vice versa, I think).
>>
>> I remember us having similar constructs in some other panel drivers
>> also. I think they are unnecessary also.
> 
> I was considering keeping the get_panel_data() funcs in the panel
> drivers though. This way, whenever the way of retrieving platform data
> changes because of DT or CDF or something else, we would just need to
> modify the get_panel_data func.

I think it's simpler if we manage the fetching of the platform data in
probe one, and just store a struct panel_acx565akm_data or such to the
panel driver's data.

For DT we don't have platform data, but we can create the same platform
data struct from the DT properties.

So the above would become something like:

struct acx565akm_device *md = &acx_dev;
struct panel_acx565akm_data *panel_data = md->pdata;

 Tomi
archit taneja Feb. 18, 2013, 8:23 a.m. UTC | #7
On Monday 18 February 2013 01:41 PM, Tomi Valkeinen wrote:
> On 2013-02-18 09:33, Archit Taneja wrote:
>> On Thursday 14 February 2013 12:28 PM, Tomi Valkeinen wrote:
>>> On 2013-02-14 08:51, Archit Taneja wrote:
>>>> On Wednesday 13 February 2013 10:59 PM, Aaro Koskinen wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, Feb 13, 2013 at 07:52:08PM +0530, Archit Taneja wrote:
>>>>>> +static struct panel_acx565akm_data *get_panel_data(struct
>>>>>> omap_dss_device *dssdev)
>>>>>> +{
>>>>>> +    return (struct panel_acx565akm_data *) dssdev->data;
>>>>>> +}
>>>>>> +
>>>>>>     static int acx_panel_probe(struct omap_dss_device *dssdev)
>>>>>>     {
>>>>>>         int r;
>>>>>>         struct acx565akm_device *md = &acx_dev;
>>>>>> +    struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
>>>>>
>>>>> Why the get_panel_data function is needed, isn't the cast unnecessary?
>>>>
>>>> the 'data' member of omap_dss_device has the type 'void *', we need to
>>>> cast it to access the panel_acx565akm_data struct pointer.
>>>
>>> You don't need an explicit cast to assign a void pointer to a pointer to
>>> something else (or vice versa, I think).
>>>
>>> I remember us having similar constructs in some other panel drivers
>>> also. I think they are unnecessary also.
>>
>> I was considering keeping the get_panel_data() funcs in the panel
>> drivers though. This way, whenever the way of retrieving platform data
>> changes because of DT or CDF or something else, we would just need to
>> modify the get_panel_data func.
>
> I think it's simpler if we manage the fetching of the platform data in
> probe one, and just store a struct panel_acx565akm_data or such to the
> panel driver's data.
>
> For DT we don't have platform data, but we can create the same platform
> data struct from the DT properties.
>
> So the above would become something like:
>
> struct acx565akm_device *md = &acx_dev;
> struct panel_acx565akm_data *panel_data = md->pdata;

okay, that seems to be a better way.

Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/video/omap2/displays/panel-acx565akm.c b/drivers/video/omap2/displays/panel-acx565akm.c
index a8fb26b..d7f69c0 100644
--- a/drivers/video/omap2/displays/panel-acx565akm.c
+++ b/drivers/video/omap2/displays/panel-acx565akm.c
@@ -29,8 +29,10 @@ 
 #include <linux/sched.h>
 #include <linux/backlight.h>
 #include <linux/fb.h>
+#include <linux/gpio.h>
 
 #include <video/omapdss.h>
+#include <video/omap-panel-data.h>
 
 #define MIPID_CMD_READ_DISP_ID		0x04
 #define MIPID_CMD_READ_RED		0x06
@@ -494,21 +496,38 @@  static struct omap_video_timings acx_panel_timings = {
 	.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
 };
 
+static struct panel_acx565akm_data *get_panel_data(struct omap_dss_device *dssdev)
+{
+	return (struct panel_acx565akm_data *) dssdev->data;
+}
+
 static int acx_panel_probe(struct omap_dss_device *dssdev)
 {
 	int r;
 	struct acx565akm_device *md = &acx_dev;
+	struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
 	struct backlight_device *bldev;
 	int max_brightness, brightness;
 	struct backlight_properties props;
 
 	dev_dbg(&dssdev->dev, "%s\n", __func__);
 
+	if (!panel_data)
+		return -EINVAL;
+
 	/* FIXME AC bias ? */
 	dssdev->panel.timings = acx_panel_timings;
 
-	if (dssdev->platform_enable)
-		dssdev->platform_enable(dssdev);
+	if (gpio_is_valid(panel_data->reset_gpio)) {
+		r = devm_gpio_request_one(&dssdev->dev, panel_data->reset_gpio,
+				GPIOF_OUT_INIT_LOW, "lcd reset");
+		if (r)
+			return r;
+	}
+
+	if (gpio_is_valid(panel_data->reset_gpio))
+		gpio_set_value(panel_data->reset_gpio, 1);
+
 	/*
 	 * After reset we have to wait 5 msec before the first
 	 * command can be sent.
@@ -520,8 +539,9 @@  static int acx_panel_probe(struct omap_dss_device *dssdev)
 	r = panel_detect(md);
 	if (r) {
 		dev_err(&dssdev->dev, "%s panel detect error\n", __func__);
-		if (!md->enabled && dssdev->platform_disable)
-			dssdev->platform_disable(dssdev);
+		if (!md->enabled && gpio_is_valid(panel_data->reset_gpio))
+			gpio_set_value(panel_data->reset_gpio, 0);
+
 		return r;
 	}
 
@@ -530,8 +550,8 @@  static int acx_panel_probe(struct omap_dss_device *dssdev)
 	mutex_unlock(&acx_dev.mutex);
 
 	if (!md->enabled) {
-		if (dssdev->platform_disable)
-			dssdev->platform_disable(dssdev);
+		if (gpio_is_valid(panel_data->reset_gpio))
+			gpio_set_value(panel_data->reset_gpio, 0);
 	}
 
 	/*------- Backlight control --------*/
@@ -584,6 +604,7 @@  static void acx_panel_remove(struct omap_dss_device *dssdev)
 static int acx_panel_power_on(struct omap_dss_device *dssdev)
 {
 	struct acx565akm_device *md = &acx_dev;
+	struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
 	int r;
 
 	dev_dbg(&dssdev->dev, "%s\n", __func__);
@@ -605,11 +626,8 @@  static int acx_panel_power_on(struct omap_dss_device *dssdev)
 	/*FIXME tweak me */
 	msleep(50);
 
-	if (dssdev->platform_enable) {
-		r = dssdev->platform_enable(dssdev);
-		if (r)
-			goto fail;
-	}
+	if (gpio_is_valid(panel_data->reset_gpio))
+		gpio_set_value(panel_data->reset_gpio, 1);
 
 	if (md->enabled) {
 		dev_dbg(&md->spi->dev, "panel already enabled\n");
@@ -638,8 +656,7 @@  static int acx_panel_power_on(struct omap_dss_device *dssdev)
 	mutex_unlock(&md->mutex);
 
 	return acx565akm_bl_update_status(md->bl_dev);
-fail:
-	omapdss_sdi_display_disable(dssdev);
+
 fail_unlock:
 	mutex_unlock(&md->mutex);
 	return r;
@@ -648,6 +665,7 @@  fail_unlock:
 static void acx_panel_power_off(struct omap_dss_device *dssdev)
 {
 	struct acx565akm_device *md = &acx_dev;
+	struct panel_acx565akm_data *panel_data = get_panel_data(dssdev);
 
 	dev_dbg(&dssdev->dev, "%s\n", __func__);
 
@@ -671,8 +689,8 @@  static void acx_panel_power_off(struct omap_dss_device *dssdev)
 	 */
 	msleep(50);
 
-	if (dssdev->platform_disable)
-		dssdev->platform_disable(dssdev);
+	if (gpio_is_valid(panel_data->reset_gpio))
+		gpio_set_value(panel_data->reset_gpio, 0);
 
 	/* FIXME need to tweak this delay */
 	msleep(100);
diff --git a/include/video/omap-panel-data.h b/include/video/omap-panel-data.h
index 86da8d3..6faf0c7 100644
--- a/include/video/omap-panel-data.h
+++ b/include/video/omap-panel-data.h
@@ -117,4 +117,12 @@  struct panel_sharp_ls037v7dw01_data {
 	int ud_gpio;
 };
 
+/**
+ * acx565akm panel driver configuration data
+ * @reset_gpio: reset signal
+ */
+struct panel_acx565akm_data {
+	int reset_gpio;
+};
+
 #endif /* __OMAP_PANEL_DATA_H */