Re: [PATCH v12 1/7] mfd: Add core driver for Nuvoton NCT6694

Ming Yu
2025-06-05T07:48:50Z

Dear Oliver,

Thank you for reviewing,

Oliver Neukum <oneukum@suse.com> 於 2025年6月4日 週三 下午6:11寫道：
>
> > +static void usb_int_callback(struct urb *urb)
> > +{
> > + struct nct6694 *nct6694 = urb->context;
> > + unsigned int *int_status = urb->transfer_buffer;
> > + int ret;
> > +
> > + switch (urb->status) {
> > + case 0:
> > + break;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + return;
> > + default:
> > + goto resubmit;
> > + }
> > +
> > + while (*int_status) {
> > + int irq = __ffs(*int_status);
> > +
> > + generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> > + *int_status &= ~BIT(irq);
> > + }
>
> Does modifying the byte have any benefit?
>

I will update the code in the next patch to use __le32 for the
variable to ensure proper endianness handling across architectures.

> > +resubmit:
> > + ret = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (ret)
> > + dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe", ERR_PTR(ret));
> > +}
> > +
> > +static void nct6694_irq_lock(struct irq_data *data)
> > +{
> > + struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> > +
> > + mutex_lock(&nct6694->irq_lock);
> > +}
>
> Why? Does this do anything but make it _harder_ to tell that you
> cannot take the lock in interrupt?
>

I plan to remove nct6694_irq_lock() and nct6694_bus_sync_unlock(), and
instead add the spinlock directly inside the function like this:
static void nct6694_irq_enable(struct irq_data *data)
{
...
spin_lock(&nct6694->irq_lock);
nct6694->irq_enable |= BIT(hwirq);
spin_unlock(&nct6694->irq_lock);
}

Do you think this approach is better?

Best regards,
Ming