Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570230 > unrolled thread

Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 02:10 +0100
    Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-31 09:10 +0100
      Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 10:00 +0100
        Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-31 10:10 +0100
          Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 10:20 +0100
            Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-31 10:40 +0100
              Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 19:50 +0100
                Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-31 20:50 +0100
                  Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Linus Walleij <linus.walleij@linaro.org> - 2017-02-01 14:10 +0100
                    Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-01 14:30 +0100
                      Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Linus Walleij <linus.walleij@linaro.org> - 2017-02-01 16:00 +0100
                        Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 18:20 +0100
                        Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-02-02 11:10 +0100

#1570230 — Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child()

On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:
> Rename devm_get_gpiod_from_child() into
> devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> function is operating on a fwnode object.

I believe this is completely pointless rename. Are you planning on
adding devm_of_get_gpiod_from_child()? Or
devm_acpt_get_gpiod_from_child()? (I sure hope not).

Also, on what object? Does it take fwnode as first argument? Or maybe we
should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
know types of all arguments?

Please, no.

Thanks.

-- 
Dmitry

[toc] | [next] | [standalone]

#1570471

On Mon, 30 Jan 2017 17:06:07 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:
> > Rename devm_get_gpiod_from_child() into
> > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > function is operating on a fwnode object.  
> 
> I believe this is completely pointless rename. Are you planning on
> adding devm_of_get_gpiod_from_child()? Or
> devm_acpt_get_gpiod_from_child()? (I sure hope not).

Of course not.

> 
> Also, on what object? Does it take fwnode as first argument? Or maybe we
> should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> know types of all arguments?

Linus suggested to rename this function [1]. I personally don't care
much about the name, though I agree with Linus that names should be
consistent and descriptive. Moreover, he's the maintainer, and I tend
to follow maintainers suggestion when I contribute to a specific
subsystem.

IIUC, you're concerned about the length of this function name. If I had
to drop something it would be the _from_child() suffix, because the
function is not even checking that the child parameter is actually a
direct child (or a descendant) of device->fwnode. Also, if we want to
be consistent with the rest of the GPIO API, we could rename it
devm_gpiod_get_from_fwnode() (with the function in added in patch 2
renamed into devm_gpiod_get_from_fwnode()).

Linus, what do you think?

One last thing, I don't want to start a discussion where we're
bikeshedding on a function name instead of focusing on the
functionality, so if it turns into this kind of discussion I'll
probably implement devm_fwnode_get_gpiod_from_child() directly in the
atmel NAND driver and wait for an agreement before switching to the
official version.

Regards,

Boris

[1]https://www.spinics.net/lists/arm-kernel/msg558986.html

[toc] | [prev] | [next] | [standalone]

#1570486

On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:
> On Mon, 30 Jan 2017 17:06:07 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:
> > > Rename devm_get_gpiod_from_child() into
> > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > function is operating on a fwnode object.  
> > 
> > I believe this is completely pointless rename. Are you planning on
> > adding devm_of_get_gpiod_from_child()? Or
> > devm_acpt_get_gpiod_from_child()? (I sure hope not).
> 
> Of course not.
> 
> > 
> > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > know types of all arguments?
> 
> Linus suggested to rename this function [1]. I personally don't care
> much about the name, though I agree with Linus that names should be
> consistent and descriptive. Moreover, he's the maintainer, and I tend
> to follow maintainers suggestion when I contribute to a specific
> subsystem.

OK, I did not know that that was Linus' request, my objection still
stands.

> 
> IIUC, you're concerned about the length of this function name. If I had
> to drop something it would be the _from_child() suffix, because the
> function is not even checking that the child parameter is actually a
> direct child (or a descendant) of device->fwnode.

OK, that sounds better. Actually, we already have
fwnode_get_named_gpiod(), unfortunately it does not do suffixes
permutations. There are also no users, except
devm_get_gpiod_from_child(). So I would:

- rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
- made new fwnode_get_named_gpiod() that did suffix permutation and
  called __fwnode_get_named_gpiod() (or pulled its implementation
  inline)
- renamed devm_get_gpiod_from_child() ->
  devm_fwnode_get_named_gpiod(dev, fwnode, con_id)
  and called fwnode_get_named_gpiod().

This would indeed match the pattern with other fwnode/property handling
APIs.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1570497

On Tue, 31 Jan 2017 00:44:47 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:
> > On Mon, 30 Jan 2017 17:06:07 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >   
> > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:  
> > > > Rename devm_get_gpiod_from_child() into
> > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > > function is operating on a fwnode object.    
> > > 
> > > I believe this is completely pointless rename. Are you planning on
> > > adding devm_of_get_gpiod_from_child()? Or
> > > devm_acpt_get_gpiod_from_child()? (I sure hope not).  
> > 
> > Of course not.
> >   
> > > 
> > > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > > know types of all arguments?  
> > 
> > Linus suggested to rename this function [1]. I personally don't care
> > much about the name, though I agree with Linus that names should be
> > consistent and descriptive. Moreover, he's the maintainer, and I tend
> > to follow maintainers suggestion when I contribute to a specific
> > subsystem.  
> 
> OK, I did not know that that was Linus' request, my objection still
> stands.
> 
> > 
> > IIUC, you're concerned about the length of this function name. If I had
> > to drop something it would be the _from_child() suffix, because the
> > function is not even checking that the child parameter is actually a
> > direct child (or a descendant) of device->fwnode.  
> 
> OK, that sounds better. Actually, we already have
> fwnode_get_named_gpiod(), unfortunately it does not do suffixes
> permutations. There are also no users, except
> devm_get_gpiod_from_child(). So I would:
> 
> - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
> - made new fwnode_get_named_gpiod() that did suffix permutation and
>   called __fwnode_get_named_gpiod() (or pulled its implementation
>   inline)

Sorry but I don't follow you. Why do you need
__fwnode_get_named_gpiod(), and what is the suffix permutation you're
mentioning here?

> - renamed devm_get_gpiod_from_child() ->
>   devm_fwnode_get_named_gpiod(dev, fwnode, con_id)
>   and called fwnode_get_named_gpiod().

Okay. I'm fine with this name, let's see what Linus says.

> 
> This would indeed match the pattern with other fwnode/property handling
> APIs.
> 
> Thanks.
> 

[toc] | [prev] | [next] | [standalone]

#1570504

On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote:
> On Tue, 31 Jan 2017 00:44:47 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:
> > > On Mon, 30 Jan 2017 17:06:07 -0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >   
> > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:  
> > > > > Rename devm_get_gpiod_from_child() into
> > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > > > function is operating on a fwnode object.    
> > > > 
> > > > I believe this is completely pointless rename. Are you planning on
> > > > adding devm_of_get_gpiod_from_child()? Or
> > > > devm_acpt_get_gpiod_from_child()? (I sure hope not).  
> > > 
> > > Of course not.
> > >   
> > > > 
> > > > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > > > know types of all arguments?  
> > > 
> > > Linus suggested to rename this function [1]. I personally don't care
> > > much about the name, though I agree with Linus that names should be
> > > consistent and descriptive. Moreover, he's the maintainer, and I tend
> > > to follow maintainers suggestion when I contribute to a specific
> > > subsystem.  
> > 
> > OK, I did not know that that was Linus' request, my objection still
> > stands.
> > 
> > > 
> > > IIUC, you're concerned about the length of this function name. If I had
> > > to drop something it would be the _from_child() suffix, because the
> > > function is not even checking that the child parameter is actually a
> > > direct child (or a descendant) of device->fwnode.  
> > 
> > OK, that sounds better. Actually, we already have
> > fwnode_get_named_gpiod(), unfortunately it does not do suffixes
> > permutations. There are also no users, except
> > devm_get_gpiod_from_child(). So I would:
> > 
> > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
> > - made new fwnode_get_named_gpiod() that did suffix permutation and
> >   called __fwnode_get_named_gpiod() (or pulled its implementation
> >   inline)
> 
> Sorry but I don't follow you. Why do you need
> __fwnode_get_named_gpiod(),

You do not need it, it will just reduce size of the patch if you use
it. I'd be perfectly fine not with having it and have everything in
fwnode_get_named_gpiod().

> and what is the suffix permutation you're
> mentioning here?

devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes
to the supplied GPIO ID while current fwnode_get_named_gpiod() takes
property name literally.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1570526

On Tue, 31 Jan 2017 01:11:55 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote:
> > On Tue, 31 Jan 2017 00:44:47 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >   
> > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:  
> > > > On Mon, 30 Jan 2017 17:06:07 -0800
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >     
> > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:    
> > > > > > Rename devm_get_gpiod_from_child() into
> > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > > > > function is operating on a fwnode object.      
> > > > > 
> > > > > I believe this is completely pointless rename. Are you planning on
> > > > > adding devm_of_get_gpiod_from_child()? Or
> > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not).    
> > > > 
> > > > Of course not.
> > > >     
> > > > > 
> > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > > > > know types of all arguments?    
> > > > 
> > > > Linus suggested to rename this function [1]. I personally don't care
> > > > much about the name, though I agree with Linus that names should be
> > > > consistent and descriptive. Moreover, he's the maintainer, and I tend
> > > > to follow maintainers suggestion when I contribute to a specific
> > > > subsystem.    
> > > 
> > > OK, I did not know that that was Linus' request, my objection still
> > > stands.
> > >   
> > > > 
> > > > IIUC, you're concerned about the length of this function name. If I had
> > > > to drop something it would be the _from_child() suffix, because the
> > > > function is not even checking that the child parameter is actually a
> > > > direct child (or a descendant) of device->fwnode.    
> > > 
> > > OK, that sounds better. Actually, we already have
> > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes
> > > permutations. There are also no users, except
> > > devm_get_gpiod_from_child(). So I would:
> > > 
> > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
> > > - made new fwnode_get_named_gpiod() that did suffix permutation and
> > >   called __fwnode_get_named_gpiod() (or pulled its implementation
> > >   inline)  
> > 
> > Sorry but I don't follow you. Why do you need
> > __fwnode_get_named_gpiod(),  
> 
> You do not need it, it will just reduce size of the patch if you use
> it. I'd be perfectly fine not with having it and have everything in
> fwnode_get_named_gpiod().

Okay.

> 
> > and what is the suffix permutation you're
> > mentioning here?  
> 
> devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes
> to the supplied GPIO ID while current fwnode_get_named_gpiod() takes
> property name literally.

fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(),
acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would
be weird/inconsistent to have the con_id suffixing logic moved in the
fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not
sure it is).

[toc] | [prev] | [next] | [standalone]

#1570990

On Tue, Jan 31, 2017 at 10:24:24AM +0100, Boris Brezillon wrote:
> On Tue, 31 Jan 2017 01:11:55 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote:
> > > On Tue, 31 Jan 2017 00:44:47 -0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >   
> > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:  
> > > > > On Mon, 30 Jan 2017 17:06:07 -0800
> > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > >     
> > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:    
> > > > > > > Rename devm_get_gpiod_from_child() into
> > > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > > > > > function is operating on a fwnode object.      
> > > > > > 
> > > > > > I believe this is completely pointless rename. Are you planning on
> > > > > > adding devm_of_get_gpiod_from_child()? Or
> > > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not).    
> > > > > 
> > > > > Of course not.
> > > > >     
> > > > > > 
> > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > > > > > know types of all arguments?    
> > > > > 
> > > > > Linus suggested to rename this function [1]. I personally don't care
> > > > > much about the name, though I agree with Linus that names should be
> > > > > consistent and descriptive. Moreover, he's the maintainer, and I tend
> > > > > to follow maintainers suggestion when I contribute to a specific
> > > > > subsystem.    
> > > > 
> > > > OK, I did not know that that was Linus' request, my objection still
> > > > stands.
> > > >   
> > > > > 
> > > > > IIUC, you're concerned about the length of this function name. If I had
> > > > > to drop something it would be the _from_child() suffix, because the
> > > > > function is not even checking that the child parameter is actually a
> > > > > direct child (or a descendant) of device->fwnode.    
> > > > 
> > > > OK, that sounds better. Actually, we already have
> > > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes
> > > > permutations. There are also no users, except
> > > > devm_get_gpiod_from_child(). So I would:
> > > > 
> > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
> > > > - made new fwnode_get_named_gpiod() that did suffix permutation and
> > > >   called __fwnode_get_named_gpiod() (or pulled its implementation
> > > >   inline)  
> > > 
> > > Sorry but I don't follow you. Why do you need
> > > __fwnode_get_named_gpiod(),  
> > 
> > You do not need it, it will just reduce size of the patch if you use
> > it. I'd be perfectly fine not with having it and have everything in
> > fwnode_get_named_gpiod().
> 
> Okay.
> 
> > 
> > > and what is the suffix permutation you're
> > > mentioning here?  
> > 
> > devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes
> > to the supplied GPIO ID while current fwnode_get_named_gpiod() takes
> > property name literally.
> 
> fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(),
> acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would
> be weird/inconsistent to have the con_id suffixing logic moved in the
> fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not
> sure it is).

Hmm, yeah, I agree, that would be weird. Then let's leave
devm_get_gpiod_from_child() as is ;)

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1571036

On Tue, 31 Jan 2017 10:39:36 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Jan 31, 2017 at 10:24:24AM +0100, Boris Brezillon wrote:
> > On Tue, 31 Jan 2017 01:11:55 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >   
> > > On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote:  
> > > > On Tue, 31 Jan 2017 00:44:47 -0800
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >     
> > > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote:    
> > > > > > On Mon, 30 Jan 2017 17:06:07 -0800
> > > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > > >       
> > > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote:      
> > > > > > > > Rename devm_get_gpiod_from_child() into
> > > > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this
> > > > > > > > function is operating on a fwnode object.        
> > > > > > > 
> > > > > > > I believe this is completely pointless rename. Are you planning on
> > > > > > > adding devm_of_get_gpiod_from_child()? Or
> > > > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not).      
> > > > > > 
> > > > > > Of course not.
> > > > > >       
> > > > > > > 
> > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we
> > > > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we
> > > > > > > know types of all arguments?      
> > > > > > 
> > > > > > Linus suggested to rename this function [1]. I personally don't care
> > > > > > much about the name, though I agree with Linus that names should be
> > > > > > consistent and descriptive. Moreover, he's the maintainer, and I tend
> > > > > > to follow maintainers suggestion when I contribute to a specific
> > > > > > subsystem.      
> > > > > 
> > > > > OK, I did not know that that was Linus' request, my objection still
> > > > > stands.
> > > > >     
> > > > > > 
> > > > > > IIUC, you're concerned about the length of this function name. If I had
> > > > > > to drop something it would be the _from_child() suffix, because the
> > > > > > function is not even checking that the child parameter is actually a
> > > > > > direct child (or a descendant) of device->fwnode.      
> > > > > 
> > > > > OK, that sounds better. Actually, we already have
> > > > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes
> > > > > permutations. There are also no users, except
> > > > > devm_get_gpiod_from_child(). So I would:
> > > > > 
> > > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod()
> > > > > - made new fwnode_get_named_gpiod() that did suffix permutation and
> > > > >   called __fwnode_get_named_gpiod() (or pulled its implementation
> > > > >   inline)    
> > > > 
> > > > Sorry but I don't follow you. Why do you need
> > > > __fwnode_get_named_gpiod(),    
> > > 
> > > You do not need it, it will just reduce size of the patch if you use
> > > it. I'd be perfectly fine not with having it and have everything in
> > > fwnode_get_named_gpiod().  
> > 
> > Okay.
> >   
> > >   
> > > > and what is the suffix permutation you're
> > > > mentioning here?    
> > > 
> > > devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes
> > > to the supplied GPIO ID while current fwnode_get_named_gpiod() takes
> > > property name literally.  
> > 
> > fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(),
> > acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would
> > be weird/inconsistent to have the con_id suffixing logic moved in the
> > fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not
> > sure it is).  
> 
> Hmm, yeah, I agree, that would be weird. Then let's leave
> devm_get_gpiod_from_child() as is ;)

Changing the internal implementation has never been the goal of this
patch. As explained in the commit log, I'm just renaming the function
to make it consistent with other fwnode functions (as suggested by
Linus).
What's happening here is exactly the kind of discussion I wanted to
avoid, and the reason I decided to not change the
devm_get_gpiod_from_child() prototype/name in the first place.

Linus, is this something you really care about? If that's the case, can
you step in?

[toc] | [prev] | [next] | [standalone]

#1571544

On Tue, Jan 31, 2017 at 8:42 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 31 Jan 2017 10:39:36 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

>> Hmm, yeah, I agree, that would be weird. Then let's leave
>> devm_get_gpiod_from_child() as is ;)
>
> Changing the internal implementation has never been the goal of this
> patch. As explained in the commit log, I'm just renaming the function
> to make it consistent with other fwnode functions (as suggested by
> Linus).
> What's happening here is exactly the kind of discussion I wanted to
> avoid, and the reason I decided to not change the
> devm_get_gpiod_from_child() prototype/name in the first place.
>
> Linus, is this something you really care about? If that's the case, can
> you step in?

I can only throw up my hands... The way I percieved it, a new function
was added, but I guess it could be that the diffstat was so
convoluted in the other patch (by the way that diff sometimes give
very confusing stuff unless you use the right fuzz) so I misunderstood
some other renaming as introducing a new function.

Please drop the patch if it is controversial.

The name of the function *is* confusing though but maybe it's not
the biggest problem in the world.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1571560

Hi Linus,

On Wed, 1 Feb 2017 14:05:43 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Jan 31, 2017 at 8:42 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Tue, 31 Jan 2017 10:39:36 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:  
> 
> >> Hmm, yeah, I agree, that would be weird. Then let's leave
> >> devm_get_gpiod_from_child() as is ;)  
> >
> > Changing the internal implementation has never been the goal of this
> > patch. As explained in the commit log, I'm just renaming the function
> > to make it consistent with other fwnode functions (as suggested by
> > Linus).
> > What's happening here is exactly the kind of discussion I wanted to
> > avoid, and the reason I decided to not change the
> > devm_get_gpiod_from_child() prototype/name in the first place.
> >
> > Linus, is this something you really care about? If that's the case, can
> > you step in?  
> 
> I can only throw up my hands...

Sorry for forcing your hand like this, but this is the kind of
discussion I'm not comfortable with (when I need to argue on something
I'm not completely convinced of, or I don't have opinion on).

> The way I percieved it, a new function
> was added, but I guess it could be that the diffstat was so
> convoluted in the other patch (by the way that diff sometimes give
> very confusing stuff unless you use the right fuzz) so I misunderstood
> some other renaming as introducing a new function.

Indeed, a new function is added (see patch 2), and this new function is
taking an additional 'index' parameter. If that's a problem, I can also
change the prototype of devm_get_gpiod_from_child() and patch all
existing users of this function, but I fear we'll end up with pretty
much the same discussion :-/.

> 
> Please drop the patch if it is controversial.
> 
> The name of the function *is* confusing though but maybe it's not
> the biggest problem in the world.

I can still name the new function as you suggested
(devm_fwnode_get_index_gpiod_from_child()), and keep the existing one
unchanged if you want.

Just let me know what you prefer.

Thanks,

Boris

[toc] | [prev] | [next] | [standalone]

#1571630

On Wed, Feb 1, 2017 at 2:22 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Wed, 1 Feb 2017 14:05:43 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:

>> > Linus, is this something you really care about? If that's the case, can
>> > you step in?
>>
>> I can only throw up my hands...
>
> Sorry for forcing your hand like this, but this is the kind of
> discussion I'm not comfortable with (when I need to argue on something
> I'm not completely convinced of, or I don't have opinion on).

Sorry, I'm just too stressed by all patches. I now read back on the
context below.

>> The way I percieved it, a new function
>> was added, but I guess it could be that the diffstat was so
>> convoluted in the other patch (by the way that diff sometimes give
>> very confusing stuff unless you use the right fuzz) so I misunderstood
>> some other renaming as introducing a new function.
>
> Indeed, a new function is added (see patch 2), and this new function is
> taking an additional 'index' parameter. If that's a problem, I can also
> change the prototype of devm_get_gpiod_from_child() and patch all
> existing users of this function, but I fear we'll end up with pretty
> much the same discussion :-/.

Yeah.

>> Please drop the patch if it is controversial.
>>
>> The name of the function *is* confusing though but maybe it's not
>> the biggest problem in the world.
>
> I can still name the new function as you suggested
> (devm_fwnode_get_index_gpiod_from_child()), and keep the existing one
> unchanged if you want.

But that is just insane. Then it is just better to apply this and the
other patch making the situation manageable.

This is a good time to do it too since I'm anyways patching around
in all the consumers this merge window.

Dmitry: is this such a big deal to you?

commit 40b7318319281b1bdec804f6435f26cadd329c13
"gpio: Support for unified device properties interface"

by Mika Westerberg introduced

fwnode_get_named_gpiod()
devm_get_gpiod_from_child()

Both are taking a fwnode as argument and the naming is as
inconsistent as it can be.

Some more churn should be expected as a side
effect of naming this function wrong in the first place.
The fwnode API change was on fast-forward and mistakes
were made, also by me, mea culpa.

When I write kernel code, I usually intuitively look for a function doing
what I want, this naming is unintuitive, and it has confused me so
it will confuse others.

Can I please apply these two patches?

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1571795

On Wed, Feb 01, 2017 at 03:51:06PM +0100, Linus Walleij wrote:
> On Wed, Feb 1, 2017 at 2:22 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Wed, 1 Feb 2017 14:05:43 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> >> > Linus, is this something you really care about? If that's the case, can
> >> > you step in?
> >>
> >> I can only throw up my hands...
> >
> > Sorry for forcing your hand like this, but this is the kind of
> > discussion I'm not comfortable with (when I need to argue on something
> > I'm not completely convinced of, or I don't have opinion on).
> 
> Sorry, I'm just too stressed by all patches. I now read back on the
> context below.
> 
> >> The way I percieved it, a new function
> >> was added, but I guess it could be that the diffstat was so
> >> convoluted in the other patch (by the way that diff sometimes give
> >> very confusing stuff unless you use the right fuzz) so I misunderstood
> >> some other renaming as introducing a new function.
> >
> > Indeed, a new function is added (see patch 2), and this new function is
> > taking an additional 'index' parameter. If that's a problem, I can also
> > change the prototype of devm_get_gpiod_from_child() and patch all
> > existing users of this function, but I fear we'll end up with pretty
> > much the same discussion :-/.
> 
> Yeah.
> 
> >> Please drop the patch if it is controversial.
> >>
> >> The name of the function *is* confusing though but maybe it's not
> >> the biggest problem in the world.
> >
> > I can still name the new function as you suggested
> > (devm_fwnode_get_index_gpiod_from_child()), and keep the existing one
> > unchanged if you want.
> 
> But that is just insane. Then it is just better to apply this and the
> other patch making the situation manageable.
> 
> This is a good time to do it too since I'm anyways patching around
> in all the consumers this merge window.
> 
> Dmitry: is this such a big deal to you?

No, not really. But sometimes it is soooo hard to pass on some
bikeshedding opportunity ;)

> 
> commit 40b7318319281b1bdec804f6435f26cadd329c13
> "gpio: Support for unified device properties interface"
> 
> by Mika Westerberg introduced
> 
> fwnode_get_named_gpiod()
> devm_get_gpiod_from_child()
> 
> Both are taking a fwnode as argument and the naming is as
> inconsistent as it can be.
> 
> Some more churn should be expected as a side
> effect of naming this function wrong in the first place.
> The fwnode API change was on fast-forward and mistakes
> were made, also by me, mea culpa.
> 
> When I write kernel code, I usually intuitively look for a function doing
> what I want, this naming is unintuitive, and it has confused me so
> it will confuse others.
> 
> Can I please apply these two patches?

You have my ack for the input bits.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1572237

On Wed, Feb 01, 2017 at 03:51:06PM +0100, Linus Walleij wrote:
> fwnode_get_named_gpiod()
> devm_get_gpiod_from_child()
> 
> Both are taking a fwnode as argument and the naming is as
> inconsistent as it can be.
> 
> Some more churn should be expected as a side
> effect of naming this function wrong in the first place.
> The fwnode API change was on fast-forward and mistakes
> were made, also by me, mea culpa.

The name fwnode_get_named_gpiod() tries to follow of_get_named_gpio() so
that we can easily convert a driver to use fwnodes instead. The other
function is named so because we look child fwnodes under a device. For
that, yes certainly we could have invented a better name ;-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web