Re: [PATCH 2/2] mtd: nand: sunxi: add reset line support

Philipp Zabel
2016-06-20T12:07:02Z

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@aosc.xyz> 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@aosc.xyz>
> > ---
> > 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.

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.

regards
Philipp