Re: [PATCH V6 8/8] drm/bridge: Add i2c based driver for ps8622/ps8625 bridge

2014-07-30 Thread Ajay kumar 
Hi Andreas,

On Tue, Jul 29, 2014 at 4:59 PM, Andreas Färber afaer...@suse.de wrote:
 Am 25.07.2014 21:22, schrieb Ajay Kumar:
 From: Vincent Palatin vpala...@chromium.org

 This patch adds drm_bridge driver for parade DisplayPort
 to LVDS bridge chip.

 Signed-off-by: Vincent Palatin vpala...@chromium.org
 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Signed-off-by: Sean Paul seanp...@chromium.org
 Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
 ---
  .../devicetree/bindings/vendor-prefixes.txt|1 +
  .../devicetree/bindings/video/bridge/ps8622.txt|   19 +
  drivers/gpu/drm/bridge/Kconfig |   10 +
  drivers/gpu/drm/bridge/Makefile|1 +
  drivers/gpu/drm/bridge/ps8622.c|  602 
 
  5 files changed, 633 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/video/bridge/ps8622.txt
  create mode 100644 drivers/gpu/drm/bridge/ps8622.c

 diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
 b/Documentation/devicetree/bindings/vendor-prefixes.txt
 index 46a311e..b4a99cc 100644
 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
 +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
 @@ -96,6 +96,7 @@ nxp NXP Semiconductors
  onnn ON Semiconductor Corp.
  opencoresOpenCores.org
  panasonicPanasonic Corporation
 +parade   Parade Technologies Inc.
  phytec   PHYTEC Messtechnik GmbH
  picochip Picochip Ltd
  plathome Plat'Home Co., Ltd.
 diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt 
 b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 new file mode 100644
 index 000..fdeafb2
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 @@ -0,0 +1,19 @@
 +ps8622-bridge bindings
 +
 +Required properties:
 + - compatible: parade,ps8622 or parade,ps8625
 + - reg: first i2c address of the bridge
 + - sleep-gpios: OF device-tree gpio specification
 + - reset-gpios: OF device-tree gpio specification
 +
 +Optional properties:
 + - lane-count: number of DP lanes to use
 +
 +Example:
 + ps8622-bridge@48 {

 Nit: Shouldn't this be lvds-bridge like in 7/8 or something else not
 derived from the specific model? Applies to the DT series as well.
Right. I will fix this while sending the next series.

 + compatible = parade,ps8622;
 + reg = 0x48;
 + sleep-gpios = gpc3 6 1 0 0;
 + reset-gpios = gpc3 1 1 0 0;
 + lane-count = 1
 + };
 [...]
 diff --git a/drivers/gpu/drm/bridge/ps8622.c 
 b/drivers/gpu/drm/bridge/ps8622.c
 new file mode 100644
 index 000..ec60fcf
 --- /dev/null
 +++ b/drivers/gpu/drm/bridge/ps8622.c
 @@ -0,0 +1,602 @@
 +/*
 + * Parade PS8622 eDP/LVDS bridge driver
 + *
 + * Copyright (C) 2014 Google, Inc.
 + *
 + * This software is licensed under the terms of the GNU General Public
 + * License version 2, as published by the Free Software Foundation, and
 + * may be copied, distributed, and modified under those terms.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 [...]
 +MODULE_AUTHOR(Vincent Palatin vpala...@chromium.org);
 +MODULE_DESCRIPTION(Parade ps8622 eDP-LVDS converter driver);
 +MODULE_LICENSE(GPL);

 GPL v2?
Ok. Will change it.

Regards,
Ajay
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V6 8/8] drm/bridge: Add i2c based driver for ps8622/ps8625 bridge

2014-07-30 Thread Thierry Reding 
On Sat, Jul 26, 2014 at 12:52:10AM +0530, Ajay Kumar wrote:
[...]
 diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
 b/Documentation/devicetree/bindings/vendor-prefixes.txt
 index 46a311e..b4a99cc 100644
 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
 +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
 @@ -96,6 +96,7 @@ nxp NXP Semiconductors
  onnn ON Semiconductor Corp.
  opencoresOpenCores.org
  panasonicPanasonic Corporation
 +parade   Parade Technologies Inc.
  phytec   PHYTEC Messtechnik GmbH
  picochip Picochip Ltd
  plathome Plat'Home Co., Ltd.

I think it's a good idea to turn this into a separate patch to avoid
conflicts.

 diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt 
 b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 new file mode 100644
 index 000..fdeafb2
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 @@ -0,0 +1,19 @@
 +ps8622-bridge bindings
 +
 +Required properties:
 + - compatible: parade,ps8622 or parade,ps8625
 + - reg: first i2c address of the bridge

I'm not sure what the deal is with devices that use more than one I2C
addresses. It would be good to hear from the device tree maintainers on
how to handle those. Technically each address is a separate device. This
is also mirrored in the Linux kernel's implementation of the I2C bus,
where it looks wrong to access more than a single address (since the
core only marks one of them used). So technically it's valid for
userspace to access any but the first page of this device.

 + - sleep-gpios: OF device-tree gpio specification
 + - reset-gpios: OF device-tree gpio specification

This should explain what these GPIOs are used for.

 +
 +Optional properties:
 + - lane-count: number of DP lanes to use
 +
 +Example:
 + ps8622-bridge@48 {
 + compatible = parade,ps8622;
 + reg = 0x48;
 + sleep-gpios = gpc3 6 1 0 0;
 + reset-gpios = gpc3 1 1 0 0;
 + lane-count = 1
 + };

Same here. It's usually best to make this a patch separate from the
driver. Not because of conflicts, but because it makes it easier for DT
reviewers to find the relevant pieces to look at.

 diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
 index 0b12d16..d73e474 100644
 --- a/drivers/gpu/drm/bridge/Kconfig
 +++ b/drivers/gpu/drm/bridge/Kconfig
 @@ -16,4 +16,14 @@ config DRM_PTN3460
   help
 ptn3460 eDP-LVDS bridge chip driver.
  
 +config DRM_PS8622
 + tristate Parade eDP/LVDS bridge
 + depends on DRM  DRM_BRIDGE

Aagin, these are unnecessary.

 diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
 index b4733e1..d1b5daa 100644
 --- a/drivers/gpu/drm/bridge/Makefile
 +++ b/drivers/gpu/drm/bridge/Makefile
 @@ -1,3 +1,4 @@
  ccflags-y := -Iinclude/drm
  
  obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
 +obj-$(CONFIG_DRM_PS8622) += ps8622.o

Please keep these sorted alphabetically.

 diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c
[...]
 +#include linux/module.h
 +#include linux/backlight.h
 +#include linux/delay.h
 +#include linux/err.h
 +#include linux/fb.h
 +#include linux/gpio.h
 +#include linux/i2c.h
 +#include linux/of.h
 +#include linux/of_device.h
 +#include linux/pm.h
 +#include linux/regulator/consumer.h

These too.

 +struct ps8622_bridge {
 + struct drm_connector connector;
 + struct i2c_client *client;
 + struct drm_bridge *bridge;

Should be embedded.

 + struct drm_panel *panel;
 + struct regulator *v12;
 + struct backlight_device *bl;
 + struct mutex enable_mutex;

I don't quite understand what this protects. Can you explain?

 +struct ps8622_device_data {
 + u8 max_lane_count;
 +};
 +
 +static const struct ps8622_device_data ps8622_data = {
 + .max_lane_count = 1,
 +};
 +
 +static const struct ps8622_device_data ps8625_data = {
 + .max_lane_count = 2,
 +};
 +
 +/* Brightness scale on the Parade chip */
 +#define PS8622_MAX_BRIGHTNESS 0xff
 +
 +/* Timings taken from the version 1.7 datasheet for the PS8622/PS8625 */
 +#define PS8622_POWER_RISE_T1_MIN_US 10
 +#define PS8622_POWER_RISE_T1_MAX_US 1
 +#define PS8622_RST_HIGH_T2_MIN_US 3000
 +#define PS8622_RST_HIGH_T2_MAX_US 3
 +#define PS8622_PWMO_END_T12_MS 200
 +#define PS8622_POWER_FALL_T16_MAX_US 1
 +#define PS8622_POWER_OFF_T17_MS 500
 +
 +#if ((PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US)  \
 + (PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US))
 +#error T2.min + T1.max must be less than T2.max + T1.min
 +#endif
 +
 +static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val)
 +{
 + int ret;
 + struct i2c_adapter *adap = client-adapter;
 + struct i2c_msg msg;
 + u8 data[] = {reg, val};
 +
 + msg.addr = client-addr + page;
 + msg.flags = 0;
 + msg.len = sizeof(data);
 +

Re: [PATCH V6 8/8] drm/bridge: Add i2c based driver for ps8622/ps8625 bridge

2014-07-29 Thread Andreas Färber 
Am 25.07.2014 21:22, schrieb Ajay Kumar:
 From: Vincent Palatin vpala...@chromium.org
 
 This patch adds drm_bridge driver for parade DisplayPort
 to LVDS bridge chip.
 
 Signed-off-by: Vincent Palatin vpala...@chromium.org
 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Signed-off-by: Sean Paul seanp...@chromium.org
 Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
 ---
  .../devicetree/bindings/vendor-prefixes.txt|1 +
  .../devicetree/bindings/video/bridge/ps8622.txt|   19 +
  drivers/gpu/drm/bridge/Kconfig |   10 +
  drivers/gpu/drm/bridge/Makefile|1 +
  drivers/gpu/drm/bridge/ps8622.c|  602 
 
  5 files changed, 633 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/video/bridge/ps8622.txt
  create mode 100644 drivers/gpu/drm/bridge/ps8622.c
 
 diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
 b/Documentation/devicetree/bindings/vendor-prefixes.txt
 index 46a311e..b4a99cc 100644
 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
 +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
 @@ -96,6 +96,7 @@ nxp NXP Semiconductors
  onnn ON Semiconductor Corp.
  opencoresOpenCores.org
  panasonicPanasonic Corporation
 +parade   Parade Technologies Inc.
  phytec   PHYTEC Messtechnik GmbH
  picochip Picochip Ltd
  plathome Plat'Home Co., Ltd.
 diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt 
 b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 new file mode 100644
 index 000..fdeafb2
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt
 @@ -0,0 +1,19 @@
 +ps8622-bridge bindings
 +
 +Required properties:
 + - compatible: parade,ps8622 or parade,ps8625
 + - reg: first i2c address of the bridge
 + - sleep-gpios: OF device-tree gpio specification
 + - reset-gpios: OF device-tree gpio specification
 +
 +Optional properties:
 + - lane-count: number of DP lanes to use
 +
 +Example:
 + ps8622-bridge@48 {

Nit: Shouldn't this be lvds-bridge like in 7/8 or something else not
derived from the specific model? Applies to the DT series as well.

 + compatible = parade,ps8622;
 + reg = 0x48;
 + sleep-gpios = gpc3 6 1 0 0;
 + reset-gpios = gpc3 1 1 0 0;
 + lane-count = 1
 + };
[...]
 diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c
 new file mode 100644
 index 000..ec60fcf
 --- /dev/null
 +++ b/drivers/gpu/drm/bridge/ps8622.c
 @@ -0,0 +1,602 @@
 +/*
 + * Parade PS8622 eDP/LVDS bridge driver
 + *
 + * Copyright (C) 2014 Google, Inc.
 + *
 + * This software is licensed under the terms of the GNU General Public
 + * License version 2, as published by the Free Software Foundation, and
 + * may be copied, distributed, and modified under those terms.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
[...]
 +MODULE_AUTHOR(Vincent Palatin vpala...@chromium.org);
 +MODULE_DESCRIPTION(Parade ps8622 eDP-LVDS converter driver);
 +MODULE_LICENSE(GPL);

GPL v2?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html