From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2950CC4338F for ; Thu, 12 Aug 2021 13:53:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0178060EB5 for ; Thu, 12 Aug 2021 13:53:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232772AbhHLNx3 (ORCPT ); Thu, 12 Aug 2021 09:53:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39978 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232351AbhHLNx2 (ORCPT ); Thu, 12 Aug 2021 09:53:28 -0400 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 37FCEC061756 for ; Thu, 12 Aug 2021 06:53:03 -0700 (PDT) Received: by mail-pj1-x102a.google.com with SMTP id nt11so9697454pjb.2 for ; Thu, 12 Aug 2021 06:53:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=180evQVgJp9t9Noq5BSbAXRwqYDbcMgOjLbWzu4W3So=; b=i+GBo1RPI08Xdjb8MbkI1Nij+ucfQWZhg5yXemflAydSugzw9A4H7ul5HgQYksRkuu +gPfXNMetbQKNoKpIGWtIPlRQ+TIPFUJKXzRzNKDHirPGqshXnfuK2fM6KmkbtjLRGzD mvDhPzsSBsyIetsmrCGOV1x9UgAUMYRCf7A6vYozu+UxzeZaAgmWjWCveBC7sctreNX9 3i4JEVnyz4fNHf4zlDTfc70VtH1a4cLuxInT4Pevuu1FuLfKdWKA4wEx58cbhCxzH1Q6 YOKxrYaF2KyBLl7MupwFKe1Yic6bzZWk5ayyFVEHyYmf03neWUqdzHk23o1Zf8nUxHlG kc7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=180evQVgJp9t9Noq5BSbAXRwqYDbcMgOjLbWzu4W3So=; b=qI/0yqIBYukVe4OWR+mm6swW/4gjFWJtsUzR70qV9es+0DeIogy2+BC66+3Wbo9PPQ Zsbo6aa2s/v1uWYUeMJ2xikrdewBDlPR5H5LKszIzZNXJKLSi6pyZOY0x9FY/jkYPVDm MU8lD6CkKYBaTL8C3+9AGFWmJNWxPqRp9uip2nhtbQb29PbcbMbSL5Xf4GfQ10Z3xZq2 mqDSpQFvRoWmiNU1AyPiolJeq2HhZI20332efDmPORcjC+RjcFaW1HyTIqd6tiVw0pp/ 02GG4oD52P78HRWu3jWIeR9kvJMIKhbhg1xXHG/Z8fCv1FYYxhcoqhYalWoBA3MxAwRb vv2w== X-Gm-Message-State: AOAM5302mhNR92Bj+HDt+bs1pn7z7vuhxau3S4Ucl/+A4nwuSW6h7WD+ S4+pMGNade1R9rHwLD6C8AfX9vLBBFgoiw== X-Google-Smtp-Source: ABdhPJwLrrOCCyf9RUVfgG1ySeLYW3Zf4EYZGwQuj4OImKWi5W5Y0LIl7Bl/IPkYSiws+jG4AA8W7g== X-Received: by 2002:aa7:9415:0:b029:3ca:3205:b1f6 with SMTP id x21-20020aa794150000b02903ca3205b1f6mr4256638pfo.27.1628776382433; Thu, 12 Aug 2021 06:53:02 -0700 (PDT) Received: from sol (106-69-178-229.dyn.iinet.net.au. [106.69.178.229]) by smtp.gmail.com with ESMTPSA id s36sm3936232pgk.64.2021.08.12.06.52.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Aug 2021 06:53:01 -0700 (PDT) Date: Thu, 12 Aug 2021 21:52:55 +0800 From: Kent Gibson To: Bartosz Golaszewski Cc: Linus Walleij , Andy Shevchenko , Jack Winch , Helmut Grohne , Ben Hutchings , "open list:GPIO SUBSYSTEM" Subject: Re: [libgpiod v2.0][PATCH] core: extend config objects Message-ID: <20210812135255.GA28109@sol> References: <20210806132810.23556-1-brgl@bgdev.pl> <20210807084809.GA17852@sol> <20210808231012.GA6224@sol> <20210810103113.GA6637@sol> <20210812102913.GA21938@sol> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote: > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson wrote: > > > > [snip] > > > > > > > > > My preference would be for gpiod_line_config_set_output_value() and > > > > variants to also set the direction for those lines to output. > > > > And maybe rename it to gpiod_line_config_set_output(). > > > > And maybe add a set_input for symmetry. > > > > > > > > > > Any naming idea for setting the direction to "AS_IS"? > > > > > > > Since as-is is vague you need to include the field name. > > So I would remove set_direction and replace it with set_input, set_output > > and set_direction_as_is (which I would expect to see used very rarely in > > the wild, as the only use case I can think of for it is undoing a > > set_input or set_output call). > > > > > > But my concern above was actually the secondary array - that confused me. > > > > And it's big - always. (OTOH it's on the heap so who cares?) > > > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could > > > > have multiple attributes set - so long as the offsets subsets match? > > > > What happens if both debounce and flags are set for the same subset? > > > > Looks like debounce wins and the flags get discarded in > > > > gpiod_line_config_to_kernel(). > > > > > > > > > > Yeah, I aimed at ironing it out when writing tests. You're probably right here. > > > > > > > Same reason I hadn't paid much attention to the implementation. > > > > > > What I had in mind for the config was an array of config for each line, > > > > only performing the mapping to uAPI when the actual request or > > > > reconfigure is performed, though that requires knowledge of the number > > > > of lines in the request to be sized efficiently in the > > > > gpiod_line_config_new(). Sizing it as GPIO_V2_LINES_MAX would be even > > > > worse than your secondary array, so don't want that. > > > > > > Or would it? Currently the full config structure is 3784 bytes on a 64 > > > bit arch. The base config is 32 bytes. If we added the default value > > > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304 > > > bytes. We'd need another base_config for default settings. > > > > > > Unless I'm missing something this does seem like the better option. > > > > > > > Yikes, I overlooked the size of the offsets array in the secondary > > config - that is a significant contributor to the config size as well. > > For some reason I was thinking that was a bitmap, but that couldn't work. > > > > In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to > > go, and that is a surprise. > > Though those array elements will require the line offset as well as the > > base_config, unless you have some other way to map offset to config? > > > > No, but that's fine - see below. > > > > > My Go library uses a map, but that isn't an option here. > > > > Resizing it dynamically is the option I was last pondering, > > > > but my preference would be to add a num_lines parameter to the new. > > > > Anyway, that was what I was wondering about... > > > > > > > > > > We could resize the array dynamically but we'd need to return error > > > codes from getters. > > > > Why? If there is no config for the requested line then you return the > > global default value, right? > > Why does that change if the array is resizable? > > Btw, I'm assuming that the gpiod_line_config would contain a pointer to > > the dynamic array, so the handle the user has would remain unchanged. > > And the getters all return ints, not pointers to internal fields. > > What am I missing? > > Memory allocation failures when resizing. > Why are you resizing in a getter? If you mean setters then fair enough - though you could just sit on the error until the request or reconfigure and return it there instead. Cheers, Kent. > > > > Also, "global default value" is different from the primary, right? > > Perhaps getters should return the primary value, which itself defaults > > to the global defaults, if the line doesn't have specific config? > > > > > We could also define the size when allocating the > > > config but it's a sub-par approach too. > > > > > > > Sure, it's a trade-off, but the alternative is requiring a 2-3k block > > even for a one line request, which seems a wee bit excessive. > > > > As you said - it's on the heap, so who cares. But this is also an > internal structure and so we can use bit fields. That should reduce > the memory footprint significantly as we now don't require more than 3 > bits for any given enum. That would leave us with the debounce period > and offset as full size variables. > > Bart > > > With the proposed API, the only other alternative I can see to give a > > small footprint is dynamic resizing, which I'm not thrilled by either. > > So just wanted to double check that you are content to lock in the > > gpiod_line_config_new API, as that will constrain any optimisation later > > on. > >