[RFC PATCH] thermal: Schedule a backup thermal shutdown workqueue after a known period of time to tackle failed poweroff
In few rare conditions like during boot up the orderly_poweroff
function might not be able to complete fully leaving the device
running at dangerously high temperatures. Hence adding a backup
workqueue to act after a known period of time and poweroff the device.
Suggested-by: Nishanth Menon
Reported-by: Nishanth Menon
Signed-off-by: Keerthy
---
drivers/thermal/Kconfig| 9 +
drivers/thermal/thermal_core.c | 26 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac6..25584ee 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -92,6 +92,15 @@ config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
endchoice
+config THERMAL_BKUP_SHUTDOWN_DELAY_MS
+int "Thermal shutdown backup delay in milli-seconds"
+depends on THERMAL
+default "5000"
+---help---
+The number of milliseconds to delay after orderly_poweroff call.
+
+Default: 5000 (5 seconds)
+
config THERMAL_GOV_FAIR_SHARE
bool "Fair-share thermal governor"
help
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..b793857 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -61,6 +61,12 @@ static DEFINE_MUTEX(thermal_governor_lock);
static struct thermal_governor *def_governor;
+#ifdef CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#define BKUP_SHUTDOWN_DELAY CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#else
+#define BKUP_SHUTDOWN_DELAY 5000
+#endif
+
static struct thermal_governor *__find_governor(const char *name)
{
struct thermal_governor *pos;
@@ -423,6 +429,18 @@ static void handle_non_critical_trips(struct
thermal_zone_device *tz,
def_governor->throttle(tz, trip);
}
+static void bkup_shutdown_func(struct work_struct *unused);
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, bkup_shutdown_func);
+
+static void bkup_shutdown_func(struct work_struct *work)
+{
+ pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+ kernel_power_off();
+
+ pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+ emergency_restart();
+}
+
static void handle_critical_trips(struct thermal_zone_device *tz,
int trip, enum thermal_trip_type trip_type)
{
@@ -443,6 +461,14 @@ static void handle_critical_trips(struct
thermal_zone_device *tz,
dev_emerg(&tz->device,
"critical temperature reached(%d C),shutting down\n",
tz->temperature / 1000);
+ /**
+* This is a backup option to shutdown the
+* system in case orderly_poweroff
+* fails
+*/
+ schedule_delayed_work(&bkup_shutdown_work,
+ msecs_to_jiffies(BKUP_SHUTDOWN_DELAY));
+
orderly_poweroff(true);
}
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] thermal: Schedule a backup thermal shutdown workqueue after a known period of time to tackle failed poweroff
Signed-off-by: Keerthy
---
drivers/thermal/Kconfig| 9 +
drivers/thermal/thermal_core.c | 26 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac6..25584ee 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -92,6 +92,15 @@ config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
endchoice
+config THERMAL_BKUP_SHUTDOWN_DELAY_MS
+int "Thermal shutdown backup delay in milli-seconds"
+depends on THERMAL
+default "5000"
+---help---
+The number of milliseconds to delay after orderly_poweroff call.
+
+Default: 5000 (5 seconds)
+
config THERMAL_GOV_FAIR_SHARE
bool "Fair-share thermal governor"
help
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..b793857 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -61,6 +61,12 @@ static DEFINE_MUTEX(thermal_governor_lock);
static struct thermal_governor *def_governor;
+#ifdef CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#define BKUP_SHUTDOWN_DELAY CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#else
+#define BKUP_SHUTDOWN_DELAY 5000
+#endif
+
static struct thermal_governor *__find_governor(const char *name)
{
struct thermal_governor *pos;
@@ -423,6 +429,18 @@ static void handle_non_critical_trips(struct
thermal_zone_device *tz,
def_governor->throttle(tz, trip);
}
+static void bkup_shutdown_func(struct work_struct *unused);
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, bkup_shutdown_func);
+
+static void bkup_shutdown_func(struct work_struct *work)
+{
+ pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+ kernel_power_off();
+
+ pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+ emergency_restart();
+}
+
static void handle_critical_trips(struct thermal_zone_device *tz,
int trip, enum thermal_trip_type trip_type)
{
@@ -443,6 +461,14 @@ static void handle_critical_trips(struct
thermal_zone_device *tz,
dev_emerg(&tz->device,
"critical temperature reached(%d C),shutting down\n",
tz->temperature / 1000);
+ /**
+* This is a backup option to shutdown the
+* system in case orderly_poweroff
+* fails
+*/
+ schedule_delayed_work(&bkup_shutdown_work,
+ msecs_to_jiffies(BKUP_SHUTDOWN_DELAY));
+
orderly_poweroff(true);
}
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] thermal: Schedule a backup thermal shutdown workqueue after a known period of time to tackle failed poweroff
Please ignore this patch. I will resend the right one.
Sorry about the noise.
On Monday 21 December 2015 11:14 AM, Keerthy wrote:
Signed-off-by: Keerthy
---
drivers/thermal/Kconfig| 9 +
drivers/thermal/thermal_core.c | 26 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac6..25584ee 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -92,6 +92,15 @@ config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
endchoice
+config THERMAL_BKUP_SHUTDOWN_DELAY_MS
+int "Thermal shutdown backup delay in milli-seconds"
+depends on THERMAL
+default "5000"
+---help---
+The number of milliseconds to delay after orderly_poweroff call.
+
+Default: 5000 (5 seconds)
+
config THERMAL_GOV_FAIR_SHARE
bool "Fair-share thermal governor"
help
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..b793857 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -61,6 +61,12 @@ static DEFINE_MUTEX(thermal_governor_lock);
static struct thermal_governor *def_governor;
+#ifdef CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#define BKUP_SHUTDOWN_DELAY CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#else
+#define BKUP_SHUTDOWN_DELAY 5000
+#endif
+
static struct thermal_governor *__find_governor(const char *name)
{
struct thermal_governor *pos;
@@ -423,6 +429,18 @@ static void handle_non_critical_trips(struct
thermal_zone_device *tz,
def_governor->throttle(tz, trip);
}
+static void bkup_shutdown_func(struct work_struct *unused);
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, bkup_shutdown_func);
+
+static void bkup_shutdown_func(struct work_struct *work)
+{
+ pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+ kernel_power_off();
+
+ pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+ emergency_restart();
+}
+
static void handle_critical_trips(struct thermal_zone_device *tz,
int trip, enum thermal_trip_type trip_type)
{
@@ -443,6 +461,14 @@ static void handle_critical_trips(struct
thermal_zone_device *tz,
dev_emerg(&tz->device,
"critical temperature reached(%d C),shutting down\n",
tz->temperature / 1000);
+ /**
+* This is a backup option to shutdown the
+* system in case orderly_poweroff
+* fails
+*/
+ schedule_delayed_work(&bkup_shutdown_work,
+ msecs_to_jiffies(BKUP_SHUTDOWN_DELAY));
+
orderly_poweroff(true);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
OMAP PM question
Hello all, For omap4, I wonder how should emif be handled when suspending the entire SoC. In playing with Robert Nelson's 4.2 kernel tree [1], I noticed that the suspend seems broken: the kernel, in the suspend_noirq phase, tries to disable the emif hwmod by calling omap_device_idle(); the board thus hangs. It appears to me that the DDR shouldn't be disabled at this moment or it needs special treatment -- in comparison, the AM35xx kernel switches its execution to on-chip SRAM before turning off the emif. I tried to examine earlier kernels (3.2, 3.4) for omap4 to see how emif is power-managed. However I didn't see much useful information. Any help is appreciated. 1. https://eewiki.net/display/linuxonarm/PandaBoard Best, Renju -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: OMAP2+: LogicPD Torpedo: Add Touchscreen Support
The development kit uses a TSC2004 chip attached to I2C3.
Signed-off-by: Adam Ford
---
arch/arm/boot/dts/logicpd-torpedo-37xx-devkit.dts | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/arm/boot/dts/logicpd-torpedo-37xx-devkit.dts
b/arch/arm/boot/dts/logicpd-torpedo-37xx-devkit.dts
index 6854bda..fb13f18 100644
--- a/arch/arm/boot/dts/logicpd-torpedo-37xx-devkit.dts
+++ b/arch/arm/boot/dts/logicpd-torpedo-37xx-devkit.dts
@@ -204,6 +204,12 @@
>;
};
+ tsc2004_pins: pinmux_tsc2004_pins {
+ pinctrl-single,pins = <
+ OMAP3_CORE1_IOPAD(0x2186, PIN_INPUT | MUX_MODE4)
/* mcbsp4_dr.gpio_153 */
+ >;
+ };
+
backlight_pins: pinmux_backlight_pins {
pinctrl-single,pins = <
OMAP3_CORE1_IOPAD(0x20B8, PIN_OUTPUT | MUX_MODE4)
/* gpmc_ncs5.gpio_56 */
@@ -262,6 +268,27 @@
};
};
+&i2c3 {
+ touchscreen: tsc2004@48 {
+ compatible = "ti,tsc2004";
+ reg = <0x48>;
+ vio-supply = <&vaux1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&tsc2004_pins>;
+ interrupts-extended = <&gpio5 25 IRQ_TYPE_EDGE_RISING>; /* gpio
153 */
+
+ touchscreen-fuzz-x = <4>;
+ touchscreen-fuzz-y = <7>;
+ touchscreen-fuzz-pressure = <2>;
+ touchscreen-size-x = <4096>;
+ touchscreen-size-y = <4096>;
+ touchscreen-max-pressure = <2048>;
+
+ ti,x-plate-ohms = <280>;
+ ti,esd-recovery-timeout-ms = <8000>;
+ };
+};
+
&uart1 {
interrupts-extended = <&intc 72 &omap3_pmx_core OMAP3_UART1_RX>;
};
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 9/9] phy: omap-usb2: use *syscon* framework API to power on/off the PHY
Hi Rob, On Sunday 20 December 2015 09:09 AM, Rob Herring wrote: > On Tue, Dec 15, 2015 at 02:46:08PM +0530, Kishon Vijay Abraham I wrote: >> Deprecate using phy-omap-control driver to power on/off the PHY, >> and use *syscon* framework to do the same. This handles >> powering on/off the PHY for the USB2 PHYs used in various TI SoCs. >> >> Signed-off-by: Kishon Vijay Abraham I >> --- >> Documentation/devicetree/bindings/phy/ti-phy.txt |8 +- >> drivers/phy/phy-omap-usb2.c | 94 >> ++ >> include/linux/phy/omap_usb.h | 23 ++ >> 3 files changed, 107 insertions(+), 18 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt >> b/Documentation/devicetree/bindings/phy/ti-phy.txt >> index 49e5b0c..a3b3945 100644 >> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt >> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt >> @@ -31,6 +31,8 @@ OMAP USB2 PHY >> >> Required properties: >> - compatible: Should be "ti,omap-usb2" >> + Should be "ti,dra7x-usb2-phy2" for the 2nd instance of USB2 PHY >> + in DRA7x > > The 2nd instance is different somehow? yeah, the bit fields are slightly different. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] ASoC: omap-hdmi-audio: add NULL test
Add NULL test on call to devm_kzalloc. The semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // @@ expression x; identifier fld; @@ * x = devm_kzalloc(...); ... when != x == NULL x->fld // Signed-off-by: Julia Lawall --- sound/soc/omap/omap-hdmi-audio.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/omap/omap-hdmi-audio.c b/sound/soc/omap/omap-hdmi-audio.c index 584b237..f83cc2b 100644 --- a/sound/soc/omap/omap-hdmi-audio.c +++ b/sound/soc/omap/omap-hdmi-audio.c @@ -368,6 +368,8 @@ static int omap_hdmi_audio_probe(struct platform_device *pdev) card->owner = THIS_MODULE; card->dai_link = devm_kzalloc(dev, sizeof(*(card->dai_link)), GFP_KERNEL); + if (!card->dai_link) + return -ENOMEM; card->dai_link->name = card->name; card->dai_link->stream_name = card->name; card->dai_link->cpu_dai_name = dev_name(ad->dssdev); -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] add NULL test
Add NULL tests on various calls to kzalloc and devm_kzalloc. The semantic match that finds these problems is as follows: (http://coccinelle.lip6.fr/) // @@ expression x,y; identifier fld; @@ ( x = \(vmalloc\|kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|krealloc\| kmemdup\|kstrdup\| devm_kzalloc\|devm_kmalloc\|devm_kcalloc\|devm_kasprintf\| kmalloc_array\)(...,<+... __GFP_NOFAIL ...+>,...); | * x = \(vmalloc\|kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|krealloc\| kmemdup\|kstrdup\| devm_kzalloc\|devm_kmalloc\|devm_kcalloc\|devm_kasprintf\| kmalloc_array\)(...); ) ... when != (x) == NULL when != (x) != NULL when != (x) == 0 when != (x) != 0 when != x = y ( x->fld | *x | x[...] ) // --- drivers/s390/char/con3215.c |2 ++ drivers/s390/char/raw3270.c |2 ++ sound/soc/fsl/imx-pcm-dma.c |2 ++ sound/soc/intel/baytrail/sst-baytrail-pcm.c |2 ++ sound/soc/omap/omap-hdmi-audio.c|2 ++ 5 files changed, 10 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] devicetree/bindings: add reset-gpios and vcc-supply for panel-dpi
Some displays have a reset input and/or need a regulator to function properly. Allow to specify them for panel-dpi devices. Signed-off-by: Uwe Kleine-König --- Documentation/devicetree/bindings/display/panel/panel-dpi.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.txt b/Documentation/devicetree/bindings/display/panel/panel-dpi.txt index 216c894d4f99..b52ac52757df 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.txt +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.txt @@ -7,6 +7,8 @@ Required properties: Optional properties: - label: a symbolic name for the panel - enable-gpios: panel enable gpio +- reset-gpios: GPIO to control the RESET pin +- vcc-supply: phandle of regulator that will be used to enable power to the display Required nodes: - "panel-timing" containing video timings -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] fbdev: omap2: panel-dpi: make (limited) use of a reset gpio
Some displays have a reset input. To assert that the display is functional the reset gpio must be deasserted. Teach the driver to get and drive such a gpio accordingly. Signed-off-by: Uwe Kleine-König -- Changes since (implicit) v1, sent with Message-Id: 1449753107-11410-4-git-send-email-...@kleine-koenig.org : - never assert reset because there are too many different panels with too many different needs for their reset. - split out dt binding changes - reword commit log --- drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c index e780fd4f8b46..201a1c1a6f42 100644 --- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c +++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c @@ -213,6 +213,16 @@ static int panel_dpi_probe_of(struct platform_device *pdev) ddata->enable_gpio = gpio; + /* +* Many different panels are supported by this driver and there are +* probably very different needs for their reset pins in regards to +* timing and order relative to the enable gpio. So for now it's just +* ensured that the reset line isn't active. +*/ + gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(gpio)) + return PTR_ERR(gpio); + ddata->backlight_gpio = -ENOENT; r = of_get_display_timing(node, "panel-timing", &timing); -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] fbdev: omap2: panel-dpi: support reset-gpios and vcc regulator
Hello, these patches are what is remaining from my previous series sent starting with Message-Id: 1449753107-11410-1-git-send-email-...@kleine-koenig.org . I split out the changes to Documentation/devicetree/bindings/video/panel-dpi.txt in a (single) separate patch. Also I changed the behaviour of the reset gpio to never assert it because there are too many different needs. Have fun Uwe Uwe Kleine-König (3): devicetree/bindings: add reset-gpios and vcc-supply for panel-dpi fbdev: omap2: panel-dpi: make (limited) use of a reset gpio fbdev: omap2: panel-dpi: implement support for a vcc regulator .../bindings/display/panel/panel-dpi.txt | 2 ++ drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 23 ++ 2 files changed, 25 insertions(+) -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] fbdev: omap2: panel-dpi: implement support for a vcc regulator
To allow supporting displays that need some logic to enable power to the
display try to get a vcc-supply property from the device tree and drive
the resulting regulator accordingly.
Signed-off-by: Uwe Kleine-König
---
Changes since (implicit) v1, sent with
Message-Id: 1449753107-11410-5-git-send-email-...@kleine-koenig.org:
- split out dt binding changes
- reword commit log
---
drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
index 201a1c1a6f42..8c3f31ebff00 100644
--- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
+++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -32,6 +33,7 @@ struct panel_drv_data {
int backlight_gpio;
struct gpio_desc *enable_gpio;
+ struct regulator *vcc_supply;
};
#define to_panel_data(p) container_of(p, struct panel_drv_data, dssdev)
@@ -83,6 +85,12 @@ static int panel_dpi_enable(struct omap_dss_device *dssdev)
if (r)
return r;
+ r = regulator_enable(ddata->vcc_supply);
+ if (r) {
+ in->ops.dpi->disable(in);
+ return r;
+ }
+
gpiod_set_value_cansleep(ddata->enable_gpio, 1);
if (gpio_is_valid(ddata->backlight_gpio))
@@ -105,6 +113,7 @@ static void panel_dpi_disable(struct omap_dss_device
*dssdev)
gpio_set_value_cansleep(ddata->backlight_gpio, 0);
gpiod_set_value_cansleep(ddata->enable_gpio, 0);
+ regulator_disable(ddata->vcc_supply);
in->ops.dpi->disable(in);
@@ -223,6 +232,10 @@ static int panel_dpi_probe_of(struct platform_device *pdev)
if (IS_ERR(gpio))
return PTR_ERR(gpio);
+ ddata->vcc_supply = devm_regulator_get(&pdev->dev, "vcc");
+ if (IS_ERR(ddata->vcc_supply))
+ return PTR_ERR(ddata->vcc_supply);
+
ddata->backlight_gpio = -ENOENT;
r = of_get_display_timing(node, "panel-timing", &timing);
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] fbdev: omap2: panel-dpi: allow specification of a reset gpio
[Cc += devicet...@vger.kernel.org]
Hello Tomi,
On Wed, Dec 16, 2015 at 07:35:30PM +0200, Tomi Valkeinen wrote:
>
> On 10/12/15 15:11, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König
> >
> > Some displays have a reset input. To assert that the display is
> > functional the reset gpio must be deasserted.
> >
> > Teach the driver to get and drive such a gpio accordingly.
> >
> > Signed-off-by: Uwe Kleine-König
> > ---
> > Documentation/devicetree/bindings/video/panel-dpi.txt | 1 +
> > drivers/video/fbdev/omap2/displays-new/panel-dpi.c| 7 +++
> > 2 files changed, 8 insertions(+)
>
> DT changes should be posted to devicet...@vger.kernel.org. And, I think,
> the binding document changes are usually as a separate patch.
Right for devicet...@vger.kernel.org, but separate patches for bindings
is news to me. Anyhow, I can split this off if you prefer.
> > diff --git a/Documentation/devicetree/bindings/video/panel-dpi.txt
> > b/Documentation/devicetree/bindings/video/panel-dpi.txt
> > index a40180b05bab..1a1d8f6f884f 100644
> > --- a/Documentation/devicetree/bindings/video/panel-dpi.txt
> > +++ b/Documentation/devicetree/bindings/video/panel-dpi.txt
> > @@ -7,6 +7,7 @@ Required properties:
> > Optional properties:
> > - label: a symbolic name for the panel
> > - enable-gpios: panel enable gpio
> > +- reset-gpios: GPIO to control the RESET pin
> >
> > Required nodes:
> > - "panel-timing" containing video timings
> > diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > index 1216341a0d19..7e2f9e0813dc 100644
> > --- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > +++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > @@ -32,6 +32,7 @@ struct panel_drv_data {
> > int backlight_gpio;
> >
> > struct gpio_desc *enable_gpio;
> > + struct gpio_desc *reset_gpio;
> > };
> >
> > #define to_panel_data(p) container_of(p, struct panel_drv_data, dssdev)
> > @@ -83,6 +84,7 @@ static int panel_dpi_enable(struct omap_dss_device
> > *dssdev)
> > if (r)
> > return r;
> >
> > + gpiod_set_value_cansleep(ddata->reset_gpio, 0);
> > gpiod_set_value_cansleep(ddata->enable_gpio, 1);
> >
> > if (gpio_is_valid(ddata->backlight_gpio))
> > @@ -211,6 +213,11 @@ static int panel_dpi_probe_of(struct platform_device
> > *pdev)
> > if (IS_ERR(ddata->enable_gpio))
> > return PTR_ERR(ddata->enable_gpio);
> >
> > + ddata->reset_gpio = devm_gpiod_get_optional(&pdev->dev,
> > + "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ddata->reset_gpio))
> > + return PTR_ERR(ddata->reset_gpio);
> > +
> > ddata->backlight_gpio = -ENOENT;
> >
> > r = of_get_display_timing(node, "panel-timing", &timing);
> >
>
> This looks a bit odd to me. This only ever sets the reset gpio to 0, on
> panel enable. If we never toggle the reset, it could be set to 0 at
> probe time, right?
>
> Reset is a bit tricky. I've seen panels where you have to have only a
> short reset pulse (reset high, wait a short time, reset low). If you
> leave the reset high, the panel draws extra power.
>
> So I think the best we can do in a generic way is just to ensure the
> reset is not asserted.
Good idea, will implement this in v2.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fbdev: omap2: panel-dpi: drop assignment to local variable
Hello, On Wed, Dec 16, 2015 at 07:29:17PM +0200, Tomi Valkeinen wrote: > On 10/12/15 15:11, Uwe Kleine-König wrote: > > From: Uwe Kleine-König > > > > The variable gpio is only used to store the return value of > > devm_gpiod_get_optional just to assign it to a member of the driver > > data. > > > > Get rid of this local variable and assign to driver data directly. > > > > Signed-off-by: Uwe Kleine-König > > --- > > drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 10 -- > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c > > b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c > > index e780fd4f8b46..1216341a0d19 100644 > > --- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c > > +++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c > > @@ -205,13 +205,11 @@ static int panel_dpi_probe_of(struct platform_device > > *pdev) > > int r; > > struct display_timing timing; > > struct videomode vm; > > - struct gpio_desc *gpio; > > > > - gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW); > > - if (IS_ERR(gpio)) > > - return PTR_ERR(gpio); > > - > > - ddata->enable_gpio = gpio; > > + ddata->enable_gpio = devm_gpiod_get_optional(&pdev->dev, > > +"enable", GPIOD_OUT_LOW); > > + if (IS_ERR(ddata->enable_gpio)) > > + return PTR_ERR(ddata->enable_gpio); > > > > ddata->backlight_gpio = -ENOENT; > > I usually try to avoid writing bad values to fields. Here > ddata->enable_gpio may get an error ptr. It probably doesn't matter as > we bail out right away, but still. If devm_gpiod_get_optional's return > value would be NULL or valid gpio_desc*, then it'd be fine. this is probably a matter of taste but still I don't see why people don't like writing to structs immediately. With the local variable you might have gpio = -ESOMETHING and ddata->enable_gpio = NULL; In the case that the error is handled correctly it doesn't matter if the value was written to the struct or not (if you accept a little performance penalty for writing the value actually to memory maybe). So the motivation is the consideration that the error might not be handled correctly after a later patch, right? But when ddata->enable_gpio is a negative error code this probably results in a crash already during development of the faulty patch, while when the struct's member isn't assigned it probably doesn't. This convinces me that writing to the struct is actually a good thing. Additionally even though the line length of gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(gpio)) return PTR_ERR(gpio); ddata->enable_gpio = gpio; is shorter (which is good), with my approach of doing: ddata->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(ddata->enable_gpio)) return PTR_ERR(ddata->enable_gpio); apart from saving an assignment also "enable_gpio" and "enable" are nearer to each other which IMHO makes it easier to see that the assignment is correct which outweighs the longer lines. This argument even gets more important when reset_gpio is added in patch 4 when the situation looks as follows: gpio = devm_gpiod_get_optional(... "enable" ...); if (IS_ERR(gpio)) ... ddata->enable_gpio = gpio; gpio = devm_gpiod_get_optional(... "reset" ...); if (IS_ERR(gpio)) ... ddata->reset_gpio = gpio; vs. ddata->enable_gpio = devm_gpiod_get_optional(... "enable" ...); if (IS_ERR(ddata->enable_gpio)) ... ddata->reset_gpio = devm_gpiod_get_optional(... "reset" ...); if (IS_ERR(ddata->reset_gpio)) ... I like my approach better, but if you don't agree, I don't care enough to argue (more). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
