Hi Andi,
Thanks for the review
> On 26 Jul 2023, at 23.21, Andi Shyti <andi.shyti@xxxxxxxxxx> 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
