| Message ID | f1ff3f2888725a1e03541e0be5c0880f0591aea2.1302031487.git.bengardiner@nanometrics.ca (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Ben, On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote: > Both the da850 and da830 have an I2C controller which has the ICPFUNC > registers. Indicate this for all da830 and da850 boards by setting the > has_pfunc flag true in the da8xx utility setup routine for registering the > I2C controller > > Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> > Cc: Sekhar Nori <nsekhar@ti.com> > Cc: Ben Dooks <ben-linux@fluff.org> > > --- > arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c > index beda8a4..da01558 100644 > --- a/arch/arm/mach-davinci/devices-da8xx.c > +++ b/arch/arm/mach-davinci/devices-da8xx.c > @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance, > else > return -EINVAL; > > + /* > + * Both the DA850 and DA830 have an I2C controller which has the > + * ICPFUNC et. al. registers > + */ > + pdata->has_pfunc = 1; The I2C driver implements a default platform data so it should actually be legal for a DA8x board to pass NULL platform data. In that case this line will crash. You should either check for pdata to be NULL or just let each board choose whether it needs recovery (I think the better option). Thanks, Sekhar
Hi Sekhar, On Wed, Apr 13, 2011 at 11:10 AM, Nori, Sekhar <nsekhar@ti.com> wrote: > Hi Ben, > > On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote: >> Both the da850 and da830 have an I2C controller which has the ICPFUNC >> registers. Indicate this for all da830 and da850 boards by setting the >> has_pfunc flag true in the da8xx utility setup routine for registering the >> I2C controller >> >> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> >> Cc: Sekhar Nori <nsekhar@ti.com> >> Cc: Ben Dooks <ben-linux@fluff.org> >> >> --- >> arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++ >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c >> index beda8a4..da01558 100644 >> --- a/arch/arm/mach-davinci/devices-da8xx.c >> +++ b/arch/arm/mach-davinci/devices-da8xx.c >> @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance, >> else >> return -EINVAL; >> >> + /* >> + * Both the DA850 and DA830 have an I2C controller which has the >> + * ICPFUNC et. al. registers >> + */ >> + pdata->has_pfunc = 1; > > The I2C driver implements a default platform data > so it should actually be legal for a DA8x board to > pass NULL platform data. In that case this line > will crash. Good catch, thanks. > [...] You should either check for pdata to > be NULL or just let each board choose whether it > needs recovery (I think the better option). I understand "check for pdata to be NULL." If you think it is the better option I'd be happy to implement it but I don't understand how to implement "let each board choose whether it needs recovery." Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
On Wed, Apr 13, 2011 at 20:52:05, Ben Gardiner wrote: > >> --- a/arch/arm/mach-davinci/devices-da8xx.c > >> +++ b/arch/arm/mach-davinci/devices-da8xx.c > >> @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance, > >> else > >> return -EINVAL; > >> > >> + /* > >> + * Both the DA850 and DA830 have an I2C controller which has the > >> + * ICPFUNC et. al. registers > >> + */ > >> + pdata->has_pfunc = 1; > > > > The I2C driver implements a default platform data > > so it should actually be legal for a DA8x board to > > pass NULL platform data. In that case this line > > will crash. > > Good catch, thanks. > > > [...] You should either check for pdata to > > be NULL or just let each board choose whether it > > needs recovery (I think the better option). > > I understand "check for pdata to be NULL." If you think it is the > better option I'd be happy to implement it but I don't understand how > to implement "let each board choose whether it needs recovery." This is done by just setting has_pfunc = 1 in the davinci_i2c_platform_data defined in the board file. Thanks, Sekhar
On Wed, Apr 13, 2011 at 11:42 AM, Nori, Sekhar <nsekhar@ti.com> wrote: > On Wed, Apr 13, 2011 at 20:52:05, Ben Gardiner wrote: >> I understand "check for pdata to be NULL." If you think it is the >> better option I'd be happy to implement it but I don't understand how >> to implement "let each board choose whether it needs recovery." > > This is done by just setting has_pfunc = 1 in the > davinci_i2c_platform_data defined in the board file. Ok -- now I get it, thanks. I spin a V2 shortly. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
On Wed, Apr 13, 2011 at 11:10 AM, Nori, Sekhar <nsekhar@ti.com> wrote: > > Hi Ben, > > On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote: > > Both the da850 and da830 have an I2C controller which has the ICPFUNC > > registers. Indicate this for all da830 and da850 boards by setting the > > has_pfunc flag true in the da8xx utility setup routine for registering the > > I2C controller > > > > Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> > > Cc: Sekhar Nori <nsekhar@ti.com> > > Cc: Ben Dooks <ben-linux@fluff.org> > > > > --- > > arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++ > > 1 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c > > index beda8a4..da01558 100644 > > --- a/arch/arm/mach-davinci/devices-da8xx.c > > +++ b/arch/arm/mach-davinci/devices-da8xx.c > > @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance, > > else > > return -EINVAL; > > > > + /* > > + * Both the DA850 and DA830 have an I2C controller which has the > > + * ICPFUNC et. al. registers > > + */ > > + pdata->has_pfunc = 1; > > The I2C driver implements a default platform data > so it should actually be legal for a DA8x board to > pass NULL platform data. In that case this line > will crash. You should either check for pdata to > be NULL or just let each board choose whether it > needs recovery (I think the better option). > I actually had a problem with using NULL for pdata with davinci I2C and had submitted a patch here that sort of fell on the floor. The problem was that the i2c_davinci_calc_clk_dividers is using platform_data without a check as described above. So at the moment using NULL doesn't really work, best as I can tell... https://lkml.org/lkml/2010/9/4/119 FWIW. -Mike
Hi Mike, On Thu, Apr 14, 2011 at 04:34:34, Mike Williamson wrote: > > The I2C driver implements a default platform data > > so it should actually be legal for a DA8x board to > > pass NULL platform data. In that case this line > > will crash. You should either check for pdata to > > be NULL or just let each board choose whether it > > needs recovery (I think the better option). > > > > I actually had a problem with using NULL for pdata with davinci I2C > and had submitted a patch here that sort of fell on the floor. The > problem was that the i2c_davinci_calc_clk_dividers is using > platform_data without a check as described above. So at the moment > using NULL doesn't really work, best as I can tell... > > https://lkml.org/lkml/2010/9/4/119 Looking at this mail chain, it looks like Ben was actually OK with your first submission and just wanted you to update the patch description saying that dev->platform_data is accessed later in the code and that is why it needs to be updated. I liked your first attempt better and may be you should try another submission with the patch description updated. Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c index beda8a4..da01558 100644 --- a/arch/arm/mach-davinci/devices-da8xx.c +++ b/arch/arm/mach-davinci/devices-da8xx.c @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance, else return -EINVAL; + /* + * Both the DA850 and DA830 have an I2C controller which has the + * ICPFUNC et. al. registers + */ + pdata->has_pfunc = 1; + pdev->dev.platform_data = pdata; return platform_device_register(pdev); }
Both the da850 and da830 have an I2C controller which has the ICPFUNC registers. Indicate this for all da830 and da850 boards by setting the has_pfunc flag true in the da8xx utility setup routine for registering the I2C controller Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> Cc: Sekhar Nori <nsekhar@ti.com> Cc: Ben Dooks <ben-linux@fluff.org> --- arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)