diff for duplicates of <20110221163027.GF10686@sortiz-mobl> diff --git a/a/1.txt b/N1/1.txt index c1f81ed..ba135e1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -86,3 +86,100 @@ Then routines like this one will look a lot more readable. > +s16 pruss_writeb(struct device *dev, u32 u32offset, > + u8 *pu8datatowrite, u16 u16bytestowrite) +>From CodingStyle: "Encoding the type of a function into the name (so-called +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@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/ diff --git a/a/content_digest b/N1/content_digest index 114769d..37d5cc5 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,9 +1,15 @@ "ref\01297435892-28278-1-git-send-email-subhasish@mistralsolutions.com\0" "ref\01297435892-28278-2-git-send-email-subhasish@mistralsolutions.com\0" - "From\0sameo@linux.intel.com (Samuel Ortiz)\0" - "Subject\0[PATCH v2 01/13] mfd: pruss mfd driver.\0" + "From\0Samuel Ortiz <sameo@linux.intel.com>\0" + "Subject\0Re: [PATCH v2 01/13] mfd: pruss mfd driver.\0" "Date\0Mon, 21 Feb 2011 17:30:28 +0100\0" - "To\0linux-arm-kernel@lists.infradead.org\0" + "To\0Subhasish Ghosh <subhasish@mistralsolutions.com>\0" + "Cc\0davinci-linux-open-source@linux.davincidsp.com" + linux-arm-kernel@lists.infradead.org + m-watkins@ti.com + nsekhar@ti.com + sachi@mistralsolutions.com + " open list <linux-kernel@vger.kernel.org>\0" "\00:1\0" "b\0" "Hi Subhasish,\n" @@ -93,6 +99,103 @@ "Then routines like this one will look a lot more readable.\n" "\n" "> +s16 pruss_writeb(struct device *dev, u32 u32offset,\n" - "> +\t\tu8 *pu8datatowrite, u16 u16bytestowrite)" + "> +\t\tu8 *pu8datatowrite, u16 u16bytestowrite)\n" + ">From CodingStyle: \"Encoding the type of a function into the name (so-called\n" + "Hungarian notation) is brain damaged\"\n" + "\n" + "Also, all your exported routines severely lack any sort of locking. An IO\n" + "mutex or spinlock is mandatory here. \n" + "\n" + "> +static int pruss_mfd_add_devices(struct platform_device *pdev)\n" + "> +{\n" + "> +\tstruct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;\n" + "> +\tstruct device *dev = &pdev->dev;\n" + "> +\tstruct mfd_cell cell;\n" + "> +\tu32 err, count;\n" + "> +\n" + "> +\tfor (count = 0; (dev_data + count)->dev_name != NULL; count++) {\n" + "> +\t\tmemset(&cell, 0, sizeof(struct mfd_cell));\n" + "> +\t\tcell.id\t\t\t= count;\n" + "> +\t\tcell.name\t\t= (dev_data + count)->dev_name;\n" + "> +\t\tcell.platform_data\t= (dev_data + count)->pdata;\n" + "> +\t\tcell.data_size\t\t= (dev_data + count)->pdata_size;\n" + "> +\n" + "> +\t\terr = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);\n" + "> +\t\tif (err) {\n" + "> +\t\t\tdev_err(dev, \"cannot add mfd cells\\n\");\n" + "> +\t\t\treturn err;\n" + "> +\t\t}\n" + "> +\t}\n" + "> +\treturn err;\n" + "> +}\n" + "So, what are the potential subdevices for this driver ? If it's a really\n" + "dynamic setup, I'm fine with passing those as platform data but then do it so\n" + "that you pass a NULL terminated da8xx_pruss_devices array. That will avoid\n" + "most of the ugly casts you're doing here.\n" + "\n" + "> diff --git a/include/linux/mfd/pruss/da8xx_pru.h b/include/linux/mfd/pruss/da8xx_pru.h\n" + "> new file mode 100644\n" + "> index 0000000..68d8421\n" + "> --- /dev/null\n" + "> +++ b/include/linux/mfd/pruss/da8xx_pru.h\n" + "> @@ -0,0 +1,122 @@\n" + "> +/*\n" + "> + * Copyright (C) 2010 Texas Instruments Incorporated\n" + "> + * Author: Jitendra Kumar <jitendra@mistralsolutions.com>\n" + "> + *\n" + "> + * This program is free software; you can redistribute it and/or modify it\n" + "> + * under the terms of the GNU General Public License as published by the\n" + "> + * Free Software Foundation version 2.\n" + "> + *\n" + "> + * This program is distributed \"as is\" WITHOUT ANY WARRANTY of any kind,\n" + "> + * whether express or implied; without even the implied warranty of\n" + "> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU\n" + "> + * General Public License for more details.\n" + "> + */\n" + "> +\n" + "> +#ifndef _PRUSS_H_\n" + "> +#define _PRUSS_H_\n" + "> +\n" + "> +#include <linux/types.h>\n" + "> +#include <linux/platform_device.h>\n" + "> +#include \"da8xx_prucore.h\"\n" + "> +\n" + "> +#define PRUSS_NUM0\t\t\tDA8XX_PRUCORE_0\n" + "> +#define PRUSS_NUM1\t\t\tDA8XX_PRUCORE_1\n" + "Those are unused.\n" + "\n" + "> diff --git a/include/linux/mfd/pruss/da8xx_prucore.h b/include/linux/mfd/pruss/da8xx_prucore.h\n" + "> new file mode 100644\n" + "> index 0000000..81f2ff9\n" + "> --- /dev/null\n" + "> +++ b/include/linux/mfd/pruss/da8xx_prucore.h\n" + "Please rename your mfd include directory to include/linux/mfd/da8xx/, so that\n" + "one can match it with the drivers/mfd/da8xx driver code.\n" + "\n" + "\n" + "> +typedef struct {\n" + "> +\tu32 CONTROL;\n" + "> +\tu32 STATUS;\n" + "> +\tu32 WAKEUP;\n" + "> +\tu32 CYCLECNT;\n" + "> +\tu32 STALLCNT;\n" + "> +\tu8 RSVD0[12];\n" + "> +\tu32 CONTABBLKIDX0;\n" + "> +\tu32 CONTABBLKIDX1;\n" + "> +\tu32 CONTABPROPTR0;\n" + "> +\tu32 CONTABPROPTR1;\n" + "> +\tu8 RSVD1[976];\n" + "> +\tu32 INTGPR[32];\n" + "> +\tu32 INTCTER[32];\n" + "> +} *da8xx_prusscore_regs;\n" + "Again, we don't need that structure.\n" + "\n" + "Cheers,\n" + "Samuel.\n" + "\n" + "\n" + "-- \n" + "Intel Open Source Technology Centre\n" + http://oss.intel.com/ -77debaaef7e1f3395f11ea3c65688ef589eba0b10ed3bae1018265bee1471f34 +6dfa436de4b85970ed4a6a7b5b71c1c670050355a8b85387101113a259db83b8
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.