[{"id":1768195,"web_url":"http://patchwork.ozlabs.org/comment/1768195/","msgid":"<20170913212639.2zyvg24364ovwfxh@ninjato>","list_archive_url":null,"date":"2017-09-13T21:26:39","subject":"Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add 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 this driver!\n\n> +/**\n> + * struct stm32f7_i2c_spec - private i2c specification timing\n> + * @rate: I2C bus speed (Hz)\n> + * @rate_min: 80% of I2C bus speed (Hz)\n> + * @rate_max: 120% of I2C bus speed (Hz)\n\nYou would generate a clock which is higher than the requested one?\nThis is highly unusual. Any special reason?\n\n> + * @fall_max: Max fall time of both SDA and SCL signals (ns)\n> + * @rise_max: Max rise time of both SDA and SCL signals (ns)\n> + * @hddat_min: Min data hold time (ns)\n> + * @vddat_max: Max data valid time (ns)\n> + * @sudat_min: Min data setup time (ns)\n> + * @l_min: Min low period of the SCL clock (ns)\n> + * @h_min: Min high period of the SCL clock (ns)\n> + */\n> +static struct stm32f7_i2c_spec i2c_specs[] = {\n> +\t[STM32_I2C_SPEED_STANDARD] = {\n> +\t\t.rate = 100000,\n> +\t\t.rate_min = 8000,\n\nThis is not 80%. Typo?\n\n> +\t\t.rate_max = 120000,\n> +\t\t.fall_max = 300,\n> +\t\t.rise_max = 1000,\n> +\t\t.hddat_min = 0,\n> +\t\t.vddat_max = 3450,\n> +\t\t.sudat_min = 250,\n> +\t\t.l_min = 4700,\n> +\t\t.h_min = 4000,\n> +\t},\n\n...\n\n> +\t/*\n> +\t * Among Prescaler possibilities discovered above figures out SCL Low\n> +\t * and High Period. Provided:\n> +\t * - SCL Low Period has to be higher than Low Period of tehs SCL Clock\n\ntehs?\n\n> +\t * defined by I2C Specification. I2C Clock has to be lower than\n> +\t * (SCL Low Period - Analog/Digital filters) / 4.\n> +\t * - SCL High Period has to be lower than High Period of the SCL Clock\n> +\t * defined by I2C Specification\n> +\t * - I2C Clock has to be lower than SCL High Period\n> +\t */\n\n...\n\n> +\t/* NACK received */\n> +\tif (status & STM32F7_I2C_ISR_NACKF) {\n> +\t\tdev_dbg(i2c_dev->dev, \"<%s>: Receive NACK\\n\", __func__);\n> +\t\twritel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);\n> +\t\tf7_msg->result = -EBADE;\n\n-ENXIO (see Documentation/i2c/fault-codes)\n\n...\n\n> +\ttimeout = wait_for_completion_timeout(&i2c_dev->complete,\n> +\t\t\t\t\t i2c_dev->adap.timeout);\n> +\tret = f7_msg->result;\n> +\n> +\tif (!timeout) {\n> +\t\tdev_dbg(i2c_dev->dev, \"Access to slave 0x%x timed out\\n\",\n> +\t\t\ti2c_dev->msg->addr);\n> +\t\tret = -ETIMEDOUT;\n> +\t}\n\nCould you rename the variable to time_left? It looks strange, basically:\n\n\tif (!timeout)\n\t\treturn -ETIMEDOUT\n\n...\n\n> +\tadap->retries = 0;\n\nWhy no retries when you check for arbitration lost?\n\n> +\tadap->algo = &stm32f7_i2c_algo;\n> +\tadap->dev.parent = &pdev->dev;\n> +\tadap->dev.of_node = pdev->dev.of_node;\n> +\n> +\tinit_completion(&i2c_dev->complete);\n> +\n> +\tret = i2c_add_adapter(adap);\n> +\tif (ret) {\n> +\t\tdev_err(&pdev->dev, \"Failed to add adapter\\n\");\n\nPlease remove, the core will print info when adding fails.\n\n\nRest looks good!\n\nThanks,\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 3xsvqm3ls2z9sMN\n\tfor ;\n\tThu, 14 Sep 2017 07:26:44 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751125AbdIMV0m (ORCPT );\n\tWed, 13 Sep 2017 17:26:42 -0400","from sauhun.de ([88.99.104.3]:43526 \"EHLO pokefinder.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751121AbdIMV0l (ORCPT );\n\tWed, 13 Sep 2017 17:26:41 -0400","from localhost (p54B333DB.dip0.t-ipconnect.de [84.179.51.219])\n\tby pokefinder.org (Postfix) with ESMTPSA id 21A9B2C3256;\n\tWed, 13 Sep 2017 23:26:40 +0200 (CEST)"],"Date":"Wed, 13 Sep 2017 23:26:39 +0200","From":"Wolfram Sang ","To":"Pierre-Yves MORDRET ","Cc":"Rob Herring , Mark Rutland ,\n\tMaxime Coquelin ,\n\tAlexandre Torgue ,\n\tRussell King ,\n\tlinux-i2c@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org","Subject":"Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add driver","Message-ID":"<20170913212639.2zyvg24364ovwfxh@ninjato>","References":"<1504251255-20469-1-git-send-email-pierre-yves.mordret@st.com>\n\t<1504251255-20469-4-git-send-email-pierre-yves.mordret@st.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"3nttgcsxruquswtt\"","Content-Disposition":"inline","In-Reply-To":"<1504251255-20469-4-git-send-email-pierre-yves.mordret@st.com>","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":1768405,"web_url":"http://patchwork.ozlabs.org/comment/1768405/","msgid":"<63ecfac9-990b-e6c5-d9bf-7708bacc10d2@st.com>","list_archive_url":null,"date":"2017-09-14T08:11:11","subject":"Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add driver","submitter":{"id":71499,"url":"http://patchwork.ozlabs.org/api/people/71499/","name":"Pierre Yves MORDRET","email":"pierre-yves.mordret@st.com"},"content":"On 09/13/2017 11:26 PM, Wolfram Sang wrote:\n> Hi,\n> \n> thanks for this driver!\n> \n>> +/**\n>> + * struct stm32f7_i2c_spec - private i2c specification timing\n>> + * @rate: I2C bus speed (Hz)\n>> + * @rate_min: 80% of I2C bus speed (Hz)\n>> + * @rate_max: 120% of I2C bus speed (Hz)\n> \n> You would generate a clock which is higher than the requested one?\n> This is highly unusual. Any special reason?\n\nWell. I allow the clock to be higher than expected.\nLooking at I2C spec again it turns out the mode specifies the max: no overshoot\nof the clock. I will lock max to 100% then.\nWill be fixed\n\n> \n>> + * @fall_max: Max fall time of both SDA and SCL signals (ns)\n>> + * @rise_max: Max rise time of both SDA and SCL signals (ns)\n>> + * @hddat_min: Min data hold time (ns)\n>> + * @vddat_max: Max data valid time (ns)\n>> + * @sudat_min: Min data setup time (ns)\n>> + * @l_min: Min low period of the SCL clock (ns)\n>> + * @h_min: Min high period of the SCL clock (ns)\n>> + */\n>> +static struct stm32f7_i2c_spec i2c_specs[] = {\n>> +\t[STM32_I2C_SPEED_STANDARD] = {\n>> +\t\t.rate = 100000,\n>> +\t\t.rate_min = 8000,\n> \n> This is not 80%. Typo?\n\nYep. This is a typo\n\n> \n>> +\t\t.rate_max = 120000,\n>> +\t\t.fall_max = 300,\n>> +\t\t.rise_max = 1000,\n>> +\t\t.hddat_min = 0,\n>> +\t\t.vddat_max = 3450,\n>> +\t\t.sudat_min = 250,\n>> +\t\t.l_min = 4700,\n>> +\t\t.h_min = 4000,\n>> +\t},\n> \n> ...\n> \n>> +\t/*\n>> +\t * Among Prescaler possibilities discovered above figures out SCL Low\n>> +\t * and High Period. Provided:\n>> +\t * - SCL Low Period has to be higher than Low Period of tehs SCL Clock\n> \n> tehs?\n\nOops.\n\n> \n>> +\t * defined by I2C Specification. I2C Clock has to be lower than\n>> +\t * (SCL Low Period - Analog/Digital filters) / 4.\n>> +\t * - SCL High Period has to be lower than High Period of the SCL Clock\n>> +\t * defined by I2C Specification\n>> +\t * - I2C Clock has to be lower than SCL High Period\n>> +\t */\n> \n> ...\n> \n>> +\t/* NACK received */\n>> +\tif (status & STM32F7_I2C_ISR_NACKF) {\n>> +\t\tdev_dbg(i2c_dev->dev, \"<%s>: Receive NACK\\n\", __func__);\n>> +\t\twritel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);\n>> +\t\tf7_msg->result = -EBADE;\n> \n> -ENXIO (see Documentation/i2c/fault-codes)\n\nOK\n\n> \n> ...\n> \n>> +\ttimeout = wait_for_completion_timeout(&i2c_dev->complete,\n>> +\t\t\t\t\t i2c_dev->adap.timeout);\n>> +\tret = f7_msg->result;\n>> +\n>> +\tif (!timeout) {\n>> +\t\tdev_dbg(i2c_dev->dev, \"Access to slave 0x%x timed out\\n\",\n>> +\t\t\ti2c_dev->msg->addr);\n>> +\t\tret = -ETIMEDOUT;\n>> +\t}\n> \n> Could you rename the variable to time_left? It looks strange, basically:\n> \n> \tif (!timeout)\n> \t\treturn -ETIMEDOUT\n> \n\nokay\n\n> ...\n> \n>> +\tadap->retries = 0;\n> \n> Why no retries when you check for arbitration lost?\n> \n>> +\tadap->algo = &stm32f7_i2c_algo;\n>> +\tadap->dev.parent = &pdev->dev;\n>> +\tadap->dev.of_node = pdev->dev.of_node;\n>> +\n>> +\tinit_completion(&i2c_dev->complete);\n>> +\n>> +\tret = i2c_add_adapter(adap);\n>> +\tif (ret) {\n>> +\t\tdev_err(&pdev->dev, \"Failed to add adapter\\n\");\n> \n> Please remove, the core will print info when adding fails.\n> \n\nI will\n\n> \n> Rest looks good!\n\nGreat !\n\n> \n> Thanks,\n> \n> Wolfram\n> \n\nThanks","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 3xtB8Y3V2qz9t1t\n\tfor ;\n\tThu, 14 Sep 2017 18:12:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751335AbdINIMK (ORCPT );\n\tThu, 14 Sep 2017 04:12:10 -0400","from mx08-00178001.pphosted.com ([91.207.212.93]:35146 \"EHLO\n\tmx07-00178001.pphosted.com\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1751206AbdINIMJ (ORCPT\n\t); Thu, 14 Sep 2017 04:12:09 -0400","from pps.filterd (m0046660.ppops.net [127.0.0.1])\n\tby mx08-.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8E898vS026290; Thu, 14 Sep 2017 10:11:24 +0200","from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35])\n\tby mx08-00178001.pphosted.com with ESMTP id 2cv5e55xef-1\n\t(version=TLSv1 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NOT);\n\tThu, 14 Sep 2017 10:11:24 +0200","from zeta.dmz-eu.st.com (zeta.dmz-eu.st.com [164.129.230.9])\n\tby beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 8DA8534;\n\tThu, 14 Sep 2017 08:11:22 +0000 (GMT)","from Webmail-eu.st.com (sfhdag5node2.st.com [10.75.127.14])\n\tby zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 5F12A15BA;\n\tThu, 14 Sep 2017 08:11:22 +0000 (GMT)","from [10.201.23.236] (10.75.127.45) by SFHDAG5NODE2.st.com\n\t(10.75.127.14) with Microsoft SMTP Server (TLS) id 15.0.1178.4;\n\tThu, 14 Sep 2017 10:11:21 +0200"],"Subject":"Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add driver","To":"Wolfram Sang ","CC":"Rob Herring , Mark Rutland ,\n\tMaxime Coquelin ,\n\tAlexandre Torgue ,\n\tRussell King ,\n\t, ,\n\t, ","References":"<1504251255-20469-1-git-send-email-pierre-yves.mordret@st.com>\n\t<1504251255-20469-4-git-send-email-pierre-yves.mordret@st.com>\n\t<20170913212639.2zyvg24364ovwfxh@ninjato>","From":"Pierre Yves MORDRET ","Message-ID":"<63ecfac9-990b-e6c5-d9bf-7708bacc10d2@st.com>","Date":"Thu, 14 Sep 2017 10:11:11 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170913212639.2zyvg24364ovwfxh@ninjato>","Content-Type":"text/plain; charset=\"windows-1252\"","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Originating-IP":"[10.75.127.45]","X-ClientProxiedBy":"SFHDAG1NODE3.st.com (10.75.127.3) To SFHDAG5NODE2.st.com\n\t(10.75.127.14)","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-14_02:, , signatures=0","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]