On Sun, Aug 16, 2020 at 04:32:34PM +0200, Bartosz Golaszewski wrote:

On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson [off-list ref] wrote:

quoted

Add support for edge detection to lines requested using
GPIO_V2_GET_LINE_IOCTL.

[snip]

quoted

+       /* event_buffer_size only valid with edge detection */
+       has_edge_detection = gpio_v2_line_config_has_edge_detection(lc);
+       if (lr.event_buffer_size && !has_edge_detection)
+               return -EINVAL;
+
        line = kzalloc(struct_size(line, descs, lr.num_lines),
                       GFP_KERNEL);
        if (!line)

@@ -666,6 +944,16 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
        line->gdev = gdev;
        get_device(&gdev->dev);

+       line->edets = kcalloc(lr.num_lines, sizeof(*line->edets),
+                             GFP_KERNEL);

You're allocating num_lines of edge detectors even if only certain
lines have edge detection (via attributes). I don't like it but it
made me think about struct line. How about having struct line which
actually only represents a single line (and it contains the relevant
gpio_desc pointer as well as the associated edge detector and any
other data only relevant for this line) and a set of lines would be
aggregated in struct line_request or line_request_data which would
additionally contain common fields? Does that even make sense?

You are right, and it makes total sense.

I'm not totally thrilled with the block allocation either, but an
earlier draft with edge detectors/debouncers created and destroyed as
required resulted in complicated lifecycle management that this approach
avoids.

I'll have a look at restructuring it as you suggest.
The only downside that springs to mind is that the gpiolib API expects
a desc array, which we'll no longer have handy, so it would have to be
built on the fly as per the sparse gets/sets.

Cheers,
Kent.