From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754265AbcFPTOO (ORCPT ); Thu, 16 Jun 2016 15:14:14 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34849 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753999AbcFPTOL (ORCPT ); Thu, 16 Jun 2016 15:14:11 -0400 MIME-Version: 1.0 In-Reply-To: <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> From: Emil Velikov Date: Thu, 16 Jun 2016 20:14:07 +0100 Message-ID: Subject: Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge To: Jitao Shi Cc: David Airlie , Thierry Reding , Matthias Brugger , Mark Rutland , stonea168@163.com, ML dri-devel , Andy Yan , Ajay Kumar , Vincent Palatin , cawa cheng , Russell King , devicetree , Pawel Moll , Ian Campbell , Rob Herring , linux-mediatek@lists.infradead.org, Yingjoe Chen , eddie.huang@mediatek.com, LAKML , Rahul Sharma , srv_heupstream@mediatek.com, "Linux-Kernel@Vger. Kernel. Org" , Sascha Hauer , Kumar Gala Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 = ®, > + }, > + { > + .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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Emil Velikov Subject: Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge Date: Thu, 16 Jun 2016 20:14:07 +0100 Message-ID: References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jitao Shi Cc: Mark Rutland , stonea168@163.com, ML dri-devel , Ajay Kumar , Vincent Palatin , cawa cheng , Yingjoe Chen , Thierry Reding , devicetree , Pawel Moll , Ian Campbell , Rob Herring , linux-mediatek@lists.infradead.org, Russell King , Matthias Brugger , eddie.huang@mediatek.com, LAKML , Rahul Sharma , srv_heupstream@mediatek.com, "Linux-Kernel@Vger. Kernel. Org" , Sascha Hauer , Kumar Gala , Andy Yan List-Id: devicetree@vger.kernel.org SGkgSml0YW8gU2hpLAoKQSBmZXcgY29tbWVudHMvc3VnZ2VzdGlvbnMgd2hpY2ggSSBob3BlIHlv dSdsbCBmaW5kIHVzZWZ1bC4gTm90ZSB0aGF0CkknbSBub3QgYW4gZXhwZXJ0IGluIHRoZSBhcmVh IHNvIHRha2UgdGhlbSB3aXRoIGEgcGluY2ggb2Ygc2FsdC4KCk9uIDIgSnVuZSAyMDE2IGF0IDEw OjU3LCBKaXRhbyBTaGkgPGppdGFvLnNoaUBtZWRpYXRlay5jb20+IHdyb3RlOgoKPiArI2RlZmlu ZSBXUklURV9TVEFUVVNfUkVHX0NNRCAgIDB4MDEKPiArI2RlZmluZSBSRUFEX1NUQVRVU19SRUdf Q01EICAgIDB4MDUKPiArI2RlZmluZSBCVVNZICAgICAgICAgICAgICAgICAgIEJJVCgwKQo+ICsj ZGVmaW5lIENMRUFSX0FMTF9QUk9URUNUICAgICAgMHgwMAo+ICsjZGVmaW5lIEJMS19QUk9URUNU X0JJVFMgICAgICAgMHgwYwo+ICsjZGVmaW5lIFNUQVRVU19SRUdfUFJPVEVDVCAgICAgQklUKDcp Cj4gKyNkZWZpbmUgV1JJVEVfRU5BQkxFX0NNRCAgICAgICAweDA2Cj4gKyNkZWZpbmUgQ0hJUF9F UkFTRV9DTUQgICAgICAgICAweGM3Cj4gKyNkZWZpbmUgTUFYX0RFVlMgICAgICAgICAgICAgICAw eDgKU29tZSBvZiB0aGUgYWJvdmUgYXJlIHVudXNlZCAtIFNQSV9NQVhfUkVUUllfQ05UIGFuZCBQ QUdFMl9TV1NQSV9DVEwKY29tZSB0byBtaWdodC4gUGxlYXNlIGRvdWJsZS1jaGVjayBhbmQgbnVr ZSBhbnkgdW51c2VkIG9uZXMgZm9yIG5vdyA/CgpBZGQgYmxhbmsgbGluZSBiZXR3ZWVuIHRoZSBt YWNybyBkZWZpbml0aW9ucyBhbmQgdGhlIHN0cnVjdC4KCj4gK3N0cnVjdCBwczg2NDBfaW5mbyB7 Cj4gKyAgICAgICB1OCBmYW1pbHlfaWQ7Cj4gKyAgICAgICB1OCB2YXJpYW50X2lkOwo+ICsgICAg ICAgdTE2IHZlcnNpb247Cj4gK307Cj4gKwo+ICtzdHJ1Y3QgcHM4NjQwIHsKPiArICAgICAgIHN0 cnVjdCBkcm1fY29ubmVjdG9yIGNvbm5lY3RvcjsKPiArICAgICAgIHN0cnVjdCBkcm1fYnJpZGdl IGJyaWRnZTsKPiArICAgICAgIHN0cnVjdCBlZGlkICplZGlkOwo+ICsgICAgICAgc3RydWN0IG1p cGlfZHNpX2RldmljZSBkc2k7Cj4gKyAgICAgICBzdHJ1Y3QgaTJjX2NsaWVudCAqcGFnZVtNQVhf REVWU107ClRocm93IGluIGFuIGVudW0gdGhhdCBwcm92aWRlcyBzeW1ib2xpYyBuYW1lcyBmb3Ig dGhlIGRpZmZlcmVudCBpMmMKY2xpZW50cyBhbmQgcmVuYW1lICJwYWdlIiB0byBjbGllbnRzIG9y IGFueXRoaW5nIGVsc2UgdGhhdCBtYWtlcyBpdApjbGVhcmVyID8gU2VlbXMgbGlrZSAnMScgYW5k ICc2JyBhcmUgbmV2ZXIgdXNlZC4KClVzaW5nIGJldHRlciBuYW1lcyB0aGFuIGNsaWVudDIvNyBp biB0aGUgYWN0dWFsIGNvZGUgd2lsbCBiZSBncmVhdCA6LSkKCj4gKyAgICAgICBzdHJ1Y3QgaTJj X2NsaWVudCAqZGRjX2kyYzsKPiArICAgICAgIHN0cnVjdCByZWd1bGF0b3JfYnVsa19kYXRhIHN1 cHBsaWVzWzJdOwo+ICsgICAgICAgc3RydWN0IGRybV9wYW5lbCAqcGFuZWw7Cj4gKyAgICAgICBz dHJ1Y3QgZ3Bpb19kZXNjICpncGlvX3JzdF9uOwo+ICsgICAgICAgc3RydWN0IGdwaW9fZGVzYyAq Z3Bpb19zbHBfbjsKPiArICAgICAgIHN0cnVjdCBncGlvX2Rlc2MgKmdwaW9fbW9kZV9zZWxfbjsK V2hhdCBkb2VzIHRoZSBfbiBzdWZmaXggbWVhbiBpbiB0aGUgYWJvdmUgdGhyZWUgPyBCaWtlc2hl ZDoKcmVzZXRfZ3Bpbywgc2xlZXBfZ3BpbyBhbmQgbW9kZV9zZWwoZWN0KV9ncGlvIGNvdWxkIGFs c28gYmUgdXNlZCA7LSkKCj4gKyAgICAgICBib29sIGVuYWJsZWQ7Cj4gKwo+ICsgICAgICAgLyog ZmlybXdhcmUgZmlsZSBpbmZvICovCj4gKyAgICAgICBzdHJ1Y3QgcHM4NjQwX2luZm8gaW5mbzsK PiArICAgICAgIGJvb2wgaW5fZndfdXBkYXRlOwo+ICsgICAgICAgLyogZm9yIGZpcm13YXJlIHVw ZGF0ZSBwcm90ZWN0ICovCj4gKyAgICAgICBzdHJ1Y3QgbXV0ZXggZndfbXV0ZXg7Cj4gK307Cj4g Kwo+ICtzdGF0aWMgY29uc3QgdTggZW5jX2N0cmxfY29kZVs2XSA9IHsgMHhhYSwgMHg1NSwgMHg1 MCwgMHg0MSwgMHg1MiwgMHg0NCB9OwpUaGVzZSBmZWVsIGEgYml0IG1hZ2ljYWwuIEFueSBjaGFu Y2UgeW91IGNhbiBnaXZlIHRoZW0gc3ltYm9saWMgbmFtZXMgPwoKPiArc3RhdGljIGNvbnN0IHU4 IGh3X2NoaXBfaWRbNF0gPSB7IDB4MDAsIDB4MGEsIDB4MDAsIDB4MzAgfTsKPiArCj4gK3N0YXRp YyBpbmxpbmUgc3RydWN0IHBzODY0MCAqYnJpZGdlX3RvX3BzODY0MChzdHJ1Y3QgZHJtX2JyaWRn ZSAqZSkKPiArewo+ICsgICAgICAgcmV0dXJuIGNvbnRhaW5lcl9vZihlLCBzdHJ1Y3QgcHM4NjQw LCBicmlkZ2UpOwo+ICt9Cj4gKwo+ICtzdGF0aWMgaW5saW5lIHN0cnVjdCBwczg2NDAgKmNvbm5l Y3Rvcl90b19wczg2NDAoc3RydWN0IGRybV9jb25uZWN0b3IgKmUpCj4gK3sKPiArICAgICAgIHJl dHVybiBjb250YWluZXJfb2YoZSwgc3RydWN0IHBzODY0MCwgY29ubmVjdG9yKTsKPiArfQo+ICsK PiArc3RhdGljIGludCBwczg2NDBfcmVhZChzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LCB1OCBy ZWcsIHU4ICpkYXRhLAo+ICsgICAgICAgICAgICAgICAgICAgICAgdTE2IGRhdGFfbGVuKQo+ICt7 Cj4gKyAgICAgICBpbnQgcmV0Owo+ICsgICAgICAgc3RydWN0IGkyY19tc2cgbXNnc1tdID0gewo+ ICsgICAgICAgICAgICAgICB7Cj4gKyAgICAgICAgICAgICAgICAuYWRkciA9IGNsaWVudC0+YWRk ciwKPiArICAgICAgICAgICAgICAgIC5mbGFncyA9IDAsCj4gKyAgICAgICAgICAgICAgICAubGVu ID0gMSwKPiArICAgICAgICAgICAgICAgIC5idWYgPSAmcmVnLAo+ICsgICAgICAgICAgICAgICB9 LAo+ICsgICAgICAgICAgICAgICB7Cj4gKyAgICAgICAgICAgICAgICAuYWRkciA9IGNsaWVudC0+ YWRkciwKPiArICAgICAgICAgICAgICAgIC5mbGFncyA9IEkyQ19NX1JELAo+ICsgICAgICAgICAg ICAgICAgLmxlbiA9IGRhdGFfbGVuLAo+ICsgICAgICAgICAgICAgICAgLmJ1ZiA9IGRhdGEsCj4g KyAgICAgICAgICAgICAgIH0KPiArICAgICAgIH07Cj4gKwo+ICsgICAgICAgcmV0ID0gaTJjX3Ry YW5zZmVyKGNsaWVudC0+YWRhcHRlciwgbXNncywgMik7Cj4gKwo+ICsgICAgICAgaWYgKHJldCA9 PSAyKQo+ICsgICAgICAgICAgICAgICByZXR1cm4gMDsKPiArICAgICAgIGlmIChyZXQgPCAwKQo+ ICsgICAgICAgICAgICAgICByZXR1cm4gcmV0Owo+ICsgICAgICAgZWxzZQo+ICsgICAgICAgICAg ICAgICByZXR1cm4gLUVJTzsKPiArfQo+ICsKPiArc3RhdGljIGludCBwczg2NDBfd3JpdGVfYnl0 ZXMoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCwgY29uc3QgdTggKmRhdGEsCj4gKyAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgdTE2IGRhdGFfbGVuKQo+ICt7Cj4gKyAgICAgICBpbnQgcmV0 Owo+ICsgICAgICAgc3RydWN0IGkyY19tc2cgbXNnOwo+ICsKPiArICAgICAgIG1zZy5hZGRyID0g Y2xpZW50LT5hZGRyOwo+ICsgICAgICAgbXNnLmZsYWdzID0gMDsKPiArICAgICAgIG1zZy5sZW4g PSBkYXRhX2xlbjsKPiArICAgICAgIG1zZy5idWYgPSAodTggKilkYXRhOwo+ICsKPiArICAgICAg IHJldCA9IGkyY190cmFuc2ZlcihjbGllbnQtPmFkYXB0ZXIsICZtc2csIDEpOwo+ICsgICAgICAg aWYgKHJldCA9PSAxKQo+ICsgICAgICAgICAgICAgICByZXR1cm4gMDsKPiArICAgICAgIGlmIChy ZXQgPCAwKQo+ICsgICAgICAgICAgICAgICByZXR1cm4gcmV0Owo+ICsgICAgICAgZWxzZQo+ICsg ICAgICAgICAgICAgICByZXR1cm4gLUVJTzsKPiArfQo+ICsKPiArc3RhdGljIGludCBwczg2NDBf d3JpdGVfYnl0ZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LCB1OCByZWcsICB1OCBkYXRhKQo+ ICt7Cj4gKyAgICAgICB1OCBidWZbXSA9IHsgcmVnLCBkYXRhIH07Cj4gKwo+ICsgICAgICAgcmV0 dXJuIHBzODY0MF93cml0ZV9ieXRlcyhjbGllbnQsIGJ1Ziwgc2l6ZW9mKGJ1ZikpOwo+ICt9Cj4g Kwo+ICtzdGF0aWMgdm9pZCBwczg2NDBfZ2V0X21jdV9md192ZXJzaW9uKHN0cnVjdCBwczg2NDAg KnBzX2JyaWRnZSkKPiArewo+ICsgICAgICAgc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCA9IHBz X2JyaWRnZS0+cGFnZVs1XTsKPiArICAgICAgIHU4IGZ3X3ZlclsyXTsKPiArCj4gKyAgICAgICBw czg2NDBfcmVhZChjbGllbnQsIDB4NCwgZndfdmVyLCBzaXplb2YoZndfdmVyKSk7Cj4gKyAgICAg ICBwc19icmlkZ2UtPmluZm8udmVyc2lvbiA9IChmd192ZXJbMF0gPDwgOCkgfCBmd192ZXJbMV07 Cj4gKwo+ICsgICAgICAgRFJNX0lORk9fT05DRSgicHM4NjQwIHJvbSBmdyB2ZXJzaW9uICVkLiVk XG4iLCBmd192ZXJbMF0sIGZ3X3ZlclsxXSk7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQgcHM4NjQw X2JyaWRnZV91bm11dGUoc3RydWN0IHBzODY0MCAqcHNfYnJpZGdlKQo+ICt7Cj4gKyAgICAgICBz dHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50ID0gcHNfYnJpZGdlLT5wYWdlWzNdOwo+ICsgICAgICAg dTggdmRvX2N0cmxfYnVmWzNdID0geyBQQUdFM19TRVRfQURELCBWRE9fQ1RMX0FERCwgVkRPX0VO IH07Cj4gKwo+ICsgICAgICAgcmV0dXJuIHBzODY0MF93cml0ZV9ieXRlcyhjbGllbnQsIHZkb19j dHJsX2J1Ziwgc2l6ZW9mKHZkb19jdHJsX2J1ZikpOwo+ICt9Cj4gKwo+ICtzdGF0aWMgaW50IHBz ODY0MF9icmlkZ2VfbXV0ZShzdHJ1Y3QgcHM4NjQwICpwc19icmlkZ2UpCj4gK3sKPiArICAgICAg IHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQgPSBwc19icmlkZ2UtPnBhZ2VbM107Cj4gKyAgICAg ICB1OCB2ZG9fY3RybF9idWZbM10gPSB7IFBBR0UzX1NFVF9BREQsIFZET19DVExfQURELCBWRE9f RElTIH07Cj4gKwo+ICsgICAgICAgcmV0dXJuIHBzODY0MF93cml0ZV9ieXRlcyhjbGllbnQsIHZk b19jdHJsX2J1Ziwgc2l6ZW9mKHZkb19jdHJsX2J1ZikpOwo+ICt9Cj4gKwo+ICtzdGF0aWMgdm9p ZCBwczg2NDBfcHJlX2VuYWJsZShzdHJ1Y3QgZHJtX2JyaWRnZSAqYnJpZGdlKQo+ICt7Cj4gKyAg ICAgICBzdHJ1Y3QgcHM4NjQwICpwc19icmlkZ2UgPSBicmlkZ2VfdG9fcHM4NjQwKGJyaWRnZSk7 Cj4gKyAgICAgICBzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50ID0gcHNfYnJpZGdlLT5wYWdlWzJd Owo+ICsgICAgICAgaW50IGVycjsKPiArICAgICAgIHU4IHNldF92ZG9fZG9uZTsKPiArICAgICAg IGt0aW1lX3QgdGltZW91dDsKPiArCj4gKyAgICAgICBpZiAocHNfYnJpZGdlLT5pbl9md191cGRh dGUpCj4gKyAgICAgICAgICAgICAgIHJldHVybjsKPiArCj4gKyAgICAgICBpZiAocHNfYnJpZGdl LT5lbmFibGVkKQo+ICsgICAgICAgICAgICAgICByZXR1cm47Cj4gKwo+ICsgICAgICAgZXJyID0g ZHJtX3BhbmVsX3ByZXBhcmUocHNfYnJpZGdlLT5wYW5lbCk7Cj4gKyAgICAgICBpZiAoZXJyIDwg MCkgewo+ICsgICAgICAgICAgICAgICBEUk1fRVJST1IoImZhaWxlZCB0byBwcmVwYXJlIHBhbmVs OiAlZFxuIiwgZXJyKTsKPiArICAgICAgICAgICAgICAgcmV0dXJuOwo+ICsgICAgICAgfQo+ICsK PiArICAgICAgIGVyciA9IHJlZ3VsYXRvcl9idWxrX2VuYWJsZShBUlJBWV9TSVpFKHBzX2JyaWRn ZS0+c3VwcGxpZXMpLAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHBzX2Jy aWRnZS0+c3VwcGxpZXMpOwo+ICsgICAgICAgaWYgKGVyciA8IDApIHsKPiArICAgICAgICAgICAg ICAgRFJNX0VSUk9SKCJjYW5ub3QgZW5hYmxlIHJlZ3VsYXRvcnMgJWRcbiIsIGVycik7Cj4gKyAg ICAgICAgICAgICAgIGdvdG8gZXJyX3BhbmVsX3VucHJlcGFyZTsKPiArICAgICAgIH0KPiArCj4g KyAgICAgICBncGlvZF9zZXRfdmFsdWUocHNfYnJpZGdlLT5ncGlvX3NscF9uLCAxKTsKPiArICAg ICAgIGdwaW9kX3NldF92YWx1ZShwc19icmlkZ2UtPmdwaW9fcnN0X24sIDApOwo+ICsgICAgICAg dXNsZWVwX3JhbmdlKDIwMDAsIDI1MDApOwo+ICsgICAgICAgZ3Bpb2Rfc2V0X3ZhbHVlKHBzX2Jy aWRnZS0+Z3Bpb19yc3RfbiwgMSk7Cj4gKwo+ICsgICAgICAgLyoKPiArICAgICAgICAqIFdhaXQg Zm9yIHRoZSBwczg2NDAgZW1iZWQgbWN1IHJlYWR5Cj4gKyAgICAgICAgKiBGaXJzdCB3YWl0IDIw MG1zIGFuZCB0aGVuIGNoZWNrIHRoZSBtY3UgcmVhZHkgZmxhZyBldmVyeSAyMG1zCj4gKyAgICAg ICAgKi8KPiArICAgICAgIG1zbGVlcCgyMDApOwo+ICsKCj4gKyAgICAgICB0aW1lb3V0ID0ga3Rp bWVfYWRkX21zKGt0aW1lX2dldCgpLCAyMDApOwo+ICsgICAgICAgZm9yICg7Oykgewo+ICsgICAg ICAgICAgICAgICBlcnIgPSBwczg2NDBfcmVhZChjbGllbnQsIFBBR0UyX0dQSU9fSCwgJnNldF92 ZG9fZG9uZSwgMSk7Cj4gKyAgICAgICAgICAgICAgIGlmIChlcnIgPCAwKSB7Cj4gKyAgICAgICAg ICAgICAgICAgICAgICAgRFJNX0VSUk9SKCJmYWlsZWQgcmVhZCBQQUdFMl9HUElPX0g6ICVkXG4i LCBlcnIpOwo+ICsgICAgICAgICAgICAgICAgICAgICAgIGdvdG8gZXJyX3JlZ3VsYXRvcnNfZGlz YWJsZTsKPiArICAgICAgICAgICAgICAgfQo+ICsgICAgICAgICAgICAgICBpZiAoKHNldF92ZG9f ZG9uZSAmIFBTX0dQSU85KSA9PSBQU19HUElPOSkKPiArICAgICAgICAgICAgICAgICAgICAgICBi cmVhazsKPiArICAgICAgICAgICAgICAgaWYgKGt0aW1lX2NvbXBhcmUoa3RpbWVfZ2V0KCksIHRp bWVvdXQpID4gMCkKPiArICAgICAgICAgICAgICAgICAgICAgICBicmVhazsKPiArICAgICAgICAg ICAgICAgbXNsZWVwKDIwKTsKPiArICAgICAgIH0KPiArClRoaXMgaXMgdGhlIGZpcnN0IHN1Y2gg Y29uc3RydWN0IGluIERSTS4gSXMgdGhlcmUgYW55dGhpbmcgd3JvbmcgdXNpbmcKc29tZXRoaW5n IGxpa2UgdGhlIGZvbGxvd2luZyA/IFNhbWUgcXVlc3Rpb24gYXBwbGllcyBmb3IgdGhlIHJlc3Qg b2YKdGhlIHBhdGNoLgoKCmNvdW50ID0gWFg7Cgpmb3IgKGkgPSAwOyBpIDwgY291bnQ7IGkrKykg ewogICAgZXJyID0gcHM4NjQwX3JlYWQoY2xpZW50LCBQQUdFMl9HUElPX0gsICZzZXRfdmRvX2Rv bmUsIDEpOwogICAgaWYgKGVyciA8IDApIHsKICAgICAgICBEUk1fRVJST1IoImZhaWxlZCByZWFk IFBBR0UyX0dQSU9fSDogJWRcbiIsIGVycik7CiAgICAgICAgZ290byBlcnJfcmVndWxhdG9yc19k aXNhYmxlOwogICAgfQogICAgaWYgKChzZXRfdmRvX2RvbmUgJiBQU19HUElPOSkgPT0gUFNfR1BJ TzkpCiAgICAgICAgYnJlYWs7CgogICAgbXNsZWVwKDIwKTsKfQoKaWYgKGkgPT0gY291bnQpIHsK ICAgIHByaW50X3NvbWVfd2FybmluZygpCiAgICBlcnJvcl9vdXQoKQp9CgoKCj4gK3N0YXRpYyBp bnQgcHM4NjQwX2dldF9tb2RlcyhzdHJ1Y3QgZHJtX2Nvbm5lY3RvciAqY29ubmVjdG9yKQo+ICt7 Cgo+ICsKPiArICAgICAgIGVkaWQgPSBkZXZtX2ttYWxsb2MoZGV2LCBzaXplb2YoZWRpZCksIEdG UF9LRVJORUwpOwo+ICsgICAgICAgaWYgKCFlZGlkKSB7Cj4gKyAgICAgICAgICAgICAgIERSTV9F UlJPUigiRmFpbGVkIHRvIGFsbG9jYXRlIEVESURcbiIpOwo+ICsgICAgICAgICAgICAgICByZXR1 cm4gMDsKUmVtb3ZlIHRoaXMuIGRybV9nZXRfZWRpZCgpIGFscmVhZHkgcmV0dXJucyBhIGttYWxs b2MnZCBodW5rIG9mIG1lbW9yeS4KCj4gKyAgICAgICB9Cj4gKwo+ICsgICAgICAgZWRpZCA9IGRy bV9nZXRfZWRpZChjb25uZWN0b3IsIHBzX2JyaWRnZS0+ZGRjX2kyYy0+YWRhcHRlcik7Cj4gKyAg ICAgICBpZiAoIWVkaWQpCj4gKyAgICAgICAgICAgICAgIGdvdG8gb3V0Owo+ICsKCgo+ICtpbnQg cHM4NjQwX2JyaWRnZV9hdHRhY2goc3RydWN0IGRybV9icmlkZ2UgKmJyaWRnZSkKPiArewo+ICsg ICAgICAgc3RydWN0IHBzODY0MCAqcHNfYnJpZGdlID0gYnJpZGdlX3RvX3BzODY0MChicmlkZ2Up Owo+ICsgICAgICAgc3RydWN0IGRldmljZSAqZGV2ID0gJnBzX2JyaWRnZS0+cGFnZVswXS0+ZGV2 Owo+ICsgICAgICAgc3RydWN0IGRldmljZV9ub2RlICpucCA9IGRldi0+b2Zfbm9kZTsKS2lsbCBv ZmYgdGhlIHRlbXBvcmFyeSB2YXJpYWJsZS4gZGV2LT5vZl9ub2RlIGlzIG5vdCB0aGF0IGxvbmcg YW5kCml0J3MgdXNlZCBvbmx5IG9uY2UuCgo+ICsgICAgICAgc3RydWN0IG1pcGlfZHNpX2hvc3Qg Kmhvc3QgPSBwc19icmlkZ2UtPmRzaS5ob3N0OwpUaGlzIGxvb2tzIGEgYml0IG9kZC4gQ2FuIHlv dSBtb3ZlIGl0IHRvIHRoZSAhZHNpX25vZGUgYmVsb3cgYW5kIGFkZCBhCnNtYWxsIGNvbW1lbnQg PwoKCgo+ICsgICAgICAgaWYgKGRzaV9ub2RlKSB7Cj4gKyAgICAgICAgICAgICAgIGhvc3QgPSBv Zl9maW5kX21pcGlfZHNpX2hvc3RfYnlfbm9kZShkc2lfbm9kZSk7Cj4gKyAgICAgICAgICAgICAg IG9mX25vZGVfcHV0KGRzaV9ub2RlKTsKPiArICAgICAgICAgICAgICAgaWYgKCFob3N0KSB7Cj4g KyAgICAgICAgICAgICAgICAgICAgICAgcmV0ID0gLUVOT0RFVjsKPiArICAgICAgICAgICAgICAg ICAgICAgICBnb3RvIGVycjsKPiArICAgICAgICAgICAgICAgfQo+ICsgICAgICAgfQo+ICsKfSBl bHNlIHsKICAgIC8vIFVzZSBYWFggaWYgdGhlcmUncyBubyBkc2kgaG9zdCBwcm92aWRlZCBieSBE VAogICAgaG9zdCA9IHBzX2JyaWRnZS0+ZHNpLmhvc3Q7Cn0KCgo+ICtzdGF0aWMgaW50IHBzODY0 MF93YWl0X3JvbV9pZGxlKHN0cnVjdCBwczg2NDAgKnBzX2JyaWRnZSkKPiArewo+ICsgICAgICAg c3RydWN0IGkyY19jbGllbnQgKmNsaWVudCA9IHBzX2JyaWRnZS0+cGFnZVswXTsKPiArICAgICAg IGludCByZXQ7Cj4gKwo+ICsgICAgICAgcmV0ID0gcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50LCBQ QUdFMl9JUk9NX0NUUkwsIElST01fRU5BQkxFKTsKPiArICAgICAgIGlmIChyZXQpCj4gKyAgICAg ICAgICAgICAgIGdvdG8gZXhpdDsKPiArCj4gKyAgICAgICByZXQgPSBwczg2NDBfd2FpdF9zcGlf cmVhZHkocHNfYnJpZGdlKTsKPiArICAgICAgIGlmIChyZXQpCj4gKyAgICAgICAgICAgICAgIGdv dG8gZXJyX3NwaTsKPiArCj4gKyAgICAgICByZXQgPSBwczg2NDBfd2FpdF9zcGlfbm9idXN5KHBz X2JyaWRnZSk7Cj4gKyAgICAgICBpZiAocmV0KQo+ICsgICAgICAgICAgICAgICBnb3RvIGVycl9z cGk7Cj4gKwo+ICsgICAgICAgcmV0ID0gcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50LCBQQUdFMl9J Uk9NX0NUUkwsIElST01fRElTQUJMRSk7Cj4gKyAgICAgICBpZiAocmV0KQo+ICsgICAgICAgICAg ICAgICBnb3RvIGV4aXQ7Cj4gKwo+ICsgICAgICAgcmV0dXJuIDA7Cj4gKwo+ICtlcnJfc3BpOgo+ ICsgICAgICAgcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50LCBQQUdFMl9JUk9NX0NUUkwsIElST01f RElTQUJMRSk7Cj4gK2V4aXQ6Cj4gKyAgICAgICBpZiAocmV0KQpyZXQgaXMgYWx3YXlzIG5vbiB6 ZXJvIGhlcmUuCgo+ICsgICAgICAgICAgICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwgIndhaXQg cHM4NjQwIHJvbSBpZGxlIGZhaWw6ICVkXG4iLCByZXQpOwo+ICsKPiArICAgICAgIHJldHVybiBy ZXQ7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQgcHM4NjQwX3NwaV9kbF9tb2RlKHN0cnVjdCBwczg2 NDAgKnBzX2JyaWRnZSkKPiArewo+ICsgICAgICAgc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCA9 IHBzX2JyaWRnZS0+cGFnZVsyXTsKPiArICAgICAgIGludCByZXQ7Cj4gKwo+ICsgICAgICAgLyog c3dpdGNoIHBzODY0MCBtb2RlIHRvIHNwaSBkbCBtb2RlICovCj4gKyAgICAgICBpZiAocHNfYnJp ZGdlLT5ncGlvX21vZGVfc2VsX24pCj4gKyAgICAgICAgICAgICAgIGdwaW9kX3NldF92YWx1ZShw c19icmlkZ2UtPmdwaW9fbW9kZV9zZWxfbiwgMCk7Cj4gKwo+ICsgICAgICAgLyogcmVzZXQgc3Bp IGludGVyZmFjZSAqLwo+ICsgICAgICAgcmV0ID0gcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50LCBQ QUdFMl9TV19SRVNFVCwKPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFNQSV9TV19S RVNFVCB8IE1QVV9TV19SRVNFVCk7Cj4gKyAgICAgICBpZiAocmV0KQo+ICsgICAgICAgICAgICAg ICBnb3RvIGV4aXQ7Cj4gKwo+ICsgICAgICAgcmV0ID0gcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50 LCBQQUdFMl9TV19SRVNFVCwgTVBVX1NXX1JFU0VUKTsKPiArICAgICAgIGlmIChyZXQpCj4gKyAg ICAgICAgICAgICAgIGdvdG8gZXhpdDsKPiArCkFkZCAicmV0dXJuIDA7IiAuLi4KCj4gK2V4aXQ6 Cj4gKyAgICAgICBpZiAocmV0KQouLi4gZHJvcCB0aGlzIGNoZWNrLgoKPiArICAgICAgICAgICAg ICAgZGV2X2VycigmY2xpZW50LT5kZXYsICJmYWlsIHJlc2V0IHNwaSBpbnRlcmZhY2U6ICVkXG4i LCByZXQpOwo+ICsKPiArICAgICAgIHJldHVybiByZXQ7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQg cHM4NjQwX3JvbV9wcmVwYXJlKHN0cnVjdCBwczg2NDAgKnBzX2JyaWRnZSkKPiArewoKCj4gKyAg ICAgICBmb3IgKGkgPSAwOyBpIDwgNjsgaSsrKQpzLzYvQVJSQVlfU0laRShlbmNfY3RybF9jb2Rl KS8KCj4gKyAgICAgICAgICAgICAgIHBzODY0MF93cml0ZV9ieXRlKGNsaWVudCwgUEFHRTJfRU5D VExTUElfV1IsIGVuY19jdHJsX2NvZGVbaV0pOwoKCj4gK3N0YXRpYyBpbnQgcHM4NjQwX3dyaXRl X3JvbShzdHJ1Y3QgcHM4NjQwICpwc19icmlkZ2UsIGNvbnN0IHN0cnVjdCBmaXJtd2FyZSAqZncp Cj4gK3sKPiArICAgICAgIHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQgPSBwc19icmlkZ2UtPnBh Z2VbMF07Cj4gKyAgICAgICBzdHJ1Y3QgZGV2aWNlICpkZXYgPSAmY2xpZW50LT5kZXY7Cj4gKyAg ICAgICBzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50MiA9IHBzX2JyaWRnZS0+cGFnZVsyXTsKPiAr ICAgICAgIHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQ3ID0gcHNfYnJpZGdlLT5wYWdlWzddOwo+ ICsgICAgICAgc2l6ZV90IHBvczsKPiArICAgICAgIHU4IGJ1ZlsyNTddLCByb21fcGFnZV9pZF9i dWZbM107Cj4gKyAgICAgICBpbnQgcmV0Owo+ICsgICAgICAgdTE2IGNweV9sZW47Cj4gKwo+ICsg ICAgICAgcHM4NjQwX3dyaXRlX2J5dGUoY2xpZW50MiwgUEFHRTJfU1BJX0NGRzMsIEkyQ19UT19T UElfUkVTRVQpOwo+ICsgICAgICAgbXNsZWVwKDEwMCk7Cj4gKyAgICAgICBwczg2NDBfd3JpdGVf Ynl0ZShjbGllbnQyLCBQQUdFMl9TUElfQ0ZHMywgMHgwMCk7Cj4gKwo+ICsgICAgICAgZm9yIChw b3MgPSAwOyBwb3MgPCBmdy0+c2l6ZTsgcG9zICs9IGNweV9sZW4pIHsKPiArICAgICAgICAgICAg ICAgcm9tX3BhZ2VfaWRfYnVmWzBdID0gUEFHRTJfUk9NQUREX0JZVEUxOwo+ICsgICAgICAgICAg ICAgICByb21fcGFnZV9pZF9idWZbMV0gPSBwb3MgPj4gODsKPiArICAgICAgICAgICAgICAgcm9t X3BhZ2VfaWRfYnVmWzJdID0gcG9zID4+IDE2OwpSZXVzZSBidWZbXSwgdGhlIHN0YWNrIGlzIGdl dHRpbmcgYmlnIGVub3VnaCBhbHJlYWR5ID8KCj4gKyAgICAgICAgICAgICAgIHJldCA9IHBzODY0 MF93cml0ZV9ieXRlcyhjbGllbnQyLCByb21fcGFnZV9pZF9idWYsIDMpOwo+ICsgICAgICAgICAg ICAgICBpZiAocmV0KQo+ICsgICAgICAgICAgICAgICAgICAgICAgIGdvdG8gZXJyb3I7Cj4gKyAg ICAgICAgICAgICAgIGNweV9sZW4gPSBmdy0+c2l6ZSA+PSAyNTYgKyBwb3MgPyAyNTYgOiBmdy0+ c2l6ZSAtIHBvczsKY3B5X2xlbiA9IG1pbigyNTYsIGZ3LT5zaXplIC0gcG9zKTsgcGVyaGFwcyA/ Cgo+ICsgICAgICAgICAgICAgICBidWZbMF0gPSAwOwo+ICsgICAgICAgICAgICAgICBtZW1jcHko YnVmICsgMSwgZnctPmRhdGEgKyBwb3MsIGNweV9sZW4pOwo+ICsgICAgICAgICAgICAgICByZXQg PSBwczg2NDBfd3JpdGVfYnl0ZXMoY2xpZW50NywgYnVmLCBjcHlfbGVuICsgMSk7Cj4gKyAgICAg ICAgICAgICAgIGlmIChyZXQpCj4gKyAgICAgICAgICAgICAgICAgICAgICAgZ290byBlcnJvcjsK PiArCj4gKyAgICAgICAgICAgICAgIGRldl9kYmcoZGV2LCAiZncgdXBkYXRlIGNvbXBsZXRlZCAl enUgLyAlenUgYnl0ZXNcbiIsIHBvcywKPiArICAgICAgICAgICAgICAgICAgICAgICBmdy0+c2l6 ZSk7Cj4gKyAgICAgICB9Cj4gKyAgICAgICByZXR1cm4gMDsKPiArCj4gK2Vycm9yOgo+ICsgICAg ICAgZGV2X2VycihkZXYsICJmYWlsZWQgd3JpdGUgZXh0ZXJuYWwgZmxhc2gsICVkXG4iLCByZXQp Owo+ICsgICAgICAgcmV0dXJuIHJldDsKPiArfQo+ICsKPiArc3RhdGljIGludCBwczg2NDBfc3Bp X25vcm1hbF9tb2RlKHN0cnVjdCBwczg2NDAgKnBzX2JyaWRnZSkKPiArewpJcyBpdCB0aGF0IHBz ODY0MF9zcGlfc2VuZF9jbWQoKS4uLiAnY2Fubm90IGZhaWwnIGluIGhlcmUsIHVubGlrZSBpbgpw czg2NDBfc3BpX2RsX21vZGUoKSBhbmQgcHM4NjQwX3JvbV9wcmVwYXJlKCkgPyBJZiBzbyBpdCBt aWdodCBiZQp3b3J0aCBhZGRpbmcgYSBsaW5lIG9yIHR3byBvZiBjb21tZW50IGFuZCBtYWtpbmcK cHM4NjQwX3NwaV9ub3JtYWxfbW9kZSgpIHJldHVybiB2b2lkLgoKCj4gK3N0YXRpYyBpbnQgcHM4 NjQwX2xvYWRfZncoc3RydWN0IHBzODY0MCAqcHNfYnJpZGdlLCBjb25zdCBzdHJ1Y3QgZmlybXdh cmUgKmZ3KQo+ICt7Cj4gKyAgICAgICBzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50ID0gcHNfYnJp ZGdlLT5wYWdlWzBdOwo+ICsgICAgICAgc3RydWN0IGRldmljZSAqZGV2ID0gJmNsaWVudC0+ZGV2 Owo+ICsgICAgICAgaW50IHJldDsKPiArICAgICAgIGJvb2wgcHM4NjQwX3N0YXR1c19iYWNrdXAg PSBwc19icmlkZ2UtPmVuYWJsZWQ7Cj4gKwo+ICsgICAgICAgcmV0ID0gcHM4NjQwX3ZhbGlkYXRl X2Zpcm13YXJlKHBzX2JyaWRnZSwgZncpOwo+ICsgICAgICAgaWYgKHJldCkKPiArICAgICAgICAg ICAgICAgcmV0dXJuIHJldDsKPiArCj4gKyAgICAgICBtdXRleF9sb2NrKCZwc19icmlkZ2UtPmZ3 X211dGV4KTsKPiArICAgICAgIGlmICghcHNfYnJpZGdlLT5pbl9md191cGRhdGUpIHsKPiArICAg ICAgICAgICAgICAgaWYgKCFwczg2NDBfc3RhdHVzX2JhY2t1cCkKPiArICAgICAgICAgICAgICAg ICAgICAgICBwczg2NDBfcHJlX2VuYWJsZSgmcHNfYnJpZGdlLT5icmlkZ2UpOwo+ICsKPiArICAg ICAgICAgICAgICAgcmV0ID0gcHM4NjQwX2VudGVyX2JsKHBzX2JyaWRnZSk7CklNSE8gb3Blbi1j b2RpbmcgcHM4NjQwX2VudGVyX2JsL3BzODY0MF9leGl0X2JsIGFuZCBoYXZpbmcgdGhlCmluX2Z3 X3VwZGF0ZSBtYW5pcHVsYXRpb24gdmlzaWJsZSB3aWxsIG1ha2UgdGhpbmdzIG1vcmUgb2J2aW91 cy4KCj4gKyAgICAgICAgICAgICAgIGlmIChyZXQpCj4gKyAgICAgICAgICAgICAgICAgICAgICAg Z290byBleGl0Owo+ICsgICAgICAgfQo+ICsKPiArICAgICAgIHJldCA9IHBzODY0MF9yb21fcHJl cGFyZShwc19icmlkZ2UpOwo+ICsgICAgICAgaWYgKHJldCkKPiArICAgICAgICAgICAgICAgZ290 byBleGl0Owo+ICsKPiArICAgICAgIHJldCA9IHBzODY0MF93cml0ZV9yb20ocHNfYnJpZGdlLCBm dyk7Cj4gKwo+ICtleGl0Ogo+ICsgICAgICAgaWYgKHJldCkKPiArICAgICAgICAgICAgICAgZGV2 X2VycihkZXYsICJGYWlsZWQgdG8gbG9hZCBmaXJtd2FyZSwgJWRcbiIsIHJldCk7Cj4gKwoKPiAr ICAgICAgIHBzODY0MF9leGl0X2JsKHBzX2JyaWRnZSwgZncpOwo+ICsgICAgICAgaWYgKCFwczg2 NDBfc3RhdHVzX2JhY2t1cCkKPiArICAgICAgICAgICAgICAgcHM4NjQwX3Bvc3RfZGlzYWJsZSgm cHNfYnJpZGdlLT5icmlkZ2UpOwpXaHkgYXJlIHdlIGNhbGxpbmcgdGhlc2UgZXZlbiBpZiB3ZSBu ZXZlciBleGVjdXRlZApwczg2NDBfcHJlX2VuYWJsZS9wczg2NDBfZW50ZXJfYmwgYWJvdmUgPwoK Cj4gK3N0YXRpYyBzc2l6ZV90IHBzODY0MF91cGRhdGVfZndfc3RvcmUoc3RydWN0IGRldmljZSAq ZGV2LAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc3RydWN0IGRldmlj ZV9hdHRyaWJ1dGUgKmF0dHIsCj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICBjb25zdCBjaGFyICpidWYsIHNpemVfdCBjb3VudCkKPiArewo+ICsgICAgICAgc3RydWN0IGky Y19jbGllbnQgKmNsaWVudCA9IHRvX2kyY19jbGllbnQoZGV2KTsKPiArICAgICAgIHN0cnVjdCBw czg2NDAgKnBzX2JyaWRnZSA9IGkyY19nZXRfY2xpZW50ZGF0YShjbGllbnQpOwo+ICsgICAgICAg Y29uc3Qgc3RydWN0IGZpcm13YXJlICpmdzsKPiArICAgICAgIGludCBlcnJvcjsKPiArCj4gKyAg ICAgICBlcnJvciA9IHJlcXVlc3RfZmlybXdhcmUoJmZ3LCBQU19GV19OQU1FLCBkZXYpOwpDYW4g dGhlIGRldmljZSBvcGVyYXRlIHdpdGhvdXQgYSBmaXJtd2FyZSA/IElmIG5vdCwgd2h5IGlzIHRo ZQpmaXJtd2FyZSBsb2FkZWQgc28gbGF0ZXIvYWZ0ZXIgdXNlciBpbnRlcmFjdGlvbiAodmlhIHN5 c2ZzKSA/IEkgZG9uJ3QKcmVjYWxsIGFueSBvdGhlciBkcml2ZXIgaW4gRFJNIHRvIHVzZSBzdWNo IGFuIGFwcHJvYWNoLgoKClJlZ2FyZHMsCkVtaWwKX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlz dHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4v bGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: emil.l.velikov@gmail.com (Emil Velikov) Date: Thu, 16 Jun 2016 20:14:07 +0100 Subject: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge In-Reply-To: <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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 = ®, > + }, > + { > + .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