Re: [PATCH] vme: change LM callback argument to void pointer

[Martyn Welch <martyn@xxxxxxxxxxxx>]

Hi Aaron,

On 24/04/16 00:03, Aaron Sierra wrote:
> Make the location monitor callback function prototype more useful by
> changing the argument from an integer to a void pointer.
>
> All VME bridge drivers were simply passing the location monitor index
> (e.g. 0-3) as the argument to these callbacks. It is much more useful
> to pass back a pointer to data that the callback-registering driver
> cares about.

Sounds good to me.

> I could not find any in-kernel callers of vme_lm_attach (or
> vme_lme_request for that matter), so this change only affects the VME
> subsystem and bridge drivers.

Yeah - there are currently a distinct lack of drivers for VME slaves. :-(

> I have tested with Tsi148 hardware, but the CA91Cx42 changes have only
> been compiled.

No problem.

Few comments:

 - Please add a clean-up of lm_cookie in vme_tsi148.c as you have done 
for vme_ca91cx42.c.
 - The documentation in Documentation/vme_api.txt needs updating.
 - Please rename "cookie" to "data".
 - Checkpatch.pl is throwing a warning, please can you fix that up.

Thanks,

Martyn

> Signed-off-by: Aaron Sierra <asierra@xxxxxxxxxxx>
> ---
>   drivers/vme/bridges/vme_ca91cx42.c | 6 ++++--
>   drivers/vme/bridges/vme_ca91cx42.h | 3 ++-
>   drivers/vme/bridges/vme_tsi148.c   | 5 +++--
>   drivers/vme/bridges/vme_tsi148.h   | 3 ++-
>   drivers/vme/vme.c                  | 4 ++--
>   drivers/vme/vme_bridge.h           | 3 ++-
>   include/linux/vme.h                | 2 +-
>   7 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vme/bridges/vme_ca91cx42.c b/drivers/vme/bridges/vme_ca91cx42.c
> index 0b2fefb..d3359a5 100644
> --- a/drivers/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/vme/bridges/vme_ca91cx42.c
> @@ -69,7 +69,7 @@ static u32 ca91cx42_LM_irqhandler(struct ca91cx42_driver *bridge, u32 stat)
>   	for (i = 0; i < 4; i++) {
>   		if (stat & CA91CX42_LINT_LM[i]) {
>   			/* We only enable interrupts if the callback is set */
> -			bridge->lm_callback[i](i);
> +			bridge->lm_callback[i](bridge->lm_cookie[i]);
>   			serviced |= CA91CX42_LINT_LM[i];
>   		}
>   	}
> @@ -1405,7 +1405,7 @@ static int ca91cx42_lm_get(struct vme_lm_resource *lm,
>    * Callback will be passed the monitor triggered.
>    */
>   static int ca91cx42_lm_attach(struct vme_lm_resource *lm, int monitor,
> -	void (*callback)(int))
> +	void (*callback)(void *), void *cookie)
>   {
>   	u32 lm_ctl, tmp;
>   	struct ca91cx42_driver *bridge;
> @@ -1433,6 +1433,7 @@ static int ca91cx42_lm_attach(struct vme_lm_resource *lm, int monitor,
>   
>   	/* Attach callback */
>   	bridge->lm_callback[monitor] = callback;
> +	bridge->lm_cookie[monitor] = cookie;
>   
>   	/* Enable Location Monitor interrupt */
>   	tmp = ioread32(bridge->base + LINT_EN);
> @@ -1472,6 +1473,7 @@ static int ca91cx42_lm_detach(struct vme_lm_resource *lm, int monitor)
>   
>   	/* Detach callback */
>   	bridge->lm_callback[monitor] = NULL;
> +	bridge->lm_cookie[monitor] = NULL;
>   
>   	/* If all location monitors disabled, disable global Location Monitor */
>   	if ((tmp & (CA91CX42_LINT_LM0 | CA91CX42_LINT_LM1 | CA91CX42_LINT_LM2 |
> diff --git a/drivers/vme/bridges/vme_ca91cx42.h b/drivers/vme/bridges/vme_ca91cx42.h
> index 02a7c79..5947831 100644
> --- a/drivers/vme/bridges/vme_ca91cx42.h
> +++ b/drivers/vme/bridges/vme_ca91cx42.h
> @@ -43,7 +43,8 @@ struct ca91cx42_driver {
>   	wait_queue_head_t dma_queue;
>   	wait_queue_head_t iack_queue;
>   	wait_queue_head_t mbox_queue;
> -	void (*lm_callback[4])(int);	/* Called in interrupt handler */
> +	void (*lm_callback[4])(void *);	/* Called in interrupt handler */
> +	void *lm_cookie[4];
>   	void *crcsr_kernel;
>   	dma_addr_t crcsr_bus;
>   	struct mutex vme_rmw;		/* Only one RMW cycle at a time */
> diff --git a/drivers/vme/bridges/vme_tsi148.c b/drivers/vme/bridges/vme_tsi148.c
> index 7db4e63..2347c40 100644
> --- a/drivers/vme/bridges/vme_tsi148.c
> +++ b/drivers/vme/bridges/vme_tsi148.c
> @@ -102,7 +102,7 @@ static u32 tsi148_LM_irqhandler(struct tsi148_driver *bridge, u32 stat)
>   	for (i = 0; i < 4; i++) {
>   		if (stat & TSI148_LCSR_INTS_LMS[i]) {
>   			/* We only enable interrupts if the callback is set */
> -			bridge->lm_callback[i](i);
> +			bridge->lm_callback[i](bridge->lm_cookie[i]);
>   			serviced |= TSI148_LCSR_INTC_LMC[i];
>   		}
>   	}
> @@ -2116,7 +2116,7 @@ static int tsi148_lm_get(struct vme_lm_resource *lm,
>    * Callback will be passed the monitor triggered.
>    */
>   static int tsi148_lm_attach(struct vme_lm_resource *lm, int monitor,
> -	void (*callback)(int))
> +	void (*callback)(void *), void *cookie)
>   {
>   	u32 lm_ctl, tmp;
>   	struct vme_bridge *tsi148_bridge;
> @@ -2146,6 +2146,7 @@ static int tsi148_lm_attach(struct vme_lm_resource *lm, int monitor,
>   
>   	/* Attach callback */
>   	bridge->lm_callback[monitor] = callback;
> +	bridge->lm_cookie[monitor] = cookie;
>   
>   	/* Enable Location Monitor interrupt */
>   	tmp = ioread32be(bridge->base + TSI148_LCSR_INTEN);
> diff --git a/drivers/vme/bridges/vme_tsi148.h b/drivers/vme/bridges/vme_tsi148.h
> index f5ed143..81a42ef 100644
> --- a/drivers/vme/bridges/vme_tsi148.h
> +++ b/drivers/vme/bridges/vme_tsi148.h
> @@ -38,7 +38,8 @@ struct tsi148_driver {
>   	void __iomem *base;	/* Base Address of device registers */
>   	wait_queue_head_t dma_queue[2];
>   	wait_queue_head_t iack_queue;
> -	void (*lm_callback[4])(int);	/* Called in interrupt handler */
> +	void (*lm_callback[4])(void *);	/* Called in interrupt handler */
> +	void *lm_cookie[4];
>   	void *crcsr_kernel;
>   	dma_addr_t crcsr_bus;
>   	struct vme_master_resource *flush_image;
> diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
> index 5b238df..8a09e02 100644
> --- a/drivers/vme/vme.c
> +++ b/drivers/vme/vme.c
> @@ -1206,7 +1206,7 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
>   EXPORT_SYMBOL(vme_lm_get);
>   
>   int vme_lm_attach(struct vme_resource *resource, int monitor,
> -	void (*callback)(int))
> +	void (*callback)(void *), void *cookie)
>   {
>   	struct vme_bridge *bridge = find_bridge(resource);
>   	struct vme_lm_resource *lm;
> @@ -1223,7 +1223,7 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
>   		return -EINVAL;
>   	}
>   
> -	return bridge->lm_attach(lm, monitor, callback);
> +	return bridge->lm_attach(lm, monitor, callback, cookie);
>   }
>   EXPORT_SYMBOL(vme_lm_attach);
>   
> diff --git a/drivers/vme/vme_bridge.h b/drivers/vme/vme_bridge.h
> index a3ef63b..31dc24f 100644
> --- a/drivers/vme/vme_bridge.h
> +++ b/drivers/vme/vme_bridge.h
> @@ -155,7 +155,8 @@ struct vme_bridge {
>   	int (*lm_set) (struct vme_lm_resource *, unsigned long long, u32, u32);
>   	int (*lm_get) (struct vme_lm_resource *, unsigned long long *, u32 *,
>   		u32 *);
> -	int (*lm_attach) (struct vme_lm_resource *, int, void (*callback)(int));
> +	int (*lm_attach) (struct vme_lm_resource *, int,
> +			  void (*callback)(void *), void *);
>   	int (*lm_detach) (struct vme_lm_resource *, int);
>   
>   	/* CR/CSR space functions */
> diff --git a/include/linux/vme.h b/include/linux/vme.h
> index d2a3b23..516c9f7c 100644
> --- a/include/linux/vme.h
> +++ b/include/linux/vme.h
> @@ -163,7 +163,7 @@ struct vme_resource *vme_lm_request(struct vme_dev *);
>   int vme_lm_count(struct vme_resource *);
>   int vme_lm_set(struct vme_resource *, unsigned long long, u32, u32);
>   int vme_lm_get(struct vme_resource *, unsigned long long *, u32 *, u32 *);
> -int vme_lm_attach(struct vme_resource *, int, void (*callback)(int));
> +int vme_lm_attach(struct vme_resource *, int, void (*callback)(void *), void *);
>   int vme_lm_detach(struct vme_resource *, int);
>   void vme_lm_free(struct vme_resource *);
>   

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel