From: Jonathan Cameron <jic23@kernel.org>
To: Denis CIOCCA <denis.ciocca@st.com>
Cc: lars@metafoo.de, linux-iio@vger.kernel.org
Subject: Re: STMicroelectronics sensors driver
Date: Sun, 16 Dec 2012 11:28:27 +0000 [thread overview]
Message-ID: <50CDB05B.9000206@kernel.org> (raw)
In-Reply-To: <1355404302-2239-1-git-send-email-denis.ciocca@st.com>
On 12/13/2012 01:11 PM, Denis CIOCCA wrote:
> Hi Jonathan & Lars-Peter,
>
> I wrote this new patch to include the common library and device drivers
> for accel, magn and gyro.
> I didn't looked still for tx and rx buffers issue because I have to study
> better the problem.
Sure, though in this case it is just a case of making sure the cacheline
aligned elements are at the end of the structure.
> What do you think about this library?
>
Couple of general things to do.
Run one or more static checkers over the code before posting (just saves time
in review by clearing out some standard issues)
(good choices are sparse, smatch and coccicheck (coccinelle wrapper)).
Sparse gives:
drivers/iio/accel/st_accel_trigger.c:25:5: warning: symbol 'st_accel_trig_acc_set_state' was not declared. Should it be
static?
drivers/iio/accel/st_accel_trigger.c:49:30: warning: symbol 'st_accel_trigger_ops' was not declared. Should it be static?
CC [M] drivers/iio/accel/st_accel_trigger.o
CHECK drivers/iio/accel/st_accel_buffer.c
CC [M] drivers/iio/accel/st_accel_buffer.o
CHECK drivers/iio/accel/st_accel_core.c
CC [M] drivers/iio/accel/st_accel_core.o
LD [M] drivers/iio/accel/st_accel.o
CHECK drivers/iio/common/st_sensors/st_sensors_core.c
CC [M] drivers/iio/common/st_sensors/st_sensors_core.o
drivers/iio/common/st_sensors/st_sensors_core.c: In function 'st_sensors_set_enable':
drivers/iio/common/st_sensors/st_sensors_core.c:182:27: warning: 'odr_out.hz' may be used uninitialized in this function
CHECK drivers/iio/common/st_sensors/st_sensors_i2c.c
drivers/iio/common/st_sensors/st_sensors_i2c.c:21:14: warning: symbol 'st_sensors_i2c_get_irq' was not declared. Should
it be static?
CC [M] drivers/iio/common/st_sensors/st_sensors_i2c.o
CHECK drivers/iio/common/st_sensors/st_sensors_spi.c
drivers/iio/common/st_sensors/st_sensors_spi.c:22:14: warning: symbol 'st_sensors_spi_get_irq' was not declared. Should
it be static?
CC [M] drivers/iio/common/st_sensors/st_sensors_spi.o
CHECK drivers/iio/common/st_sensors/st_sensors_trigger.c
CC [M] drivers/iio/common/st_sensors/st_sensors_trigger.o
CHECK drivers/iio/common/st_sensors/st_sensors_buffer.c
CC [M] drivers/iio/common/st_sensors/st_sensors_buffer.o
LD [M] drivers/iio/common/st_sensors/st_sensors_triggered_buffer.o
CHECK drivers/iio/gyro/st_gyro_i2c.c
CC [M] drivers/iio/gyro/st_gyro_i2c.o
CHECK drivers/iio/gyro/st_gyro_spi.c
CC [M] drivers/iio/gyro/st_gyro_spi.o
CHECK drivers/iio/gyro/st_gyro_trigger.c
drivers/iio/gyro/st_gyro_trigger.c:23:5: warning: symbol 'st_gyro_trig_acc_set_state' was not declared. Should it be static?
drivers/iio/gyro/st_gyro_trigger.c:47:30: warning: symbol 'st_gyro_trigger_ops' was not declared. Should it be static?
CC [M] drivers/iio/gyro/st_gyro_trigger.o
CHECK drivers/iio/gyro/st_gyro_buffer.c
CC [M] drivers/iio/gyro/st_gyro_buffer.o
CHECK drivers/iio/gyro/st_gyro_core.c
drivers/iio/gyro/st_gyro_core.c:95:28: warning: symbol 'st_gyro_16bit_channels' was not declared. Should it be static?
CC [M] drivers/iio/gyro/st_gyro_core.o
LD [M] drivers/iio/gyro/st_gyro.o
CHECK drivers/iio/magnetometer/st_magn_i2c.c
CC [M] drivers/iio/magnetometer/st_magn_i2c.o
CHECK drivers/iio/magnetometer/st_magn_spi.c
CC [M] drivers/iio/magnetometer/st_magn_spi.o
CHECK drivers/iio/magnetometer/st_magn_trigger.c
drivers/iio/magnetometer/st_magn_trigger.c:43:30: warning: symbol 'st_magn_trigger_ops' was not declared. Should it be
static?
CC [M] drivers/iio/magnetometer/st_magn_trigger.o
CHECK drivers/iio/magnetometer/st_magn_buffer.c
CC [M] drivers/iio/magnetometer/st_magn_buffer.o
CHECK drivers/iio/magnetometer/st_magn_core.c
CC [M] drivers/iio/magnetometer/st_magn_core.o
LD [M] drivers/iio/magnetometer/st_magn.o
Kernel: arch/arm/boot/Image is ready
Kernel: arch/arm/boot/zImage is ready
Building modules, stage 2.
MODPOST 226 modules
Coccinelle picks up various things including...
drivers/iio/common/st_sensors/st_sensors_spi.c:67:1-7: preceding lock on line 49
(basically you don't release the mutex on an error path).
Anyhow, these tools are a great help in catching some
types of problems.
prev parent reply other threads:[~2012-12-16 11:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-13 13:11 STMicroelectronics sensors driver Denis CIOCCA
2012-12-13 13:11 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2012-12-16 12:15 ` Jonathan Cameron
2012-12-13 13:11 ` [PATCH 2/4] iio:accel: Add STMicroelectronics accelerometers driver Denis CIOCCA
2012-12-16 11:51 ` Jonathan Cameron
2012-12-13 13:11 ` [PATCH 3/4] iio:gyro: Add STMicroelectronics gyroscopes driver Denis CIOCCA
2012-12-13 13:11 ` [PATCH 4/4] iio:magnetometer: Add STMicroelectronics magnetometers driver Denis CIOCCA
2012-12-16 11:28 ` Jonathan Cameron [this message]
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=50CDB05B.9000206@kernel.org \
--to=jic23@kernel.org \
--cc=denis.ciocca@st.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
/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).