See below...
k
On 10/20/2010 09:53 AM, Arjan van de Ven wrote:
On+
+/* For tracking power state as set by User Space code */
+static unsigned char power_state = false;
why?
no. we don't want to add this ABI to the MeeGo kernel... it needs to be
done with device runtime PM anyway
(which is a requirement for MeeGo drivers on these platforms)
This is the sysfs interface that was previously in the driver (as it is
with the existing ak8974 driver)... we've left it there for now, while
we work on the runtime PM (which will follow). As alan mentioned, these
i2c sensor devices are "proving problematic", and we will clean up as
appropriate when the PM work is complete.
+static bool set_mode(struct i2c_client *client, unsigned char new_mode)
+{
+ unsigned char curr_mode;
+
+ /* Read the current mode and return if it's already in the new
mode */
+ curr_mode = i2c_smbus_read_byte_data(client, REG_CNTL);
+ if (curr_mode == new_mode)
+ return true;
+
+ /* Changing operating modes requires entry to Power Down mode
first */
+ i2c_smbus_write_byte_data(client, REG_CNTL, POWER_DOWN);
what locking is in use to ensure that nobody else talks to the hardware
during this sequence
There are no interrupts associated with this device, and the only
interface is through the virtual file read, from User Space. (The
filesystem isn't going to allow a read of this file to be preempted by
another such read.)
+
+ /* Delay a bit, then ensure the device is in Power Down mode */
+ msleep(0);
how long is this delay ??????
The duration is not critical to the HW interface... allowing a
schedule() really...
+static bool wait_for_data_ready(struct i2c_client *client)
+{
+ unsigned char data_ready = 0;
+ int wait_count = RETRY_COUNT;
+
+ /* Wait for Data Ready bit in ST1 to become 1 */
+ while (--wait_count) {
+ data_ready = i2c_smbus_read_byte_data(client, REG_ST1);
+ if (data_ready)
+ return true; /* Success */
Linux convention is to return 0 on success, negative error codes on failure
Obviously this is a static local to only this driver... so i assume you
just mean the its inconsistent to the coding standard, correct? Do we
need to get this now, or can we cycle back on this?
+ msleep(0);
how long is this???
+
see above.
+ /* Work function for retrieving magnetic field values from sensor */
+ while (retries++< RETRY_COUNT) {
+ /* Set Single Measurement mode */
+ if (set_mode(client, mode)) {
and here you need a lock or refcoutn (hint: runtime PM will likely just
give you that) to make sure nobody powers your device down as you go.
see above on the locking...
+ (void)set_mode(client, POWER_DOWN);
uh? why that cast?
just avoiding the compiler warning and making the ignore of the return
explicit. Can change if critical.
+ if (!set_mode(client, POWER_DOWN))
+ printk(KERN_WARNING "ak8975: Problem exiting Fuse ROM
mode !\n");
dev_err ? dev_warn ?
sure. can change if critical.
_______________________________________________
Meego-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
