Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray <andrew.murray@arm.com>]

On Mon, Nov 18, 2019 at 01:10:58PM +0000, Robin Murphy wrote:
> On 18/11/2019 12:39 pm, Andrew Murray wrote:
> > On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> > > On 18/11/2019 11:59 am, Mark Brown wrote:
> > > > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > > > splattered everywhere.
> > > > 
> > > > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > > > +	if (!rockchip->vpcie3v3)
> > > > >    		return;
> > > > >    	/*
> > > > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > > > >    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > > > >    			return PTR_ERR(rockchip->vpcie12v);
> > > > >    		dev_info(dev, "no vpcie12v regulator found\n");
> > > > > +		rockchip->vpcie12v = NULL;
> > > > 
> > > > According to the API NULL is a valid regulator.  We don't currently
> > > > actually do this but it's storing up surprises if you treat it as
> > > > invalid.
> > > 
> > > Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> > > immediately dereferencing its argument without any checks. If we'd rather
> > > not bake in that assumption then this patch can happily be ignored.
> > 
> > I'd suggest we drop this patch.
> > 
> > "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> > -4095 inclusive.
> 
> Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition quoted
> above ;)

Doh.

Andrew Murray

> 
> But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's
> just forget about it to avoid any possible confusion.
> 
> Cheers,
> Robin.