| Message ID | 1527714464-8642-1-git-send-email-eajames@linux.vnet.ibm.com |
|---|---|
| Headers | show |
| Series | i2c: Add FSI-attached I2C master algorithm | expand |
On Thu, May 31, 2018 at 12:07 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote: > This series adds an algorithm for an I2C master physically located on an FSI > slave device. The I2C master has multiple ports, each of which may be connected > to an I2C slave. Access to the I2C master registers is achieved over FSI bus. > > Due to the multi-port nature of the I2C master, the driver instantiates a new > I2C adapter for each port connected to a slave. The connected ports should be > defined in the device tree under the I2C master device. I'll comment the series later, though you have to address previous comments first: - understand devm_ purpose and how it works - understand list_for_each*() macros - discuss with maintainer a design of enumerating ports - ... (whatever I forgot and others added) ... > Changes since v7 > - Fix grammer in Kconfig (a -> an) > - Change I2C registers to use BIT and GENMASK > - Remove custom macros and use FIELD_PREP and FIELD_GET > - Fix a few unecessary initializations and "return rc" that are always zero > - Clean up the read/write fifo functions a bit > - Few other clean-up items > > Changes since v6 > - Remove spinlock for reset functionality; it's unecessary and doesn't work > with the latest FSI core. > - Use a mutex instead of a semaphore, and don't wait for timeout to get the > lock. > - Use usleeps instead of schedule_timeout; it's not worth the overhead when > the wait should be very short in between sending the command and receiving > the response. > > Changes since v5 > - Fix reset functionality and do a reset after every transfer failure > > Eddie James (7): > dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation > i2c: Add FSI-attached I2C master algorithm > i2c: fsi: Add port structures > i2c: fsi: Add abort and hardware reset procedures > i2c: fsi: Add transfer implementation > i2c: fsi: Add I2C master locking > i2c: fsi: Add bus recovery > > Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 ++ > drivers/i2c/busses/Kconfig | 11 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-fsi.c | 722 ++++++++++++++++++++++ > 4 files changed, 774 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt > create mode 100644 drivers/i2c/busses/i2c-fsi.c > > -- > 1.8.3.1 >
On Thu, 2018-05-31 at 00:31 +0300, Andy Shevchenko wrote: > On Thu, May 31, 2018 at 12:07 AM, Eddie James > <eajames@linux.vnet.ibm.com> wrote: > > This series adds an algorithm for an I2C master physically located on an FSI > > slave device. The I2C master has multiple ports, each of which may be connected > > to an I2C slave. Access to the I2C master registers is achieved over FSI bus. > > > > Due to the multi-port nature of the I2C master, the driver instantiates a new > > I2C adapter for each port connected to a slave. The connected ports should be > > defined in the device tree under the I2C master device. > > I'll comment the series later, though you have to address previous > comments first: > - understand devm_ purpose and how it works I think it is perfectly understood and I don't see what your problem here is. So please be a proper civil human being an express your concern precisely rather than with aggressive comments. Now to clarify that specific point, devm purpose is to automatically clean up the resources used by the device when it is torn down. However, in this specific case, it makes sense to dispose of the port structure explicitly because this is a failure in registering an individual port which doesn't lead to a failure of the entire driver. Thus not freeing it means the structure would remain allocated uselessly until the whole driver is torn down. > - understand list_for_each*() macros This is a genuine misunderstanding of Eddies, I'm sure he will address it, your tone however isn't appropriate. > - discuss with maintainer a design of enumerating ports I've been at that game for at least a good 2 decades. Maintainers generally do *not* discuss design until a patch is proposed. I even still try every now and then, maintainers are like lawyers, they don't want to tell you what to do in case they still want to reject it after seeing it later :-) I know I've been one of them for long enough. If you have specific issues with how this is done, please express them clearly. It's quite possible that there's some better way to do what Eddie is doing here, but without *construtive* feedback this is pointless. > - ... (whatever I forgot and others added) ... I believe he addressed most of your comments. There is one mistake left (the list_for_each) which will be easily fixed. As for the rest, it's just ramblings unless you or Wolfram can propose a better alternative in which case I'm sure Eddie will be happy to implement it. I'm disappointed here because we have an example of somebody rather new producing what is overall pretty damn good code, despite a few corner issues, and being (again) treated like crap. This isn't the right way to operate, and I believe this has been made clear many times before. Cheers, Ben. > > Changes since v7 > > - Fix grammer in Kconfig (a -> an) > > - Change I2C registers to use BIT and GENMASK > > - Remove custom macros and use FIELD_PREP and FIELD_GET > > - Fix a few unecessary initializations and "return rc" that are always zero > > - Clean up the read/write fifo functions a bit > > - Few other clean-up items > > > > Changes since v6 > > - Remove spinlock for reset functionality; it's unecessary and doesn't work > > with the latest FSI core. > > - Use a mutex instead of a semaphore, and don't wait for timeout to get the > > lock. > > - Use usleeps instead of schedule_timeout; it's not worth the overhead when > > the wait should be very short in between sending the command and receiving > > the response. > > > > Changes since v5 > > - Fix reset functionality and do a reset after every transfer failure > > > > Eddie James (7): > > dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation > > i2c: Add FSI-attached I2C master algorithm > > i2c: fsi: Add port structures > > i2c: fsi: Add abort and hardware reset procedures > > i2c: fsi: Add transfer implementation > > i2c: fsi: Add I2C master locking > > i2c: fsi: Add bus recovery > > > > Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 ++ > > drivers/i2c/busses/Kconfig | 11 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-fsi.c | 722 ++++++++++++++++++++++ > > 4 files changed, 774 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt > > create mode 100644 drivers/i2c/busses/i2c-fsi.c > > > > -- > > 1.8.3.1 > > > > >
On Thu, May 31, 2018 at 1:42 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Thu, 2018-05-31 at 00:31 +0300, Andy Shevchenko wrote: >> On Thu, May 31, 2018 at 12:07 AM, Eddie James >> <eajames@linux.vnet.ibm.com> wrote: >> I'll comment the series later, though you have to address previous >> comments first: >> - understand devm_ purpose and how it works > > I think it is perfectly understood and I don't see what your problem > here is. So please be a proper civil human being an express your > concern precisely rather than with aggressive comments. I apologize for this kind of tone, let's assume it was a bad day. > Now to clarify that specific point, devm purpose is to automatically > clean up the resources used by the device when it is torn down. > > However, in this specific case, it makes sense to dispose of the port > structure explicitly because this is a failure in registering an > individual port which doesn't lead to a failure of the entire driver. > > Thus not freeing it means the structure would remain allocated > uselessly until the whole driver is torn down. Yep, so, why do we care? If it holds few hundreds of bytes, can't we live with it? If no, the devm_k*alloc() is a wrong choice in the first place. >> - discuss with maintainer a design of enumerating ports > > I've been at that game for at least a good 2 decades. Maintainers > generally do *not* discuss design until a patch is proposed. I even > still try every now and then, maintainers are like lawyers, they don't > want to tell you what to do in case they still want to reject it after > seeing it later :-) I know I've been one of them for long enough. > > If you have specific issues with how this is done, please express them > clearly. It's quite possible that there's some better way to do what > Eddie is doing here, but without *construtive* feedback this is > pointless. It feels like you duplicate approach which is done in OF generic case. That is my concern. Though, if Wolfram is telling that is OK, I have no objections. > I'm disappointed here because we have an example of somebody rather new > producing what is overall pretty damn good code, That is true. His code much better than many I have seen before. > despite a few corner > issues, and being (again) treated like crap. Sorry for that, life is harsh. > This isn't the right way to operate, and I believe this has been made > clear many times before. Yes.
On Thu, 2018-05-31 at 09:29 +0300, Andy Shevchenko wrote: > > If you have specific issues with how this is done, please express them > > clearly. It's quite possible that there's some better way to do what > > Eddie is doing here, but without *construtive* feedback this is > > pointless. > > It feels like you duplicate approach which is done in OF generic case. > That is my concern. Though, if Wolfram is telling that is OK, I have > no objections. THe OF generic case is about discovering slaves underneath a port, not ports inside of a mulit-port master. I am not aware of a generic mechanism for the latter. We *could* make the ports sub-devices but it gets messy then to arbitrate the communication and deal with the common part. I've seen (and written) multi-port masters in the past that use a similar approach to what Eddie's doing and it works fine. > > I'm disappointed here because we have an example of somebody rather new > > producing what is overall pretty damn good code, > > That is true. His code much better than many I have seen before Thanks. Also thanks for taking the time to review. > > despite a few corner > > issues, and being (again) treated like crap. > > Sorry for that, life is harsh. > > > This isn't the right way to operate, and I believe this has been made > > clear many times before. > > Yes. Cheers, Ben.