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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 58D50C3DA4A for ; Wed, 14 Aug 2024 13:54:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gKaQIEEeg47lQwefNjsJzio/ktNqgN3Gi0cUkSve3hg=; b=g2doo7yt+1Fd/pGbrMlLYgKTNd CjrjdXXf0JBQS+zyWUejY7ef1XG6WZHH+vsRGInu6FEqsNOBAcIGmKq6m/OKC2G/Z3BwoG8Ojwie3 KGElF0DaMAbEZFbdtk+qFvZEa5HwQ/4xrg64VCPyDAGiCaFhlDjLs+aTsUJYAkpvcz/hSv3pzFK2O 3RVTbfyK9/54UKULtK8Kn4QbtSzEJ0KPx24nKbw4kISihqp77L9xW9P5nNTuLxkZU9I0evRgk4c10 gdzkEzM7sI/Bdno/23CoZIYzhc+G5bCF1hygYibf91anjMFYewTA6dNm1zKCenRGeaJPvvCFYbZoA j9ycaiUg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1seESc-00000007AYs-4BwL; Wed, 14 Aug 2024 13:54:47 +0000 Received: from mgamail.intel.com ([192.198.163.14]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1seER4-00000007A60-0FeX; Wed, 14 Aug 2024 13:53:11 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1723643590; x=1755179590; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=sXDg6SJ866AaNtAk2Tp+FQv+cJ3dVaRfO0uMyWTSizM=; b=LBXZavRuN/Clknov31BXS/vhDRnJkwoQYV6YME382TaXupz4JSLilSsb XzQwqqZnud5P9O+cxFgmPxw3pRdRIxExnNA2TZ9/XShjlcLV2fj6aLbOO y5RjtKMaw494WPrWtUpam8Nu2SU3rtf1yGJu4gut8kCgwgaxiuXtAPYtf dV+5yKxi6nGTlDW3F74VTYHGBLCX/IWzJ+BvD643bvmzLoKgEGt5J+vnA xzrfAjEZR19YJvYWK+/+2UKXI1siDRi6HgaR1Gnwwb/J34HPjA/64ONmZ ipaShrqrCAYrrZhBRxFfUYcJxfp1lf8kxRo6C4VNuc8q/+mwPOzxQ5AGV w==; X-CSE-ConnectionGUID: FnDjOaxhTsqvRaBcRUsLnQ== X-CSE-MsgGUID: zk1H3/1uS2+Mj2FNljuRmA== X-IronPort-AV: E=McAfee;i="6700,10204,11164"; a="22028619" X-IronPort-AV: E=Sophos;i="6.10,146,1719903600"; d="scan'208";a="22028619" Received: from orviesa010.jf.intel.com ([10.64.159.150]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Aug 2024 06:53:09 -0700 X-CSE-ConnectionGUID: GZjAjcOBS/2tvWYCAMukvQ== X-CSE-MsgGUID: DprzkeJOTT65SAJzcEe9iA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.10,146,1719903600"; d="scan'208";a="58898765" Received: from smile.fi.intel.com ([10.237.72.54]) by orviesa010.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Aug 2024 06:53:05 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.98) (envelope-from ) id 1seEQv-0000000FCFz-3dM9; Wed, 14 Aug 2024 16:53:01 +0300 Date: Wed, 14 Aug 2024 16:53:01 +0300 From: Andy Shevchenko To: Chen-Yu Tsai Cc: Rob Herring , Saravana Kannan , Matthias Brugger , AngeloGioacchino Del Regno , Wolfram Sang , Benson Leung , Tzung-Bi Shih , Mark Brown , Liam Girdwood , chrome-platform@lists.linux.dev, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, Douglas Anderson , Johan Hovold , Jiri Kosina , linux-i2c@vger.kernel.org Subject: Re: [PATCH v4 4/6] i2c: of-prober: Add GPIO and regulator support Message-ID: References: <20240808095931.2649657-1-wenst@chromium.org> <20240808095931.2649657-5-wenst@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240814_065310_163047_09CD48DF X-CRM114-Status: GOOD ( 37.14 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Aug 14, 2024 at 07:34:00PM +0800, Chen-Yu Tsai wrote: > On Tue, Aug 13, 2024 at 7:41 PM Andy Shevchenko > wrote: > > On Thu, Aug 08, 2024 at 05:59:27PM +0800, Chen-Yu Tsai wrote: ... > > > This adds GPIO and regulator management to the I2C OF component prober. > > Can this be two patches? > > You mean one for GPIOs and one for regulators right? Sure. Yes. ... > > > +#define RESOURCE_MAX 8 > > > > Badly (broadly) named constant. Is it not the same that defines arguments in > > the OF phandle lookup? Can you use that instead? > > I'm not sure what you are referring to. This is how many unique instances > of a given resource (GPIOs or regulators) the prober will track. > > MAX_TRACKED_RESOURCES maybe? Better, but still ambiguous. We have a namespace approach, something like I2C_PROBER_... I have checked the existing constant and it's not what you want, so forget about that part, only name of the definition is questionable. ... > > > +#define REGULATOR_SUFFIX "-supply" > > > > Name is bad, also move '-' to the code, it's not part of the suffix, it's a > > separator AFAICT. > > OF_REGULATOR_SUPPLY_SUFFIX then? > > Also, "supply" is not a valid property. It is always "X-supply". > Having the '-' directly in the string makes things simpler, albeit > making the name slightly off. Let's use proper SUFFIX and separator separately. #define I2C_PROBER_..._SUFFIX "supply" (or alike) ... > > > + p = strstr(prop->name, REGULATOR_SUFFIX); > > > > strstr()?! Are you sure it will have no side effects on some interesting names? > > > > > + if (!p) > > > + return 0; > > > > > + if (strcmp(p, REGULATOR_SUFFIX)) > > > + return 0; > > > > Ah, you do it this way... > > > > What about > > About? (feels like an unfinished comment) Yes, sorry for that. Since you found a better alternative, no need to finish this part :-) > Looking around, it seems I could just rename and export "is_supply_name()" > from drivers/regulator/of_regulator.c . Even better! Something similar most likely can be done with GPIO (if not, we are always open to the ideas how to deduplicate the code). ... > > > +#define GPIO_SUFFIX "-gpio" > > > > Bad define name, and why not "gpios"? > > "-gpio" in strstr() would match against both "foo-gpio" and "foo-gpios". > More like laziness. And opens can of worms with whatever ending of the property name. Again, let's have something that GPIO library provides for everybody. ... > > > + ret = of_parse_phandle_with_args_map(node, prop->name, "gpio", 0, &phargs); > > > + if (ret) > > > + return ret; (1) > > > + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober"); > > > + if (IS_ERR(gpiod)) { > > > + of_node_put(phargs.np); > > > + return PTR_ERR(gpiod); > > > + } > > > > Try not to mix fwnode and OF specifics. You may rely on fwnode for GPIO completely. > > Well, fwnode doesn't give a way to identify GPIOs without requesting them. > > Instead I think I could first request GPIOs exclusively, and if that fails > try non-exclusive and see if that GPIO descriptor matches any known one. > If not then something in the DT is broken and it should error out. Then > the phandle parsing code could be dropped. What I meant, the, e.g., (1) can be rewritten using fwnode API, but if you know better way of doing things, then go for it. > > > + if (data->gpiods_num == ARRAY_SIZE(data->gpiods)) { > > > + of_node_put(phargs.np); > > > + gpiod_put(gpiod); > > > + return -ENOMEM; > > > + } ... > > > + for (int i = data->regulators_num; i >= 0; i--) > > > + regulator_put(data->regulators[i]); > > > > Bulk regulators? > > Bulk regulators API uses its own data structure which is not just an > array. So unlike gpiod_*_array() it can't be used in this case. But it sounds like a bulk regulator case... Whatever, it's Mark's area and he might suggest something better. -- With Best Regards, Andy Shevchenko