> -----Original Message----- > From: Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx> > Sent: Tuesday, April 30, 2019 8:59 PM > To: minyard@xxxxxxx; wsa@xxxxxxxxxxxxx; Vadim Pasternak > <vadimp@xxxxxxxxxxxx>; Michael Shych <michaelsh@xxxxxxxxxxxx> > Cc: Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > linux-i2c@xxxxxxxxxxxxxxx > Subject: [PATCH v4 0/1] Add support for IPMB driver > > Thank you for your feedback Vadim. I have addressed your comments. Hi Asmaa, Thank you for your comments and added doc. > > 1) You are correct. This driver is not specific to Mellanox so I have removed the > Mellanox attribute. > > 2) I have added a documentation file called IPMB.txt which explains how this > module works and how it should be instantiated. It is very similar to the existing > linux i2c-slave-eeprom module. > > The HW for my testing works as follows: > A BMC is connected to a Satellite MC via I2C (I2C is equivalent to IPMB). The > BMC initiates the IPMB requests and sends them via I2C. > Obviously the BMC needs its own driver to do this which I haven't included in this > code. We have no intent of upstreaming that at the moment. I believe you are going to do it at some point, right? > This ipmb-dev-int driver is to be loaded and instantiated on the Satellite MC to > be able to receive IPMB requests. These IPMB request messages will be picked > up by a user space program such (in my case it is OpenIPMI) to handle the > request and generate a response. > The response will be then passed from the user program back to kernel space. > Then this driver would send that response back to the BMC. > > 3) You asked the following: > > "Is it expected to be zero in vaid case?" > The 8 least significant bits of the sum is always expected to be 0 in the case > where the checksum is valid. I have added a comment for clarifications. > > "why do you need this cast?" > buf[++ipmb_dev_p->msg_idx]=(u8)(client->addr<<1) > This is because client->addr is of type unsigned short which is > 2 bytes so it is safer to typecast it to u8 (u8* buf) Better, if you can avoid cast. Would compiler warn if you use for example rol16(client->addr, 1) & GENMASK(7, 0); or something like it? > > "It could be only single ipmb-dev within the system? Couldn't it be few, like > master/slave for example?" > My understanding of your question is that: what if we have multiple instances of > ipmb-dev-int, that we register it under different addresses? > This driver only works as a slave so it will only be instantiated once on the > Satellite MC under one slave address. I mentioned some config like: BMC1 (master) -- busA --| Satellite BMC2 (standby) -- busB --| Since this is not Mellanox specific driver, maybe it would be good to support multiple instances of it. > > Asmaa Mnebhi (1): > Add support for IPMB driver > > Documentation/IPMB.txt | 53 ++++++ > drivers/char/ipmi/Kconfig | 8 + > drivers/char/ipmi/Makefile | 1 + > drivers/char/ipmi/ipmb_dev_int.c | 381 > +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 443 insertions(+) > create mode 100644 Documentation/IPMB.txt create mode 100644 > drivers/char/ipmi/ipmb_dev_int.c > > -- > 2.1.2
