Message ID | 20230725143023.86325-2-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9010:0:b0:3e4:2afc:c1 with SMTP id l16csp2526352vqg; Tue, 25 Jul 2023 07:55:19 -0700 (PDT) X-Google-Smtp-Source: APBJJlEZjJTLfVt8MfBfb4b8wkxJZIo5jkJC3u26vzMW0QI4N2w1ZPmpGdkDweJh9p3ynYXbpPli X-Received: by 2002:a05:6a20:1450:b0:13a:4ba4:4dc5 with SMTP id a16-20020a056a20145000b0013a4ba44dc5mr7451906pzi.60.1690296918839; Tue, 25 Jul 2023 07:55:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690296918; cv=none; d=google.com; s=arc-20160816; b=GZfWcVtrtJlRZjrcvSYT9MuXj2C538llTgoBV63gOJyZts30VrhJ6lCxbbKc81nSDe iuRsofC7tU+jXKlSEeseZbyvey4R6SJGKUku/8vyxCyK0F6zGXan6Zmq9l+4ZTPtadya ajND45cc+qw7eDodB8sulYKTlzIE32qdPFwpIKYm5fRUiUB2566VH6G2m4eNUk/ZYWZ5 oJ8lIZ4NtwZbG4SFFMxf0ex/6xcTsMSxgCAKCIWM6q9eoLzBxQj9hRwB9DHAeEmfET2M iNkI4sJEBZiY1ILzGBroSbje8YHXjuR/fcVy6m5k1mE09KHgsgUSE/hVuEPpd8Wt0Tiq 3hIg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=1GhyqpRvw1kTZ62AoUNcNgBOizA+V+s2cq9jIbHCgT4=; fh=NaDthh2xTRZWW7gK+A73G8SBVbRn5n/sFk5nDI66CNc=; b=YAatHG89ODeIERxzc3aqjQBNhfEvOSSMRB5BYztAXrsElahJnKtk6SII9uVbVxF7DA vRqXbfOwodgHbfHIee7oI4HiHzjkvhjkxNUNldZvb6g0mKBalsOVNSRdna7cfSAFLAKx eaUDshbPGc435+TJZkHOQtkZgr/xSg6L9aeex77f+Y1sDrNkb0GAQwSxp0eKRdAK9HRE WSSvAEyeh79H/Z1ey81A2SAIJ+2mtxSybd6D28AJZBb8LK8oc77pxfRII8Y/ThDdY8Q7 Yvy0XtAQY4LvQlYAwV6DTht9O5Dhqm2jqes3IpfVZ89BUyy/rKAkseHK99EXzwekB8f3 JKnQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=iAT1YKHj; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id x10-20020a056a00188a00b0068231201456si12313355pfh.173.2023.07.25.07.55.05; Tue, 25 Jul 2023 07:55:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=iAT1YKHj; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232663AbjGYOa1 (ORCPT <rfc822;kautuk.consul.80@gmail.com> + 99 others); Tue, 25 Jul 2023 10:30:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42918 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231941AbjGYOaW (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 25 Jul 2023 10:30:22 -0400 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E5ED091; Tue, 25 Jul 2023 07:30:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1690295422; x=1721831422; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=CIw2IxUCCqjyWS0CA3LTB+Jmwhhx7Qzi2MvLY6OymHs=; b=iAT1YKHjZ7Z2Nla8qqdgAHtWceL8PCrQTlZVBblKUx0a0GXeg5nCNwCK 7taYxgrFLHFLgnQBxDs76gUHTq0x8JNb0Kj2AcWqJSpHEg0KRAUD/uyd3 fz4OUcsnmWg83rzgnWK/hKY+fPbUzc16FjxGm/bqnaAnS8Btyl1iwtiG7 Qwy5Bhc/B7L5Vaw+IKnop4mzksdP59AA0T+tJFABuP9/c/dnFWxRyBnud 7YxRYis0GCyGDTJTsDNrX1XRxfkgwNWsdnUnVQHhEOVx7UpEA437dwhXA HS2D9bR4JP4751EB70JnlEQbIOq64Kp0el8qInLBPXm0JUkbbS8C//qlg A==; X-IronPort-AV: E=McAfee;i="6600,9927,10782"; a="433992461" X-IronPort-AV: E=Sophos;i="6.01,230,1684825200"; d="scan'208";a="433992461" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jul 2023 07:30:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10782"; a="1056804937" X-IronPort-AV: E=Sophos;i="6.01,230,1684825200"; d="scan'208";a="1056804937" Received: from black.fi.intel.com ([10.237.72.28]) by fmsmga005.fm.intel.com with ESMTP; 25 Jul 2023 07:30:16 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id 96B1D155; Tue, 25 Jul 2023 17:30:24 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Jarkko Nikula <jarkko.nikula@linux.intel.com>, Mario Limonciello <mario.limonciello@amd.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Mika Westerberg <mika.westerberg@linux.intel.com>, Jan Dabros <jsd@semihalf.com>, Andi Shyti <andi.shyti@kernel.org> Subject: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code Date: Tue, 25 Jul 2023 17:30:15 +0300 Message-Id: <20230725143023.86325-2-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b In-Reply-To: <20230725143023.86325-1-andriy.shevchenko@linux.intel.com> References: <20230725143023.86325-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772404782033252521 X-GMAIL-MSGID: 1772404782033252521 |
Series |
i2c: designware: code consolidation & cleanups
|
|
Commit Message
Andy Shevchenko
July 25, 2023, 2:30 p.m. UTC
Instead of checking in callers, move the call to the callee.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/i2c/busses/i2c-designware-common.c | 11 +++++++++--
drivers/i2c/busses/i2c-designware-pcidrv.c | 3 +--
drivers/i2c/busses/i2c-designware-platdrv.c | 3 +--
3 files changed, 11 insertions(+), 6 deletions(-)
Comments
Hi Andy, On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote: > Instead of checking in callers, move the call to the callee. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-common.c | 11 +++++++++-- > drivers/i2c/busses/i2c-designware-pcidrv.c | 3 +-- > drivers/i2c/busses/i2c-designware-platdrv.c | 3 +-- > 3 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c > index cdd8c67d9129..683f7a9beb46 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[], > kfree(buf.pointer); > } > > -int i2c_dw_acpi_configure(struct device *device) > +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) > { > - struct dw_i2c_dev *dev = dev_get_drvdata(device); > struct i2c_timings *t = &dev->timings; > u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0; > > @@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device) > dev->sda_hold_time = fs_ht; > break; > } > +} > + > +int i2c_dw_acpi_configure(struct device *device) I was about to ask you why are we keeping this int, but then I saw that you are making it void in the next patch :) Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
On 7/26/23 00:45, Andi Shyti wrote: > Hi Andy, > > On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote: >> Instead of checking in callers, move the call to the callee. >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/i2c/busses/i2c-designware-common.c | 11 +++++++++-- >> drivers/i2c/busses/i2c-designware-pcidrv.c | 3 +-- >> drivers/i2c/busses/i2c-designware-platdrv.c | 3 +-- >> 3 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c >> index cdd8c67d9129..683f7a9beb46 100644 >> --- a/drivers/i2c/busses/i2c-designware-common.c >> +++ b/drivers/i2c/busses/i2c-designware-common.c >> @@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[], >> kfree(buf.pointer); >> } >> >> -int i2c_dw_acpi_configure(struct device *device) >> +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) Because of this dual dev pointer obscurity which is cleaned in the next patch and Andi's comment below in my opinion it makes sense to combine patches 1 and 2. >> { >> - struct dw_i2c_dev *dev = dev_get_drvdata(device); >> struct i2c_timings *t = &dev->timings; >> u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0; >> >> @@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device) >> dev->sda_hold_time = fs_ht; >> break; >> } >> +} >> + >> +int i2c_dw_acpi_configure(struct device *device) > > I was about to ask you why are we keeping this int, but then I > saw that you are making it void in the next patch :) >
On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote: > On 7/26/23 00:45, Andi Shyti wrote: > > On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote: ... > > > -int i2c_dw_acpi_configure(struct device *device) > > > +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) > > Because of this dual dev pointer obscurity which is cleaned in the next > patch and Andi's comment below in my opinion it makes sense to combine > patches 1 and 2. Besides that these 2 are logically slightly different, the changes don't drop the duality here. And there is also the other patch at the end of the series that makes the below disappear. Not sure that any of these would be the best approach (Git commit is cheap, maintenance and backporting might be harder). So, ideas are welcome! ... > > > +int i2c_dw_acpi_configure(struct device *device) > > > > I was about to ask you why are we keeping this int, but then I > > saw that you are making it void in the next patch :)
On 7/31/23 23:14, Andy Shevchenko wrote: > On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote: >> On 7/26/23 00:45, Andi Shyti wrote: >>> On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote: > > ... > >>>> -int i2c_dw_acpi_configure(struct device *device) >>>> +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) >> >> Because of this dual dev pointer obscurity which is cleaned in the next >> patch and Andi's comment below in my opinion it makes sense to combine >> patches 1 and 2. > > Besides that these 2 are logically slightly different, the changes don't drop > the duality here. And there is also the other patch at the end of the series > that makes the below disappear. > > Not sure that any of these would be the best approach (Git commit is cheap, > maintenance and backporting might be harder). So, ideas are welcome! > Unless I'm missing something you won't need to carry both struct dw_i2c_dev *dev and struct device *device since struct dw_i2c_dev carries it already and it's set before calling the dw_i2c_of_configure() and i2c_dw_acpi_configure(). Also it feels needless to add new _do_configure() functions since only reason for them seems to be how patches are organized now. So if instead of this in i2c_dw_fw_parse_and_configure() if (is_of_node(fwnode)) i2c_dw_of_do_configure(dev, dev->dev); else if (is_acpi_node(fwnode)) i2c_dw_acpi_do_configure(dev, dev->dev); let end result be if (is_of_node(fwnode)) i2c_dw_of_configure(dev); else if (is_acpi_node(fwnode)) i2c_dw_acpi_configure(dev); My gut feeling says patchset would be a bit simpler if we aim for this end result in mind. Simplest patches like int to void return type conversion first since either i2c_dw_acpi_configure() and dw_i2c_of_configure() return is not used now. Then perhaps dw_i2c_of_configure() renaming. Moving to common code I don't know how well it's splittable into smaller patches or would single bigger patch look better.
On Thu, Aug 03, 2023 at 04:43:32PM +0300, Jarkko Nikula wrote: > On 7/31/23 23:14, Andy Shevchenko wrote: > > On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote: > > > On 7/26/23 00:45, Andi Shyti wrote: > > > > On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote: > > > > ... > > > > > > > -int i2c_dw_acpi_configure(struct device *device) > > > > > +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) > > > > > > Because of this dual dev pointer obscurity which is cleaned in the next > > > patch and Andi's comment below in my opinion it makes sense to combine > > > patches 1 and 2. > > > > Besides that these 2 are logically slightly different, the changes don't drop > > the duality here. And there is also the other patch at the end of the series > > that makes the below disappear. > > > > Not sure that any of these would be the best approach (Git commit is cheap, > > maintenance and backporting might be harder). So, ideas are welcome! > > > Unless I'm missing something you won't need to carry both struct dw_i2c_dev > *dev and struct device *device since struct dw_i2c_dev carries it already > and it's set before calling the dw_i2c_of_configure() and > i2c_dw_acpi_configure(). > > Also it feels needless to add new _do_configure() functions since only > reason for them seems to be how patches are organized now. > > So if instead of this in i2c_dw_fw_parse_and_configure() > > if (is_of_node(fwnode)) > i2c_dw_of_do_configure(dev, dev->dev); > else if (is_acpi_node(fwnode)) > i2c_dw_acpi_do_configure(dev, dev->dev); > > let end result be > > if (is_of_node(fwnode)) > i2c_dw_of_configure(dev); > else if (is_acpi_node(fwnode)) > i2c_dw_acpi_configure(dev); > > My gut feeling says patchset would be a bit simpler if we aim for this end > result in mind. > > Simplest patches like int to void return type conversion first since either > i2c_dw_acpi_configure() and dw_i2c_of_configure() return is not used now. > Then perhaps dw_i2c_of_configure() renaming. > > Moving to common code I don't know how well it's splittable into smaller > patches or would single bigger patch look better. Does this all mean that the series needs to be refactored?
On Sun, Sep 24, 2023 at 10:56:00PM +0200, Wolfram Sang wrote: > On Thu, Aug 03, 2023 at 04:43:32PM +0300, Jarkko Nikula wrote: > > On 7/31/23 23:14, Andy Shevchenko wrote: ... > > Moving to common code I don't know how well it's splittable into smaller > > patches or would single bigger patch look better. > > Does this all mean that the series needs to be refactored? At least that is my impression. Just have no time to look at it (yet).
> > Does this all mean that the series needs to be refactored? > > At least that is my impression. Just have no time to look at it (yet). Okay, I'll set it to 'changes requested' then. Thanks for the heads up!
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index cdd8c67d9129..683f7a9beb46 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[], kfree(buf.pointer); } -int i2c_dw_acpi_configure(struct device *device) +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) { - struct dw_i2c_dev *dev = dev_get_drvdata(device); struct i2c_timings *t = &dev->timings; u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0; @@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device) dev->sda_hold_time = fs_ht; break; } +} + +int i2c_dw_acpi_configure(struct device *device) +{ + struct dw_i2c_dev *dev = dev_get_drvdata(device); + + if (has_acpi_companion(device)) + i2c_dw_acpi_do_configure(dev, device); return 0; } diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 61d7a27aa070..d9d80650fbdc 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -303,8 +303,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, i2c_dw_adjust_bus_speed(dev); - if (has_acpi_companion(&pdev->dev)) - i2c_dw_acpi_configure(&pdev->dev); + i2c_dw_acpi_configure(&pdev->dev); r = i2c_dw_validate_speed(dev); if (r) { diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 970c1c3b0402..4eedb0734438 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -314,8 +314,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) if (pdev->dev.of_node) dw_i2c_of_configure(pdev); - if (has_acpi_companion(&pdev->dev)) - i2c_dw_acpi_configure(&pdev->dev); + i2c_dw_acpi_configure(&pdev->dev); ret = i2c_dw_validate_speed(dev); if (ret)