| Message ID | 20230816154442.8417-3-asmaa@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix Nvidia BlueField-3 GPIO access | expand |
> > v1->v2: > > - No changes. > > Is this correct? Ah my apologies, I added the wrong comment here. I put it in "v2 1/2". > > +static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip) { > > + unsigned int id = 0; > > + int ret; > > + > > + if (chip->ngpio % MLXBF3_GPIO_MAX_PINS_PER_BLOCK) > > + id = 1; > > This id calculation seems wrong to me as I said in v1 review. > Why do you think the above is what you want and not just working by luck? I would like to get the gpio block id which can only be 0 or 1 on BlueField-3 (only 2 gpio blocks, one with 32 gpio pins and one with 24 gpio pins). The above logic was an "easy" way for me to get the gpio block id. Then the pin_base for each gpio block is: pin_base = id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK
Hi Asmaa, kernel test robot noticed the following build warnings: [auto build test WARNING on linusw-pinctrl/devel] [also build test WARNING on linusw-pinctrl/for-next brgl/gpio/for-next linus/master v6.5-rc6 next-20230816] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/pinctrl-mlxbf3-Remove-gpio_disable_free/20230816-234711 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel patch link: https://lore.kernel.org/r/20230816154442.8417-3-asmaa%40nvidia.com patch subject: [PATCH v2 2/2] gpio: mlxbf3: Support add_pin_ranges() config: i386-buildonly-randconfig-r005-20230817 (https://download.01.org/0day-ci/archive/20230817/202308170926.sCNjFYJH-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce: (https://download.01.org/0day-ci/archive/20230817/202308170926.sCNjFYJH-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308170926.sCNjFYJH-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpio/gpio-mlxbf3.c:164:6: warning: unused variable 'ret' [-Wunused-variable] int ret; ^ 1 warning generated. vim +/ret +164 drivers/gpio/gpio-mlxbf3.c 160 161 static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip) 162 { 163 unsigned int id = 0; > 164 int ret; 165 166 if (chip->ngpio % MLXBF3_GPIO_MAX_PINS_PER_BLOCK) 167 id = 1; 168 169 return gpiochip_add_pin_range(chip, "MLNXBF34:00", 170 chip->base, id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK, 171 chip->ngpio); 172 } 173
On Thu, Aug 17, 2023 at 12:28 AM Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > > > v1->v2: > > > - No changes. > > > > Is this correct? > Ah my apologies, I added the wrong comment here. I put it in "v2 1/2". > > > > +static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip) { > > > + unsigned int id = 0; > > > + int ret; > > > + > > > + if (chip->ngpio % MLXBF3_GPIO_MAX_PINS_PER_BLOCK) > > > + id = 1; > > > > This id calculation seems wrong to me as I said in v1 review. > > Why do you think the above is what you want and not just working by luck? > > I would like to get the gpio block id which can only be 0 or 1 on BlueField-3 (only 2 gpio blocks, one with 32 gpio pins and one with 24 gpio pins). > The above logic was an "easy" way for me to get the gpio block id. Then the pin_base for each gpio block is: > pin_base = id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK It's fragile. Use a direct case switch for that, which will be more explicit and robust (however still can fail for any new chip revision/version where it might be a different GPIO layout).
Hi Asmaa, kernel test robot noticed the following build warnings: [auto build test WARNING on linusw-pinctrl/devel] [also build test WARNING on linusw-pinctrl/for-next brgl/gpio/for-next linus/master v6.5-rc6 next-20230816] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/pinctrl-mlxbf3-Remove-gpio_disable_free/20230816-234711 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel patch link: https://lore.kernel.org/r/20230816154442.8417-3-asmaa%40nvidia.com patch subject: [PATCH v2 2/2] gpio: mlxbf3: Support add_pin_ranges() config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230817/202308171834.7ikT2B4p-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171834.7ikT2B4p-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308171834.7ikT2B4p-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/gpio/gpio-mlxbf3.c: In function 'mlxbf3_gpio_add_pin_ranges': >> drivers/gpio/gpio-mlxbf3.c:164:13: warning: unused variable 'ret' [-Wunused-variable] 164 | int ret; | ^~~ vim +/ret +164 drivers/gpio/gpio-mlxbf3.c 160 161 static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip) 162 { 163 unsigned int id = 0; > 164 int ret; 165 166 if (chip->ngpio % MLXBF3_GPIO_MAX_PINS_PER_BLOCK) 167 id = 1; 168 169 return gpiochip_add_pin_range(chip, "MLNXBF34:00", 170 chip->base, id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK, 171 chip->ngpio); 172 } 173
> > I would like to get the gpio block id which can only be 0 or 1 on BlueField-3 > (only 2 gpio blocks, one with 32 gpio pins and one with 24 gpio pins). > > The above logic was an "easy" way for me to get the gpio block id. Then the > pin_base for each gpio block is: > > pin_base = id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK > > It's fragile. Use a direct case switch for that, which will be more explicit and > robust (however still can fail for any new chip revision/version where it might > be a different GPIO layout). > Thanks Andy! Will do. Hopefully it is too late to change the BF3 hardware at this point so we should be good ; ) .
diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c index e30cee108986..d00cc61ea7b8 100644 --- a/drivers/gpio/gpio-mlxbf3.c +++ b/drivers/gpio/gpio-mlxbf3.c @@ -158,6 +158,19 @@ static const struct irq_chip gpio_mlxbf3_irqchip = { GPIOCHIP_IRQ_RESOURCE_HELPERS, }; +static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip) +{ + unsigned int id = 0; + int ret; + + if (chip->ngpio % MLXBF3_GPIO_MAX_PINS_PER_BLOCK) + id = 1; + + return gpiochip_add_pin_range(chip, "MLNXBF34:00", + chip->base, id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK, + chip->ngpio); +} + static int mlxbf3_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -197,6 +210,7 @@ static int mlxbf3_gpio_probe(struct platform_device *pdev) gc->request = gpiochip_generic_request; gc->free = gpiochip_generic_free; gc->owner = THIS_MODULE; + gc->add_pin_ranges = mlxbf3_gpio_add_pin_ranges; irq = platform_get_irq(pdev, 0); if (irq >= 0) { @@ -243,6 +257,7 @@ static struct platform_driver mlxbf3_gpio_driver = { }; module_platform_driver(mlxbf3_gpio_driver); +MODULE_SOFTDEP("pre: pinctrl-mlxbf3"); MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver"); MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>"); MODULE_LICENSE("Dual BSD/GPL");
Support add_pin_ranges() so that pinctrl_gpio_request() can be called. The GPIO value is not modified when the user runs the "gpioset" tool. This is because when gpiochip_generic_request is invoked by the gpio-mlxbf3 driver, "pin_ranges" is empty so it skips "pinctrl_gpio_request()". pinctrl_gpio_request() is essential in the code flow because it changes the mux value so that software has control over modifying the GPIO value. Adding add_pin_ranges() creates a dependency on the pinctrl-mlxbf3.c driver. Fixes: cd33f216d24 ("gpio: mlxbf3: Add gpio driver support") Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> --- v1->v2: - No changes. drivers/gpio/gpio-mlxbf3.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)