--- On Wed, 23/11/11, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote: > So 'dev' is a driver_data structure. > but you pass it to dev_warn(). This is not what > dev_warn() is expecting. Correct; alas Philipp discovered this the hard way. I'm on it. > > +int navpoint_suspend(struct device *dev) > > static? Yes, will do. > > +int navpoint_resume(struct device *dev) > > static? Ditto. > > + /* Wait until SSP port is ready > for slave clock operations */ > > + while (pxa_ssp_read_reg(ssp, SSSR) > & SSSR_CSS) > > + msleep(1); > > What if it never becomes ready? Should this time > out? This loop was an early sanity check which I never removed. But I agree that broken hardware might cause it to hang. So I'll either remove it or, more usefully, add a timeout with error message. > > + drv_data = kzalloc(sizeof(struct > driver_data), GFP_KERNEL); > > We normally like to see sizeof(*drv_data) now. OK. > > + ret = request_irq(ssp->irq, > navpoint_int, 0, pdev->name, drv_data); > > + if (ret) > > + goto ret2; > > What if you immediately receive an interrupt right now, > after requesting > this interrupt? Will your handler try to dereference > anything in > drv_data which is -currently- NULL ? (I think you'll > oops when > dereferencing drv_data->ssp->something.) Shouldn't happen because the SSP port is not enabled until the later call to navpoint_resume(). However, Philipp has discovered spurious interrupts with the haret bootloader; perhaps it already enabled the SSP port. I'm on it. > > + (void) > navpoint_resume(&pdev->dev); > > That cast to 'void' is not required. True, but that function must return a value and I was just documenting the fact that it's being intentionally ignored here. I'm happy to remove it. > > + (void) > navpoint_suspend(&pdev->dev); > > Cast to void is not required. Ditto. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
