Re: [PATCH v4 1/1] mfd: Add Host Interface (HIF) support for Nuvoton NCT6694

[Andrew Lunn <andrew@lunn.ch>]

>  MAINTAINERS                         |   1 +
>  drivers/gpio/gpio-nct6694.c         |   7 -
>  drivers/hwmon/nct6694-hwmon.c       |  21 -
>  drivers/i2c/busses/i2c-nct6694.c    |   7 -
>  drivers/mfd/Kconfig                 |  47 +-
>  drivers/mfd/Makefile                |   3 +-
>  drivers/mfd/nct6694-hif.c           | 663 ++++++++++++++++++++++++++++
>  drivers/mfd/nct6694.c               | 111 +++--
>  drivers/net/can/usb/nct6694_canfd.c |   6 -

The networking change here is very small, so my influence as a
networking Maintainer should be considered small.

However, i would say this patch is too big, does too many different
things at once, making it harder to review. Please could you break it
up into lots of small patches, each with good commit messages, and
being obviously correct.

> +F:	drivers/mfd/nct6694-hif.c

Maybe move all the usb code into nct6694-usb.c ?

> - * USB command module type for NCT6694 GPIO controller.
> - * This defines the module type used for communication with the NCT6694
> - * GPIO controller over the USB interface.
> - */
> -#define NCT6694_GPIO_MOD	0xFF
> -
>  #define NCT6694_GPIO_VER	0x90
>  #define NCT6694_GPIO_VALID	0x110
>  #define NCT6694_GPI_DATA	0x120

Moving code from one place to another can be a patch. Just moving code
is quick and easy to review, and it gets it out of more complex
patches which are harder to review.

> +static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char err_status)
> +{
> +	switch (err_status) {
> +	case NCT6694_NO_ERROR:
> +		return 0;
> +	case NCT6694_NOT_SUPPORT_ERROR:
> +		dev_err(nct6694->dev, "Command is not supported!\n");
> +		break;

Maybe EOPNOTSUPP?

> +	case NCT6694_NO_RESPONSE_ERROR:
> +		dev_warn(nct6694->dev, "Command received no response!\n");
> +		break;
> +	case NCT6694_TIMEOUT_ERROR:
> +		dev_warn(nct6694->dev, "Command timed out!\n");
> +		break;

Maybe ETIMEDOUT?


> +	case NCT6694_PENDING:
> +		dev_err(nct6694->dev, "Command is pending!\n");
> +		break;

EBUSY?

Having different error codes can make it easier to debug when things
so wrong. But you also have dev_err(), so it is less important.

> -static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char err_status)
> +static int nct6694_usb_err_handling(struct nct6694 *nct6694,
> +				    unsigned char err_status)

These renames can happen in one patch. Again, it is quick and easy to
review.

>  
> -	guard(mutex)(&nct6694->access_lock);
> +	guard(mutex)(&udata->access_lock);

This change is not obviously correct. Can moving the lock be made of
patch of its own, with an explanation of why?

> -	nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL);
> -	if (!nct6694->usb_msg)
> +	udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
> +	if (!udata)
>  		return -ENOMEM;
>  
> -	nct6694->int_buffer = devm_kzalloc(dev, sizeof(*nct6694->int_buffer), GFP_KERNEL);
> -	if (!nct6694->int_buffer)
> +	udata->usb_msg = devm_kzalloc(dev, sizeof(*udata->usb_msg), GFP_KERNEL);
> +	if (!udata->usb_msg)
>  		return -ENOMEM;
>  
> -	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!nct6694->int_in_urb)
> +	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
> +	if (!udata->int_buffer)
>  		return -ENOMEM;
>  
> +	udata->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!udata->int_in_urb)
> +		return -ENOMEM;


In this hunk, diff(1) has done a poor job and made it harder to
review. If i understand the code correctly, udata contains USB
specific data? Maybe call it usdata? That also has the same length as
ntc6694, which has some minor advantages. What you might find is that
if you have a patch adding only the allocation of usbdata, and then a
patch moving things into usbdata, diff(1) does a better job, and the
code is more obviously correct.

> @@ -305,16 +344,15 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	}
>  
>  	nct6694->dev = dev;
> -	nct6694->udev = udev;
> +
> +	spin_lock_init(&nct6694->irq_lock);
>  
>  	ida_init(&nct6694->gpio_ida);
>  	ida_init(&nct6694->i2c_ida);
>  	ida_init(&nct6694->canfd_ida);
>  	ida_init(&nct6694->wdt_ida);
>  
> -	spin_lock_init(&nct6694->irq_lock);
> -

Why has the spin_lock_init() moved? Having lots of small patches would
make that stand out, and when you reviewed your own patches, you might
decided to change it back, because it does not appear to be needed.

Given the size of this patch, i'm finding it hard to see the overall
structure. Generally, when you have one device with two different
access mechanisms, you end up with three files, two implementing
access, and the third with the common code. With one big patch, i
don't see this common code.

	Andrew