Patchwork [2/2] powerpc: pcm030/032: add pagesize to dts

login
register
mail settings
Submitter Wolfram Sang
Date Nov. 15, 2010, 5:25 p.m.
Message ID <1289841916-3825-2-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/71256/
State Superseded
Delegated to: Grant Likely
Headers show

Comments

Wolfram Sang - Nov. 15, 2010, 5:25 p.m.
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(-)
Anton Vorontsov - Nov. 15, 2010, 5:32 p.m.
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,
Mitch Bradley - Nov. 15, 2010, 9:06 p.m.
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,
>
Anton Vorontsov - Nov. 15, 2010, 9:24 p.m.
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,
Grant Likely - Nov. 15, 2010, 9:58 p.m.
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.
Wolfram Sang - Nov. 15, 2010, 10:11 p.m.
> > >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
Mitch Bradley - Nov. 15, 2010, 10:17 p.m.
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.
David Gibson - Nov. 15, 2010, 10:30 p.m.
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.
Wolfram Sang - Nov. 16, 2010, 9:45 p.m.
> 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
Anton Vorontsov - Nov. 16, 2010, 10:03 p.m.
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,

Patch

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>;
 			};
 		};