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

[Philipp Zabel]

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.

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