Hi Alain,
On Fri, Dec 08, 2023 at 05:47:13PM +0100, Alain Volmat wrote:
> The stm32mp25 has only a single interrupt line used for both
> events and errors. In order to cope with that, reorganise the
> error handling code so that it can be called either from the
> common handler (used in case of SoC having only a single IT line)
> and the error handler for others.
> The CR1 register also embeds a new FMP bit, necessary when running
> at Fast Mode Plus frequency. This bit should be used instead of
> the SYSCFG bit used on other platforms.
> Add a new compatible to distinguish between the SoCs and two
> boolean within the setup structure in order to know if the
> platform has a single/multiple IT lines and if the FMP bit
> within CR1 is available or not.
>
> Signed-off-by: Alain Volmat <alain.volmat@xxxxxxxxxxx>
> Signed-off-by: Valentin Caron <valentin.caron@xxxxxxxxxxx>
your SoB here should come last because you are the one sending
the patch.
> ---
> drivers/i2c/busses/i2c-stm32f7.c | 230 ++++++++++++++++++-------------
> 1 file changed, 133 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 2a011deec3c5..5634332900fb 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -50,6 +50,7 @@
> #define STM32F7_I2C_TXDR 0x28
>
> /* STM32F7 I2C control 1 */
> +#define STM32_I2C_CR1_FMP BIT(24)
> #define STM32F7_I2C_CR1_PECEN BIT(23)
> #define STM32F7_I2C_CR1_ALERTEN BIT(22)
> #define STM32F7_I2C_CR1_SMBHEN BIT(20)
> @@ -226,6 +227,8 @@ struct stm32f7_i2c_spec {
> * @rise_time: Rise time (ns)
> * @fall_time: Fall time (ns)
> * @fmp_clr_offset: Fast Mode Plus clear register offset from set register
> + * @single_it_line: Only a single IT line is used for both events/errors
> + * @fmp_cr1_bit: Fast Mode Plus control is done via a bit in CR1
Is the Fast Mode Plus an optional feature?
> */
> struct stm32f7_i2c_setup {
> u32 speed_freq;
> @@ -233,6 +236,8 @@ struct stm32f7_i2c_setup {
> u32 rise_time;
> u32 fall_time;
> u32 fmp_clr_offset;
> + bool single_it_line;
> + bool fmp_cr1_bit;
> };
[...]
> -static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> +static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev, u32 status)
> {
> void __iomem *base = i2c_dev->base;
> - u32 cr2, status, mask;
> + u32 cr2, mask;
> u8 val;
> int ret;
>
> - status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
> -
good to see this change here, relates to my comment in patch 1.
But I think this should go on a different patch.
> /* Slave transmitter mode */
> if (status & STM32F7_I2C_ISR_TXIS) {
> i2c_slave_event(i2c_dev->slave_running,
> @@ -1494,17 +1504,81 @@ static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> return IRQ_HANDLED;
> }
[...]
> - setup = of_device_get_match_data(&pdev->dev);
> - if (!setup) {
> - dev_err(&pdev->dev, "Can't get device data\n");
> - return -ENODEV;
> + ret = devm_request_threaded_irq(&pdev->dev, irq_error,
> + NULL,
> + stm32f7_i2c_isr_error_thread,
> + IRQF_ONESHOT,
> + pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq error %i\n",
> + irq_error);
please use dev_err_probe();
> + return ret;
> + }
out of the scope of the patch and just for curiosity: does the
driver work without being able to signal on the error interrupt
line?
Overall the patch looks good to me, though.
Andi
