Re: [PATCH v3 4/7] drm/i2c: adv7511: Create a MIPI DSI device

[Archit Taneja <architt@codeaurora.org>]

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");
>>
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation