Re: [PATCH v1 3/3] soc: samsung: usi: implement support for USIv1

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 10:40:15PM +0200, Ivaylo Ivanov wrote:
>  /* USIv2: System Register: SW_CONF register bits */
>  #define USI_V2_SW_CONF_NONE	0x0
>  #define USI_V2_SW_CONF_UART	BIT(0)
> @@ -34,7 +46,8 @@
>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>  
>  enum exynos_usi_ver {
> -	USI_VER2 = 2,
> +	USI_VER1 = 1,
> +	USI_VER2,
>  };
>  
>  struct exynos_usi_variant {
> @@ -66,6 +79,16 @@ struct exynos_usi_mode {
>  	unsigned int val;		/* mode register value */
>  };
>  
> +static const struct exynos_usi_mode exynos_usi_v1_modes[] = {
> +	[USI_V1_NONE]		= { .name = "none", .val = USI_V1_SW_CONF_NONE },
> +	[USI_V1_I2C0]		= { .name = "i2c0", .val = USI_V1_SW_CONF_I2C0 },
> +	[USI_V1_I2C1]		= { .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
> +	[USI_V1_I2C0_1]		= { .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
> +	[USI_V1_SPI]		= { .name = "spi", .val = USI_V1_SW_CONF_SPI },
> +	[USI_V1_UART]		= { .name = "uart", .val = USI_V1_SW_CONF_UART },
> +	[USI_V1_UART_I2C1]	= { .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },

Now I see why you duplicated the IDs... With my approach your code here
is even simpler. Allows to drop USI_VER1 as well.


> +};
> +
>  static const struct exynos_usi_mode exynos_usi_modes[] = {
>  	[USI_V2_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
>  	[USI_V2_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
> @@ -83,11 +106,24 @@ static const struct exynos_usi_variant exynos850_usi_data = {
>  	.clk_names	= exynos850_usi_clk_names,
>  };
>  
> +static const struct exynos_usi_variant exynos8895_usi_data = {
> +	.ver		= USI_VER1,
> +	.sw_conf_mask	= USI_V1_SW_CONF_MASK,
> +	.min_mode	= USI_V1_NONE,
> +	.max_mode	= USI_V1_UART_I2C1,
> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
> +	.clk_names	= exynos850_usi_clk_names,
> +};
> +
>  static const struct of_device_id exynos_usi_dt_match[] = {
>  	{
>  		.compatible = "samsung,exynos850-usi",
>  		.data = &exynos850_usi_data,
>  	},
> +	{
> +		.compatible = "samsung,exynos8895-usi",
> +		.data = &exynos8895_usi_data,
> +	},
>  	{ } /* sentinel */
>  };
>  MODULE_DEVICE_TABLE(of, exynos_usi_dt_match);
> @@ -105,18 +141,32 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>  {
>  	unsigned int val;
>  	int ret;
> +	const char *name;
>  
> +	usi->mode = mode;
>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>  		return -EINVAL;
>  
> -	val = exynos_usi_modes[mode].val;
> +	switch (usi->data->ver) {
> +	case USI_VER1:
> +		val = exynos_usi_v1_modes[mode].val;
> +		name = exynos_usi_v1_modes[usi->mode].name;
> +		break;
> +	case USI_VER2:
> +		val = exynos_usi_modes[mode].val;
> +		name = exynos_usi_modes[usi->mode].name;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>  				 usi->data->sw_conf_mask, val);
> +

No, why? Drop.

Best regards,
Krzysztof