netdev - Re: [PATCH v9 2/7] gpio: Add Nuvoton NCT6694 GPIO support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOoeyxV3WajFf+YCAP6y5pzEmQdcNHrU2yFqU9LGgO1e8Faq5g@mail.gmail.com>
Date: Thu, 10 Apr 2025 10:45:29 +0800
From: Ming Yu <a0282524688@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: lee@...nel.org, linus.walleij@...aro.org, andi.shyti@...nel.org, 
	mkl@...gutronix.de, mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com, 
	alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-rtc@...r.kernel.org, linux-usb@...r.kernel.org, 
	Ming Yu <tmyu0@...oton.com>, Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v9 2/7] gpio: Add Nuvoton NCT6694 GPIO support

Dear Bartosz,

Thank you for reviewing.

Bartosz Golaszewski <brgl@...ev.pl> 於 2025年4月9日 週三 下午7:39寫道：
>
...
> > +static int nct6694_gpio_probe(struct platform_device *pdev)
> > +{
> > +       const struct mfd_cell *cell = mfd_get_cell(pdev);
> > +       struct device *dev = &pdev->dev;
> > +       struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > +       struct nct6694_gpio_data *data;
> > +       struct gpio_irq_chip *girq;
> > +       int ret, irq, i;
> > +       char **names;
> > +
> > +       irq = irq_create_mapping(nct6694->domain,
> > +                                NCT6694_IRQ_GPIO0 + cell->id);
> > +       if (!irq)
> > +               return -EINVAL;
> > +
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data) {
> > +               ret = -ENOMEM;
> > +               goto dispose_irq;
> > +       }
> > +
> > +       names = devm_kcalloc(dev, NCT6694_NR_GPIO, sizeof(char *),
> > +                            GFP_KERNEL);
> > +       if (!names) {
> > +               ret = -ENOMEM;
> > +               goto dispose_irq;
> > +       }
> > +
> > +       for (i = 0; i < NCT6694_NR_GPIO; i++) {
> > +               names[i] = devm_kasprintf(dev, GFP_KERNEL, "GPIO%X%d",
> > +                                         cell->id, i);
> > +               if (!names[i]) {
> > +                       ret = -ENOMEM;
> > +                       goto dispose_irq;
> > +               }
> > +       }
> > +
> > +       data->irq = irq;
> > +       data->nct6694 = nct6694;
> > +       data->group = cell->id;
> > +
> > +       data->gpio.names                = (const char * const*)names;
> > +       data->gpio.label                = pdev->name;
> > +       data->gpio.direction_input      = nct6694_direction_input;
> > +       data->gpio.get                  = nct6694_get_value;
> > +       data->gpio.direction_output     = nct6694_direction_output;
> > +       data->gpio.set                  = nct6694_set_value;
>
> Please use the set_rv variant, regular set is deprecated now.
>

Understood. Fix it in v10.

> > +       data->gpio.get_direction        = nct6694_get_direction;
> > +       data->gpio.set_config           = nct6694_set_config;
> > +       data->gpio.init_valid_mask      = nct6694_init_valid_mask;
> > +       data->gpio.base                 = -1;
> > +       data->gpio.can_sleep            = false;
> > +       data->gpio.owner                = THIS_MODULE;
> > +       data->gpio.ngpio                = NCT6694_NR_GPIO;
> > +
> > +       platform_set_drvdata(pdev, data);
> > +
> > +       ret = devm_mutex_init(dev, &data->lock);
> > +       if (ret)
> > +               goto dispose_irq;
> > +
> > +       ret = devm_mutex_init(dev, &data->irq_lock);
> > +       if (ret)
> > +               goto dispose_irq;
> > +
> > +       ret = nct6694_get_irq_trig(data);
> > +       if (ret) {
> > +               dev_err_probe(dev, ret, "Failed to get irq trigger type\n");
> > +               goto dispose_irq;
> > +       }
> > +
> > +       girq = &data->gpio.irq;
> > +       gpio_irq_chip_set_chip(girq, &nct6694_irq_chip);
> > +       girq->parent_handler = NULL;
> > +       girq->num_parents = 0;
> > +       girq->parents = NULL;
> > +       girq->default_type = IRQ_TYPE_NONE;
> > +       girq->handler = handle_level_irq;
> > +       girq->threaded = true;
> > +
> > +       ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_irq_handler,
> > +                                       IRQF_ONESHOT | IRQF_SHARED,
> > +                                       "gpio-nct6694", data);
> > +       if (ret) {
> > +               dev_err_probe(dev, ret, "Failed to request irq\n");
> > +               goto dispose_irq;
> > +       }
> > +
> > +       ret = devm_gpiochip_add_data(dev, &data->gpio, data);
> > +       if (ret)
> > +               goto dispose_irq;
> > +
> > +       return 0;
> > +
> > +dispose_irq:
> > +       irq_dispose_mapping(irq);
> > +       return ret;
> > +}
> > +
> > +static void nct6694_gpio_remove(struct platform_device *pdev)
> > +{
> > +       struct nct6694_gpio_data *data = platform_get_drvdata(pdev);
> > +
> > +       devm_free_irq(&pdev->dev, data->irq, data);
>
> That's definitely not right. If you need to use the devm_free variant
> in remove(), then you're doing something wrong. Most likely you can
> rely on the devres release path here...
>
> > +       irq_dispose_mapping(data->irq);
>
> ... and schedule this as a custom devm action.
>

Okay, I will add the devm_add_action_or_reset() to call
irq_dispose_mapping() in the next patch.


Best regards,
Ming