| 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: <168829c7-fd56-4fd2-80b0-07c768779559@linaro.org>
Date: Tue, 17 Dec 2024 15:53:10 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Linus Walleij <linus.walleij@...aro.org>, xianwei.zhao@...ogic.com
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.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,
On 17/12/2024 15:49, Linus Walleij wrote:
> 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?
Sunxi is Allwinner SoCs.
Neil
>
> - Also fix MAINTAINERS accordingly.
>
> - 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?
>
>> +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?
>
>> +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.
>
>> +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?
>
>> +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