Hi Linus,
Thanks for your reply.
On 2024/12/17 22:49, Linus Walleij wrote:
[ EXTERNAL EMAIL ]
Hi Xianwei,
thanks for your patch!
On Wed, Dec 11, 2024 at 7:48 AM Xianwei Zhao via B4 Relay
<devnull+xianwei.zhao.amlogic.com@xxxxxxxxxx> wrote:
From: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
Add a new pinctrl driver for Amlogic SoCs. All future Amlogic
SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support
new Amlogic SoCs, only need to add the corresponding dts file.
Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
First: are we sure these new SoCs have nothing in common
with sunxi? Because then the sunxi code should be reused.
In any way I recommend:
- Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic
so we keep this sorted by actual vendor, sunxi is apparently
yours (AMlogic:s) isn't it?
It isn't. Sunxi is Allwinner SoCs.
- Also fix MAINTAINERS accordingly.
Sending the official version will be synchronized.
- Add new driver under drivers/pinctrl/amlogic
- Do not change the Kconfig symbols for sunxi and
we should be fine.
>> +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np,
+ struct pinctrl_map **map,
+ unsigned int *num_maps)
+{
+ struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ const struct aml_pctl_group *grp;
+ struct device *dev = info->dev;
+ struct pinctrl_map *new_map;
+ struct device_node *parent;
+ int map_num, i;
+
+ grp = aml_pctl_find_group_by_name(info, np->name);
+ if (!grp) {
+ dev_err(dev, "unable to find group for node %pOFn\n", np);
+ return -EINVAL;
+ }
+
+ if (grp->num_configs)
+ map_num = grp->npins + 1;
+ else
+ map_num = 1;
+ new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL);
+ if (!new_map)
+ return -ENOMEM;
+
+ parent = of_get_parent(np);
+ if (!parent) {
+ devm_kfree(dev, new_map);
+ return -EINVAL;
+ }
+
+ *map = new_map;
+ *num_maps = map_num;
+ new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
+ new_map[0].data.mux.function = parent->name;
+ new_map[0].data.mux.group = np->name;
+ of_node_put(parent);
+
+ if (grp->num_configs) {
+ new_map++;
+ for (i = 0; i < grp->npins; i++) {
+ new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[i].data.configs.group_or_pin =
+ pin_get_name(pctldev, grp->pins[i]);
+ new_map[i].data.configs.configs = grp->configs;
+ new_map[i].data.configs.num_configs = grp->num_configs;
+ }
+ }
+
+ dev_info(dev, "maps: function %s group %s num %d\n",
+ (*map)->data.mux.function, grp->name, map_num);
+
+ return 0;
+}
+
+static void aml_dt_free_map(struct pinctrl_dev *pctldev,
+ struct pinctrl_map *map, unsigned int num_maps)
+{
+}
+
+static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s,
+ unsigned int offset)
+{
+ seq_printf(s, " %s", dev_name(pcdev->dev));
+}
+
+static const struct pinctrl_ops aml_pctrl_ops = {
+ .get_groups_count = aml_get_groups_count,
+ .get_group_name = aml_get_group_name,
+ .get_group_pins = aml_get_group_pins,
+ .dt_node_to_map = aml_dt_node_to_map,
+ .dt_free_map = aml_dt_free_map,
+ .pin_dbg_show = aml_pin_dbg_show,
+};
+
+static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info,
+ unsigned int bank_idx, unsigned int offset)
+{
+ struct aml_gpio_bank *bank;
+ int retval = -EINVAL;
+ int i;
+
+ for (i = 0; i < info->nbanks; i++) {
+ bank = &info->banks[i];
+ if (bank->bank_idx == bank_idx) {
+ if (offset < bank->gpio_chip.ngpio)
+ retval = bank->pin_base + offset;
+ break;
+ }
+ }
+ if (retval == -EINVAL)
+ dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset);
+
+ return retval;
+}
+
+static int aml_pctl_dt_parse_groups(struct device_node *np,
+ struct aml_pctl_group *grp,
+ struct aml_pinctrl *info, int idx)
+{
+ struct device *dev = info->dev;
+ struct aml_pinconf *conf;
+ struct property *of_pins;
+ unsigned int bank_idx;
+ unsigned int offset, npins;
+ int i = 0;
+ int ret;
+
+ of_pins = of_find_property(np, "pinmux", NULL);
+ if (!of_pins) {
+ dev_info(dev, "Missing pinmux property\n");
+ return -ENOENT;
+ }
+
+ npins = of_pins->length / sizeof(u32);
+ grp->npins = npins;
+ grp->name = np->name;
+ grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL);
+ grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL);
+
+ if (!grp->pins || !grp->pin_conf)
+ return -ENOMEM;
+
+ ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs,
+ &grp->num_configs);
But can't you just move this code around? grp->num_configs give the
number of configs, so why do you have to go and look up pinmux
above, can't you just use grp->num_configs instead of of_pins
and npins above?
They are different.
The of_pins(grp->npins) specifies the mux values for pin-mux register
and pin index in pinctrl. It can include multiple pins in groups.
The grp->configs and grp->num_configs specify the configuration
information for all pins of this groups(such as bias-pull-up,
drive-strength-microamp)
uart-d-pins2{
pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>,
<AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>,
<AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>,
<AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>;
bias-pull-up;
drive-strength-microamp = <4000>;
};
+static int aml_pctl_parse_functions(struct device_node *np,
+ struct aml_pinctrl *info, u32 index,
+ int *grp_index)
+{
+ struct device *dev = info->dev;
+ struct aml_pmx_func *func;
+ struct aml_pctl_group *grp;
+ int ret, i;
+
+ func = &info->functions[index];
+ func->name = np->name;
+ func->ngroups = of_get_child_count(np);
+ if (func->ngroups == 0)
+ return dev_err_probe(dev, -EINVAL, "No groups defined\n");
+
+ func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL);
+ if (!func->groups)
+ return -ENOMEM;
+
+ i = 0;
+ for_each_child_of_node_scoped(np, child) {
+ func->groups[i] = child->name;
+ grp = &info->groups[*grp_index];
+ *grp_index += 1;
+ ret = aml_pctl_dt_parse_groups(child, grp, info, i++);
+ if (ret)
+ return ret;
+ }
+ dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups);
+
+ return 0;
+}
+
+static u32 aml_bank_pins(struct device_node *np)
+{
+ u32 value;
+
+ if (of_property_read_u32(np, "npins", &value) < 0)
+ return 0;
+ else
+ return value;
+}
+
+static u32 aml_bank_reg_gpio_offset(struct device_node *np)
+{
+ u32 value;
+
+ if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0)
+ return 0;
+ else
+ return value;
+}
+
+static u32 aml_bank_reg_mux_offset(struct device_node *np)
+{
+ u32 value;
+
+ if (of_property_read_u32(np, "reg-mux-offset", &value) < 0)
+ return 0;
+ else
+ return value;
+}
+
+static u32 aml_bank_bit_mux_offset(struct device_node *np)
+{
+ u32 value;
+
+ if (of_property_read_u32(np, "bit-mux-offset", &value) < 0)
+ return 0;
+ else
+ return value;
+}
+
+static u32 aml_bank_index(struct device_node *np)
+{
+ u32 value;
+
+ if (of_property_read_u32(np, "bank-index", &value) < 0)
+ return 0;
+ else
+ return value;
+}
Do we really need helpers for all of this? Can't you just
open code it, at least if it's just used in one place?
I will delete this function, I will move the logic to where it was called.
+static unsigned int aml_count_pins(struct device_node *np)
+{
+ struct device_node *child;
+ unsigned int pins = 0;
+
+ for_each_child_of_node(np, child) {
+ if (of_property_read_bool(child, "gpio-controller"))
+ pins += aml_bank_pins(child);
+ }
+
+ return pins;
+}
+
+static void aml_pctl_dt_child_count(struct aml_pinctrl *info,
+ struct device_node *np)
+{
+ struct device_node *child;
+
+ for_each_child_of_node(np, child) {
+ if (of_property_read_bool(child, "gpio-controller")) {
+ info->nbanks++;
+ } else {
+ info->nfunctions++;
+ info->ngroups += of_get_child_count(child);
+ }
+ }
+}
This looks like a weird dependency between gpio chips and
pins that I don't quite understand. Some comments and
references to the bindings will be needed so it is clear
what is going on.
A pinctrl device contains two types of nodes. The one named GPIO bank
which includes "gpio-controller" property. The other one named function
which includes one or more pin groups.
The pin group include pinmux property(pin index in pinctrl dev,and mux
vlaue in mux reg) and pin configuration properties.
I will add comment in next verison.
+static struct regmap *aml_map_resource(struct aml_pinctrl *info,
+ struct device_node *node, char *name)
+{
+ struct resource res;
+ void __iomem *base;
+ int i;
+
+ i = of_property_match_string(node, "reg-names", name);
+ if (of_address_to_resource(node, i, &res))
+ return NULL;
+
+ base = devm_ioremap_resource(info->dev, &res);
+ if (IS_ERR(base))
+ return ERR_CAST(base);
This looks like reimplementation of
devm_platform_ioremap_resource_byname(), can't you just
pass your platform device here?
I will fix it.
+static int aml_pctl_probe_dt(struct platform_device *pdev,
+ struct pinctrl_desc *pctl_desc,
+ struct aml_pinctrl *info)
Because there is clearly a platform device involved.
I guess I will have more comments as the series progress, but this
is a good starting point!
Yours,
Linus Walleij
