No subject

Hungarian notation) is brain damaged"

Also, all your exported routines severely lack any sort of locking. An IO
mutex or spinlock is mandatory here. 

> +static int pruss_mfd_add_devices(struct platform_device *pdev)
> +{
> +	struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
> +	struct device *dev = &pdev->dev;
> +	struct mfd_cell cell;
> +	u32 err, count;
> +
> +	for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
> +		memset(&cell, 0, sizeof(struct mfd_cell));
> +		cell.id			= count;
> +		cell.name		= (dev_data + count)->dev_name;
> +		cell.platform_data	= (dev_data + count)->pdata;
> +		cell.data_size		= (dev_data + count)->pdata_size;
> +
> +		err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
> +		if (err) {
> +			dev_err(dev, "cannot add mfd cells\n");
> +			return err;
> +		}
> +	}
> +	return err;
> +}
So, what are the potential subdevices for this driver ? If it's a really
dynamic setup, I'm fine with passing those as platform data but then do it so
that you pass a NULL terminated da8xx_pruss_devices array. That will avoid
most of the ugly casts you're doing here.

> diff --git a/include/linux/mfd/pruss/da8xx_pru.h b/include/linux/mfd/pruss/da8xx_pru.h
> new file mode 100644
> index 0000000..68d8421
> --- /dev/null
> +++ b/include/linux/mfd/pruss/da8xx_pru.h
> @@ -0,0 +1,122 @@
> +/*
> + * Copyright (C) 2010 Texas Instruments Incorporated
> + * Author: Jitendra Kumar <jitendra at mistralsolutions.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as  published by the
> + * Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef _PRUSS_H_
> +#define _PRUSS_H_
> +
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include "da8xx_prucore.h"
> +
> +#define PRUSS_NUM0			DA8XX_PRUCORE_0
> +#define PRUSS_NUM1			DA8XX_PRUCORE_1
Those are unused.

> diff --git a/include/linux/mfd/pruss/da8xx_prucore.h b/include/linux/mfd/pruss/da8xx_prucore.h
> new file mode 100644
> index 0000000..81f2ff9
> --- /dev/null
> +++ b/include/linux/mfd/pruss/da8xx_prucore.h
Please rename your mfd include directory to include/linux/mfd/da8xx/, so that
one can match it with the drivers/mfd/da8xx driver code.


> +typedef struct {
> +	u32 CONTROL;
> +	u32 STATUS;
> +	u32 WAKEUP;
> +	u32 CYCLECNT;
> +	u32 STALLCNT;
> +	u8  RSVD0[12];
> +	u32 CONTABBLKIDX0;
> +	u32 CONTABBLKIDX1;
> +	u32 CONTABPROPTR0;
> +	u32 CONTABPROPTR1;
> +	u8  RSVD1[976];
> +	u32 INTGPR[32];
> +	u32 INTCTER[32];
> +} *da8xx_prusscore_regs;
Again, we don't need that structure.

Cheers,
Samuel.


-- 
Intel Open Source Technology Centre
http://oss.intel.com/