[{"id":1758272,"web_url":"http://patchwork.ozlabs.org/comment/1758272/","msgid":"<20170827153054.ijqxbjk25zpskojl@ninjato>","list_archive_url":null,"date":"2017-08-27T15:30:54","subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","submitter":{"id":22495,"url":"http://patchwork.ozlabs.org/api/people/22495/","name":"Wolfram Sang","email":"wsa@the-dreams.de"},"content":"Hi,\n\nthanks for your submission.\n\n> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)\n> +{\n> +\tdev_err(&i2c_dev->adap.dev, \": ======dump i2c-%d reg=======\\n\",\n> +\t\ti2c_dev->adap.nr);\n> +\tdev_err(&i2c_dev->adap.dev, \": I2C_CTRL:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + I2C_CTL));\n> +\tdev_err(&i2c_dev->adap.dev, \": I2C_ADDR_CFG:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + I2C_ADDR_CFG));\n> +\tdev_err(&i2c_dev->adap.dev, \": I2C_COUNT:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + I2C_COUNT));\n> +\tdev_err(&i2c_dev->adap.dev, \": I2C_RX:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + I2C_RX));\n> +\tdev_err(&i2c_dev->adap.dev, \": I2C_STATUS:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + I2C_STATUS));\n> +\tdev_err(&i2c_dev->adap.dev, \": ADDR_DVD0:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + ADDR_DVD0));\n> +\tdev_err(&i2c_dev->adap.dev, \": ADDR_DVD1:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + ADDR_DVD1));\n> +\tdev_err(&i2c_dev->adap.dev, \": ADDR_STA0_DVD:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + ADDR_STA0_DVD));\n> +\tdev_err(&i2c_dev->adap.dev, \": ADDR_RST:0x%x\\n\",\n> +\t\treadl(i2c_dev->base + ADDR_RST));\n\nI really thing register dumps should be dev_dbg().\n\n> +}\n> +\n> +static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)\n> +{\n> +\twritel(count, i2c_dev->base + I2C_COUNT);\n> +}\n> +\n> +static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)\n> +{\n> +\tunsigned int tmp = readl(i2c_dev->base + I2C_CTL);\n\nu32? Here and in many other places?\n\n...\n\n> +static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)\n> +{\n> +\tstruct sprd_i2c *i2c_dev = dev_id;\n> +\tstruct i2c_msg *msg = i2c_dev->msg;\n> +\tint ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;\n> +\tu32 i2c_count = readl(i2c_dev->base + I2C_COUNT);\n> +\tu32 i2c_tran;\n> +\n> +\tif (msg->flags & I2C_M_RD)\n> +\t\ti2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;\n> +\telse\n> +\t\ti2c_tran = i2c_count;\n> +\n> +\t/*\n> +\t * If we got one ACK from slave when writing data, and we did not\n\nHere you say: \"If we get ack...\"\n\n> +\t * finish this transmission (i2c_tran is not zero), then we should\n> +\t * continue to write data.\n> +\t *\n> +\t * For reading data, ack is always 0, if i2c_tran is not 0 which\n> +\t * means we still need to contine to read data from slave.\n> +\t */\n> +\tif (i2c_tran && !ack) {\n\n... but the code gives the assumption you did NOT get an ack. So, either\nrename the variable to 'ack_err' or keep it 'ack' and invert the logic\nwhen initializing the variable.\n\n> +\t\tsprd_i2c_data_transfer(i2c_dev);\n> +\t\treturn IRQ_HANDLED;\n> +\t}\n> +\n> +\ti2c_dev->err = 0;\n> +\n> +\t/*\n> +\t * If we did not get one ACK from slave when writing data, we should\n> +\t * dump all registers to check I2C status.\n\nWhy? I would say no. NACK from a slave can always happen, e.g. when an\nEEPROM is busy erasing a page.\n\n> +\t */\n> +\tif (ack) {\n> +\t\ti2c_dev->err = -EIO;\n> +\t\tsprd_i2c_dump_reg(i2c_dev);\n> +\t} else if (msg->flags & I2C_M_RD && i2c_dev->count) {\n> +\t\tsprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);\n> +\t}\n> +\n> +\t/* Transmission is done and clear ack and start operation */\n> +\tsprd_i2c_clear_ack(i2c_dev);\n> +\tsprd_i2c_clear_start(i2c_dev);\n> +\tcomplete(&i2c_dev->complete);\n> +\n> +\treturn IRQ_HANDLED;\n> +}\n\n...\n\n> +\n> +\tpm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);\n> +\tpm_runtime_use_autosuspend(i2c_dev->dev);\n> +\tpm_runtime_set_active(i2c_dev->dev);\n> +\tpm_runtime_enable(i2c_dev->dev);\n> +\n> +\tret = pm_runtime_get_sync(i2c_dev->dev);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&pdev->dev, \"i2c%d pm runtime resume failed!\\n\",\n> +\t\t\tpdev->id);\n\nError message has wrong text.\n\n> +\t\tgoto err_rpm_put;\n> +\t}\n> +\n> +static int sprd_i2c_init(void)\n> +{\n> +\treturn platform_driver_register(&sprd_i2c_driver);\n> +}\n> +arch_initcall_sync(sprd_i2c_init);\n\narch_initcall? and no exit() function? Why is it that way and/or why\ncan't you use platform_module_driver()?\n\nRest looks good. I like the comments you added to the code.\n\nRegards,\n\n Wolfram","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xgJlB1Dy1z9sDB\n\tfor ;\n\tMon, 28 Aug 2017 01:31:02 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751185AbdH0Pa5 (ORCPT );\n\tSun, 27 Aug 2017 11:30:57 -0400","from sauhun.de ([88.99.104.3]:37511 \"EHLO pokefinder.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751174AbdH0Pa4 (ORCPT );\n\tSun, 27 Aug 2017 11:30:56 -0400","from localhost (p54B33E6E.dip0.t-ipconnect.de [84.179.62.110])\n\tby pokefinder.org (Postfix) with ESMTPSA id AF3792C3504;\n\tSun, 27 Aug 2017 17:30:54 +0200 (CEST)"],"Date":"Sun, 27 Aug 2017 17:30:54 +0200","From":"Wolfram Sang ","To":"Baolin Wang ","Cc":"mark.rutland@arm.com, robh+dt@kernel.org,\n\tandriy.shevchenko@linux.intel.com, linux-i2c@vger.kernel.org,\n\tdevicetree@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tbroonie@kernel.org, baolin.wang@linaro.org","Subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","Message-ID":"<20170827153054.ijqxbjk25zpskojl@ninjato>","References":"\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gvxh3z3vm35cohlm\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1758344,"web_url":"http://patchwork.ozlabs.org/comment/1758344/","msgid":"","list_archive_url":null,"date":"2017-08-28T03:21:09","subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","submitter":{"id":66012,"url":"http://patchwork.ozlabs.org/api/people/66012/","name":"Baolin Wang","email":"baolin.wang@linaro.org"},"content":"Hi Wolfram,\n\nOn 27 August 2017 at 23:30, Wolfram Sang wrote:\n> Hi,\n>\n> thanks for your submission.\n>\n>> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)\n>> +{\n>> + dev_err(&i2c_dev->adap.dev, \": ======dump i2c-%d reg=======\\n\",\n>> + i2c_dev->adap.nr);\n>> + dev_err(&i2c_dev->adap.dev, \": I2C_CTRL:0x%x\\n\",\n>> + readl(i2c_dev->base + I2C_CTL));\n>> + dev_err(&i2c_dev->adap.dev, \": I2C_ADDR_CFG:0x%x\\n\",\n>> + readl(i2c_dev->base + I2C_ADDR_CFG));\n>> + dev_err(&i2c_dev->adap.dev, \": I2C_COUNT:0x%x\\n\",\n>> + readl(i2c_dev->base + I2C_COUNT));\n>> + dev_err(&i2c_dev->adap.dev, \": I2C_RX:0x%x\\n\",\n>> + readl(i2c_dev->base + I2C_RX));\n>> + dev_err(&i2c_dev->adap.dev, \": I2C_STATUS:0x%x\\n\",\n>> + readl(i2c_dev->base + I2C_STATUS));\n>> + dev_err(&i2c_dev->adap.dev, \": ADDR_DVD0:0x%x\\n\",\n>> + readl(i2c_dev->base + ADDR_DVD0));\n>> + dev_err(&i2c_dev->adap.dev, \": ADDR_DVD1:0x%x\\n\",\n>> + readl(i2c_dev->base + ADDR_DVD1));\n>> + dev_err(&i2c_dev->adap.dev, \": ADDR_STA0_DVD:0x%x\\n\",\n>> + readl(i2c_dev->base + ADDR_STA0_DVD));\n>> + dev_err(&i2c_dev->adap.dev, \": ADDR_RST:0x%x\\n\",\n>> + readl(i2c_dev->base + ADDR_RST));\n>\n> I really thing register dumps should be dev_dbg().\n\nOK. Will fix in next version.\n\n>\n>> +}\n>> +\n>> +static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)\n>> +{\n>> + writel(count, i2c_dev->base + I2C_COUNT);\n>> +}\n>> +\n>> +static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)\n>> +{\n>> + unsigned int tmp = readl(i2c_dev->base + I2C_CTL);\n>\n> u32? Here and in many other places?\n\nOK.\n\n>\n> ...\n>\n>> +static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)\n>> +{\n>> + struct sprd_i2c *i2c_dev = dev_id;\n>> + struct i2c_msg *msg = i2c_dev->msg;\n>> + int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;\n>> + u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);\n>> + u32 i2c_tran;\n>> +\n>> + if (msg->flags & I2C_M_RD)\n>> + i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;\n>> + else\n>> + i2c_tran = i2c_count;\n>> +\n>> + /*\n>> + * If we got one ACK from slave when writing data, and we did not\n>\n> Here you say: \"If we get ack...\"\n>\n>> + * finish this transmission (i2c_tran is not zero), then we should\n>> + * continue to write data.\n>> + *\n>> + * For reading data, ack is always 0, if i2c_tran is not 0 which\n>> + * means we still need to contine to read data from slave.\n>> + */\n>> + if (i2c_tran && !ack) {\n>\n> ... but the code gives the assumption you did NOT get an ack. So, either\n> rename the variable to 'ack_err' or keep it 'ack' and invert the logic\n> when initializing the variable.\n\nIf ack == 0 means we got one ack. I will invert the logic as you suggested.\n\n>\n>> + sprd_i2c_data_transfer(i2c_dev);\n>> + return IRQ_HANDLED;\n>> + }\n>> +\n>> + i2c_dev->err = 0;\n>> +\n>> + /*\n>> + * If we did not get one ACK from slave when writing data, we should\n>> + * dump all registers to check I2C status.\n>\n> Why? I would say no. NACK from a slave can always happen, e.g. when an\n> EEPROM is busy erasing a page.\n\nFor our I2C controller databook, if the master did not get one ACK\nfrom slave when writing data to salve, we should send one STOP signal\nto abort this data transfer or generate one repeated START signal to\nstart one new data transfer cycle. Considering our I2C usage\nscenarios, we should dump registers to analyze I2C status and notify\nto user to re-start new data transfer.\n\n>\n>> + */\n>> + if (ack) {\n>> + i2c_dev->err = -EIO;\n>> + sprd_i2c_dump_reg(i2c_dev);\n>> + } else if (msg->flags & I2C_M_RD && i2c_dev->count) {\n>> + sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);\n>> + }\n>> +\n>> + /* Transmission is done and clear ack and start operation */\n>> + sprd_i2c_clear_ack(i2c_dev);\n>> + sprd_i2c_clear_start(i2c_dev);\n>> + complete(&i2c_dev->complete);\n>> +\n>> + return IRQ_HANDLED;\n>> +}\n>\n> ...\n>\n>> +\n>> + pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);\n>> + pm_runtime_use_autosuspend(i2c_dev->dev);\n>> + pm_runtime_set_active(i2c_dev->dev);\n>> + pm_runtime_enable(i2c_dev->dev);\n>> +\n>> + ret = pm_runtime_get_sync(i2c_dev->dev);\n>> + if (ret < 0) {\n>> + dev_err(&pdev->dev, \"i2c%d pm runtime resume failed!\\n\",\n>> + pdev->id);\n>\n> Error message has wrong text.\n\nWill fix it.\n\n>\n>> + goto err_rpm_put;\n>> + }\n>> +\n>> +static int sprd_i2c_init(void)\n>> +{\n>> + return platform_driver_register(&sprd_i2c_driver);\n>> +}\n>> +arch_initcall_sync(sprd_i2c_init);\n>\n> arch_initcall? and no exit() function? Why is it that way and/or why\n> can't you use platform_module_driver()?\n\nAs I explained before, in our Spreadtrum platform, our regulator\ndriver will depend on I2C driver and the regulator driver uses\nsubsys_initcall() level to initialize. Moreover some other drivers\nlike GPU, they will depend on regulator to set voltage and they also\nneed initialization much earlier.\n\nSince it is arch_initcall() level, Andy suggested I should get rid of\ntristate (use bool) and drop module.h here and all leftovers like\nMODULE_*() calls including module_exit().\n\n>\n> Rest looks good. I like the comments you added to the code.\n\nReally appreciated for your comments.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"Pc7q4HOr\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xgcVk4H3Qz9t2x\n\tfor ;\n\tMon, 28 Aug 2017 13:21:18 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751885AbdH1DVN (ORCPT );\n\tSun, 27 Aug 2017 23:21:13 -0400","from mail-oi0-f43.google.com ([209.85.218.43]:33759 \"EHLO\n\tmail-oi0-f43.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751941AbdH1DVK (ORCPT\n\t); Sun, 27 Aug 2017 23:21:10 -0400","by mail-oi0-f43.google.com with SMTP id r203so32403198oih.0\n\tfor ; Sun, 27 Aug 2017 20:21:10 -0700 (PDT)","by 10.157.56.246 with HTTP; Sun, 27 Aug 2017 20:21:09 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=UbtuqeL/hxrOWmWey0lMduMJkqHNBabmTkVz+TKDYWY=;\n\tb=Pc7q4HOrI+DuXSNMXBI9Er6wNEJ80DAZ0FYW7j/BMuGDpbkUF3a1YmN4GIxjokisp/\n\t26cy7niwtPtnWwiW7/U6A7GinmsHPr7X73k9WAOq880V91wJWM94ZmT56YlWo8YpPv61\n\tGB4/U9Y1jsvpTPL2o7/zCh17VA2rtYPmLEziU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=UbtuqeL/hxrOWmWey0lMduMJkqHNBabmTkVz+TKDYWY=;\n\tb=Pyfl0tQNFFIoOn7bpSrVJx2/pnn2zA1Wc+DFi8coeCGgS0kSwQzW/zChFUkEBgPoDu\n\t5WH0Yy8OweeS783CRjGECKtvpQaFiP25c4vRQ1U2EECynVzOfuvncDZN7WgAOyBP6prd\n\t68iybAfxt6q7gy/Jk2S3lK8AWle+MhX4Aj7YcYazcC23l+G8TuUOmT+R/f3iMpdfy35J\n\tRzWoSo1qjemkEPraSua+LtAf+oM9boB5yEXCPrKGufGsMeZWIcbOVvECxGqPCW8JH837\n\taO0xpsb37eSoEvsflrFI58EOtHD8dxJLKEVRvUlNnAGLd2wscTCnQkagcMOkP7FYceDP\n\tL7xw==","X-Gm-Message-State":"AHYfb5gJmdl9ZFtnB3WRtlaHZGkIYK0mGcFyoYU3i6QmEK4bfYJ8AdTw\n\tksWnAXjw5NZAkJ+eRVT5hrbKLxg7exY8","X-Received":"by 10.202.6.193 with SMTP id 184mr7829876oig.269.1503890469953; \n\tSun, 27 Aug 2017 20:21:09 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170827153054.ijqxbjk25zpskojl@ninjato>","References":"\n\t\n\t<20170827153054.ijqxbjk25zpskojl@ninjato>","From":"Baolin Wang ","Date":"Mon, 28 Aug 2017 11:21:09 +0800","Message-ID":"","Subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","To":"Wolfram Sang ","Cc":"Baolin Wang ,\n\tMark Rutland , Rob Herring ,\n\tandriy.shevchenko@linux.intel.com, linux-i2c@vger.kernel.org,\n\tdevicetree@vger.kernel.org, LKML ,\n\tMark Brown ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1758632,"web_url":"http://patchwork.ozlabs.org/comment/1758632/","msgid":"<20170828151306.GA10688@katana>","list_archive_url":null,"date":"2017-08-28T15:13:06","subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","submitter":{"id":22495,"url":"http://patchwork.ozlabs.org/api/people/22495/","name":"Wolfram Sang","email":"wsa@the-dreams.de"},"content":"> >> + /*\n> >> + * If we did not get one ACK from slave when writing data, we should\n> >> + * dump all registers to check I2C status.\n> >\n> > Why? I would say no. NACK from a slave can always happen, e.g. when an\n> > EEPROM is busy erasing a page.\n> \n> For our I2C controller databook, if the master did not get one ACK\n> from slave when writing data to salve, we should send one STOP signal\n> to abort this data transfer or generate one repeated START signal to\n> start one new data transfer cycle. Considering our I2C usage\n\nYes, so far so good.\n\n> scenarios, we should dump registers to analyze I2C status and notify\n> to user to re-start new data transfer.\n\nI disagree here. You notify the users by returning -EIO. The upper layer\n(e.g. the i2c client driver) will handle it, like the EEPROM driver\nmight retry after a while. This all is expected behaviour, so no need to\nprint the registers to the logfile.\n\nIf you really, really want to keep it, make it debug output. But I think\nthe sentence \"we should dump all registers\" needs to be rephrased.\n\n> As I explained before, in our Spreadtrum platform, our regulator\n> driver will depend on I2C driver and the regulator driver uses\n> subsys_initcall() level to initialize. Moreover some other drivers\n> like GPU, they will depend on regulator to set voltage and they also\n> need initialization much earlier.\n> \n> Since it is arch_initcall() level, Andy suggested I should get rid of\n> tristate (use bool) and drop module.h here and all leftovers like\n> MODULE_*() calls including module_exit().\n\nI see. So the driver is really so essential for proper bootup that it is\nnot even allowed to be unloaded. I might make an exception here and\nallow arch_initcall() then. But I do wonder: did you try deferred\nprobing all over the place?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tsecure) header.d=mail.zeus03.de header.i=@mail.zeus03.de\n\theader.b=\"nYaTecPG\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xgwJL1w04z9s7F\n\tfor ;\n\tTue, 29 Aug 2017 01:13:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751347AbdH1PNK (ORCPT );\n\tMon, 28 Aug 2017 11:13:10 -0400","from www.zeus03.de ([194.117.254.33]:48082 \"EHLO mail.zeus03.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751232AbdH1PNJ (ORCPT );\n\tMon, 28 Aug 2017 11:13:09 -0400","(qmail 4167 invoked from network); 28 Aug 2017 17:13:07 +0200","from p200300cf5bc49300021de0fffea0c865.dip0.t-ipconnect.de (HELO\n\tlocalhost) (l3s3148p1@2003:cf:5bc4:9300:21d:e0ff:fea0:c865)\n\tby mail.zeus03.de with ESMTPSA (ECDHE-RSA-AES256-GCM-SHA384\n\tencrypted, authenticated); 28 Aug 2017 17:13:07 +0200"],"DKIM-Signature":"v=1; a=rsa-sha256; c=simple; d=mail.zeus03.de; h=date\n\t:from:to:cc:subject:message-id:references:mime-version\n\t:content-type:in-reply-to; s=k1; bh=TqJ6h3sS1c/2XqcUxaO1hEPRa9GM\n\tBJTkrt35Hpw7fiE=; b=nYaTecPGia1nxvxm4ZIou5HzfiXdErWMeFzSTR06D9E0\n\t7qQjGnbukDxrBl49/F+4JWBw7qlYhPG1RpyDl3HzCDt87+fIeB3V6oO2YITS9C+W\n\trNj3ssMCrFZMWBsgVUmktDE31WfcpSktN4SpoVSO1nq4jMG9N2fIDw30Vz7SEFs=","Date":"Mon, 28 Aug 2017 17:13:06 +0200","From":"Wolfram Sang ","To":"Baolin Wang ","Cc":"Baolin Wang ,\n\tMark Rutland , Rob Herring ,\n\tandriy.shevchenko@linux.intel.com, linux-i2c@vger.kernel.org,\n\tdevicetree@vger.kernel.org, LKML ,\n\tMark Brown ","Subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","Message-ID":"<20170828151306.GA10688@katana>","References":"\n\t\n\t<20170827153054.ijqxbjk25zpskojl@ninjato>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"LZvS9be/3tNcYl/X\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1758983,"web_url":"http://patchwork.ozlabs.org/comment/1758983/","msgid":"","list_archive_url":null,"date":"2017-08-29T01:58:14","subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","submitter":{"id":66012,"url":"http://patchwork.ozlabs.org/api/people/66012/","name":"Baolin Wang","email":"baolin.wang@linaro.org"},"content":"Hi Wolfram,\n\nOn 28 August 2017 at 23:13, Wolfram Sang wrote:\n>\n>> >> + /*\n>> >> + * If we did not get one ACK from slave when writing data, we should\n>> >> + * dump all registers to check I2C status.\n>> >\n>> > Why? I would say no. NACK from a slave can always happen, e.g. when an\n>> > EEPROM is busy erasing a page.\n>>\n>> For our I2C controller databook, if the master did not get one ACK\n>> from slave when writing data to salve, we should send one STOP signal\n>> to abort this data transfer or generate one repeated START signal to\n>> start one new data transfer cycle. Considering our I2C usage\n>\n> Yes, so far so good.\n>\n>> scenarios, we should dump registers to analyze I2C status and notify\n>> to user to re-start new data transfer.\n>\n> I disagree here. You notify the users by returning -EIO. The upper layer\n> (e.g. the i2c client driver) will handle it, like the EEPROM driver\n> might retry after a while. This all is expected behaviour, so no need to\n> print the registers to the logfile.\n>\n> If you really, really want to keep it, make it debug output. But I think\n> the sentence \"we should dump all registers\" needs to be rephrased.\n\nMake sense. I will remove the registers printing here.\n\n>\n>> As I explained before, in our Spreadtrum platform, our regulator\n>> driver will depend on I2C driver and the regulator driver uses\n>> subsys_initcall() level to initialize. Moreover some other drivers\n>> like GPU, they will depend on regulator to set voltage and they also\n>> need initialization much earlier.\n>>\n>> Since it is arch_initcall() level, Andy suggested I should get rid of\n>> tristate (use bool) and drop module.h here and all leftovers like\n>> MODULE_*() calls including module_exit().\n>\n> I see. So the driver is really so essential for proper bootup that it is\n> not even allowed to be unloaded. I might make an exception here and\n\nYes.\n\n> allow arch_initcall() then. But I do wonder: did you try deferred\n> probing all over the place?\n\nMany modules (like GPU) need set voltage earlier by regulator which\ndepends on I2C ( or we also need regulate voltage for big-cores ASAP),\nif we defer to set voltage for GPU or other modules, maybe will cause\nsome system problems. Thanks for your comments.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"KGhUjGSx\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhBck4Ydqz9s65\n\tfor ;\n\tTue, 29 Aug 2017 11:58:30 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751257AbdH2B6Q (ORCPT );\n\tMon, 28 Aug 2017 21:58:16 -0400","from mail-oi0-f46.google.com ([209.85.218.46]:33712 \"EHLO\n\tmail-oi0-f46.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751237AbdH2B6P (ORCPT\n\t); Mon, 28 Aug 2017 21:58:15 -0400","by mail-oi0-f46.google.com with SMTP id r203so17566334oih.0\n\tfor ; Mon, 28 Aug 2017 18:58:15 -0700 (PDT)","by 10.157.56.246 with HTTP; Mon, 28 Aug 2017 18:58:14 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=O9eUYRSPndE+oB4ZscleXOGWIlta9+laqsEYKmdab8w=;\n\tb=KGhUjGSxFiMBdpsR2WsoA8HhK+OWPnqLaLcon6qFPkwqaLFzNI4auWiSI0mMgMTrxl\n\tKuMH+pX/jl8WvvsZ3o42QCtlL6TWNh+JdXrOOQbbRrCC2vt/YMJdB+sp2PrygCbvKBLC\n\t3VVcDa0gd/2iEPVXtrcn1w4BbitkspIdWtCFg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=O9eUYRSPndE+oB4ZscleXOGWIlta9+laqsEYKmdab8w=;\n\tb=XuamA8bmpE1uH8HorZVpS3jXXqVOAnUokQWtiiPpLrto6V0qTryWVtzdY2lQkWBvWT\n\tmRN/kWYWVAaBPh3RCSblPR2sBQrA9WAkaD7Xfks2xXMWyMP2ctY/jlF9+E5gWGHSjnv6\n\tqUcwiYVuu7RC2KkeRmPhWf4BN/5uuOJc15S3PjO72u0UmD7RrdpBnI/FOUP2NSzdANAP\n\tN24jMChI3PjgfZZyEtv6BQxhOevegVu22h8LmU5+xMQLUkJ1y6KiVDub9fhmbdoIXjjg\n\tMYaBLt7n7E65NbyzZEdpqXu8BNxe7xCvDEGFcvdtkA35T3KShAfFkUapv/6TVEoUbxAP\n\tluIA==","X-Gm-Message-State":"AHYfb5jQIYxklRxSxbpyskVAGn+RmbY6DwKR5Irqgv6B7oIbcdpXMESi\n\tEeb618aDDHQnxaZqDuSKk9SdG6e0e014","X-Received":"by 10.202.197.14 with SMTP id v14mr2541111oif.88.1503971894720; \n\tMon, 28 Aug 2017 18:58:14 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170828151306.GA10688@katana>","References":"\n\t\n\t<20170827153054.ijqxbjk25zpskojl@ninjato>\n\t\n\t<20170828151306.GA10688@katana>","From":"Baolin Wang ","Date":"Tue, 29 Aug 2017 09:58:14 +0800","Message-ID":"","Subject":"Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver","To":"Wolfram Sang ","Cc":"Baolin Wang ,\n\tMark Rutland , Rob Herring ,\n\tandriy.shevchenko@linux.intel.com, linux-i2c@vger.kernel.org,\n\tdevicetree@vger.kernel.org, LKML ,\n\tMark Brown ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]