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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 44CAEC71153 for ; Mon, 28 Aug 2023 18:01:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232987AbjH1SBO (ORCPT ); Mon, 28 Aug 2023 14:01:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55648 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233001AbjH1SAr (ORCPT ); Mon, 28 Aug 2023 14:00:47 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0328010D; Mon, 28 Aug 2023 11:00:45 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 965B963D91; Mon, 28 Aug 2023 18:00:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5695FC433C7; Mon, 28 Aug 2023 18:00:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1693245644; bh=anTSmw6MgyMNpmqC7sR5fOUZIGW4VnZ7ptwo6QfqrwM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=F2kqKyUReIXbWgeBCBB0IPwXKeN+gGSKd7SxvtUnsdlacikInN2+FxRKGxDdy0v+u bwDxue+PcVHpJIVlXwOOjBierv35ag0lVbtPsWSOiXGBnQWN1fzUa3zqGE+U8jV9kZ v6HhvUlEHvpdnMnePOUioD94jG30D5RXkcHdD2f+kt47KmrVBb9TKgZoSLwuc6NQDz IFmmFG1ik+J2IawdrQr47pc4XN/FxbWfxgHod2iifEDS7q9C8RdLPl7rizqKwzeRN0 wNuKqIBszTVKsTe3gyUMwV1y2CMygohiF6940p7LJ4MImSC7QncIG9VDO4IlGXvoi7 m71sa1EOu+eBQ== Date: Mon, 28 Aug 2023 19:01:01 +0100 From: Jonathan Cameron To: Andy Shevchenko Cc: Greg Kroah-Hartman , Antoniu Miclaus , Jonathan Cameron , Gerald Loacker , Gwendal Grignou , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, Daniel Scally , Heikki Krogerus , Sakari Ailus , "Rafael J. Wysocki" , Lars-Peter Clausen , Michael Hennerich Subject: Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string() Message-ID: <20230828190101.50f70921@jic23-huawei> In-Reply-To: References: <20230808162800.61651-1-andriy.shevchenko@linux.intel.com> <20230808162800.61651-3-andriy.shevchenko@linux.intel.com> <20230809185944.1ae78e34@jic23-huawei> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Thu, 10 Aug 2023 16:26:54 +0300 Andy Shevchenko wrote: > On Wed, Aug 09, 2023 at 06:59:44PM +0100, Jonathan Cameron wrote: > > On Tue, 8 Aug 2023 19:27:56 +0300 > > Andy Shevchenko wrote: > > ... > > > > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > > > + const char *propname, const char * const *array, size_t n) > > > > Hi Andy, > > > > Whilst I'm not 100% sold on adding ever increasing complexity to what we > > match, this one feels like a common enough thing to be worth providing. > > Yep, that's why I considered it's good to add (and because of new comers). > > > Looking at the usecases I wonder if it would be better to pass in > > an unsigned int *ret which is only updated on a match? > > So the question is here are we going to match (pun intended) the prototype to > the device_property_match*() family of functions or to device_property_read_*() > one. If the latter, this has to be renamed, but then it probably will contradict > the semantics as we are _matching_ against something and not just _reading_ > something. > > That said, do you agree that current implementation is (slightly) better from > these aspects? Anyway, look at the below. > > > That way the common properties approach of not checking the return value > > if we have an optional property would apply. > > > > e.g. patch 3 > > Only? I didn't look further :) > > > would end up with a block that looks like: > > > > st->input_mode = ADMV1014_IQ_MODE; > > device_property_match_property_string(&spi->dev, "adi,input-mode", > > input_mode_names, > > ARRAY_SIZE(input_mode_names), > > &st->input_mode); > > > > Only neat and tidy if the thing being optionally read into is an unsigned int > > though (otherwise you still need a local variable) > > We also can have a hybrid variant, returning in both sides > > int device_property_match_property_string(..., size_t *index) > { > if (index) > *index = ret; > return ret; > } > > (also note the correct return type as it has to match to @n). > > Would it be still okay or too over engineered? > Probably over engineered.... Lets stick to what you have. If various firmware folk are happy with the new function that's fine by me. Rafael? Jonathan