Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice

[Felipe Balbi]

On Tue, Feb 03, 2015 at 06:21:24PM -0200, Fabio Estevam wrote:
> Hi Felipe,
> 
> On Sun, Feb 1, 2015 at 3:37 PM, Felipe Balbi <ba...@ti.com> wrote:
> 
> > I like this, very much. Two comments though. We requested the gpio with
> > _optional(), and NULL is a valid gpio_desc, this if (nop->gpiod_reset)
> > is unnecessary. And also, since we don't have anymore the assert
> 
> But if the reset-gpios property is not present in the dts, then
> nop->gpiod_reset is NULL, and it is better not to call
> 'gpiod_direction_output(nop->gpiod_reset, 1)'  in this case, right?

it doesn't make a difference though, right ?
gpiod_direction_output(NULL, 1) won't do anything.

/me goes look at code

Man this is messed up. If you follow gpiod_get_optional, you'll end up
at gpiod_get_index_optional() which I quote below:

1989 struct gpio_desc *__must_check __gpiod_get_index_optional(struct device 
*dev,
1990                                                         const char *con_id,
1991                                                         unsigned int index,
1992                                                         enum gpiod_flags 
flags)
1993 {
1994         struct gpio_desc *desc;
1995 
1996         desc = gpiod_get_index(dev, con_id, index, flags);
1997         if (IS_ERR(desc)) {
1998                 if (PTR_ERR(desc) == -ENOENT)
1999                         return NULL;
2000         }
2001 
2002         return desc;
2003 }
2004 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);

So, if the error code is -ENOENT it returns NULL, that tells me NULL is
a valid gpio descriptor. If you follow gpiod_direction_output() however,
what you get is:

1072 int gpiod_direction_output(struct gpio_desc *desc, int value)
1073 {
1074         if (!desc || !desc->chip) {
1075                 pr_warn("%s: invalid GPIO\n", __func__);
1076                 return -EINVAL;
1077         }
1078         if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
1079                 value = !value;
1080         return _gpiod_direction_output_raw(desc, value);
1081 }
1082 EXPORT_SYMBOL_GPL(gpiod_direction_output);

That pr_warn() tells me NULL is *not* a valid gpio descriptor. Linus, is
that just something that was missed until now ?

> > argument, we should rename nop_reset_set() to nop_reset.
> 
> Agreed, will change it in v2.

Thanks

-- 
balbi

signature.asc
Description: Digital signature