On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> 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@xxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("NXP ptn3460 eDP-LVDS converter driver");
>> +MODULE_LICENSE("GPL");
>
> This should be "GPL v2".
Ok. Will fix it.
Ajay
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
