Re: [PATCH v6 2/2] gpio: tps65219: Add support for TI TPS65214 PMIC

[Shree Ramamoorthy <s-ramamoorthy@ti.com>]

On 7/3/25 2:05 PM, Christophe JAILLET wrote:
> Le 03/07/2025 à 20:07, Shree Ramamoorthy a écrit :
>> Add support for the TI TPS65214 PMIC with the addition of an id_table,
>> separate TPS65214 template_chip, and device-specific _change_direction
>> functions.
>>
>> - Use platform_get_device_id() to assign dev-specific information.
>> - Use different change_direction() functions since TPS65214's GPIO
>>    configuration bits are changeable during device operation through bit
>>    GPIO_CONFIG in GENERAL_CONFIG register.
>> - Remove MODULE_ALIAS since it is now generated by MODULE_DEVICE_TABLE.
>>
>> Reviewed-by: Jonathan Cormier <jcormier@criticallink.com>
>> Tested-by: Jonathan Cormier <jcormier@criticallink.com>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>
> ...
>
>> +static int tps65214_gpio_change_direction(struct gpio_chip *gc, 
>> unsigned int offset,
>> +                      unsigned int direction)
>> +{
>> +    struct tps65219_gpio *gpio = gpiochip_get_data(gc);
>> +    struct device *dev = gpio->tps->dev;
>> +    int val, ret;
>> +
>> +    /* Verified if GPIO or GPO in parent function
>
> Nitpick: should the /* be on a separate line?
>
Will fix this.

>> +     * Masked value: 0 = GPIO, 1 = VSEL
>> +     */
>> +    ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, 
>> &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = !!(val & BIT(TPS65219_GPIO0_DIR_MASK));
>> +    if (ret)
>> +        dev_err(dev, "GPIO%d configured as VSEL, not GPIO\n", offset);
>> +
>> +    ret = regmap_update_bits(gpio->tps->regmap, 
>> TPS65219_REG_GENERAL_CONFIG,
>> +                 TPS65214_GPIO0_DIR_MASK, direction);
>> +    if (ret)
>> +        dev_err(dev,
>> +            "Fail to change direction to %u for GPIO%d.\n",
>
> Nitpick: keep it on the previous line?

Will update this as well.

>
>> +            direction, offset);
>> +
>> +    return ret;
>> +}
>
> ...
>
>> +static const struct gpio_chip tps65214_template_chip = {
>> +    .label            = "tps65214-gpio",
>> +    .owner            = THIS_MODULE,
>> +    .get_direction        = tps65214_gpio_get_direction,
>> +    .direction_input    = tps65219_gpio_direction_input,
>> +    .direction_output    = tps65219_gpio_direction_output,
>> +    .get            = tps65219_gpio_get,
>> +    .set_rv            = tps65219_gpio_set,
>> +    .base            = -1,
>> +    .ngpio            = 2,
>> +    .can_sleep        = true,
>> +};
>> +
>>   static const struct gpio_chip tps65219_template_chip = {
>>       .label            = "tps65219-gpio",
>>       .owner            = THIS_MODULE,
>> @@ -154,7 +218,7 @@ static const struct gpio_chip 
>> tps65219_template_chip = {
>>       .direction_input    = tps65219_gpio_direction_input,
>>       .direction_output    = tps65219_gpio_direction_output,
>>       .get            = tps65219_gpio_get,
>> -    .set            = tps65219_gpio_set,
>> +    .set_rv            = tps65219_gpio_set,
>
> Is this correct? Does it even compile?
> tps65219_gpio_set() returns void and .set_rv() expects a return value.
>
> (same for tps65214_template_chip above)

I sent this out too quickly, will add in the corresponding return values & re-test. Thanks for reviewing!

>
>>       .base            = -1,
>>       .ngpio            = 3,
>>       .can_sleep        = true,
>
> ...
>
> CJ

-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer