Hi Martin,
> So do you really want to limit yourself to just a single line per
object?
I figured a single l_gpio_writer object could easily represent multiple
lines. But for handling power to my peripheral only a single line is
needed.
If/when we implement multi-line handling, the single-line functions can
be turned into wrappers that set up 1-sized arrays.
I think you're not going to gain much up front and will make your job
(or someone else's) much harder if you do that.
>>
>> void l_gpio_out_free(struct l_gpio_out *gpio_out);
>>
>> bool l_gpio_out_set(struct l_gpio_out *gpio_out, bool value);
>>
>> bool l_gpio_find(char *chip, uint32_t *line_num);
>
> I'm guessing this is missing a const char * variable?
Ahh, yes. So like this:
bool l_gpio_find(const char *label, char *chip, uint32_t *line_num);
I wanted this to search all available gpio chips and return the
chip-name in char *chip.
So chip is a return value? In that case char **chip. But I'm still
confused how you're going to accomplish this? Are you going to scan
sysfs for this?
My use-case as a BSP developer is to tell the application developer to
simply open gpio label "FOO" and then let the BSP care about gpio chips
and line numbers. The label might exist on gpio chip X on one board, and
on gpio chip Y on another.
If we really want to support cases where a label exists on multiple
chips, then we could implement the above as
char **l_gpio_find_chip(const char *label);
This sounds more general
The user can decide what to do if multiple devices are returned...
>>
>> This should allow to add the needed parts for multiple-lines, inputs,
>> and gpio-chips at a later point. I.e. adding API for
>>
>> struct l_gpio_chip;
>> struct l_gpio_in;
>>
>> and maybe also
>>
>> struct l_gpio_out *l_gpio_out_many(const char *chip,
>> uint32_t line_num1, bool val1,
>> ..., -1);
>
> This still doesn't solve the dynamically discovered lines issue.
varargs are probably not the right choice.
I think ultimately we may want both, but I'm fine holding off
implementing them until that day comes.
> I would maybe do something like this. Perhaps some of this might be
> redundant or not needed...
>
> struct l_gpio_info *l_gpio_discover_lines(const char *device);
> const char *l_gpio_info_get_name(struct l_gpio_info *);
> const char *l_gpio_info_get_label(struct l_gpio_info *);
> uint32_t l_gpio_info_get_num_lines(struct l_gpio_info *);
> char **l_gpio_get_lines(struct l_gpio_info *);
> uint32_t l_gpio_info_find_offset(struct l_gpio_info *, const char *name);
> const char *l_gpio_info_get_name(struct l_gpio_info *, uint32_t offset);
> void l_gpio_info_free(struct l_gpio_info *);
I think we should use l_gpio_device instead of l_gpio_info to make it
clear that the names and labels are for the chip and not the line.
That's fine, or use l_gpio_chip or l_gpio_chip_info to keep in line with
the kernel naming.
> int l_gpio_get_values_by_name(const char *device, const char *line, ...);
> int l_gpio_get_values_by_offset(const char *device, int offset, ...);
>
> struct l_gpio_writer *l_gpio_new(const char *device, uint32_t n_lines,
> const uint32_t lines[][2]);
I like this. I guess the following functions should have _writer in
their names, and take a struct l_gpio_writer * as first argument?
I'm also fine naming them l_gpio. Not sure what looks better yet
honestly. We can fight over that later.
> l_gpio_free(struct l_gpio *);
> bool l_gpio_set_linesv(struct l_gpio *gpio, uint32_t n_lines, const
> uint32_t lines[][2]);
> bool l_gpio_set_line(struct l_gpio *gpio, uint32_t line, uint32_t value);
> bool l_gpio_set_lines(struct l_gpio *gpio, int line, ...);
I prefer the l_gpio_set_linesv() approach to this.
Sure, lets implement the set_line & set_linesv first.
> So at the bare minimum, you could do:
>
> struct l_gpio_info *info = l_gpio_discover_lines("/foo/bar");
> uint32_t line = l_gpio_info_find_offset(info, "gpio_line");
> uint32_t lines[1][2];
> struct l_gpio *gpio;
>
> lines[0][0] = line;
> lines[0][1] = some_default;
> l_gpio_info_free(info);
>
> gpio = l_gpio_new("/foo/bar", 1, lines);
>
> // off
> l_gpio_set_line(gpio, lines[0][0], 0);
>
> // on
> l_gpio_set_line(gpio, lines[0][0], 1);
How about:
char **devices;
struct l_gpio_device *device;
uint32_t lines[1][2];
struct l_gpio_writer *line;
devices = l_gpio_devices_with_line_label("my_label");
if (!devices)
return;
if (l_strv_length(devices) != 1) {
l_error("label not unique");
return;
}
device = l_gpio_device_new(devices[0]);
if (!device)
return;
l_strfreev(devices);
if (!l_gpio_device_find_offset(device, "my_label", &lines[0][0]);
return;
lines[0][1] = 0;
line = l_gpio_writer_new(device, 1, lines);
if (!line)
return;
l_gpio_device_free(device);
// off
l_gpio_writer_set(line, lines[0][0], 0);
// on
l_gpio_writer_set(line, lines[0][0], 1);
// many
l_gpio_writer_setv(line, 1, lines);
l_gpio_writer_free(line);
That looks fine to me.
We could also consider using a struct l_gpio_line_value to represent
line numbers and values:
struct l_gpio_line_value {
uint32_t line_number;
line_offset I guess.
uint32_t line_value;
};
and take an array of those as argument instead of uint32_t[][2].
Yep. I thought about that, but generally I dislike introducing
non-opaque types into the public namespace unless absolutely needed.
But in this case it doesn't matter a whole lot, so I'm fine either way.
Regards,
-Denis