| Message ID | IA1PR20MB49530F0476B98DBB835B344FBBDE2@IA1PR20MB4953.namprd20.prod.outlook.com |
|---|---|
| Headers | show |
| Series | riscv: sophgo: Add pinctrl support for CV1800 series SoC | expand |
Hi Inochi, thanks for your patch! Some comments below: On Thu, Jul 4, 2024 at 7:47 AM Inochi Amaoto <inochiama@outlook.com> wrote: > Add pinctrl support for Sophgo CV1800 series SoC. > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> (...) > + 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. > + 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. > + 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. > + 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) > + 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
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@outlook.com> wrote: > > > Add pinctrl support for Sophgo CV1800 series SoC. > > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > (...) > > + 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