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

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 1/3/25 10:29, Krzysztof Kozlowski wrote:
> 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.

We can't really drop USI_VER1, as we'll fall into USIV2-specific code, like so:
if (usi->data->ver == USI_VER2) return exynos_usi_enable(usi);

Thanks for the feedback!

Best regards,
Ivaylo

>
>
>> +};
>> +
>>  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
>