On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
Dear Marek Vasut,
thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut marek.vasut@gmail.com
wrote:
On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.
[...]
Quick points:
- The license
Please suggest the appropriate file header text.
Uh ... you should know the license !!!
removed the BSD part
Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.
[..]
+struct lpc_tpm {
struct tpm_locality locality[TPM_TOTAL_LOCALITIES];+};
Do you need such envelope ?
I think I do - this accurately describes the structure of the chip.
There's just one member ... it's weird?
I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.
can't you pass the locality array ?
[..]
Dot missing at the end.
ok.
Please fix globally.
done
+#define TPM_DRIVER_ERR (-1)
- /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+/* Retrieve burst count value out of the status register contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + TIS_STS_BURST_COUNT_MASK))
Do you need the cast ?
I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.
Make it an inline function then, this will do the typechecking for you.
I am not sure what is wrong with a short macro in this case - is this against the coding style?
It doesn't do typechecking.
+/*
- Structures defined below allow creating descriptions of TPM
vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
- */
+struct device_name {
u16 dev_id;const char * const dev_name;+};
+struct vendor_name {
u16 vendor_id;const char *vendor_name;const struct device_name *dev_names;+};
+static const struct device_name infineon_devices[] = {
{0xb, "SLB9635 TT 1.2"},{0}+};
+static const struct vendor_name vendor_names[] = {
{0x15d1, "Infineon", infineon_devices},+};
+/*
- Cached vendor/device ID pair to indicate that the device has been
already + * discovered
- */
+static u32 vendor_dev_id;
+/* TPM access going through macros to make tracing easier. */ +#define tpm_read(ptr) ({ \
u32 __ret; \__ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \debug(PREFIX "Read reg 0x%x returns 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __ret); \__ret; })Make this inline function
+#define tpm_write(value, ptr) ({ \
u32 __v = value; \debug(PREFIX "Write reg 0x%x with 0x%x\n", \(u32)ptr - (u32)lpc_tpm_dev, __v); \if (sizeof(*ptr) == 1) \writeb(__v, ptr); \else \writel(__v, ptr); })DTTO
Are you sure these will work as inline functions?
Why not ? Also, why do you introduce the __v ?
macro vs function: need to be able to tell the pointed object size at run time.
This seems wrong like hell.
__v is needed to avoid side effects when invoking the macro.
Side effects ? What side effects ?
[...]
Cheers