[{"id":3194018,"web_url":"http://patchwork.ozlabs.org/comment/3194018/","msgid":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","list_archive_url":null,"date":"2023-10-05T21:58:32","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":85956,"url":"http://patchwork.ozlabs.org/api/people/85956/","name":"Andi Shyti","email":"andi.shyti@kernel.org"},"content":"Hi Chris,\n\nLooks good, just a few questions.\n\n> +static int\n> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n> +{\n> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n> +\tint ret;\n> +\tu32 val;\n> +\n> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n> +\t\t\t\t\t1000, 5000);\n\nhere you are busy looping for 1ms between reads which is a long\ntime. Why not using read_poll_timeout() instead?\n\n> +\tif (ret) {\n> +\t\tdev_err(&adap->dev, \"recovery timeout\\n\");\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (val & MV64XXX_I2C_UNSTUCK_ERROR) {\n> +\t\tdev_err(&adap->dev, \"recovery failed\\n\");\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tdev_info(&adap->dev, \"recovery complete after %d pulses\\n\", MV64XXX_I2C_UNSTUCK_COUNT(val));\n\ndev_dbg?\n\n> +\treturn 0;\n> +}\n> +\n\n[...]\n\n> -\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\")) {\n> +\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\") ||\n> +\t    of_device_is_compatible(np, \"marvell,armada-8k-i2c\")) {\n\nshould this be part of a different patch?\n\n>  \t\tdrv_data->offload_enabled = false;\n>  \t\t/* The delay is only needed in standard mode (100kHz) */\n>  \t\tif (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)\n> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,\n>  }\n>  #endif /* CONFIG_OF */\n>  \n> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,\n> -\t\t\t\t\t  struct device *dev)\n> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,\n> +\t\t\t\t\t      struct device *dev)\n> +{\n> +\tstruct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;\n> +\n> +\tdev_info(dev, \"using FSM for recovery\\n\");\n\ndev_dbg?\n\n> +\trinfo->recover_bus = mv64xxx_i2c_recover_bus;\n> +\tdrv_data->adapter.bus_recovery_info = rinfo;\n> +\n> +\treturn 0;\n> +\n> +}\n> +\n\n[...]\n\n> +\t/* optional unstuck support */\n> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n> +\tif (res) {\n> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n\nOK, we failed to ioremap... but instead of returning an error,\nwouldn't it be better to just set unstuck_reg to NULL and move\nforward without unstuck support?\n\nMaybe you will stil crash later because something might have\nhappened, but failing on purpose on an optional feature looks a\nbit too drastic to me. What do you think?\n\nThanks,\nAndi","headers":{"Return-Path":"<linux-i2c-owner@vger.kernel.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=lG/Y1i8O;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-i2c-owner@vger.kernel.org; receiver=patchwork.ozlabs.org)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby legolas.ozlabs.org (Postfix) with ESMTP id 4S1lnF6SJ4z1yqD\n\tfor <incoming@patchwork.ozlabs.org>; Fri,  6 Oct 2023 08:58:41 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n        id S229735AbjJEV6l (ORCPT <rfc822;incoming@patchwork.ozlabs.org>);\n        Thu, 5 Oct 2023 17:58:41 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:35310 \"EHLO\n        lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n        with ESMTP id S229537AbjJEV6k (ORCPT\n        <rfc822;linux-i2c@vger.kernel.org>); Thu, 5 Oct 2023 17:58:40 -0400","from smtp.kernel.org (relay.kernel.org [52.25.139.140])\n        by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8CDF995;\n        Thu,  5 Oct 2023 14:58:39 -0700 (PDT)","by smtp.kernel.org (Postfix) with ESMTPSA id 44926C433C8;\n        Thu,  5 Oct 2023 21:58:37 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n        s=k20201202; t=1696543119;\n        bh=wg9da7L1CQtw+tuYOmZfz5J+NgLFvScI+3C0EoG/geo=;\n        h=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n        b=lG/Y1i8OpAQPQat88kfyzi7NWqcNQ9Cz13NjQkWCQDOP2ullmbktnb7+j7T7OUkOb\n         f5JlDNF7/gNrGv6WsIbY9PvLXmfivEukYrAgLUezsgB+ory5ukH15S8Xv4FryuoHvm\n         cV6XP4Z6Hh8AKexq5/ZCHA5kDyDTcR1bhFJGILAMXqivj54d0rEwKfK6rNAAZDIKju\n         4UmdJQrB8vGY0vpbClN4hFjfJpvZNUa0FqIuD82KqRjuSbdHiAIHw7AxVLYJ98X3s9\n         Ahf54l68UPAnMvovAyIJZLRuXff0bqHURlk7YlgOcDStezWyy6odd8F6NpHTigPFWo\n         88Q1dlHsLuGAw==","Date":"Thu, 5 Oct 2023 23:58:32 +0200","From":"Andi Shyti <andi.shyti@kernel.org>","To":"Chris Packham <chris.packham@alliedtelesis.co.nz>","Cc":"gregory.clement@bootlin.com, robh+dt@kernel.org,\n        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,\n        pierre.gondois@arm.com, linux-i2c@vger.kernel.org,\n        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Message-ID":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>","X-Spam-Status":"No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,\n        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,\n        SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n        lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"<linux-i2c.vger.kernel.org>","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":3194031,"web_url":"http://patchwork.ozlabs.org/comment/3194031/","msgid":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","list_archive_url":null,"date":"2023-10-05T22:39:03","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":27499,"url":"http://patchwork.ozlabs.org/api/people/27499/","name":"Chris Packham","email":"chris.packham@alliedtelesis.co.nz"},"content":"On 6/10/23 10:58, Andi Shyti wrote:\n> Hi Chris,\n>\n> Looks good, just a few questions.\n>\n>> +static int\n>> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n>> +{\n>> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n>> +\tint ret;\n>> +\tu32 val;\n>> +\n>> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n>> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n>> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n>> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n>> +\t\t\t\t\t1000, 5000);\n> here you are busy looping for 1ms between reads which is a long\n> time. Why not using read_poll_timeout() instead?\n\nI needed to use the atomic variant because this ends up getting called \nfrom an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I \nprobably don't need to wait so long between reads those times were just \npulled out of thin air. In my experimentation the faults that can be \ncleared do so within a couple of clocks, if it hasn't cleared within 8 \nclocks it's not going to.\n\n>> +\tif (ret) {\n>> +\t\tdev_err(&adap->dev, \"recovery timeout\\n\");\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (val & MV64XXX_I2C_UNSTUCK_ERROR) {\n>> +\t\tdev_err(&adap->dev, \"recovery failed\\n\");\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tdev_info(&adap->dev, \"recovery complete after %d pulses\\n\", MV64XXX_I2C_UNSTUCK_COUNT(val));\n> dev_dbg?\nack.\n>> +\treturn 0;\n>> +}\n>> +\n> [...]\n>\n>> -\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\")) {\n>> +\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\") ||\n>> +\t    of_device_is_compatible(np, \"marvell,armada-8k-i2c\")) {\n> should this be part of a different patch?\n\nYes sorry. Originally I was going to use a new compatible to indicate \nthe unstuck support but went with the 2nd reg cell so this is unnecessary.\n\n>\n>>   \t\tdrv_data->offload_enabled = false;\n>>   \t\t/* The delay is only needed in standard mode (100kHz) */\n>>   \t\tif (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)\n>> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,\n>>   }\n>>   #endif /* CONFIG_OF */\n>>   \n>> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,\n>> -\t\t\t\t\t  struct device *dev)\n>> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,\n>> +\t\t\t\t\t      struct device *dev)\n>> +{\n>> +\tstruct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;\n>> +\n>> +\tdev_info(dev, \"using FSM for recovery\\n\");\n> dev_dbg?\n>\n>> +\trinfo->recover_bus = mv64xxx_i2c_recover_bus;\n>> +\tdrv_data->adapter.bus_recovery_info = rinfo;\n>> +\n>> +\treturn 0;\n>> +\n>> +}\n>> +\n> [...]\n>\n>> +\t/* optional unstuck support */\n>> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n>> +\tif (res) {\n>> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n>> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n>> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n> OK, we failed to ioremap... but instead of returning an error,\n> wouldn't it be better to just set unstuck_reg to NULL and move\n> forward without unstuck support?\n>\n> Maybe you will stil crash later because something might have\n> happened, but failing on purpose on an optional feature looks a\n> bit too drastic to me. What do you think?\n\nPersonally I think if the reg property is supplied in the dts we'd \nbetter be able to use it. If the feature is not wanted then the way to \nindicate this is by supplying only one reg cell.\n\nI'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get \nthis landed.\n\n>\n> Thanks,\n> Andi","headers":{"Return-Path":"<linux-i2c-owner@vger.kernel.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=alliedtelesis.co.nz header.i=@alliedtelesis.co.nz\n header.a=rsa-sha256 header.s=mail181024 header.b=E1XFAgsZ;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-i2c-owner@vger.kernel.org; receiver=patchwork.ozlabs.org)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby legolas.ozlabs.org (Postfix) with ESMTP id 4S1mh50Gwyz1yqD\n\tfor <incoming@patchwork.ozlabs.org>; Fri,  6 Oct 2023 09:39:17 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n        id S229865AbjJEWjM (ORCPT <rfc822;incoming@patchwork.ozlabs.org>);\n        Thu, 5 Oct 2023 18:39:12 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:40738 \"EHLO\n        lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n        with ESMTP id S230357AbjJEWjL (ORCPT\n        <rfc822;linux-i2c@vger.kernel.org>); Thu, 5 Oct 2023 18:39:11 -0400","from gate2.alliedtelesis.co.nz (gate2.alliedtelesis.co.nz\n [202.36.163.20])\n        by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E7DC8D6\n        for <linux-i2c@vger.kernel.org>; Thu,  5 Oct 2023 15:39:08 -0700 (PDT)","from svr-chch-seg1.atlnz.lc (mmarshal3.atlnz.lc [10.32.18.43])\n        (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n         key-exchange X25519 server-signature RSA-PSS (4096 bits)\n server-digest SHA256)\n        (Client did not present a certificate)\n        by gate2.alliedtelesis.co.nz (Postfix) with ESMTPS id DDFD42C07F0;\n        Fri,  6 Oct 2023 11:39:04 +1300 (NZDT)","from svr-chch-ex2.atlnz.lc (Not Verified[2001:df5:b000:bc8::76]) by\n svr-chch-seg1.atlnz.lc with Trustwave SEG (v8,2,6,11305)\n        id <B651f3b080001>; Fri, 06 Oct 2023 11:39:04 +1300","from svr-chch-ex1.atlnz.lc (2001:df5:b000:bc8::77) by\n svr-chch-ex2.atlnz.lc (2001:df5:b000:bc8::76) with Microsoft SMTP Server\n (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id\n 15.2.1118.37; Fri, 6 Oct 2023 11:39:04 +1300","from svr-chch-ex2.atlnz.lc (2001:df5:b000:bc8::76) by\n svr-chch-ex1.atlnz.lc (2001:df5:b000:bc8::77) with Microsoft SMTP Server\n (TLS) id 15.0.1497.48; Fri, 6 Oct 2023 11:39:04 +1300","from svr-chch-ex2.atlnz.lc ([fe80::a9eb:c9b7:8b52:9567]) by\n svr-chch-ex2.atlnz.lc ([fe80::a9eb:c9b7:8b52:9567%15]) with mapi id\n 15.02.1118.037; Fri, 6 Oct 2023 11:39:04 +1300"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=alliedtelesis.co.nz;\n        s=mail181024; t=1696545544;\n        bh=Jln0tzUpoR2xr/aeOoHAnxoXo48BHvn0YAHWLviV5iA=;\n        h=From:To:CC:Subject:Date:References:In-Reply-To:From;\n        b=E1XFAgsZxIZ6ukokAFv30wxjhY9eMRuvSAg2OsJM5WbeqXUceAeGCbXvFbE4C3aNv\n         8klwjIHEbo6/BIj8NSnYX7dz7ZiT8saxcrtGtLf4fqZrkma/krIK48VqCkNPZvjmCv\n         iy2nPoAhbv83VU5ICi+PddeFUfnnjx3ZCVdCkQCha4nwBUvGjIS5TLYXivjA4FwWaA\n         THaCSdMa12jUmqHp3bW00gTibJ0DiMoMg9qhEFPrJcLD2qMUb7K1000Lu3bbiqi6eq\n         XlgnJnjCREXpNtpx9YPsIn7TQdfAKOxUqsFWMlSqVWOlcTcMF7U1jurWvZVWEtBAAd\n         ZfcrAiho28kog==","From":"Chris Packham <Chris.Packham@alliedtelesis.co.nz>","To":"Andi Shyti <andi.shyti@kernel.org>","CC":"\"gregory.clement@bootlin.com\" <gregory.clement@bootlin.com>,\n        \"robh+dt@kernel.org\" <robh+dt@kernel.org>,\n        \"krzysztof.kozlowski+dt@linaro.org\"\n        <krzysztof.kozlowski+dt@linaro.org>,\n        \"conor+dt@kernel.org\" <conor+dt@kernel.org>,\n        \"pierre.gondois@arm.com\" <pierre.gondois@arm.com>,\n        \"linux-i2c@vger.kernel.org\" <linux-i2c@vger.kernel.org>,\n        \"devicetree@vger.kernel.org\" <devicetree@vger.kernel.org>,\n        \"linux-kernel@vger.kernel.org\" <linux-kernel@vger.kernel.org>","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Thread-Topic":"[PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Thread-Index":"AQHZ8NPnYKzvy/9jwESCWo7X3TBvqLA68pMAgAALUoA=","Date":"Thu, 5 Oct 2023 22:39:03 +0000","Message-ID":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>\n <20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","In-Reply-To":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","Accept-Language":"en-NZ, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.33.22.30]","Content-Type":"text/plain; charset=\"utf-8\"","Content-ID":"<EFDC78327C2D514A97030AEF06C8B8A5@atlnz.lc>","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-SEG-SpamProfiler-Analysis":"v=2.3 cv=Vf2Jw2h9 c=1 sm=1 tr=0\n a=Xf/6aR1Nyvzi7BryhOrcLQ==:117 a=xqWC_Br6kY4A:10 a=75chYTbOgJ0A:10\n a=IkcTkHD0fZMA:10 a=bhdUkHdE2iEA:10 a=FqOU3ekQ0Ek1LeW_-iAA:9\n a=QEXdDO2ut3YA:10","X-SEG-SpamProfiler-Score":"0","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,\n        SPF_HELO_PASS,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no\n        version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n        lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"<linux-i2c.vger.kernel.org>","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":3194041,"web_url":"http://patchwork.ozlabs.org/comment/3194041/","msgid":"<20231005230737.6j57y7d27vnsbws3@zenone.zhora.eu>","list_archive_url":null,"date":"2023-10-05T23:07:37","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":85956,"url":"http://patchwork.ozlabs.org/api/people/85956/","name":"Andi Shyti","email":"andi.shyti@kernel.org"},"content":"Hi Chris,\n\n> >> +static int\n> >> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n> >> +{\n> >> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n> >> +\tint ret;\n> >> +\tu32 val;\n> >> +\n> >> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n> >> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n> >> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n> >> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n> >> +\t\t\t\t\t1000, 5000);\n> > here you are busy looping for 1ms between reads which is a long\n> > time. Why not using read_poll_timeout() instead?\n> \n> I needed to use the atomic variant because this ends up getting called \n> from an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I \n> probably don't need to wait so long between reads those times were just \n> pulled out of thin air. In my experimentation the faults that can be \n> cleared do so within a couple of clocks, if it hasn't cleared within 8 \n> clocks it's not going to.\n\nIt's still a long time to wait in atomic context...\nreadl_poll_timeout_atomic() waits in udelays, where the maximum\naccepted waiting time is 10us. Here you are waiting 100 times\nmore.\n\nIf we can't be within that value I would rather use a thread.\n\nOr, you could also consider using threaded_irq()... but this\nmight have a bit of a higher impact.\n\n[...]\n\n> >> +\t/* optional unstuck support */\n> >> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n> >> +\tif (res) {\n> >> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n> >> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n> >> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n> > OK, we failed to ioremap... but instead of returning an error,\n> > wouldn't it be better to just set unstuck_reg to NULL and move\n> > forward without unstuck support?\n> >\n> > Maybe you will stil crash later because something might have\n> > happened, but failing on purpose on an optional feature looks a\n> > bit too drastic to me. What do you think?\n> \n> Personally I think if the reg property is supplied in the dts we'd \n> better be able to use it. If the feature is not wanted then the way to \n> indicate this is by supplying only one reg cell.\n> \n> I'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get \n> this landed.\n\nDon't ahve a strong opinion... as you like. Mine is just an\nopinion and your argument is valid :-)\n\nAndi","headers":{"Return-Path":"<linux-i2c-owner@vger.kernel.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=eqVpbGcZ;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-i2c-owner@vger.kernel.org; receiver=patchwork.ozlabs.org)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby legolas.ozlabs.org (Postfix) with ESMTP id 4S1nNc4Nj1z1yq9\n\tfor <incoming@patchwork.ozlabs.org>; Fri,  6 Oct 2023 10:10:56 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n        id S229550AbjJEXKV (ORCPT <rfc822;incoming@patchwork.ozlabs.org>);\n        Thu, 5 Oct 2023 19:10:21 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:38670 \"EHLO\n        lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n        with ESMTP id S229670AbjJEXIB (ORCPT\n        <rfc822;linux-i2c@vger.kernel.org>); Thu, 5 Oct 2023 19:08:01 -0400","from smtp.kernel.org (relay.kernel.org [52.25.139.140])\n        by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5845211B;\n        Thu,  5 Oct 2023 16:07:43 -0700 (PDT)","by smtp.kernel.org (Postfix) with ESMTPSA id 66543C433C7;\n        Thu,  5 Oct 2023 23:07:41 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n        s=k20201202; t=1696547262;\n        bh=SLxoCaj8+sjzohhZAKAeitCTwO69mwCyvpBXx/t3mn8=;\n        h=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n        b=eqVpbGcZYRKBnm1peUNSFdaEbGBkiVzzK9XT0+U9GtZSBzyVT0XT1/fzM3QYQIzD7\n         vp5vXGgdVO60OwsJDq2ekLnAWy6jMdflpjNR1ZN3hZKSUSyrOJ73vLU/ULSgUW2etD\n         a6R32TzhXCg0Z6lGQ+QIsQK5IWGpCKZ/cfWr147n68P4T5zOZVA8QQhjLNJkFwM5Jg\n         sSbEJ+TXGxZSgB87hJpTNNdNav1mL6YthEqe8ZqDpehIRgd8kIwFyavyJt62J1DMH7\n         Q3k4e3SEynwm59DGiIZoUlRjI4/+Ho2UFrFfg7y/HV5HJt98OtESzPpgP6bHyId4GA\n         1i0TZKOZ/Mjew==","Date":"Fri, 6 Oct 2023 01:07:37 +0200","From":"Andi Shyti <andi.shyti@kernel.org>","To":"Chris Packham <Chris.Packham@alliedtelesis.co.nz>","Cc":"\"gregory.clement@bootlin.com\" <gregory.clement@bootlin.com>,\n        \"robh+dt@kernel.org\" <robh+dt@kernel.org>,\n        \"krzysztof.kozlowski+dt@linaro.org\"\n        <krzysztof.kozlowski+dt@linaro.org>,\n        \"conor+dt@kernel.org\" <conor+dt@kernel.org>,\n        \"pierre.gondois@arm.com\" <pierre.gondois@arm.com>,\n        \"linux-i2c@vger.kernel.org\" <linux-i2c@vger.kernel.org>,\n        \"devicetree@vger.kernel.org\" <devicetree@vger.kernel.org>,\n        \"linux-kernel@vger.kernel.org\" <linux-kernel@vger.kernel.org>","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Message-ID":"<20231005230737.6j57y7d27vnsbws3@zenone.zhora.eu>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>\n <20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>\n <57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","X-Spam-Status":"No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,\n        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,\n        SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n        lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"<linux-i2c.vger.kernel.org>","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]