[PATCH v7 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver
Andy Shevchenko
andy.shevchenko at gmail.com
Wed Apr 10 01:54:09 PDT 2024
Tue, Apr 09, 2024 at 09:56:37AM +0000, Jacky Huang kirjoitti:
> From: Jacky Huang <ychuang3 at nuvoton.com>
>
> Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and
> add support for ma35d1 pinctrl.
...
> + * Copyright (C) 2023 Nuvoton Technology Corp.
Almos mid of 2024...
...
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
Do you really need all of these pf*.h?
Don't you miss other headers to be included (spoiler: a lot!)
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
Move this group...
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
...to be here to show the relation to the pin control subsystem.
> +#include "../core.h"
> +#include "../pinconf.h"
> +#include "pinctrl-ma35.h"
...
> +#define MA35_GP_MODE_MASK_WIDTH 2
> +
> +#define MA35_GP_SLEWCTL_MASK_WIDTH 2
I looked at the code how you use these... Oh, please switch to FIELD_GET() /
FIELD_PREP() (don't forget to include bitfield.h)
...
> +struct ma35_pin_func {
> + const char *name;
> + const char **groups;
> + u32 ngroups;
> +};
NIH struct pinfunction.
You may still have a wrapper (as struct pingroup below most likely will be
embedded in your custom data type), see the pinctrl-intel.h examples.
...
> +struct ma35_pin_group {
> + const char *name;
> + unsigned int npins;
> + unsigned int *pins;
NIH struct pingroup.
> + struct ma35_pin_setting *settings;
> +};
...
> +struct ma35_pin_bank {
> + void __iomem *reg_base;
> + struct clk *clk;
> + int irq;
> + u8 nr_pins;
> + const char *name;
> + u8 bank_num;
Interleaved fields with such a different type may lead to waste of memory. Run
`pahole` and update the ordering in this struct accordingly.
> + bool valid;
> + struct device_node *np;
Can you keep it fwnode-based?
> + struct gpio_chip chip;
> + u32 irqtype;
> + u32 irqinten;
> + struct regmap *regmap;
> + struct device *dev;
> + spinlock_t lock;
No use in RT_PREEMPT?
> +};
...
> + new_map = devm_kcalloc(pctldev->dev, map_num, sizeof(*new_map), GFP_KERNEL);
> + if (!new_map)
> + return -ENOMEM;
> +
> + *map = new_map;
> + *num_maps = map_num;
> + /* create mux map */
> + parent = of_get_parent(np);
> + if (!parent) {
> + devm_kfree(pctldev->dev, new_map);
Huh?! Are you sure you know the scope of usage of devm_*()?
> + return -EINVAL;
> + }
> +
...
> + regval &= ~GENMASK(setting->shift + MA35_MFP_BITS_PER_PORT - 1,
> + setting->shift);
This will generate an awful code. Use respective FIELD_*() macros.
...
> + regval &= ~GENMASK(gpio * MA35_GP_MODE_MASK_WIDTH - 1,
> + gpio * MA35_GP_MODE_MASK_WIDTH);
> + regval |= mode << gpio * MA35_GP_MODE_MASK_WIDTH;
Ditto.
...
> + regval &= GENMASK(gpio * MA35_GP_MODE_MASK_WIDTH - 1,
> + gpio * MA35_GP_MODE_MASK_WIDTH);
> +
> + return regval >> gpio * MA35_GP_MODE_MASK_WIDTH;
Ditto.
...
> +static int ma35_gpio_core_direction_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct ma35_pin_bank *bank = gpiochip_get_data(gc);
> + void __iomem *reg_mode = bank->reg_base + MA35_GP_REG_MODE;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&bank->lock, flags);
Use cleanup.h, i.e. guard() in this case, from day 1.
Similar approach for all other places.
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
...
> + regval = readl(reg_dout);
> + if (val)
> + writel(regval | BIT(gpio), reg_dout);
> + else
> + writel(regval & ~BIT(gpio), reg_dout);
Can you split regval update and make only a single writel() call? It's slightly
better pattern
read()
if (foo)
v =
else
v =
write()
> + ma35_gpio_set_mode(reg_mode, gpio, MA35_GP_MODE_OUTPUT);
...
> + regval &= ~GENMASK(bit_offs + MA35_MFP_BITS_PER_PORT - 1, bit_offs);
FIELD_GET()
...
> + writel(1<<num, reg_intsrc);
Oh, something happened here, besides indentation.
Have you meant BIT() ?
...
> + unsigned int num = (d->hwirq);
Read Documentation on how to access hwirq field and what type of the value
should be. There are plently of the examples already in the kernel. Just
check the recent (more or less) ones.
...
> + /*
> + * The MA35_GP_REG_INTEN bits 0 ~ 15 control low-level or falling edge trigger,
> + * while bits 16 ~ 31 control high-level or rising edge trigger.
> + * We disable both type of interrupt.
> + */
> + regval &= ~(BIT(num + 16) | BIT(num));
You have this idiom more than once in the code, make the definition and use it.
Move comment closer to that definition.
...
> + irq_set_handler_locked(d, handle_edge_irq);
> + bank->irqtype &= ~(0x1 << num);
> + bank->irqinten |= (0x1 << num);
> + bank->irqinten &= ~(0x1 << (num + 16));
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + irq_set_handler_locked(d, handle_level_irq);
> + bank->irqtype |= (0x1 << num);
> + bank->irqinten &= ~(0x1 << num);
> + bank->irqinten |= (0x1 << (num + 16));
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + irq_set_handler_locked(d, handle_level_irq);
> + bank->irqtype |= (0x1 << num);
> + bank->irqinten &= ~(0x1 << (num + 16));
> + bank->irqinten |= (0x1 << num);
> + break;
BIT() in all cases.
...
> + irq_set_handler_locked(d, handle_bad_irq);
Just assign it in the probe.
...
> +static struct irq_chip ma35_gpio_irqchip = {
> + .name = "MA35-GPIO-IRQ",
> + .irq_disable = ma35_irq_gpio_mask,
> + .irq_enable = ma35_irq_gpio_unmask,
> + .irq_ack = ma35_irq_gpio_ack,
> + .irq_mask = ma35_irq_gpio_mask,
> + .irq_unmask = ma35_irq_gpio_unmask,
> + .irq_set_type = ma35_irq_irqtype,
> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE | IRQCHIP_IMMUTABLE,
It doesn't look like IMMUTABLE. Again, check other examples, there are a lot.
...
> +static void ma35_irq_demux_intgroup(struct irq_desc *desc)
> +{
> + struct ma35_pin_bank *bank = gpiochip_get_data(irq_desc_get_handler_data(desc));
> + struct irq_domain *irqdomain = bank->chip.irq.domain;
> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> + unsigned long isr;
> + int offset;
> +
> + chained_irq_enter(irqchip, desc);
> + isr = readl(bank->reg_base + MA35_GP_REG_INTSRC);
> + if (isr)
Unneeded duplicate check.
> + for_each_set_bit(offset, &isr, bank->nr_pins)
> + generic_handle_irq(irq_find_mapping(irqdomain, offset));
> +
> + chained_irq_exit(irqchip, desc);
> +}
...
> + for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
Why pre-increments?
> + if (!bank->valid) {
> + dev_warn(&pdev->dev, "bank %s is not valid\n",
> + bank->np->name);
Do not use fwnode / of_node name like this, use proper specifiers: %pfw / %pOF.
> + continue;
> + }
> + bank->irqtype = 0;
> + bank->irqinten = 0;
> + bank->chip.label = bank->name;
> + bank->chip.of_gpio_n_cells = 2;
> + bank->chip.parent = &pdev->dev;
> + bank->chip.request = ma35_gpio_core_to_request;
> + bank->chip.direction_input = ma35_gpio_core_direction_in;
> + bank->chip.direction_output = ma35_gpio_core_direction_out;
> + bank->chip.get = ma35_gpio_core_get;
> + bank->chip.set = ma35_gpio_core_set;
> + bank->chip.base = -1;
> + bank->chip.ngpio = bank->nr_pins;
> + bank->chip.can_sleep = false;
> + spin_lock_init(&bank->lock);
> +
> + if (bank->irq > 0) {
> + struct gpio_irq_chip *girq;
> +
> + girq = &bank->chip.irq;
> + gpio_irq_chip_set_chip(girq, &ma35_gpio_irqchip);
> + girq->parent_handler = ma35_irq_demux_intgroup;
> + girq->num_parents = 1;
> +
> + girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents),
Use num_parents instead of 1.
> + GFP_KERNEL);
> + if (!girq->parents) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + girq->parents[0] = bank->irq;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> + }
> +
> + ret = gpiochip_add_data(&bank->chip, bank);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register gpio_chip %s, error code: %d\n",
> + bank->chip.label, ret);
> + goto fail;
> + }
> + }
...
> +fail:
> + for (--i, --bank; i >= 0; --i, --bank) {
while (i--) {
bank--;
...
}
much better to read.
> + if (!bank->valid)
> + continue;
> + gpiochip_remove(&bank->chip);
> + }
> + return ret;
> +}
...
> +static int ma35_get_bank_data(struct ma35_pin_bank *bank, struct ma35_pinctrl *npctl)
> +{
> + struct resource res;
> +
> + if (of_address_to_resource(bank->np, 0, &res)) {
> + dev_err(npctl->dev, "cannot find IO resource for bank\n");
> + return -ENOENT;
> + }
> +
> + bank->reg_base = devm_ioremap_resource(npctl->dev, &res);
> + if (IS_ERR(bank->reg_base)) {
> + dev_err(npctl->dev, "cannot ioremap resource for bank\n");
> + return PTR_ERR(bank->reg_base);
> + }
Use fwnode_iomap()
> + bank->irq = irq_of_parse_and_map(bank->np, 0);
Use fwnode_get_irq()
> + bank->nr_pins = MA35_GPIO_PORT_MAX;
> +
> + bank->clk = of_clk_get(bank->np, 0);
Can't you use simple clk_get()? Why?
> + if (IS_ERR(bank->clk))
> + return PTR_ERR(bank->clk);
> +
> + return clk_prepare_enable(bank->clk);
> +}
...
> + for_each_gpiochip_node(&pdev->dev, child) {
> + struct device_node *np = to_of_node(child);
No need, just keep everythin fwnode based.
> + bank = &ctrl->pin_banks[id];
> + bank->np = np;
> + bank->regmap = pctl->regmap;
> + bank->dev = &pdev->dev;
> + if (!ma35_get_bank_data(bank, pctl))
> + bank->valid = true;
> + id++;
> + }
...
> + regval &= ~GENMASK(port * MA35_GP_PUSEL_MASK_WIDTH - 1,
> + port * MA35_GP_PUSEL_MASK_WIDTH);
FIELD_*()
...
> + regval &= GENMASK(port * MA35_GP_PUSEL_MASK_WIDTH - 1,
> + port * MA35_GP_PUSEL_MASK_WIDTH);
> + regval >>= port * MA35_GP_PUSEL_MASK_WIDTH;
Ditto.
...
> + if (regval & BIT(port))
> + return MVOLT_3300;
> + else
> + return MVOLT_1800;
Can be written as
return (regval & BIT()) ? _3300 : _1800;
...
> + if (port < MA35_GP_DSH_BASE_PORT) {
> + regval = readl(base + MA35_GP_REG_DSL);
> + ds_val = (regval & GENMASK(port * 4 + 2, port * 4)) >> port * 4;
This is too complicated way of saying
ds_val = (regval >> port * 4) & GENMASK(2, 0);
> + } else {
> + port -= MA35_GP_DSH_BASE_PORT;
> + regval = readl(base + MA35_GP_REG_DSH);
> + ds_val = (regval & GENMASK(port * 4 + 2, port * 4)) >> port * 4;
Ditto.
> + }
...
> + if (ma35_pinconf_get_power_source(npctl, pin) == MVOLT_1800) {
> + for (i = 0; i < ARRAY_SIZE(ds_1800mv_tbl); i++) {
> + if (ds_1800mv_tbl[i] == strength)
> + ds_val = i;
And still continue?!
> + }
> + } else {
> + for (i = 0; i < ARRAY_SIZE(ds_3300mv_tbl); i++) {
> + if (ds_3300mv_tbl[i] == strength)
> + ds_val = i;
Ditto.
> + }
> + }
Wondering if the linear_ranges or other existing tables approaches can be used here.
...
> + if (port < MA35_GP_DSH_BASE_PORT) {
> + regval = readl(base + MA35_GP_REG_DSL);
> + regval &= ~GENMASK(port * 4 + 2, port * 4);
> + regval |= ds_val << port * 4;
As per above
~(GENMASK(2, 0) << (port * 4))
> + writel(regval, base + MA35_GP_REG_DSL);
> + } else {
> + port -= MA35_GP_DSH_BASE_PORT;
> + regval = readl(base + MA35_GP_REG_DSH);
> + regval &= ~GENMASK(port * 4 + 2, port * 4);
Ditto.
> + regval |= ds_val << port * 4;
> + writel(regval, base + MA35_GP_REG_DSH);
> + }
...
> + regval &= ~GENMASK(port * MA35_GP_SLEWCTL_MASK_WIDTH - 1,
> + port * MA35_GP_SLEWCTL_MASK_WIDTH);
> + regval |= rate << port * MA35_GP_SLEWCTL_MASK_WIDTH;
FIELD_*()
...
> +static int ma35_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, unsigned long *config)
> +{
> + struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + u32 arg;
> + int ret;
> +
> + switch (param) {
> + case PIN_CONFIG_BIAS_DISABLE:
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + case PIN_CONFIG_BIAS_PULL_UP:
> + if (ma35_pinconf_get_pull(npctl, pin) == param)
> + arg = 1;
> + else
> + return -EINVAL;
> + break;
Check for the error case first and get rid of redundant 'else'.
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + ret = ma35_pinconf_get_drive_strength(npctl, pin, &arg);
> + if (ret)
> + return ret;
> + break;
> +
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + arg = ma35_pinconf_get_schmitt_enable(npctl, pin);
> + break;
> +
> + case PIN_CONFIG_SLEW_RATE:
> + arg = ma35_pinconf_get_slew_rate(npctl, pin);
> + break;
> +
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + arg = ma35_pinconf_get_output(npctl, pin);
> + break;
> +
> + case PIN_CONFIG_POWER_SOURCE:
> + arg = ma35_pinconf_get_power_source(npctl, pin);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
...
> +static int ma35_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *configs, unsigned int num_configs)
> +{
> + struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param param;
> + unsigned int arg = 0;
> + int i, ret = 0;
> +
> + for (i = 0; i < num_configs; i++) {
> + param = pinconf_to_config_param(configs[i]);
> + arg = pinconf_to_config_argument(configs[i]);
> +
> + switch (param) {
> + case PIN_CONFIG_BIAS_DISABLE:
> + case PIN_CONFIG_BIAS_PULL_UP:
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + ret = ma35_pinconf_set_pull(npctl, pin, param);
> + break;
> +
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + ret = ma35_pinconf_set_drive_strength(npctl, pin, arg);
> + break;
> +
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + ret = ma35_pinconf_set_schmitt(npctl, pin, 1);
> + break;
> +
> + case PIN_CONFIG_INPUT_SCHMITT:
> + ret = ma35_pinconf_set_schmitt(npctl, pin, arg);
> + break;
> +
> + case PIN_CONFIG_SLEW_RATE:
> + ret = ma35_pinconf_set_slew_rate(npctl, pin, arg);
> + break;
> +
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + ret = ma35_pinconf_set_output(npctl, pin, arg);
> + break;
> +
> + case PIN_CONFIG_POWER_SOURCE:
> + ret = ma35_pinconf_set_power_source(npctl, pin, arg);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
No returning an error if we got ret != 0?!
> + }
> + return ret;
> +}
...
> +static int ma35_pinctrl_parse_groups(struct device_node *np, struct ma35_pin_group *grp,
> + struct ma35_pinctrl *npctl, u32 index)
> +{
> + unsigned long *configs;
> + unsigned int nconfigs;
> + struct ma35_pin_setting *pin;
> + const __be32 *list;
Huh?!
> + int i, j, size, ret;
> +
> + dev_dbg(npctl->dev, "group(%d): %s\n", index, np->name);
> +
> + grp->name = np->name;
> +
> + ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &nconfigs);
> + if (ret)
> + return ret;
> +
> + /*
> + * the binding format is nuvoton,pins = <bank pin-mfp pin-function>,
> + * do sanity check and calculate pins number
> + */
/*
* Respect English grammar and puntuation
* in all multi-line comments. See the difference
* how it's done above and in this example.
*/
> + list = of_get_property(np, "nuvoton,pins", &size);
Oh, no.
Use respective of_property_ calls.
> + size /= sizeof(*list);
> + if (!size || size % 3) {
> + dev_err(npctl->dev, "wrong setting!\n");
> + return -EINVAL;
> + }
> + grp->npins = size / 3;
> +
> + grp->pins = devm_kcalloc(npctl->dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
> + if (!grp->pins)
> + return -ENOMEM;
> +
> + grp->settings = devm_kcalloc(npctl->dev, grp->npins, sizeof(*grp->settings), GFP_KERNEL);
> + if (!grp->settings)
> + return -ENOMEM;
> +
> + pin = grp->settings;
> +
> + for (i = 0, j = 0; i < size; i += 3, j++) {
> + pin->offset = be32_to_cpu(*list++) * MA35_MFP_REG_SZ_PER_BANK + MA35_MFP_REG_BASE;
> + pin->shift = (be32_to_cpu(*list++) * MA35_MFP_BITS_PER_PORT) % 32;
> + pin->muxval = be32_to_cpu(*list++);
> + pin->configs = configs;
> + pin->nconfigs = nconfigs;
> + grp->pins[j] = npctl->info->get_pin_num(pin->offset, pin->shift);
> + pin++;
> + }
> + return 0;
> +}
...
> + for_each_child_of_node(np, child) {
> + if (of_property_read_bool(child, "gpio-controller"))
> + continue;
There is a macro already present, for_each_gpiochip_node().
> + npctl->nfunctions++;
> + npctl->ngroups += of_get_child_count(child);
> + }
...
> + for_each_child_of_node(np, child) {
> + if (of_property_read_bool(child, "gpio-controller"))
> + continue;
Ditto.
> + ret = ma35_pinctrl_parse_functions(child, npctl, idx++);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to parse function\n");
> + of_node_put(child);
> + return ret;
> + }
> + }
...
> + if (!info || !info->pins || !info->npins) {
> + dev_err(&pdev->dev, "wrong pinctrl info\n");
> + return -EINVAL;
return dev_err_probe(...);
> + }
...
> + ret = devm_pinctrl_register_and_init(&pdev->dev, ma35_pinctrl_desc,
> + npctl, &npctl->pctl);
Haveing
struct device *dev = &pdev->dev;
makes in particular this one better
ret = devm_pinctrl_register_and_init(dev, ma35_pinctrl_desc, npctl, &npctl->pctl);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "fail to register MA35 pinctrl\n");
...
> +int __maybe_unused ma35_pinctrl_suspend(struct device *dev)
No __maybe_unused.
Just use respective PM macros (pm_ptr(), DEFINE_,*PM_OPS, etc).
> +{
> + struct ma35_pinctrl *npctl = dev_get_drvdata(dev);
> +
> + return pinctrl_force_sleep(npctl->pctl);
> +}
> +
> +int __maybe_unused ma35_pinctrl_resume(struct device *dev)
Ditto.
> +{
> + struct ma35_pinctrl *npctl = dev_get_drvdata(dev);
> +
> + return pinctrl_force_default(npctl->pctl);
> +}
...
> +struct ma35_mux_desc {
> + const char *name;
> + u32 muxval;
> +};
> +
> +struct ma35_pin_data {
> + u32 offset;
> + u32 shift;
> + struct ma35_mux_desc *muxes;
> +};
Don't pin control subsystem has already some data types suitable in this cases?
...
> +#define MA35_PIN(num, n, o, s, ...) { \
> + .number = num, \
> + .name = #n, \
Don't we have macros for this?
> + .drv_data = &(struct ma35_pin_data) { \
> + .offset = o, \
> + .shift = s, \
> + .muxes = (struct ma35_mux_desc[]) { \
> + __VA_ARGS__, { } }, \
> + }, \
> +}
...
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
Missing headers and stray ones. Please, get this into order by following IWYU
principle.
> +#include <linux/of.h>
> +#include <linux/of_device.h>
How are these being used?
> +#include <linux/pinctrl/pinctrl.h>
...
> +static const struct dev_pm_ops ma35d1_pinctrl_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(ma35_pinctrl_suspend, ma35_pinctrl_resume)
> +};
New PM macros, please.
...
> +static const struct of_device_id ma35d1_pinctrl_of_match[] = {
> + { .compatible = "nuvoton,ma35d1-pinctrl", },
> + { },
No comma in the terminator entry.
> +};
...
> +static struct platform_driver ma35d1_pinctrl_driver = {
> + .probe = ma35d1_pinctrl_probe,
> + .driver = {
> + .name = "ma35d1-pinctrl",
> + .pm = &ma35d1_pinctrl_pm_ops,
New PM macro here.
> + .of_match_table = ma35d1_pinctrl_of_match,
> + },
> +};
--
With Best Regards,
Andy Shevchenko
More information about the linux-arm-kernel
mailing list