| Message ID | 20241101080614.1070819-5-andrei.stefanescu@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: siul2-s32g2: add initial GPIO driver | expand |
On Fri, Nov 1, 2024 at 9:06 AM Andrei Stefanescu <andrei.stefanescu@oss.nxp.com> wrote: > The SIUL2 module is now represented as an mfd device. The pinctrl driver > is now an mfd_cell. Therefore, remove its compatible and adjust its > probing in order to get the necessary information from its mfd parent. > > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> I assume this needs to go in with the rest of the patches. Yours, Linus Walleij
Hi Andrei, 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 lee-mfd/for-mfd-next shawnguo/for-next linus/master v6.12-rc5 next-20241101] [cannot apply to lee-mfd/for-mfd-fixes] [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/Andrei-Stefanescu/dt-bindings-mfd-add-support-for-the-NXP-SIUL2-module/20241101-160940 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel patch link: https://lore.kernel.org/r/20241101080614.1070819-5-andrei.stefanescu%40oss.nxp.com patch subject: [PATCH v5 4/7] pinctrl: s32: convert the driver into an mfd cell config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20241102/202411020514.qOUrieWa-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 639a7ac648f1e50ccd2556e17d401c04f9cce625) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241102/202411020514.qOUrieWa-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/202411020514.qOUrieWa-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/pinctrl/nxp/pinctrl-s32cc.c:103: warning: Function parameter or struct member 'gpio_configs_lock' not described in 's32_pinctrl' >> drivers/pinctrl/nxp/pinctrl-s32cc.c:103: warning: Excess struct member 'gpiop_configs_lock' description in 's32_pinctrl' Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for MODVERSIONS Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y] Selected by [y]: - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y] vim +103 drivers/pinctrl/nxp/pinctrl-s32cc.c fd84aaa8173d3f Chester Lin 2023-02-20 82 157a51f7e5e81e Andrei Stefanescu 2024-11-01 83 /** 157a51f7e5e81e Andrei Stefanescu 2024-11-01 84 * struct s32_pinctrl - private driver data fd84aaa8173d3f Chester Lin 2023-02-20 85 * @dev: a pointer back to containing device fd84aaa8173d3f Chester Lin 2023-02-20 86 * @pctl: a pointer to the pinctrl device structure fd84aaa8173d3f Chester Lin 2023-02-20 87 * @regions: reserved memory regions with start/end pin fd84aaa8173d3f Chester Lin 2023-02-20 88 * @info: structure containing information about the pin fd84aaa8173d3f Chester Lin 2023-02-20 89 * @gpio_configs: Saved configurations for GPIO pins fd84aaa8173d3f Chester Lin 2023-02-20 90 * @gpiop_configs_lock: lock for the `gpio_configs` list 157a51f7e5e81e Andrei Stefanescu 2024-11-01 91 * @saved_context: Configuration saved over system sleep fd84aaa8173d3f Chester Lin 2023-02-20 92 */ fd84aaa8173d3f Chester Lin 2023-02-20 93 struct s32_pinctrl { fd84aaa8173d3f Chester Lin 2023-02-20 94 struct device *dev; fd84aaa8173d3f Chester Lin 2023-02-20 95 struct pinctrl_dev *pctl; fd84aaa8173d3f Chester Lin 2023-02-20 96 struct s32_pinctrl_mem_region *regions; fd84aaa8173d3f Chester Lin 2023-02-20 97 struct s32_pinctrl_soc_info *info; fd84aaa8173d3f Chester Lin 2023-02-20 98 struct list_head gpio_configs; fd84aaa8173d3f Chester Lin 2023-02-20 99 spinlock_t gpio_configs_lock; fd84aaa8173d3f Chester Lin 2023-02-20 100 #ifdef CONFIG_PM_SLEEP fd84aaa8173d3f Chester Lin 2023-02-20 101 struct s32_pinctrl_context saved_context; fd84aaa8173d3f Chester Lin 2023-02-20 102 #endif fd84aaa8173d3f Chester Lin 2023-02-20 @103 }; fd84aaa8173d3f Chester Lin 2023-02-20 104
On Fri, Nov 01, 2024 at 10:06:10AM +0200, Andrei Stefanescu wrote: > + /* Order is MSCR regions first, then IMCR ones */ > for (i = 0; i < mem_regions; i++) { > - base = devm_platform_get_and_ioremap_resource(pdev, i, &res); > - if (IS_ERR(base)) > - return PTR_ERR(base); > - > - snprintf(ipctl->regions[i].name, > - sizeof(ipctl->regions[i].name), "map%u", i); > - > - s32_regmap_config.name = ipctl->regions[i].name; > - s32_regmap_config.max_register = resource_size(res) - > - s32_regmap_config.reg_stride; > - > - map = devm_regmap_init_mmio(&pdev->dev, base, > - &s32_regmap_config); > - if (IS_ERR(map)) { > - dev_err(&pdev->dev, "Failed to init regmap[%u]\n", i); > - return PTR_ERR(map); > - } > - > - ipctl->regions[i].map = map; > + regmap_type = i < mem_regions / 2 ? SIUL2_MSCR : SIUL2_IMCR; > + j = i % mfd->num_siul2; > + ipctl->regions[i].map = mfd->siul2[j].regmaps[regmap_type]; > ipctl->regions[i].pin_range = &info->soc_data->mem_pin_ranges[i]; This looks like breaking all the users. Nothing in commit msg about this: about rationale, impact or backwards compatibility. Nothing in changelog in cover letter explaining such sudden change in approach. Sorry, that's a NAK. Best regards, Krzysztof
Hi Krzysztof, On 02/11/2024 10:51, Krzysztof Kozlowski wrote: > On Fri, Nov 01, 2024 at 10:06:10AM +0200, Andrei Stefanescu wrote: >> + /* Order is MSCR regions first, then IMCR ones */ >> for (i = 0; i < mem_regions; i++) { >> - base = devm_platform_get_and_ioremap_resource(pdev, i, &res); >> - if (IS_ERR(base)) >> - return PTR_ERR(base); >> - >> - snprintf(ipctl->regions[i].name, >> - sizeof(ipctl->regions[i].name), "map%u", i); >> - >> - s32_regmap_config.name = ipctl->regions[i].name; >> - s32_regmap_config.max_register = resource_size(res) - >> - s32_regmap_config.reg_stride; >> - >> - map = devm_regmap_init_mmio(&pdev->dev, base, >> - &s32_regmap_config); >> - if (IS_ERR(map)) { >> - dev_err(&pdev->dev, "Failed to init regmap[%u]\n", i); >> - return PTR_ERR(map); >> - } >> - >> - ipctl->regions[i].map = map; >> + regmap_type = i < mem_regions / 2 ? SIUL2_MSCR : SIUL2_IMCR; >> + j = i % mfd->num_siul2; >> + ipctl->regions[i].map = mfd->siul2[j].regmaps[regmap_type]; >> ipctl->regions[i].pin_range = &info->soc_data->mem_pin_ranges[i]; > > This looks like breaking all the users. Nothing in commit msg about > this: about rationale, impact or backwards compatibility. > > Nothing in changelog in cover letter explaining such sudden change in > approach. > > Sorry, that's a NAK. I will add a detailed explanation for the change in v6. I changed the existing implementation because of feedback received in this series and in [0]. The SIUL2 module has the pinctrl&GPIO functionality tightly coupled, which required us to carve out the registers between GPIO & pinctrl and it resulted in a long and detailed list. Also, in the future, we will also have some nvmem cells exported which are part of SIUL2. Best regards, Andrei [0] - https://lore.kernel.org/linux-gpio/20241002135920.3647322-1-andrei.stefanescu@oss.nxp.com/ > > Best regards, > Krzysztof >
diff --git a/drivers/pinctrl/nxp/pinctrl-s32.h b/drivers/pinctrl/nxp/pinctrl-s32.h index add3c77ddfed..829211741050 100644 --- a/drivers/pinctrl/nxp/pinctrl-s32.h +++ b/drivers/pinctrl/nxp/pinctrl-s32.h @@ -38,6 +38,7 @@ struct s32_pinctrl_soc_data { const struct pinctrl_pin_desc *pins; unsigned int npins; const struct s32_pin_range *mem_pin_ranges; + const struct regmap **regmaps; unsigned int mem_regions; }; diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c index 501eb296c760..709e823b9c7c 100644 --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c @@ -12,6 +12,7 @@ #include <linux/gpio/driver.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/mfd/nxp-siul2.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> @@ -44,12 +45,6 @@ enum s32_write_type { S32_PINCONF_OVERWRITE, }; -static struct regmap_config s32_regmap_config = { - .reg_bits = 32, - .val_bits = 32, - .reg_stride = 4, -}; - static u32 get_pin_no(u32 pinmux) { return (pinmux & S32_PIN_ID_MASK) >> S32_PIN_ID_SHIFT; @@ -85,14 +80,15 @@ struct s32_pinctrl_context { unsigned int *pads; }; -/* +/** + * struct s32_pinctrl - private driver data * @dev: a pointer back to containing device * @pctl: a pointer to the pinctrl device structure * @regions: reserved memory regions with start/end pin * @info: structure containing information about the pin * @gpio_configs: Saved configurations for GPIO pins * @gpiop_configs_lock: lock for the `gpio_configs` list - * @s32_pinctrl_context: Configuration saved over system sleep + * @saved_context: Configuration saved over system sleep */ struct s32_pinctrl { struct device *dev; @@ -123,14 +119,13 @@ s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin) return NULL; } -static inline int s32_check_pin(struct pinctrl_dev *pctldev, - unsigned int pin) +static int s32_check_pin(struct pinctrl_dev *pctldev, unsigned int pin) { return s32_get_region(pctldev, pin) ? 0 : -EINVAL; } -static inline int s32_regmap_read(struct pinctrl_dev *pctldev, - unsigned int pin, unsigned int *val) +static int s32_regmap_read(struct pinctrl_dev *pctldev, unsigned int pin, + unsigned int *val) { struct s32_pinctrl_mem_region *region; unsigned int offset; @@ -145,7 +140,7 @@ static inline int s32_regmap_read(struct pinctrl_dev *pctldev, return regmap_read(region->map, offset, val); } -static inline int s32_regmap_write(struct pinctrl_dev *pctldev, +static int s32_regmap_write(struct pinctrl_dev *pctldev, unsigned int pin, unsigned int val) { @@ -163,7 +158,7 @@ static inline int s32_regmap_write(struct pinctrl_dev *pctldev, } -static inline int s32_regmap_update(struct pinctrl_dev *pctldev, unsigned int pin, +static int s32_regmap_update(struct pinctrl_dev *pctldev, unsigned int pin, unsigned int mask, unsigned int val) { struct s32_pinctrl_mem_region *region; @@ -475,8 +470,8 @@ static int s32_get_slew_regval(int arg) return -EINVAL; } -static inline void s32_pin_set_pull(enum pin_config_param param, - unsigned int *mask, unsigned int *config) +static void s32_pin_set_pull(enum pin_config_param param, + unsigned int *mask, unsigned int *config) { switch (param) { case PIN_CONFIG_BIAS_DISABLE: @@ -838,20 +833,21 @@ static int s32_pinctrl_parse_functions(struct device_node *np, static int s32_pinctrl_probe_dt(struct platform_device *pdev, struct s32_pinctrl *ipctl) { + struct nxp_siul2_mfd *mfd = dev_get_drvdata(pdev->dev.parent); struct s32_pinctrl_soc_info *info = ipctl->info; - struct device_node *np = pdev->dev.of_node; - struct resource *res; - struct regmap *map; - void __iomem *base; - unsigned int mem_regions = info->soc_data->mem_regions; + unsigned int mem_regions; + struct device_node *np; + u32 nfuncs = 0, i = 0, j; + u8 regmap_type; int ret; - u32 nfuncs = 0; - u32 i = 0; + np = pdev->dev.parent->of_node; if (!np) return -ENODEV; - if (mem_regions == 0 || mem_regions >= 10000) { + /* one MSCR and one IMCR region per SIUL2 module */ + mem_regions = info->soc_data->mem_regions; + if (mem_regions != mfd->num_siul2 * 2) { dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions); return -EINVAL; } @@ -861,26 +857,11 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev, if (!ipctl->regions) return -ENOMEM; + /* Order is MSCR regions first, then IMCR ones */ for (i = 0; i < mem_regions; i++) { - base = devm_platform_get_and_ioremap_resource(pdev, i, &res); - if (IS_ERR(base)) - return PTR_ERR(base); - - snprintf(ipctl->regions[i].name, - sizeof(ipctl->regions[i].name), "map%u", i); - - s32_regmap_config.name = ipctl->regions[i].name; - s32_regmap_config.max_register = resource_size(res) - - s32_regmap_config.reg_stride; - - map = devm_regmap_init_mmio(&pdev->dev, base, - &s32_regmap_config); - if (IS_ERR(map)) { - dev_err(&pdev->dev, "Failed to init regmap[%u]\n", i); - return PTR_ERR(map); - } - - ipctl->regions[i].map = map; + regmap_type = i < mem_regions / 2 ? SIUL2_MSCR : SIUL2_IMCR; + j = i % mfd->num_siul2; + ipctl->regions[i].map = mfd->siul2[j].regmaps[regmap_type]; ipctl->regions[i].pin_range = &info->soc_data->mem_pin_ranges[i]; } @@ -918,13 +899,13 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev, int s32_pinctrl_probe(struct platform_device *pdev, const struct s32_pinctrl_soc_data *soc_data) { - struct s32_pinctrl *ipctl; - int ret; - struct pinctrl_desc *s32_pinctrl_desc; - struct s32_pinctrl_soc_info *info; #ifdef CONFIG_PM_SLEEP struct s32_pinctrl_context *saved_context; #endif + struct pinctrl_desc *s32_pinctrl_desc; + struct s32_pinctrl_soc_info *info; + struct s32_pinctrl *ipctl; + int ret; if (!soc_data || !soc_data->pins || !soc_data->npins) { dev_err(&pdev->dev, "wrong pinctrl info\n"); diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c index 440ff1879424..9c7fe545cc85 100644 --- a/drivers/pinctrl/nxp/pinctrl-s32g2.c +++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c @@ -10,6 +10,7 @@ #include <linux/err.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/mfd/nxp-siul2.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> @@ -713,12 +714,10 @@ static const struct pinctrl_pin_desc s32_pinctrl_pads_siul2[] = { static const struct s32_pin_range s32_pin_ranges_siul2[] = { /* MSCR pin ID ranges */ S32_PIN_RANGE(0, 101), - S32_PIN_RANGE(112, 122), - S32_PIN_RANGE(144, 190), + S32_PIN_RANGE(112, 190), /* IMCR pin ID ranges */ S32_PIN_RANGE(512, 595), - S32_PIN_RANGE(631, 909), - S32_PIN_RANGE(942, 1007), + S32_PIN_RANGE(631, 1007), }; static const struct s32_pinctrl_soc_data s32_pinctrl_data = { @@ -728,22 +727,9 @@ static const struct s32_pinctrl_soc_data s32_pinctrl_data = { .mem_regions = ARRAY_SIZE(s32_pin_ranges_siul2), }; -static const struct of_device_id s32_pinctrl_of_match[] = { - { - .compatible = "nxp,s32g2-siul2-pinctrl", - .data = &s32_pinctrl_data, - }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match); - static int s32g_pinctrl_probe(struct platform_device *pdev) { - const struct s32_pinctrl_soc_data *soc_data; - - soc_data = of_device_get_match_data(&pdev->dev); - - return s32_pinctrl_probe(pdev, soc_data); + return s32_pinctrl_probe(pdev, &s32_pinctrl_data); } static const struct dev_pm_ops s32g_pinctrl_pm_ops = { @@ -753,7 +739,6 @@ static const struct dev_pm_ops s32g_pinctrl_pm_ops = { static struct platform_driver s32g_pinctrl_driver = { .driver = { .name = "s32g-siul2-pinctrl", - .of_match_table = s32_pinctrl_of_match, .pm = pm_sleep_ptr(&s32g_pinctrl_pm_ops), .suppress_bind_attrs = true, },
The SIUL2 module is now represented as an mfd device. The pinctrl driver is now an mfd_cell. Therefore, remove its compatible and adjust its probing in order to get the necessary information from its mfd parent. Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com> --- drivers/pinctrl/nxp/pinctrl-s32.h | 1 + drivers/pinctrl/nxp/pinctrl-s32cc.c | 75 +++++++++++------------------ drivers/pinctrl/nxp/pinctrl-s32g2.c | 23 ++------- 3 files changed, 33 insertions(+), 66 deletions(-)