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=-9.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable 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 39BCEC433DF for ; Thu, 15 Oct 2020 16:32:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CE74D2225A for ; Thu, 15 Oct 2020 16:31:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="elBWdkC0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2390013AbgJOQb7 (ORCPT ); Thu, 15 Oct 2020 12:31:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48084 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389892AbgJOQb7 (ORCPT ); Thu, 15 Oct 2020 12:31:59 -0400 Received: from mail-lj1-x242.google.com (mail-lj1-x242.google.com [IPv6:2a00:1450:4864:20::242]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A9E49C061755 for ; Thu, 15 Oct 2020 09:31:58 -0700 (PDT) Received: by mail-lj1-x242.google.com with SMTP id x16so3824374ljh.2 for ; Thu, 15 Oct 2020 09:31:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EniOGIlKiMQbGzMpLF3yZh6eMy9XXygigTCPkkQmyHo=; b=elBWdkC0Wrb/BP//lSIYmpj9KnbBCmk0ZrOIDdUsOfUoPsS5rsNzoriXR3iR0WRvsm MDBi6TPs9/XFEtk4yuHTlXHowyL2+NkwnvkLofS3CchBWNxoTqLIDWqSfTfJUr2Getl6 xDffJ8KdjyZbMDGqmpH1MUJr5bK1pkT2qs8hI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EniOGIlKiMQbGzMpLF3yZh6eMy9XXygigTCPkkQmyHo=; b=iIbXmY7+sS7iGxbGKIxvhYd19AiItUQSdlaHNDsRsiXHR1r9QYVyKIjAvEoqv1HCYF +EFWu55jybKHRiZ49vcXiueg67FW16swcog5CqifCwYB6p3k9DBnxEhZjRFnNDrczqAQ JKX0HkPHf76Y0gTSl0M+PM7pFcdLJg1lSEjsK3aot2aIrFusLbKqPmYy31hxjUmeObld cwrRvaLDb0VshFTmhox8cZFVGyRGH+3AWlcifjT2RYBDWo2OwqYYI7kr7Bf+BOo3EERq 0ZMVLQV7d9hAXoxEuLSZR5QTcU82PGr6x1VIrqXPDQ/BFnU3+pFCSXRBa5BnpcWtDvBU nPnQ== X-Gm-Message-State: AOAM531avdjMCkcJIimaAXwQ435yyxPY0xZ/cjf0f1uACTL3iS/V85YD LYNlLK1jFdTyDEm7HVU1fBDo49sQVd7H5g== X-Google-Smtp-Source: ABdhPJzf1f9QmqpJV3FKw9qqfeQ68ahOcLKD2jdsdLdfFVMzuPo905MUVUt6RMPlo+U4p+5hKRx60Q== X-Received: by 2002:a2e:b704:: with SMTP id j4mr1499036ljo.108.1602779516784; Thu, 15 Oct 2020 09:31:56 -0700 (PDT) Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com. [209.85.208.173]) by smtp.gmail.com with ESMTPSA id l14sm1470905lji.99.2020.10.15.09.31.55 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 15 Oct 2020 09:31:56 -0700 (PDT) Received: by mail-lj1-f173.google.com with SMTP id x16so3824278ljh.2 for ; Thu, 15 Oct 2020 09:31:55 -0700 (PDT) X-Received: by 2002:a2e:85c4:: with SMTP id h4mr1539454ljj.250.1602779515328; Thu, 15 Oct 2020 09:31:55 -0700 (PDT) MIME-Version: 1.0 References: <20201014180137.v2.1.Idef164c23d326f5e5edecfc5d3eb2a68fcf18be1@changeid> <6ac96561-0415-0ac6-8771-99c8bdee0881@axentia.se> In-Reply-To: <6ac96561-0415-0ac6-8771-99c8bdee0881@axentia.se> From: Evan Green Date: Thu, 15 Oct 2020 09:31:18 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] i2c: i2c-mux-gpio: Enable this driver in ACPI land To: Peter Rosin Cc: Wolfram Sang , Randy Dunlap , Peter Korsgaard , linux-i2c@vger.kernel.org, LKML Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org On Thu, Oct 15, 2020 at 3:23 AM Peter Rosin wrote: > > Hi! > > On 2020-10-15 03:02, Evan Green wrote: > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state > > property translates directly to a fwnode_property_*() call. The child > > reg property translates naturally into _ADR in ACPI. > > > > The i2c-parent binding is a relic from the days when all direct children > > of an i2c controller in Linux had to be i2c devices. These days that > > I2C controller. I2C devices. > > I fail to see why this "relic" has to be explicitly blamed on Linux? In the > beginning, the bindings for all I2C controllers (sometimes implicitely, > sometimes explicitely) specified that all child nodes had to be I2C devices. > The *bindings* were simply not as flexible before the i2c-bus subnode was > invented only a few years ago. So, there are arguments that the "problem" > was in DT-land and that Linux just followed suit. Gotcha, that makes sense. I was probably reading between the lines incorrectly in your previous reply. I'll blame it on the bindings :) > > > implementation detail has been worked out, so the i2c-mux can sit > > as a direct child of its parent controller, which is where it makes the > > most sense from a hardware description perspective. For the ACPI > > implementation we'll assume that's always how the i2c-mux-gpio is > > instantiated. > > There is potential to match this and make i2c-parent optional for the > DT case and require it to be a child of its parent in such cases, if > someone has the time/energy... I won't plan to since I don't have a device like this, but yeah I agree this would be a fine convention for DT to start following as well. > > > > > Signed-off-by: Evan Green > > --- > > > > Changes in v2: > > - Make it compile properly when !CONFIG_ACPI (Randy) > > - Update commit message regarding i2c-parent (Peter) > > > > drivers/i2c/muxes/i2c-mux-gpio.c | 103 ++++++++++++++++++++++--------- > > 1 file changed, 75 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c > > index 4effe563e9e8d..8e4008f4a9b5d 100644 > > --- a/drivers/i2c/muxes/i2c-mux-gpio.c > > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c > > @@ -49,34 +49,80 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan) > > return 0; > > } > > > > -#ifdef CONFIG_OF > > -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux, > > - struct platform_device *pdev) > > +#ifdef CONFIG_ACPI > > + > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev, > > + struct fwnode_handle *fwdev, > > + unsigned int *adr) > > + > > +{ > > + unsigned long long adr64; > > + acpi_status status; > > + > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev), > > + METHOD_NAME__ADR, > > + NULL, &adr64); > > + > > + if (!ACPI_SUCCESS(status)) { > > + dev_err(dev, "Cannot get address"); > > Missing trailing \n Whoops. > > > + return -EINVAL; > > + } > > + > > + *adr = adr64; > > Maybe this is too pedantic? Optional, ignore if I'm just being insane... > > if (*adr != adr64) { > dev_err(dev, "Address out of range\n"); > return -EINVAL; > } > Sure, will add. > > + return 0; > > +} > > + > > +#else > > + > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev, > > + struct fwnode_handle *fwdev, > > + unsigned int *adr) > > +{ > > + return -EINVAL; > > +} > > + > > +#endif > > + > > +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux, > > + struct platform_device *pdev) > > { > > - struct device_node *np = pdev->dev.of_node; > > - struct device_node *adapter_np, *child; > > - struct i2c_adapter *adapter; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + acpi_handle dev_handle; > > Remove the dev_handle declaration here...(push)... > > > + struct device_node *adapter_np; > > + struct i2c_adapter *adapter = NULL; > > + struct fwnode_handle *child = NULL; > > Why do you need these two " = NULL" here? I can't believe compilers are > that stupid? If they are, fine. But otherwise, why single out these two > pointers and add NULL only there and not everywhere? But NULL everywhere > would be ugly... The adapter NULL is there because in theory you could have !is_acpi_node && !is_of_node (maybe in some weird compile test?), and then you would be checking an uninitialized value. child I could safely not initialize, so I'll change that. > > > unsigned *values; > > - int i = 0; > > + int rc, i = 0; > > > > - if (!np) > > - return -ENODEV; > > + if (is_of_node(dev->fwnode)) { > > + if (!np) > > + return -ENODEV; > > > > - adapter_np = of_parse_phandle(np, "i2c-parent", 0); > > - if (!adapter_np) { > > - dev_err(&pdev->dev, "Cannot parse i2c-parent\n"); > > - return -ENODEV; > > + adapter_np = of_parse_phandle(np, "i2c-parent", 0); > > + if (!adapter_np) { > > + dev_err(&pdev->dev, "Cannot parse i2c-parent\n"); > > You should do "&pdev->dev" -> "dev" here, because I hate having > the dev variable and then not use it. But that should perhaps be > a preparatory patch, because I see more instances and this is an > orthogonal change. I see 3 other instances of it in this function. I'll create an initial patch that introduces the dev local and uses it throughout this function, then another patch with my main change on top. > > > + return -ENODEV; > > + } > > + adapter = of_find_i2c_adapter_by_node(adapter_np); > > + of_node_put(adapter_np); > > + > > + } else if (is_acpi_node(dev->fwnode)) { > > + /* > > + * In ACPI land the mux should be a direct child of the i2c > > + * bus it muxes. > > + */ > > + dev_handle = ACPI_HANDLE(dev->parent); > > ...(pop)...and perhaps say > > acpi_handle dev_handle = ACPI_HANDLE(dev->parent); > > here? Will do. Thanks for the prompt review. -Evan