From: Linus Walleij <linus.walleij@linaro.org> To: Piyush Mehta <piyush.mehta@xilinx.com> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>, Rob Herring <robh+dt@kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, git <git@xilinx.com>, Srinivas Goud <sgoud@xilinx.com>, Michal Simek <michal.simek@xilinx.com> Subject: Re: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Date: Fri, 18 Jun 2021 11:43:55 +0200 [thread overview] Message-ID: <CACRpkdYv6yosZ1KJazrMzaizpYz-cv-y4LcCqHm+Q94jva8sAA@mail.gmail.com> (raw) In-Reply-To: <20210615080553.2021061-3-piyush.mehta@xilinx.com> Hi Piyush! thanks for your patch! On Tue, Jun 15, 2021 at 10:06 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> OK, sounds interesting! > +#include <linux/slab.h> 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 <linux/bits.h> > +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
WARNING: multiple messages have this Message-ID (diff)
From: Linus Walleij <linus.walleij@linaro.org> To: Piyush Mehta <piyush.mehta@xilinx.com> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>, Rob Herring <robh+dt@kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, git <git@xilinx.com>, Srinivas Goud <sgoud@xilinx.com>, Michal Simek <michal.simek@xilinx.com> Subject: Re: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Date: Fri, 18 Jun 2021 11:43:55 +0200 [thread overview] Message-ID: <CACRpkdYv6yosZ1KJazrMzaizpYz-cv-y4LcCqHm+Q94jva8sAA@mail.gmail.com> (raw) In-Reply-To: <20210615080553.2021061-3-piyush.mehta@xilinx.com> Hi Piyush! thanks for your patch! On Tue, Jun 15, 2021 at 10:06 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> OK, sounds interesting! > +#include <linux/slab.h> 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 <linux/bits.h> > +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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-18 9:44 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-15 8:05 [PATCH 0/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta 2021-06-15 8:05 ` Piyush Mehta 2021-06-15 8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta 2021-06-15 8:05 ` Piyush Mehta 2021-06-15 10:28 ` Michal Simek 2021-06-15 10:28 ` Michal Simek 2021-06-24 20:50 ` Rob Herring 2021-06-24 20:50 ` Rob Herring 2021-06-15 8:05 ` [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta 2021-06-15 8:05 ` Piyush Mehta 2021-06-18 9:43 ` Linus Walleij [this message] 2021-06-18 9:43 ` Linus Walleij 2021-06-21 17:09 ` Piyush Mehta 2021-06-21 17:09 ` Piyush Mehta
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACRpkdYv6yosZ1KJazrMzaizpYz-cv-y4LcCqHm+Q94jva8sAA@mail.gmail.com \ --to=linus.walleij@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=devicetree@vger.kernel.org \ --cc=git@xilinx.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=michal.simek@xilinx.com \ --cc=piyush.mehta@xilinx.com \ --cc=robh+dt@kernel.org \ --cc=sgoud@xilinx.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.