Hi Jitao Shi,

A few comments/suggestions which I hope you'll find useful. Note that
I'm not an expert in the area so take them with a pinch of salt.

On 2 June 2016 at 10:57, Jitao Shi [off-list ref] wrote:

+#define WRITE_STATUS_REG_CMD   0x01
+#define READ_STATUS_REG_CMD    0x05
+#define BUSY                   BIT(0)
+#define CLEAR_ALL_PROTECT      0x00
+#define BLK_PROTECT_BITS       0x0c
+#define STATUS_REG_PROTECT     BIT(7)
+#define WRITE_ENABLE_CMD       0x06
+#define CHIP_ERASE_CMD         0xc7
+#define MAX_DEVS               0x8

Some of the above are unused - SPI_MAX_RETRY_CNT and PAGE2_SWSPI_CTL
come to might. Please double-check and nuke any unused ones for now ?

Add blank line between the macro definitions and the struct.

+struct ps8640_info {
+       u8 family_id;
+       u8 variant_id;
+       u16 version;
+};
+
+struct ps8640 {
+       struct drm_connector connector;
+       struct drm_bridge bridge;
+       struct edid *edid;
+       struct mipi_dsi_device dsi;
+       struct i2c_client *page[MAX_DEVS];

Throw in an enum that provides symbolic names for the different i2c
clients and rename "page" to clients or anything else that makes it
clearer ? Seems like '1' and '6' are never used.

Using better names than client2/7 in the actual code will be great :-)

+       struct i2c_client *ddc_i2c;
+       struct regulator_bulk_data supplies[2];
+       struct drm_panel *panel;
+       struct gpio_desc *gpio_rst_n;
+       struct gpio_desc *gpio_slp_n;
+       struct gpio_desc *gpio_mode_sel_n;

What does the _n suffix mean in the above three ? Bikeshed:
reset_gpio, sleep_gpio and mode_sel(ect)_gpio could also be used ;-)

+       bool enabled;
+
+       /* firmware file info */
+       struct ps8640_info info;
+       bool in_fw_update;
+       /* for firmware update protect */
+       struct mutex fw_mutex;
+};
+
+static const u8 enc_ctrl_code[6] = { 0xaa, 0x55, 0x50, 0x41, 0x52, 0x44 };

These feel a bit magical. Any chance you can give them symbolic names ?

+static const u8 hw_chip_id[4] = { 0x00, 0x0a, 0x00, 0x30 };
+
+static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
+{
+       return container_of(e, struct ps8640, bridge);
+}
+
+static inline struct ps8640 *connector_to_ps8640(struct drm_connector *e)
+{
+       return container_of(e, struct ps8640, connector);
+}
+
+static int ps8640_read(struct i2c_client *client, u8 reg, u8 *data,
+                      u16 data_len)
+{
+       int ret;
+       struct i2c_msg msgs[] = {
+               {
+                .addr = client->addr,
+                .flags = 0,
+                .len = 1,
+                .buf = &reg,
+               },
+               {
+                .addr = client->addr,
+                .flags = I2C_M_RD,
+                .len = data_len,
+                .buf = data,
+               }
+       };
+
+       ret = i2c_transfer(client->adapter, msgs, 2);
+
+       if (ret == 2)
+               return 0;
+       if (ret < 0)
+               return ret;
+       else
+               return -EIO;
+}
+
+static int ps8640_write_bytes(struct i2c_client *client, const u8 *data,
+                             u16 data_len)
+{
+       int ret;
+       struct i2c_msg msg;
+
+       msg.addr = client->addr;
+       msg.flags = 0;
+       msg.len = data_len;
+       msg.buf = (u8 *)data;
+
+       ret = i2c_transfer(client->adapter, &msg, 1);
+       if (ret == 1)
+               return 0;
+       if (ret < 0)
+               return ret;
+       else
+               return -EIO;
+}
+
+static int ps8640_write_byte(struct i2c_client *client, u8 reg,  u8 data)
+{
+       u8 buf[] = { reg, data };
+
+       return ps8640_write_bytes(client, buf, sizeof(buf));
+}
+
+static void ps8640_get_mcu_fw_version(struct ps8640 *ps_bridge)
+{
+       struct i2c_client *client = ps_bridge->page[5];
+       u8 fw_ver[2];
+
+       ps8640_read(client, 0x4, fw_ver, sizeof(fw_ver));
+       ps_bridge->info.version = (fw_ver[0] << 8) | fw_ver[1];
+
+       DRM_INFO_ONCE("ps8640 rom fw version %d.%d\n", fw_ver[0], fw_ver[1]);
+}
+
+static int ps8640_bridge_unmute(struct ps8640 *ps_bridge)
+{
+       struct i2c_client *client = ps_bridge->page[3];
+       u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_EN };
+
+       return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
+}
+
+static int ps8640_bridge_mute(struct ps8640 *ps_bridge)
+{
+       struct i2c_client *client = ps_bridge->page[3];
+       u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_DIS };
+
+       return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
+}
+
+static void ps8640_pre_enable(struct drm_bridge *bridge)
+{
+       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+       struct i2c_client *client = ps_bridge->page[2];
+       int err;
+       u8 set_vdo_done;
+       ktime_t timeout;
+
+       if (ps_bridge->in_fw_update)
+               return;
+
+       if (ps_bridge->enabled)
+               return;
+
+       err = drm_panel_prepare(ps_bridge->panel);
+       if (err < 0) {
+               DRM_ERROR("failed to prepare panel: %d\n", err);
+               return;
+       }
+
+       err = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
+                                   ps_bridge->supplies);
+       if (err < 0) {
+               DRM_ERROR("cannot enable regulators %d\n", err);
+               goto err_panel_unprepare;
+       }
+
+       gpiod_set_value(ps_bridge->gpio_slp_n, 1);
+       gpiod_set_value(ps_bridge->gpio_rst_n, 0);
+       usleep_range(2000, 2500);
+       gpiod_set_value(ps_bridge->gpio_rst_n, 1);
+
+       /*
+        * Wait for the ps8640 embed mcu ready
+        * First wait 200ms and then check the mcu ready flag every 20ms
+        */
+       msleep(200);
+

+       timeout = ktime_add_ms(ktime_get(), 200);
+       for (;;) {
+               err = ps8640_read(client, PAGE2_GPIO_H, &set_vdo_done, 1);
+               if (err < 0) {
+                       DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", err);
+                       goto err_regulators_disable;
+               }
+               if ((set_vdo_done & PS_GPIO9) == PS_GPIO9)
+                       break;
+               if (ktime_compare(ktime_get(), timeout) > 0)
+                       break;
+               msleep(20);
+       }
+

This is the first such construct in DRM. Is there anything wrong using
something like the following ? Same question applies for the rest of
the patch.


count = XX;

for (i = 0; i < count; i++) {
    err = ps8640_read(client, PAGE2_GPIO_H, &set_vdo_done, 1);
    if (err < 0) {
        DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", err);
        goto err_regulators_disable;
    }
    if ((set_vdo_done & PS_GPIO9) == PS_GPIO9)
        break;

    msleep(20);
}

if (i == count) {
    print_some_warning()
    error_out()
}

+static int ps8640_get_modes(struct drm_connector *connector)
+{

+
+       edid = devm_kmalloc(dev, sizeof(edid), GFP_KERNEL);
+       if (!edid) {
+               DRM_ERROR("Failed to allocate EDID\n");
+               return 0;

Remove this. drm_get_edid() already returns a kmalloc'd hunk of memory.

+       }
+
+       edid = drm_get_edid(connector, ps_bridge->ddc_i2c->adapter);
+       if (!edid)
+               goto out;
+

+int ps8640_bridge_attach(struct drm_bridge *bridge)
+{
+       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+       struct device *dev = &ps_bridge->page[0]->dev;
+       struct device_node *np = dev->of_node;

Kill off the temporary variable. dev->of_node is not that long and
it's used only once.

+       struct mipi_dsi_host *host = ps_bridge->dsi.host;

This looks a bit odd. Can you move it to the !dsi_node below and add a
small comment ?

+       if (dsi_node) {
+               host = of_find_mipi_dsi_host_by_node(dsi_node);
+               of_node_put(dsi_node);
+               if (!host) {
+                       ret = -ENODEV;
+                       goto err;
+               }
+       }
+

} else {
    // Use XXX if there's no dsi host provided by DT
    host = ps_bridge->dsi.host;
}

+static int ps8640_wait_rom_idle(struct ps8640 *ps_bridge)
+{
+       struct i2c_client *client = ps_bridge->page[0];
+       int ret;
+
+       ret = ps8640_write_byte(client, PAGE2_IROM_CTRL, IROM_ENABLE);
+       if (ret)
+               goto exit;
+
+       ret = ps8640_wait_spi_ready(ps_bridge);
+       if (ret)
+               goto err_spi;
+
+       ret = ps8640_wait_spi_nobusy(ps_bridge);
+       if (ret)
+               goto err_spi;
+
+       ret = ps8640_write_byte(client, PAGE2_IROM_CTRL, IROM_DISABLE);
+       if (ret)
+               goto exit;
+
+       return 0;
+
+err_spi:
+       ps8640_write_byte(client, PAGE2_IROM_CTRL, IROM_DISABLE);
+exit:
+       if (ret)

ret is always non zero here.

+               dev_err(&client->dev, "wait ps8640 rom idle fail: %d\n", ret);
+
+       return ret;
+}
+
+static int ps8640_spi_dl_mode(struct ps8640 *ps_bridge)
+{
+       struct i2c_client *client = ps_bridge->page[2];
+       int ret;
+
+       /* switch ps8640 mode to spi dl mode */
+       if (ps_bridge->gpio_mode_sel_n)
+               gpiod_set_value(ps_bridge->gpio_mode_sel_n, 0);
+
+       /* reset spi interface */
+       ret = ps8640_write_byte(client, PAGE2_SW_RESET,
+                               SPI_SW_RESET | MPU_SW_RESET);
+       if (ret)
+               goto exit;
+
+       ret = ps8640_write_byte(client, PAGE2_SW_RESET, MPU_SW_RESET);
+       if (ret)
+               goto exit;
+

Add "return 0;" ...

+exit:
+       if (ret)

... drop this check.

+               dev_err(&client->dev, "fail reset spi interface: %d\n", ret);
+
+       return ret;
+}
+
+static int ps8640_rom_prepare(struct ps8640 *ps_bridge)
+{

+       for (i = 0; i < 6; i++)

s/6/ARRAY_SIZE(enc_ctrl_code)/

+               ps8640_write_byte(client, PAGE2_ENCTLSPI_WR, enc_ctrl_code[i]);

+static int ps8640_write_rom(struct ps8640 *ps_bridge, const struct firmware *fw)
+{
+       struct i2c_client *client = ps_bridge->page[0];
+       struct device *dev = &client->dev;
+       struct i2c_client *client2 = ps_bridge->page[2];
+       struct i2c_client *client7 = ps_bridge->page[7];
+       size_t pos;
+       u8 buf[257], rom_page_id_buf[3];
+       int ret;
+       u16 cpy_len;
+
+       ps8640_write_byte(client2, PAGE2_SPI_CFG3, I2C_TO_SPI_RESET);
+       msleep(100);
+       ps8640_write_byte(client2, PAGE2_SPI_CFG3, 0x00);
+
+       for (pos = 0; pos < fw->size; pos += cpy_len) {
+               rom_page_id_buf[0] = PAGE2_ROMADD_BYTE1;
+               rom_page_id_buf[1] = pos >> 8;
+               rom_page_id_buf[2] = pos >> 16;

Reuse buf[], the stack is getting big enough already ?

+               ret = ps8640_write_bytes(client2, rom_page_id_buf, 3);
+               if (ret)
+                       goto error;
+               cpy_len = fw->size >= 256 + pos ? 256 : fw->size - pos;

cpy_len = min(256, fw->size - pos); perhaps ?

+               buf[0] = 0;
+               memcpy(buf + 1, fw->data + pos, cpy_len);
+               ret = ps8640_write_bytes(client7, buf, cpy_len + 1);
+               if (ret)
+                       goto error;
+
+               dev_dbg(dev, "fw update completed %zu / %zu bytes\n", pos,
+                       fw->size);
+       }
+       return 0;
+
+error:
+       dev_err(dev, "failed write external flash, %d\n", ret);
+       return ret;
+}
+
+static int ps8640_spi_normal_mode(struct ps8640 *ps_bridge)
+{

Is it that ps8640_spi_send_cmd()... 'cannot fail' in here, unlike in
ps8640_spi_dl_mode() and ps8640_rom_prepare() ? If so it might be
worth adding a line or two of comment and making
ps8640_spi_normal_mode() return void.

+static int ps8640_load_fw(struct ps8640 *ps_bridge, const struct firmware *fw)
+{
+       struct i2c_client *client = ps_bridge->page[0];
+       struct device *dev = &client->dev;
+       int ret;
+       bool ps8640_status_backup = ps_bridge->enabled;
+
+       ret = ps8640_validate_firmware(ps_bridge, fw);
+       if (ret)
+               return ret;
+
+       mutex_lock(&ps_bridge->fw_mutex);
+       if (!ps_bridge->in_fw_update) {
+               if (!ps8640_status_backup)
+                       ps8640_pre_enable(&ps_bridge->bridge);
+
+               ret = ps8640_enter_bl(ps_bridge);

IMHO open-coding ps8640_enter_bl/ps8640_exit_bl and having the
in_fw_update manipulation visible will make things more obvious.

+               if (ret)
+                       goto exit;
+       }
+
+       ret = ps8640_rom_prepare(ps_bridge);
+       if (ret)
+               goto exit;
+
+       ret = ps8640_write_rom(ps_bridge, fw);
+
+exit:
+       if (ret)
+               dev_err(dev, "Failed to load firmware, %d\n", ret);
+

+       ps8640_exit_bl(ps_bridge, fw);
+       if (!ps8640_status_backup)
+               ps8640_post_disable(&ps_bridge->bridge);

Why are we calling these even if we never executed
ps8640_pre_enable/ps8640_enter_bl above ?

+static ssize_t ps8640_update_fw_store(struct device *dev,
+                                     struct device_attribute *attr,
+                                     const char *buf, size_t count)
+{
+       struct i2c_client *client = to_i2c_client(dev);
+       struct ps8640 *ps_bridge = i2c_get_clientdata(client);
+       const struct firmware *fw;
+       int error;
+
+       error = request_firmware(&fw, PS_FW_NAME, dev);

Can the device operate without a firmware ? If not, why is the
firmware loaded so later/after user interaction (via sysfs) ? I don't
recall any other driver in DRM to use such an approach.


Regards,
Emil