Re: [project-aspen-dev] [PATCH] PCI: histb: add power control GPIO for PCIe slot

[Shawn Guo <shawn.guo@linaro.org>]

Hi Daniel,

On Tue, Jan 23, 2018 at 09:39:44AM +0000, Daniel Thompson wrote:
> > @@ -335,15 +344,28 @@ static int histb_pcie_probe(struct platform_device *pdev)
> >  		return PTR_ERR(pci->dbi_base);
> >  	}
> >  
> > +	hipcie->power_gpio = of_get_named_gpio_flags(np,
> > +				"power-gpios", 0, &of_flags);
> > +	if (of_flags & OF_GPIO_ACTIVE_LOW)
> > +		flag |= GPIOF_ACTIVE_LOW;
> 
> Why isn't this inside the if statement?

Are you asking why the flag manipulation is not in the gpio_is_valid()
if-clause below?

I guess this is a copy of how reset_gpio is handled.  It might be a bit
more sensible to check the validity of the GPIO before handling the
flag, but practically the current code doesn't really hurt too much.

I will take Fabio's suggestion to reimplement it with a fixed regulator.
But thanks for the comment anyway.

Shawn

> > +	if (gpio_is_valid(hipcie->power_gpio)) {
> > +		ret = devm_gpio_request_one(dev, hipcie->power_gpio,
> > +				flag, "PCIe device power control");
> > +		if (ret) {
> > +			dev_err(dev, "unable to request power gpio\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	hipcie->reset_gpio = of_get_named_gpio_flags(np,
> >  				"reset-gpios", 0, &of_flags);
> >  	if (of_flags & OF_GPIO_ACTIVE_LOW)
> >  		flag |= GPIOF_ACTIVE_LOW;
> >  	if (gpio_is_valid(hipcie->reset_gpio)) {
> >  		ret = devm_gpio_request_one(dev, hipcie->reset_gpio,
> > -				flag, "PCIe device power control");
> > +				flag, "PCIe device reset control");
> >  		if (ret) {
> > -			dev_err(dev, "unable to request gpio\n");
> > +			dev_err(dev, "unable to request reset gpio\n");
> >  			return ret;
> >  		}
> >  	}
> > -- 
> > 1.9.1
> >