| Message ID | 1406316130-4744-9-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Am 25.07.2014 21:22, schrieb Ajay Kumar: > From: Vincent Palatin <vpalatin@chromium.org> > > This patch adds drm_bridge driver for parade DisplayPort > to LVDS bridge chip. > > Signed-off-by: Vincent Palatin <vpalatin@chromium.org> > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> > Signed-off-by: Ajay Kumar <ajaykumar.rs@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. > opencores OpenCores.org > panasonic Panasonic 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 0000000..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 0000000..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 <vpalatin@chromium.org>"); > +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); > +MODULE_LICENSE("GPL"); "GPL v2"? Regards, Andreas
Hi Andreas, On Tue, Jul 29, 2014 at 4:59 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 25.07.2014 21:22, schrieb Ajay Kumar: >> From: Vincent Palatin <vpalatin@chromium.org> >> >> This patch adds drm_bridge driver for parade DisplayPort >> to LVDS bridge chip. >> >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> >> Signed-off-by: Andrew Bresticker <abrestic@chromium.org> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@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. >> opencores OpenCores.org >> panasonic Panasonic 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 0000000..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 0000000..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 <vpalatin@chromium.org>"); >> +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); >> +MODULE_LICENSE("GPL"); > > "GPL v2"? Ok. Will change it. Regards, Ajay
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. > opencores OpenCores.org > panasonic Panasonic 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 0000000..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 10000 > +#define PS8622_RST_HIGH_T2_MIN_US 3000 > +#define PS8622_RST_HIGH_T2_MAX_US 30000 > +#define PS8622_PWMO_END_T12_MS 200 > +#define PS8622_POWER_FALL_T16_MAX_US 10000 > +#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); > + msg.buf = data; > + > + ret = i2c_transfer(adap, &msg, 1); > + if (ret != 1) > + pr_warn("PS8622 I2C write (0x%02x,0x%02x,0x%02x) failed: %d\n", > + client->addr + page, reg, val, ret); > + return !(ret == 1); > +} > + > +static int ps8622_send_config(struct ps8622_bridge *ps_bridge) > +{ > + struct i2c_client *cl = ps_bridge->client; > + int err = 0; > + > + /* wait 20ms after power ON */ > + usleep_range(20000, 30000); It's unusual to do this here. The function doesn't know when it's being called. It could theoretically be called at any point. Better to move the sleep to after where the device is powered on. > + > + err |= ps8622_set(cl, 0x02, 0xa1, 0x01); /* HPD low */ > + /* SW setting */ > + err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage > + * is lower to 96% */ I'm not at all a fan of this kind of error chaining because you loose all context about specific errors. Also if there's a systematic error you'll want to abort as early as possible. If the device doesn't respond to the first command sent, then it likely won't respond to the next either. No need to keep trying in that case. > + /* RCO SS setting */ > + err |= ps8622_set(cl, 0x04, 0xe3, 0x20); /* [5:4] = b01 0.5%, b10 1%, > + * b11 1.5% */ Also I find it very hard to follow these calls. Since you have enough information about what each of these registers does, can't you provide symbolic constants? It would probably also help to separate these into logical groups (or individual function calls if there are no meaningful groups). That way you'll get automatic documentation via function names and this function would look a lot less cluttered. > +static int ps8622_backlight_get(struct backlight_device *bl) > +{ > + return bl->props.brightness; > +} Backlight devices no longer need this kind of trivial implementation. It is what the backlight core will do by default if the driver doesn't implement .get_brightness(). > +static void ps8622_pre_enable(struct drm_bridge *bridge) > +{ > + struct ps8622_bridge *ps_bridge = bridge->driver_private; > + int ret; > + > + mutex_lock(&ps_bridge->enable_mutex); > + if (ps_bridge->enabled) > + goto out; > + > + gpiod_set_value(ps_bridge->gpio_rst_n, 0); > + > + if (ps_bridge->v12) { > + ret = regulator_enable(ps_bridge->v12); > + if (ret) > + DRM_ERROR("fails to enable ps_bridge->v12"); > + } > + > + drm_panel_prepare(ps_bridge->panel); > + > + gpiod_set_value(ps_bridge->gpio_slp_n, 1); This uses high-active logic for the sleep pin, so shouldn't it rather be named "gpio_slp"? > + /* > + * T1 is the range of time that it takes for the power to rise after we > + * enable the lcd fet. Is that the time it takes for the FET's power to rise? In that case it would be a property of the FET, not the bridge. > T2 is the range of time in which the data sheet > + * specifies we should deassert the reset pin. > + * > + * If it takes T1.max for the power to rise, we need to wait atleast > + * T2.min before deasserting the reset pin. If it takes T1.min for the > + * power to rise, we need to wait at most T2.max before deasserting the > + * reset pin. > + */ Note that to my knowledge none of the sleep functions give you any guarantees about the maximum amount of time they'll sleep, only the minimum. So there's no such thing as "wait at most" in Linux. > + usleep_range(PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US, > + PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US); > + > + gpiod_set_value(ps_bridge->gpio_rst_n, 1); This also uses high-active logic. > + ret = ps8622_send_config(ps_bridge); > + if (ret) > + DRM_ERROR("Failed to send config to bridge (%d)\n", ret); Isn't this a fatal error? What will be the consequences if this doesn't succeed? I guess it doesn't matter much either way since the API can't propagate errors at this point anyway... > +static void ps8622_enable(struct drm_bridge *bridge) > +{ > + struct ps8622_bridge *ps_bridge = bridge->driver_private; > + > + mutex_lock(&ps_bridge->enable_mutex); > + drm_panel_enable(ps_bridge->panel); > + mutex_unlock(&ps_bridge->enable_mutex); What's the enable_mutex protecting here? > +static void ps8622_disable(struct drm_bridge *bridge) > +{ > + struct ps8622_bridge *ps_bridge = bridge->driver_private; > + > + mutex_lock(&ps_bridge->enable_mutex); > + > + if (!ps_bridge->enabled) > + goto out; > + > + ps_bridge->enabled = false; > + > + drm_panel_disable(ps_bridge->panel); > + msleep(PS8622_PWMO_END_T12_MS); > + > + /* > + * This doesn't matter if the regulators are turned off, but something > + * else might keep them on. In that case, we want to assert the slp gpio > + * to lower power. > + */ > + gpiod_set_value(ps_bridge->gpio_slp_n, 0); > + > + if (ps_bridge->v12) > + regulator_disable(ps_bridge->v12); > + > + /* > + * Sleep for at least the amount of time that it takes the power rail to > + * fall to prevent asserting the rst gpio from doing anything. > + */ > + usleep_range(PS8622_POWER_FALL_T16_MAX_US, > + 2 * PS8622_POWER_FALL_T16_MAX_US); > + gpiod_set_value(ps_bridge->gpio_rst_n, 0); Does this even matter? Why would it be bad if the reset was asserted before power went away? Isn't the end result that the chip will be off and reset to an initial state the next time it's power up anyway? > +static enum drm_connector_status ps8622_detect(struct drm_connector *connector, > + bool force) Alignment here is slightly odd. We usually follow (in DRM and many other parts of the kernel) the convention of aligning arguments on subsequent lines with the first argument on the first line. > +int ps8622_post_encoder_init(struct drm_bridge *bridge) > +{ > + struct ps8622_bridge *ps_bridge = bridge->driver_private; > + int ret; > + > + /* bridge is attached to encoder. > + * safe to remove it from the bridge_lookup table. > + */ > + drm_bridge_remove_from_lookup(bridge); Like I said for the ptn3460 driver, this should not be here. > + ret = drm_bridge_init(bridge->drm_dev, bridge); > + if (ret) { > + DRM_ERROR("Failed to initialize bridge with drm\n"); > + return ret; > + } And this should probably happen somewhere else to avoid having to do it for every driver. > +static const struct of_device_id ps8622_devices[] = { > + { > + .compatible = "parade,ps8622", > + .data = &ps8622_data, > + }, { > + .compatible = "parade,ps8625", > + .data = &ps8625_data, > + }, { The above uses a mix of spaces and tabs for padding (tabs between .data and =). Single spaces will do. > + /* end node */ Nit: it's not a node, it's an entry or element. > + } > +}; > +MODULE_DEVICE_TABLE(of, ps8622_devices); > + > +static inline struct ps8622_device_data *get_device_data(struct device *dev) > +{ > + const struct of_device_id *match = > + of_match_device(ps8622_devices, dev); > + > + return (struct ps8622_device_data *)match->data; > +} Please drop this. First of all, match->data is void *, so there's no need for the explicit casting. Second, match->data is also const, so you're effectively casting that away. > + > +static int ps8622_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &(client->dev); No need for the parentheses here. > + struct device_node *panel_node, *backlight_node; > + struct drm_bridge *bridge; > + struct backlight_device *backlight_dev; > + struct ps8622_bridge *ps_bridge; Have you considered naming this simply "ps" to make it shorter? There's some weird formatting below just because this is a very long variable name. > + const struct ps8622_device_data *device_data; > + int ret; It's just as easy (and shorter) to do this here: const struct ps8622_device_data *data; const struct of_device_id *match; match = of_match_device(ps8622_devices, dev); data = match->data; > + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); > + if (!bridge) { > + DRM_ERROR("Failed to allocate drm bridge\n"); > + return -ENOMEM; > + } > + > + ps_bridge = devm_kzalloc(dev, sizeof(*ps_bridge), GFP_KERNEL); > + if (!ps_bridge) { > + DRM_ERROR("could not allocate ps bridge\n"); > + return -ENOMEM; > + } > + > + panel_node = of_parse_phandle(dev->of_node, "panel", 0); > + if (panel_node) { > + ps_bridge->panel = of_drm_find_panel(panel_node); > + of_node_put(panel_node); > + if (!ps_bridge->panel) > + return -EPROBE_DEFER; > + } > + > + backlight_dev = NULL; It's more idiomatic to initialize these when you declare them to avoid sprinkling them across the code like this. > + backlight_node = of_parse_phandle(dev->of_node, "backlight", 0); > + if (backlight_node) { > + backlight_dev = of_find_backlight_by_node(backlight_node); > + of_node_put(backlight_node); > + if (!backlight_dev) > + return -EPROBE_DEFER; It seems like in this case you aren't storing the backlight anywhere. Is there a reason for that? Is it because the same backlight is used by the panel driver and it does the controlling from there? If so it might be better to use a boolean property here rather than a phandle to another backlight device. Using a phandle usually signal that you do want to use the device in some way. > + } > + > + mutex_init(&ps_bridge->enable_mutex); > + > + bridge->dev = dev; > + bridge->driver_private = ps_bridge; > + bridge->funcs = &ps8622_bridge_funcs; > + > + ps_bridge->client = client; > + ps_bridge->bridge = bridge; > + > + device_data = get_device_data(dev); > + > + ps_bridge->v12 = devm_regulator_get(&client->dev, "vdd_bridge"); vdd_bridge is not a valid name for a regulator. This will translate to vdd_bridge-supply for a DT property and you shouldn't be using underscores for them. Also this property isn't documented in the binding. > + if (IS_ERR(ps_bridge->v12)) { > + DRM_INFO("no 1.2v regulator found for PS8622\n"); > + ps_bridge->v12 = NULL; > + } Does this ever happen? As far as I know the regulator core will give you a dummy regulator when booting using DT, so errors returned here will always be real errors that you likely won't want to ignore. > + > + ps_bridge->gpio_slp_n = devm_gpiod_get(&client->dev, "sleep"); > + if (IS_ERR(ps_bridge->gpio_slp_n)) { > + ret = PTR_ERR(ps_bridge->gpio_slp_n); > + DRM_ERROR("cannot get gpio_slp_n %d\n", ret); > + goto err_client; > + } else { > + ret = gpiod_direction_output(ps_bridge->gpio_slp_n, 1); > + if (ret) { > + DRM_ERROR("cannot configure gpio_slp_n\n"); > + goto err_client; > + } > + } > + > + ps_bridge->gpio_rst_n = devm_gpiod_get(&client->dev, "reset"); > + if (IS_ERR(ps_bridge->gpio_rst_n)) { > + ret = PTR_ERR(ps_bridge->gpio_rst_n); > + DRM_ERROR("cannot get gpio_rst_n %d\n", ret); > + goto err_client; > + } else { > + /* > + * Assert the reset pin high to avoid the bridge being > + * initialized prematurely > + */ > + ret = gpiod_direction_output(ps_bridge->gpio_rst_n, 1); > + if (ret) { > + DRM_ERROR("cannot configure gpio_slp_n\n"); > + goto err_client; > + } > + } Just as a side-note, there's an API under discussion that will allow gpio_desc's to be configured at the same time as they are requested. This driver should probably convert to that API. > + ps_bridge->max_lane_count = device_data->max_lane_count; > + > + if (of_property_read_u8(dev->of_node, "lane-count", > + &ps_bridge->lane_count)) > + ps_bridge->lane_count = ps_bridge->max_lane_count; > + else if (ps_bridge->lane_count > ps_bridge->max_lane_count) { > + DRM_INFO("lane-count property is too high for DP bridge\n"); > + ps_bridge->lane_count = ps_bridge->max_lane_count; > + } Perhaps this should clarify that you're falling back to max_lane_count. > + > + if (!backlight_dev) { > + ps_bridge->bl = backlight_device_register("ps8622-backlight", > + dev, ps_bridge, &ps8622_backlight_ops, > + NULL); > + if (IS_ERR(ps_bridge->bl)) { > + DRM_ERROR("failed to register backlight\n"); > + ret = PTR_ERR(ps_bridge->bl); > + ps_bridge->bl = NULL; No need to set this to NULL since you'll return with an error and ps_bridge will be freed. > + goto err_client; > + } > + ps_bridge->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS; > + ps_bridge->bl->props.brightness = PS8622_MAX_BRIGHTNESS; Perhaps you want to initialize bl->props.power to FB_BLANK_POWERDOWN at this point. The reason is that if you don't then the backlight will be on by default (FB_BLANK_UNBLANK == 0). Many backlight drivers do call backlight_update_status() after registering with the core to set the initial state. It looks like you're not doing that here, but it might be better to still set this to reflect what the initial state should be. I'm assuming you want it disabled to avoid visual glitches when you turn on the display. Of course if the bootloader already turned it on and initialized the display, then you'll get flickering... but I guess that's a problem to solve another day. > + } > + > + i2c_set_clientdata(client, ps_bridge); > + > + drm_bridge_add_for_lookup(bridge); > + > + return 0; > + > +err_client: > + DRM_ERROR("device probe failed : %d\n", ret); No need for this. The driver core typically tells you already. > +static int ps8622_remove(struct i2c_client *client) > +{ > + struct ps8622_bridge *ps_bridge = i2c_get_clientdata(client); > + > + if (ps_bridge->bl) > + backlight_device_unregister(ps_bridge->bl); > + > + return 0; > +} You'll also want to call drm_bridge_remove() here. > +static const struct i2c_device_id ps8622_i2c_table[] = { > + {"parade", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, ps8622_i2c_table); This table doesn't look right. Shouldn't that be: { "ps8622", 0 }, { "ps8625", 0 }, ? And perhaps use the .driver_data field to refer to the device data like for the of_device_id table? > +struct i2c_driver ps8622_driver = { > + .id_table = ps8622_i2c_table, > + .probe = ps8622_probe, > + .remove = ps8622_remove, > + .driver = { > + .name = "parade", That's an awfully generic name. I'd suggest "ps8622". > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ps8622_devices), As for ptn3460, this has a hard dependency on OF, so of_match_ptr() isn't useful. > + }, > +}; > +module_i2c_driver(ps8622_driver); > + > +MODULE_AUTHOR("Vincent Palatin <vpalatin@chromium.org>"); > +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); > +MODULE_LICENSE("GPL"); "GPL v2" Thierry
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. opencores OpenCores.org panasonic Panasonic 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 0000000..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 { + 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/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 + depends on OF + depends on I2C + select DRM_PANEL + select BACKLIGHT_LCD_SUPPORT + select BACKLIGHT_CLASS_DEVICE + help + parade eDP-LVDS bridge chip driver. endmenu 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 diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c new file mode 100644 index 0000000..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. + */ + +#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> + +#include <drm/drm_panel.h> + +#include "drmP.h" +#include "drm_crtc.h" +#include "drm_crtc_helper.h" + +struct ps8622_bridge { + struct drm_connector connector; + struct i2c_client *client; + struct drm_bridge *bridge; + struct drm_panel *panel; + struct regulator *v12; + struct backlight_device *bl; + struct mutex enable_mutex; + + struct gpio_desc *gpio_slp_n; + struct gpio_desc *gpio_rst_n; + + u8 max_lane_count; + u8 lane_count; + + bool enabled; +}; + +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 10000 +#define PS8622_RST_HIGH_T2_MIN_US 3000 +#define PS8622_RST_HIGH_T2_MAX_US 30000 +#define PS8622_PWMO_END_T12_MS 200 +#define PS8622_POWER_FALL_T16_MAX_US 10000 +#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); + msg.buf = data; + + ret = i2c_transfer(adap, &msg, 1); + if (ret != 1) + pr_warn("PS8622 I2C write (0x%02x,0x%02x,0x%02x) failed: %d\n", + client->addr + page, reg, val, ret); + return !(ret == 1); +} + +static int ps8622_send_config(struct ps8622_bridge *ps_bridge) +{ + struct i2c_client *cl = ps_bridge->client; + int err = 0; + + /* wait 20ms after power ON */ + usleep_range(20000, 30000); + + err |= ps8622_set(cl, 0x02, 0xa1, 0x01); /* HPD low */ + /* SW setting */ + err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage + * is lower to 96% */ + /* RCO SS setting */ + err |= ps8622_set(cl, 0x04, 0xe3, 0x20); /* [5:4] = b01 0.5%, b10 1%, + * b11 1.5% */ + err |= ps8622_set(cl, 0x04, 0xe2, 0x80); /* [7] RCO SS enable */ + /* RPHY Setting */ + err |= ps8622_set(cl, 0x04, 0x8a, 0x0c); /* [3:2] CDR tune wait cycle + * before measure for fine tune + * b00: 1us b01: 0.5us b10:2us + * b11: 4us */ + err |= ps8622_set(cl, 0x04, 0x89, 0x08); /* [3] RFD always on */ + err |= ps8622_set(cl, 0x04, 0x71, 0x2d); /* CTN lock in/out: + * 20000ppm/80000ppm. + * Lock out 2 times. */ + /* 2.7G CDR settings */ + err |= ps8622_set(cl, 0x04, 0x7d, 0x07); /* NOF=40LSB for HBR CDR + * setting */ + err |= ps8622_set(cl, 0x04, 0x7b, 0x00); /* [1:0] Fmin=+4bands */ + err |= ps8622_set(cl, 0x04, 0x7a, 0xfd); /* [7:5] DCO_FTRNG=+-40% */ + /* 1.62G CDR settings */ + err |= ps8622_set(cl, 0x04, 0xc0, 0x12); /* [5:2]NOF=64LSB [1:0]DCO + * scale is 2/5 */ + err |= ps8622_set(cl, 0x04, 0xc1, 0x92); /* Gitune=-37% */ + err |= ps8622_set(cl, 0x04, 0xc2, 0x1c); /* Fbstep=100% */ + err |= ps8622_set(cl, 0x04, 0x32, 0x80); /* [7] LOS signal disable */ + /* RPIO Setting */ + err |= ps8622_set(cl, 0x04, 0x00, 0xb0); /* [7:4] LVDS driver bias + * current : 75% (250mV swing) + * */ + err |= ps8622_set(cl, 0x04, 0x15, 0x40); /* [7:6] Right-bar GPIO output + * strength is 8mA */ + /* EQ Training State Machine Setting */ + err |= ps8622_set(cl, 0x04, 0x54, 0x10); /* RCO calibration start */ + /* Logic, needs more than 10 I2C command */ + err |= ps8622_set(cl, 0x01, 0x02, 0x80 | ps_bridge->max_lane_count); + /* [4:0] MAX_LANE_COUNT set to + * max supported lanes */ + err |= ps8622_set(cl, 0x01, 0x21, 0x80 | ps_bridge->lane_count); + /* [4:0] LANE_COUNT_SET set to + * chosen lane count */ + err |= ps8622_set(cl, 0x00, 0x52, 0x20); + err |= ps8622_set(cl, 0x00, 0xf1, 0x03); /* HPD CP toggle enable */ + err |= ps8622_set(cl, 0x00, 0x62, 0x41); + err |= ps8622_set(cl, 0x00, 0xf6, 0x01); /* Counter number, add 1ms + * counter delay */ + err |= ps8622_set(cl, 0x00, 0x77, 0x06); /* [6]PWM function control by + * DPCD0040f[7], default is PWM + * block always works. */ + err |= ps8622_set(cl, 0x00, 0x4c, 0x04); /* 04h Adjust VTotal tolerance + * to fix the 30Hz no display + * issue */ + err |= ps8622_set(cl, 0x01, 0xc0, 0x00); /* DPCD00400='h00, Parade OUI = + * 'h001cf8 */ + err |= ps8622_set(cl, 0x01, 0xc1, 0x1c); /* DPCD00401='h1c */ + err |= ps8622_set(cl, 0x01, 0xc2, 0xf8); /* DPCD00402='hf8 */ + err |= ps8622_set(cl, 0x01, 0xc3, 0x44); /* DPCD403~408 = ASCII code + * D2SLV5='h4432534c5635 */ + err |= ps8622_set(cl, 0x01, 0xc4, 0x32); /* DPCD404 */ + err |= ps8622_set(cl, 0x01, 0xc5, 0x53); /* DPCD405 */ + err |= ps8622_set(cl, 0x01, 0xc6, 0x4c); /* DPCD406 */ + err |= ps8622_set(cl, 0x01, 0xc7, 0x56); /* DPCD407 */ + err |= ps8622_set(cl, 0x01, 0xc8, 0x35); /* DPCD408 */ + err |= ps8622_set(cl, 0x01, 0xca, 0x01); /* DPCD40A, Initial Code major + * revision '01' */ + err |= ps8622_set(cl, 0x01, 0xcb, 0x05); /* DPCD40B, Initial Code minor + * revision '05' */ + if (ps_bridge->bl) { + err |= ps8622_set(cl, 0x01, 0xa5, 0xa0); + /* DPCD720, internal PWM */ + err |= ps8622_set(cl, 0x01, 0xa7, + ps_bridge->bl->props.brightness); + /* FFh for 100% brightness, + * 0h for 0% brightness */ + } else { + err |= ps8622_set(cl, 0x01, 0xa5, 0x80); + /* DPCD720, external PWM */ + } + err |= ps8622_set(cl, 0x01, 0xcc, 0x13); /* Set LVDS output as 6bit-VESA + * mapping, single LVDS channel + * */ + err |= ps8622_set(cl, 0x02, 0xb1, 0x20); /* Enable SSC set by register + * */ + err |= ps8622_set(cl, 0x04, 0x10, 0x16); /* Set SSC enabled and +/-1% + * central spreading */ + /* Logic end */ + err |= ps8622_set(cl, 0x04, 0x59, 0x60); /* MPU Clock source: LC => RCO + * */ + err |= ps8622_set(cl, 0x04, 0x54, 0x14); /* LC -> RCO */ + err |= ps8622_set(cl, 0x02, 0xa1, 0x91); /* HPD high */ + + return err ? -EIO : 0; +} + +static int ps8622_backlight_update(struct backlight_device *bl) +{ + struct ps8622_bridge *ps_bridge = dev_get_drvdata(&bl->dev); + int ret, brightness = bl->props.brightness; + + if (bl->props.power != FB_BLANK_UNBLANK || + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) + brightness = 0; + + mutex_lock(&ps_bridge->enable_mutex); + + if (!ps_bridge->enabled) { + ret = -EINVAL; + goto out; + } + + ret = ps8622_set(ps_bridge->client, 0x01, 0xa7, brightness); + +out: + mutex_unlock(&ps_bridge->enable_mutex); + return ret; +} + +static int ps8622_backlight_get(struct backlight_device *bl) +{ + return bl->props.brightness; +} + +static const struct backlight_ops ps8622_backlight_ops = { + .update_status = ps8622_backlight_update, + .get_brightness = ps8622_backlight_get, +}; + +static void ps8622_pre_enable(struct drm_bridge *bridge) +{ + struct ps8622_bridge *ps_bridge = bridge->driver_private; + int ret; + + mutex_lock(&ps_bridge->enable_mutex); + if (ps_bridge->enabled) + goto out; + + gpiod_set_value(ps_bridge->gpio_rst_n, 0); + + if (ps_bridge->v12) { + ret = regulator_enable(ps_bridge->v12); + if (ret) + DRM_ERROR("fails to enable ps_bridge->v12"); + } + + drm_panel_prepare(ps_bridge->panel); + + gpiod_set_value(ps_bridge->gpio_slp_n, 1); + + /* + * T1 is the range of time that it takes for the power to rise after we + * enable the lcd fet. T2 is the range of time in which the data sheet + * specifies we should deassert the reset pin. + * + * If it takes T1.max for the power to rise, we need to wait atleast + * T2.min before deasserting the reset pin. If it takes T1.min for the + * power to rise, we need to wait at most T2.max before deasserting the + * reset pin. + */ + usleep_range(PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US, + PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US); + + gpiod_set_value(ps_bridge->gpio_rst_n, 1); + + ret = ps8622_send_config(ps_bridge); + if (ret) + DRM_ERROR("Failed to send config to bridge (%d)\n", ret); + + ps_bridge->enabled = true; + +out: + mutex_unlock(&ps_bridge->enable_mutex); +} + +static void ps8622_enable(struct drm_bridge *bridge) +{ + struct ps8622_bridge *ps_bridge = bridge->driver_private; + + mutex_lock(&ps_bridge->enable_mutex); + drm_panel_enable(ps_bridge->panel); + mutex_unlock(&ps_bridge->enable_mutex); +} + +static void ps8622_disable(struct drm_bridge *bridge) +{ + struct ps8622_bridge *ps_bridge = bridge->driver_private; + + mutex_lock(&ps_bridge->enable_mutex); + + if (!ps_bridge->enabled) + goto out; + + ps_bridge->enabled = false; + + drm_panel_disable(ps_bridge->panel); + msleep(PS8622_PWMO_END_T12_MS); + + /* + * This doesn't matter if the regulators are turned off, but something + * else might keep them on. In that case, we want to assert the slp gpio + * to lower power. + */ + gpiod_set_value(ps_bridge->gpio_slp_n, 0); + + if (ps_bridge->v12) + regulator_disable(ps_bridge->v12); + + /* + * Sleep for at least the amount of time that it takes the power rail to + * fall to prevent asserting the rst gpio from doing anything. + */ + usleep_range(PS8622_POWER_FALL_T16_MAX_US, + 2 * PS8622_POWER_FALL_T16_MAX_US); + gpiod_set_value(ps_bridge->gpio_rst_n, 0); + + msleep(PS8622_POWER_OFF_T17_MS); + +out: + mutex_unlock(&ps_bridge->enable_mutex); +} + +static void ps8622_post_disable(struct drm_bridge *bridge) +{ + struct ps8622_bridge *ps_bridge = bridge->driver_private; + + drm_panel_unprepare(ps_bridge->panel); +} + +static void ps8622_destroy(struct drm_bridge *bridge) +{ + drm_bridge_cleanup(bridge); +} + +static int ps8622_get_modes(struct drm_connector *connector) +{ + struct ps8622_bridge *ps_bridge; + + ps_bridge = container_of(connector, struct ps8622_bridge, connector); + + return drm_panel_get_modes(ps_bridge->panel); +} + +static struct drm_encoder *ps8622_best_encoder(struct drm_connector *connector) +{ + struct ps8622_bridge *ps_bridge; + + ps_bridge = container_of(connector, struct ps8622_bridge, connector); + + return ps_bridge->bridge->encoder; +} + +static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = { + .get_modes = ps8622_get_modes, + .best_encoder = ps8622_best_encoder, +}; + +static enum drm_connector_status ps8622_detect(struct drm_connector *connector, + bool force) +{ + return connector_status_connected; +} + +static void ps8622_connector_destroy(struct drm_connector *connector) +{ + drm_connector_cleanup(connector); +} + +static const struct drm_connector_funcs ps8622_connector_funcs = { + .dpms = drm_helper_connector_dpms, + .fill_modes = drm_helper_probe_single_connector_modes, + .detect = ps8622_detect, + .destroy = ps8622_connector_destroy, +}; + +int ps8622_post_encoder_init(struct drm_bridge *bridge) +{ + struct ps8622_bridge *ps_bridge = bridge->driver_private; + int ret; + + /* bridge is attached to encoder. + * safe to remove it from the bridge_lookup table. + */ + drm_bridge_remove_from_lookup(bridge); + + ret = drm_bridge_init(bridge->drm_dev, bridge); + if (ret) { + DRM_ERROR("Failed to initialize bridge with drm\n"); + return ret; + } + + /* connector implementation */ + ps_bridge->connector.polled = bridge->connector_polled; + + ret = drm_connector_init(bridge->drm_dev, &ps_bridge->connector, + &ps8622_connector_funcs, DRM_MODE_CONNECTOR_LVDS); + if (ret) { + DRM_ERROR("Failed to initialize connector with drm\n"); + return ret; + } + drm_connector_helper_add(&ps_bridge->connector, + &ps8622_connector_helper_funcs); + drm_connector_register(&ps_bridge->connector); + drm_mode_connector_attach_encoder(&ps_bridge->connector, + bridge->encoder); + + if (ps_bridge->panel) + drm_panel_attach(ps_bridge->panel, &ps_bridge->connector); + + return ret; +} + +static const struct drm_bridge_funcs ps8622_bridge_funcs = { + .post_encoder_init = ps8622_post_encoder_init, + .pre_enable = ps8622_pre_enable, + .enable = ps8622_enable, + .disable = ps8622_disable, + .post_disable = ps8622_post_disable, + .destroy = ps8622_destroy, +}; + +static const struct of_device_id ps8622_devices[] = { + { + .compatible = "parade,ps8622", + .data = &ps8622_data, + }, { + .compatible = "parade,ps8625", + .data = &ps8625_data, + }, { + /* end node */ + } +}; +MODULE_DEVICE_TABLE(of, ps8622_devices); + +static inline struct ps8622_device_data *get_device_data(struct device *dev) +{ + const struct of_device_id *match = + of_match_device(ps8622_devices, dev); + + return (struct ps8622_device_data *)match->data; +} + +static int ps8622_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct device *dev = &(client->dev); + struct device_node *panel_node, *backlight_node; + struct drm_bridge *bridge; + struct backlight_device *backlight_dev; + struct ps8622_bridge *ps_bridge; + const struct ps8622_device_data *device_data; + int ret; + + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); + if (!bridge) { + DRM_ERROR("Failed to allocate drm bridge\n"); + return -ENOMEM; + } + + ps_bridge = devm_kzalloc(dev, sizeof(*ps_bridge), GFP_KERNEL); + if (!ps_bridge) { + DRM_ERROR("could not allocate ps bridge\n"); + return -ENOMEM; + } + + panel_node = of_parse_phandle(dev->of_node, "panel", 0); + if (panel_node) { + ps_bridge->panel = of_drm_find_panel(panel_node); + of_node_put(panel_node); + if (!ps_bridge->panel) + return -EPROBE_DEFER; + } + + backlight_dev = NULL; + backlight_node = of_parse_phandle(dev->of_node, "backlight", 0); + if (backlight_node) { + backlight_dev = of_find_backlight_by_node(backlight_node); + of_node_put(backlight_node); + if (!backlight_dev) + return -EPROBE_DEFER; + } + + mutex_init(&ps_bridge->enable_mutex); + + bridge->dev = dev; + bridge->driver_private = ps_bridge; + bridge->funcs = &ps8622_bridge_funcs; + + ps_bridge->client = client; + ps_bridge->bridge = bridge; + + device_data = get_device_data(dev); + + ps_bridge->v12 = devm_regulator_get(&client->dev, "vdd_bridge"); + if (IS_ERR(ps_bridge->v12)) { + DRM_INFO("no 1.2v regulator found for PS8622\n"); + ps_bridge->v12 = NULL; + } + + ps_bridge->gpio_slp_n = devm_gpiod_get(&client->dev, "sleep"); + if (IS_ERR(ps_bridge->gpio_slp_n)) { + ret = PTR_ERR(ps_bridge->gpio_slp_n); + DRM_ERROR("cannot get gpio_slp_n %d\n", ret); + goto err_client; + } else { + ret = gpiod_direction_output(ps_bridge->gpio_slp_n, 1); + if (ret) { + DRM_ERROR("cannot configure gpio_slp_n\n"); + goto err_client; + } + } + + ps_bridge->gpio_rst_n = devm_gpiod_get(&client->dev, "reset"); + if (IS_ERR(ps_bridge->gpio_rst_n)) { + ret = PTR_ERR(ps_bridge->gpio_rst_n); + DRM_ERROR("cannot get gpio_rst_n %d\n", ret); + goto err_client; + } else { + /* + * Assert the reset pin high to avoid the bridge being + * initialized prematurely + */ + ret = gpiod_direction_output(ps_bridge->gpio_rst_n, 1); + if (ret) { + DRM_ERROR("cannot configure gpio_slp_n\n"); + goto err_client; + } + } + + ps_bridge->max_lane_count = device_data->max_lane_count; + + if (of_property_read_u8(dev->of_node, "lane-count", + &ps_bridge->lane_count)) + ps_bridge->lane_count = ps_bridge->max_lane_count; + else if (ps_bridge->lane_count > ps_bridge->max_lane_count) { + DRM_INFO("lane-count property is too high for DP bridge\n"); + ps_bridge->lane_count = ps_bridge->max_lane_count; + } + + if (!backlight_dev) { + ps_bridge->bl = backlight_device_register("ps8622-backlight", + dev, ps_bridge, &ps8622_backlight_ops, + NULL); + if (IS_ERR(ps_bridge->bl)) { + DRM_ERROR("failed to register backlight\n"); + ret = PTR_ERR(ps_bridge->bl); + ps_bridge->bl = NULL; + goto err_client; + } + ps_bridge->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS; + ps_bridge->bl->props.brightness = PS8622_MAX_BRIGHTNESS; + } + + i2c_set_clientdata(client, ps_bridge); + + drm_bridge_add_for_lookup(bridge); + + return 0; + +err_client: + DRM_ERROR("device probe failed : %d\n", ret); + return ret; +} + +static int ps8622_remove(struct i2c_client *client) +{ + struct ps8622_bridge *ps_bridge = i2c_get_clientdata(client); + + if (ps_bridge->bl) + backlight_device_unregister(ps_bridge->bl); + + return 0; +} + +static const struct i2c_device_id ps8622_i2c_table[] = { + {"parade", 0}, + {}, +}; +MODULE_DEVICE_TABLE(i2c, ps8622_i2c_table); + +struct i2c_driver ps8622_driver = { + .id_table = ps8622_i2c_table, + .probe = ps8622_probe, + .remove = ps8622_remove, + .driver = { + .name = "parade", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(ps8622_devices), + }, +}; +module_i2c_driver(ps8622_driver); + +MODULE_AUTHOR("Vincent Palatin <vpalatin@chromium.org>"); +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); +MODULE_LICENSE("GPL");