Introduce MEN Board Information EEPROM driver

drivers/misc/eeprom/men_eeprod.c line 49

Quoted 2 lines · Andreas Werner

drivers/misc/eeprom/men_eeprod.c new file +2 -0

+struct eeprom_data {
+	uint8_t eeprod_id;

Please use the "real" kernel types, "u8" here, and "u32" in other places you use uint32_t (those are userspace types, not kernel types, sorry.)

drivers/misc/eeprom/men_eeprod.c line 369

Quoted 6 lines

drivers/misc/eeprom/men_eeprod.c new file +6 -0

+static DEVICE_ATTR(eeprod_id, S_IRUGO, show_eeprod_id, NULL);
+static DEVICE_ATTR(revision, S_IRUGO, show_revision, NULL);
+static DEVICE_ATTR(serial, S_IRUGO, show_serialnr, NULL);
+static DEVICE_ATTR(hw_name, S_IRUGO, show_hw_name, NULL);
+static DEVICE_ATTR(prod_date, S_IRUGO, show_prod_date, NULL);
+static DEVICE_ATTR(rep_date, S_IRUGO, show_rep_date, NULL);

DEVICE_ATTR_RO() please.

Added driver to support the MEN Board Information EEPROM. The driver exports the production information as read only sysfs entries, as well as a user section which is read/write accessible.

Tested on PPC QorIQ and Intel Atom E680.

Tested-by Johannes Thumshirn <[email protected]>

Signed-off-by Andreas Werner <[email protected]>

drivers/misc/eeprom/men_eeprod.c new file +5 -0

+struct eeprom_data {
+	uint8_t eeprod_id;
+
+	uint8_t revision[3];
+	uint32_t serialnr;

* PGP Signed by an unknown key

Quoted 4 lines · Andreas Werner

Added driver to support the MEN Board Information EEPROM. The driver exports the production information as read only sysfs entries, as well as a user section which is read/write accessible.

Quoted 4 lines

Tested on PPC QorIQ and Intel Atom E680.

Tested-by Johannes Thumshirn <[email protected]>

Signed-off-by Andreas Werner <[email protected]>

I guess this is just a standard EEPROM with a well defined layout? Why don't you want to use the standard driver then and parse the thing in userspace?

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 9 lines

drivers/misc/eeprom/men_eeprod.c new file +9 -0

+struct eeprom_data {
+	uint8_t eeprod_id;
+
+	uint8_t revision[3];
+	uint32_t serialnr;
+	uint8_t board_model;
+	char hw_name[6];
+
+	uint8_t reserved;

drivers/misc/eeprom/men_eeprod.c line 62

Quoted 6 lines

drivers/misc/eeprom/men_eeprod.c new file +6 -0

+
+	__be16 prod_date;
+	__be16 rep_date;
+
+	uint8_t reserved2[4];
+};

And what if the compiler reorders?

* Unknown Key
* 0x14A029B6

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

* PGP Signed by an unknown key

Quoted 3 lines

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things may look different, though.

Quoted 3 lines

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the data. Or do a board specific hook which reads the data and prints it to the logfiles...

Quoted 4 lines

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 3 lines

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Quoted 2 lines

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

* Unknown Key
* 0x14A029B6

Quoted 10 lines · Wolfram Sang

* PGP Signed by an unknown key

Quoted 3 lines

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things may look different, though.

I also understand your point of view :-).
Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it.

Quoted 8 lines

Quoted 3 lines

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the data. Or do a board specific hook which reads the data and prints it to the logfiles...

Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 9 lines

Quoted 3 lines

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have.

Quoted 11 lines

Quoted 2 lines

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor.

Regards
Andy

Quoted 3 lines

* Unknown Key
* 0x14A029B6

Quoted 67 lines · Andreas Werner

Quoted 10 lines · Wolfram Sang

* PGP Signed by an unknown key

Quoted 3 lines

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things may look different, though.

I also understand your point of view :-).
Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it.

Quoted 8 lines

Quoted 3 lines

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the data. Or do a board specific hook which reads the data and prints it to the logfiles...

Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 9 lines

Quoted 3 lines

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have.

Quoted 11 lines

Quoted 2 lines

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor.

Quoted 3 lines

Regards
Andy

Greg what do you think about that driver as a Maintainer of the sysfs?

To we have other ways to get those kind of drivers in the mainline kernel?

Quoted 68 lines · Andreas Werner

Quoted 67 lines · Andreas Werner

Quoted 10 lines · Wolfram Sang

* PGP Signed by an unknown key

Quoted 3 lines

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things may look different, though.

I also understand your point of view :-).
Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it.

Quoted 8 lines

Quoted 3 lines

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the data. Or do a board specific hook which reads the data and prints it to the logfiles...

Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 9 lines

Quoted 3 lines

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have.

Quoted 11 lines

Quoted 2 lines

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor.

Quoted 5 lines

Quoted 3 lines

Regards
Andy

Greg what do you think about that driver as a Maintainer of the sysfs?

I maintain the sysfs core that drivers use, I don't dictate the policy that those drivers create and send to userspace, that is up to the maintainer of the subsystem. There are some basic rules of sysfs (one value per file), but other than that, please work with the maintainer to come up with an interface that will work for all types of this device, not just a one-off interface which does not scale at all, as Wolfram has pointed out.

Quoted 1 line

To we have other ways to get those kind of drivers in the mainline kernel?

Yes, work on a common interface that your driver can use, and it can be accepted. Why do you not want to do that?

thanks,

greg k-h

Quoted 75 lines · Greg KH

Quoted 68 lines · Andreas Werner

Quoted 67 lines · Andreas Werner

Quoted 10 lines · Wolfram Sang

* PGP Signed by an unknown key

Quoted 3 lines

I do not want to parse the things in userspace because this EEPROM data are related to the hardware and i want to give our customer the easiest way to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things may look different, though.

I also understand your point of view :-).
Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it.

Quoted 8 lines

Quoted 3 lines

The current state to read the eeprom data is, that customer needs to install a big environment where the tool is integrated to have access to those kind of simple data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the data. Or do a board specific hook which reads the data and prints it to the logfiles...

Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Consider how bloated the sysfs-ABI might get if every vendor who uses an eeprom wants to expose the data this way?

Quoted 9 lines

Quoted 3 lines

Yes and no. The possible sysfs entries gets bloated if every vendor will do it like this way, but normally there is just one Board EEPROM on the board, therefore only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have.

Quoted 11 lines

Quoted 2 lines

I mean its the same for every i2c device like a temperature sensor, I can also read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor.

Quoted 5 lines

Quoted 3 lines

Regards
Andy

Greg what do you think about that driver as a Maintainer of the sysfs?

Quoted 8 lines

I maintain the sysfs core that drivers use, I don't dictate the policy that those drivers create and send to userspace, that is up to the maintainer of the subsystem. There are some basic rules of sysfs (one value per file), but other than that, please work with the maintainer to come up with an interface that will work for all types of this device, not just a one-off interface which does not scale at all, as Wolfram has pointed out.

Ok.

Quoted 7 lines

Quoted 1 line

To we have other ways to get those kind of drivers in the mainline kernel?

Yes, work on a common interface that your driver can use, and it can be accepted. Why do you not want to do that?

thanks,

Quoted 1 line

greg k-h

I have never talked about that I do not want to do it. I just want to discuss it with you.

Right now we are at a point that i know that those kind of drivers can't be upstream and i will try to find a way of abstraction to get it upstream.

Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it.

Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want.

Quoted 5 lines

I am not talking about runtime here, I don't care about that. I am talking about the ABI we create and we have to maintain basically forever. And with vendor specific configuartion data I have doubts with that being stable.

Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have.

Quoted 8 lines

These is a HUGE difference. If I read tempX_input, I don't need to care if the sensor is I2C or SPI or whatever. The kernel abstracts that away. The files you create are for your I2C EEPROM only. Data gets "reformatted" and access gets hidden, but nothing is abstracted away. It would be different if we had a generic convention for "serial_id" or stuff like that. But as configuration data is highly specific I don't see this coming.

For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor.

Here it gets frustrating. It seems you have no idea what an OS is for, not even after I tried to describe it :(

* PGP Signed by an unknown key

Quoted 2 lines

Here it gets frustrating. It seems you have no idea what an OS is for, not even after I tried to describe it :(

Sorry, that might have been too strong. Still, we can't map any hardware which is out there 1:1 into userspace, we need abstraction.

If you want to help with this abstraction, this is more than appreciated. If you want to keep your driver, this will have to stay out-of-tree, I'm afraid.

Regards,

Wolfram

* Unknown Key
* 0x14A029B6

Quoted 9 lines

drivers/misc/eeprom/men_eeprod.c new file +9 -0

+struct eeprom_data {
+	uint8_t eeprod_id;
+
+	uint8_t revision[3];
+	uint32_t serialnr;
+	uint8_t board_model;
+	char hw_name[6];
+
+	uint8_t reserved;

drivers/misc/eeprom/men_eeprod.c line 62

Quoted 6 lines

drivers/misc/eeprom/men_eeprod.c new file +6 -0

+
+	__be16 prod_date;
+	__be16 rep_date;
+
+	uint8_t reserved2[4];
+};

And what if the compiler reorders?

Quoted 9 lines · Wolfram Sang

Quoted 9 lines

drivers/misc/eeprom/men_eeprod.c new file +9 -0

+struct eeprom_data {
+	uint8_t eeprod_id;
+
+	uint8_t revision[3];
+	uint32_t serialnr;
+	uint8_t board_model;
+	char hw_name[6];
+
+	uint8_t reserved;

Quoted 8 lines

drivers/misc/eeprom/men_eeprod.c line 62

Quoted 6 lines

drivers/misc/eeprom/men_eeprod.c new file +6 -0

+
+	__be16 prod_date;
+	__be16 rep_date;
+
+	uint8_t reserved2[4];
+};

And what if the compiler reorders?

It's not allowed to reorder, but it can add padding wherever it wants to, which if this is a on-device structure, can cause problems. Use __packed to prevent that.