Re: [PATCH] - TPM device driver layer (tpm.c|h) - 2nd repost

[Richard MUSIL]

Hello Andrew,

I am including 2nd version of the patch, slightly modified according
to your comments. See inline my response:

On 20.11.2007 7:37, Andrew Morton wrote:
> On Tue, 25 Sep 2007 15:14:50 +0200 Richard MUSIL <richard.musil@xxxxxx> wrote:
> 
>> The patch follows even more below.
> 
> Thanks.  We prefer that contributors sign off their work as per
> Documentation/SubmittingPatches.  Please review that and if agrereable,
> send a Signed-off-by: for this work.

Ok, done in repost.

>>  /*
>> + * Once all references to platform device are down to 0,
>> + * release all allocated structures.
>> + * In case vendor provided release function,
>> + * call it too.
>> + */
>> +static void tpm_dev_release(struct device *dev)
>> +{
>> +	struct tpm_chip *chip = dev_get_drvdata(dev);
>> +	/* call vendor release, if defined */
> 
> That's not the most useful of comments ;)

Agreed and removed :).

>> +	if (chip->vendor.release)
>> +		chip->vendor.release(dev);
>> +
>> +	/* it *should* be: chip->release != NULL */
> 
> And that one's actually wrong in the context of kernel coding practices. 
> But whatever.

Well I am not sure, what is exactly against coding practices (this is
my first patch, so bear with me). Was it the comment? Or the "likely".
But, anyway, I guess I was a bit paranoic. chip->release is set to 
original device::release and this should be set to platform_device_release
at least (and if someone messed with it, it should not be NULL anyway).
So I removed complete condition.

> 
>> +	if (likely(chip->release))
>> +		chip->release(dev);
> 
>>From my reading, neither of these fields can ever be NULL, so the tests
> simply aren't needed?

The first condition is needed, since the vendor does not have to
define release function for its own purpose. In fact, for example
current TIS driver does not even know about it.

--
Richard