[v8,0/7] i2c: Add FSI-attached I2C master algorithm
mbox series

Message ID 1527714464-8642-1-git-send-email-eajames@linux.vnet.ibm.com
Headers show
Series
  • i2c: Add FSI-attached I2C master algorithm
Related show

Message

Eddie James May 30, 2018, 9:07 p.m. UTC
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.

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

Comments

Andy Shevchenko May 30, 2018, 9:31 p.m. UTC | #1
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
>
Benjamin Herrenschmidt May 30, 2018, 10:42 p.m. UTC | #2
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
> > 
> 
> 
>
Andy Shevchenko May 31, 2018, 6:29 a.m. UTC | #3
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.
Benjamin Herrenschmidt May 31, 2018, 11:33 p.m. UTC | #4
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.