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
> >
