| Message ID | 1504251255-20469-1-git-send-email-pierre-yves.mordret@st.com |
|---|---|
| Headers | show |
| Series | Add support for the STM32F7 I2C | expand |
2017-09-01 9:34 GMT+02:00 Pierre-Yves MORDRET <pierre-yves.mordret@st.com>: > This patchset adds support for the I2C controller embedded in STM32F7xx SoC. > It enables I2C transfer in interrupt mode with Standard-mode, Fast-mode and > Fast-mode+ bus speed. Hi Wolfram, I notice that those patches aren't in pull request for 4.14 and not in i2c-next branch. What can we do to progress in this topic? Does issues/remarks still need to be fixed in this code ? Thanks for your advices, Benjamin > --- > Version history: > v3: > * Move stm32f7_i2c_match above stm32f7_i2c_driver > * of_device_get_match_data instead of of_match_device > * Improve I2C Speed DT gathering > * dev_err into dev_dbg for Arbitration loss > * Remove useless space aligned > > v2: > * Implement an I2C timings computation algorithm instead of static > values(bindings). Algorithm uses generic I2C SCL Falling/Rising > bindings and System clock to compute its timings. > * I2C Device Tree Update > --- > Pierre-Yves MORDRET (5): > dt-bindings: i2c-stm32: Document the STM32F7 I2C bindings > i2c: i2c-stm32f4: use generic definition of speed enum > i2c: i2c-stm32f7: add driver > ARM: dts: stm32: Add I2C1 support for STM32F746 SoC > ARM: dts: stm32: Add I2C1 support for STM32F746 eval board > > .../devicetree/bindings/i2c/i2c-stm32.txt | 29 +- > arch/arm/boot/dts/stm32746g-eval.dts | 8 + > arch/arm/boot/dts/stm32f746.dtsi | 22 + > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-stm32.h | 20 + > drivers/i2c/busses/i2c-stm32f4.c | 18 +- > drivers/i2c/busses/i2c-stm32f7.c | 974 +++++++++++++++++++++ > 8 files changed, 1068 insertions(+), 14 deletions(-) > create mode 100644 drivers/i2c/busses/i2c-stm32.h > create mode 100644 drivers/i2c/busses/i2c-stm32f7.c > > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, thanks for this driver! > +/** > + * struct stm32f7_i2c_spec - private i2c specification timing > + * @rate: I2C bus speed (Hz) > + * @rate_min: 80% of I2C bus speed (Hz) > + * @rate_max: 120% of I2C bus speed (Hz) You would generate a clock which is higher than the requested one? This is highly unusual. Any special reason? > + * @fall_max: Max fall time of both SDA and SCL signals (ns) > + * @rise_max: Max rise time of both SDA and SCL signals (ns) > + * @hddat_min: Min data hold time (ns) > + * @vddat_max: Max data valid time (ns) > + * @sudat_min: Min data setup time (ns) > + * @l_min: Min low period of the SCL clock (ns) > + * @h_min: Min high period of the SCL clock (ns) > + */ > +static struct stm32f7_i2c_spec i2c_specs[] = { > + [STM32_I2C_SPEED_STANDARD] = { > + .rate = 100000, > + .rate_min = 8000, This is not 80%. Typo? > + .rate_max = 120000, > + .fall_max = 300, > + .rise_max = 1000, > + .hddat_min = 0, > + .vddat_max = 3450, > + .sudat_min = 250, > + .l_min = 4700, > + .h_min = 4000, > + }, ... > + /* > + * Among Prescaler possibilities discovered above figures out SCL Low > + * and High Period. Provided: > + * - SCL Low Period has to be higher than Low Period of tehs SCL Clock tehs? > + * defined by I2C Specification. I2C Clock has to be lower than > + * (SCL Low Period - Analog/Digital filters) / 4. > + * - SCL High Period has to be lower than High Period of the SCL Clock > + * defined by I2C Specification > + * - I2C Clock has to be lower than SCL High Period > + */ ... > + /* NACK received */ > + if (status & STM32F7_I2C_ISR_NACKF) { > + dev_dbg(i2c_dev->dev, "<%s>: Receive NACK\n", __func__); > + writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR); > + f7_msg->result = -EBADE; -ENXIO (see Documentation/i2c/fault-codes) ... > + timeout = wait_for_completion_timeout(&i2c_dev->complete, > + i2c_dev->adap.timeout); > + ret = f7_msg->result; > + > + if (!timeout) { > + dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n", > + i2c_dev->msg->addr); > + ret = -ETIMEDOUT; > + } Could you rename the variable to time_left? It looks strange, basically: if (!timeout) return -ETIMEDOUT ... > + adap->retries = 0; Why no retries when you check for arbitration lost? > + adap->algo = &stm32f7_i2c_algo; > + adap->dev.parent = &pdev->dev; > + adap->dev.of_node = pdev->dev.of_node; > + > + init_completion(&i2c_dev->complete); > + > + ret = i2c_add_adapter(adap); > + if (ret) { > + dev_err(&pdev->dev, "Failed to add adapter\n"); Please remove, the core will print info when adding fails. Rest looks good! Thanks, Wolfram
On 09/13/2017 11:26 PM, Wolfram Sang wrote: > Hi, > > thanks for this driver! > >> +/** >> + * struct stm32f7_i2c_spec - private i2c specification timing >> + * @rate: I2C bus speed (Hz) >> + * @rate_min: 80% of I2C bus speed (Hz) >> + * @rate_max: 120% of I2C bus speed (Hz) > > You would generate a clock which is higher than the requested one? > This is highly unusual. Any special reason? Well. I allow the clock to be higher than expected. Looking at I2C spec again it turns out the mode specifies the max: no overshoot of the clock. I will lock max to 100% then. Will be fixed > >> + * @fall_max: Max fall time of both SDA and SCL signals (ns) >> + * @rise_max: Max rise time of both SDA and SCL signals (ns) >> + * @hddat_min: Min data hold time (ns) >> + * @vddat_max: Max data valid time (ns) >> + * @sudat_min: Min data setup time (ns) >> + * @l_min: Min low period of the SCL clock (ns) >> + * @h_min: Min high period of the SCL clock (ns) >> + */ >> +static struct stm32f7_i2c_spec i2c_specs[] = { >> + [STM32_I2C_SPEED_STANDARD] = { >> + .rate = 100000, >> + .rate_min = 8000, > > This is not 80%. Typo? Yep. This is a typo > >> + .rate_max = 120000, >> + .fall_max = 300, >> + .rise_max = 1000, >> + .hddat_min = 0, >> + .vddat_max = 3450, >> + .sudat_min = 250, >> + .l_min = 4700, >> + .h_min = 4000, >> + }, > > ... > >> + /* >> + * Among Prescaler possibilities discovered above figures out SCL Low >> + * and High Period. Provided: >> + * - SCL Low Period has to be higher than Low Period of tehs SCL Clock > > tehs? Oops. > >> + * defined by I2C Specification. I2C Clock has to be lower than >> + * (SCL Low Period - Analog/Digital filters) / 4. >> + * - SCL High Period has to be lower than High Period of the SCL Clock >> + * defined by I2C Specification >> + * - I2C Clock has to be lower than SCL High Period >> + */ > > ... > >> + /* NACK received */ >> + if (status & STM32F7_I2C_ISR_NACKF) { >> + dev_dbg(i2c_dev->dev, "<%s>: Receive NACK\n", __func__); >> + writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR); >> + f7_msg->result = -EBADE; > > -ENXIO (see Documentation/i2c/fault-codes) OK > > ... > >> + timeout = wait_for_completion_timeout(&i2c_dev->complete, >> + i2c_dev->adap.timeout); >> + ret = f7_msg->result; >> + >> + if (!timeout) { >> + dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n", >> + i2c_dev->msg->addr); >> + ret = -ETIMEDOUT; >> + } > > Could you rename the variable to time_left? It looks strange, basically: > > if (!timeout) > return -ETIMEDOUT > okay > ... > >> + adap->retries = 0; > > Why no retries when you check for arbitration lost? > >> + adap->algo = &stm32f7_i2c_algo; >> + adap->dev.parent = &pdev->dev; >> + adap->dev.of_node = pdev->dev.of_node; >> + >> + init_completion(&i2c_dev->complete); >> + >> + ret = i2c_add_adapter(adap); >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to add adapter\n"); > > Please remove, the core will print info when adding fails. > I will > > Rest looks good! Great ! > > Thanks, > > Wolfram > Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This patchset adds support for the I2C controller embedded in STM32F7xx SoC. It enables I2C transfer in interrupt mode with Standard-mode, Fast-mode and Fast-mode+ bus speed. --- Version history: v3: * Move stm32f7_i2c_match above stm32f7_i2c_driver * of_device_get_match_data instead of of_match_device * Improve I2C Speed DT gathering * dev_err into dev_dbg for Arbitration loss * Remove useless space aligned v2: * Implement an I2C timings computation algorithm instead of static values(bindings). Algorithm uses generic I2C SCL Falling/Rising bindings and System clock to compute its timings. * I2C Device Tree Update --- Pierre-Yves MORDRET (5): dt-bindings: i2c-stm32: Document the STM32F7 I2C bindings i2c: i2c-stm32f4: use generic definition of speed enum i2c: i2c-stm32f7: add driver ARM: dts: stm32: Add I2C1 support for STM32F746 SoC ARM: dts: stm32: Add I2C1 support for STM32F746 eval board .../devicetree/bindings/i2c/i2c-stm32.txt | 29 +- arch/arm/boot/dts/stm32746g-eval.dts | 8 + arch/arm/boot/dts/stm32f746.dtsi | 22 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-stm32.h | 20 + drivers/i2c/busses/i2c-stm32f4.c | 18 +- drivers/i2c/busses/i2c-stm32f7.c | 974 +++++++++++++++++++++ 8 files changed, 1068 insertions(+), 14 deletions(-) create mode 100644 drivers/i2c/busses/i2c-stm32.h create mode 100644 drivers/i2c/busses/i2c-stm32f7.c