Re: [PATCH] ARM: Kirkwood: fix unused mvsdio gpio pins

[Sebastian Hesselbarth]

On 03/23/2013 05:30 PM, Thomas Petazzoni wrote:
> Dear Sebastian Hesselbarth,
>
> On Sat, 23 Mar 2013 16:25:54 +0100, Sebastian Hesselbarth wrote:
>
> > I understand that you proposed patch fixes mvsdio grab mpp0 by accident.
> > But what if you have a kirkwood board where cd-gpio _is_ connected to mpp0?
>
> It didn't work with the existing mvsdio driver, so the purpose of my
> patch was merely to restore the old behavior, in order to avoid having
> to change all the instances of mvsdio_platform_data, knowing that those
> would anyway go away as we convert boards to the Device Tree.

Thomas,

I agree both approaches fix the same issue. I haven't seen your patch
earlier and was just proposing a patch for the same issue. You or Jason
are free to choose whatever patch you like.

> > Not that there is one I know of, but IMHO the only useful patch is to
> > set passed values to an invalid gpio number.
>
> To me, it remains a fragile way of doing things. Let's say tomorrow you
> add a new "int foo_gpio" field in mvsdio_platform_data. The whole
> purpose of C99 struct initializers is that you don't have to change all
> instances of the structure because all fields that are not initialized
> with .<field>  =<value>  are guaranteed to be zero. If you need to set
> foo_gpio to -1 everywhere when you add this field, it becomes quite
> annoying.

I understand that struct initialization with zero is done for a purpose.
But gpio = 0 is a valid gpio number and engineers will always count from
zero ;)

Given the fact that .gpio = 0 is valid, you would have to add some
.really_use_that_number_i_was_too_lazy_to_set = 1.. but mvsdio_plaform_data
won't stay long as you already pointed out, and I am happy with any fix
for the issue.

> > > That said, I have nothing against explicitly setting those GPIO values
> > > to an invalid value. Maybe -EINVAL would make more sense than just -1 ?
> >
> > Every invalid gpio number will be sufficient. But -EINVAL doesn't make
> > more sense than -1 does. Having no cd-gpio is not an "Invalid argument".
>
> It's just that I've seen -EINVAL being used on some other platforms, at
> least mach-at91/, but I agree it's not an invalid argument per se.

If you don't like -1, you can choose -ENOENT. But -1 has been used for ages
as "invalid" or "unused" so it was my first guess.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/