RE: [PATCH 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

["Jaewon Kim" <jaewon02.kim@samsung.com>]

Hello Krzysztof


> On 11/11/2021 09:43, Jaewon Kim wrote:
> > Serial IPs(UART, I2C, SPI) are integrated into New IP-Core called
> > USI(Universal Serial Interface).
> >
> > As it is integrated into USI, there are additinal HW changes.
> > Registers to control USI and sysreg to set serial IPs have been added.
> > Also, some timing registres have been changed.
> >
> > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> > ---
> >  drivers/i2c/busses/i2c-exynos5.c | 133
> > ++++++++++++++++++++++++++++---
> >  1 file changed, 123 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-exynos5.c
> > b/drivers/i2c/busses/i2c-exynos5.c
> > index 97d4f3ac0abd..6a05af550aa5 100644
> > --- a/drivers/i2c/busses/i2c-exynos5.c
> > +++ b/drivers/i2c/busses/i2c-exynos5.c
> > @@ -22,6 +22,8 @@
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> >
> >  /*
> >   * HSI2C controller from Samsung supports 2 modes of operation @@
> > -166,9 +168,21 @@
> >
> >  #define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100))
> >
> > +/* USI(Universal Serial Interface) Register map */
> > +#define USI_CON					0xc4
> > +#define USI_OPTION				0xc8
> > +
> > +/* USI(Universal Serial Interface) Register bits */
> > +#define USI_CON_RESET				BIT(0)
> > +
> > +/* SYSREG Register bit */
> > +#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
> > +#define SYSREG_I2C_SW_CONF			BIT(2)
> > +
> >  enum i2c_type_exynos {
> >  	I2C_TYPE_EXYNOS5,
> >  	I2C_TYPE_EXYNOS7,
> > +	I2C_TYPE_EXYNOSAUTOV9,
> 
> The type in driver could stay USI, I only wanted the compatible to be using SoC product ID/number. But
> current AUTOV9 is fine as well.
> 
> >  };
> >
> >  struct exynos5_i2c {
> > @@ -199,6 +213,10 @@ struct exynos5_i2c {
> >
> >  	/* Version of HS-I2C Hardware */
> >  	const struct exynos_hsi2c_variant *variant;
> > +
> > +	/* USI sysreg info */
> > +	struct regmap		*usi_sysreg;
> > +	unsigned int		usi_offset;
> >  };
> >
> >  /**
> > @@ -212,24 +230,34 @@ struct exynos5_i2c {
> >   */
> >  struct exynos_hsi2c_variant {
> >  	unsigned int		fifo_depth;
> > +	unsigned int		has_usi;
> 
> Sorry for not noticing it earlier, but this should be bool.


Sorry, I miss latest change in 
"tty: serial: samsung: Improve naming for common macro "

I will change it to bool.

> 
> >  	enum i2c_type_exynos	hw;
> >  };
> >
> >  static const struct exynos_hsi2c_variant exynos5250_hsi2c_data = {
> >  	.fifo_depth	= 64,
> > +	.has_usi	= 0,
> 
> And this should be "false".
> 
> >  	.hw		= I2C_TYPE_EXYNOS5,
> >  };
> >
> >  static const struct exynos_hsi2c_variant exynos5260_hsi2c_data = {
> >  	.fifo_depth	= 16,
> > +	.has_usi	= 0,
> >  	.hw		= I2C_TYPE_EXYNOS5,
> >  };
> >
> >  static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
> >  	.fifo_depth	= 16,
> > +	.has_usi	= 0,
> >  	.hw		= I2C_TYPE_EXYNOS7,
> >  };
> >
> > +static const struct exynos_hsi2c_variant exynosautov9_hsi2c_data = {
> > +	.fifo_depth	= 64,
> > +	.has_usi	= 1,
> > +	.hw		= I2C_TYPE_EXYNOSAUTOV9,
> > +};
> > +
> >  static const struct of_device_id exynos5_i2c_match[] = {
> >  	{
> >  		.compatible = "samsung,exynos5-hsi2c", @@ -243,6 +271,9 @@ static
> > const struct of_device_id exynos5_i2c_match[] = {
> >  	}, {
> >  		.compatible = "samsung,exynos7-hsi2c",
> >  		.data = &exynos7_hsi2c_data
> > +	}, {
> > +		.compatible = "samsung,exynosautov9-hsi2c",
> > +		.data = &exynosautov9_hsi2c_data
> >  	}, {},
> >  };
> >  MODULE_DEVICE_TABLE(of, exynos5_i2c_match); @@ -281,6 +312,31 @@
> > static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
> >  		i2c->op_clock;
> >  	int div, clk_cycle, temp;
> >
> > +	/* In case of HSI2C controllers in EXYNOSAUTOV9
> 
> Linux coding comment please, so with a separate /* :
> 
> /*
>  * In case of....

I miss this.
I will change it next version.

> 
> Rest looks good, thanks for the changes!
> 
> Best regards,
> Krzysztof

Thanks
Jaewon Kim