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
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
Hi Chanwoo,
> >>> +#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.
These are all advantages of regmap, I agree.
I haven't delved into regmap yet.
>
> > 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?
Sorry, I misunderstood regmap, I thought he could read register like read otp tables by nvmem regardless of his bitness and size.
So my answer should be incorrect.
Thank,
Stanley