Re: [PATCH v2 4/7] i2c: stm32f7: add support for stm32mp25 soc

[Alain Volmat]

Hi Andi,

thanks for your review.

I'll shortly post a v3 to correct all your comments.

On Sat, Dec 09, 2023 at 01:07:47AM +0100, Andi Shyti wrote:
> 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.

Fixed.

> 
> > ---
> >  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?

Yes, from what I've seen, it seems an optional feature on some versions
of stm32f7.

> 
> >   */
> >  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.

Agreed. I've split this small change into a dedicated commit done before
this 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();

Done as well in a dedicated commit at the very beginning of the serie.

> 
> > +			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?

Sorry, not sure to understand the question.  Just for clarification,
on the MP25 not having a dedicated error line doesn't mean we are not
signaled for errors.  It is simply they just come via the same
interrupt line, hence we need to check for both event and error
within the same handler.
On systems using two interrupts line, the error interrupt irq line
should be handled in order to tackle those errors cases.
Sorry, does this answer your question ?

> 
> Overall the patch looks good to me, though.
> 
> Andi