Date:	Tue, 11 Sep 2012 11:47:02 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Samuel Iglesias Gonsálvez 
	<siglesias@...lia.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org,
	Jens Taprogge <jens.taprogge@...rogge.org>,
	linux-kernel@...r.kernel.org,
	industrypack-devel@...ts.sourceforge.net
Subject: Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new
 callbacks to tpci200


I had a few style comments on this patchset.  Nothing that couldn't
be fixed later.

On Mon, Sep 10, 2012 at 10:51:41AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@...rogge.org>
> 
> Provide get_clockrate, set_clockrate, get_error, get_timeout and reset_timeout
> callbacks.
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@...rogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@...lia.com>
> ---
>  drivers/staging/ipack/bridges/tpci200.c |  107 +++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
> index 4383953..861b00d 100644
> --- a/drivers/staging/ipack/bridges/tpci200.c
> +++ b/drivers/staging/ipack/bridges/tpci200.c
> @@ -14,6 +14,20 @@
>  #include <linux/module.h>
>  #include "tpci200.h"
>  
> +static u16 tpci200_status_timeout[] = {
> +	TPCI200_A_TIMEOUT,
> +	TPCI200_B_TIMEOUT,
> +	TPCI200_C_TIMEOUT,
> +	TPCI200_D_TIMEOUT,
> +};
> +
> +static u16 tpci200_status_error[] = {
> +	TPCI200_A_ERROR,
> +	TPCI200_B_ERROR,
> +	TPCI200_C_ERROR,
> +	TPCI200_D_ERROR,
> +};
> +
>  static struct ipack_bus_ops tpci200_bus_ops;
>  
>  static int tpci200_slot_unregister(struct ipack_device *dev);
> @@ -507,6 +521,94 @@ out_unlock:
>  	return res;
>  }
>  
> +static int tpci200_get_clockrate(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;

The point of the underscores in the __le16 is that you don't want to
pollute user space headers in glibc with a bunch of kernel typedefs.
It is not needed here.  (Or if it is, then we would need to replace
the u16 uses as well).

> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->control[dev->slot];
> +	return (ioread16(addr) & TPCI200_CLK32) ? 32 : 8;
> +}
> +
> +static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 reg;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->control[dev->slot];
> +
> +	/* Ensure the control register is not changed by another task after we
> +	 * have read it. */
> +	mutex_lock(&tpci200->mutex);
> +	reg = ioread16(addr);
> +	switch (mherz) {
> +	case 8:
> +		reg &= ~(TPCI200_CLK32); break;
> +	case 32:
> +		reg |= TPCI200_CLK32; break;

Put the breaks on the next line so that we can see them.  At first I
thought it fell through.

> +	default:
> +		mutex_unlock(&tpci200->mutex);
> +		return -EINVAL;
> +	}
> +	iowrite16(reg, addr);
> +	mutex_unlock(&tpci200->mutex);
> +	return 0;
> +}
> +
> +static int tpci200_get_error(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_error[dev->slot];
> +	return (ioread16(addr) & mask) ? 1 : 0;
> +}
> +
> +static int tpci200_get_timeout(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_timeout[dev->slot];
> +
> +	return (ioread16(addr) & mask) ? 1 : 0;
> +}
> +
> +static int tpci200_reset_timeout(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_timeout[dev->slot];
> +
> +	iowrite16(mask, addr);
> +	return 0;
> +}
> +
> +
> +

Only one blank line is here.

>  static void tpci200_uninstall(struct tpci200_board *tpci200)
>  {
>  	int i;
> @@ -524,6 +626,11 @@ static struct ipack_bus_ops tpci200_bus_ops = {
>  	.request_irq = tpci200_request_irq,
>  	.free_irq = tpci200_free_irq,
>  	.remove_device = tpci200_slot_unregister,
> +	.get_clockrate = tpci200_get_clockrate,
> +	.set_clockrate = tpci200_set_clockrate,
> +	.get_error     = tpci200_get_error,
> +	.get_timeout   = tpci200_get_timeout,
> +	.reset_timeout = tpci200_reset_timeout,
>  };
>  
>  static int tpci200_install(struct tpci200_board *tpci200)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> devel mailing list
> devel@...uxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists