[lkml]   [2022]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO

Michael Walle <> 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

 \ /
  Last update: 2022-07-04 18:01    [W:0.078 / U:3.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site