On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote: > On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <marek.va...@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.va...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot