Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x

[Kent Yoder <key@linux.vnet.ibm.com>]

On Wed, Dec 05, 2012 at 10:14:48PM +0100, Peter Hüwe wrote:
> Hi Kent, Matthias,
> 
> Am Mittwoch, 5. Dezember 2012, 19:07:20 schrieb Kent Yoder:
> >   Heh, duh, well of course it is. I've now staged everything I'm
> > planning on pushing at:
> > 
> > git://github.com/shpedoikal/linux.git tpmdd-12-05-12
> > 
> > Please test and let me know if I missed anything.
> 
> 
> I just checked out your commit from github.
> 
> Here's a part of the review:
> 
> Smatch still complains a bit, sparse is fine.:
> 
>  make -C /data/data-old/linux-2.6/ M=`pwd` C=1 CHECK=smatch modules
> make: Entering directory `/data/data-old/linux-2.6'
>   CHECK   /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c
> /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:535 tpm_stm_i2c_recv() warn: variable dereferenced before check 'chip' (see line 531)
> /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:748 tpm_st33_i2c_probe() warn: variable dereferenced before check 'platform_data' (see line 659)
> /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:848 tpm_st33_i2c_pm_resume() warn: should this be a bitwise op?
> /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:848 tpm_st33_i2c_pm_resume() warn: should this be a bitwise op?
>   CC [M]  /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.o
>   Building modules, stage 2.
>   MODPOST 6 modules
>   LD [M]  /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.ko
> make: Leaving directory `/data/data-old/linux-2.6'
> 
> Please check.
> 
> 
> (maybe also fix the checkpatch -strict stuff as well? Would be nice ;)
> 
> 
> 
> 
> 
> drivers/char/tpm/tpm_stm_st33_i2c.h 
> - Does the driver really need a seperate headerfile in drivers/char/tpm/ ? for me it seems everything can be included into the c-file.
> 
> > struct st_tpm_hash {
> >	int size;
> >	u8 *data;
> > };
> is unused - please remove.
> 
> > #define MINOR_NUM_I2C		224
> Please remove, it's unused and if you really need it use the one from tpm.h
> 
> include/linux/i2c/tpm_stm_st33_i2c.h
> I'm not sure if this is needed publicly? Or does only your driver need this?
> 
> >struct st33zp24_platform_data {
> Telling from the name I have no idea what this device is.
> 
> 
> 
> 
> 
> drivers/char/tpm/tpm_stm_st33_i2c.c
> 
> >enum stm33zp24_int_flags {
> >	TPM_GLOBAL_INT_ENABLE = 0x80,
> >	TPM_INTF_CMD_READY_INT = 0x080,
> >	TPM_INTF_FIFO_AVALAIBLE_INT = 0x040,
> >	TPM_INTF_WAKE_UP_READY_INT = 0x020,
> >	TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> >	TPM_INTF_STS_VALID_INT = 0x002,
> >	TPM_INTF_DATA_AVAIL_INT = 0x001,
> >};
> 
> Why the leading zeros? please remove.
> 
> 
> > static int tpm_st33_i2c_pm_suspend(struct i2c_client *client, pm_message_t mesg)
> >...
> > static int tpm_st33_i2c_pm_resume(struct i2c_client *client)
> >,,,
> >static struct i2c_driver tpm_st33_i2c_driver = {
> >	.driver = {
> >		   .owner = THIS_MODULE,
> >		   .name = TPM_ST33_I2C,
> >		   },
> >	.probe = tpm_st33_i2c_probe,
> >	.remove = tpm_st33_i2c_remove,
> >	.resume = tpm_st33_i2c_pm_resume,
> >	.suspend = tpm_st33_i2c_pm_suspend,
> >	.id_table = tpm_st33_i2c_id
> >};
> 
> Please convert resume/suspend  to .driver.pm 
> 
> It's pretty easy.
> See this post  for details
> http://sourceforge.net/mailarchive/message.php?msg_id=29516784
> Rafael did spent quite a lot of effort to convert almost every driver back then, so we should 'fix' new ones.

Not sure how easy this will be considering these routines are
i2c-specific -- they don't just call the tpm_tpm_* functions like the
other drivers.

Anyway, I left this off but I believe I've fixed up the other major
issues. We can leave the more nit-picky stuff for another day.

Please test:

git://github.com/shpedoikal/linux.git tpmdd-12-06-12

Kent

> 
> 
> > /*
> >  * tpm_st33_i2c_init initialize driver
> >  * @return: 0 if successful, else non zero value.
> >  */
> > static int __init tpm_st33_i2c_init(void)
> > {
> > 	return i2c_add_driver(&tpm_st33_i2c_driver);
> > }
> > 
> > /*
> >  * tpm_st33_i2c_exit The kernel calls this function during unloading the
> >  * module or during shut down process
> >  */
> > static void __exit tpm_st33_i2c_exit(void)
> > {
> > 	i2c_del_driver(&tpm_st33_i2c_driver);
> > }
> > 
> > module_init(tpm_st33_i2c_init);
> > module_exit(tpm_st33_i2c_exit);
> 
> Hooray for oneliners ;)
> + module_i2c_driver(tpm_st33_i2c_driver);
> 
> 
> 
> Keep on hacking ;)
> 
> Thanks,
> PeterH
> 
>