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
>
