RE: [PATCH v7 5/9] gpio: regmap: Add gpio_regmap_operation to extend reg_mask_xlate callback

[Yu-Chun Lin [林祐君]]

Hi Michael,

>
>> @@ -71,7 +73,7 @@ static int gpio_regmap_simple_xlate(struct 
>> gpio_regmap *gpio,  static int gpio_regmap_get(struct gpio_chip *chip, 
>> unsigned int offset)  {
>>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
>> -	unsigned int base, val, reg, mask;
>> +	unsigned int base, val, reg, mask, dir_mask;
>>  	int ret;
>>  
>>  	/* we might not have an output register if we are input only */ @@ 
>> -80,7 +82,18 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>>  	else
>>  		base = gpio_regmap_addr(gpio->reg_set_base);
>>  
>> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_OP, base, offset, &reg, &dir_mask);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(gpio->regmap, reg, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val & dir_mask)
>> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_OUT, base, offset, &reg, &mask);
>> +	else
>> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, 
>> +&reg, &mask);
>
> What's going on here? Looks like I've missed this in the previous
> patches. All the other drivers we are now reading the value twice.
> In the regmap_read() above and the one that follows just after this
> hunk.
> 
> Has the gpio controller two different bits, one for output and one
> for input? Are you sure, the input bit doesn't reflect the output
> bit if it's configured as an output?
> 
> IMHO this shouldn't be part of the core. rtd1625_reg_mask_xlate()
> should either return RTD1625_GPIO_IN or RTD1625_GPIO_OUT depending
> on the mode, if that's needed at all. I'd guess RTD1625_GPIO_IN
> will just work fine and will actually fetch the actual line state.
>
> -michael
>

Regarding the hardware, this controller indeed has dedicated and separate
bits for IN, OUT, and DIR. If configured as an output, reading the IN bit
does not reflect the actual state, so we must check the DIR bit first to
know which bit to read.

As you pointed out, my implementation would cause other drivers to read
twice. Therefore, I will keep GPIO_REGMAP_GET_OP, drop GPIO_REGMAP_IN and
GPIO_REGMAP_OUT, and move the register reading logic into my custom
reg_mask_xlate().

Best Regards,
Yu-Chun

>>
>> >  	if (ret)
>> >  		return ret;
>>
>...
>
>>  
>> +/**
>> + * enum gpio_regmap_operation - Operation type for reg_mask_xlate 
>> +callback
>> + *
>> + * Traditionally, the operation type was inferred from the base register.
>> + * However, that approach does not always work — for example, when 
>> +all control
>> + * bits of a single GPIO reside in the same register. This enum 
>> +allows the
>> + * reg_mask_xlate callback to explicitly distinguish between operation types.
>> + * The user is free to choose which method to use.
>> + *
>> + * Value operations:
>> + * @GPIO_REGMAP_GET_OP: Mask for reading direction to detect if GPIO is input or
>> + *			output. Used in gpio_regmap_get() to determine the GPIO
>> + *			direction.
>
> This is also not very intuitive. I'd expect there is only one
> operation for the gpio_regmap_get and that is exactly this one.
>
>> + * @GPIO_REGMAP_IN: Mask for reading input value. Used when GPIO is configured as
>> + *			input.
>> + * @GPIO_REGMAP_OUT: Mask for reading output value. Used when GPIO is configured as
>> + *			output.
>> + *
>> + * Output operations:
>> + * @GPIO_REGMAP_SET_OP: Mask for setting GPIO output value.
>> + *
>> + * Direction operations:
>> + * @GPIO_REGMAP_GET_DIR_OP: Mask for reading GPIO direction (input/output).
>> + * @GPIO_REGMAP_SET_DIR_OP: Mask for setting GPIO direction (input/output).
>> + */
>> +enum gpio_regmap_operation {
>> +	GPIO_REGMAP_GET_OP,
>> +	GPIO_REGMAP_SET_OP,
>> +	GPIO_REGMAP_GET_DIR_OP,
>> +	GPIO_REGMAP_SET_DIR_OP,
>> +	GPIO_REGMAP_IN,
>> +	GPIO_REGMAP_OUT,
>> +};