[i2c] [PATCH] I2C bus driver for Philips ARM boards

[Jean Delvare]

Vitaly,

> > Which is OK if it is the last message, but not if there are more
> > messages to follow. Is the technical documentation for this chip
> > available? Please see if it mentions repeated start. Maybe you need to
> > set both start_bit and stop_bit to generate a repeated start condition.
> >
> > What testing did this driver receive so far?
>
> It's used for audio volume control, power management chip (PNX010x) and 
> USB configuration (PNX4008).
> It was also tested with simple EEPROM on both.

OK, EEPROMs will behave OK if stop + start are sent instead of repeated
start (at least on single master busses), so maybe the problem exists
and was never spotted. This needs to be investigated.

> >>>> +	i2c_pnx->suspend = plat_data->suspend;
> >>>> +	i2c_pnx->resume = plat_data->resume;
> >>>> +	i2c_pnx->calculate_input_freq = plat_data->calculate_input_freq;
> >>>> +	i2c_pnx->set_clock_run = plat_data->set_clock_run;
> >>>> +	i2c_pnx->set_clock_stop = plat_data->set_clock_stop;
> >>>>         
> >>> Why don't you simply store a pointer to the platform data, rather than
> >>> duplicating every entry?
> >>>       
> >> Hmmm, isn't it released after bootup?
> >>     
> >
> > Only if marked __init, and this isn't the case here.
> >   
> Ah, ok, I missed __initdata in there. I'd rather set it in 
> arch/arm/mach-pnx4008/i2c.c as it seems to be completely wrong to rely 
> on  the assumption that no other pnx target's i2c arch-specific init 
> won't do that.

No! You're looking at it by the wrong end. Code your driver properly,
so as to minimize the amount of data which needs to be copied. Then
other arch-specific coders will have to code properly on their side,
too. Anticipating bad coding behavior encourages bad coding, so it is
not the way to go.

There is no reason to have one struct i2c_pnx_platform_data and one
struct i2c_pnx, when they have the same definition. Make it only one
structure, which is declared and (mostly) filled by the arch code. In
the probe function you only have to fill the adapter pointer, and you
are done.

> >>>> +	iounmap((void *)alg_data->ioaddr);
> >>>>         
> >>> Unnecessary cast?
> >>>       
> >> ioaddr is u32
> >>     
> > Ah, I missed that, good point. Now this raises another question, why is
> > it an u32 in the first place, rather than the void* I'd expect?
> >   
> It was done in order not to have reverse casts for I2C_REG_xxx macros.

You shouldn't need casts there either. ioread32 and iowrite32 expect
pointers, not integers, as their last parameter.

> > OK, but this driver can still be selected as a module. What if the USB
> > part is built-in, or also built as a module but loaded first? What about
> > other platforms which wouldn't need this early init?
> >   
> I don't care too much about wrong module loading sequence.
> As of other... Hmm, what about a) adding #ifndef  MODULE for 
> subsys_initcall; b) adding a config option; c) adding a cmdline parameter?

I just don't know, maybe we can postpone this to later and/or let more
experienced people suggest what should be done.

Thanks,
-- 
Jean Delvare