| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <23899c54-14ad-4724-9336-2df6fb485fd6@amlogic.com>
Date: Wed, 18 Dec 2024 17:37:11 +0800
From: Xianwei Zhao <xianwei.zhao@...ogic.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs
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@...nel.org> wrote:
>
>> From: Xianwei Zhao <xianwei.zhao@...ogic.com>
>>
>> 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@...ogic.com>
>
> 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
Powered by blists - more mailing lists