| Message ID | 20230726090409.16606-1-tychang@realtek.com |
|---|---|
| Headers | show |
| Series | Add pinctrl driver support for Realtek DHC SoCs | expand |
Hi TY Chang, thanks for your patch! On Wed, Jul 26, 2023 at 11:06 AM TY Chang <tychang@realtek.com> wrote: > Add device tree bindings for RTD1315E. > > Signed-off-by: TY Chang <tychang@realtek.com> Maybe you could write a short paragraph about the RTD1315E so we know what this is? I guess it is some SoC with some intended use case? (...) > +description: | > + Binding for Realtek DHC RTD1315E SoC pin control. Same text should go here in that case. > + realtek,pdrive: > + description: | > + An integer describing the level to adjust PMOS output driving capability. > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 7 > + > + realtek,ndrive: > + description: | > + An integer describing the level to adjust NMOS output driving capability. > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 7 I would rename these realtek,drive-strength-p and realtek,drive-strength-n. You need to explain what is meant with PMOS and NMOS here. If it is what I think it is, I think some ASCII art would be handy! You can reuse my ASCII art from Documentation/driver-api/gpio/driver.rst: VDD | OD ||--+ +--/ ---o|| P-MOS-FET | ||--+ IN --+ +----- out | ||--+ +--/ ----|| N-MOS-FET OS ||--+ | GND Maybe you wanna delete the OD switch if these drivers don't support that. What does the values 0..7 actually correspond to? Is it the number of transistors/driver stages simply? Then write that. We need to think whether this is so generically useful that it should simply be drive-strength-pmos and drive-strength-nmos, simply put, as other SoCs may implement the same. What do people think? > + realtek,dcycle: > + description: | > + An integer describing the level to adjust output duty cycle. > + Valid arguments are described as below: > + 0: 0ns > + 2: + 0.25ns > + 3: + 0.5ns > + 4: -0.25ns > + 5: -0.5ns > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [ 0, 2, 3, 4, 5 ] This does not explain the duty cycle of *what*? It looks really useful so please explain thoroughly what it does. I guess this is not PWM because then you could use PIN_CONFIG_MODE_PWM. Yours, Linus Walleij
Hi TY Chang, thanks for the patches! On Wed, Jul 26, 2023 at 11:06 AM TY Chang <tychang@realtek.com> wrote: > These patches add the bindings and the pinctrl drivers for Realtek > DHC(Digital Home Center) RTD SoCs(RTD1619B, RTD1319D and RTD1315E). I really like the architecture with a central driver an modules for each SoC that makes this nicely maintainable in the future. There are some questions around the NMOS/PMOS totempole config (I guess that is what it is) so we need to hash that out into the documentation, then I think the rest falls into place pretty naturally. Yours, Linus Walleij
Hi Rob, >On Wed, Jul 26, 2023 at 05:04:07PM +0800, TY Chang wrote: >> Add device tree bindings for RTD1315E. >> >> Signed-off-by: TY Chang <tychang@realtek.com> >> --- >> .../pinctrl/realtek,rtd1315e-pinctrl.yaml | 165 ++++++++++++++++++ >> 1 file changed, 165 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/pinctrl/realtek,rtd1315e-pinctrl.yam >> l >> >> diff --git >> a/Documentation/devicetree/bindings/pinctrl/realtek,rtd1315e-pinctrl.y >> aml >> b/Documentation/devicetree/bindings/pinctrl/realtek,rtd1315e-pinctrl.y >> aml >> new file mode 100644 >> index 000000000000..b8fb6335ed65 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/realtek,rtd1315e-pinct >> +++ rl.yaml >> @@ -0,0 +1,165 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2023 >> +Realtek Semiconductor Corporation %YAML 1.2 >> +--- >> +$id: >> +http://devicetree.org/schemas/pinctrl/realtek,rtd1315e-pinctrl.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Realtek DHC RTD1315E Pin Controller >> + >> +maintainers: >> + - TY Chang <tychang@realtek.com> >> + >> +description: | >> + Binding for Realtek DHC RTD1315E SoC pin control. >> + >> +properties: >> + compatible: >> + const: realtek,rtd13xxe-pinctrl > >Don't use wildcards in compatible strings. I will fix it in the next version. > >> + >> + reg: >> + maxItems: 1 >> + >> +patternProperties: >> + '^.*$': >> + if: >> + type: object > >For new bindings, define a suffix (e.g. "-pins") for the node name. I will fix it. > >> + then: >> + allOf: >> + - $ref: pincfg-node.yaml# >> + - $ref: pinmux-node.yaml# >> + >> + properties: >> + pins: >> + items: >> + enum: [ gpio_0, gpio_1, emmc_rst_n, emmc_dd_sb, emmc_clk, >emmc_cmd, >> + gpio_6, gpio_7, gpio_8, gpio_9, gpio_10, gpio_11, >gpio_12, >> + gpio_13, gpio_14, gpio_15, gpio_16, gpio_17, gpio_18, >gpio_19, >> + gpio_20, emmc_data_0, emmc_data_1, emmc_data_2, >usb_cc2, gpio_25, >> + gpio_26, gpio_27, gpio_28, gpio_29, gpio_30, gpio_31, >gpio_32, >> + gpio_33, gpio_34, gpio_35, hif_data, hif_en, hif_rdy, >hif_clk, >> + gpio_dummy_40, gpio_dummy_41, gpio_dummy_42, >gpio_dummy_43, >> + gpio_dummy_44, gpio_dummy_45, gpio_46, gpio_47, >gpio_48, gpio_49, >> + gpio_50, usb_cc1, emmc_data_3, emmc_data_4, ir_rx, >ur0_rx, ur0_tx, >> + gpio_57, gpio_58, gpio_59, gpio_60, gpio_61, gpio_62, >gpio_dummy_63, >> + gpio_dummy_64, gpio_dummy_65, gpio_66, gpio_67, >gpio_68, gpio_69, >> + gpio_70, gpio_71, gpio_72, gpio_dummy_73, >emmc_data_5, emmc_data_6, >> + emmc_data_7, gpio_dummy_77, gpio_78, gpio_79, >gpio_80, gpio_81, >> + ur2_loc, gspi_loc, hi_width, sf_en, arm_trace_dbg_en, >> + ejtag_aucpu_loc, ejtag_acpu_loc, ejtag_vcpu_loc, >ejtag_scpu_loc, >> + dmic_loc, vtc_dmic_loc, vtc_tdm_loc, vtc_i2si_loc, >tdm_ai_loc, >> + ai_loc, spdif_loc, hif_en_loc, scan_switch, wd_rset, >boot_sel, >> + reset_n, testmode ] >> + >> + function: >> + enum: [ gpio, nf, emmc, ao, gspi_loc0, gspi_loc1, uart0, uart1, >> + uart2_loc0, uart2_loc1, i2c0, i2c1, i2c4, i2c5, pcie1, >> + etn_led, etn_phy, spi, pwm0_loc0, pwm0_loc1, >pwm1_loc0, >> + pwm1_loc1, pwm2_loc0, pwm2_loc1, pwm3_loc0, >pwm3_loc1, >> + spdif_optical_loc0, spdif_optical_loc1, usb_cc1, usb_cc2, >> + sd, dmic_loc0, dmic_loc1, ai_loc0, ai_loc1, tdm_ai_loc0, >> + tdm_ai_loc1, hi_loc0, hi_m, vtc_i2so, vtc_i2si_loc0, >> + vtc_i2si_loc1, vtc_dmic_loc0, vtc_dmic_loc1, >vtc_tdm_loc0, >> + vtc_tdm_loc1, dc_fan, pll_test_loc0, pll_test_loc1, >> + ir_rx, uart2_disable, gspi_disable, hi_width_disable, >> + hi_width_1bit, sf_disable, sf_enable, scpu_ejtag_loc0, >> + scpu_ejtag_loc1, scpu_ejtag_loc2, scpu_ejtag_loc3, >> + acpu_ejtag_loc0, acpu_ejtag_loc1, acpu_ejtag_loc2, >> + vcpu_ejtag_loc0, vcpu_ejtag_loc1, vcpu_ejtag_loc2, >> + aucpu_ejtag_loc0, aucpu_ejtag_loc1, aucpu_ejtag_loc2, >> + gpu_ejtag, iso_tristate, dbg_out0, dbg_out1, >standby_dbg, >> + spdif, arm_trace_debug_disable, >arm_trace_debug_enable, >> + aucpu_ejtag_disable, acpu_ejtag_disable, >vcpu_ejtag_disable, >> + scpu_ejtag_disable, vtc_dmic_loc_disable, >vtc_tdm_disable, >> + vtc_i2si_disable, tdm_ai_disable, ai_disable, spdif_disable, >> + hif_disable, hif_enable, test_loop, pmic_pwrup ] >> + >> + >> + drive-strength: >> + enum: [4, 8] >> + >> + bias-pull-down: true >> + >> + bias-pull-up: true >> + >> + bias-disable: true >> + >> + input-schmitt-enable: true >> + >> + input-schmitt-disable: true >> + >> + drive-push-pull: true >> + >> + power-source: >> + description: | >> + Valid arguments are described as below: >> + 0: power supply of 1.8V >> + 1: power supply of 3.3V >> + enum: [0, 1] >> + >> + realtek,pdrive: >> + description: | >> + An integer describing the level to adjust PMOS output driving >capability. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 7 >> + >> + realtek,ndrive: >> + description: | >> + An integer describing the level to adjust NMOS output driving >capability. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 7 >> + >> + realtek,dcycle: >> + description: | >> + An integer describing the level to adjust output duty cycle. >> + Valid arguments are described as below: >> + 0: 0ns >> + 2: + 0.25ns >> + 3: + 0.5ns >> + 4: -0.25ns >> + 5: -0.5ns >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [ 0, 2, 3, 4, 5 ] >> + >> + required: >> + - pins >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + pinctrl@4e000 { >> + compatible = "realtek,rtd13xxe-pinctrl"; >> + reg = <0x4e000 0x130>; >> + >> + emmc_pins_hs200 { > >Don't use '_' in node names. I will fix it. > >> + pins = "emmc_clk", >> + "emmc_cmd", >> + "emmc_data_0", >> + "emmc_data_1", >> + "emmc_data_2", >> + "emmc_data_3", >> + "emmc_data_4", >> + "emmc_data_5", >> + "emmc_data_6", >> + "emmc_data_7"; >> + function = "emmc"; >> + realtek,pdrive = <0x2>; >> + realtek,ndrive = <0x2>; >> + }; >> + >> + i2c_pins_0 { >> + pins = "gpio_12", >> + "gpio_13"; >> + function = "i2c0"; >> + drive-strength = <4>; >> + }; >> + }; >> -- >> 2.41.0 >> > Thanks, TY Chang
Wed, Jul 26, 2023 at 05:04:04PM +0800, TY Chang kirjoitti: > Add RTD1315E support using realtek common pinctrl driver. ... > +config PINCTRL_RTD1315E > + tristate "Realtek DHC 1315E pin controller driver" > + depends on PINCTRL_RTD Why not select and the above be hidden symbol? ... > +#include <linux/module.h> > +#include <linux/of.h> Not used. Use correct headers > +#include <linux/platform_device.h> + Blank line > +#include <linux/pinctrl/pinctrl.h> Make use of struct pinconfig, struct pinfunction and respective PINCTRL_PIN*() macros. ... > +#define RTD1315E_GROUP(_name) \ > + { \ > + .name = # _name, \ > + .pins = rtd1315e_ ## _name ## _pins, \ > + .num_pins = ARRAY_SIZE(rtd1315e_ ## _name ## _pins), \ > + } NIH PINCTRL_PINGROUP(). ... > +#define RTD1315E_FUNC(_name) \ > + { \ > + .name = # _name, \ > + .groups = rtd1315e_ ## _name ## _groups, \ > + .num_groups = ARRAY_SIZE(rtd1315e_ ## _name ## _groups), \ > + } NIH PINCTRL_PINFUNCTION(). ... > +static const struct of_device_id rtd1315e_pinctrl_of_match[] = { > + { .compatible = "realtek,rtd13xxe-pinctrl", }, > + {}, No comma for the terminator entry. > +}; ... Same comments for all new drivers in the series.
Hi Linus, Thanks for your review. > >Hi TY Chang, > >thanks for your patch! > >On Wed, Jul 26, 2023 at 11:06 AM TY Chang <tychang@realtek.com> wrote: > >> Add device tree bindings for RTD1315E. >> >> Signed-off-by: TY Chang <tychang@realtek.com> > >Maybe you could write a short paragraph about the RTD1315E so we know what >this is? I guess it is some SoC with some intended use case? > I will add it in the next version. >(...) >> +description: | >> + Binding for Realtek DHC RTD1315E SoC pin control. > >Same text should go here in that case. > >> + realtek,pdrive: >> + description: | >> + An integer describing the level to adjust PMOS output driving >capability. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 7 >> + >> + realtek,ndrive: >> + description: | >> + An integer describing the level to adjust NMOS output driving >capability. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 7 > >I would rename these realtek,drive-strength-p and realtek,drive-strength-n. Sure, I think naming them like that makes it clearer. > >You need to explain what is meant with PMOS and NMOS here. If it is what I think >it is, I think some ASCII art would be handy! > >You can reuse my ASCII art from Documentation/driver-api/gpio/driver.rst: > > VDD > | > OD ||--+ > +--/ ---o|| P-MOS-FET > | ||--+ > IN --+ +----- out > | ||--+ > +--/ ----|| N-MOS-FET > OS ||--+ > | > GND > >Maybe you wanna delete the OD switch if these drivers don't support that. > >What does the values 0..7 actually correspond to? Is it the number of >transistors/driver stages simply? Then write that. > >We need to think whether this is so generically useful that it should simply be >drive-strength-pmos and drive-strength-nmos, simply put, as other SoCs may >implement the same. What do people think? > I will add these in the next version. The values 0..7 is the level of the driving strength. These leves can impact the rising/falling time of the waveform, assisting in achieving the desired transfer speed. >> + realtek,dcycle: >> + description: | >> + An integer describing the level to adjust output duty cycle. >> + Valid arguments are described as below: >> + 0: 0ns >> + 2: + 0.25ns >> + 3: + 0.5ns >> + 4: -0.25ns >> + 5: -0.5ns >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [ 0, 2, 3, 4, 5 ] > >This does not explain the duty cycle of *what*? > >It looks really useful so please explain thoroughly what it does. > >I guess this is not PWM because then you could use PIN_CONFIG_MODE_PWM. > This is not PWM. The duty cycle here is to adjust the proportion of positive and negative waveforms, and is adjusted in nanosecond(ns). >Yours, >Linus Walleij > Thanks, TY Chang