Messages in this thread | | | Date | Thu, 23 Feb 2023 10:30:13 +0100 | Subject | Re: [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register mode driver | From | Krzysztof Kozlowski <> |
| |
On 23/02/2023 01:58, Ryan Chen wrote: > Hello Krzysztof, > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Wednesday, February 22, 2023 4:28 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring >> <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew >> Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; >> openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register >> mode driver >> >> On 22/02/2023 04:36, Ryan Chen wrote: >> >>>>> + >>>>> + return 0; >>>>> + >>>>> +free_irq: >>>>> + devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus); >>>> >>>> Why? >>>> >>>>> +unmap: >>>>> + devm_iounmap(&pdev->dev, i2c_bus->reg_base); >>>> >>>> Why? >>>> >>>>> +free_mem: >>>>> + devm_kfree(&pdev->dev, i2c_bus); >>>> >>>> Why? >>>> >>> >>> Sorry, those are probe following, if any error, will goto this label. >>> To release mem/unmap/free_irq. Is this unnecessary? >> >> Releasing managed resources is usualyl unnecessary. Therefore I am asking >> why do you think it is necessary here? >> >>> I saw many driver submit is remove all probe fail goto label, is just return >> ERR. >>> Do you mean I direct go for this way? >> >> Why would you do it differently? > > Thanks, I will remove those labels, and modify to dev_err_probe in previous probe return. > >>> >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int ast2600_i2c_remove(struct platform_device *pdev) { >>>>> + struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev); >>>>> + >>>>> + /* Disable everything. */ >>>>> + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); >>>>> + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER); >>>>> + >>>>> + devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus); >>>>> + >>>>> + i2c_del_adapter(&i2c_bus->adap); >>>> >>>> Wrong order of cleanup. It should be reversed to the probe, unless >>>> you have some reason, but then please explain. >>> >>> Sorry, this in remove function, it should do i2c_del_adapter(&i2c_bus->adap) >> in the end. >>> Why this should revered to probe? >> >> Because it's logical, you do the same with error paths of probe, it it usually the >> only way to make sure some of the resources are not used by some other piece >> (e.g. interrupt handler is called while i2c adapter is unregistered). > > Sorry, I can't catch your point. > Do you mean remove devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus); > Keep i2c_del_adapter(&i2c_bus->adap) here, because interrupt is called while i2c adapter is unregistered ?
Again, maybe clearer, actions should be in reversed order comparing to the probe actions. Why would you remove pieces of code from here if I asked to do them in different order?
Best regards, Krzysztof
| |