> -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@xxxxxxxxxxx] > Sent: Thursday, November 18, 2021 4:30 AM > To: Kavyasree Kotagiri - I30978 <Kavyasree.Kotagiri@xxxxxxxxxxxxx> > Cc: robh+dt@xxxxxxxxxx; linus.walleij@xxxxxxxxxx; linux- > gpio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx> > 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
