| Message ID | 20220703111057.23246-2-aidanmacdonald.0x0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-regmap support for register fields and other hooks | expand |
On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > 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 the proposal. My comments below. ... > +static void > +gpio_regmap_simple_field_xlate(struct gpio_regmap *gpio, > + unsigned int base, unsigned int offset, > + unsigned int *reg, unsigned int *mask, > + unsigned int *values) > +{ > + gpio->reg_mask_xlate(gpio, base, offset, reg, mask); > + values[0] = 0; > + values[1] = *mask; This is a fragile and less compile-check prone approach. If you know the amount of values, make a specific data type for that, or pass the length of the output buffer.. > +} ... > + unsigned int values[2]; > + return (val & mask) == values[1]; > + unsigned int values[2]; How will the callee know that it's only 2 available? > + regmap_update_bits(gpio->regmap, reg, mask, values[!!val]); If we have special meaning of the values, perhaps it needs to follow an enum of some definitions, so everybody will understand how indices are mapped to the actual data in the array. > + unsigned int values[2]; > + regmap_update_bits(gpio->regmap, reg, mask, values[1]); > + unsigned int values[2]; > + if ((val & mask) == values[invert]) How do you guarantee this won't overflow? (see above comment about indices mapping) > + unsigned int values[2]; As per above comments.
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 - 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. 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; } And in the {set,get}_direction() you can then check both values and convert it from or to GPIO_LINE_DIRECTION_{OUT,IN}. Thoughts? -michael
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. > - 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. > 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. > > 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? > > -michael
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> >> 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 the proposal. My comments below. > > ... > >> +static void >> +gpio_regmap_simple_field_xlate(struct gpio_regmap *gpio, >> + unsigned int base, unsigned int offset, >> + unsigned int *reg, unsigned int *mask, >> + unsigned int *values) >> +{ >> + gpio->reg_mask_xlate(gpio, base, offset, reg, mask); >> + values[0] = 0; >> + values[1] = *mask; > > This is a fragile and less compile-check prone approach. If you know > the amount of values, make a specific data type for that, or pass the > length of the output buffer.. > >> +} > > ... > >> + unsigned int values[2]; > >> + return (val & mask) == values[1]; > >> + unsigned int values[2]; > > How will the callee know that it's only 2 available? > > >> + regmap_update_bits(gpio->regmap, reg, mask, values[!!val]); > > If we have special meaning of the values, perhaps it needs to follow > an enum of some definitions, so everybody will understand how indices > are mapped to the actual data in the array. > >> + unsigned int values[2]; > >> + regmap_update_bits(gpio->regmap, reg, mask, values[1]); > >> + unsigned int values[2]; > >> + if ((val & mask) == values[invert]) > > How do you guarantee this won't overflow? (see above comment about > indices mapping) > >> + unsigned int values[2]; > > As per above comments. The documentation states that ->reg_field_xlate returns values[0] and values[1] for low/high level or input/output direction. IOW, 0 is low level / input direction and 1 is high level / output direction. Embedding the array in a struct seems like a better idea though, thanks.
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?
Michael Walle <michael@walle.cc> writes: > 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.. > 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?
Am 2022-07-06 22:46, schrieb Aidan MacDonald: >> Am 2022-07-04 18:01, schrieb Aidan MacDonald: >>>> 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. I'm not dismissing it, but I want to wait for an actual user for it :) > 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. What bothers me with your approach is that you are returning an integer and in one case you are interpreting it as gpio direction and in the other case you are interpreting it as a gpio state, while mine is more explicit, i.e. a xlate_direction() for the direction and if there will be support for multi bit input/output then there would be a xlate_state() or similar. Granted, it is more code, but easier to understand IMHO. -michael
Michael Walle <michael@walle.cc> writes: > Am 2022-07-06 22:46, schrieb Aidan MacDonald: >>> Am 2022-07-04 18:01, schrieb Aidan MacDonald: >>>>> 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. > > I'm not dismissing it, but I want to wait for an actual user > for it :) > >> 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. > > What bothers me with your approach is that you are returning > an integer and in one case you are interpreting it as gpio > direction and in the other case you are interpreting it as > a gpio state, while mine is more explicit, i.e. a > xlate_direction() for the direction and if there will be > support for multi bit input/output then there would be a > xlate_state() or similar. Granted, it is more code, but > easier to understand IMHO. > > -michael Fair enough. I'll use your approach then. I thought the semantic mix-up was a worthwhile compromise, but perhaps not. Technically the part that 'interprets' is not the values themselves, but the index into the values array, which makes things a bit more confusing. You have to keep in mind how the registers would behave if you had a single bit, because it's the bit value that indexes the values array. It's not _that_ hard to keep straight but obviously... not as obvious. :)
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 6383136cbe59..9256b922c654 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -27,6 +27,10 @@ struct gpio_regmap { int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask); + void (*reg_field_xlate)(struct gpio_regmap *gpio, + unsigned int base, unsigned int offset, + unsigned int *reg, unsigned int *mask, + unsigned int *values); void *driver_data; }; @@ -52,10 +56,22 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, return 0; } +static void +gpio_regmap_simple_field_xlate(struct gpio_regmap *gpio, + unsigned int base, unsigned int offset, + unsigned int *reg, unsigned int *mask, + unsigned int *values) +{ + gpio->reg_mask_xlate(gpio, base, offset, reg, mask); + values[0] = 0; + values[1] = *mask; +} + 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 values[2]; int ret; /* we might not have an output register if we are input only */ @@ -64,15 +80,13 @@ 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, ®, &mask); - if (ret) - return ret; + gpio->reg_field_xlate(gpio, base, offset, ®, &mask, values); ret = regmap_read(gpio->regmap, reg, &val); if (ret) return ret; - return !!(val & mask); + return (val & mask) == values[1]; } static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, @@ -81,12 +95,10 @@ static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, struct gpio_regmap *gpio = gpiochip_get_data(chip); unsigned int base = gpio_regmap_addr(gpio->reg_set_base); unsigned int reg, mask; + unsigned int values[2]; - gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); - if (val) - regmap_update_bits(gpio->regmap, reg, mask, mask); - else - regmap_update_bits(gpio->regmap, reg, mask, 0); + gpio->reg_field_xlate(gpio, base, offset, ®, &mask, values); + regmap_update_bits(gpio->regmap, reg, mask, values[!!val]); } static void gpio_regmap_set_with_clear(struct gpio_chip *chip, @@ -94,14 +106,15 @@ static void gpio_regmap_set_with_clear(struct gpio_chip *chip, { struct gpio_regmap *gpio = gpiochip_get_data(chip); unsigned int base, reg, mask; + unsigned int values[2]; if (val) base = gpio_regmap_addr(gpio->reg_set_base); else base = gpio_regmap_addr(gpio->reg_clr_base); - gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); - regmap_write(gpio->regmap, reg, mask); + gpio->reg_field_xlate(gpio, base, offset, ®, &mask, values); + regmap_update_bits(gpio->regmap, reg, mask, values[1]); } static int gpio_regmap_get_direction(struct gpio_chip *chip, @@ -109,6 +122,7 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip, { struct gpio_regmap *gpio = gpiochip_get_data(chip); unsigned int base, val, reg, mask; + unsigned int values[2]; int invert, ret; if (gpio->reg_dir_out_base) { @@ -121,47 +135,36 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip, return -EOPNOTSUPP; } - ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); - if (ret) - return ret; + gpio->reg_field_xlate(gpio, base, offset, ®, &mask, values); ret = regmap_read(gpio->regmap, reg, &val); if (ret) return ret; - if (!!(val & mask) ^ invert) - return GPIO_LINE_DIRECTION_OUT; - else + if ((val & mask) == values[invert]) return GPIO_LINE_DIRECTION_IN; + else + return GPIO_LINE_DIRECTION_OUT; } static int gpio_regmap_set_direction(struct gpio_chip *chip, unsigned int offset, bool output) { struct gpio_regmap *gpio = gpiochip_get_data(chip); - unsigned int base, val, reg, mask; - int invert, ret; + unsigned int base, reg, mask; + unsigned int values[2]; if (gpio->reg_dir_out_base) { base = gpio_regmap_addr(gpio->reg_dir_out_base); - invert = 0; } else if (gpio->reg_dir_in_base) { base = gpio_regmap_addr(gpio->reg_dir_in_base); - invert = 1; + output = !output; } else { return -EOPNOTSUPP; } - ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); - if (ret) - return ret; - - if (invert) - val = output ? 0 : mask; - else - val = output ? mask : 0; - - return regmap_update_bits(gpio->regmap, reg, mask, val); + gpio->reg_field_xlate(gpio, base, offset, ®, &mask, values); + return regmap_update_bits(gpio->regmap, reg, mask, values[output]); } static int gpio_regmap_direction_input(struct gpio_chip *chip, @@ -215,6 +218,10 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config if (config->reg_dir_out_base && config->reg_dir_in_base) return ERR_PTR(-EINVAL); + /* only one of these should be provided */ + if (config->reg_field_xlate && config->reg_mask_xlate) + return ERR_PTR(-EINVAL); + gpio = kzalloc(sizeof(*gpio), GFP_KERNEL); if (!gpio) return ERR_PTR(-ENOMEM); @@ -225,6 +232,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config gpio->ngpio_per_reg = config->ngpio_per_reg; gpio->reg_stride = config->reg_stride; gpio->reg_mask_xlate = config->reg_mask_xlate; + gpio->reg_field_xlate = config->reg_field_xlate; gpio->reg_dat_base = config->reg_dat_base; gpio->reg_set_base = config->reg_set_base; gpio->reg_clr_base = config->reg_clr_base; @@ -242,6 +250,9 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config if (!gpio->reg_mask_xlate) gpio->reg_mask_xlate = gpio_regmap_simple_xlate; + if (!gpio->reg_field_xlate) + gpio->reg_field_xlate = gpio_regmap_simple_field_xlate; + chip = &gpio->gpio_chip; chip->parent = config->parent; chip->fwnode = config->fwnode; diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h index a9f7b7faf57b..a673dbfe88a3 100644 --- a/include/linux/gpio/regmap.h +++ b/include/linux/gpio/regmap.h @@ -37,6 +37,10 @@ struct regmap; * offset to a register/bitmask pair. If not * given the default gpio_regmap_simple_xlate() * is used. + * @reg_field_xlate: (Optional) Translates base address and GPIO offset + * to register, mask, and field values. If not given + * the default gpio_regmap_field_xlate() is used, which + * is implemented in terms of ->reg_mask_xlate. * @drvdata: (Optional) Pointer to driver specific data which is * not used by gpio-remap but is provided "as is" to the * driver callback(s). @@ -45,6 +49,12 @@ struct regmap; * register and mask pair. The base address is one of the given register * base addresses in this structure. * + * ->reg_field_xlate supports chips that have more than one bit per GPIO for + * controlling the output level or direction. In addition to the register and + * mask it should output field values for low/high level or in/out direction + * in ``values[0]`` and ``values[1]``. If ->reg_field_xlate is provided then + * it overrides any ->reg_mask_xlate callback. + * * Although all register base addresses are marked as optional, there are * several rules: * 1. if you only have @reg_dat_base set, then it is input-only @@ -81,6 +91,10 @@ struct gpio_regmap_config { int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask); + void (*reg_field_xlate)(struct gpio_regmap *gpio, + unsigned int base, unsigned int offset, + unsigned int *reg, unsigned int *mask, + unsigned int *values); void *drvdata; };
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. Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- drivers/gpio/gpio-regmap.c | 73 +++++++++++++++++++++---------------- include/linux/gpio/regmap.h | 14 +++++++ 2 files changed, 56 insertions(+), 31 deletions(-)