Messages in this thread |  | | Date | Mon, 04 Jul 2022 21:46:39 +0200 | From | Michael Walle <> | Subject | Re: [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO |
| |
Am 2022-07-04 18:01, schrieb Aidan MacDonald: > Michael Walle <michael@walle.cc> writes: > >> Am 2022-07-03 13:10, schrieb Aidan MacDonald: >>> Some devices use a multi-bit register field to change the GPIO >>> input/output direction. Add the ->reg_field_xlate() callback to >>> support such devices in gpio-regmap. >>> ->reg_field_xlate() builds on ->reg_mask_xlate() by allowing the >>> driver to return a mask and values to describe a register field. >>> gpio-regmap will use the mask to isolate the field and compare or >>> update it using the values to implement GPIO level and direction >>> get and set ops. >> >> Thanks for working on this. Here are my thoughts on how to improve >> it: >> - I'm wary on the value translation of the set and get, you >> don't need that at the moment, correct? I'd concentrate on >> the direction for now. >> - I'd add a xlate_direction(), see below for an example > > Yeah, I only need direction, but there's no advantage to creating a > specific mechanism. I'm not opposed to doing that but I don't see > how it can be done cleanly. Being more general is more consistent > for the API and implementation -- even if the extra flexibility > probably won't be needed, it doesn't hurt.
I'd prefer to keep it to the current use case. I'm not sure if there are many controllers which have more than one bit for the input and output state. And if, we are still limited to one register, what if the bits are distributed among multiple registers..
>> - because we can then handle the value too, we don't need the >> invert handling in the {set,get}_direction. drop it there >> and handle it in a simple_xlat. In gpio_regmap, >> store "reg_dir_base" and "invert_direction", derived from >> config->reg_dir_in_base and config->reg_dir_out_base. >> > > I think this is more complicated and less consistent than handling > reg_dir_in/out_base separately.
It is just an internal implementation detail; I'm not talking about changing the configuration. And actually, there was once confusion about the reg_dir_in_base and reg_dir_out_base, IIRC. I'd need to find that thread again. But for now, I'd keep the configuration anyway.
Think about it. If you already have the value translation (which you need), why would you still do the invert inside the {set,get}_direction? It is just a use case of the translation function actually. (Also, an invert only makes sense with a one bit value).
You could do something like: if (config->reg_dir_out_base) { gpio->xlat_direction = gpio_regmap_simple_xlat_direction; gpio->reg_dir_base = config->reg_dir_out_base; } if (config->reg_dir_in_base) { gpio->xlat_direction = gpio_regmap_simple_xlat_direction_inverted; gpio->reg_dir_base = config->reg_dir_in_base; }
But both of these function would be almost the same, thus my example below.
Mhh. Actually I just noticed while writing this.. we need a new config->reg_dir_base anyway, otherwise you'd need to either pick reg_dir_in_base or reg_dir_out_base to work with a custom .xlat_direction callback.
if (config->xlat_direction) { gpio->xlat_direction = config->gpio_xlat_direction; gpio->reg_dir_base = config->reg_dir_base; }
Since there are no users of config->reg_dir_in_base, we can just kill that one. These were just added because it was based on bgpio. Then it will just be:
gpio->reg_dir_base = config->reg_dir_base; gpio->direction_xlat = config->direction_xlat; if (!gpio->direction_xlat) gpio->direction_xlat = gpio_regmap_simple_direction_xlat;
If someone needs an inverted direction, he can either have a custom direction_xlat or we'll introduce a config->invert_direction option.
>> static int gpio_regmap_simple_xlat_direction(struct gpio_regmap *gpio >> unsigend int base, >> unsigned int offset, >> unsigned int *dir_out, >> unsigned int *dir_in) >> { >> unsigned int line = offset % gpio->ngpio_per_reg; >> unsigned int mask = BIT(line); >> >> if (!gpio->invert_direction) { >> *dir_out = mask; >> *dir_in = 0; >> } else { >> *dir_out = 0; >> *dir_in = mask; >> } >> >> return 0; >> } > > This isn't really an independent function: what do *dir_out and *dir_in > mean on their own? You need use the matching mask from ->reg_mask_xlate > for those values to be of any use. And those two functions have to > match > up because they need to agree on the same mask.
Yes. I was thinking it isn't an issue because the driver implementing this will need to know the mask anyway. But maybe it is better to also pass the mask, which was obtained by the .reg_mask_xlat(). Or we could just repeat the corresponding value within the value and the caller could also apply the mask to this returned value.
I.e. if you have a two bit value 01 for output and 10 for input and you have a 32bit register with 16 values, you can use *dir_out = 0x55555555; *dir_in = 0xaaaaaaaa;
Not that easy to understand. But maybe you find it easier than me to write documentation ;)
-michael
>> >> And in the {set,get}_direction() you can then check both >> values and convert it from or to GPIO_LINE_DIRECTION_{OUT,IN}. > > Agreed, checking both values and erroring out if the register has an > unexpected value is a good idea. > >> >> Thoughts?
|  |