| 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: <e654f86d-3ea3-4be5-8f6b-dfe8f96c24a2@gmail.com>
Date: Mon, 15 Apr 2024 09:18:27 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linus.walleij@...aro.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
p.zabel@...gutronix.de, j.neuschaefer@....net,
linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH v7 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO
driver
Dear Andy,
Sorry for the late response to your mail.
Due to email inbox policy issues, I just found this email.
Regarding your comments, I will make all the modifications in the next
version.
Thank you for your help.
Best Regards,
Jacky Huang
On 2024/4/10 下午 04:54, Andy Shevchenko wrote:
> CAUTION - External Email: Do not click links or open attachments unless you acknowledge the sender and content.
>
>
> Tue, Apr 09, 2024 at 09:56:37AM +0000, Jacky Huang kirjoitti:
>> From: Jacky Huang <ychuang3@...oton.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
>
>
Powered by blists - more mailing lists