>> + /* Enable the lock bits on all PLLs */
>> + for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
>> + val = readl(reg + pll_regs[i]);
>> + val |= BIT(29);
>
>Having a define for that would be nice here
>
>> + writel(val, reg + pll_regs[i]);
>> + }
>> +
>> + /*
>> + * In order to pass the EMI certification, the SDM function of
>> + * the peripheral 1 bus is enabled, and the frequency is still
>> + * calculated using the previous division factor.
>> + */
>> + writel(0xd1303333, reg + SUN50I_A100_PLL_PERIPH1_PATTERN0_REG);
>
>Same here

Having a define? I don’t quite understand what you mean,
can you give me an example?

MBR,
Yangtao

2020-06-03 09:46:48

by Frank Lee

[permalink] [raw]

Subject: 回复：[PATCH 2/4] pinctrl: sunxi: add support for t he Allwinner A100 pin controller

>> + SUNXI_PIN(SUNXI_PINCTRL_PIN(F, 0),
>> + SUNXI_FUNCTION(0x0, "gpio_in"),
>> + SUNXI_FUNCTION(0x1, "gpio_out"),
>> + SUNXI_FUNCTION(0x2, "mmc0"), /* D1 */
>> + SUNXI_FUNCTION(0x3, "jtag"), /* MS1 */
>> + SUNXI_FUNCTION(0x4, "jtag"), /* MS_GPU */
>
>We should use another name here, since the code will just pick the first one and
>ignore the second. What about jtag-gpu?

The underscores are used in front, so changing it to jtag_gpu may be more consistent.

>> + SUNXI_PIN(SUNXI_PINCTRL_PIN(B, 7),
>> + SUNXI_FUNCTION(0x0, "gpio_in"),
>> + SUNXI_FUNCTION(0x1, "gpio_out"),
>> + SUNXI_FUNCTION(0x2, "spdif"), /* DIN */
>> + SUNXI_FUNCTION(0x3, "i2s0"), /* DOUT0 */
>> + SUNXI_FUNCTION(0x4, "i2s0"), /* DIN1 */
>
>I guess the second one would be i2s1?

No, each i2s may have many inputs and outputs.

SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 19),
SUNXI_FUNCTION(0x0, "gpio_in"),
SUNXI_FUNCTION(0x1, "gpio_out"),
SUNXI_FUNCTION(0x2, "cir0"), /* IN */
SUNXI_FUNCTION(0x3, "i2s3_dout3"), /* DOUT3 */
SUNXI_FUNCTION(0x4, "i2s3_din3"), /* DIN3 */
SUNXI_FUNCTION(0x5, "ledc"),
SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 19)),

Considering that the same pin has multiple same functions,
so add a suffix, like i2s3_dout3 and i2s3_din3?

Or specify muxsel in the device tree may be another solution.

>> + SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 2),
>> + SUNXI_FUNCTION(0x0, "gpio_in"),
>> + SUNXI_FUNCTION(0x1, "gpio_out"),
>> + SUNXI_FUNCTION(0x2, "i2c1"), /* SCK */
>> + SUNXI_FUNCTION(0x3, "cpu"), /* CUR_W */
>
>What is "cpu" supposed to be?

As far as I know, this is a function that has never been used.
The hardware guy said that it can be used to control the CPU to reduce the current.

Thx,
Yangtao

2020-06-03 09:54:16

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 1/4] clk: sunxi-ng: add support for the Allwinner A100 CCU

On Wed, Jun 3, 2020 at 5:42 PM 李扬韬 <[email protected]> wrote:
>
> >> + /* Enable the lock bits on all PLLs */
> >> + for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
> >> + val = readl(reg + pll_regs[i]);
> >> + val |= BIT(29);
> >
> >Having a define for that would be nice here
> >
> >> + writel(val, reg + pll_regs[i]);
> >> + }
> >> +
> >> + /*
> >> + * In order to pass the EMI certification, the SDM function of
> >> + * the peripheral 1 bus is enabled, and the frequency is still
> >> + * calculated using the previous division factor.
> >> + */
> >> + writel(0xd1303333, reg + SUN50I_A100_PLL_PERIPH1_PATTERN0_REG);
> >
> >Same here
>
> Having a define? I don’t quite understand what you mean,
> can you give me an example?

What Maxime means is that 0xd1303333 is a magic number.
It is better to make a properly named macro, or many macros
that you then bitwise-OR together. So you should make macros
for each bitfield in that register, which would likely include
the SDM calculation factors, the enable bit, and any other fields.

ChenYu

> MBR,
> Yangtao

2020-06-05 15:17:39

On Tue, Jun 16, 2020 at 11:27 AM Frank Lee <[email protected]> wrote:
> HI Chen-Yu, Linus,
> On Fri, Jun 5, 2020 at 11:13 PM Maxime Ripard <[email protected]> wrote:

> > > Considering that the same pin has multiple same functions,
> > > so add a suffix, like i2s3_dout3 and i2s3_din3?
> > >
> > > Or specify muxsel in the device tree may be another solution.
> >
> > Having muxsel is not really an option. We have two sets of bindings to
> > maintain already, adding a third one would make it fairly hard to
> > maintain. And the second binding we support is the generic pinctrl
> > binding, so I'm not really sure why we would want to move away from
> > that.
> >
> > And I'm not really fond of having a suffix either. It kind of breaks the
> > consistency we had so far, and ideally I'd like to keep that.
> >
> > Chen-Yu, Linus? Any input on that one?

This is entirely up to the driver maintainer. They need to keep the
code together and the naming consistent with how they perceive the
hardware works and/or is intended to work.

Yours,
Linus Walleij