Re: [PATCH v8 1/4] drm/bridge: add support for sn65dsi86 bridge driver

spanda-sgV2jX0FEOL9JmXXK+q4OQ
2018-06-13T11:06:51Z

On 2018-06-12 05:05, Stephen Boyd wrote:
> Quoting Sandeep Panda (2018-06-04 22:40:15)
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> new file mode 100644
>> index 0000000..add6e0f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -0,0 +1,666 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
> [...]
>> +
>> +static const struct regmap_config ti_sn_bridge_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .volatile_table = &ti_sn_bridge_volatile_table,
>> + .cache_type = REGCACHE_NONE,
>> +};
>> +
>> +static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>> + unsigned int reg, u16 val)
>> +{
>> + regmap_write(pdata->regmap, reg, val & 0xFF);
>> + regmap_write(pdata->regmap, reg + 1, val >> 8);
>> +}
>> +
>> +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
>> +{
>> + struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
>> + int ret = 0;
>
> Please don't assign variables and then reassign them again immediately
> after. It hides use before real initialization bugs.
>
Ok.

>> +
>> + ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM,
>> pdata->supplies);
> [...]
>> +
>> +static int ti_sn_bridge_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct ti_sn_bridge *pdata;
>> + struct device_node *ddc_node;
>> + struct mipi_dsi_host *host;
>> + struct mipi_dsi_device *dsi;
>> + int ret = 0;
>> + const struct mipi_dsi_device_info info = { .type =
>> "ti_sn_bridge",
>> + .channel = 0,
>> + .node = NULL,
>> + };
>> +
>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> + DRM_ERROR("device doesn't support I2C\n");
>> + return -ENODEV;
>> + }
>> +
>> + ret = ti_sn_bridge_parse_dsi_host(pdata);
>> + if (ret)
>> + return ret;
>> +
>> + host = of_find_mipi_dsi_host_by_node(pdata->host_node);
>> + if (!host) {
>> + DRM_ERROR("failed to find dsi host\n");
>
> Not sure we want to print an error and then return -EPROBE_DEFER.
> Usually EPROBE_DEFER is silent.
>
>> + ret = -EPROBE_DEFER;
>> + goto err_dsi_host;
>> + }
>> +
> [...]
>> +
>> + /* TODO: setting to 4 lanes always for now */
>> + dsi->lanes = 4;
>> + 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) {
>> + DRM_ERROR("failed to attach dsi to host\n");
>> + goto err_dsi_attach;
>> + }
>> + pdata->dsi = dsi;
>> +
>> + pdata->refclk = devm_clk_get(pdata->dev, "refclk");
>
> We need to check for error
>
> if (IS_ERR(pdata->refclk))
>
> And then if it's EPROBE_DEFER I suppose we would bail out, otherwise
> assume it's not present?

Yes found this issue while testing the driver on actual sn65dsi86 HW, i
will fix this in next patchset.