Hi Kent,
short reply from my private account - so only talking on behalf of myself.
Am Montag, 4. M?rz 2013, 18:41:04 schrieb Kent Yoder:
> Hi Peter,
>
> Sorry for the long delay in getting this reviewed...
No problem.
>
> > - for (count = 0; count < MAX_COUNT; count++) {
> > - usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> > - rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
> > - if (rc > 0)
> > - break;
> > + if (tpm_dev.chip_type == SLB9645) {
> > + /* use a combined read for newer chips
> > + * unfortunately the smbus functions are not suitable due to
> > + * the 32 byte limit of the smbus.
> > + * retries should usually not be needed, but are kept just to
> > + * be on the safe side.
> > + */
>
> Bump out that last line of comment by a column.
You mean I should add 1 space - okay, no problem.
(and I'm nitty picky.... ;) *g*
> >
> > if (rc <= 0)
> >
> > return -EIO;
>
> This confused me for a second, but I see __i2c_transfer returns the number
> of messages transferred. Maybe worth adding a comment.
Maybe I can add one - I'll think about it.
> >
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
> > + { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
> > + { .compatible = "infineon,slb9635tt", .data = (void *)0 },
> > + { .compatible = "infineon,slb9645tt", .data = (void *)1 },
>
> Here "name" and "type" are left empty in of_device_id. Will there be
> times when those are needed? Like informational messages from the OF
> subsystem?
Hmm, what do you propose?
name = chip type ? or name = tpm_i2c_infineon?
type = tpm ?
> >
> > + .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
>
> Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
> lives in there.
NACK.
of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as
#define of_match_ptr(_ptr) (_ptr)
or
#define of_match_ptr(_ptr) NULL
depending on CONFIG_OF is set.
> Won't you also need to add OF to Kconfig?
Not really, as the only stuff we're using is the compatible id - the driver can
live without it and can be probed from userspace or plain old platform data.
I probably have compile tested it with and without CONFIG_OF.
Is it worth a v3?
or a small update patch which adds the of name/type and the space? (I can
create this patch immediately)
Thanks,
Peter
>> >
>> > +
>> > +#ifdef CONFIG_OF
>> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
>> > + { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
>> > + { .compatible = "infineon,slb9635tt", .data = (void *)0 },
>> > + { .compatible = "infineon,slb9645tt", .data = (void *)1 },
>>
>> Here "name" and "type" are left empty in of_device_id. Will there be
>> times when those are needed? Like informational messages from the OF
>> subsystem?
>
> Hmm, what do you propose?
> name = chip type ? or name = tpm_i2c_infineon?
> type = tpm ?
Doesn't matter to me, I just wanted to be sure you hadn't missed anything.
>> > + .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
>>
>> Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
>> lives in there.
> NACK.
> of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as
>
> #define of_match_ptr(_ptr) (_ptr)
> or
> #define of_match_ptr(_ptr) NULL
> depending on CONFIG_OF is set.
Thanks.
>> Won't you also need to add OF to Kconfig?
> Not really, as the only stuff we're using is the compatible id - the driver can
> live without it and can be probed from userspace or plain old platform data.
> I probably have compile tested it with and without CONFIG_OF.
Ok.
> Is it worth a v3?
> or a small update patch which adds the of name/type and the space? (I can
> create this patch immediately)
Not a problem, I can apply as-is with the fixup.
Kent
>
> Thanks,
> Peter
>
>
>
On Mon, Mar 4, 2013 at 2:08 PM, Kent Yoder <[email protected]> wrote:
>>> >
>>> > +
>>> > +#ifdef CONFIG_OF
>>> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
>>> > + { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
>>> > + { .compatible = "infineon,slb9635tt", .data = (void *)0 },
>>> > + { .compatible = "infineon,slb9645tt", .data = (void *)1 },
>>>
>>> Here "name" and "type" are left empty in of_device_id. Will there be
>>> times when those are needed? Like informational messages from the OF
>>> subsystem?
>>
>> Hmm, what do you propose?
>> name = chip type ? or name = tpm_i2c_infineon?
>> type = tpm ?
>
> Doesn't matter to me, I just wanted to be sure you hadn't missed anything.
>
>>> > + .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
>>>
>>> Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
>>> lives in there.
>> NACK.
>> of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as
>>
>> #define of_match_ptr(_ptr) (_ptr)
>> or
>> #define of_match_ptr(_ptr) NULL
>> depending on CONFIG_OF is set.
>
> Thanks.
>
>>> Won't you also need to add OF to Kconfig?
>> Not really, as the only stuff we're using is the compatible id - the driver can
>> live without it and can be probed from userspace or plain old platform data.
>> I probably have compile tested it with and without CONFIG_OF.
>
> Ok.
>
>> Is it worth a v3?
>> or a small update patch which adds the of name/type and the space? (I can
>> create this patch immediately)
>
> Not a problem, I can apply as-is with the fixup.
Staged here:
https://github.com/shpedoikal/linux.git tpmdd-03-04-13
Thanks,
Kent
> Kent
>
>>
>> Thanks,
>> Peter
>>
>>
>>