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=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,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 62AA6C48BDF for ; Fri, 18 Jun 2021 09:44:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3D9EE60FE3 for ; Fri, 18 Jun 2021 09:44:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231676AbhFRJq1 (ORCPT ); Fri, 18 Jun 2021 05:46:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37406 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231436AbhFRJqT (ORCPT ); Fri, 18 Jun 2021 05:46:19 -0400 Received: from mail-lf1-x135.google.com (mail-lf1-x135.google.com [IPv6:2a00:1450:4864:20::135]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 24A99C061760 for ; Fri, 18 Jun 2021 02:44:08 -0700 (PDT) Received: by mail-lf1-x135.google.com with SMTP id bp38so15750167lfb.0 for ; Fri, 18 Jun 2021 02:44:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=8MpyH2W/Qi16vZF1RutDfX/LXWayA+uMuphu1a4gDY8=; b=WS97S26sHLsjWjnE4s2jX0/ynLhPoe0+VUb0hrL9k3lZ0LxP5jHd7PkAhea6aF429B Y+qwux8Btsy6lCFc/+i0aB6x+MGHD46ahPFfVf9Kn937OEb7QczXPOCIZ5HHNUyUf3It eRH8Rij5W3fCdq0jCzeEpTpd+D1wycYbbYve+Z6hubf30WMg4FgSff9vD2mA7cJkyhFf 5lWW9sZtAOSfGYqO0hAZaCntF5/qPm97kpkeIYlGl3o7puopBYT1CW/z6FTdet91yO38 sBphYsO/Ty7OXJY8TkssfooKxvef+GmimD8/2MjK1tXm5GRKIrMxXyV0BQP8arKq2Pft ZlIQ== 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=8MpyH2W/Qi16vZF1RutDfX/LXWayA+uMuphu1a4gDY8=; b=TqRuKuSEVyGQROd3BE1pXioOOy1FGEGxZdvgKu4Gm73IVk7l2va/6niv22us4Ijr2w NWZww8uV4A5r6TkDS0ZmX8I4We0i6+Csfa+BHE5Xd06EnqXwciX0gYSiJE/iUjGSY5rg ONYCd77UGTFu/poM7YSLILq/5P43DVmNNOR0nic2CtMGDXKhdPj4VhW/mns2wcSHTDXw /bFGoviw94OBDQgWS2V/xHhT/k4TywsqJTIuCAGkzajfoPn+K7Wh1HLc6jpJI+vFUHdm VfR2vtYgugxf+bbkS1xEelqMUKzWpLLPBaF/YNK+rlT8o/ExbnPfGoq2mEEky3ATcHti waHw== X-Gm-Message-State: AOAM5328qyFVU7ZAr5l8UqCGwjvKfgxxECm1uoMUNJKd30TgdRWmEcmf BEXL/QeffNIcRxvrQFznlyV1GnRtbwjXU9HF+SL43A== X-Google-Smtp-Source: ABdhPJzo4qbbSF3xLRiPOBtDaxlez1Z9u7fCpK1MwfRoaiuf8Qyc5m63k0jWd26FO6A0lO02OSeSEHK0P3YE5fo4/UQ= X-Received: by 2002:a05:6512:3241:: with SMTP id c1mr2595058lfr.29.1624009446392; Fri, 18 Jun 2021 02:44:06 -0700 (PDT) MIME-Version: 1.0 References: <20210615080553.2021061-1-piyush.mehta@xilinx.com> <20210615080553.2021061-3-piyush.mehta@xilinx.com> In-Reply-To: <20210615080553.2021061-3-piyush.mehta@xilinx.com> From: Linus Walleij Date: Fri, 18 Jun 2021 11:43:55 +0200 Message-ID: Subject: Re: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller To: Piyush Mehta Cc: Bartosz Golaszewski , Rob Herring , "open list:GPIO SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , linux-kernel , Linux ARM , git , Srinivas Goud , Michal Simek Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Piyush! thanks for your patch! On Tue, Jun 15, 2021 at 10:06 AM Piyush Mehta wrote: > This patch adds support for the mode pin GPIO controller. GPIO Modepin > driver set and get the value and status of the PS_MODE pin, based on > device-tree pin configuration. These 4-bits boot-mode pins are dedicated > configurable as input/output. After the stabilization of the system, > these mode pins are sampled. > > Signed-off-by: Piyush Mehta OK, sounds interesting! > +#include I think I saw somewhere that this is not needed anymore, check if you need it. > +#define GET_OUTEN_PIN(pin) (1U << (pin)) Delete this macro and just use BIT(pin) inline. #include > +static int modepin_gpio_get_value(struct gpio_chip *chip, unsigned int pin) > +{ > + u32 out_en; > + u32 regval = 0; > + int ret; > + > + out_en = GET_OUTEN_PIN(pin); Drop this and out_en > + ret = zynqmp_pm_bootmode_read(®val); > + if (ret) { > + pr_err("modepin: get value err %d\n", ret); > + return ret; > + } > + > + return (out_en & (regval >> 8U)) ? 1 : 0; return !!(regval & BIT(pin + 8)); should work and is easier to read IMO. We just check the right bit immediately. > +static void modepin_gpio_set_value(struct gpio_chip *chip, unsigned int pin, > + int state) > +{ > + u32 out_en; > + u32 bootpin_val = 0; > + int ret; > + > + out_en = GET_OUTEN_PIN(pin); Skip this helper variable. > + state = state != 0 ? out_en : 0; Uh that is really hard to read and modified a parameter. Skip that too. > + bootpin_val = (state << (8U)) | out_en; What you want is mask and set. bootpin_val = BIT(pin + 8); > + /* Configure bootpin value */ > + ret = zynqmp_pm_bootmode_write(bootpin_val); This just looks weird. Why are you not reading the value first since you are using read/modify/write? I *think* you want to do this: ret = zynqmp_pm_bootmode_read(&val); if (ret) /* error handling */ if (state) val |= BIT(pin + 8); else val &= ~BIT(pin + 8); ret = zynqmp_pm_bootmode_write(val); if (ret) /* error handling */ > +/* > + * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input > + * @chip: gpio_chip instance to be worked on > + * @pin: gpio pin number within the device > + * > + * Return: 0 always > + */ > +static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int pin) > +{ > + return 0; > +} I think you said this was configurable in the commit message. Use the define GPIO_LINE_DIRECTION_OUT rather than 0. > +static int modepin_gpio_dir_out(struct gpio_chip *chip, unsigned int pin, > + int state) > +{ > + return 0; > +} Configurable? > + status = devm_gpiochip_add_data(&pdev->dev, chip, chip); > + if (status) > + dev_err_probe(&pdev->dev, status, > + "Failed to add GPIO chip\n"); just return dev_err_probe(...) Yours, Linus Walleij