| Message ID | 20211029092703.18886-1-kavyasree.kotagiri@microchip.com |
|---|---|
| Headers | show |
| Series | Extend pinctrl-ocelot driver for lan966x | expand |
On Fri, Oct 29, 2021 at 11:27 AM Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> wrote: > This patch series extends pinctrl-ocelot driver to support also > the lan966x. Alexandre Belloni and Quentin Schultz worked on this driver a lot, so paging them for feedback/review. Yours, Linus Walleij
Hi Alex and Quentin, Please provide your comments on this patch series. Thanks, Kavya > -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Tuesday, November 9, 2021 10:29 AM > To: Kavyasree Kotagiri - I30978 <Kavyasree.Kotagiri@microchip.com>; > Alexandre Belloni <alexandre.belloni@bootlin.com>; > quentin.schulz@bootlin.com > Cc: robh+dt@kernel.org; linux-gpio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 0/2] Extend pinctrl-ocelot driver for lan966x > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Fri, Oct 29, 2021 at 11:27 AM Kavyasree Kotagiri > <kavyasree.kotagiri@microchip.com> wrote: > > > This patch series extends pinctrl-ocelot driver to support also > > the lan966x. > > Alexandre Belloni and Quentin Schultz worked on this > driver a lot, so paging them for feedback/review. > > Yours, > Linus Walleij
Hello Kavya, On 29/10/2021 14:57:03+0530, Kavyasree Kotagiri wrote: > + LAN966X_PIN(76), > + LAN966X_PIN(77), > +}; > + > + One blank line should be removed > static int ocelot_get_functions_count(struct pinctrl_dev *pctldev) > { > return ARRAY_SIZE(ocelot_function_names); > @@ -709,6 +1056,9 @@ static int ocelot_pin_function_idx(struct ocelot_pinctrl *info, > for (i = 0; i < OCELOT_FUNC_PER_PIN; i++) { > if (function == p->functions[i]) > return i; > + > + if (function == p->a_functions[i]) > + return i + OCELOT_FUNC_PER_PIN; > } > > return -1; > @@ -744,6 +1094,36 @@ static int ocelot_pinmux_set_mux(struct pinctrl_dev *pctldev, > return 0; > } > > +static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned int group) > +{ > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > + struct ocelot_pin_caps *pin = info->desc->pins[group].drv_data; > + unsigned int p = pin->pin % 32; > + int f; > + > + f = ocelot_pin_function_idx(info, group, selector); > + if (f < 0) > + return -EINVAL; > + > + /* > + * f is encoded on three bits. > + * bit 0 of f goes in BIT(pin) of ALT[0], bit 1 of f goes in BIT(pin) of > + * ALT[1], bit 2 of f goes in BIT(pin) of ALT[2] > + * This is racy because both registers can't be updated at the same time That's three registers, not two so I guess this sentence should be reworked. > + * but it doesn't matter much for now. > + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets > + */ > + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin), > + BIT(p), f << p); > + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin), > + BIT(p), (f >> 1) << p); > + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin), > + BIT(p), (f >> 2) << p); > + I would have thought the hardware would be fixed because you now have 78 pins and this probably will get worse over time. This is really a poor choice of interface as now you will get two transient states instead of one. > + return 0; > +} > + > #define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32))) > > static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev, > @@ -774,6 +1154,23 @@ static int ocelot_gpio_request_enable(struct pinctrl_dev *pctldev, > return 0; > } > > +static int lan966x_gpio_request_enable(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned int offset) > +{ > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > + unsigned int p = offset % 32; > + > + regmap_update_bits(info->map, REG_ALT(0, info, offset), > + BIT(p), 0); > + regmap_update_bits(info->map, REG_ALT(1, info, offset), > + BIT(p), 0); > + regmap_update_bits(info->map, REG_ALT(2, info, offset), > + BIT(p), 0); > + > + return 0; > +} > + > static const struct pinmux_ops ocelot_pmx_ops = { > .get_functions_count = ocelot_get_functions_count, > .get_function_name = ocelot_get_function_name, > @@ -783,6 +1180,15 @@ static const struct pinmux_ops ocelot_pmx_ops = { > .gpio_request_enable = ocelot_gpio_request_enable, > }; > > +static const struct pinmux_ops lan966x_pmx_ops = { > + .get_functions_count = ocelot_get_functions_count, > + .get_function_name = ocelot_get_function_name, > + .get_function_groups = ocelot_get_function_groups, > + .set_mux = lan966x_pinmux_set_mux, > + .gpio_set_direction = ocelot_gpio_set_direction, > + .gpio_request_enable = lan966x_gpio_request_enable, > +}; > + > static int ocelot_pctl_get_groups_count(struct pinctrl_dev *pctldev) > { > struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > @@ -1074,6 +1480,14 @@ static struct pinctrl_desc sparx5_desc = { > .npins = ARRAY_SIZE(sparx5_pins), > .pctlops = &ocelot_pctl_ops, > .pmxops = &ocelot_pmx_ops, > +}; > + > +static struct pinctrl_desc lan966x_desc = { > + .name = "lan966x-pinctrl", > + .pins = lan966x_pins, > + .npins = ARRAY_SIZE(lan966x_pins), > + .pctlops = &ocelot_pctl_ops, > + .pmxops = &lan966x_pmx_ops, > .confops = &ocelot_confops, > .owner = THIS_MODULE, > }; > @@ -1114,6 +1528,7 @@ static int ocelot_create_group_func_map(struct device *dev, > return 0; > } > > + Useless blank line > static int ocelot_pinctrl_register(struct platform_device *pdev, > struct ocelot_pinctrl *info) > { > @@ -1337,6 +1752,7 @@ static const struct of_device_id ocelot_pinctrl_of_match[] = { > { .compatible = "mscc,ocelot-pinctrl", .data = &ocelot_desc }, > { .compatible = "mscc,jaguar2-pinctrl", .data = &jaguar2_desc }, > { .compatible = "microchip,sparx5-pinctrl", .data = &sparx5_desc }, > + { .compatible = "microchip,lan966x-pinctrl", .data = &lan966x_desc }, > {}, > }; > > -- > 2.17.1 >
> -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@bootlin.com] > Sent: Thursday, November 18, 2021 4:30 AM > To: Kavyasree Kotagiri - I30978 <Kavyasree.Kotagiri@microchip.com> > Cc: robh+dt@kernel.org; linus.walleij@linaro.org; linux- > gpio@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 2/2] pinctrl: ocelot: Extend support for lan966x > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hello Kavya, > > On 29/10/2021 14:57:03+0530, Kavyasree Kotagiri wrote: > > + LAN966X_PIN(76), > > + LAN966X_PIN(77), > > +}; > > + > > + > > One blank line should be removed > This is older patch series. Blank lines are already fixed in v3 patch series. > > static int ocelot_get_functions_count(struct pinctrl_dev *pctldev) > > { > > return ARRAY_SIZE(ocelot_function_names); > > @@ -709,6 +1056,9 @@ static int ocelot_pin_function_idx(struct > ocelot_pinctrl *info, > > for (i = 0; i < OCELOT_FUNC_PER_PIN; i++) { > > if (function == p->functions[i]) > > return i; > > + > > + if (function == p->a_functions[i]) > > + return i + OCELOT_FUNC_PER_PIN; > > } > > > > return -1; > > @@ -744,6 +1094,36 @@ static int ocelot_pinmux_set_mux(struct > pinctrl_dev *pctldev, > > return 0; > > } > > > > +static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev, > > + unsigned int selector, unsigned int group) > > +{ > > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > > + struct ocelot_pin_caps *pin = info->desc->pins[group].drv_data; > > + unsigned int p = pin->pin % 32; > > + int f; > > + > > + f = ocelot_pin_function_idx(info, group, selector); > > + if (f < 0) > > + return -EINVAL; > > + > > + /* > > + * f is encoded on three bits. > > + * bit 0 of f goes in BIT(pin) of ALT[0], bit 1 of f goes in BIT(pin) of > > + * ALT[1], bit 2 of f goes in BIT(pin) of ALT[2] > > + * This is racy because both registers can't be updated at the same time > > That's three registers, not two so I guess this sentence should be > reworked. > I agree. I will change it. > > + * but it doesn't matter much for now. > > + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets > > + */ > > + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin), > > + BIT(p), f << p); > > + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin), > > + BIT(p), (f >> 1) << p); > > + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin), > > + BIT(p), (f >> 2) << p); > > + > > I would have thought the hardware would be fixed because you now have 78 > pins and this probably will get worse over time. This is really a poor > choice of interface as now you will get two transient states instead of > one. > Sorry, I couldn't get you. please elaborate what you meant by this comment? > > + return 0; > > +} > > + > > #define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32))) > > > > static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev, > > @@ -774,6 +1154,23 @@ static int ocelot_gpio_request_enable(struct > pinctrl_dev *pctldev, > > return 0; > > } > > > > +static int lan966x_gpio_request_enable(struct pinctrl_dev *pctldev, > > + struct pinctrl_gpio_range *range, > > + unsigned int offset) > > +{ > > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > > + unsigned int p = offset % 32; > > + > > + regmap_update_bits(info->map, REG_ALT(0, info, offset), > > + BIT(p), 0); > > + regmap_update_bits(info->map, REG_ALT(1, info, offset), > > + BIT(p), 0); > > + regmap_update_bits(info->map, REG_ALT(2, info, offset), > > + BIT(p), 0); > > + > > + return 0; > > +} > > + > > static const struct pinmux_ops ocelot_pmx_ops = { > > .get_functions_count = ocelot_get_functions_count, > > .get_function_name = ocelot_get_function_name, > > @@ -783,6 +1180,15 @@ static const struct pinmux_ops ocelot_pmx_ops = > { > > .gpio_request_enable = ocelot_gpio_request_enable, > > }; > > > > +static const struct pinmux_ops lan966x_pmx_ops = { > > + .get_functions_count = ocelot_get_functions_count, > > + .get_function_name = ocelot_get_function_name, > > + .get_function_groups = ocelot_get_function_groups, > > + .set_mux = lan966x_pinmux_set_mux, > > + .gpio_set_direction = ocelot_gpio_set_direction, > > + .gpio_request_enable = lan966x_gpio_request_enable, > > +}; > > + > > static int ocelot_pctl_get_groups_count(struct pinctrl_dev *pctldev) > > { > > struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > > @@ -1074,6 +1480,14 @@ static struct pinctrl_desc sparx5_desc = { > > .npins = ARRAY_SIZE(sparx5_pins), > > .pctlops = &ocelot_pctl_ops, > > .pmxops = &ocelot_pmx_ops, > > +}; > > + > > +static struct pinctrl_desc lan966x_desc = { > > + .name = "lan966x-pinctrl", > > + .pins = lan966x_pins, > > + .npins = ARRAY_SIZE(lan966x_pins), > > + .pctlops = &ocelot_pctl_ops, > > + .pmxops = &lan966x_pmx_ops, > > .confops = &ocelot_confops, > > .owner = THIS_MODULE, > > }; > > @@ -1114,6 +1528,7 @@ static int ocelot_create_group_func_map(struct > device *dev, > > return 0; > > } > > > > + > > Useless blank line > Already fixed in v3 patch series. > > static int ocelot_pinctrl_register(struct platform_device *pdev, > > struct ocelot_pinctrl *info) > > { > > @@ -1337,6 +1752,7 @@ static const struct of_device_id > ocelot_pinctrl_of_match[] = { > > { .compatible = "mscc,ocelot-pinctrl", .data = &ocelot_desc }, > > { .compatible = "mscc,jaguar2-pinctrl", .data = &jaguar2_desc }, > > { .compatible = "microchip,sparx5-pinctrl", .data = &sparx5_desc }, > > + { .compatible = "microchip,lan966x-pinctrl", .data = &lan966x_desc }, > > {}, > > }; > > > > -- > > 2.17.1 > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 18/11/2021 09:24:56+0000, Kavyasree.Kotagiri@microchip.com wrote: > > > + * but it doesn't matter much for now. > > > + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets > > > + */ > > > + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin), > > > + BIT(p), f << p); > > > + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin), > > > + BIT(p), (f >> 1) << p); > > > + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin), > > > + BIT(p), (f >> 2) << p); > > > + > > > > I would have thought the hardware would be fixed because you now have 78 > > pins and this probably will get worse over time. This is really a poor > > choice of interface as now you will get two transient states instead of > > one. > > > Sorry, I couldn't get you. please elaborate what you meant by this comment? > Not mush you can do on your side, this was just a rant. I raised the issue to Allan, hoping that this will get fixed in the next SoCs ;)