Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/29/22 12:59, Andy Shevchenko wrote:
> On Tue, Nov 29, 2022 at 1:32 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/29/22 11:28, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> The tps68470 has support for 2 indicator LED outputs called
>>>> ileda and iledb, at support for these as GPIO pins 10 + 11.
>>>
>>> add ?
>>
>> This models ileda and iledb outputs *as* GPIO pins 10 + 11
>> on the linux gpiochip.
>>
>> But yes it also adds gpio pins 10 + 11 to the gpiochip, so
>> either one works I guess :)
> 
> I had to be a bit more precise. 'at support'?! Perhaps it should be
> 'add support'?
> 
> ...
> 
>>>> +static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
>>>> +                                          unsigned int *reg, int *mask)
>>>
>>> Hmm... Usual way is to put the get/set flag at the end of the list of
>>> parameters.
>>
>> For functions returning values by reference I always follow the
>> pattern of input parameters first, then output parameters.
>>
>>> Also why not naming it as 'dir' to avoid confusion with the _get in
>>> the function name?
>>
>> Because dir is meaningless without an enum to to define what a dir
>> of 0/false means. Where as get is clear without such an enum.
>> get is set to true when this function is called from
>> tps68470_gpio_get() and false when it is called from
>> tps68470_gpio_set(). It does not get more straight forward then that.
> 
> But it's about the buffer (in hw sense) to read value from. What about
> naming it out_or_in?

"out or in" still does not make clear what a value of true means
does true mean out or does it mean in ? 

I'm sorry, but just like with your comments on patch 1/3
I really don't see the problem here. Unlike "dir" or "out_or_in"
get is a perfectly fine parameter name which is actually clearly
defined by the parameter-name itself.

And the function + the callers are all in the same file, so
anyone reading the code can easily deduce the meaning from
the code.

Regards,

Hans