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

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

Hi Andy,

Thank you for the reviews.

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 :)

> 
> ...
> 
>> +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.

Regards,

Hans




> 
>> +{
>> +       if (offset < TPS68470_N_REGULAR_GPIO) {
>> +               if (get)
>> +                       *reg = TPS68470_REG_GPDI;
>> +               else
>> +                       *reg = TPS68470_REG_GPDO;
>> +               *mask = BIT(offset);
>> +       } else if (offset < (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT)) {
>> +               *reg = TPS68470_REG_SGPO;
>> +               *mask = BIT(offset - TPS68470_N_REGULAR_GPIO);
>> +       } else {
>> +               *reg = TPS68470_REG_ILEDCTL;
>> +               if (offset == (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT))
>> +                       *mask = TPS68470_ILEDCTL_ENA;
>> +               else
>> +                       *mask = TPS68470_ILEDCTL_ENB;
>> +       }
>> +}
>