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 <balbi@xxxxxx> 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
Attachment:
signature.asc
Description: Digital signature
