[PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Add an atomic_xfer method to the driver so that it behaves correctly
when controlling a PMIC that is responsible for device shutdown.

The atomic_xfer method added is similar to the one from the i2c-mv64xxx
driver. When running an atomic_xfer a bool flag in the driver data is
set, the interrupt is not unmasked on transfer start, and the IRQ
handler is manually invoked while waiting for pending transfers to
complete.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v1:
 - Removed dma in atomic

 drivers/i2c/busses/i2c-stm32f7.c | 111 ++++++++++++++++++++++---------
 1 file changed, 78 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index e897d9101434..d944b8f85d1c 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
 	u32 dnf_dt;
 	u32 dnf;
 	struct stm32f7_i2c_alert *alert;
+	bool atomic;
 };
 
 /*
@@ -905,38 +906,43 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
 	}
 
-	/* Enable NACK, STOP, error and transfer complete interrupts */
-	cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
-		STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
-
-	/* Clear DMA req and TX/RX interrupt */
-	cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
-			STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
-
-	/* Configure DMA or enable RX/TX interrupt */
-	i2c_dev->use_dma = false;
-	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
-		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
-					      msg->flags & I2C_M_RD,
-					      f7_msg->count, f7_msg->buf,
-					      stm32f7_i2c_dma_callback,
-					      i2c_dev);
-		if (!ret)
-			i2c_dev->use_dma = true;
-		else
-			dev_warn(i2c_dev->dev, "can't use DMA\n");
-	}
+	if (!i2c_dev->atomic) {
+		/* Enable NACK, STOP, error and transfer complete interrupts */
+		cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
+			STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
+
+		/* Clear DMA req and TX/RX interrupt */
+		cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
+				STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
+
+		/* Configure DMA or enable RX/TX interrupt */
+		i2c_dev->use_dma = false;
+		if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
+			ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
+					msg->flags & I2C_M_RD,
+					f7_msg->count, f7_msg->buf,
+					stm32f7_i2c_dma_callback,
+					i2c_dev);
+			if (!ret)
+				i2c_dev->use_dma = true;
+			else
+				dev_warn(i2c_dev->dev, "can't use DMA\n");
+		}
 
-	if (!i2c_dev->use_dma) {
-		if (msg->flags & I2C_M_RD)
-			cr1 |= STM32F7_I2C_CR1_RXIE;
-		else
-			cr1 |= STM32F7_I2C_CR1_TXIE;
+		if (!i2c_dev->use_dma) {
+			if (msg->flags & I2C_M_RD)
+				cr1 |= STM32F7_I2C_CR1_RXIE;
+			else
+				cr1 |= STM32F7_I2C_CR1_TXIE;
+		} else {
+			if (msg->flags & I2C_M_RD)
+				cr1 |= STM32F7_I2C_CR1_RXDMAEN;
+			else
+				cr1 |= STM32F7_I2C_CR1_TXDMAEN;
+		}
 	} else {
-		if (msg->flags & I2C_M_RD)
-			cr1 |= STM32F7_I2C_CR1_RXDMAEN;
-		else
-			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
+		/* Disable all interrupts */
+		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
 	}
 
 	/* Configure Start/Repeated Start */
@@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
+static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
+{
+	ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
+
+	while (ktime_compare(ktime_get(), timeout) < 0) {
+		udelay(5);
+		stm32f7_i2c_isr_event(0, i2c_dev);
+
+		if (try_wait_for_completion(&i2c_dev->complete))
+			return 1;
+	}
+
+	return 0;
+}
+
+static int stm32f7_i2c_xfer_core(struct i2c_adapter *i2c_adap,
 			    struct i2c_msg msgs[], int num)
 {
 	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
@@ -1694,8 +1715,13 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 	stm32f7_i2c_xfer_msg(i2c_dev, msgs);
 
-	time_left = wait_for_completion_timeout(&i2c_dev->complete,
-						i2c_dev->adap.timeout);
+	if (!i2c_dev->atomic) {
+		time_left = wait_for_completion_timeout(&i2c_dev->complete,
+							i2c_dev->adap.timeout);
+	} else {
+		time_left = stm32f7_i2c_wait_polling(i2c_dev);
+	}
+
 	ret = f7_msg->result;
 	if (ret) {
 		if (i2c_dev->use_dma)
@@ -1727,6 +1753,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
 	return (ret < 0) ? ret : num;
 }
 
+static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
+			    struct i2c_msg msgs[], int num)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+	i2c_dev->atomic = 0;
+	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
+static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
+			    struct i2c_msg msgs[], int num)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+	i2c_dev->atomic = 1;
+	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
 static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 				  unsigned short flags, char read_write,
 				  u8 command, int size,
@@ -2095,6 +2139,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm stm32f7_i2c_algo = {
 	.master_xfer = stm32f7_i2c_xfer,
+	.master_xfer_atomic = stm32f7_i2c_xfer_atomic,
 	.smbus_xfer = stm32f7_i2c_smbus_xfer,
 	.functionality = stm32f7_i2c_func,
 	.reg_slave = stm32f7_i2c_reg_slave,
-- 
2.40.0

Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Sean,

[...]

> @@ -905,38 +906,43 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>  		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>  	}
>  
> -	/* Enable NACK, STOP, error and transfer complete interrupts */
> -	cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> -		STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> -
> -	/* Clear DMA req and TX/RX interrupt */
> -	cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> -			STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> -
> -	/* Configure DMA or enable RX/TX interrupt */
> -	i2c_dev->use_dma = false;
> -	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> -		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> -					      msg->flags & I2C_M_RD,
> -					      f7_msg->count, f7_msg->buf,
> -					      stm32f7_i2c_dma_callback,
> -					      i2c_dev);
> -		if (!ret)
> -			i2c_dev->use_dma = true;
> -		else
> -			dev_warn(i2c_dev->dev, "can't use DMA\n");
> -	}
> +	if (!i2c_dev->atomic) {
> +		/* Enable NACK, STOP, error and transfer complete interrupts */
> +		cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> +			STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> +
> +		/* Clear DMA req and TX/RX interrupt */
> +		cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> +				STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> +
> +		/* Configure DMA or enable RX/TX interrupt */
> +		i2c_dev->use_dma = false;
> +		if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> +			ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> +					msg->flags & I2C_M_RD,
> +					f7_msg->count, f7_msg->buf,
> +					stm32f7_i2c_dma_callback,
> +					i2c_dev);
> +			if (!ret)
> +				i2c_dev->use_dma = true;
> +			else
> +				dev_warn(i2c_dev->dev, "can't use DMA\n");
> +		}
>  
> -	if (!i2c_dev->use_dma) {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXIE;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXIE;
> +		if (!i2c_dev->use_dma) {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXIE;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXIE;
> +		} else {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		}
>  	} else {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		/* Disable all interrupts */
> +		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;

if you do

	if (i2c_dev->atomic) {
		/* Disable all interrupts */
		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
		return;
	}

you save all the above from a leveel of indentation.

>  	}
>  
>  	/* Configure Start/Repeated Start */
> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
> +{
> +	ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
> +
> +	while (ktime_compare(ktime_get(), timeout) < 0) {
> +		udelay(5);
> +		stm32f7_i2c_isr_event(0, i2c_dev);
> +
> +		if (try_wait_for_completion(&i2c_dev->complete))
> +			return 1;

I'm wondering if it makes really sense to have a complete() and
wait_for_completion() scheme here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32f7_i2c_xfer_core(struct i2c_adapter *i2c_adap,
>  			    struct i2c_msg msgs[], int num)
>  {
>  	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> @@ -1694,8 +1715,13 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  
>  	stm32f7_i2c_xfer_msg(i2c_dev, msgs);
>  
> -	time_left = wait_for_completion_timeout(&i2c_dev->complete,
> -						i2c_dev->adap.timeout);
> +	if (!i2c_dev->atomic) {
> +		time_left = wait_for_completion_timeout(&i2c_dev->complete,
> +							i2c_dev->adap.timeout);
> +	} else {
> +		time_left = stm32f7_i2c_wait_polling(i2c_dev);
> +	}

please, drop the brackets here... and time_left here serves only
not to get the -ETIMEDOUT... looks a bit ugly to me, but can't
think of a better way.

> +
>  	ret = f7_msg->result;
>  	if (ret) {
>  		if (i2c_dev->use_dma)
> @@ -1727,6 +1753,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	return (ret < 0) ? ret : num;
>  }
>  
> +static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 0;

false

> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +
> +static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 1;

true

Andi

> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +

Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Alain Volmat]

Hi Sean,

sorry for the delay for this review.  Thank you Andi for
the review as well.

Few other comments in addition to what Andi already mentioned.

On Tue, Jul 18, 2023 at 12:54:35PM +0200, Sean Nyekjaer wrote:
> Add an atomic_xfer method to the driver so that it behaves correctly
> when controlling a PMIC that is responsible for device shutdown.
> 
> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
> driver. When running an atomic_xfer a bool flag in the driver data is
> set, the interrupt is not unmasked on transfer start, and the IRQ
> handler is manually invoked while waiting for pending transfers to
> complete.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v1:
>  - Removed dma in atomic
> 
>  drivers/i2c/busses/i2c-stm32f7.c | 111 ++++++++++++++++++++++---------
>  1 file changed, 78 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index e897d9101434..d944b8f85d1c 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>  	u32 dnf_dt;
>  	u32 dnf;
>  	struct stm32f7_i2c_alert *alert;
> +	bool atomic;

I am wondering if this atomic really needs to be within the struct.
It could well be given as last arg of stm32f7_i2c_xfer_core and
stm32f7_i2c_xfer functions.


>  };
>  
>  /*
> @@ -905,38 +906,43 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>  		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>  	}
>  
> -	/* Enable NACK, STOP, error and transfer complete interrupts */
> -	cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> -		STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> -
> -	/* Clear DMA req and TX/RX interrupt */
> -	cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> -			STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> -
> -	/* Configure DMA or enable RX/TX interrupt */
> -	i2c_dev->use_dma = false;
> -	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> -		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> -					      msg->flags & I2C_M_RD,
> -					      f7_msg->count, f7_msg->buf,
> -					      stm32f7_i2c_dma_callback,
> -					      i2c_dev);
> -		if (!ret)
> -			i2c_dev->use_dma = true;
> -		else
> -			dev_warn(i2c_dev->dev, "can't use DMA\n");
> -	}
> +	if (!i2c_dev->atomic) {
> +		/* Enable NACK, STOP, error and transfer complete interrupts */
> +		cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> +			STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> +
> +		/* Clear DMA req and TX/RX interrupt */
> +		cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> +				STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> +
> +		/* Configure DMA or enable RX/TX interrupt */
> +		i2c_dev->use_dma = false;
> +		if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> +			ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> +					msg->flags & I2C_M_RD,
> +					f7_msg->count, f7_msg->buf,
> +					stm32f7_i2c_dma_callback,
> +					i2c_dev);
> +			if (!ret)
> +				i2c_dev->use_dma = true;
> +			else
> +				dev_warn(i2c_dev->dev, "can't use DMA\n");
> +		}
>  
> -	if (!i2c_dev->use_dma) {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXIE;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXIE;
> +		if (!i2c_dev->use_dma) {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXIE;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXIE;
> +		} else {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		}
>  	} else {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		/* Disable all interrupts */
> +		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
>  	}
>  
>  	/* Configure Start/Repeated Start */
> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
> +{
> +	ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
> +
> +	while (ktime_compare(ktime_get(), timeout) < 0) {
> +		udelay(5);
> +		stm32f7_i2c_isr_event(0, i2c_dev);
> +
> +		if (try_wait_for_completion(&i2c_dev->complete))
> +			return 1;

I agree with the complete / wait_for_completion approach since it allows
to keep most of code common by manually calling the isr_event for
checking status bits.  However what about using completion_done instead
of try_wait_for_completion here ? This shouldn't change much since
anyway there is a reinit_completion at the beginning of the xfer
function, but at least function naming feels better since not refering
to waiting ..

> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32f7_i2c_xfer_core(struct i2c_adapter *i2c_adap,
>  			    struct i2c_msg msgs[], int num)
>  {
>  	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> @@ -1694,8 +1715,13 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  
>  	stm32f7_i2c_xfer_msg(i2c_dev, msgs);
>  
> -	time_left = wait_for_completion_timeout(&i2c_dev->complete,
> -						i2c_dev->adap.timeout);
> +	if (!i2c_dev->atomic) {
> +		time_left = wait_for_completion_timeout(&i2c_dev->complete,
> +							i2c_dev->adap.timeout);
> +	} else {
> +		time_left = stm32f7_i2c_wait_polling(i2c_dev);
> +	}
> +
>  	ret = f7_msg->result;
>  	if (ret) {
>  		if (i2c_dev->use_dma)
> @@ -1727,6 +1753,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	return (ret < 0) ? ret : num;
>  }
>  
> +static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 0;
> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +
> +static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 1;
> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +
>  static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>  				  unsigned short flags, char read_write,
>  				  u8 command, int size,
> @@ -2095,6 +2139,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
>  
>  static const struct i2c_algorithm stm32f7_i2c_algo = {
>  	.master_xfer = stm32f7_i2c_xfer,
> +	.master_xfer_atomic = stm32f7_i2c_xfer_atomic,
>  	.smbus_xfer = stm32f7_i2c_smbus_xfer,
>  	.functionality = stm32f7_i2c_func,
>  	.reg_slave = stm32f7_i2c_reg_slave,
> -- 

Regards,
Alain

> 2.40.0
> 

Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Andi,

Thanks for the review

> On 26 Jul 2023, at 23.21, Andi Shyti <andi.shyti@kernel.org> wrote:
> 
> Hi Sean,
> 
> [...]
> 
>> @@ -905,38 +906,43 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>> cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>> }
>> 
>> - /* Enable NACK, STOP, error and transfer complete interrupts */
>> - cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
>> - STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
>> -
>> - /* Clear DMA req and TX/RX interrupt */
>> - cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
>> - STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
>> -
>> - /* Configure DMA or enable RX/TX interrupt */
>> - i2c_dev->use_dma = false;
>> - if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
>> - ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
>> -      msg->flags & I2C_M_RD,
>> -      f7_msg->count, f7_msg->buf,
>> -      stm32f7_i2c_dma_callback,
>> -      i2c_dev);
>> - if (!ret)
>> - i2c_dev->use_dma = true;
>> - else
>> - dev_warn(i2c_dev->dev, "can't use DMA\n");
>> - }
>> + if (!i2c_dev->atomic) {
>> + /* Enable NACK, STOP, error and transfer complete interrupts */
>> + cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
>> + STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
>> +
>> + /* Clear DMA req and TX/RX interrupt */
>> + cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
>> + STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
>> +
>> + /* Configure DMA or enable RX/TX interrupt */
>> + i2c_dev->use_dma = false;
>> + if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
>> + ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
>> + msg->flags & I2C_M_RD,
>> + f7_msg->count, f7_msg->buf,
>> + stm32f7_i2c_dma_callback,
>> + i2c_dev);
>> + if (!ret)
>> + i2c_dev->use_dma = true;
>> + else
>> + dev_warn(i2c_dev->dev, "can't use DMA\n");
>> + }
>> 
>> - if (!i2c_dev->use_dma) {
>> - if (msg->flags & I2C_M_RD)
>> - cr1 |= STM32F7_I2C_CR1_RXIE;
>> - else
>> - cr1 |= STM32F7_I2C_CR1_TXIE;
>> + if (!i2c_dev->use_dma) {
>> + if (msg->flags & I2C_M_RD)
>> + cr1 |= STM32F7_I2C_CR1_RXIE;
>> + else
>> + cr1 |= STM32F7_I2C_CR1_TXIE;
>> + } else {
>> + if (msg->flags & I2C_M_RD)
>> + cr1 |= STM32F7_I2C_CR1_RXDMAEN;
>> + else
>> + cr1 |= STM32F7_I2C_CR1_TXDMAEN;
>> + }
>> } else {
>> - if (msg->flags & I2C_M_RD)
>> - cr1 |= STM32F7_I2C_CR1_RXDMAEN;
>> - else
>> - cr1 |= STM32F7_I2C_CR1_TXDMAEN;
>> + /* Disable all interrupts */
>> + cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
> 
> if you do
> 
> if (i2c_dev->atomic) {
> /* Disable all interrupts */
> cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
> return;
> }
> 
> you save all the above from a leveel of indentation.

Agree, it would be best not to indent this.
But the last step in the function is to write the cr1 value :) Goto doesn’t seem very pretty, but up to you...


> 
>> }
>> 
>> /* Configure Start/Repeated Start */
>> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>> 

[ … ]

>> - time_left = wait_for_completion_timeout(&i2c_dev->complete,
>> - i2c_dev->adap.timeout);
>> + if (!i2c_dev->atomic) {
>> + time_left = wait_for_completion_timeout(&i2c_dev->complete,
>> + i2c_dev->adap.timeout);
>> + } else {
>> + time_left = stm32f7_i2c_wait_polling(i2c_dev);
>> + }
> 
> please, drop the brackets here... and time_left here serves only
> not to get the -ETIMEDOUT... looks a bit ugly to me, but can't
> think of a better way.

Done.

> 
>> +
>> ret = f7_msg->result;
>> if (ret) {
>> if (i2c_dev->use_dma)
>> @@ -1727,6 +1753,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>> return (ret < 0) ? ret : num;
>> }
>> 
>> +static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>> +    struct i2c_msg msgs[], int num)
>> +{
>> + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> +
>> + i2c_dev->atomic = 0;
> 
> false
> 
>> + return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
>> +}
>> +
>> +static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
>> +    struct i2c_msg msgs[], int num)
>> +{
>> + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> +
>> + i2c_dev->atomic = 1;
> 
> true
> 
> Andi

/Sean

Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Alain,

Thanks for the review

> On 2 Aug 2023, at 12.07, Alain Volmat <alain.volmat@foss.st.com> wrote:
> 
> Hi Sean,
> 
> sorry for the delay for this review.  Thank you Andi for
> the review as well.

Also from my side :) Vacation.

> 
> Few other comments in addition to what Andi already mentioned.
> 
> On Tue, Jul 18, 2023 at 12:54:35PM +0200, Sean Nyekjaer wrote:
>> Add an atomic_xfer method to the driver so that it behaves correctly
>> when controlling a PMIC that is responsible for device shutdown.
>> 
>> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
>> driver. When running an atomic_xfer a bool flag in the driver data is
>> set, the interrupt is not unmasked on transfer start, and the IRQ
>> handler is manually invoked while waiting for pending transfers to
>> complete.
>> 
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> Changes since v1:
>> - Removed dma in atomic
>> 
>> drivers/i2c/busses/i2c-stm32f7.c | 111 ++++++++++++++++++++++---------
>> 1 file changed, 78 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
>> index e897d9101434..d944b8f85d1c 100644
>> --- a/drivers/i2c/busses/i2c-stm32f7.c
>> +++ b/drivers/i2c/busses/i2c-stm32f7.c
>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>> u32 dnf_dt;
>> u32 dnf;
>> struct stm32f7_i2c_alert *alert;
>> + bool atomic;
> 
> I am wondering if this atomic really needs to be within the struct.
> It could well be given as last arg of stm32f7_i2c_xfer_core and
> stm32f7_i2c_xfer functions.

Agree.

> 
> 
>> };
>> 
>> 

[ … ]

>> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>> 
>> -static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>> +static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> + ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
>> +
>> + while (ktime_compare(ktime_get(), timeout) < 0) {
>> + udelay(5);
>> + stm32f7_i2c_isr_event(0, i2c_dev);
>> +
>> + if (try_wait_for_completion(&i2c_dev->complete))
>> + return 1;
> 
> I agree with the complete / wait_for_completion approach since it allows
> to keep most of code common by manually calling the isr_event for
> checking status bits.  However what about using completion_done instead
> of try_wait_for_completion here ? This shouldn't change much since
> anyway there is a reinit_completion at the beginning of the xfer
> function, but at least function naming feels better since not refering
> to waiting ..

I’ll take a look at the completion_done()

> 
>> + }
>> 

[ … ]

/Sean

Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Alain,

> On 16 Aug 2023, at 09.02, Sean Nyekjaer <sean@geanix.com> wrote:
> 

[ … ]

>>> _dev {
>>> u32 dnf_dt;
>>> u32 dnf;
>>> struct stm32f7_i2c_alert *alert;
>>> + bool atomic;
>> 
>> I am wondering if this atomic really needs to be within the struct.
>> It could well be given as last arg of stm32f7_i2c_xfer_core and
>> stm32f7_i2c_xfer functions.
> 
> Agree.

Scratch that…
The atomic was included in the struct because it’s also used in the isr function, as the isr function is calling stm32f7_i2c_xfer_msg()

/Sean