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

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

Message

Eddie James June 13, 2018, 7:36 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 v9
 - Switch the status masks to use a combination of bits rather than directly
   coding the value
 - Remove the interrupt mask definition as it was unused
 - Fixed return value of master_xfer function (return number of xfrs, not 0)

Changes since v8
 - Drop unecessary else statements
 - Use i++ instead of ++i
 - Use kzalloc/kfree instead of devm_kzalloc/devm_kfree for port structure
 - Drop the list_empty check in remove

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                      | 727 ++++++++++++++++++++++
 4 files changed, 779 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
 create mode 100644 drivers/i2c/busses/i2c-fsi.c

Comments

Andy Shevchenko June 14, 2018, 9:05 a.m. UTC | #1
On Wed, Jun 13, 2018 at 10:36 PM, 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.
>

You missed to include tag I gave

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

be careful next time.

> Changes since v9
>  - Switch the status masks to use a combination of bits rather than directly
>    coding the value
>  - Remove the interrupt mask definition as it was unused
>  - Fixed return value of master_xfer function (return number of xfrs, not 0)
>
> Changes since v8
>  - Drop unecessary else statements
>  - Use i++ instead of ++i
>  - Use kzalloc/kfree instead of devm_kzalloc/devm_kfree for port structure
>  - Drop the list_empty check in remove
>
> 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                      | 727 ++++++++++++++++++++++
>  4 files changed, 779 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
>  create mode 100644 drivers/i2c/busses/i2c-fsi.c
>
> --
> 1.8.3.1
>
Joel Stanley June 18, 2018, 4:53 a.m. UTC | #2
On 14 June 2018 at 05:06, 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.

Looks good Eddie. I had a read over what you have, and I would be
happy to see this merged in 4.19.

Reviewed-By: Joel Stanley <joel@jms.id.au>

Also, I have merged this into the 4.17 based openbmc tree for testing
while we wait for it to be merged upstream.

Cheers,

Joel
Wolfram Sang June 26, 2018, 2:39 a.m. UTC | #3
On Wed, Jun 13, 2018 at 02:36:12PM -0500, Eddie James 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.

Thanks for this series and your patience.

While I can see why it also helps reviewing to send it as a series of
multiple patches, I consider applying the driver itself as just one
hunk. I am not decided on this yet.

I have a few comments, especially about recovery. I replied to the
relevant patches with more detail.

Also, are you (or someone from your company) willing to maintain the
driver? Then, an addition to MAINTAINERS would be much appreciated.

Thanks,

   Wolfram
Eddie James June 27, 2018, 1:53 p.m. UTC | #4
On 06/25/2018 09:39 PM, Wolfram Sang wrote:
> On Wed, Jun 13, 2018 at 02:36:12PM -0500, Eddie James 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.
> Thanks for this series and your patience.
>
> While I can see why it also helps reviewing to send it as a series of
> multiple patches, I consider applying the driver itself as just one
> hunk. I am not decided on this yet.
>
> I have a few comments, especially about recovery. I replied to the
> relevant patches with more detail.
>
> Also, are you (or someone from your company) willing to maintain the
> driver? Then, an addition to MAINTAINERS would be much appreciated.

Thanks for the review Wolfram! I addressed your comments about recovery, 
please let me know what you think. I will fix the email for the dt patch 
and make an addition to MAINTAINERS for the next version.

Thanks,
Eddie

>
> Thanks,
>
>     Wolfram
>