Dear Linus,

Thank you for your comments,

Linus Walleij [off-list ref] 於 2024年12月20日 週五 下午8:42寫道：

Hi Ming,

thanks for your patch!

Some nits below:

On Tue, Dec 10, 2024 at 11:46 AM Ming Yu [off-list ref] wrote:

quoted

This driver supports GPIO and IRQ functionality for NCT6694 MFD
device based on USB interface.

Signed-off-by: Ming Yu <tmyu0@nuvoton.com>

(...)

quoted

+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nct6694.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>

#include <linux/bits.h>
is missing, include it explicitly.

quoted

+       return !(BIT(offset) & data->xmit_buf);

Here you use the BIT() macro from <linux/bits.h>

Understood. I will add the header in the next patch.

quoted

+static int nct6694_direction_input(struct gpio_chip *gpio, unsigned int offset)
+{
+       struct nct6694_gpio_data *data = gpiochip_get_data(gpio);
+       int ret;
+
+       guard(mutex)(&data->lock);
+
+       ret = nct6694_read_msg(data->nct6694, NCT6694_GPIO_MOD,
+                              NCT6694_GPO_DIR + data->group,
+                              NCT6694_GPIO_LEN, &data->xmit_buf);
+       if (ret < 0)
+               return ret;
+
+       data->xmit_buf &= ~(1 << offset);

data->xmit_buf &= ~BIT(offset);

Okay! Fix it in the v4.

quoted

+static int nct6694_direction_output(struct gpio_chip *gpio,
+                                   unsigned int offset, int val)
+{
+       struct nct6694_gpio_data *data = gpiochip_get_data(gpio);
+       int ret;
+
+       guard(mutex)(&data->lock);
+
+       /* Set direction to output */
+       ret = nct6694_read_msg(data->nct6694, NCT6694_GPIO_MOD,
+                              NCT6694_GPO_DIR + data->group,
+                              NCT6694_GPIO_LEN, &data->xmit_buf);
+       if (ret < 0)
+               return ret;
+
+       data->xmit_buf |= (1 << offset);

data->xmit_buf |= BIT(offset);

Okay! Fix it in the v4.

quoted

+       if (val)
+               data->xmit_buf |= (1 << offset);
+       else
+               data->xmit_buf &= ~(1 << offset);

Same

Okay! Fix it in the v4.

quoted

+static void nct6694_set_value(struct gpio_chip *gpio, unsigned int offset,
+                             int val)
+{

(...)

quoted

+       if (val)
+               data->xmit_buf |= (1 << offset);
+       else
+               data->xmit_buf &= ~(1 << offset);

Same

Okay! Fix it in the v4.

quoted

+static irqreturn_t nct6694_irq_handler(int irq, void *priv)
+{
+       struct nct6694_gpio_data *data = priv;
+       unsigned char status;
+
+       guard(mutex)(&data->lock);
+
+       nct6694_read_msg(data->nct6694, NCT6694_GPIO_MOD,
+                        NCT6694_GPI_STS + data->group,
+                        NCT6694_GPIO_LEN, &data->xmit_buf);
+
+       status = data->xmit_buf;
+
+       while (status) {
+               int bit = __ffs(status);
+
+               data->xmit_buf = BIT(bit);
+               handle_nested_irq(irq_find_mapping(data->gpio.irq.domain, bit));
+               status &= ~(1 << bit);

Same

Just use BIT() consistently please.

Okay! Fix it in the v4.

Best regards,
Ming