| Message ID | 1406316130-4744-8-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: [...] > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 1e2f96c..0b12d16 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -6,4 +6,14 @@ config DRM_BRIDGE > > menu "bridge chips" > depends on DRM_BRIDGE > + > +config DRM_PTN3460 > + tristate "NXP ptn3460 eDP/LVDS bridge" > + depends on DRM && DRM_BRIDGE I don't think you need these two dependencies any longer since they are implicit in the "bridge chips" menu. > diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c [...] > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/i2c.h> > +#include <linux/gpio.h> > +#include <linux/delay.h> > +#include <drm/drm_panel.h> These should be ordered alphabetically, but they are already this way in the original driver, so the reordering can be a separate patch. > +struct ptn3460_bridge { > + struct drm_connector connector; > + struct i2c_client *client; > + struct drm_bridge *bridge; I think it would be much more natural for ptn3460_bridge to embed struct drm_bridge rather than store a pointer to it. > + struct drm_panel *panel; > + struct edid *edid; > + struct gpio_desc *gpio_pd_n; > + struct gpio_desc *gpio_rst_n; > + u32 edid_emulation; > + bool enabled; > +}; > + > +static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, > + u8 *buf, int len) > +{ > + int ret; > + > + ret = i2c_master_send(ptn_bridge->client, &addr, 1); > + if (ret <= 0) { > + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > + return ret; > + } > + > + ret = i2c_master_recv(ptn_bridge->client, buf, len); > + if (ret <= 0) { > + DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); > + return ret; > + } This isn't introduced by this patch, but doesn't this require locking so that this is an atomic transaction? Perhaps it could be rewritten using i2c_smbus_read_block_data()? > +static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, > + char val) > +{ > + int ret; > + char buf[2]; > + > + buf[0] = addr; > + buf[1] = val; > + > + ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); > + if (ret <= 0) { > + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > + return ret; > + } > + > + return 0; > +} Same here, this looks like it could be i2c_smbus_write_byte_data(). > +static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) > +{ > + int ret; > + char val; > + > + /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */ > + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR, > + ptn_bridge->edid_emulation); > + if (ret) { > + DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret); > + return ret; > + } > + > + /* Enable EDID emulation and select the desired EDID */ > + val = 1 << PTN3460_EDID_ENABLE_EMULATION | > + ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION; > + > + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val); > + if (ret) { > + DRM_ERROR("Failed to write edid value, ret=%d\n", ret); > + return ret; > + } > + > + return 0; > +} s/edid/EDID/ in the above (and below, too), but again the original driver had this, so it can be a separate patch. > +static void ptn3460_pre_enable(struct drm_bridge *bridge) > +{ > + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; If you embed drm_bridge within ptn3460_bridge then you can use the much more canonical container_of() macro (or rather a driver-specific inline function that wraps it) and no longer need the drm_bridge.driver_private field. > + int ret; > + > + if (ptn_bridge->enabled) > + return; > + > + gpiod_set_value(ptn_bridge->gpio_pd_n, 1); > + > + gpiod_set_value(ptn_bridge->gpio_rst_n, 0); > + udelay(10); This shouldn't be using udelay(), usleep_range(10, 20) (or similar) would be better. Again, can be a separate patch. > + gpiod_set_value(ptn_bridge->gpio_rst_n, 1); It also seems like you've converted to using the gpiod_*() API, but the driver previously used gpio_is_valid() to check that both PD and RST pins had valid GPIOs associated with them. The device tree binding said that they are required, though. So this patch actually does the right thing by making them non-optional but it also changes behaviour from the original. Like I said earlier, I would very much prefer that this conversion be split into separate patches rather than one patch that removes the old driver and a second patch that adds a new one. It makes it really difficult to tell what's really changing, breaks bisectability and generally makes our lives miserable. > + > + drm_panel_prepare(ptn_bridge->panel); This should check for errors. > +static void ptn3460_enable(struct drm_bridge *bridge) > +{ > + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; > + > + drm_panel_enable(ptn_bridge->panel); Should check for errors as well. > +int ptn3460_get_modes(struct drm_connector *connector) static? There seem to be quite a few functions that can be locally scoped. Again, this seems to be the case in the original driver as but it should definitely be fixed at some point. > +{ > + struct ptn3460_bridge *ptn_bridge; > + u8 *edid; > + int ret, num_modes; > + bool power_off; > + > + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); > + > + if (ptn_bridge->edid) > + return drm_add_edid_modes(connector, ptn_bridge->edid); > + > + power_off = !ptn_bridge->enabled; > + ptn3460_pre_enable(ptn_bridge->bridge); > + > + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > + if (!edid) { > + DRM_ERROR("Failed to allocate edid\n"); > + return 0; > + } > + > + ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, > + EDID_LENGTH); > + if (ret) { > + kfree(edid); > + num_modes = 0; Maybe instead of doing this here you can initialize the variable when you declare it? It's always been that way, so can be a separate patch, too. > +struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) > +{ > + struct ptn3460_bridge *ptn_bridge; > + > + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); You use this long construct a couple of times, so it's useful to introduce a helper, such as this: static inline struct ptn3460_bridge * connector_to_ptn3460(struct drm_connector *connector) { return container_of(connector, struct ptn3460_bridge, connector); } > +int ptn3460_post_encoder_init(struct drm_bridge *bridge) > +{ > + struct ptn3460_bridge *ptn_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); No, you should never do this. First, you're not adding it back to the registry when the bridge is detached, so unloading and reloading the display driver will fail. Second there should never be a need to remove it from the registry as long as the driver itself is loaded. If you're concerned about a single bridge being used multiple times, there's already code to handle that in your previous patch: int drm_bridge_attach_encoder(...) { ... if (bridge->encoder) return -EBUSY; ... } Generally the registry should contain a list of bridges that have been registered, whether they're used or not is irrelevant. > + ret = drm_bridge_init(bridge->drm_dev, bridge); > + if (ret) { > + DRM_ERROR("Failed to initialize bridge with drm\n"); > + return ret; > + } > + > + /* connector implementation */ > + ptn_bridge->connector.polled = bridge->connector_polled; Why does this need to be handed from bridge to connector? You implement both the connector and the bridge in this driver, so can't you directly set ptn_bridge->connector.polled as appropriate? > + ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector, > + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); > + if (ret) { > + DRM_ERROR("Failed to initialize connector with drm\n"); > + return ret; > + } > + drm_connector_helper_add(&ptn_bridge->connector, > + &ptn3460_connector_helper_funcs); > + drm_connector_register(&ptn_bridge->connector); > + drm_mode_connector_attach_encoder(&ptn_bridge->connector, > + bridge->encoder); > + > + if (ptn_bridge->panel) > + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); > + > + return ret; > +} I'm thinking that eventually we'll want to register the connector only when a panel is attached to the bridge. This will only become important when we implement bridge chaining because if you instantiate a connector for each bridge then you'll get a list of connectors for the DRM device representing the output of each bridge rather than just the final one that goes to the display. > +static int ptn3460_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; > + struct drm_bridge *bridge; > + struct ptn3460_bridge *ptn_bridge; > + int ret; > + > + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); > + if (!bridge) { > + DRM_ERROR("Failed to allocate drm bridge\n"); > + return -ENOMEM; > + } > + > + ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL); > + if (!ptn_bridge) { > + DRM_ERROR("Failed to allocate ptn bridge\n"); > + return -ENOMEM; > + } No need for error messages on allocation failures. The allocator will already complain itself. Also I think it's usually better to use the dev_*() functions to print messages, especially given that at this stage we're not even hooked up to DRM in the first place. So in general I try to use DRM_*() functions only from DRM-specific callbacks (or functions called from them) and dev_*() otherwise. > +static int ptn3460_remove(struct i2c_client *client) > +{ > + return 0; > +} This function should remove the bridge from the lookup table by calling drm_bridge_remove(). > + > +static const struct i2c_device_id ptn3460_i2c_table[] = { > + {"nxp,ptn3460", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, ptn3460_i2c_table); > + > +static const struct of_device_id ptn3460_match[] = { > + { .compatible = "nxp,ptn3460" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, ptn3460_match); > + > +struct i2c_driver ptn3460_driver = { Is there a reason why this can't be static? > + .id_table = ptn3460_i2c_table, > + .probe = ptn3460_probe, > + .remove = ptn3460_remove, > + .driver = { > + .name = "nxp,ptn3460", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ptn3460_match), You don't need of_match_ptr() here since you already depend on OF in Kconfig, therefore of_match_ptr(x) will always evaluate to x. > + }, > +}; > +module_i2c_driver(ptn3460_driver); > + > +MODULE_AUTHOR("Sean Paul <seanpaul@chromium.org>"); > +MODULE_DESCRIPTION("NXP ptn3460 eDP-LVDS converter driver"); > +MODULE_LICENSE("GPL"); This should be "GPL v2". Thierry
On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: > [...] >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 1e2f96c..0b12d16 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -6,4 +6,14 @@ config DRM_BRIDGE >> >> menu "bridge chips" >> depends on DRM_BRIDGE >> + >> +config DRM_PTN3460 >> + tristate "NXP ptn3460 eDP/LVDS bridge" >> + depends on DRM && DRM_BRIDGE > > I don't think you need these two dependencies any longer since they are > implicit in the "bridge chips" menu. Ok. >> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c > [...] >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_gpio.h> >> +#include <linux/i2c.h> >> +#include <linux/gpio.h> >> +#include <linux/delay.h> >> +#include <drm/drm_panel.h> > > These should be ordered alphabetically, but they are already this way in > the original driver, so the reordering can be a separate patch. This can be done later. >> +struct ptn3460_bridge { >> + struct drm_connector connector; >> + struct i2c_client *client; >> + struct drm_bridge *bridge; > > I think it would be much more natural for ptn3460_bridge to embed struct > drm_bridge rather than store a pointer to it. Right. As you said, we can eliminate driver_private and use container_of. >> + struct drm_panel *panel; >> + struct edid *edid; >> + struct gpio_desc *gpio_pd_n; >> + struct gpio_desc *gpio_rst_n; >> + u32 edid_emulation; >> + bool enabled; >> +}; >> + >> +static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, >> + u8 *buf, int len) >> +{ >> + int ret; >> + >> + ret = i2c_master_send(ptn_bridge->client, &addr, 1); >> + if (ret <= 0) { >> + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); >> + return ret; >> + } >> + >> + ret = i2c_master_recv(ptn_bridge->client, buf, len); >> + if (ret <= 0) { >> + DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); >> + return ret; >> + } > > This isn't introduced by this patch, but doesn't this require locking so > that this is an atomic transaction? > > Perhaps it could be rewritten using i2c_smbus_read_block_data()? Well, I am not quite aware of i2c functions. I will have a look into it though. >> +static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, >> + char val) >> +{ >> + int ret; >> + char buf[2]; >> + >> + buf[0] = addr; >> + buf[1] = val; >> + >> + ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); >> + if (ret <= 0) { >> + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} > > Same here, this looks like it could be i2c_smbus_write_byte_data(). > >> +static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) >> +{ >> + int ret; >> + char val; >> + >> + /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */ >> + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR, >> + ptn_bridge->edid_emulation); >> + if (ret) { >> + DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret); >> + return ret; >> + } >> + >> + /* Enable EDID emulation and select the desired EDID */ >> + val = 1 << PTN3460_EDID_ENABLE_EMULATION | >> + ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION; >> + >> + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val); >> + if (ret) { >> + DRM_ERROR("Failed to write edid value, ret=%d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} > > s/edid/EDID/ in the above (and below, too), but again the original > driver had this, so it can be a separate patch. This can be done later. >> +static void ptn3460_pre_enable(struct drm_bridge *bridge) >> +{ >> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; > > If you embed drm_bridge within ptn3460_bridge then you can use the much > more canonical container_of() macro (or rather a driver-specific inline > function that wraps it) and no longer need the drm_bridge.driver_private > field. Agreed. >> + int ret; >> + >> + if (ptn_bridge->enabled) >> + return; >> + >> + gpiod_set_value(ptn_bridge->gpio_pd_n, 1); >> + >> + gpiod_set_value(ptn_bridge->gpio_rst_n, 0); >> + udelay(10); > > This shouldn't be using udelay(), usleep_range(10, 20) (or similar) > would be better. Again, can be a separate patch. > >> + gpiod_set_value(ptn_bridge->gpio_rst_n, 1); > > It also seems like you've converted to using the gpiod_*() API, but the > driver previously used gpio_is_valid() to check that both PD and RST > pins had valid GPIOs associated with them. The device tree binding said > that they are required, though. > > So this patch actually does the right thing by making them non-optional > but it also changes behaviour from the original. Like I said earlier, I > would very much prefer that this conversion be split into separate > patches rather than one patch that removes the old driver and a second > patch that adds a new one. It makes it really difficult to tell what's > really changing, breaks bisectability and generally makes our lives > miserable. Ok. I will add these as incremental changes. >> + >> + drm_panel_prepare(ptn_bridge->panel); > > This should check for errors. Ok. >> +static void ptn3460_enable(struct drm_bridge *bridge) >> +{ >> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; >> + >> + drm_panel_enable(ptn_bridge->panel); > > Should check for errors as well. Ok. >> +int ptn3460_get_modes(struct drm_connector *connector) > > static? There seem to be quite a few functions that can be locally > scoped. Again, this seems to be the case in the original driver as > but it should definitely be fixed at some point. Ok. >> +{ >> + struct ptn3460_bridge *ptn_bridge; >> + u8 *edid; >> + int ret, num_modes; >> + bool power_off; >> + >> + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); >> + >> + if (ptn_bridge->edid) >> + return drm_add_edid_modes(connector, ptn_bridge->edid); >> + >> + power_off = !ptn_bridge->enabled; >> + ptn3460_pre_enable(ptn_bridge->bridge); >> + >> + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); >> + if (!edid) { >> + DRM_ERROR("Failed to allocate edid\n"); >> + return 0; >> + } >> + >> + ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, >> + EDID_LENGTH); >> + if (ret) { >> + kfree(edid); >> + num_modes = 0; > > Maybe instead of doing this here you can initialize the variable when > you declare it? It's always been that way, so can be a separate patch, > too. Ok. >> +struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) >> +{ >> + struct ptn3460_bridge *ptn_bridge; >> + >> + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); > > You use this long construct a couple of times, so it's useful to > introduce a helper, such as this: > > static inline struct ptn3460_bridge * > connector_to_ptn3460(struct drm_connector *connector) > { > return container_of(connector, struct ptn3460_bridge, connector); > } Ok, will use this. >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge) >> +{ >> + struct ptn3460_bridge *ptn_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); > > No, you should never do this. First, you're not adding it back to the > registry when the bridge is detached, so unloading and reloading the > display driver will fail. Second there should never be a need to remove > it from the registry as long as the driver itself is loaded. If you're > concerned about a single bridge being used multiple times, there's > already code to handle that in your previous patch: > > int drm_bridge_attach_encoder(...) > { > ... > > if (bridge->encoder) > return -EBUSY; > > ... > } > > Generally the registry should contain a list of bridges that have been > registered, whether they're used or not is irrelevant. I was just wondering if it is ok to have a node in two independent lists? bridge_lookup_table and the other mode_config.bridge_list? >> + ret = drm_bridge_init(bridge->drm_dev, bridge); >> + if (ret) { >> + DRM_ERROR("Failed to initialize bridge with drm\n"); >> + return ret; >> + } >> + >> + /* connector implementation */ >> + ptn_bridge->connector.polled = bridge->connector_polled; > > Why does this need to be handed from bridge to connector? You implement > both the connector and the bridge in this driver, so can't you directly > set ptn_bridge->connector.polled as appropriate? As explained for the previous patch, how to choose the polled flag? >> + ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector, >> + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); >> + if (ret) { >> + DRM_ERROR("Failed to initialize connector with drm\n"); >> + return ret; >> + } >> + drm_connector_helper_add(&ptn_bridge->connector, >> + &ptn3460_connector_helper_funcs); >> + drm_connector_register(&ptn_bridge->connector); >> + drm_mode_connector_attach_encoder(&ptn_bridge->connector, >> + bridge->encoder); >> + >> + if (ptn_bridge->panel) >> + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); >> + >> + return ret; >> +} > > I'm thinking that eventually we'll want to register the connector only > when a panel is attached to the bridge. This will only become important > when we implement bridge chaining because if you instantiate a connector > for each bridge then you'll get a list of connectors for the DRM device > representing the output of each bridge rather than just the final one > that goes to the display. So, do not initialize connector if there is no panel? and, get_modes via panel instead of doing it by edid-emulation? >> +static int ptn3460_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct device *dev = &(client->dev); > > No need for the parentheses here. Ok. >> + struct device_node *panel_node; >> + struct drm_bridge *bridge; >> + struct ptn3460_bridge *ptn_bridge; >> + int ret; >> + >> + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); >> + if (!bridge) { >> + DRM_ERROR("Failed to allocate drm bridge\n"); >> + return -ENOMEM; >> + } >> + >> + ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL); >> + if (!ptn_bridge) { >> + DRM_ERROR("Failed to allocate ptn bridge\n"); >> + return -ENOMEM; >> + } > > No need for error messages on allocation failures. The allocator will > already complain itself. > > Also I think it's usually better to use the dev_*() functions to print > messages, especially given that at this stage we're not even hooked up > to DRM in the first place. > > So in general I try to use DRM_*() functions only from DRM-specific > callbacks (or functions called from them) and dev_*() otherwise. Ok, will fix them. >> +static int ptn3460_remove(struct i2c_client *client) >> +{ >> + return 0; >> +} > > This function should remove the bridge from the lookup table by calling > drm_bridge_remove(). Just one doubt, already asked above. >> + >> +static const struct i2c_device_id ptn3460_i2c_table[] = { >> + {"nxp,ptn3460", 0}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(i2c, ptn3460_i2c_table); >> + >> +static const struct of_device_id ptn3460_match[] = { >> + { .compatible = "nxp,ptn3460" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, ptn3460_match); >> + >> +struct i2c_driver ptn3460_driver = { > > Is there a reason why this can't be static? Will make it static. >> + .id_table = ptn3460_i2c_table, >> + .probe = ptn3460_probe, >> + .remove = ptn3460_remove, >> + .driver = { >> + .name = "nxp,ptn3460", >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(ptn3460_match), > > You don't need of_match_ptr() here since you already depend on OF in > Kconfig, therefore of_match_ptr(x) will always evaluate to x. Ok, will fix it. >> + }, >> +}; >> +module_i2c_driver(ptn3460_driver); >> + >> +MODULE_AUTHOR("Sean Paul <seanpaul@chromium.org>"); >> +MODULE_DESCRIPTION("NXP ptn3460 eDP-LVDS converter driver"); >> +MODULE_LICENSE("GPL"); > > This should be "GPL v2". Ok. Will fix it. Ajay
On Wed, Jul 30, 2014 at 08:46:44PM +0530, Ajay kumar wrote: > On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: [...] > >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge) > >> +{ > >> + struct ptn3460_bridge *ptn_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); > > > > No, you should never do this. First, you're not adding it back to the > > registry when the bridge is detached, so unloading and reloading the > > display driver will fail. Second there should never be a need to remove > > it from the registry as long as the driver itself is loaded. If you're > > concerned about a single bridge being used multiple times, there's > > already code to handle that in your previous patch: > > > > int drm_bridge_attach_encoder(...) > > { > > ... > > > > if (bridge->encoder) > > return -EBUSY; > > > > ... > > } > > > > Generally the registry should contain a list of bridges that have been > > registered, whether they're used or not is irrelevant. > I was just wondering if it is ok to have a node in two independent lists? > bridge_lookup_table and the other mode_config.bridge_list? Oh, it reuses the head field for the registry. I hadn't noticed before. No, you certainly can't have the same node in two lists. Honestly I don't quite understand why there was a need to expose drm_bridge as a drm_mode_object in the first place since it's never exported to userspace. So I think that perhaps we could simply get rid of the base field and not tie in drm_bridge objects with the DRM object as we currently do. But until Sean or Rob comment on this it might be better to simply add another struct list_head field for the registry. That way both can coexist and we can independently still decide to remove the base and head fields if they're no longer needed. > >> + ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector, > >> + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); > >> + if (ret) { > >> + DRM_ERROR("Failed to initialize connector with drm\n"); > >> + return ret; > >> + } > >> + drm_connector_helper_add(&ptn_bridge->connector, > >> + &ptn3460_connector_helper_funcs); > >> + drm_connector_register(&ptn_bridge->connector); > >> + drm_mode_connector_attach_encoder(&ptn_bridge->connector, > >> + bridge->encoder); > >> + > >> + if (ptn_bridge->panel) > >> + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); > >> + > >> + return ret; > >> +} > > > > I'm thinking that eventually we'll want to register the connector only > > when a panel is attached to the bridge. This will only become important > > when we implement bridge chaining because if you instantiate a connector > > for each bridge then you'll get a list of connectors for the DRM device > > representing the output of each bridge rather than just the final one > > that goes to the display. > So, do not initialize connector if there is no panel? and, get_modes > via panel instead of doing it by edid-emulation? If there's no panel, then there's nothing to connect the connector to, so it's unneeded. Also if you have chained bridges, then each bridge would expose a connector and it would become impossible to choose the correct one. So only the final bridge in the chain should instantiate the connector. .get_modes() still needs to be done from the bridge because that is the most closely connected to the display controller and therefore dictates the timing that the display controller needs to generate. Querying the panel's .get_modes() might be useful to figure out which emulation mode to use in the bridge. Thierry
On Wed, Jul 30, 2014 at 9:10 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jul 30, 2014 at 08:46:44PM +0530, Ajay kumar wrote: >> On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote: >> > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: > [...] >> >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge) >> >> +{ >> >> + struct ptn3460_bridge *ptn_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); >> > >> > No, you should never do this. First, you're not adding it back to the >> > registry when the bridge is detached, so unloading and reloading the >> > display driver will fail. Second there should never be a need to remove >> > it from the registry as long as the driver itself is loaded. If you're >> > concerned about a single bridge being used multiple times, there's >> > already code to handle that in your previous patch: >> > >> > int drm_bridge_attach_encoder(...) >> > { >> > ... >> > >> > if (bridge->encoder) >> > return -EBUSY; >> > >> > ... >> > } >> > >> > Generally the registry should contain a list of bridges that have been >> > registered, whether they're used or not is irrelevant. >> I was just wondering if it is ok to have a node in two independent lists? >> bridge_lookup_table and the other mode_config.bridge_list? > > Oh, it reuses the head field for the registry. I hadn't noticed before. > No, you certainly can't have the same node in two lists. Honestly I > don't quite understand why there was a need to expose drm_bridge as a > drm_mode_object in the first place since it's never exported to > userspace. > > So I think that perhaps we could simply get rid of the base field and > not tie in drm_bridge objects with the DRM object as we currently do. > But until Sean or Rob comment on this it might be better to simply add > another struct list_head field for the registry. That way both can > coexist and we can independently still decide to remove the base and > head fields if they're no longer needed. Ok. What shall I name the new list_head? >> >> + ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector, >> >> + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); >> >> + if (ret) { >> >> + DRM_ERROR("Failed to initialize connector with drm\n"); >> >> + return ret; >> >> + } >> >> + drm_connector_helper_add(&ptn_bridge->connector, >> >> + &ptn3460_connector_helper_funcs); >> >> + drm_connector_register(&ptn_bridge->connector); >> >> + drm_mode_connector_attach_encoder(&ptn_bridge->connector, >> >> + bridge->encoder); >> >> + >> >> + if (ptn_bridge->panel) >> >> + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); >> >> + >> >> + return ret; >> >> +} >> > >> > I'm thinking that eventually we'll want to register the connector only >> > when a panel is attached to the bridge. This will only become important >> > when we implement bridge chaining because if you instantiate a connector >> > for each bridge then you'll get a list of connectors for the DRM device >> > representing the output of each bridge rather than just the final one >> > that goes to the display. >> So, do not initialize connector if there is no panel? and, get_modes >> via panel instead of doing it by edid-emulation? > > If there's no panel, then there's nothing to connect the connector to, > so it's unneeded. Also if you have chained bridges, then each bridge > would expose a connector and it would become impossible to choose the > correct one. So only the final bridge in the chain should instantiate > the connector. Since there is only a single bridge when it comes to ptn3460/ps8622, lets not talk about chaining of bridges now. And, I agree that if there is no panel, then no need to register connector. > .get_modes() still needs to be done from the bridge because that is the > most closely connected to the display controller and therefore dictates > the timing that the display controller needs to generate. > > Querying the panel's .get_modes() might be useful to figure out which > emulation mode to use in the bridge. But, get_modes from panel returns me only the no_of_modes but not the actual mode structure. How do I compare the list of supported emulation modes? Ajay
On Wed, Jul 30, 2014 at 09:44:32PM +0530, Ajay kumar wrote: > On Wed, Jul 30, 2014 at 9:10 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Wed, Jul 30, 2014 at 08:46:44PM +0530, Ajay kumar wrote: > >> On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > >> > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: > > [...] > >> >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge) > >> >> +{ > >> >> + struct ptn3460_bridge *ptn_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); > >> > > >> > No, you should never do this. First, you're not adding it back to the > >> > registry when the bridge is detached, so unloading and reloading the > >> > display driver will fail. Second there should never be a need to remove > >> > it from the registry as long as the driver itself is loaded. If you're > >> > concerned about a single bridge being used multiple times, there's > >> > already code to handle that in your previous patch: > >> > > >> > int drm_bridge_attach_encoder(...) > >> > { > >> > ... > >> > > >> > if (bridge->encoder) > >> > return -EBUSY; > >> > > >> > ... > >> > } > >> > > >> > Generally the registry should contain a list of bridges that have been > >> > registered, whether they're used or not is irrelevant. > >> I was just wondering if it is ok to have a node in two independent lists? > >> bridge_lookup_table and the other mode_config.bridge_list? > > > > Oh, it reuses the head field for the registry. I hadn't noticed before. > > No, you certainly can't have the same node in two lists. Honestly I > > don't quite understand why there was a need to expose drm_bridge as a > > drm_mode_object in the first place since it's never exported to > > userspace. > > > > So I think that perhaps we could simply get rid of the base field and > > not tie in drm_bridge objects with the DRM object as we currently do. > > But until Sean or Rob comment on this it might be better to simply add > > another struct list_head field for the registry. That way both can > > coexist and we can independently still decide to remove the base and > > head fields if they're no longer needed. > Ok. What shall I name the new list_head? "list" would be a good choice in my opinion. > > .get_modes() still needs to be done from the bridge because that is the > > most closely connected to the display controller and therefore dictates > > the timing that the display controller needs to generate. > > > > Querying the panel's .get_modes() might be useful to figure out which > > emulation mode to use in the bridge. > But, get_modes from panel returns me only the no_of_modes but > not the actual mode structure. How do I compare the list of supported > emulation modes? You could iterate over the connector's probed_modes list which should contain all the modes that the panel reported (after .get_modes() was called). Thierry
diff --git a/Documentation/devicetree/bindings/video/bridge/ptn3460.txt b/Documentation/devicetree/bindings/video/bridge/ptn3460.txt new file mode 100644 index 0000000..03366c4 --- /dev/null +++ b/Documentation/devicetree/bindings/video/bridge/ptn3460.txt @@ -0,0 +1,27 @@ +ptn3460 bridge bindings + +Required properties: + - compatible: "nxp,ptn3460" + - reg: i2c address of the bridge + - powerdown-gpios: OF device-tree gpio specification + - reset-gpios: OF device-tree gpio specification + - edid-emulation: The EDID emulation entry to use + +-------+------------+------------------+ + | Value | Resolution | Description | + | 0 | 1024x768 | NXP Generic | + | 1 | 1920x1080 | NXP Generic | + | 2 | 1920x1080 | NXP Generic | + | 3 | 1600x900 | Samsung LTM200KT | + | 4 | 1920x1080 | Samsung LTM230HT | + | 5 | 1366x768 | NXP Generic | + | 6 | 1600x900 | ChiMei M215HGE | + +-------+------------+------------------+ + +Example: + lvds-bridge@20 { + compatible = "nxp,ptn3460"; + reg = <0x20>; + powerdown-gpios = <&gpy2 5 1 0 0>; + reset-gpios = <&gpx1 5 1 0 0>; + edid-emulation = <5>; + }; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 1e2f96c..0b12d16 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -6,4 +6,14 @@ config DRM_BRIDGE menu "bridge chips" depends on DRM_BRIDGE + +config DRM_PTN3460 + tristate "NXP ptn3460 eDP/LVDS bridge" + depends on DRM && DRM_BRIDGE + depends on OF + depends on I2C + select DRM_PANEL + help + ptn3460 eDP-LVDS bridge chip driver. + endmenu diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index be16eca..b4733e1 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1 +1,3 @@ ccflags-y := -Iinclude/drm + +obj-$(CONFIG_DRM_PTN3460) += ptn3460.o diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c new file mode 100644 index 0000000..f41302a --- /dev/null +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -0,0 +1,405 @@ +/* + * NXP PTN3460 DP/LVDS bridge driver + * + * Copyright (C) 2013 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/of.h> +#include <linux/of_gpio.h> +#include <linux/i2c.h> +#include <linux/gpio.h> +#include <linux/delay.h> +#include <drm/drm_panel.h> + +#include "drmP.h" +#include "drm_edid.h" +#include "drm_crtc.h" +#include "drm_crtc_helper.h" + +#define PTN3460_EDID_ADDR 0x0 +#define PTN3460_EDID_EMULATION_ADDR 0x84 +#define PTN3460_EDID_ENABLE_EMULATION 0 +#define PTN3460_EDID_EMULATION_SELECTION 1 +#define PTN3460_EDID_SRAM_LOAD_ADDR 0x85 + +struct ptn3460_bridge { + struct drm_connector connector; + struct i2c_client *client; + struct drm_bridge *bridge; + struct drm_panel *panel; + struct edid *edid; + struct gpio_desc *gpio_pd_n; + struct gpio_desc *gpio_rst_n; + u32 edid_emulation; + bool enabled; +}; + +static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, + u8 *buf, int len) +{ + int ret; + + ret = i2c_master_send(ptn_bridge->client, &addr, 1); + if (ret <= 0) { + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); + return ret; + } + + ret = i2c_master_recv(ptn_bridge->client, buf, len); + if (ret <= 0) { + DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); + return ret; + } + + return 0; +} + +static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, + char val) +{ + int ret; + char buf[2]; + + buf[0] = addr; + buf[1] = val; + + ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); + if (ret <= 0) { + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); + return ret; + } + + return 0; +} + +static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) +{ + int ret; + char val; + + /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */ + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR, + ptn_bridge->edid_emulation); + if (ret) { + DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret); + return ret; + } + + /* Enable EDID emulation and select the desired EDID */ + val = 1 << PTN3460_EDID_ENABLE_EMULATION | + ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION; + + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val); + if (ret) { + DRM_ERROR("Failed to write edid value, ret=%d\n", ret); + return ret; + } + + return 0; +} + +static void ptn3460_pre_enable(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; + int ret; + + if (ptn_bridge->enabled) + return; + + gpiod_set_value(ptn_bridge->gpio_pd_n, 1); + + gpiod_set_value(ptn_bridge->gpio_rst_n, 0); + udelay(10); + gpiod_set_value(ptn_bridge->gpio_rst_n, 1); + + drm_panel_prepare(ptn_bridge->panel); + + /* + * There's a bug in the PTN chip where it falsely asserts hotplug before + * it is fully functional. We're forced to wait for the maximum start up + * time specified in the chip's datasheet to make sure we're really up. + */ + msleep(90); + + ret = ptn3460_select_edid(ptn_bridge); + if (ret) + DRM_ERROR("Select edid failed ret=%d\n", ret); + + ptn_bridge->enabled = true; +} + +static void ptn3460_enable(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; + + drm_panel_enable(ptn_bridge->panel); +} + +static void ptn3460_disable(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; + + if (!ptn_bridge->enabled) + return; + + ptn_bridge->enabled = false; + + drm_panel_disable(ptn_bridge->panel); + + gpiod_set_value(ptn_bridge->gpio_rst_n, 1); + gpiod_set_value(ptn_bridge->gpio_pd_n, 0); +} + +static void ptn3460_post_disable(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; + + drm_panel_unprepare(ptn_bridge->panel); +} + +void ptn3460_bridge_destroy(struct drm_bridge *bridge) +{ + drm_bridge_cleanup(bridge); +} + +int ptn3460_get_modes(struct drm_connector *connector) +{ + struct ptn3460_bridge *ptn_bridge; + u8 *edid; + int ret, num_modes; + bool power_off; + + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); + + if (ptn_bridge->edid) + return drm_add_edid_modes(connector, ptn_bridge->edid); + + power_off = !ptn_bridge->enabled; + ptn3460_pre_enable(ptn_bridge->bridge); + + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (!edid) { + DRM_ERROR("Failed to allocate edid\n"); + return 0; + } + + ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, + EDID_LENGTH); + if (ret) { + kfree(edid); + num_modes = 0; + goto out; + } + + ptn_bridge->edid = (struct edid *)edid; + drm_mode_connector_update_edid_property(connector, ptn_bridge->edid); + + num_modes = drm_add_edid_modes(connector, ptn_bridge->edid); + +out: + if (power_off) + ptn3460_disable(ptn_bridge->bridge); + + return num_modes; +} + +struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) +{ + struct ptn3460_bridge *ptn_bridge; + + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); + + return ptn_bridge->bridge->encoder; +} + +struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = { + .get_modes = ptn3460_get_modes, + .best_encoder = ptn3460_best_encoder, +}; + +enum drm_connector_status ptn3460_detect(struct drm_connector *connector, + bool force) +{ + return connector_status_connected; +} + +void ptn3460_connector_destroy(struct drm_connector *connector) +{ + drm_connector_cleanup(connector); +} + +struct drm_connector_funcs ptn3460_connector_funcs = { + .dpms = drm_helper_connector_dpms, + .fill_modes = drm_helper_probe_single_connector_modes, + .detect = ptn3460_detect, + .destroy = ptn3460_connector_destroy, +}; + +int ptn3460_post_encoder_init(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_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 */ + ptn_bridge->connector.polled = bridge->connector_polled; + + ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector, + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); + if (ret) { + DRM_ERROR("Failed to initialize connector with drm\n"); + return ret; + } + drm_connector_helper_add(&ptn_bridge->connector, + &ptn3460_connector_helper_funcs); + drm_connector_register(&ptn_bridge->connector); + drm_mode_connector_attach_encoder(&ptn_bridge->connector, + bridge->encoder); + + if (ptn_bridge->panel) + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); + + return ret; +} + +struct drm_bridge_funcs ptn3460_bridge_funcs = { + .post_encoder_init = ptn3460_post_encoder_init, + .pre_enable = ptn3460_pre_enable, + .enable = ptn3460_enable, + .disable = ptn3460_disable, + .post_disable = ptn3460_post_disable, + .destroy = ptn3460_bridge_destroy, +}; + +static int ptn3460_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct device *dev = &(client->dev); + struct device_node *panel_node; + struct drm_bridge *bridge; + struct ptn3460_bridge *ptn_bridge; + int ret; + + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); + if (!bridge) { + DRM_ERROR("Failed to allocate drm bridge\n"); + return -ENOMEM; + } + + ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL); + if (!ptn_bridge) { + DRM_ERROR("Failed to allocate ptn bridge\n"); + return -ENOMEM; + } + + panel_node = of_parse_phandle(dev->of_node, "panel", 0); + if (panel_node) { + ptn_bridge->panel = of_drm_find_panel(panel_node); + of_node_put(panel_node); + if (!ptn_bridge->panel) + return -EPROBE_DEFER; + } + + bridge->dev = dev; + bridge->driver_private = ptn_bridge; + bridge->funcs = &ptn3460_bridge_funcs; + + ptn_bridge->client = client; + ptn_bridge->bridge = bridge; + + ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown"); + if (IS_ERR(ptn_bridge->gpio_pd_n)) { + ret = PTR_ERR(ptn_bridge->gpio_pd_n); + DRM_ERROR("cannot get gpio_pd_n %d\n", ret); + return ret; + } else { + ret = gpiod_direction_output(ptn_bridge->gpio_pd_n, 1); + if (ret) { + DRM_ERROR("cannot configure gpio_pd_n\n"); + return ret; + } + } + + ptn_bridge->gpio_rst_n = devm_gpiod_get(&client->dev, "reset"); + if (IS_ERR(ptn_bridge->gpio_rst_n)) { + ret = PTR_ERR(ptn_bridge->gpio_rst_n); + DRM_ERROR("cannot get gpio_rst_n %d\n", ret); + return ret; + } else { + /* + * Request the reset pin low to avoid the bridge being + * initialized prematurely + */ + ret = gpiod_direction_output(ptn_bridge->gpio_rst_n, 0); + if (ret) { + DRM_ERROR("cannot configure gpio_pd_n\n"); + return ret; + } + } + + ret = of_property_read_u32(dev->of_node, "edid-emulation", + &ptn_bridge->edid_emulation); + if (ret) { + DRM_ERROR("Can't read edid emulation value\n"); + return -ENODEV; + } + + i2c_set_clientdata(client, ptn_bridge); + + drm_bridge_add_for_lookup(bridge); + + return 0; +} + +static int ptn3460_remove(struct i2c_client *client) +{ + return 0; +} + +static const struct i2c_device_id ptn3460_i2c_table[] = { + {"nxp,ptn3460", 0}, + {}, +}; +MODULE_DEVICE_TABLE(i2c, ptn3460_i2c_table); + +static const struct of_device_id ptn3460_match[] = { + { .compatible = "nxp,ptn3460" }, + {}, +}; +MODULE_DEVICE_TABLE(of, ptn3460_match); + +struct i2c_driver ptn3460_driver = { + .id_table = ptn3460_i2c_table, + .probe = ptn3460_probe, + .remove = ptn3460_remove, + .driver = { + .name = "nxp,ptn3460", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(ptn3460_match), + }, +}; +module_i2c_driver(ptn3460_driver); + +MODULE_AUTHOR("Sean Paul <seanpaul@chromium.org>"); +MODULE_DESCRIPTION("NXP ptn3460 eDP-LVDS converter driver"); +MODULE_LICENSE("GPL");