| Message ID | 1457521038-5906-5-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Archit, Thank you for the patch. On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote: > In order to pass DSI specific parameters to the DSI host, we need the > driver to create a mipi_dsi_device DSI device that attaches to the > host. > > Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI > device using this host. Finally, attach this device to the DSI host. > > Populate DT parameters (number of data lanes for now) that are required > for DSI RX to work correctly. Hardcode few other parameters (rgb, > embedded_sync) for now. > > Signed-off-by: Archit Taneja <architt@codeaurora.org> This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the kernel won't link. As the ADV7511 doesn't require DSI, could you make it optional with conditional compilation to avoid pulling in dead code ? > --- > drivers/gpu/drm/i2c/adv7511.c | 130 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 123 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c > index 75be17c..6c2a89a 100644 > --- a/drivers/gpu/drm/i2c/adv7511.c > +++ b/drivers/gpu/drm/i2c/adv7511.c > @@ -11,6 +11,7 @@ > #include <linux/i2c.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/of_graph.h> > #include <linux/regmap.h> > #include <linux/slab.h> > > @@ -19,6 +20,7 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_crtc_helper.h> > #include <drm/drm_edid.h> > +#include <drm/drm_mipi_dsi.h> > > #include "adv7511.h" > > @@ -56,6 +58,11 @@ struct adv7511 { > > struct gpio_desc *gpio_pd; > > + /* ADV7533 DSI RX related params */ > + struct device_node *host_node; > + struct mipi_dsi_device *dsi; > + u8 num_dsi_lanes; > + > enum adv7511_type type; > }; > > @@ -392,8 +399,10 @@ static void adv7511_set_link_config(struct adv7511 > *adv7511, > > static void adv7533_dsi_power_on(struct adv7511 *adv) > { > - /* set number of dsi lanes (hardcoded to 4 for now) */ > - regmap_write(adv->regmap_cec, 0x1c, 0x4 << 4); > + struct mipi_dsi_device *dsi = adv->dsi; > + > + /* set number of dsi lanes */ > + regmap_write(adv->regmap_cec, 0x1c, dsi->lanes << 4); > /* disable internal timing generator */ > regmap_write(adv->regmap_cec, 0x27, 0x0b); > /* enable hdmi */ > @@ -889,6 +898,51 @@ static void adv7511_bridge_mode_set(struct drm_bridge > *bridge, adv7511_mode_set(adv, mode, adj_mode); > } > > +static int adv7533_attach_dsi(struct adv7511 *adv) > +{ > + struct device *dev = &adv->i2c_main->dev; > + struct mipi_dsi_host *host; > + struct mipi_dsi_device *dsi; > + int ret = 0; > + const struct mipi_dsi_device_info info = { .type = "adv7533", > + .channel = 0, > + .node = NULL, > + }; > + > + host = of_find_mipi_dsi_host_by_node(adv->host_node); > + if (!host) { > + dev_err(dev, "failed to find dsi host\n"); > + return -EPROBE_DEFER; > + } > + > + dsi = mipi_dsi_device_register_full(host, &info); > + if (IS_ERR(dsi)) { > + dev_err(dev, "failed to create dsi device\n"); > + ret = PTR_ERR(dsi); > + goto err_dsi_device; > + } > + > + adv->dsi = dsi; > + > + dsi->lanes = adv->num_dsi_lanes; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | > + MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE; > + > + ret = mipi_dsi_attach(dsi); > + if (ret < 0) { > + dev_err(dev, "failed to attach dsi to host\n"); > + goto err_dsi_attach; > + } > + > + return 0; > + > +err_dsi_attach: > + mipi_dsi_device_unregister(dsi); > +err_dsi_device: > + return ret; > +} > + > static int adv7511_bridge_attach(struct drm_bridge *bridge) > { > struct adv7511 *adv = bridge_to_adv7511(bridge); > @@ -912,6 +966,9 @@ static int adv7511_bridge_attach(struct drm_bridge > *bridge) &adv7511_connector_helper_funcs); > drm_mode_connector_attach_encoder(&adv->connector, bridge->encoder); > > + if (adv->type == ADV7533) > + ret = adv7533_attach_dsi(adv); > + > return ret; > } > > @@ -1009,6 +1066,41 @@ static int adv7511_parse_dt(struct device_node *np, > return 0; > } > > +static int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > +{ > + u32 num_lanes; > + struct device_node *endpoint; > + > + of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); > + > + if (num_lanes < 1 || num_lanes > 4) > + return -EINVAL; > + > + adv->num_dsi_lanes = num_lanes; > + > + endpoint = of_graph_get_next_endpoint(np, NULL); > + if (!endpoint) { > + DRM_ERROR("ADV7533 DSI input endpoint not found\n"); > + return -ENODEV; > + } > + > + adv->host_node = of_graph_get_remote_port_parent(endpoint); > + if (!adv->host_node) { > + DRM_ERROR("DSI host node not found\n"); > + of_node_put(endpoint); > + return -ENODEV; > + } > + > + of_node_put(endpoint); > + of_node_put(adv->host_node); > + > + /* TODO: Check if these need to be parsed by DT or not */ > + adv->rgb = true; > + adv->embedded_sync = false; > + > + return 0; > +} > + > static const int edid_i2c_addr = 0x7e; > static const int packet_i2c_addr = 0x70; > static const int cec_i2c_addr = 0x78; > @@ -1038,11 +1130,12 @@ static int adv7511_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) > > memset(&link_config, 0, sizeof(link_config)); > > - if (adv7511->type == ADV7511) { > + if (adv7511->type == ADV7511) > ret = adv7511_parse_dt(dev->of_node, &link_config); > - if (ret) > - return ret; > - } > + else > + ret = adv7533_parse_dt(dev->of_node, adv7511); > + if (ret) > + return ret; > > /* > * The power down GPIO is optional. If present, toggle it from active to > @@ -1155,6 +1248,11 @@ static int adv7511_remove(struct i2c_client *i2c) > { > struct adv7511 *adv7511 = i2c_get_clientdata(i2c); > > + if (adv7511->type == ADV7533) { > + mipi_dsi_detach(adv7511->dsi); > + mipi_dsi_device_unregister(adv7511->dsi); > + } > + > drm_bridge_remove(&adv7511->bridge); > > i2c_unregister_device(adv7511->i2c_cec); > @@ -1183,6 +1281,10 @@ static const struct of_device_id adv7511_of_ids[] = { > }; > MODULE_DEVICE_TABLE(of, adv7511_of_ids); > > +static struct mipi_dsi_driver adv7533_dsi_driver = { > + .driver.name = "adv7533", > +}; > + > static struct i2c_driver adv7511_driver = { > .driver = { > .name = "adv7511", > @@ -1193,7 +1295,21 @@ static struct i2c_driver adv7511_driver = { > .remove = adv7511_remove, > }; > > -module_i2c_driver(adv7511_driver); > +static int __init adv7511_init(void) > +{ > + mipi_dsi_driver_register(&adv7533_dsi_driver); > + > + return i2c_add_driver(&adv7511_driver); > +} > +module_init(adv7511_init); > + > +static void __exit adv7511_exit(void) > +{ > + i2c_del_driver(&adv7511_driver); > + > + mipi_dsi_driver_unregister(&adv7533_dsi_driver); > +} > +module_exit(adv7511_exit); > > MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>"); > MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver");
Hi Laurent, On 04/22/2016 03:59 AM, Laurent Pinchart wrote: > Hi Archit, > > Thank you for the patch. > > On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote: >> In order to pass DSI specific parameters to the DSI host, we need the >> driver to create a mipi_dsi_device DSI device that attaches to the >> host. >> >> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI >> device using this host. Finally, attach this device to the DSI host. >> >> Populate DT parameters (number of data lanes for now) that are required >> for DSI RX to work correctly. Hardcode few other parameters (rgb, >> embedded_sync) for now. >> >> Signed-off-by: Archit Taneja <architt@codeaurora.org> > > This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the kernel won't > link. As the ADV7511 doesn't require DSI, could you make it optional with > conditional compilation to avoid pulling in dead code ? You're right. The driver's Kconfig should at least select DRM_MIPI_DSI in the current state to make sure we don't break build. Do you suggest we create another config option for ADV7533, which selects DRM_MIPI_DSI and builds the ADV7533 parts? Or did you mean something else? Thanks for the review. Archit > >> --- >> drivers/gpu/drm/i2c/adv7511.c | 130 ++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 123 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c >> index 75be17c..6c2a89a 100644 >> --- a/drivers/gpu/drm/i2c/adv7511.c >> +++ b/drivers/gpu/drm/i2c/adv7511.c >> @@ -11,6 +11,7 @@ >> #include <linux/i2c.h> >> #include <linux/module.h> >> #include <linux/of_device.h> >> +#include <linux/of_graph.h> >> #include <linux/regmap.h> >> #include <linux/slab.h> >> >> @@ -19,6 +20,7 @@ >> #include <drm/drm_atomic_helper.h> >> #include <drm/drm_crtc_helper.h> >> #include <drm/drm_edid.h> >> +#include <drm/drm_mipi_dsi.h> >> >> #include "adv7511.h" >> >> @@ -56,6 +58,11 @@ struct adv7511 { >> >> struct gpio_desc *gpio_pd; >> >> + /* ADV7533 DSI RX related params */ >> + struct device_node *host_node; >> + struct mipi_dsi_device *dsi; >> + u8 num_dsi_lanes; >> + >> enum adv7511_type type; >> }; >> >> @@ -392,8 +399,10 @@ static void adv7511_set_link_config(struct adv7511 >> *adv7511, >> >> static void adv7533_dsi_power_on(struct adv7511 *adv) >> { >> - /* set number of dsi lanes (hardcoded to 4 for now) */ >> - regmap_write(adv->regmap_cec, 0x1c, 0x4 << 4); >> + struct mipi_dsi_device *dsi = adv->dsi; >> + >> + /* set number of dsi lanes */ >> + regmap_write(adv->regmap_cec, 0x1c, dsi->lanes << 4); >> /* disable internal timing generator */ >> regmap_write(adv->regmap_cec, 0x27, 0x0b); >> /* enable hdmi */ >> @@ -889,6 +898,51 @@ static void adv7511_bridge_mode_set(struct drm_bridge >> *bridge, adv7511_mode_set(adv, mode, adj_mode); >> } >> >> +static int adv7533_attach_dsi(struct adv7511 *adv) >> +{ >> + struct device *dev = &adv->i2c_main->dev; >> + struct mipi_dsi_host *host; >> + struct mipi_dsi_device *dsi; >> + int ret = 0; >> + const struct mipi_dsi_device_info info = { .type = "adv7533", >> + .channel = 0, >> + .node = NULL, >> + }; >> + >> + host = of_find_mipi_dsi_host_by_node(adv->host_node); >> + if (!host) { >> + dev_err(dev, "failed to find dsi host\n"); >> + return -EPROBE_DEFER; >> + } >> + >> + dsi = mipi_dsi_device_register_full(host, &info); >> + if (IS_ERR(dsi)) { >> + dev_err(dev, "failed to create dsi device\n"); >> + ret = PTR_ERR(dsi); >> + goto err_dsi_device; >> + } >> + >> + adv->dsi = dsi; >> + >> + dsi->lanes = adv->num_dsi_lanes; >> + dsi->format = MIPI_DSI_FMT_RGB888; >> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | >> + MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE; >> + >> + ret = mipi_dsi_attach(dsi); >> + if (ret < 0) { >> + dev_err(dev, "failed to attach dsi to host\n"); >> + goto err_dsi_attach; >> + } >> + >> + return 0; >> + >> +err_dsi_attach: >> + mipi_dsi_device_unregister(dsi); >> +err_dsi_device: >> + return ret; >> +} >> + >> static int adv7511_bridge_attach(struct drm_bridge *bridge) >> { >> struct adv7511 *adv = bridge_to_adv7511(bridge); >> @@ -912,6 +966,9 @@ static int adv7511_bridge_attach(struct drm_bridge >> *bridge) &adv7511_connector_helper_funcs); >> drm_mode_connector_attach_encoder(&adv->connector, bridge->encoder); >> >> + if (adv->type == ADV7533) >> + ret = adv7533_attach_dsi(adv); >> + >> return ret; >> } >> >> @@ -1009,6 +1066,41 @@ static int adv7511_parse_dt(struct device_node *np, >> return 0; >> } >> >> +static int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) >> +{ >> + u32 num_lanes; >> + struct device_node *endpoint; >> + >> + of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); >> + >> + if (num_lanes < 1 || num_lanes > 4) >> + return -EINVAL; >> + >> + adv->num_dsi_lanes = num_lanes; >> + >> + endpoint = of_graph_get_next_endpoint(np, NULL); >> + if (!endpoint) { >> + DRM_ERROR("ADV7533 DSI input endpoint not found\n"); >> + return -ENODEV; >> + } >> + >> + adv->host_node = of_graph_get_remote_port_parent(endpoint); >> + if (!adv->host_node) { >> + DRM_ERROR("DSI host node not found\n"); >> + of_node_put(endpoint); >> + return -ENODEV; >> + } >> + >> + of_node_put(endpoint); >> + of_node_put(adv->host_node); >> + >> + /* TODO: Check if these need to be parsed by DT or not */ >> + adv->rgb = true; >> + adv->embedded_sync = false; >> + >> + return 0; >> +} >> + >> static const int edid_i2c_addr = 0x7e; >> static const int packet_i2c_addr = 0x70; >> static const int cec_i2c_addr = 0x78; >> @@ -1038,11 +1130,12 @@ static int adv7511_probe(struct i2c_client *i2c, >> const struct i2c_device_id *id) >> >> memset(&link_config, 0, sizeof(link_config)); >> >> - if (adv7511->type == ADV7511) { >> + if (adv7511->type == ADV7511) >> ret = adv7511_parse_dt(dev->of_node, &link_config); >> - if (ret) >> - return ret; >> - } >> + else >> + ret = adv7533_parse_dt(dev->of_node, adv7511); >> + if (ret) >> + return ret; >> >> /* >> * The power down GPIO is optional. If present, toggle it from active to >> @@ -1155,6 +1248,11 @@ static int adv7511_remove(struct i2c_client *i2c) >> { >> struct adv7511 *adv7511 = i2c_get_clientdata(i2c); >> >> + if (adv7511->type == ADV7533) { >> + mipi_dsi_detach(adv7511->dsi); >> + mipi_dsi_device_unregister(adv7511->dsi); >> + } >> + >> drm_bridge_remove(&adv7511->bridge); >> >> i2c_unregister_device(adv7511->i2c_cec); >> @@ -1183,6 +1281,10 @@ static const struct of_device_id adv7511_of_ids[] = { >> }; >> MODULE_DEVICE_TABLE(of, adv7511_of_ids); >> >> +static struct mipi_dsi_driver adv7533_dsi_driver = { >> + .driver.name = "adv7533", >> +}; >> + >> static struct i2c_driver adv7511_driver = { >> .driver = { >> .name = "adv7511", >> @@ -1193,7 +1295,21 @@ static struct i2c_driver adv7511_driver = { >> .remove = adv7511_remove, >> }; >> >> -module_i2c_driver(adv7511_driver); >> +static int __init adv7511_init(void) >> +{ >> + mipi_dsi_driver_register(&adv7533_dsi_driver); >> + >> + return i2c_add_driver(&adv7511_driver); >> +} >> +module_init(adv7511_init); >> + >> +static void __exit adv7511_exit(void) >> +{ >> + i2c_del_driver(&adv7511_driver); >> + >> + mipi_dsi_driver_unregister(&adv7533_dsi_driver); >> +} >> +module_exit(adv7511_exit); >> >> MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>"); >> MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver"); >
Hi Laurent, On 04/22/2016 10:40 AM, Archit Taneja wrote: > Hi Laurent, > > > On 04/22/2016 03:59 AM, Laurent Pinchart wrote: >> Hi Archit, >> >> Thank you for the patch. >> >> On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote: >>> In order to pass DSI specific parameters to the DSI host, we need the >>> driver to create a mipi_dsi_device DSI device that attaches to the >>> host. >>> >>> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI >>> device using this host. Finally, attach this device to the DSI host. >>> >>> Populate DT parameters (number of data lanes for now) that are required >>> for DSI RX to work correctly. Hardcode few other parameters (rgb, >>> embedded_sync) for now. >>> >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >> >> This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the >> kernel won't >> link. As the ADV7511 doesn't require DSI, could you make it optional with >> conditional compilation to avoid pulling in dead code ? > > You're right. The driver's Kconfig should at least select DRM_MIPI_DSI > in the current state to make sure we don't break build. > > Do you suggest we create another config option for ADV7533, which > selects DRM_MIPI_DSI and builds the ADV7533 parts? Or did you mean > something else? Do you have any suggestions for this point? For the next revision, I've just selected DRM_MIPI_DSI in the Kconfig to ensure build doesn't break. For this driver, to conditionally compile DRM_MIPI_DSI, it essentially means we allow conditional support for ADV7533. I would imagine us #ifdef-ing out the ADV7533 compatible string in the of_match_table. Is this something we want to do? Thanks, Archit > >> >>> --- >>> drivers/gpu/drm/i2c/adv7511.c | 130 >>> ++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 123 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i2c/adv7511.c >>> b/drivers/gpu/drm/i2c/adv7511.c >>> index 75be17c..6c2a89a 100644 >>> --- a/drivers/gpu/drm/i2c/adv7511.c >>> +++ b/drivers/gpu/drm/i2c/adv7511.c >>> @@ -11,6 +11,7 @@ >>> #include <linux/i2c.h> >>> #include <linux/module.h> >>> #include <linux/of_device.h> >>> +#include <linux/of_graph.h> >>> #include <linux/regmap.h> >>> #include <linux/slab.h> >>> >>> @@ -19,6 +20,7 @@ >>> #include <drm/drm_atomic_helper.h> >>> #include <drm/drm_crtc_helper.h> >>> #include <drm/drm_edid.h> >>> +#include <drm/drm_mipi_dsi.h> >>> >>> #include "adv7511.h" >>> >>> @@ -56,6 +58,11 @@ struct adv7511 { >>> >>> struct gpio_desc *gpio_pd; >>> >>> + /* ADV7533 DSI RX related params */ >>> + struct device_node *host_node; >>> + struct mipi_dsi_device *dsi; >>> + u8 num_dsi_lanes; >>> + >>> enum adv7511_type type; >>> }; >>> >>> @@ -392,8 +399,10 @@ static void adv7511_set_link_config(struct adv7511 >>> *adv7511, >>> >>> static void adv7533_dsi_power_on(struct adv7511 *adv) >>> { >>> - /* set number of dsi lanes (hardcoded to 4 for now) */ >>> - regmap_write(adv->regmap_cec, 0x1c, 0x4 << 4); >>> + struct mipi_dsi_device *dsi = adv->dsi; >>> + >>> + /* set number of dsi lanes */ >>> + regmap_write(adv->regmap_cec, 0x1c, dsi->lanes << 4); >>> /* disable internal timing generator */ >>> regmap_write(adv->regmap_cec, 0x27, 0x0b); >>> /* enable hdmi */ >>> @@ -889,6 +898,51 @@ static void adv7511_bridge_mode_set(struct >>> drm_bridge >>> *bridge, adv7511_mode_set(adv, mode, adj_mode); >>> } >>> >>> +static int adv7533_attach_dsi(struct adv7511 *adv) >>> +{ >>> + struct device *dev = &adv->i2c_main->dev; >>> + struct mipi_dsi_host *host; >>> + struct mipi_dsi_device *dsi; >>> + int ret = 0; >>> + const struct mipi_dsi_device_info info = { .type = "adv7533", >>> + .channel = 0, >>> + .node = NULL, >>> + }; >>> + >>> + host = of_find_mipi_dsi_host_by_node(adv->host_node); >>> + if (!host) { >>> + dev_err(dev, "failed to find dsi host\n"); >>> + return -EPROBE_DEFER; >>> + } >>> + >>> + dsi = mipi_dsi_device_register_full(host, &info); >>> + if (IS_ERR(dsi)) { >>> + dev_err(dev, "failed to create dsi device\n"); >>> + ret = PTR_ERR(dsi); >>> + goto err_dsi_device; >>> + } >>> + >>> + adv->dsi = dsi; >>> + >>> + dsi->lanes = adv->num_dsi_lanes; >>> + dsi->format = MIPI_DSI_FMT_RGB888; >>> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>> MIPI_DSI_MODE_VIDEO_SYNC_PULSE | >>> + MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE; >>> + >>> + ret = mipi_dsi_attach(dsi); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to attach dsi to host\n"); >>> + goto err_dsi_attach; >>> + } >>> + >>> + return 0; >>> + >>> +err_dsi_attach: >>> + mipi_dsi_device_unregister(dsi); >>> +err_dsi_device: >>> + return ret; >>> +} >>> + >>> static int adv7511_bridge_attach(struct drm_bridge *bridge) >>> { >>> struct adv7511 *adv = bridge_to_adv7511(bridge); >>> @@ -912,6 +966,9 @@ static int adv7511_bridge_attach(struct drm_bridge >>> *bridge) &adv7511_connector_helper_funcs); >>> drm_mode_connector_attach_encoder(&adv->connector, >>> bridge->encoder); >>> >>> + if (adv->type == ADV7533) >>> + ret = adv7533_attach_dsi(adv); >>> + >>> return ret; >>> } >>> >>> @@ -1009,6 +1066,41 @@ static int adv7511_parse_dt(struct device_node >>> *np, >>> return 0; >>> } >>> >>> +static int adv7533_parse_dt(struct device_node *np, struct adv7511 >>> *adv) >>> +{ >>> + u32 num_lanes; >>> + struct device_node *endpoint; >>> + >>> + of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); >>> + >>> + if (num_lanes < 1 || num_lanes > 4) >>> + return -EINVAL; >>> + >>> + adv->num_dsi_lanes = num_lanes; >>> + >>> + endpoint = of_graph_get_next_endpoint(np, NULL); >>> + if (!endpoint) { >>> + DRM_ERROR("ADV7533 DSI input endpoint not found\n"); >>> + return -ENODEV; >>> + } >>> + >>> + adv->host_node = of_graph_get_remote_port_parent(endpoint); >>> + if (!adv->host_node) { >>> + DRM_ERROR("DSI host node not found\n"); >>> + of_node_put(endpoint); >>> + return -ENODEV; >>> + } >>> + >>> + of_node_put(endpoint); >>> + of_node_put(adv->host_node); >>> + >>> + /* TODO: Check if these need to be parsed by DT or not */ >>> + adv->rgb = true; >>> + adv->embedded_sync = false; >>> + >>> + return 0; >>> +} >>> + >>> static const int edid_i2c_addr = 0x7e; >>> static const int packet_i2c_addr = 0x70; >>> static const int cec_i2c_addr = 0x78; >>> @@ -1038,11 +1130,12 @@ static int adv7511_probe(struct i2c_client *i2c, >>> const struct i2c_device_id *id) >>> >>> memset(&link_config, 0, sizeof(link_config)); >>> >>> - if (adv7511->type == ADV7511) { >>> + if (adv7511->type == ADV7511) >>> ret = adv7511_parse_dt(dev->of_node, &link_config); >>> - if (ret) >>> - return ret; >>> - } >>> + else >>> + ret = adv7533_parse_dt(dev->of_node, adv7511); >>> + if (ret) >>> + return ret; >>> >>> /* >>> * The power down GPIO is optional. If present, toggle it from >>> active to >>> @@ -1155,6 +1248,11 @@ static int adv7511_remove(struct i2c_client *i2c) >>> { >>> struct adv7511 *adv7511 = i2c_get_clientdata(i2c); >>> >>> + if (adv7511->type == ADV7533) { >>> + mipi_dsi_detach(adv7511->dsi); >>> + mipi_dsi_device_unregister(adv7511->dsi); >>> + } >>> + >>> drm_bridge_remove(&adv7511->bridge); >>> >>> i2c_unregister_device(adv7511->i2c_cec); >>> @@ -1183,6 +1281,10 @@ static const struct of_device_id >>> adv7511_of_ids[] = { >>> }; >>> MODULE_DEVICE_TABLE(of, adv7511_of_ids); >>> >>> +static struct mipi_dsi_driver adv7533_dsi_driver = { >>> + .driver.name = "adv7533", >>> +}; >>> + >>> static struct i2c_driver adv7511_driver = { >>> .driver = { >>> .name = "adv7511", >>> @@ -1193,7 +1295,21 @@ static struct i2c_driver adv7511_driver = { >>> .remove = adv7511_remove, >>> }; >>> >>> -module_i2c_driver(adv7511_driver); >>> +static int __init adv7511_init(void) >>> +{ >>> + mipi_dsi_driver_register(&adv7533_dsi_driver); >>> + >>> + return i2c_add_driver(&adv7511_driver); >>> +} >>> +module_init(adv7511_init); >>> + >>> +static void __exit adv7511_exit(void) >>> +{ >>> + i2c_del_driver(&adv7511_driver); >>> + >>> + mipi_dsi_driver_unregister(&adv7533_dsi_driver); >>> +} >>> +module_exit(adv7511_exit); >>> >>> MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>"); >>> MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver"); >> >
Hi Archit, On Tuesday 03 May 2016 12:27:38 Archit Taneja wrote: > On 04/22/2016 10:40 AM, Archit Taneja wrote: > > On 04/22/2016 03:59 AM, Laurent Pinchart wrote: > >> On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote: > >>> In order to pass DSI specific parameters to the DSI host, we need the > >>> driver to create a mipi_dsi_device DSI device that attaches to the > >>> host. > >>> > >>> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI > >>> device using this host. Finally, attach this device to the DSI host. > >>> > >>> Populate DT parameters (number of data lanes for now) that are required > >>> for DSI RX to work correctly. Hardcode few other parameters (rgb, > >>> embedded_sync) for now. > >>> > >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> > >> > >> This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the > >> kernel won't link. As the ADV7511 doesn't require DSI, could you make it > >> optional with conditional compilation to avoid pulling in dead code ? > > > > You're right. The driver's Kconfig should at least select DRM_MIPI_DSI > > in the current state to make sure we don't break build. > > > > Do you suggest we create another config option for ADV7533, which > > selects DRM_MIPI_DSI and builds the ADV7533 parts? Or did you mean > > something else? > > Do you have any suggestions for this point? For the next revision, I've > just selected DRM_MIPI_DSI in the Kconfig to ensure build doesn't break. > > For this driver, to conditionally compile DRM_MIPI_DSI, it essentially > means we allow conditional support for ADV7533. I would imagine us > #ifdef-ing out the ADV7533 compatible string in the of_match_table. Is > this something we want to do? If it's not much trouble I think that would be useful to avoid bloating the kernel with unused features, yes. You might want to add stub functions to include/drm/drm_mipi_dsi.h when CONFIG_DRM_MIPI_DSI is not defined to avoid sprinkling the driver with lots of #ifdefs. > >>> --- > >>> > >>> drivers/gpu/drm/i2c/adv7511.c | 130 +++++++++++++++++++++++++++++++++--- > >>> 1 file changed, 123 insertions(+), 7 deletions(-)
On 05/10/2016 02:08 AM, Laurent Pinchart wrote: > Hi Archit, > > On Tuesday 03 May 2016 12:27:38 Archit Taneja wrote: >> On 04/22/2016 10:40 AM, Archit Taneja wrote: >>> On 04/22/2016 03:59 AM, Laurent Pinchart wrote: >>>> On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote: >>>>> In order to pass DSI specific parameters to the DSI host, we need the >>>>> driver to create a mipi_dsi_device DSI device that attaches to the >>>>> host. >>>>> >>>>> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI >>>>> device using this host. Finally, attach this device to the DSI host. >>>>> >>>>> Populate DT parameters (number of data lanes for now) that are required >>>>> for DSI RX to work correctly. Hardcode few other parameters (rgb, >>>>> embedded_sync) for now. >>>>> >>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>> >>>> This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the >>>> kernel won't link. As the ADV7511 doesn't require DSI, could you make it >>>> optional with conditional compilation to avoid pulling in dead code ? >>> >>> You're right. The driver's Kconfig should at least select DRM_MIPI_DSI >>> in the current state to make sure we don't break build. >>> >>> Do you suggest we create another config option for ADV7533, which >>> selects DRM_MIPI_DSI and builds the ADV7533 parts? Or did you mean >>> something else? >> >> Do you have any suggestions for this point? For the next revision, I've >> just selected DRM_MIPI_DSI in the Kconfig to ensure build doesn't break. >> >> For this driver, to conditionally compile DRM_MIPI_DSI, it essentially >> means we allow conditional support for ADV7533. I would imagine us >> #ifdef-ing out the ADV7533 compatible string in the of_match_table. Is >> this something we want to do? > > If it's not much trouble I think that would be useful to avoid bloating the > kernel with unused features, yes. You might want to add stub functions to > include/drm/drm_mipi_dsi.h when CONFIG_DRM_MIPI_DSI is not defined to avoid > sprinkling the driver with lots of #ifdefs. Yeah, I can do this. Now that there isn't a chance for it to get in the 4.7 merge window, there's plenty time. Archit > >>>>> --- >>>>> >>>>> drivers/gpu/drm/i2c/adv7511.c | 130 +++++++++++++++++++++++++++++++++--- >>>>> 1 file changed, 123 insertions(+), 7 deletions(-) >
diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c index 75be17c..6c2a89a 100644 --- a/drivers/gpu/drm/i2c/adv7511.c +++ b/drivers/gpu/drm/i2c/adv7511.c @@ -11,6 +11,7 @@ #include <linux/i2c.h> #include <linux/module.h> #include <linux/of_device.h> +#include <linux/of_graph.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -19,6 +20,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_edid.h> +#include <drm/drm_mipi_dsi.h> #include "adv7511.h" @@ -56,6 +58,11 @@ struct adv7511 { struct gpio_desc *gpio_pd; + /* ADV7533 DSI RX related params */ + struct device_node *host_node; + struct mipi_dsi_device *dsi; + u8 num_dsi_lanes; + enum adv7511_type type; }; @@ -392,8 +399,10 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, static void adv7533_dsi_power_on(struct adv7511 *adv) { - /* set number of dsi lanes (hardcoded to 4 for now) */ - regmap_write(adv->regmap_cec, 0x1c, 0x4 << 4); + struct mipi_dsi_device *dsi = adv->dsi; + + /* set number of dsi lanes */ + regmap_write(adv->regmap_cec, 0x1c, dsi->lanes << 4); /* disable internal timing generator */ regmap_write(adv->regmap_cec, 0x27, 0x0b); /* enable hdmi */ @@ -889,6 +898,51 @@ static void adv7511_bridge_mode_set(struct drm_bridge *bridge, adv7511_mode_set(adv, mode, adj_mode); } +static int adv7533_attach_dsi(struct adv7511 *adv) +{ + struct device *dev = &adv->i2c_main->dev; + struct mipi_dsi_host *host; + struct mipi_dsi_device *dsi; + int ret = 0; + const struct mipi_dsi_device_info info = { .type = "adv7533", + .channel = 0, + .node = NULL, + }; + + host = of_find_mipi_dsi_host_by_node(adv->host_node); + if (!host) { + dev_err(dev, "failed to find dsi host\n"); + return -EPROBE_DEFER; + } + + dsi = mipi_dsi_device_register_full(host, &info); + if (IS_ERR(dsi)) { + dev_err(dev, "failed to create dsi device\n"); + ret = PTR_ERR(dsi); + goto err_dsi_device; + } + + adv->dsi = dsi; + + dsi->lanes = adv->num_dsi_lanes; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | + MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE; + + ret = mipi_dsi_attach(dsi); + if (ret < 0) { + dev_err(dev, "failed to attach dsi to host\n"); + goto err_dsi_attach; + } + + return 0; + +err_dsi_attach: + mipi_dsi_device_unregister(dsi); +err_dsi_device: + return ret; +} + static int adv7511_bridge_attach(struct drm_bridge *bridge) { struct adv7511 *adv = bridge_to_adv7511(bridge); @@ -912,6 +966,9 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) &adv7511_connector_helper_funcs); drm_mode_connector_attach_encoder(&adv->connector, bridge->encoder); + if (adv->type == ADV7533) + ret = adv7533_attach_dsi(adv); + return ret; } @@ -1009,6 +1066,41 @@ static int adv7511_parse_dt(struct device_node *np, return 0; } +static int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) +{ + u32 num_lanes; + struct device_node *endpoint; + + of_property_read_u32(np, "adi,dsi-lanes", &num_lanes); + + if (num_lanes < 1 || num_lanes > 4) + return -EINVAL; + + adv->num_dsi_lanes = num_lanes; + + endpoint = of_graph_get_next_endpoint(np, NULL); + if (!endpoint) { + DRM_ERROR("ADV7533 DSI input endpoint not found\n"); + return -ENODEV; + } + + adv->host_node = of_graph_get_remote_port_parent(endpoint); + if (!adv->host_node) { + DRM_ERROR("DSI host node not found\n"); + of_node_put(endpoint); + return -ENODEV; + } + + of_node_put(endpoint); + of_node_put(adv->host_node); + + /* TODO: Check if these need to be parsed by DT or not */ + adv->rgb = true; + adv->embedded_sync = false; + + return 0; +} + static const int edid_i2c_addr = 0x7e; static const int packet_i2c_addr = 0x70; static const int cec_i2c_addr = 0x78; @@ -1038,11 +1130,12 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) memset(&link_config, 0, sizeof(link_config)); - if (adv7511->type == ADV7511) { + if (adv7511->type == ADV7511) ret = adv7511_parse_dt(dev->of_node, &link_config); - if (ret) - return ret; - } + else + ret = adv7533_parse_dt(dev->of_node, adv7511); + if (ret) + return ret; /* * The power down GPIO is optional. If present, toggle it from active to @@ -1155,6 +1248,11 @@ static int adv7511_remove(struct i2c_client *i2c) { struct adv7511 *adv7511 = i2c_get_clientdata(i2c); + if (adv7511->type == ADV7533) { + mipi_dsi_detach(adv7511->dsi); + mipi_dsi_device_unregister(adv7511->dsi); + } + drm_bridge_remove(&adv7511->bridge); i2c_unregister_device(adv7511->i2c_cec); @@ -1183,6 +1281,10 @@ static const struct of_device_id adv7511_of_ids[] = { }; MODULE_DEVICE_TABLE(of, adv7511_of_ids); +static struct mipi_dsi_driver adv7533_dsi_driver = { + .driver.name = "adv7533", +}; + static struct i2c_driver adv7511_driver = { .driver = { .name = "adv7511", @@ -1193,7 +1295,21 @@ static struct i2c_driver adv7511_driver = { .remove = adv7511_remove, }; -module_i2c_driver(adv7511_driver); +static int __init adv7511_init(void) +{ + mipi_dsi_driver_register(&adv7533_dsi_driver); + + return i2c_add_driver(&adv7511_driver); +} +module_init(adv7511_init); + +static void __exit adv7511_exit(void) +{ + i2c_del_driver(&adv7511_driver); + + mipi_dsi_driver_unregister(&adv7533_dsi_driver); +} +module_exit(adv7511_exit); MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>"); MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver");
In order to pass DSI specific parameters to the DSI host, we need the driver to create a mipi_dsi_device DSI device that attaches to the host. Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI device using this host. Finally, attach this device to the DSI host. Populate DT parameters (number of data lanes for now) that are required for DSI RX to work correctly. Hardcode few other parameters (rgb, embedded_sync) for now. Signed-off-by: Archit Taneja <architt@codeaurora.org> --- drivers/gpu/drm/i2c/adv7511.c | 130 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 123 insertions(+), 7 deletions(-)