linux-kernel - RE: [PATCH v1 1/2] extcon: add Realtek DHC RTD SoC Type-C driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2df3dc449c894e50b126a1b6941eb4d7@realtek.com>
Date:   Tue, 29 Aug 2023 11:15:49 +0000
From:   Stanley Chang[昌育德] 
        <stanley_chang@...ltek.com>
To:     Chanwoo Choi <chanwoo@...nel.org>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
CC:     Chanwoo Choi <cw00.choi@...sung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v1 1/2] extcon: add Realtek DHC RTD SoC Type-C driver

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.
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.

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