From: "Peter Hüwe" <PeterHuewe@gmx.de>
To: Kent Yoder <key@linux.vnet.ibm.com>
Cc: Mathias LEBLANC <Mathias.LEBLANC@st.com>,
"Jean-Luc BLANC" <jean-luc.blanc@st.com>,
"Sirrix@jasper.es" <Sirrix@jasper.es>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Rajiv Andrade <mail@srajiv.net>,
"tpmdd-devel@lists.sourceforge.net"
<tpmdd-devel@lists.sourceforge.net>,
Kent Yoder <shpedoikal@gmail.com>
Subject: Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x
Date: Wed, 5 Dec 2012 22:14:48 +0100 [thread overview]
Message-ID: <201212052214.48840.PeterHuewe@gmx.de> (raw)
In-Reply-To: <20121205180720.GG21050@ennui.austin.ibm.com>
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.
> /*
> * 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
next prev parent reply other threads:[~2012-12-05 21:14 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-19 22:15 [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x Mathias Leblanc
2012-11-19 22:15 ` [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 2.6 Mathias Leblanc
2012-11-26 17:46 ` [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x Kent Yoder
2012-11-27 8:44 ` Mathias LEBLANC
2012-11-27 14:48 ` [tpmdd-devel] " Kent Yoder
2012-11-28 8:54 ` Mathias LEBLANC
2012-11-28 15:31 ` Kent Yoder
2012-11-28 17:48 ` Mathias LEBLANC
2012-11-28 19:04 ` Kent Yoder
2012-11-29 0:04 ` Peter Hüwe
2012-12-05 16:11 ` Mathias LEBLANC
2012-12-05 17:13 ` Kent Yoder
2012-12-05 17:45 ` Kent Yoder
2012-12-05 18:07 ` Kent Yoder
2012-12-05 20:20 ` Peter Hüwe
2012-12-05 21:00 ` Kent Yoder
2012-12-05 21:39 ` Peter Hüwe
2012-12-05 21:14 ` Peter Hüwe [this message]
2012-12-05 23:09 ` Kent Yoder
2012-12-06 0:10 ` Peter Hüwe
2012-12-06 15:06 ` Kent Yoder
2012-12-08 4:00 ` [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x OOPS! Peter Hüwe
[not found] ` <20130108172053.GA11223@ennui.austin.ibm.com>
2013-01-09 14:31 ` Mathias LEBLANC
2013-01-09 19:41 ` Peter Hüwe
2013-01-22 23:30 ` Kent Yoder
2012-12-06 0:20 ` [PATCH] char/tpm: Use struct dev_pm_ops for power management Peter Huewe
2012-12-06 15:07 ` Kent Yoder
2012-12-10 18:11 ` Mathias LEBLANC
2012-12-06 16:27 ` Kent Yoder
2012-12-08 3:55 ` Peter Hüwe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201212052214.48840.PeterHuewe@gmx.de \
--to=peterhuewe@gmx.de \
--cc=Mathias.LEBLANC@st.com \
--cc=Sirrix@jasper.es \
--cc=jean-luc.blanc@st.com \
--cc=key@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mail@srajiv.net \
--cc=shpedoikal@gmail.com \
--cc=tpmdd-devel@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).