Re: [PATCH 1/7] dt-bindings: pinctrl: Add pinctrl for Sophgo CV1800 series SoC.

[Inochi Amaoto <inochiama@xxxxxxxxxxx>]

On Fri, Jul 05, 2024 at 11:50:09AM GMT, Linus Walleij wrote:
> Hi Inochi,
> 
> thanks for your patch!
> 
> Some comments below:
> 
> On Thu, Jul 4, 2024 at 7:47 AM Inochi Amaoto <inochiama@xxxxxxxxxxx> wrote:
> 
> > Add pinctrl support for Sophgo CV1800 series SoC.
> >
> > Signed-off-by: Inochi Amaoto <inochiama@xxxxxxxxxxx>
> (...)
> > +          bias-pull-up:
> > +            type: boolean
> 
> description: Setting this true will result in how many Ohms of pull up
> and to what voltage?
> 
> > +          bias-pull-down:
> > +            type: boolean
> 
> How many Ohms by default? It's very helpful for designers writing the
> device tree to know this.
> 

Pullup in 79k and pulldown is 87k. I will add these to the description.

> > +          drive-strength:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            minimum: 0
> > +            maximum: 7
> 
> 0,1,  ... 7 ... units of what? What does the values represent?
> 7 mA? I think not. This should be in mA and parsed to any custom
> values.
> 
> If you need more resolution, consider using driver-strength-microamp and
> parse that value instead.
> 

This value depends on the type of the pin. Different pin have different
typical drive strength. I will switch to the real value and check it in
the driver.

> > +          input-schmitt:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            minimum: 0
> > +            maximum: 7
> 
> What is this custom property? We support input-schmitt-enable
> and input-schmitt-disable, if you want to add a new standard property
> input-schmitt, then send a separate patch to
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> and explain what the argument means, it should be in SI units.
> 
> For a full custom property, use sophgo, prefixes.
> 

Thanks for the reminder. I have seen `input-schmitt` in the generic 
pinconf table. I will sumbit a new patch and add this to the 
pincfg-node.

> > +          slew-rate:
> > +            enum: [ 0, 1 ]
> 
> What is slew rate 0 and 1? What does it represent?
> 
> microvolts per second how much (I don't know, just guessing)
> 

According to the document from sophgo, 0 is fast rate and 1 is slow.

> > +          sophgo,bus-holder:
> > +            description: enable bus holder
> 
> This description is a bit too little, we need to know what this thing
> is and what it is
> used for. If it is definied in some other DT binding then reference that file.
> 
> Yours,
> Linus Walleij

Thanks I will improve it.

Regards,
Inochi