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@xxxxxxxxxx> 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;
>> + }
>> +}
>
