| Message ID | 1289841916-3825-2-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Grant Likely |
| Headers | show |
On Mon, Nov 15, 2010 at 06:25:16PM +0100, Wolfram Sang wrote: > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > --- > arch/powerpc/boot/dts/pcm030.dts | 1 + > arch/powerpc/boot/dts/pcm032.dts | 3 ++- > 2 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts > index 8a4ec30..e7c36bc 100644 > --- a/arch/powerpc/boot/dts/pcm030.dts > +++ b/arch/powerpc/boot/dts/pcm030.dts > @@ -259,6 +259,7 @@ > eeprom@52 { > compatible = "catalyst,24c32"; > reg = <0x52>; > + pagesize = <32>; I think you'd better drop the pagesize property altogether, and instead make the compatible string more specific (if needed at all. are there any 'catalyst,24c32' chips with pagesize != 32?) Thanks,
On 11/15/2010 7:32 AM, Anton Vorontsov wrote: > On Mon, Nov 15, 2010 at 06:25:16PM +0100, Wolfram Sang wrote: >> Signed-off-by: Wolfram Sang<w.sang@pengutronix.de> >> --- >> arch/powerpc/boot/dts/pcm030.dts | 1 + >> arch/powerpc/boot/dts/pcm032.dts | 3 ++- >> 2 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts >> index 8a4ec30..e7c36bc 100644 >> --- a/arch/powerpc/boot/dts/pcm030.dts >> +++ b/arch/powerpc/boot/dts/pcm030.dts >> @@ -259,6 +259,7 @@ >> eeprom@52 { >> compatible = "catalyst,24c32"; >> reg =<0x52>; >> + pagesize =<32>; > > I think you'd better drop the pagesize property altogether, and > instead make the compatible string more specific (if needed at > all. are there any 'catalyst,24c32' chips with pagesize != 32?) Microchip makes a 24c32 part that looks pretty similar to the catalyst part, but Microchip's has a 64-byte page size compared to Catalyst's 32. It would probably be feasible to have a generic I2C EEPROM driver that could handle many different parts, parameterized by total size, block size, and page size. > > Thanks, >
On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote: [...] > >> eeprom@52 { > >> compatible = "catalyst,24c32"; > >> reg =<0x52>; > >>+ pagesize =<32>; > > > >I think you'd better drop the pagesize property altogether, and > >instead make the compatible string more specific (if needed at > >all. are there any 'catalyst,24c32' chips with pagesize != 32?) > > Microchip makes a 24c32 part that looks pretty similar to the > catalyst part, but Microchip's has a 64-byte page size compared to > Catalyst's 32. Well, when using microchip part, the compatible string would be "microchip,24c32", correct? Then we have all the information already, no need for the pagesize. > It would probably be feasible to have a generic I2C EEPROM driver > that could handle many different parts, parameterized by total size, > block size, and page size. I guess it can do this already via I2C ID table. The problem is that I2C core is only using part ID (no vendor ID matching). So, the current driver may just implement quirks like this: if (of_device_is_compatible(np, "catalyst,24c32")) pagesize = 32; Or, if it's some generic pattern, something like if (of_device_is_compatible_vendor(np, "catalyst")) pagesize /= 2; Thanks,
On Mon, Nov 15, 2010 at 2:24 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote: > [...] >> >> eeprom@52 { >> >> compatible = "catalyst,24c32"; >> >> reg =<0x52>; >> >>+ pagesize =<32>; >> > >> >I think you'd better drop the pagesize property altogether, and >> >instead make the compatible string more specific (if needed at >> >all. are there any 'catalyst,24c32' chips with pagesize != 32?) >> >> Microchip makes a 24c32 part that looks pretty similar to the >> catalyst part, but Microchip's has a 64-byte page size compared to >> Catalyst's 32. > > Well, when using microchip part, the compatible string would be > "microchip,24c32", correct? Then we have all the information > already, no need for the pagesize. > >> It would probably be feasible to have a generic I2C EEPROM driver >> that could handle many different parts, parameterized by total size, >> block size, and page size. The current at24.c driver is already parameterized; but part of the parameter data is statically linked into the board support code. > > I guess it can do this already via I2C ID table. The problem > is that I2C core is only using part ID (no vendor ID matching). This could potentially be changed for at24 devices since the i2c subsystem already matches by name. It would mean adding the vendor prefix to all instantiations of the device in the kernel, although it would mess up the current heuristic used by of_modalias_node() (not a show-stopper). > > So, the current driver may just implement quirks like this: > > if (of_device_is_compatible(np, "catalyst,24c32")) > pagesize = 32; > > Or, if it's some generic pattern, something like > > if (of_device_is_compatible_vendor(np, "catalyst")) > pagesize /= 2; This would get ugly in a hurry. It would be better to make it data driven by searching for a better match in an of_device_id table so that the workarounds don't grow over time and eventually achieve sentience. g.
> > >I think you'd better drop the pagesize property altogether, and > > >instead make the compatible string more specific (if needed at > > >all. are there any 'catalyst,24c32' chips with pagesize != 32?) > > > > Microchip makes a 24c32 part that looks pretty similar to the > > catalyst part, but Microchip's has a 64-byte page size compared to > > Catalyst's 32. > > Well, when using microchip part, the compatible string would be > "microchip,24c32", correct? Then we have all the information > already, no need for the pagesize. Hmm, there are myriads of I2C eeproms out there, this table would be enourmous. Even worse, I seem to recall that I had once seen a manufacturer increasing the page-size from one charge to the next without changing the part-number, so I got this feeling "you can't map pagesize to manufacturer/type" which I still have. Sadly, this was long ago, so I can't proof it right now. Will try to dig up some datasheets when in the office tomorrow. In general, I2C EEPROMs are really a mess, the basic access method is the same, but except that everything else is possible :) Thus, this approach. Thus, this approach. Regards, Wolfram
In general I think it's better to report parameter values directly, instead of inferring them from manufacturer and part numbers. That way you at least have a fighting chance of avoiding a kernel upgrade when a part changes. Of course, that only works when the device tree is exported from the boot firmware instead of having to carry the device tree inside the kernel.
On Mon, Nov 15, 2010 at 12:17:51PM -1000, Mitch Bradley wrote: > In general I think it's better to report parameter values directly, > instead of inferring them from manufacturer and part numbers. That > way you at least have a fighting chance of avoiding a kernel upgrade > when a part changes. I tend to agree. Although a fallback to deducing from the manufacturer / part id if the pagesize property is missing would be sensible.
> Even worse, I seem to recall that I had once seen a manufacturer increasing the > page-size from one charge to the next without changing the part-number, so I > got this feeling "you can't map pagesize to manufacturer/type" which I still > have. Sadly, this was long ago, so I can't proof it right now. Will try to dig > up some datasheets when in the office tomorrow. Had a look, couldn't find anything :( And now? Regards, Wolfram
On Tue, Nov 16, 2010 at 10:45:37PM +0100, Wolfram Sang wrote: > > > Even worse, I seem to recall that I had once seen a manufacturer increasing the > > page-size from one charge to the next without changing the part-number, so I > > got this feeling "you can't map pagesize to manufacturer/type" which I still > > have. Sadly, this was long ago, so I can't proof it right now. Will try to dig > > up some datasheets when in the office tomorrow. > > Had a look, couldn't find anything :( And now? Well, it seems that there are enough people in "pagesize" camp anyway, I'd say just go ahead with the current approach. :-) It's an additional information, so won't do any harm anyway... Thanks,
diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts index 8a4ec30..e7c36bc 100644 --- a/arch/powerpc/boot/dts/pcm030.dts +++ b/arch/powerpc/boot/dts/pcm030.dts @@ -259,6 +259,7 @@ eeprom@52 { compatible = "catalyst,24c32"; reg = <0x52>; + pagesize = <32>; }; }; diff --git a/arch/powerpc/boot/dts/pcm032.dts b/arch/powerpc/boot/dts/pcm032.dts index 85d857a..e175e2c 100644 --- a/arch/powerpc/boot/dts/pcm032.dts +++ b/arch/powerpc/boot/dts/pcm032.dts @@ -257,8 +257,9 @@ reg = <0x51>; }; eeprom@52 { - compatible = "at24,24c32"; + compatible = "catalyst,24c32"; reg = <0x52>; + pagesize = <32>; }; };
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> --- arch/powerpc/boot/dts/pcm030.dts | 1 + arch/powerpc/boot/dts/pcm032.dts | 3 ++- 2 files changed, 3 insertions(+), 1 deletions(-)