* [PATCH/RFC] mfd: as3711: add OF support
From: Guennadi Liakhovetski @ 2013-02-15 10:07 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Samuel Ortiz,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown,
Magnus Damm, Simon Horman, Richard Purdie, Andrew Morton,
Liam Girdwood
Add device-tree bindings to the AS3711 regulator and backlight drivers.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
As usual - comments to the new bindings are very welcome!
Documentation/devicetree/bindings/mfd/as3711.txt | 73 +++++++++++++
drivers/mfd/as3711.c | 30 +++++-
drivers/regulator/as3711-regulator.c | 69 ++++++++++++-
drivers/video/backlight/as3711_bl.c | 118 +++++++++++++++++++++-
4 files changed, 282 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/as3711.txt
diff --git a/Documentation/devicetree/bindings/mfd/as3711.txt b/Documentation/devicetree/bindings/mfd/as3711.txt
new file mode 100644
index 0000000..d98cf18
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/as3711.txt
@@ -0,0 +1,73 @@
+AS3711 is an I2C PMIC from Austria MicroSystems with multiple DCDC and LDO power
+supplies, a battery charger and an RTC. So far only bindings for the two stepup
+DCDC converters are defined. Other DCDC and LDO supplies are configured, using
+standard regulator properties, they must belong to a sub-node, called
+"regulators" and be called "sd1" to "sd4" and "ldo1" to "ldo8." Stepup converter
+configuration should be placed in a subnode, called "backlight."
+
+Compulsory properties:
+- compatible : must be "ams,as3711"
+- reg : specifies the I2C address
+
+To use the SU1 converter as a backlight source the following two properties must
+be provided:
+- su1-dev : framebuffer phandle
+- su1-max-uA : maximum current
+
+To use the SU2 converter as a backlight source the following two properties must
+be provided:
+- su2-dev : framebuffer phandle
+- su1-max-uA : maximum current
+
+Additionally one of these properties must be provided to select the type of
+feedback used:
+- su2-feedback-voltage : voltage feedback is used
+- su2-feedback-curr1 : CURR1 input used for current feedback
+- su2-feedback-curr2 : CURR2 input used for current feedback
+- su2-feedback-curr3 : CURR3 input used for current feedback
+- su2-feedback-curr-auto: automatic current feedback selection
+
+and one of these to select the over-voltage protection pin
+- su2-fbprot-lx-sd4 : LX_SD4 is used for over-voltage protection
+- su2-fbprot-gpio2 : GPIO2 is used for over-voltage protection
+- su2-fbprot-gpio3 : GPIO3 is used for over-voltage protection
+- su2-fbprot-gpio4 : GPIO4 is used for over-voltage protection
+
+If "su2-feedback-curr-auto" is selected, one or more of the following properties
+have to be specified:
+- su2-auto-curr1 : use CURR1 input for current feedback
+- su2-auto-curr2 : use CURR2 input for current feedback
+- su2-auto-curr3 : use CURR3 input for current feedback
+
+Example:
+
+as3711@40 {
+ compatible = "ams,as3711";
+ reg = <0x40>;
+
+ regulators {
+ sd4 {
+ regulator-name = "1.215V";
+ regulator-min-microvolt = <1215000>;
+ regulator-max-microvolt = <1235000>;
+ };
+ ldo2 {
+ regulator-name = "2.8V CPU";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+ };
+
+ backlight {
+ compatible = "ams,as3711-bl";
+ su2-dev = <&lcdc>;
+ su2-max-uA = <36000>;
+ su2-feedback-curr-auto;
+ su2-fbprot-gpio4;
+ su2-auto-curr1;
+ su2-auto-curr2;
+ su2-auto-curr3;
+ };
+};
diff --git a/drivers/mfd/as3711.c b/drivers/mfd/as3711.c
index e994c96..5e0e8b3 100644
--- a/drivers/mfd/as3711.c
+++ b/drivers/mfd/as3711.c
@@ -112,16 +112,37 @@ static const struct regmap_config as3711_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
+#ifdef CONFIG_OF
+static struct of_device_id as3711_of_match[] = {
+ {.compatible = "ams,as3711",},
+ {}
+};
+MODULE_DEVICE_TABLE(of, as3711_of_match);
+#endif
+
static int as3711_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct as3711 *as3711;
- struct as3711_platform_data *pdata = client->dev.platform_data;
+ struct as3711_platform_data *pdata;
unsigned int id1, id2;
int ret;
- if (!pdata)
- dev_dbg(&client->dev, "Platform data not found\n");
+ if (!client->dev.of_node) {
+ pdata = client->dev.platform_data;
+ if (!pdata)
+ dev_dbg(&client->dev, "Platform data not found\n");
+ } else {
+ if (!of_device_is_available(client->dev.of_node))
+ return -ENODEV;
+
+ pdata = devm_kzalloc(&client->dev,
+ sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(&client->dev, "Failed to allocate pdata\n");
+ return -ENOMEM;
+ }
+ }
as3711 = devm_kzalloc(&client->dev, sizeof(struct as3711), GFP_KERNEL);
if (!as3711) {
@@ -193,7 +214,8 @@ static struct i2c_driver as3711_i2c_driver = {
.driver = {
.name = "as3711",
.owner = THIS_MODULE,
- },
+ .of_match_table = of_match_ptr(as3711_of_match),
+ },
.probe = as3711_i2c_probe,
.remove = as3711_i2c_remove,
.id_table = as3711_i2c_id,
diff --git a/drivers/regulator/as3711-regulator.c b/drivers/regulator/as3711-regulator.c
index f0ba8c4..cf145fc 100644
--- a/drivers/regulator/as3711-regulator.c
+++ b/drivers/regulator/as3711-regulator.c
@@ -13,9 +13,11 @@
#include <linux/init.h>
#include <linux/mfd/as3711.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
#include <linux/slab.h>
struct as3711_regulator_info {
@@ -276,6 +278,57 @@ static struct as3711_regulator_info as3711_reg_info[] = {
#define AS3711_REGULATOR_NUM ARRAY_SIZE(as3711_reg_info)
+static const char *as3711_regulator_of_names[AS3711_REGULATOR_NUM] = {
+ [AS3711_REGULATOR_SD_1] = "sd1",
+ [AS3711_REGULATOR_SD_2] = "sd2",
+ [AS3711_REGULATOR_SD_3] = "sd3",
+ [AS3711_REGULATOR_SD_4] = "sd4",
+ [AS3711_REGULATOR_LDO_1] = "ldo1",
+ [AS3711_REGULATOR_LDO_2] = "ldo2",
+ [AS3711_REGULATOR_LDO_3] = "ldo3",
+ [AS3711_REGULATOR_LDO_4] = "ldo4",
+ [AS3711_REGULATOR_LDO_5] = "ldo5",
+ [AS3711_REGULATOR_LDO_6] = "ldo6",
+ [AS3711_REGULATOR_LDO_7] = "ldo7",
+ [AS3711_REGULATOR_LDO_8] = "ldo8",
+};
+
+static int as3711_regulator_parse_dt(struct device *dev)
+{
+ struct as3711_regulator_pdata *pdata = dev_get_platdata(dev);
+ struct device_node *regulators + of_find_node_by_name(dev->parent->of_node, "regulators");
+ struct of_regulator_match *matches, *match;
+ const int count = AS3711_REGULATOR_NUM;
+ int ret, i;
+
+ if (!regulators) {
+ dev_err(dev, "regulator node not found\n");
+ return -ENODEV;
+ }
+
+ matches = devm_kzalloc(dev, sizeof(*matches) * count, GFP_KERNEL);
+ if (!matches)
+ return -ENOMEM;
+
+ for (i = 0, match = matches; i < count; i++, match++) {
+ match->name = as3711_regulator_of_names[i];
+ match->driver_data = as3711_reg_info + i;
+ }
+
+ ret = of_regulator_match(dev->parent, regulators, matches, count);
+ if (ret < 0) {
+ dev_err(dev, "Error parsing regulator init data: %d\n", ret);
+ return ret;
+ }
+
+ for (i = 0, match = matches; i < count; i++, match++)
+ if (match->of_node)
+ pdata->init_data[i] = match->init_data;
+
+ return 0;
+}
+
static int as3711_regulator_probe(struct platform_device *pdev)
{
struct as3711_regulator_pdata *pdata = dev_get_platdata(&pdev->dev);
@@ -289,8 +342,18 @@ static int as3711_regulator_probe(struct platform_device *pdev)
int ret;
int id;
- if (!pdata)
- dev_dbg(&pdev->dev, "No platform data...\n");
+ if (!pdata) {
+ dev_err(&pdev->dev, "No platform data...\n");
+ return -ENODEV;
+ }
+
+ if (pdev->dev.parent->of_node) {
+ ret = as3711_regulator_parse_dt(&pdev->dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "DT parsing failed: %d\n", ret);
+ return ret;
+ }
+ }
regs = devm_kzalloc(&pdev->dev, AS3711_REGULATOR_NUM *
sizeof(struct as3711_regulator), GFP_KERNEL);
@@ -300,7 +363,7 @@ static int as3711_regulator_probe(struct platform_device *pdev)
}
for (id = 0, ri = as3711_reg_info; id < AS3711_REGULATOR_NUM; ++id, ri++) {
- reg_data = pdata ? pdata->init_data[id] : NULL;
+ reg_data = pdata->init_data[id];
/* No need to register if there is no regulator data */
if (!reg_data)
diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
index c6bc65d..90c9208 100644
--- a/drivers/video/backlight/as3711_bl.c
+++ b/drivers/video/backlight/as3711_bl.c
@@ -257,6 +257,109 @@ static int as3711_bl_register(struct platform_device *pdev,
return 0;
}
+static int as3711_backlight_parse_dt(struct device *dev)
+{
+ struct as3711_bl_pdata *pdata = dev_get_platdata(dev);
+ struct device_node *bl + of_find_node_by_name(dev->parent->of_node, "backlight"), *fb;
+ int ret;
+
+ if (!bl) {
+ dev_dbg(dev, "backlight node not found\n");
+ return -ENODEV;
+ }
+
+ fb = of_parse_phandle(bl, "su1-dev", 0);
+ if (fb) {
+ pdata->su1_fb = fb->full_name;
+
+ ret = of_property_read_u32(bl, "su1-max-uA", &pdata->su1_max_uA);
+ if (pdata->su1_max_uA <= 0)
+ ret = -EINVAL;
+ if (ret < 0)
+ return ret;
+ }
+
+ fb = of_parse_phandle(bl, "su2-dev", 0);
+ if (fb) {
+ int count = 0;
+
+ pdata->su2_fb = fb->full_name;
+
+ ret = of_property_read_u32(bl, "su2-max-uA", &pdata->su2_max_uA);
+ if (pdata->su2_max_uA <= 0)
+ ret = -EINVAL;
+ if (ret < 0)
+ return ret;
+
+ if (of_find_property(bl, "su2-feedback-voltage", NULL)) {
+ pdata->su2_feedback = AS3711_SU2_VOLTAGE;
+ count++;
+ }
+ if (of_find_property(bl, "su2-feedback-curr1", NULL)) {
+ pdata->su2_feedback = AS3711_SU2_CURR1;
+ count++;
+ }
+ if (of_find_property(bl, "su2-feedback-curr2", NULL)) {
+ pdata->su2_feedback = AS3711_SU2_CURR2;
+ count++;
+ }
+ if (of_find_property(bl, "su2-feedback-curr3", NULL)) {
+ pdata->su2_feedback = AS3711_SU2_CURR3;
+ count++;
+ }
+ if (of_find_property(bl, "su2-feedback-curr-auto", NULL)) {
+ pdata->su2_feedback = AS3711_SU2_CURR_AUTO;
+ count++;
+ }
+ if (count != 1)
+ return -EINVAL;
+
+ count = 0;
+ if (of_find_property(bl, "su2-fbprot-lx-sd4", NULL)) {
+ pdata->su2_fbprot = AS3711_SU2_LX_SD4;
+ count++;
+ }
+ if (of_find_property(bl, "su2-fbprot-gpio2", NULL)) {
+ pdata->su2_fbprot = AS3711_SU2_GPIO2;
+ count++;
+ }
+ if (of_find_property(bl, "su2-fbprot-gpio3", NULL)) {
+ pdata->su2_fbprot = AS3711_SU2_GPIO3;
+ count++;
+ }
+ if (of_find_property(bl, "su2-fbprot-gpio4", NULL)) {
+ pdata->su2_fbprot = AS3711_SU2_GPIO4;
+ count++;
+ }
+ if (count != 1)
+ return -EINVAL;
+
+ count = 0;
+ if (of_find_property(bl, "su2-auto-curr1", NULL)) {
+ pdata->su2_auto_curr1 = true;
+ count++;
+ }
+ if (of_find_property(bl, "su2-auto-curr2", NULL)) {
+ pdata->su2_auto_curr2 = true;
+ count++;
+ }
+ if (of_find_property(bl, "su2-auto-curr3", NULL)) {
+ pdata->su2_auto_curr3 = true;
+ count++;
+ }
+
+ /*
+ * At least one su2-auto-curr* must be specified iff
+ * AS3711_SU2_CURR_AUTO is used
+ */
+ if (!count ^ pdata->su2_feedback != AS3711_SU2_CURR_AUTO)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int as3711_backlight_probe(struct platform_device *pdev)
{
struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
@@ -266,11 +369,24 @@ static int as3711_backlight_probe(struct platform_device *pdev)
unsigned int max_brightness;
int ret;
- if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
+ if (!pdata) {
dev_err(&pdev->dev, "No platform data, exiting...\n");
return -ENODEV;
}
+ if (pdev->dev.parent->of_node) {
+ ret = as3711_backlight_parse_dt(&pdev->dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "DT parsing failed: %d\n", ret);
+ return ret;
+ }
+ }
+
+ if (!pdata->su1_fb && !pdata->su2_fb) {
+ dev_err(&pdev->dev, "No framebuffer specified\n");
+ return -EINVAL;
+ }
+
/*
* Due to possible hardware damage I chose to block all modes,
* unsupported on my hardware. Anyone, wishing to use any of those modes
--
1.7.2.5
^ permalink raw reply related
* [GIT PULL] omapdss fixes for 3.8
From: Tomi Valkeinen @ 2013-02-14 17:27 UTC (permalink / raw)
To: Linus Torvalds, linux-omap, linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]
Hi Linus,
It'd be great if these two late fixes would still make it into 3.8. The other
one fixes ARM kernel compilation when using 'allyesconfig', and the other makes
DPI displays function again on OMAP3630 boards.
Tomi
The following changes since commit 836dc9e3fbbab0c30aa6e664417225f5c1fb1c39:
Linux 3.8-rc7 (2013-02-09 08:20:39 +1100)
are available in the git repository at:
git://gitorious.org/linux-omap-dss2/linux.git tags/omapdss-for-3.8-rc8
for you to fetch changes up to 91e83ffd6d3ba4de21202b4f541777a7a8db02c8:
omapdrm: fix the dependency to omapdss (2013-02-14 13:08:29 +0200)
----------------------------------------------------------------
* Fix ARM compilation with "allyesconfig" (omapdrm: fix the dependency to
omapdss)
* fix DPI displays on OMAP3630 (OMAPDSS: add FEAT_DPI_USES_VDDS_DSI to
omap3630_dss_feat_list)
----------------------------------------------------------------
NeilBrown (1):
OMAPDSS: add FEAT_DPI_USES_VDDS_DSI to omap3630_dss_feat_list
Tomi Valkeinen (1):
omapdrm: fix the dependency to omapdss
drivers/staging/omapdrm/Kconfig | 2 +-
drivers/video/omap2/dss/dss_features.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Igor Grinberg @ 2013-02-14 13:51 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CDE12.60701@ti.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/14/13 14:52, Tomi Valkeinen wrote:
> On 2013-02-14 14:37, Igor Grinberg wrote:
>> On 02/14/13 12:59, Tomi Valkeinen wrote:
>>> On 2013-02-14 11:43, Igor Grinberg wrote:
>>
>>>>> True, it's generic, but does it work reliably? The panel hardware is now
>>>>> partly handled in the backlight driver, and partly in the omap's panel
>>>>> driver (and wherever on other platforms).
>>>>
>>>> It works reliably on other platforms, but not on OMAP - because
>>>> we need to cope with the OMAP specific framework...
>>
>>> How do you handle the gpios on other platform? Those are the ones
>>> causing the issues here, right?
>>
>> Well, I'm also talking about something that is a history already.
>> Remember, we had multiple panel drivers inside the
>> video/omap2/displays and then they were consolidated into the
>> "generic dpi/dsi/whatever".
>
> Sorry, I miss the point. Was that a bad thing? Didn't it simplify the
> task for you with simple panels? It could've been taken even further,
> though (see below).
Yes it was a good thing (I have already told this below).
>
>> And yes you are right, on the platforms I'm aware of, the GPIO is not
>> handled. Apparently its hardware default (pull resistor) is always on...
>
> Ok, so the simple fix of setting the GPIOs only in the board file is
> acceptable for now.
Yep. I also told this already in one of the previous emails.
>
> Can the LCD_BL_GPIO be handled by the omap panel driver? Otherwise the
> backlight will supposedly be always on. Is it just a simple switch for
> the BL power, which does not affect the SPI in any way?
Yes, it can for now.
Also, I think we should also take into account the backlight framework,
including PMW.
>
>>> Or is there something else with OMAP DSS that you need to specifically
>>> cope with?
>>
>> The fact there is a need to create a OMAP specific driver.
>> I'm not talking about the generic driver which only needs to have the
>> controller specific data (e.g. porches, pixel clock, bus width).
>> The generic driver was one of the good ways to go.
>
> Well, we could also have an even more generic driver that takes the
> video timings from the board file as platform data. Then all you would
> need to do is to define the timings in the board file, as I think is
> done for other platforms also.
Yep. That sounds reasonable also for cases where the bus width is the
hardware (board) property.
>
> I'm not very fond of that idea, as I think hardcoded device specific
> data should not be given as parameters, but they should be handled by
> the device driver internally, as it should know that device specific
> data already.
Yes, that is why I think the generic driver for "simple" panels
was a good idea. There was some flexibility missing though.
For example the resolution setting which in turn drags another set
of timings and pixel clock.
There are panels that support more than one resolution.
>
> But, in practice, making this kind of even more generic panel driver
> will probably make life easier for everyone, so I think we'll have one
> with CDF.
I'm looking forward to see it happening!
- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRHOvEAAoJEBDE8YO64EfaCZ0P/32SYC7MZRJMfUVrnzZtJZpn
9BKzrvO0h/GKZMbKoKKNFwmvlTxEocELLkljF35ipbc51C/dpD45ZmMBvu4s8owE
6bopGw6ssTahTKk0zY5PekTamZT7UlY86hI9ZcZBxSzY8xaj7UCSPnJ3qzY2ZGzA
4heJW01mlHr9i1qkOzxz0IHA2CdQmQltAsW12NFCWYBRpDfhrYtECwhlnvLXHEzy
nqXNpRHOnrL/hqvJwor1X+D/O1JlyxUiVr6+7sAZqXxvv/iMX8Nc1XeTObgGbse6
1bpipPD57etqISsN1g9ur1tU5f6KvoR4IA35RaCCkBFINIXMEBl7kr5X9Bcg3giE
3pz23k91KRloOcXJ0OvLHp7RnSrwswU/4C3Cse+95cY0wyChaVlwJtySEnfGALWV
aiuw89v6DXaC5WDQq55UtTc3qjc23Ehut8i184PAv5HKrDHLLjVg4HqgGjC9uGEn
JKOxOnfMTstqyKC2whW+Pep3g4npJAHsn/0QmpdSJQ/vEwm7CmXBZeR6DwcTSLAT
xR1SUWwHav2HqpYUz4y7TgIPuwsR+VNKn/mSOTbhbLB4s9bowPMxMiqz2AQn3ige
YyMTo7KnObivXZydrVrx0/e/DJC4ENGpE3macVQytr0BpjJhK1+XiNMHa17+Mymm
U45VDKrUgaYcYXW2L4JN
=xeSz
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Tomi Valkeinen @ 2013-02-14 12:52 UTC (permalink / raw)
To: Igor Grinberg; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CDA91.1050300@compulab.co.il>
[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]
On 2013-02-14 14:37, Igor Grinberg wrote:
> On 02/14/13 12:59, Tomi Valkeinen wrote:
>> On 2013-02-14 11:43, Igor Grinberg wrote:
>
>>>> True, it's generic, but does it work reliably? The panel hardware is now
>>>> partly handled in the backlight driver, and partly in the omap's panel
>>>> driver (and wherever on other platforms).
>>>
>>> It works reliably on other platforms, but not on OMAP - because
>>> we need to cope with the OMAP specific framework...
>
>> How do you handle the gpios on other platform? Those are the ones
>> causing the issues here, right?
>
> Well, I'm also talking about something that is a history already.
> Remember, we had multiple panel drivers inside the
> video/omap2/displays and then they were consolidated into the
> "generic dpi/dsi/whatever".
Sorry, I miss the point. Was that a bad thing? Didn't it simplify the
task for you with simple panels? It could've been taken even further,
though (see below).
> And yes you are right, on the platforms I'm aware of, the GPIO is not
> handled. Apparently its hardware default (pull resistor) is always on...
Ok, so the simple fix of setting the GPIOs only in the board file is
acceptable for now.
Can the LCD_BL_GPIO be handled by the omap panel driver? Otherwise the
backlight will supposedly be always on. Is it just a simple switch for
the BL power, which does not affect the SPI in any way?
>> Or is there something else with OMAP DSS that you need to specifically
>> cope with?
>
> The fact there is a need to create a OMAP specific driver.
> I'm not talking about the generic driver which only needs to have the
> controller specific data (e.g. porches, pixel clock, bus width).
> The generic driver was one of the good ways to go.
Well, we could also have an even more generic driver that takes the
video timings from the board file as platform data. Then all you would
need to do is to define the timings in the board file, as I think is
done for other platforms also.
I'm not very fond of that idea, as I think hardcoded device specific
data should not be given as parameters, but they should be handled by
the device driver internally, as it should know that device specific
data already.
But, in practice, making this kind of even more generic panel driver
will probably make life easier for everyone, so I think we'll have one
with CDF.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 27/33] OMAPDSS: n8x0 panel: handle gpio data in panel driver
From: Aaro Koskinen @ 2013-02-14 12:45 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <511C8578.1070907@ti.com>
On Thu, Feb 14, 2013 at 12:04:32PM +0530, Archit Taneja wrote:
> On Wednesday 13 February 2013 11:05 PM, Aaro Koskinen wrote:
> >On Wed, Feb 13, 2013 at 07:52:19PM +0530, Archit Taneja wrote:
> >>@@ -444,6 +445,20 @@ static int n8x0_panel_probe(struct omap_dss_device *dssdev)
> >> dssdev->ctrl.rfbi_timings = n8x0_panel_timings;
> >> dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE;
> >>
> >>+ if (gpio_is_valid(bdata->panel_reset)) {
> >>+ r = devm_gpio_request_one(&dssdev->dev, bdata->panel_reset,
> >>+ GPIOF_OUT_INIT_LOW, "PANEL RESET");
> >>+ if (r)
> >>+ return r;
> >>+ }
> >>+
> >>+ if (gpio_is_valid(bdata->ctrl_pwrdown)) {
> >>+ r = devm_gpio_request_one(&dssdev->dev, bdata->ctrl_pwrdown,
> >>+ GPIOF_OUT_INIT_LOW, "PANEL PWRDOWN");
> >>+ if (r)
> >>+ return r;
> >>+ }
> >>+
> >
> >In the error case, the other GPIO is not freed. Also maybe you should
> >free them on module removal, because now the module owns the GPIOs.
>
> Wouldn't the usage of devm_* functions take care of this? If the
> device isn't registered successfully, then all allocations/requests
> done using devm_* functions will be free'd automatically.
Sorry, I didn't realized they are devm_* now. You are right.
A.
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Igor Grinberg @ 2013-02-14 12:37 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CC37C.3020706@ti.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/14/13 12:59, Tomi Valkeinen wrote:
> On 2013-02-14 11:43, Igor Grinberg wrote:
>
>>> True, it's generic, but does it work reliably? The panel hardware is now
>>> partly handled in the backlight driver, and partly in the omap's panel
>>> driver (and wherever on other platforms).
>>
>> It works reliably on other platforms, but not on OMAP - because
>> we need to cope with the OMAP specific framework...
>
> How do you handle the gpios on other platform? Those are the ones
> causing the issues here, right?
Well, I'm also talking about something that is a history already.
Remember, we had multiple panel drivers inside the
video/omap2/displays and then they were consolidated into the
"generic dpi/dsi/whatever".
And yes you are right, on the platforms I'm aware of, the GPIO is not
handled. Apparently its hardware default (pull resistor) is always on...
>
> Or is there something else with OMAP DSS that you need to specifically
> cope with?
The fact there is a need to create a OMAP specific driver.
I'm not talking about the generic driver which only needs to have the
controller specific data (e.g. porches, pixel clock, bus width).
The generic driver was one of the good ways to go.
>
>>> Thinking about it, if you do move the gpio handling to the backlight
>>> driver, the panel driver will only handle the DPI video stream. Then it
>>> should not have any effect on the SPI side (presuming the panel doesn't
>>> use the pixel clock as func clock), although there's probably still
>>> possibility for display artifacts on enable and disable, if the order of
>>> operations goes the wrong way.
>>
>> Yep, again, that is correct.
>
> It's correct that there may be artifacts? How do you manage the ordering
> of the operations on other platforms?
Yep, there might be artifacts if the ordering is incorrect.
The ordering is something that should be solved by the CDF.
>
>>>> And since the toppoly was and is used on other systems, why the hell
>>>> should anyone duplicate the driver just to please the OMAP specific
>>>> panel framework? The real problem is that this framework should not be
>>
>>> Not to please. To make it reliable.
>>
>> Well, there are multiple ways to make it reliable.
>> And I don't think that the best would be: make it OMAP specific.
>
> I'm not saying it's the best option. I'm saying it's a realistic option
> to get it working.
>
>>> Well, if duplicating the code gives us reliable drivers, versus
>>> unreliable without duplicating, then... I don't see it as that bad.
>>
>> Hmmm... I don't think this fits the mainline (upstream) philosophy.
>> This can be also extrapolated into: let's make our own Linux ARM fork
>> so it will be more reliable...
>> This is the way how vendor specific kernel releases work.
>
> Well, we are talking about a smallish driver here. Not an arch fork. If
> the options are a) platform specific driver that works, or b) generic
> driver that's not reliable, or c) no driver at all, I can't really see
> why a) would be such a horrible option for the time being.
I think there is b.5) where the driver is both generic and reliable,
but I haven't looked into this deep enough.
>
> But this discussion is getting a bit out of hand. It sounds to me that
> for this panel in question we can manage with the current approach, so
> this whole line of discussion doesn't matter for this specific problem.
>
>>> If it was easy, somebody would've done it.
>>
>> In fact this is done all the time on multiple drivers and frameworks.
>> Also, I don't say this is easy, but I also don't think this too hard.
>> It is also a function of resources (time/will/experience/etc.).
>
> I think the CDF discussions have already proven that it is quite hard.
> But feel free to contribute.
I will contribute as much as I can in terms of my paid work.
I always do...
>
> And I've talked about a common display framework already years ago, and
> I've tried to design OMAP DSS panels from start in such a way that they
> try to depend on OMAP DSS features as little as possible, to make it
> easier to generalize them. Just to prove I'm not indifferent about the
> issue =).
Yep, your work was fruitful and no one can take it from you!
That's why I said that I'm not trying to blame or accuse anyone.
That is more of a wish that the CDF will come as quickly as it can.
Because currently it is clear that there are cases where we don't
have a proper support...
- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRHNqRAAoJEBDE8YO64Efac08QAKBgU7xFkdBHU0ekAgRIafhO
kd0EgPXwPZDvuAaoj5pzdMI65alRSuoQxp5ohHyz0aFfGRk0Q66f6/zXzTsGXPLP
pRTkqGGTHD5Qtv2IvhHPvLMRGW+87QiuK0NU0BJqV/1JRPz1UyKMJrwc5U3Acf4J
B7bQnWLlIcPftIt2zHwuvZZMYZUL8f5/dXGWMhxmTF7rse7YcTjCwQO4eabTIX5r
/GUanF+qlwKJzk7nxQR1DFQdN+7Vv8vdumJi38sq9fouLgXlhmzOgXvxMgT7lq8T
l+7Bg7l+E9yrHaXmWf3zjZBAk6/wBnzzrmmK5V6Gjg2PiEh6Rs5ARWPrdc/FQCpt
4bXTKEktLonHj2rya8intrPLw7lP4AQN81QsQOLeRIPGoSCcDsi4RGAFrWdXR0Xt
Du9ea/liAD3+qiNIErQcTlR5zDKAIhMvWycG+ZFj0kVFUFb6p/F1TKcqVy0O7HAG
SLSsrCyO8jA/UxAKQifhiUU2Hoq9a3VhQi507lbXiN0NTsk686mp/W8YJPB/di+v
44wayaE5Kyw5iawLj9WUyTpw29sOysiewp+z7XuaF3fM5lWeMy2e0/mNbe4iCcCG
LYMl0NWcQn0S7okkQz5HHmCJp3L3/Se5sMfGKJRzpG0vK1DR+m8nJh0MGbEu/14C
B9+3VrImRZS8te/wnPqE
ºnK
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Tomi Valkeinen @ 2013-02-14 10:59 UTC (permalink / raw)
To: Igor Grinberg; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CB1B3.80605@compulab.co.il>
[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]
On 2013-02-14 11:43, Igor Grinberg wrote:
>> True, it's generic, but does it work reliably? The panel hardware is now
>> partly handled in the backlight driver, and partly in the omap's panel
>> driver (and wherever on other platforms).
>
> It works reliably on other platforms, but not on OMAP - because
> we need to cope with the OMAP specific framework...
How do you handle the gpios on other platform? Those are the ones
causing the issues here, right?
Or is there something else with OMAP DSS that you need to specifically
cope with?
>> Thinking about it, if you do move the gpio handling to the backlight
>> driver, the panel driver will only handle the DPI video stream. Then it
>> should not have any effect on the SPI side (presuming the panel doesn't
>> use the pixel clock as func clock), although there's probably still
>> possibility for display artifacts on enable and disable, if the order of
>> operations goes the wrong way.
>
> Yep, again, that is correct.
It's correct that there may be artifacts? How do you manage the ordering
of the operations on other platforms?
>>> And since the toppoly was and is used on other systems, why the hell
>>> should anyone duplicate the driver just to please the OMAP specific
>>> panel framework? The real problem is that this framework should not be
>
>> Not to please. To make it reliable.
>
> Well, there are multiple ways to make it reliable.
> And I don't think that the best would be: make it OMAP specific.
I'm not saying it's the best option. I'm saying it's a realistic option
to get it working.
>> Well, if duplicating the code gives us reliable drivers, versus
>> unreliable without duplicating, then... I don't see it as that bad.
>
> Hmmm... I don't think this fits the mainline (upstream) philosophy.
> This can be also extrapolated into: let's make our own Linux ARM fork
> so it will be more reliable...
> This is the way how vendor specific kernel releases work.
Well, we are talking about a smallish driver here. Not an arch fork. If
the options are a) platform specific driver that works, or b) generic
driver that's not reliable, or c) no driver at all, I can't really see
why a) would be such a horrible option for the time being.
But this discussion is getting a bit out of hand. It sounds to me that
for this panel in question we can manage with the current approach, so
this whole line of discussion doesn't matter for this specific problem.
>> If it was easy, somebody would've done it.
>
> In fact this is done all the time on multiple drivers and frameworks.
> Also, I don't say this is easy, but I also don't think this too hard.
> It is also a function of resources (time/will/experience/etc.).
I think the CDF discussions have already proven that it is quite hard.
But feel free to contribute.
And I've talked about a common display framework already years ago, and
I've tried to design OMAP DSS panels from start in such a way that they
try to depend on OMAP DSS features as little as possible, to make it
easier to generalize them. Just to prove I'm not indifferent about the
issue =).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Igor Grinberg @ 2013-02-14 9:43 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CA9B3.70401@ti.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/14/13 11:09, Tomi Valkeinen wrote:
> On 2013-02-14 10:37, Igor Grinberg wrote:
>> On 02/14/13 09:09, Tomi Valkeinen wrote:
>>> On 2013-02-14 08:56, Igor Grinberg wrote:
>>>> On 02/13/13 17:59, Tomi Valkeinen wrote:
>>
>>>>> Okay, so I just realized there's an spi backlight driver used here, and
>>>>> that backlight driver is actually handling the SPI transactions with the
>>>>> panel, instead of the panel driver. So this looks quite messed up.
>>>>
>>>> Yep, it always was.
>>>> The whole DSS specific panel handling inside the
>>>> drivers/video/omap2/displays is a mess.
>>
>>> Well, that's not mess itself, it's just omap specific panel framework.
>>> But dividing single device handling into two separate places is a mess.
>>
>> Yes, you are right it is not the mess, but it prevents the panel to
>> be used on other systems and that is BAD.
>> At the very least, drivers/video/backlight is a generic place that can be
>> used not just on OMAP.
>
> True, it's generic, but does it work reliably? The panel hardware is now
> partly handled in the backlight driver, and partly in the omap's panel
> driver (and wherever on other platforms).
It works reliably on other platforms, but not on OMAP - because
we need to cope with the OMAP specific framework...
>
> At least currently there's a dependency between the two, as the LCD_EN
> gpio is handled by the panel driver, which affects the functioning of
> the backlight driver. Is it ensured that the panel driver does not
> disable the panel when the backlight driver does spi transactions?
>
> That's what I meant with the mess, it's difficult to make it work
> reliably. I know that for some panels such a two-driver approach would
> not work at all. Although I guess it's working well enough for you for
> this panel.
Yep, that is correct - this is the mess.
>
> Thinking about it, if you do move the gpio handling to the backlight
> driver, the panel driver will only handle the DPI video stream. Then it
> should not have any effect on the SPI side (presuming the panel doesn't
> use the pixel clock as func clock), although there's probably still
> possibility for display artifacts on enable and disable, if the order of
> operations goes the wrong way.
Yep, again, that is correct.
>
>> And since the toppoly was and is used on other systems, why the hell
>> should anyone duplicate the driver just to please the OMAP specific
>> panel framework? The real problem is that this framework should not be
>
> Not to please. To make it reliable.
Well, there are multiple ways to make it reliable.
And I don't think that the best would be: make it OMAP specific.
>
>> OMAP specific...
>> Of course I'm aware of the fact that currently there is no generic
>> panel framework, but forging something OMAP specific which is obviously
>> used on most of the other architectures/platforms (and I mean
>> panel<->controller relations), is not a good way to go.
>
> Well, if duplicating the code gives us reliable drivers, versus
> unreliable without duplicating, then... I don't see it as that bad.
Hmmm... I don't think this fits the mainline (upstream) philosophy.
This can be also extrapolated into: let's make our own Linux ARM fork
so it will be more reliable...
This is the way how vendor specific kernel releases work.
>
>> Although, I'm also aware of the fact that most things are done this way:
>> do several specific drivers/frameworks, find the common stuff, and extract
>> it into a core driver/framework. So I don't want to blame anyone - that's
>> just the way how we do things, right?
>
> If it was easy, somebody would've done it.
In fact this is done all the time on multiple drivers and frameworks.
Also, I don't say this is easy, but I also don't think this too hard.
It is also a function of resources (time/will/experience/etc.).
>
>>>>> For a quick solution, can we just set the LCD_EN at boot time (with the
>>>>> msleep), and not touch it after that?
>>>>
>>>> That would be sensible for now, so this series can be merged.
>>>> As a more appropriate (and long term) solution,
>>>> I plan on moving the panel reset pin handling to the spi backlight
>>>> driver itself.
>>
>>> Well, if you must. But I suggest moving the whole panel handling into a
>>> (omap specific) panel driver, as it's done for other panels. That way
>>> you'll have a proper panel driver for it, for omap, and when CDF comes,
>>> you'll get a platform independent panel driver for it.
>>
>> You can't just move generic architecture/platform independent stuff
>> into OMAP specific framework... Just think about this... It's insane.
>
> As I said, reliable vs unreliable. That's not insane.
>
> But again, if you can handle this particular panel reliably with the
> two-driver approach, I'm fine with it.
Again, it works reliably on other platforms,
why would OMAP be an exception?
>
>> In addition, AFAIR, the reset pin is the property of the toppoly panel
>> hardware, so that is why I think, we should let the toppoly driver
>> (currently spi backlight, later hopefully CDF) handle it correctly
>> along with the spi sequences.
>
> Yes, that sounds ok.
>
> Tomi
>
- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRHLGzAAoJEBDE8YO64EfaOJMP/AtR9IkN+eWwdbt0R1Vu9vKH
mFgSCENqErAjYi3zM0YScj+E2zSby4rfXCY14Goh46DBCBXjRWYms6P/nZgGSLYT
lIup5YljWxWJxM0nvrykok872Atx+TSJukx3nD5foWieu4tNRRvhqN7+ckBO7R4D
J1D5uy3wH8Ea3SZ/foPrzewTeajnOeZxzhprfodLdmKIuqxInVE0KrWqrcefspNI
TvWAjLCHtoM4LDCZRaiHs3mN03QMdcJc1BfWeJe1eVx6YXSBqNTTG6mSgUegQyOG
PnC1T3kzS5Vuhmk9NfUmL19LInAljPVDoQomUqG6N140M6jol4ru+A5yE/NZ/tSl
j/8vz5pE8JXp0ueQt1X1vkAGL+Lgzbyrf38xQTxnjSLggO3OFHOv6AzlY453Lfem
gz6Xpjq+2Kcqxghfndd4yXnOjdlyWDN6dvYBthBBixmt34c6nNtdoXmakAXyw8wW
qSyT3sO6WgE53ROZRh9W2FCiLXdJ1rHYMBRRY4nbKNhOhtC/vSF4UVsFBUuAaqul
a6QToMggpugY8n3lm/SZ6LFWJDaHjnkUAVXxq3/GiclJSFwBnHqsOT1bfjsF5OfC
YnaldNBbH0WvmFN89Ds/inW1MLcFc/yWirB0Utj7ysb5AL4vH4QLs1dokzjxnTyK
WJmOMyQi7Jk+ocUSBgu2
=G6lL
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v2 0/1] OMAP4: DSS: Add panel for Blaze Tablet boards
From: Tomi Valkeinen @ 2013-02-14 9:24 UTC (permalink / raw)
To: Ruslan Bilovol
Cc: andi, FlorianSchandinat, linux-fbdev, linux-kernel, linux-omap
In-Reply-To: <CAB=otbSX_O-zbHZuPWPSEU0D1s3uKfZfeatoguoRiwogS5tVHQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]
On 2013-02-14 02:07, Ruslan Bilovol wrote:
> This patch is exactly a part of mainlining of the BlazeTablet board support :)
> The goal is to have as much as possible support of this board in the
> 'vanilla' kernel.
> The BlazeTablet mainlining is already ongoing (
> https://patchwork.kernel.org/patch/2118281/ )
> and I hope we will have some support of this board in near future.
> The BlazeTablet's LCD panel support is very important thing for us to
> have it in mainline because
> without it the BlazeTablet becomes a 'black brick' and it is painful
> for us to port this
> driver against each new version of kernel.
Ok, good to hear you're pushing it to mainline. However, you should
mentally prepare for it being a black brick =(.
Tony said he's not adding new board files, only DT from now on. The
display subsystem driver and the panel drivers do not work with DT yet,
and it will still take some time for that to realize.
Thus adding this driver would help nothing, as it couldn't be used. And
after the DSS and the panel drivers are made to work with DT (which is a
big job, needing large rewrites of the drivers), there's a rework that
has to be done with this driver to make it compatible with the new DSS
model.
So, I'm still inclined to say that we shouldn't merge this.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Tomi Valkeinen @ 2013-02-14 9:09 UTC (permalink / raw)
To: Igor Grinberg; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511CA247.80606@compulab.co.il>
[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]
On 2013-02-14 10:37, Igor Grinberg wrote:
> On 02/14/13 09:09, Tomi Valkeinen wrote:
>> On 2013-02-14 08:56, Igor Grinberg wrote:
>>> On 02/13/13 17:59, Tomi Valkeinen wrote:
>
>>>> Okay, so I just realized there's an spi backlight driver used here, and
>>>> that backlight driver is actually handling the SPI transactions with the
>>>> panel, instead of the panel driver. So this looks quite messed up.
>>>
>>> Yep, it always was.
>>> The whole DSS specific panel handling inside the
>>> drivers/video/omap2/displays is a mess.
>
>> Well, that's not mess itself, it's just omap specific panel framework.
>> But dividing single device handling into two separate places is a mess.
>
> Yes, you are right it is not the mess, but it prevents the panel to
> be used on other systems and that is BAD.
> At the very least, drivers/video/backlight is a generic place that can be
> used not just on OMAP.
True, it's generic, but does it work reliably? The panel hardware is now
partly handled in the backlight driver, and partly in the omap's panel
driver (and wherever on other platforms).
At least currently there's a dependency between the two, as the LCD_EN
gpio is handled by the panel driver, which affects the functioning of
the backlight driver. Is it ensured that the panel driver does not
disable the panel when the backlight driver does spi transactions?
That's what I meant with the mess, it's difficult to make it work
reliably. I know that for some panels such a two-driver approach would
not work at all. Although I guess it's working well enough for you for
this panel.
Thinking about it, if you do move the gpio handling to the backlight
driver, the panel driver will only handle the DPI video stream. Then it
should not have any effect on the SPI side (presuming the panel doesn't
use the pixel clock as func clock), although there's probably still
possibility for display artifacts on enable and disable, if the order of
operations goes the wrong way.
> And since the toppoly was and is used on other systems, why the hell
> should anyone duplicate the driver just to please the OMAP specific
> panel framework? The real problem is that this framework should not be
Not to please. To make it reliable.
> OMAP specific...
> Of course I'm aware of the fact that currently there is no generic
> panel framework, but forging something OMAP specific which is obviously
> used on most of the other architectures/platforms (and I mean
> panel<->controller relations), is not a good way to go.
Well, if duplicating the code gives us reliable drivers, versus
unreliable without duplicating, then... I don't see it as that bad.
> Although, I'm also aware of the fact that most things are done this way:
> do several specific drivers/frameworks, find the common stuff, and extract
> it into a core driver/framework. So I don't want to blame anyone - that's
> just the way how we do things, right?
If it was easy, somebody would've done it.
>>>> For a quick solution, can we just set the LCD_EN at boot time (with the
>>>> msleep), and not touch it after that?
>>>
>>> That would be sensible for now, so this series can be merged.
>>> As a more appropriate (and long term) solution,
>>> I plan on moving the panel reset pin handling to the spi backlight
>>> driver itself.
>
>> Well, if you must. But I suggest moving the whole panel handling into a
>> (omap specific) panel driver, as it's done for other panels. That way
>> you'll have a proper panel driver for it, for omap, and when CDF comes,
>> you'll get a platform independent panel driver for it.
>
> You can't just move generic architecture/platform independent stuff
> into OMAP specific framework... Just think about this... It's insane.
As I said, reliable vs unreliable. That's not insane.
But again, if you can handle this particular panel reliably with the
two-driver approach, I'm fine with it.
> In addition, AFAIR, the reset pin is the property of the toppoly panel
> hardware, so that is why I think, we should let the toppoly driver
> (currently spi backlight, later hopefully CDF) handle it correctly
> along with the spi sequences.
Yes, that sounds ok.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Igor Grinberg @ 2013-02-14 8:37 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511C8D8F.9060805@ti.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/14/13 09:09, Tomi Valkeinen wrote:
> On 2013-02-14 08:56, Igor Grinberg wrote:
>> On 02/13/13 17:59, Tomi Valkeinen wrote:
>
>>> Okay, so I just realized there's an spi backlight driver used here, and
>>> that backlight driver is actually handling the SPI transactions with the
>>> panel, instead of the panel driver. So this looks quite messed up.
>>
>> Yep, it always was.
>> The whole DSS specific panel handling inside the
>> drivers/video/omap2/displays is a mess.
>
> Well, that's not mess itself, it's just omap specific panel framework.
> But dividing single device handling into two separate places is a mess.
Yes, you are right it is not the mess, but it prevents the panel to
be used on other systems and that is BAD.
At the very least, drivers/video/backlight is a generic place that can be
used not just on OMAP.
And since the toppoly was and is used on other systems, why the hell
should anyone duplicate the driver just to please the OMAP specific
panel framework? The real problem is that this framework should not be
OMAP specific...
Of course I'm aware of the fact that currently there is no generic
panel framework, but forging something OMAP specific which is obviously
used on most of the other architectures/platforms (and I mean
panel<->controller relations), is not a good way to go.
Although, I'm also aware of the fact that most things are done this way:
do several specific drivers/frameworks, find the common stuff, and extract
it into a core driver/framework. So I don't want to blame anyone - that's
just the way how we do things, right?
>
>> Those panels can be (and are) used not only with OMAP based boards.
>
> True, but as there's no generic panel framework, that's the best we can
> do. But see CDF (common display framework) discussions if you're
> interested in what's hopefully coming soon.
Yep, I've seen the CDF discussion and I think this is a good way to go.
>
>>> For a quick solution, can we just set the LCD_EN at boot time (with the
>>> msleep), and not touch it after that?
>>
>> That would be sensible for now, so this series can be merged.
>> As a more appropriate (and long term) solution,
>> I plan on moving the panel reset pin handling to the spi backlight
>> driver itself.
>
> Well, if you must. But I suggest moving the whole panel handling into a
> (omap specific) panel driver, as it's done for other panels. That way
> you'll have a proper panel driver for it, for omap, and when CDF comes,
> you'll get a platform independent panel driver for it.
You can't just move generic architecture/platform independent stuff
into OMAP specific framework... Just think about this... It's insane.
>
> Of course, if you have multiple platforms already using that backlight
> driver, the omap specific approach may not be enticing. So perhaps it's
> easier to just do the quick fix and wait for CDF.
That is exactly what I am talking about.
In addition, AFAIR, the reset pin is the property of the toppoly panel
hardware, so that is why I think, we should let the toppoly driver
(currently spi backlight, later hopefully CDF) handle it correctly
along with the spi sequences.
- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRHKJGAAoJEBDE8YO64EfacksP/j79jaDbnXpFcwT9KlInp8OE
e+XNi5Vt8zbqhj4gHtxZlN/eIQsVRfuivm9CTp5aJSZHBDAJlPNKobmwjFrDLO9V
RtYTwLAcuWyOdnutIQ52xNwXSntQknd8yxm1qJZMEBjEP+mcQxISWXXMsdxlQEiT
emNtU42W16ZOR34kHUoVfYLkV0v02/JVygt3oaU71+mrNBOt+5L6cHcXaQZPKSes
LUOcyz0qJfzKbnmmZnP/+clTIids83u8rVCNZ1/JoIIlR4rvtNcxRM8Apa8KFJx/
PVT38ds62F0L0qbxL3UmI1uJS2KuEHuJyjYo0uDeQqeeSyz7Q3ZG4TwAJYkWZdWQ
TdFbVrsXbK408FT33VIP4rOzDjqO93IK6f5ld0tZoIvL59NLwgXIejJn6jTNNcU4
p25mUXGSDnaZrNU5cC7d/MzSMt60XQx3UiHjEXD3eJAT33yb+DdBaQwloMCXJQOx
vnseFqhuAzgFHd9LEl47LBg7eXudjaSvWYfJOV0SoB9s7QM8m/YUhnmqmtvdCqZL
fKMJcAjCgm0BG2P6ss79sl6P4XDoBF1LOwSwz4dRmocA3TP7vBNkuRoK08vQe6gv
Qi7hJ05ioa8THt77FxMHtf+ZrO34/L6gHxZqrOD++OgPPdL6qtegemyp4IaKKbUg
q3Mpgsr4ODyStdjEXxTC
=lIHm
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 00/33] OMAPDSS: platform_enable/disable callback removal from panel drivers
From: Archit Taneja @ 2013-02-14 7:29 UTC (permalink / raw)
To: Tony Lindgren; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <20130213164647.GF7144@atomide.com>
Hi,
On Wednesday 13 February 2013 10:16 PM, Tony Lindgren wrote:
> * Archit Taneja <archit@ti.com> [130213 06:26]:
>> init functions in omap board files request panel specific gpios, and provide
>> functions which omapdss panel drivers call to enable or disable them.
>>
>> Instead of the board files requesting these gpios, they should just pass the
>> platform specific data(like the gpio numbers), the panel should retrieve the
>> platform data and request the gpios. Doing this prevents the need of the panel
>> driver calling platform functions in board files.
>>
>> Panel drivers have their own platform data struct, and the board files populate
>> these structs and pass the pointer to the 'data' field of omap_dss_device. This
>> work will make it easier for the panel drivers be more adaptable to the
>> DT model.
>>
>> There is also removal of passing panel reset_gpio numbers through
>> omap_dss_device struct directly, reset gpios are passed through platform data
>> only.
>
> To avoid merge conflicts and dependencies between drivers and core
> Soc code, please break thes kind of patches into following parts:
>
> 1. Any platform_data header changes needed so both I and Tomi
> can pull it in as needed.
>
> 2. Changes to DSS drivers. Please keep stubs around for the
> board specific callback functions so omap2plus_defconfig
> won't break with just #1 merged into arm soc tree.
The build won't break, and the kernel will boot up properly, but the
panels won't work till the time #3 is also merged,
>
> 3. All the arch/arm/*omap* changes based on #1 above to
> drop obsolete callback functions and add new pdata if still
> needed. This needs to build and boot on #1 so I can merge
> this in via arm soc tree.
>
> 4. Any .dts changes needed.
We don't have any .dts changes for DSS as of now.
I'll split the patches accordingly.
Thanks,
Archit
^ permalink raw reply
* Re: [PATCH 16/33] OMAPDSS: acx565akm panel: handle gpios in panel driver
From: Archit Taneja @ 2013-02-14 7:20 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Aaro Koskinen, linux-omap, linux-fbdev
In-Reply-To: <511C8B16.5060605@ti.com>
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
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Tomi Valkeinen @ 2013-02-14 7:09 UTC (permalink / raw)
To: Igor Grinberg; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511C8A83.5070407@compulab.co.il>
[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]
On 2013-02-14 08:56, Igor Grinberg wrote:
> On 02/13/13 17:59, Tomi Valkeinen wrote:
>> Okay, so I just realized there's an spi backlight driver used here, and
>> that backlight driver is actually handling the SPI transactions with the
>> panel, instead of the panel driver. So this looks quite messed up.
>
> Yep, it always was.
> The whole DSS specific panel handling inside the
> drivers/video/omap2/displays is a mess.
Well, that's not mess itself, it's just omap specific panel framework.
But dividing single device handling into two separate places is a mess.
> Those panels can be (and are) used not only with OMAP based boards.
True, but as there's no generic panel framework, that's the best we can
do. But see CDF (common display framework) discussions if you're
interested in what's hopefully coming soon.
>> For a quick solution, can we just set the LCD_EN at boot time (with the
>> msleep), and not touch it after that?
>
> That would be sensible for now, so this series can be merged.
> As a more appropriate (and long term) solution,
> I plan on moving the panel reset pin handling to the spi backlight
> driver itself.
Well, if you must. But I suggest moving the whole panel handling into a
(omap specific) panel driver, as it's done for other panels. That way
you'll have a proper panel driver for it, for omap, and when CDF comes,
you'll get a platform independent panel driver for it.
Of course, if you have multiple platforms already using that backlight
driver, the omap specific approach may not be enticing. So perhaps it's
easier to just do the quick fix and wait for CDF.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 16/33] OMAPDSS: acx565akm panel: handle gpios in panel driver
From: Tomi Valkeinen @ 2013-02-14 6:58 UTC (permalink / raw)
To: Archit Taneja; +Cc: Aaro Koskinen, linux-omap, linux-fbdev
In-Reply-To: <511C896C.10007@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling
From: Igor Grinberg @ 2013-02-14 6:56 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <511BB87E.60003@ti.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/13/13 17:59, Tomi Valkeinen wrote:
> On 2013-02-13 17:28, Tomi Valkeinen wrote:
>> On 2013-02-13 17:16, Igor Grinberg wrote:
>>> Hi Archit,
>>>
>>> On 02/13/13 16:21, Archit Taneja wrote:
>>>> The cm-t35 board file currently requests gpios required to configure the tdo35s
>>>> panel, and provides platform_enable/disable callbacks to configure them.
>>>>
>>>> These tasks have been moved to the generic dpi panel driver itself and shouldn't
>>>> be done in the board files.
>>>>
>>>> Remove the gpio requests and the platform callbacks from the board file.
>>>> Add the gpio information to generic dpi panel's platform data so that it's
>>>> passed to the panel driver.
>>>>
>>>> Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was
>>>> enabled after a 50 millisecond delay. This code has been removed and is not
>>>> taken care of in the generic panel driver. The impact of this needs to be
>>>> tested. The panel's gpios are also not exported any more. Hence, they can't be
>>>> accessed via sysfs interface.
>>>
>>> Indeed, there is an impact - the LCD no longer works.
>>> The reason for the LCD_EN gpio being pushed high after the 50ms delay,
>>> is to get the LCD out of reset, so the SPI transaction will succeed
>>> and initialize the LCD.
>>> Now, when you remove the gpio handling for the LCD_EN pin,
>>> the LCD no longer works.
>>
>> So between what is the sleep done? It's not clear from the code. LCD_EN
>> needs to be 0 for 50ms, or...?
>>
>> If the panel requires specific reset handling, does it work right even
>> currently when the panel is turned off and later turned on? The msleep
>> is only used at boot time.
>
> Okay, so I just realized there's an spi backlight driver used here, and
> that backlight driver is actually handling the SPI transactions with the
> panel, instead of the panel driver. So this looks quite messed up.
Yep, it always was.
The whole DSS specific panel handling inside the
drivers/video/omap2/displays is a mess.
Those panels can be (and are) used not only with OMAP based boards.
>
> For a quick solution, can we just set the LCD_EN at boot time (with the
> msleep), and not touch it after that?
That would be sensible for now, so this series can be merged.
As a more appropriate (and long term) solution,
I plan on moving the panel reset pin handling to the spi backlight
driver itself.
- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRHIqCAAoJEBDE8YO64Efamw0P/R2wt0tpCI1ecrnutVGMX4bF
Pyjk2B65uDWqoqZ/cpJUqnvmupXl5UdrA7eqKjTBh1A+g81UVFcNDMuJsbPIIiYI
1pimZieAq0T6Vag00PKImKlkhJYfC7JVBbESij/NONlzYtPkbZ91Y+Ik4DZXnyZf
1TS4GbHQ25tjl73PkwlzLUcJIDIogsimSrkM+aWXPE8LmvrBEQs0LhAObPsAFtgL
1An3hvA2Tkhh9QgerWQd9YiqX994tv1PGRLBEXTbjh1yihzKSNleuvw3NdM+wf9i
9Y5l9IV6L2dtYBMLCpzkiGQBDdOzoq+fObRnSgK6Kr1mfXot+MAlLrk9gCeWcq1b
c2oU/imKWB4sZys21pTnjIxAIzzRDoGW40qXuibTW4DoAYaVHuEBPtphjMVCBCcQ
sJaIVXpsChQ3vvtHOgllnInMjCnlXJ3Piqr4y5glTPxu9mZHdPr6VDpWdqRmvyr9
V7fRQztwXB3Td+SZVDD1yBqoXKlKCX4QPlLAqH3FI9s1WhDHcJePcoDJY0/QyXB1
IeQRlEwBBEZAYy/kr9/pwbZzXeh5V5dK6wAq8aT+thS22zl3nJbKjW//vN06+ib+
WAnHRSZ8iCbUX2cRVF1k+DCQOTi8QCbI6WTcLsgenLeSrbEuzilfgsrsvd6LHfjD
oTODiiD9QInP2sBfknUp
=tWsB
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 16/33] OMAPDSS: acx565akm panel: handle gpios in panel driver
From: Archit Taneja @ 2013-02-14 6:52 UTC (permalink / raw)
To: Aaro Koskinen; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <20130213172913.GD21750@blackmetal.musicnaut.iki.fi>
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
^ permalink raw reply
* Re: [PATCH 27/33] OMAPDSS: n8x0 panel: handle gpio data in panel driver
From: Archit Taneja @ 2013-02-14 6:46 UTC (permalink / raw)
To: Aaro Koskinen; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <20130213173527.GE21750@blackmetal.musicnaut.iki.fi>
Hi,
On Wednesday 13 February 2013 11:05 PM, Aaro Koskinen wrote:
> Hi,
>
> On Wed, Feb 13, 2013 at 07:52:19PM +0530, Archit Taneja wrote:
>> @@ -444,6 +445,20 @@ static int n8x0_panel_probe(struct omap_dss_device *dssdev)
>> dssdev->ctrl.rfbi_timings = n8x0_panel_timings;
>> dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE;
>>
>> + if (gpio_is_valid(bdata->panel_reset)) {
>> + r = devm_gpio_request_one(&dssdev->dev, bdata->panel_reset,
>> + GPIOF_OUT_INIT_LOW, "PANEL RESET");
>> + if (r)
>> + return r;
>> + }
>> +
>> + if (gpio_is_valid(bdata->ctrl_pwrdown)) {
>> + r = devm_gpio_request_one(&dssdev->dev, bdata->ctrl_pwrdown,
>> + GPIOF_OUT_INIT_LOW, "PANEL PWRDOWN");
>> + if (r)
>> + return r;
>> + }
>> +
>
> In the error case, the other GPIO is not freed. Also maybe you should
> free them on module removal, because now the module owns the GPIOs.
Wouldn't the usage of devm_* functions take care of this? If the device
isn't registered successfully, then all allocations/requests done using
devm_* functions will be free'd automatically.
Archit
^ permalink raw reply
* Re: [PATCH v2 0/1] OMAP4: DSS: Add panel for Blaze Tablet boards
From: Ruslan Bilovol @ 2013-02-14 0:07 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: andi, FlorianSchandinat, linux-fbdev, linux-kernel, linux-omap
In-Reply-To: <511B4C60.50404@ti.com>
Hi Tomi,
On Wed, Feb 13, 2013 at 10:18 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On 2013-02-08 17:43, Ruslan Bilovol wrote:
>> Hi,
>>
>> This patch adds support for TC358765 DSI-to-LVDS transmitter
>> from Toshiba, that is used in OMAP4 Blaze Tablet development
>> platform. It was originally developed a long time ago and
>> was used internally survived few kernel migrations.
>> Different people worked in this driver during last two
>> years changing its sources to what each new version of
>> kernel expects.
>>
>> Current version is ported from internal kernel v3.4 to 3.8.0-rc6
>> Tested basic functioning under busybox.
>
> Thanks for updating the driver to a recent kernel.
>
> I didn't review the patch line by line, but with a brief look it looks
> fine. However, I'm not quite sure what to do with this patch.
>
> The problem is that the driver is a combined driver for the TC358765
> chip and the panel on Blaze Tablet, and we are very much trying to fix
> that design issue so that the drivers for the chip and panel could be
> separate, as they should.
>
> So I fear that if I now accept the patch, it'll increase my burden of
> managing the display drivers during this design rework. Furthermore, to
> my knowledge the Blaze Tablet is quite a rare board, and it's not even
> supported in the mainline kernel, so this driver wouldn't be usable by
> itself.
This patch is exactly a part of mainlining of the BlazeTablet board support :)
The goal is to have as much as possible support of this board in the
'vanilla' kernel.
The BlazeTablet mainlining is already ongoing (
https://patchwork.kernel.org/patch/2118281/ )
and I hope we will have some support of this board in near future.
The BlazeTablet's LCD panel support is very important thing for us to
have it in mainline because
without it the BlazeTablet becomes a 'black brick' and it is painful
for us to port this
driver against each new version of kernel.
>
> So if I'm right about the blaze tablet status, I'm inclined to say that
> I think this should be still kept out of tree.
Considering my information above, (and your information that design
rework will be started later), can we pick this driver for now in its
current state?
Regards,
Ruslan
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH RESEND v2 2/2] drivers/video: fsl-diu-fb: fix bugs in interrupt handling
From: Anatolij Gustschin @ 2013-02-13 20:13 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1360308959-3096-2-git-send-email-agust@denx.de>
On Wed, 13 Feb 2013 11:55:24 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 13 Feb 2013 10:21:52 +0100
> Anatolij Gustschin <agust@denx.de> wrote:
>
> > On Tue, 12 Feb 2013 17:01:55 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > > On Tue, 12 Feb 2013 16:54:58 -0800
> > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > > Thanks, I queued both these with a plan to merge into 3.9-rc1.
> > >
> > > No I didn't. The patches have already found their way into linux-next
> > > via some other tree.
> >
> > Yes, via MPC5XXX tree. Since I didn't receive response from fbdev
> > maintainer I applied both patches in my powerpc MPC5XXX next branch
> > so that these can be exposed in linux-next at least. MPC5121 uses the
> > fsl-diu-fb driver, so these patches could go via MPC5XXX tree, but I
> > can't push them via my tree without an ACK from fbdev maintainer.
>
> The patches look OK to me - I suggest you go ahead and merge them up
> for 3.9-rc1.
Okay, will do.
> > > Without any -stable tags, either. You don't think we should fix the
> > > "24 and 16 bpp" thing in 3.8.x and earlier?
> >
> > I didn't add any -stable tags since my hope was that the patches
> > will go into v3.8 via fbdev tree. It would be good to fix the bpp
> > issue in 3.8.x. But the issue is not critical for earlier maintained
> > stable versions I think.
>
> Please remember to add the cc:stable to the changelogs if you want
> these in 3.8.x and earlier.
Okay, thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH RESEND v2 2/2] drivers/video: fsl-diu-fb: fix bugs in interrupt handling
From: Andrew Morton @ 2013-02-13 19:55 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1360308959-3096-2-git-send-email-agust@denx.de>
On Wed, 13 Feb 2013 10:21:52 +0100
Anatolij Gustschin <agust@denx.de> wrote:
> On Tue, 12 Feb 2013 17:01:55 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > On Tue, 12 Feb 2013 16:54:58 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > > Thanks, I queued both these with a plan to merge into 3.9-rc1.
> >
> > No I didn't. The patches have already found their way into linux-next
> > via some other tree.
>
> Yes, via MPC5XXX tree. Since I didn't receive response from fbdev
> maintainer I applied both patches in my powerpc MPC5XXX next branch
> so that these can be exposed in linux-next at least. MPC5121 uses the
> fsl-diu-fb driver, so these patches could go via MPC5XXX tree, but I
> can't push them via my tree without an ACK from fbdev maintainer.
The patches look OK to me - I suggest you go ahead and merge them up
for 3.9-rc1.
> > Without any -stable tags, either. You don't think we should fix the
> > "24 and 16 bpp" thing in 3.8.x and earlier?
>
> I didn't add any -stable tags since my hope was that the patches
> will go into v3.8 via fbdev tree. It would be good to fix the bpp
> issue in 3.8.x. But the issue is not critical for earlier maintained
> stable versions I think.
Please remember to add the cc:stable to the changelogs if you want
these in 3.8.x and earlier.
^ permalink raw reply
* Re: [PATCH v2 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards
From: Ruslan Bilovol @ 2013-02-13 19:23 UTC (permalink / raw)
To: Andi Shyti
Cc: tomi.valkeinen, FlorianSchandinat, linux-fbdev, linux-kernel,
linux-omap
In-Reply-To: <20130210235125.GA8098@jack.whiskey>
Hi Andi,
Thanks for your review,
On Mon, Feb 11, 2013 at 1:51 AM, Andi Shyti <andi@etezian.org> wrote:
> Hi Ruslan,
>
>> TC358765 is DSI-to-LVDS transmitter from Toshiba, used in
>> OMAP44XX Blaze Tablet and Blaze Tablet2 boards.
>
> I think it's fine, just some nitpicks and checkpatch warnings
>
>> +struct {
>> + struct device *dev;
>> + struct dentry *dir;
>> +} tc358765_debug;
>
> Should this be static?
Agree.
>
>> +struct tc358765_reg {
>> + const char *name;
>> + u16 reg;
>> + u8 perm:2;
>> +} tc358765_regs[] = {
>
> Should this be static as well?
Agree.
>
>> + { "D1S_ZERO", D1S_ZERO, A_RW },
>> + { "D2S_ZERO", D2S_ZERO, A_RW },
>> + { "D3S_ZERO", D3S_ZERO, A_RW },
>> + { "PPI_CLRFLG", PPI_CLRFLG, A_RW },
>> + { "PPI_CLRSIPO", PPI_CLRSIPO, A_RW },
>> + { "PPI_HSTimeout", PPI_HSTimeout, A_RW },
>
> WARNING: Avoid CamelCase: <PPI_HSTimeout>
> #136: FILE: video/omap2/displays/panel-tc358765.c:136:
> + { "PPI_HSTimeout", PPI_HSTimeout, A_RW },
>
>> + { "PPI_HSTimeoutEnable", PPI_HSTimeoutEnable, A_RW },
>
> WARNING: Avoid CamelCase: <PPI_HSTimeoutEnable>
> #137: FILE: video/omap2/displays/panel-tc358765.c:137:
> + { "PPI_HSTimeoutEnable", PPI_HSTimeoutEnable, A_RW },
Hmm... I though these registers had such naming in the documentation,
however, after looking into it I found the names are in upper case there
so this will be fixed.
>
>
>> +static int tc358765_read_block(u16 reg, u8 *data, int len)
>> +{
>> + unsigned char wb[2];
>> + struct i2c_msg msg[2];
>> + int r;
>> + mutex_lock(&tc358765_i2c->xfer_lock);
>> + wb[0] = (reg & 0xff00) >> 8;
>> + wb[1] = reg & 0xff;
>> + msg[0].addr = tc358765_i2c->client->addr;
>> + msg[0].len = 2;
>> + msg[0].flags = 0;
>> + msg[0].buf = wb;
>> + msg[1].addr = tc358765_i2c->client->addr;
>> + msg[1].flags = I2C_M_RD;
>> + msg[1].len = len;
>> + msg[1].buf = data;
>> +
>> + r = i2c_transfer(tc358765_i2c->client->adapter, msg, ARRAY_SIZE(msg));
>> + mutex_unlock(&tc358765_i2c->xfer_lock);
>> +
>> + if (r = ARRAY_SIZE(msg))
>> + return len;
>> +
>> + return r;
>
> If you like, here you could do
>
> return (r = ARRAY_SIZE(msg)) ? len : r;
>
> Usually I like more this notation because keeps the code more
> compact and immediate to read, but you don't have to
Yes, makes sense for "beautification" :)
Will change
>
>> + mutex_lock(&tc358765_i2c->xfer_lock);
>> + ret = i2c_transfer(tc358765_i2c->client->adapter, &msg, 1);
>> + mutex_unlock(&tc358765_i2c->xfer_lock);
>> +
>> + if (ret != 1)
>> + return ret;
>> + return 0;
>
> Also here you could do
>
> return (ret != 1) ? ret : 0;
>
> But this is more taste :)
>
>> +static ssize_t tc358765_seq_write(struct file *filp, const char __user *ubuf,
>> + size_t size, loff_t *ppos)
>> +{
>> + struct device *dev = tc358765_debug.dev;
>> + unsigned i, reg_count;
>> + u32 value = 0;
>> + int error = 0;
>> + /* kids, don't use register names that long */
>
> I won't, promised, but please, drop this comment :)
>
>> + char name[30];
>> + char buf[50];
>> +
>> + if (size >= sizeof(buf))
>> + size = sizeof(buf);
>
> what's the point of this?
This is a way to limit copied from userspace data by available buffer size,
widely used in current kernel sources. Are you implying there is some
better (more graceful) way?
>
>> +static void tc358765_uninitialize_debugfs(void)
>> +{
>> + if (tc358765_debug.dir)
>> + debugfs_remove_recursive(tc358765_debug.dir);
>
> WARNING: debugfs_remove_recursive(NULL) is safe this check is probably not required
> #435: FILE: video/omap2/displays/panel-tc358765.c:435:
> + if (tc358765_debug.dir)
> + debugfs_remove_recursive(tc358765_debug.dir);
Will fix it..
>
>
>> +static struct tc358765_board_data *get_board_data(struct omap_dss_device
>> + *dssdev)
>> +{
>> + return (struct tc358765_board_data *)dssdev->data;
>
> You shouldn't need for this cast, does it complain?
yes, we don't need this :)
>
>> +}
>> +
>> +static void tc358765_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + *timings = dssdev->panel.timings;
>> +}
>> +
>> +static void tc358765_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + dev_info(&dssdev->dev, "set_timings() not implemented\n");
>
> ... then drop the function :)
I need to check if this will have any side effects in places where
this is used. I mean next:
| if (blabla->set_timings)
| blabla->set_timings();
| else
| do_another_thing();
But it seems this may be safely removed
>
>> + if ((pins[2] & 1) || (pins[3] & 1)) {
>> + lanes |= (1 << 1);
>> + ret |= tc358765_write_register(dssdev, PPI_D0S_CLRSIPOCOUNT,
>> + board_data->clrsipo);
>> + }
>> + if ((pins[4] & 1) || (pins[5] & 1)) {
>> + lanes |= (1 << 2);
>> + ret |= tc358765_write_register(dssdev, PPI_D1S_CLRSIPOCOUNT,
>> + board_data->clrsipo);
>> + }
>> + if ((pins[6] & 1) || (pins[7] & 1)) {
>> + lanes |= (1 << 3);
>> + ret |= tc358765_write_register(dssdev, PPI_D2S_CLRSIPOCOUNT,
>> + board_data->clrsipo);
>> + }
>> + if ((pins[8] & 1) || (pins[9] & 1)) {
>> + lanes |= (1 << 4);
>> + ret |= tc358765_write_register(dssdev, PPI_D3S_CLRSIPOCOUNT,
>> + board_data->clrsipo);
>> + }
>
> Can't this be done in one single multiwrighting command since
> this registers are consecutive?
>
> You build once the array to write and you send it at once.
In this particular case I disagree. Yes, it will be a little bit
faster, however:
1) we write this for panel initialization only (so no impact in other cases)
2) multiwriting of array will make code reading more difficult
So I would like to leave it as-is
Same is for next your similar comment.
>
> Moreover it would be nice to have a name for all those nombers
Okay, will replace magic numbers by defined values
>
>> + ret |= tc358765_write_register(dssdev, HTIM1,
>> + (tc358765_timings.hbp << 16) | tc358765_timings.hsw);
>> + ret |= tc358765_write_register(dssdev, HTIM2,
>> + ((tc358765_timings.hfp << 16) | tc358765_timings.x_res));
>> + ret |= tc358765_write_register(dssdev, VTIM1,
>> + ((tc358765_timings.vbp << 16) | tc358765_timings.vsw));
>> + ret |= tc358765_write_register(dssdev, VTIM2,
>> + ((tc358765_timings.vfp << 16) | tc358765_timings.y_res));
>
> also this and all the other cases I haven't checked
>
>> +static int tc358765_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct tc358765_data *d2d = dev_get_drvdata(&dssdev->dev);
>> + int r = 0;
>
> this initialisation is useless
Agree
>
>> + if (r) {
>> + dev_dbg(&dssdev->dev, "enable failed\n");
>
> Here you could choose a different print level, I would have used
> dev_err instead.
Agree
>
>> +static int tc358765_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + tc358765_i2c = devm_kzalloc(&client->dev, sizeof(*tc358765_i2c), GFP_KERNEL);
>
> WARNING: line over 80 characters
> #927: FILE: video/omap2/displays/panel-tc358765.c:927:
> + tc358765_i2c = devm_kzalloc(&client->dev, sizeof(*tc358765_i2c), GFP_KERNEL);
Agree :)
>
>
>> + /* store i2c_client pointer on private data structure */
>> + tc358765_i2c->client = client;
>> +
>> + /* store private data structure pointer on i2c_client structure */
>> + i2c_set_clientdata(client, tc358765_i2c);
>> +
>> + /* init mutex */
>> + mutex_init(&tc358765_i2c->xfer_lock);
>> + dev_err(&client->dev, "D2L i2c initialized\n");
>
> while here dev_dbg (or dev_info) are better.
Agree
>
>> +static int __init tc358765_init(void)
>> +{
>> + int r;
>> + tc358765_i2c = NULL;
>> + r = i2c_add_driver(&tc358765_i2c_driver);
>> + if (r < 0) {
>> + printk(KERN_WARNING "d2l i2c driver registration failed\n");
>
> WARNING: Prefer netdev_warn(netdev, ... then dev_warn(dev, ... then pr_warn(... to printk(KERN_WARNING ...
> #981: FILE: video/omap2/displays/panel-tc358765.c:981:
> + printk(KERN_WARNING "d2l i2c driver registration failed\n");
Agree
--
Best regards,
Ruslan
>
>
> Andi
^ permalink raw reply
* Re: [PATCH 27/33] OMAPDSS: n8x0 panel: handle gpio data in panel driver
From: Aaro Koskinen @ 2013-02-13 17:35 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <1360765345-19312-28-git-send-email-archit@ti.com>
Hi,
On Wed, Feb 13, 2013 at 07:52:19PM +0530, Archit Taneja wrote:
> @@ -444,6 +445,20 @@ static int n8x0_panel_probe(struct omap_dss_device *dssdev)
> dssdev->ctrl.rfbi_timings = n8x0_panel_timings;
> dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE;
>
> + if (gpio_is_valid(bdata->panel_reset)) {
> + r = devm_gpio_request_one(&dssdev->dev, bdata->panel_reset,
> + GPIOF_OUT_INIT_LOW, "PANEL RESET");
> + if (r)
> + return r;
> + }
> +
> + if (gpio_is_valid(bdata->ctrl_pwrdown)) {
> + r = devm_gpio_request_one(&dssdev->dev, bdata->ctrl_pwrdown,
> + GPIOF_OUT_INIT_LOW, "PANEL PWRDOWN");
> + if (r)
> + return r;
> + }
> +
In the error case, the other GPIO is not freed. Also maybe you should
free them on module removal, because now the module owns the GPIOs.
A.
^ permalink raw reply
* Re: [PATCH 16/33] OMAPDSS: acx565akm panel: handle gpios in panel driver
From: Aaro Koskinen @ 2013-02-13 17:29 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <1360765345-19312-17-git-send-email-archit@ti.com>
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.
^ permalink raw reply
* Re: [PATCH 00/33] OMAPDSS: platform_enable/disable callback removal from panel drivers
From: Tony Lindgren @ 2013-02-13 16:46 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, linux-omap, linux-fbdev
In-Reply-To: <1360765345-19312-1-git-send-email-archit@ti.com>
* Archit Taneja <archit@ti.com> [130213 06:26]:
> init functions in omap board files request panel specific gpios, and provide
> functions which omapdss panel drivers call to enable or disable them.
>
> Instead of the board files requesting these gpios, they should just pass the
> platform specific data(like the gpio numbers), the panel should retrieve the
> platform data and request the gpios. Doing this prevents the need of the panel
> driver calling platform functions in board files.
>
> Panel drivers have their own platform data struct, and the board files populate
> these structs and pass the pointer to the 'data' field of omap_dss_device. This
> work will make it easier for the panel drivers be more adaptable to the
> DT model.
>
> There is also removal of passing panel reset_gpio numbers through
> omap_dss_device struct directly, reset gpios are passed through platform data
> only.
To avoid merge conflicts and dependencies between drivers and core
Soc code, please break thes kind of patches into following parts:
1. Any platform_data header changes needed so both I and Tomi
can pull it in as needed.
2. Changes to DSS drivers. Please keep stubs around for the
board specific callback functions so omap2plus_defconfig
won't break with just #1 merged into arm soc tree.
3. All the arch/arm/*omap* changes based on #1 above to
drop obsolete callback functions and add new pdata if still
needed. This needs to build and boot on #1 so I can merge
this in via arm soc tree.
4. Any .dts changes needed.
Regards,
Tony
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox