| Message ID | d169930cfe45d4c05c79f7dd00471d732441529d.1413416105.git.andreas.werner@men.de |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote: > +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.) > +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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote: > 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 <johannes.thumshirn@men.de> > Signed-off-by: Andreas Werner <andreas.werner@men.de> 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? > +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; > + > + __be16 prod_date; > + __be16 rep_date; > + > + uint8_t reserved2[4]; > +}; And what if the compiler reorders?
On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote: > > +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; > > + > > + __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. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 11:34:14AM +0200, Greg KH wrote: > On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote: > > > +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; > > > + > > > + __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. Yes, took the wrong word, sorry, I meant __packed.
> 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. > 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... > > 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 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. > 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.
On Thu, Oct 16, 2014 at 10:44:12AM +0200, Greg KH wrote: > On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote: > > +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.) > You are right i will change it. > > +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. OK no problem, will change it. Thanks. Regards Andy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote: > * PGP Signed by an unknown key > > On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote: > > 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 <johannes.thumshirn@men.de> > > Signed-off-by: Andreas Werner <andreas.werner@men.de> > > 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? > Yes it is a standard 256byte eeprom. 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. If our support department get some questions from the customer they always want to have exact information about the board. Therefore I want to use this driver and sysfs to get the informations as fast as possible without installing anything. > 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. > > +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; > > + > > + __be16 prod_date; > > + __be16 rep_date; > > + > > + uint8_t reserved2[4]; > > +}; > > And what if the compiler reorders? Shit you are right, I will pack it.# > > > * Unknown Key > * 0x14A029B6 Regards Andy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > * PGP Signed by an unknown key > > > > 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. > > 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. > > > 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 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. > > 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 > > * Unknown Key > * 0x14A029B6 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. As I pointed out, there are ways to do it other than a seperate driver. > Yes of course there are a lot of possibilities. This was just an example > what we currently use and what was developed years ago. And please notice that this solution you chose is not acceptible for upstream. We can't have n copies of the at24 driver with just some minor differences. This is a maintenance nightmare. Yes, I know it was easiest to start with and it works, but that does not help here. > 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. i2cget won't modify any data. i2cset does, if anyone wants to do that. BTW it does that also after you removed your driver, so basically no big difference here. > > 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. Exactly, that was what I was saying. What we probably should have in at24 is regions which should not be exposed to userspace, because they contain config data. That would be nice. I'm not sure if we can add table based decoding to at24, that needs some experiments. But it will really need such experiments and some more effort. > > 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 :(
> 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
On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > * PGP Signed by an unknown key > > > > > > > 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. > > > > 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. > > > > > 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 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. > > > > 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 > 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? Regards Andy > > > > * Unknown Key > > * 0x14A029B6 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: > On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > > * PGP Signed by an unknown key > > > > > > > > > > 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. > > > > > > 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. > > > > > > > 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 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. > > > > > > 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 > > > > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 20, 2014 at 10:24:22AM +0200, Wolfram Sang wrote: > * PGP Signed by an unknown key > > > Here it gets frustrating. It seems you have no idea what an OS is for, > > not even after I tried to describe it :( > I am pretty sure that i know what an OS is for. > 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. > Yes, my goal is to find a good way to get hardware support upstream, and it is also nice to discuss my point of view with your upstream point of few. And yes, i want to help to find a way to develope an abstraction. Regards Andy > Regards, > > Wolfram > > > * Unknown Key > * 0x14A029B6 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote: > On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: > > On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > > > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > > > * PGP Signed by an unknown key > > > > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > > > > 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 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. > > > > > > > > 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 > > > > > > > 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. > Ok. > > 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 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. Thanks Andy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/20/14 13:09, Andreas Werner wrote: > On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote: >> On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: >>> On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: >>>> On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: >>>>> * PGP Signed by an unknown key >>>>> >>>>> >>>>>> 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. >>>> >>>>>> 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. >>>> >>>>>>> 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 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. >>>> >>>>>> 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 >>>> >>> >>> 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. >> > > Ok. > >>> 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 > > 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. IMO, at24 provides you a good enough abstraction which you parse from user space with a help of a really small utility or a shell script... That is a really small effort. If you want to take it further, may it is worth looking at how the DMI stuff is exported on x86 (except that you talk to eeprom directly)?
diff --git a/MAINTAINERS b/MAINTAINERS index 503da28..88ede76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6029,6 +6029,12 @@ F: drivers/leds/leds-menf21bmc.c F: drivers/hwmon/menf21bmc_hwmon.c F: Documentation/hwmon/menf21bmc +MEN EEPROD (Board information EEPROM) +M: Andreas Werner <andreas.werner@men.de> +S: Supported +F: drivers/misc/eeprom/men_eeprod.c +F: Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod + METAG ARCHITECTURE M: James Hogan <james.hogan@imgtec.com> L: linux-metag@vger.kernel.org diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig index 9536852f..e087d08 100644 --- a/drivers/misc/eeprom/Kconfig +++ b/drivers/misc/eeprom/Kconfig @@ -62,6 +62,16 @@ config EEPROM_MAX6875 This driver can also be built as a module. If so, the module will be called max6875. +config EEPROM_MEN_EEPROD + tristate "MEN Board Information EEPROM" + depends on I2C && SYSFS + help + If you say yes here you get support for the MEN Board Information + EEPROM. This driver supports read-only access to the production + data section, and read-write access to the user section. + + This driver can also be built as a module. If so, the module + will be called men_eeprod. config EEPROM_93CX6 tristate "EEPROM 93CX6 support" diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile index 9507aec..8c70a81 100644 --- a/drivers/misc/eeprom/Makefile +++ b/drivers/misc/eeprom/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_EEPROM_AT24) += at24.o obj-$(CONFIG_EEPROM_AT25) += at25.o obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o obj-$(CONFIG_EEPROM_MAX6875) += max6875.o +obj-$(CONFIG_EEPROM_MEN_EEPROD) += men_eeprod.o obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o diff --git a/drivers/misc/eeprom/men_eeprod.c b/drivers/misc/eeprom/men_eeprod.c new file mode 100644 index 0000000..28264df --- /dev/null +++ b/drivers/misc/eeprom/men_eeprod.c @@ -0,0 +1,560 @@ +/* + * men_eeprod - MEN board information EEPROM driver. + * + * This is the driver for the Board Information EEPROM on almost + * all of the MEN boards. + * The driver exports each of the predefined eeprom sections as sysfs entries + * including an entry for user data. + * + * The EEPROM can be normally found at I2C address 0x57. + * + * Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +/* + * Supports the following EEPROM layouts: + * + * Name eeprod_id user_section size + * ---------------------------------------------- + * EEPROD2 0x0E 232 byte + * + * See Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod + * for more details. + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/jiffies.h> +#include <linux/delay.h> +#include <linux/sysfs.h> + +#define EEPROM_ID_ADDR 0x00 +#define EEPROM_SIZE 256 +#define EEPROD2_ID 0x0E + +#define DATE_YEAR_BIAS 1990 + +/* + * Internal structure of the Board Information EEPROM + * production data section + */ +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; + + __be16 prod_date; + __be16 rep_date; + + uint8_t reserved2[4]; +}; + +struct men_eeprod_data { + struct bin_attribute user_section; + struct i2c_client *client; + struct eeprom_data eeprom; + struct mutex lock; + int smb_access; +}; + +static unsigned int i2c_timeout = 25; +module_param(i2c_timeout, uint, 0); +MODULE_PARM_DESC(i2c_timeout, "Time (in ms) to try reads and writes (default 25)"); + +#define OFFSET_TO_USR_SECTION(off) (off + sizeof(struct eeprom_data)) + +static inline int eeprod_get_day(__be16 eeprod_date) +{ + return be16_to_cpu(eeprod_date) & 0x001f; +} + +static inline int eeprod_get_month(__be16 eeprod_date) +{ + return (be16_to_cpu(eeprod_date) >> 5) & 0x000f; +} + +static inline int eeprod_get_year(__be16 eeprod_date) +{ + return ((be16_to_cpu(eeprod_date) >> 9) & 0x007f) + DATE_YEAR_BIAS; +} + +static ssize_t men_eeprom_read(struct men_eeprod_data *drv_data, + char *buf, loff_t offset, size_t count) +{ + struct i2c_client *i2c_client = drv_data->client; + unsigned long timeout, read_time; + int ret_val; + + /* + * Read fail if the previous write did not copmlete yet. + * Therefore we try to read a few times until this succeed. + */ + timeout = jiffies + msecs_to_jiffies(i2c_timeout); + do { + read_time = jiffies; + + /* + * if there is just one byte requested, we use read byte data. + * This will also protect us against a rollover if there is + * just one byte left to read. + */ + if (count == 1) { + ret_val = i2c_smbus_read_byte_data(i2c_client, offset); + if (ret_val >= 0) { + buf[0] = ret_val; + ret_val = 1; + } + goto err_byte; + } + + switch (drv_data->smb_access) { + case I2C_SMBUS_I2C_BLOCK_DATA: + if (count > I2C_SMBUS_BLOCK_MAX) + count = I2C_SMBUS_BLOCK_MAX; + + ret_val = i2c_smbus_read_i2c_block_data(i2c_client, + offset, count, + buf); + if (ret_val >= 0) + return count; + break; + case I2C_SMBUS_WORD_DATA: + ret_val = i2c_smbus_read_word_data(i2c_client, offset); + if (ret_val >= 0) { + buf[0] = ret_val & 0xff; + buf[1] = ret_val >> 8; + return 2; + } + break; + default: + ret_val = i2c_smbus_read_byte_data(i2c_client, offset); + if (ret_val >= 0) { + buf[0] = ret_val; + return 1; + } + break; + } + +err_byte: + usleep_range(600, 1000); + } while (time_before(read_time, timeout)); + + return -ETIMEDOUT; + +} + +static ssize_t men_user_section_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + char *buf, loff_t off, size_t count) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + int user_section_size = attr->size; + ssize_t retval = 0; + int bytes_read; + + if (off > user_section_size) + return 0; + + if (off + count > user_section_size) + count = user_section_size - off; + + off = OFFSET_TO_USR_SECTION(off); + + mutex_lock(&drv_data->lock); + while (count) { + bytes_read = men_eeprom_read(drv_data, buf, off, count); + + if (bytes_read <= 0) { + if (retval == 0) + retval = bytes_read; + break; + } + + buf += bytes_read; + off += bytes_read; + count -= bytes_read; + retval += bytes_read; + } + mutex_unlock(&drv_data->lock); + + return retval; +} + +static ssize_t men_eeprom_write(struct men_eeprod_data *drv_data, + char *buf, loff_t offset, size_t count) +{ + struct i2c_client *i2c_client = drv_data->client; + unsigned long timeout, write_time; + uint16_t word_data; + int ret_val; + + /* + * Write fail if the previous write did not copmlete yet. + * Therefore we try to write a few times until this succeed. + */ + timeout = jiffies + msecs_to_jiffies(i2c_timeout); + do { + write_time = jiffies; + + /* + * if there is just one byte to write, we use write byte data. + * This will also protect us against a rollover if there is + * just one byte left to write. + */ + if (count == 1) { + ret_val = i2c_smbus_write_byte_data(i2c_client, offset, + buf[0]); + if (!ret_val) + return 1; + goto err_byte; + } + + switch (drv_data->smb_access) { + case I2C_SMBUS_I2C_BLOCK_DATA: + if (count > I2C_SMBUS_BLOCK_MAX) + count = I2C_SMBUS_BLOCK_MAX; + + ret_val = i2c_smbus_write_i2c_block_data(i2c_client, + offset, count, + buf); + if (!ret_val) + return count; + break; + case I2C_SMBUS_WORD_DATA: + word_data = buf[0] | (buf[1] << 8); + + ret_val = i2c_smbus_write_word_data(i2c_client, offset, + word_data); + if (!ret_val) + return 2; + break; + default: + ret_val = i2c_smbus_write_byte_data(i2c_client, offset, + buf[0]); + if (!ret_val) + return 1; + break; + } +err_byte: + usleep_range(600, 1000); + } while (time_before(write_time, timeout)); + + return -ETIMEDOUT; +} + +static ssize_t men_user_section_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, char *buf, + loff_t off, size_t count) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + int user_section_size = attr->size; + int bytes_written; + ssize_t retval = 0; + + if (off > user_section_size) + return 0; + + if (off + count > user_section_size) + count = user_section_size - off; + + off = OFFSET_TO_USR_SECTION(off); + + mutex_lock(&drv_data->lock); + while (count) { + bytes_written = men_eeprom_write(drv_data, buf, off, count); + + if (bytes_written <= 0) { + if (retval == 0) + retval = bytes_written; + break; + } + + buf += bytes_written; + off += bytes_written; + count -= bytes_written; + retval += bytes_written; + } + mutex_unlock(&drv_data->lock); + + return retval; +} + +static ssize_t +show_eeprod_id(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + struct eeprom_data *eeprom = &drv_data->eeprom; + + return sprintf(buf, "0x%02x\n", eeprom->eeprod_id); +} + +static ssize_t +show_revision(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + struct eeprom_data *eeprom = &drv_data->eeprom; + + return sprintf(buf, "%02d.%02d.%02d\n", eeprom->revision[0], + eeprom->revision[1], eeprom->revision[2]); +} + +static ssize_t +show_serialnr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + struct eeprom_data *eeprom = &drv_data->eeprom; + + return sprintf(buf, "%d\n", cpu_to_be32(eeprom->serialnr)); +} + +static ssize_t +show_hw_name(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + struct eeprom_data *eeprom = &drv_data->eeprom; + + return sprintf(buf, "%s%02d\n", eeprom->hw_name, eeprom->board_model); +} + +static ssize_t +show_prod_date(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + __be16 eeprod_date = drv_data->eeprom.prod_date; + + return sprintf(buf, "%04d-%02d-%02d\n", + eeprod_get_year(eeprod_date), + eeprod_get_month(eeprod_date), + eeprod_get_day(eeprod_date)); +} + +static ssize_t +show_rep_date(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct men_eeprod_data *drv_data = dev_get_drvdata(dev); + __be16 eeprod_date = drv_data->eeprom.rep_date; + + /* + * could be empty if the board was never send back + * to the repair department. + */ + if (eeprod_date == cpu_to_be16(0xffff)) + return -EINVAL; + + return sprintf(buf, "%04d-%02d-%02d\n", + eeprod_get_year(eeprod_date), + eeprod_get_month(eeprod_date), + eeprod_get_day(eeprod_date)); +} + +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); + +static struct attribute *eeprod_attrs[] = { + &dev_attr_eeprod_id.attr, + &dev_attr_revision.attr, + &dev_attr_serial.attr, + &dev_attr_hw_name.attr, + &dev_attr_prod_date.attr, + &dev_attr_rep_date.attr, + NULL, +}; + +static struct attribute_group eeprod_attr_group = { + .attrs = eeprod_attrs, +}; + +static struct bin_attribute eeprom_user_attr = { + .attr = { + .name = "user_section", + .mode = S_IRUGO | S_IWUSR, + }, + .size = EEPROM_SIZE - sizeof(struct eeprom_data), + .read = men_user_section_read, + .write = men_user_section_write, +}; + +static int men_eeprod_read_prod_data(struct men_eeprod_data *drv_data) +{ + struct eeprom_data *eeprom = &drv_data->eeprom; + uint8_t *eeprom_byte; + int i, ret; + + eeprom_byte = (uint8_t *)eeprom + 1; + + for (i = 1; i < sizeof(*eeprom); i++) { + ret = i2c_smbus_read_byte_data(drv_data->client, i); + if (ret < 0) + return ret; + + *(eeprom_byte++) = ret; + } + return 0; +} + +static int men_eeprod_calc_parity(struct eeprom_data *eeprom) +{ + uint8_t *eeprom_byte; + int parity, len; + + eeprom_byte = (uint8_t *)eeprom + 1; + len = sizeof(*eeprom) - 1; + parity = 0x0f; + + while (len--) { + parity ^= (*eeprom_byte >> 4); + parity ^= (*eeprom_byte) & 0x0f; + eeprom_byte++; + } + + return parity; +} + +static int men_eeprod_i2c_functionality(struct men_eeprod_data *drv_data) +{ + struct i2c_adapter *i2c_adapter = drv_data->client->adapter; + int ret; + + /* + * As the minimum we need read/write byte data + * which every adapter should support + */ + ret = i2c_check_functionality(i2c_adapter, + I2C_FUNC_SMBUS_BYTE_DATA); + if (!ret) + return -ENODEV; + + if (i2c_check_functionality(i2c_adapter, + I2C_FUNC_SMBUS_I2C_BLOCK)) { + drv_data->smb_access = I2C_SMBUS_I2C_BLOCK_DATA; + } else if (i2c_check_functionality(i2c_adapter, + I2C_FUNC_SMBUS_WORD_DATA)) { + drv_data->smb_access = I2C_SMBUS_WORD_DATA; + } else { + drv_data->smb_access = I2C_SMBUS_BYTE_DATA; + } + + return 0; +} + +static int +men_eeprod_probe(struct i2c_client *client, const struct i2c_device_id *id) +{ + struct men_eeprod_data *drv_data; + int eeprod_id, eeprod_chksum; + int parity, ret; + + drv_data = devm_kzalloc(&client->dev, sizeof(*drv_data), GFP_KERNEL); + if (!drv_data) + return -ENOMEM; + + drv_data->client = client; + mutex_init(&drv_data->lock); + + i2c_set_clientdata(client, drv_data); + + ret = men_eeprod_i2c_functionality(drv_data); + if (ret) + return ret; + + eeprod_id = i2c_smbus_read_byte_data(client, EEPROM_ID_ADDR); + if (eeprod_id < 0) + return eeprod_id; + + eeprod_chksum = eeprod_id & 0x0f; + eeprod_id >>= 4; + drv_data->eeprom.eeprod_id = eeprod_id; + + if (eeprod_id == EEPROD2_ID) { + dev_info(&client->dev, + "found MEN Board EEPROM. ID: 0x%02x\n", eeprod_id); + } else { + dev_err(&client->dev, + "board eeprom not supported. ID: 0x%02x\n", eeprod_id); + return -ENXIO; + } + + ret = men_eeprod_read_prod_data(drv_data); + if (ret < 0) { + dev_err(&client->dev, "failed to read EEPROM board data\n"); + return ret; + } + + parity = men_eeprod_calc_parity(&drv_data->eeprom); + if (parity != eeprod_chksum) { + dev_err(&client->dev, "checksum error. eeprom in invalid state\n"); + return -EINVAL; + } + + drv_data->user_section = eeprom_user_attr; + ret = sysfs_create_bin_file(&client->dev.kobj, + &drv_data->user_section); + if (ret) { + dev_err(&client->dev, + "failed to create user_section sysfs entry\n"); + return ret; + } + + ret = sysfs_create_group(&client->dev.kobj, &eeprod_attr_group); + if (ret < 0) { + dev_err(&client->dev, "failed to create sysfs entries\n"); + goto err_sysfs; + } + + dev_info(&client->dev, "MEN Board Information EEPROM registered\n"); + + return 0; + +err_sysfs: + sysfs_remove_bin_file(&client->dev.kobj, &drv_data->user_section); + return ret; +} + +static int men_eeprod_remove(struct i2c_client *client) +{ + struct men_eeprod_data *drv_data = i2c_get_clientdata(client); + + sysfs_remove_group(&client->dev.kobj, &eeprod_attr_group); + sysfs_remove_bin_file(&client->dev.kobj, &drv_data->user_section); + return 0; +} + +static const struct i2c_device_id men_eeprod_ids[] = { + { "men_eeprod" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, men_eeprod_ids); + +static struct i2c_driver men_eeprod_driver = { + .driver = { + .name = "men_eeprod", + .owner = THIS_MODULE, + }, + .probe = men_eeprod_probe, + .remove = men_eeprod_remove, + .id_table = men_eeprod_ids, +}; + +module_i2c_driver(men_eeprod_driver); + +MODULE_DESCRIPTION("MEN Board Information EEPROM driver"); +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>"); +MODULE_LICENSE("GPL v2");