Michael Walle <michael@xxxxxxxx> writes: > Am 2022-07-04 18:01, schrieb Aidan MacDonald: >> Michael Walle <michael@xxxxxxxx> 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.. > I found three drivers (not exhaustive) that have fields for setting the output level: gpio-amd8111, gpio-creg-snps, and gpio-lp3943. Admittedly that's more than I expected, so maybe we shouldn't dismiss the idea of multi-bit output fields. If you still think the API you're suggesting is better then I can go with it, but IMHO it's more code and more special cases, even if only a little bit. >>> - 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?