Hi Philipp, On Mon, 20 Jun 2016 14:05:54 +0200 Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: > Am Sonntag, den 19.06.2016, 14:06 +0200 schrieb Boris Brezillon: > > +Philipp > > > > On Sun, 19 Jun 2016 19:37:39 +0800 > > Icenowy Zheng <icenowy@xxxxxxxx> wrote: > > > > > The NAND controller on some sun8i chips needs its reset line to be deasserted > > > before they can enter working state. This commit added the reset line process > > > to the driver. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxxx> > > > --- > > > drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > > > index a83a690..1502748 100644 > > > --- a/drivers/mtd/nand/sunxi_nand.c > > > +++ b/drivers/mtd/nand/sunxi_nand.c > > > @@ -39,6 +39,7 @@ > > > #include <linux/gpio.h> > > > #include <linux/interrupt.h> > > > #include <linux/iopoll.h> > > > +#include <linux/reset.h> > > > > > > #define NFC_REG_CTL 0x0000 > > > #define NFC_REG_ST 0x0004 > > > @@ -269,6 +270,7 @@ struct sunxi_nfc { > > > void __iomem *regs; > > > struct clk *ahb_clk; > > > struct clk *mod_clk; > > > + struct reset_control *reset; > > > unsigned long assigned_cs; > > > unsigned long clk_rate; > > > struct list_head chips; > > > @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > > > if (ret) > > > goto out_ahb_clk_unprepare; > > > > > > + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > > > + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > > > + return PTR_ERR(nfc->reset); > > > > Actually you should test for != -ENOENT, because all error codes except > > this one should stop the ->probe(). > > > > BTW, this devm_reset_control_get_optional() is really weird. While most > > _optional() methods return NULL when the element is not defined in the > > DT, this one returns -ENOTENT, which makes it impossible to > > differentiate a real error from a undefined reset line (which is a > > valid case for _optional()). > > Of course it's possible, -ENOENT is only returned if the reset line is > not defined in the device tree. Okay, testing for err != -ENOENT is the right thing to do here. Maybe this could be documented. > > Note that gpiod_get_(index_)optional do nothing more that replacing > -ENOENT with NULL. And phydev_optional_get replaces -ENODEV with NULL. > And regulator_get_optional, if I understand it correctly, never returns > NULL. > > > Philipp, is there a good reason for doing that? > > Historically, NULL has not been a valid value for rstc. I suppose we > could add NULL checks to the reset_control_assert/deassert/reset/status > functions and align the reset API a bit with gpiod. I just wouldn't want > to see any IS_ERR_OR_NULL error handling in the drivers. Well, returning NULL is mainly a convenient way to differentiate real errors from missing optional reset lines (which are not errors). Now, if you say checking for -ENOENT is the right thing to do here, I'm fine with that. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html