Date: Wed, 25 Feb 2015 13:30:08 +0700
From: Johan Hovold <johan@kernel.org>
To: Peter Hung <hpeter@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw,
        peter_hong@fintek.com.tw, Peter Hung <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V7 02/11] USB: f81232: add preparatory functions
Message-ID: <20150225063008.GU12405@localhost>
References: <1424772986-5542-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1424772986-5542-3-git-send-email-hpeter+linux_kernel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1424772986-5542-3-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.5.22 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3854
Lines: 158

On Tue, Feb 24, 2015 at 06:16:17PM +0800, Peter Hung wrote:
> We add f81232_get_register()/f81232_set_register() and necessary defines
> for future patch with communication with F81232 USB control endpoint.
>
> Because of this is a preparatory patch, there are unused function warning
> with compiling. The functions will used with following patches.

Ok, adding these separately facilitates review, but please add them in
the patches that first use them after addressing the comments below (to
avoid those build warning). Sorry about the extra work.

> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..1f29b95 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -30,6 +30,11 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST	0xA0
> +#define F81232_GET_REGISTER	0xc0
> +#define F81232_SET_REGISTER	0x40
> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -50,6 +55,80 @@ struct f81232_private {
>  	u8 modem_status;
>  };
>  
> +static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> +{

Please rename "data" to "val" here (and in set_register).

> +	int status;
> +	u8 *tmp;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	if (!data)
> +		return -EFAULT;

No need to test for NULL here.

> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;

Use sizeof(*val) throughout.

> +
> +	status = usb_control_msg(dev,
> +				usb_rcvctrlpipe(dev, 0),
> +				F81232_REGISTER_REQUEST,
> +				F81232_GET_REGISTER,
> +				reg,
> +				0,
> +				tmp,
> +				sizeof(u8),
> +				USB_CTRL_GET_TIMEOUT);
> +	if (status <= 0) {

Test for status != sizeof(*val) here.

> +		/* show something with failed */

Drop the comment.

> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> +		if (status == 0)
> +			status = -EIO;
> +		else
> +			status = usb_translate_errors(status);
> +	} else {
> +		status = 0; /* on success */

Comment not needed.

> +		memcpy((void*) data, (void*) tmp, sizeof(u8));

Just do

	*val = *tmp;

here.

> +	}
> +
> +	kfree(tmp);
> +	return status;
> +}
> +
> +static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)

Use val instead of data here as well.

> +{
> +	int status;
> +	u8 *tmp;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	memcpy((void*) tmp, (void*) &data, sizeof(u8));

*tmp = *val

> +
> +	status = usb_control_msg(dev,
> +				usb_sndctrlpipe(dev, 0),
> +				F81232_REGISTER_REQUEST,
> +				F81232_SET_REGISTER,
> +				reg,
> +				0,
> +				tmp,
> +				sizeof(u8),
> +				USB_CTRL_SET_TIMEOUT);
> +	if (status <= 0) {
> +		/* show something with failed */
> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> +		if (status == 0)
> +			status = -EIO;
> +		else
> +			status = usb_translate_errors(status);
> +	} else
> +		status = 0; /* on success */

Missing brackets on else branch. No need for comment.

> +
> +	kfree(tmp);
> +	return status;
> +}

Most comments to get_register apply here as well.

>  static void f81232_update_line_status(struct usb_serial_port *port,
>  				      unsigned char *data,
>  				      unsigned int actual_length)

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/