Hi,
this is the author.
Ben Dooks said the following:
> On Thu, Jan 07, 2010 at 11:54:20AM -0800, David Daney wrote:
<snip>
>> +#ifndef NO_IRQ
>> +#define NO_IRQ (-1)
>> +#endif
>
> this does not fill me with a warm joyous feeling...
see near end of reply.
>
>> +struct octeon_i2c {
>> + wait_queue_head_t queue;
>> + struct i2c_adapter adap;
>> + int irq;
>> + int twsi_freq;
>> + int sys_freq;
>> + uint8_t twsi_ctl;
>> + resource_size_t twsi_phys;
>> + void __iomem *twsi_base;
>> + resource_size_t regsize;
>> + struct device *dev;
>> +};
>
> kerneldoc or any documentation at-all would be nice.
This is driver private data, used within this file only. Usage should be
rather obvious?!
<snip>
>> + do {
>> + data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
>> + if (data == STAT_IDLE)
>> + break;
>> + udelay(1);
>> + } while (!time_after(jiffies, orig_jiffies + 2));
>
> you sure you want to busy wait like this?
>
Hmm, yes?
Its a busy wait of typically 10us, but not guaranteed.
Timeout is 1 to 2 jiffies.
Any other suggestion?
<snip>
>> + i2c_data = pdev->dev.platform_data;
>> +
>> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + irq = platform_get_irq(pdev, 0);
>> +
>> + if (res_mem == NULL) {
>> + dev_dbg(i2c->dev, "%s: found no memory resource\n", __func__);
>> + kfree(i2c);
>> + return -ENODEV;
>> + }
>> +
>> + if (i2c_data == NULL) {
>> + dev_dbg(i2c->dev, "%s: no I2C frequence data\n", __func__);
>> + kfree(i2c);
>> + return -ENODEV;
>> + }
>
> returning -ENODEV isn't a good idea, the device layer won't print an
> error on seeing -ENODEV as it thinks this is a probe for a device that
> was never there.
>
ACK, couldn't find something really appropriate. Will change to EINVAL.
<snip>
>> + i2c->twsi_phys = res_mem->start;
>> + i2c->regsize = resource_size(res_mem);
>> + i2c->twsi_freq = i2c_data->i2c_freq;
>> + i2c->sys_freq = i2c_data->sys_freq;
>> +
>> + if (!request_mem_region(i2c->twsi_phys, i2c->regsize, res_mem->name)) {
>> + dev_dbg(i2c->dev,
>> + "%s i2c-cavium - request_mem_region failed\n",
>> + __func__);
>> + goto fail_region;
>> + }
>> + i2c->twsi_base = ioremap(i2c->twsi_phys, i2c->regsize);
>> +
>> + init_waitqueue_head(&i2c->queue);
>> +
>> + i2c->irq = irq;
>> + if (i2c->irq != NO_IRQ) {
>
> platform_get_irq() returns a negative error cdoe if no irq is found,
> so you need to do (irq < 0) here otherwise it'll never trip. Should
> also mean yo ucan get rid of NO_IRQ define above.
NO_IRQ is thought of as polling.
The code assumes that request_irq() denies irq < 0
After reviewing kernel code I think this should not be assumed.
I will have to contemplate about this.
<snip>
>> +static struct platform_driver octeon_i2c_driver = {
>> + .probe = octeon_i2c_probe,
>> + .remove = __exit_p(octeon_i2c_remove),
> __devexit_p() here, probably should change __exit to __devexit on the
> remove function.
I thought I had read something about abuse of __devexit (to be used only
for hotplugable devices), but I can't find it anymore.
I'll change.
Currently I'm unsure about using __devinit for probe().
It will be called twice (2 devices), but I can't absolutely tell that
second call will be in system initialization phase.
Your 2 Cents?
>> + .driver = {
>> + .owner = THIS_MODULE,
>> + .name = DRV_NAME,
>> + },
>> +};
>> +
>> +static int __init octeon_i2c_init(void)
>> +{
>> + int rv;
>> +
>> + rv = platform_driver_register(&octeon_i2c_driver);
>> + printk(KERN_INFO "driver %s is loaded\n", DRV_NAME);
>> + return rv;
>> +}
>
> I'd rather not see 'driver loaded' messages if possible.
Whats about adjusting your message filter? ;-)
Would KERN_DEBUG be more appropriate?
We love such messages in our projects. You can filter for the 'loaded
phrases and have an fast overview this way.
--
Kind Regards,
Michael
