Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c70c08b4-be67-14da-be25-5473ffd0ca7d@kernel.org>
Date:   Wed, 30 Aug 2023 04:18:02 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v1 1/2] extcon: add Realtek DHC RTD SoC Type-C driver
To:     =?UTF-8?B?U3RhbmxleSBDaGFuZ1vmmIzogrLlvrdd?= 
        <stanley_chang@realtek.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>
Cc:     Chanwoo Choi <cw00.choi@samsung.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
References: <20230822102846.4683-1-stanley_chang@realtek.com>
 <af247603-6a8d-7c05-4342-c6f615a7f508@kernel.org>
 <2df3dc449c894e50b126a1b6941eb4d7@realtek.com>
From:   Chanwoo Choi <chanwoo@kernel.org>
Content-Language: en-US
In-Reply-To: <2df3dc449c894e50b126a1b6941eb4d7@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Stanley,

On 23. 8. 29. 20:15, Stanley Chang[昌育德] wrote:
> Hi Chanwoo,
> 
>>> +static int rtd129x_switch_type_c_plug_config(struct type_c_data *type_c,
>>> +                                          int dr_mode, int cc)
>>> +{
>>> +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL_CC1_0;
>>> +     int val_cc;
>>> +
>>> +#define TYPE_C_EN_SWITCH BIT(29)
>>> +#define TYPE_C_TXRX_SEL (BIT(28) | BIT(27))
>>> +#define TYPE_C_SWITCH_MASK (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
>>> +#define TYPE_C_ENABLE_CC1 TYPE_C_EN_SWITCH
>>> +#define TYPE_C_ENABLE_CC2 (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
>>> +#define TYPE_C_DISABLE_CC ~TYPE_C_SWITCH_MASK
>>> +
>>> +     val_cc = readl(reg);
>>
>> I'd like you to use regmap interface to access the register
>> by using regmap_read, regmap_write. You can create the regmap instance
>> via devm_regmap_init_mmio() on probe instead of using 'type_c->reg_base'
>> at the multipe point.
>>
>> For example,
>>         struct regmap_config rtk_regmap_config = {
>>                 .reg_bits = 32,
>>                 .val_bits = 32,
>>         };
>>
>>         void __iomem *base;
>>
>>         base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>         if (IS_ERR(base))
>>                 return PTR_ERR(base);
>>
>>         regmap = devm_regmap_init_mmio(dev, base,
>> &rtk_regmap_config);
>>
>>         ---
>>
>>         And then just call regmap_read without any calculation between
>>         base address and specific register.
>>
>>         regmap_read(regmap, USB_TYPEC_CTRL_CC1_0)
>>
> 
> I studied mmio's regmap.
> 
> It only changed one encoding method. And simplifies the calculation between the base address and the specific register.

The regmap provides the consistent interface to access register
regardless of mmio/i2c/spi and so on. It is the advantage of regmap.

And regmap provides the like register dump via debugfs,
is able to specify the kind of register like writable, readable, volatile type.
It is possible to specify the more detailed register information in device driver
in order to improve the readability. It allows you to express more detail of this device
than just working.


> If the register is 32-bit aligned, other operations look the same as readl/writel.
> I think regmap is more simplified if the read registers are not 32-bit aligned, e.g. nvmem read/write.

I'm sorry. I don't understand of what is accurate meaning.
Could you please explain your opinion more detailed with example of this patch?

> So it would be more intuitive for me to keep writel/readl here
> 
>>
>>> +     val_cc &= ~TYPE_C_SWITCH_MASK;
>>> +
>>> +     if (cc == DISABLE_CC) {
>>> +             val_cc &= TYPE_C_DISABLE_CC;
>>> +     } else if (cc == ENABLE_CC1) {
>>> +             val_cc |= TYPE_C_ENABLE_CC1;
>>> +     } else if (cc == ENABLE_CC2) {
>>> +             val_cc |= TYPE_C_ENABLE_CC2;
>>> +     } else {
>>> +             dev_err(type_c->dev, "%s: Error cc setting cc=0x%x\n",
>> __func__, cc);
>>> +             return -EINVAL;
>>> +     }
>>> +     writel(val_cc, reg);
>>> +
> 
> Thanks,
> Stanley

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi