| Message ID | 1525943800-14095-4-git-send-email-djw@t-chip.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/05/18 10:16, djw@t-chip.com.cn wrote: > From: Levin Du <djw@t-chip.com.cn> > > Adding a new gpio controller named "gpio-syscon10" to rk3328, providing > access to the pins defined in the syscon GRF_SOC_CON10. This is the GPIO_MUTE pin, right? The public TRM is rather vague, but cross-referencing against the datasheet and schematics implies that it's the "gpiomut_*" part of the GRF bit names which is most significant. It might be worth using a more descriptive name here, since "syscon10" is pretty much meaningless at the board level. Robin. > Boards using these special pins to control regulators or LEDs, can now > utilize existing drivers like gpio-regulator and leds-gpio. > > Signed-off-by: Levin Du <djw@t-chip.com.cn> > > --- > > Changes in v1: > - Split from V0 and add to rk3328.dtsi for general use. > > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > index b8e9da1..73a822d 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > @@ -309,6 +309,12 @@ > mode-loader = <BOOT_BL_DOWNLOAD>; > }; > > + gpio_syscon10: gpio-syscon10 { > + compatible = "rockchip,gpio-syscon"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <0 0x0428 0>; > + }; > }; > > uart0: serial@ff110000 { >
On 2018-05-10 8:50 PM, Robin Murphy wrote: > On 10/05/18 10:16, djw@t-chip.com.cn wrote: >> From: Levin Du <djw@t-chip.com.cn> >> >> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing >> access to the pins defined in the syscon GRF_SOC_CON10. > > This is the GPIO_MUTE pin, right? The public TRM is rather vague, but > cross-referencing against the datasheet and schematics implies that > it's the "gpiomut_*" part of the GRF bit names which is most significant. > > It might be worth using a more descriptive name here, since "syscon10" > is pretty much meaningless at the board level. > > Robin. > Previously I though other bits might be able to reference from syscon10, other than GPIO_MUTE alone. If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as `<&gpio-mute 1>`. Yet other bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which is not good. I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which overrides the properties of bit_count, data_bit_offset and dir_bit_offset in the driver. For example: gpio_mute: gpio-mute { compatible = "rockchip,gpio-syscon"; gpio-controller; #gpio-cells = <2>; gpio,syscon-dev = <0 0x0428 0>; gpio,syscon-bit = <1 1 0>; }; That way, the mute pin is strictly specified as <&gpio_mute 0>, and <&gpio_mute 1> will be invalid. I think that is neat, and consistent with the gpio_mute name. Thanks Levin
On Thu, May 10, 2018 at 4:16 AM, <djw@t-chip.com.cn> wrote: > From: Levin Du <djw@t-chip.com.cn> > > Adding a new gpio controller named "gpio-syscon10" to rk3328, providing > access to the pins defined in the syscon GRF_SOC_CON10. > > Boards using these special pins to control regulators or LEDs, can now > utilize existing drivers like gpio-regulator and leds-gpio. > > Signed-off-by: Levin Du <djw@t-chip.com.cn> > > --- > > Changes in v1: > - Split from V0 and add to rk3328.dtsi for general use. > > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > index b8e9da1..73a822d 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > @@ -309,6 +309,12 @@ > mode-loader = <BOOT_BL_DOWNLOAD>; > }; > > + gpio_syscon10: gpio-syscon10 { GPIO controller nodes should be named just 'gpio'. > + compatible = "rockchip,gpio-syscon"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <0 0x0428 0>; This property is not documented and takes a phandle. > + }; > }; > > uart0: serial@ff110000 { > -- > 2.7.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 10, 2018 at 10:45 PM, Levin Du <djw@t-chip.com.cn> wrote: > On 2018-05-10 8:50 PM, Robin Murphy wrote: >> >> On 10/05/18 10:16, djw@t-chip.com.cn wrote: >>> >>> From: Levin Du <djw@t-chip.com.cn> >>> >>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing >>> access to the pins defined in the syscon GRF_SOC_CON10. >> >> >> This is the GPIO_MUTE pin, right? The public TRM is rather vague, but >> cross-referencing against the datasheet and schematics implies that it's the >> "gpiomut_*" part of the GRF bit names which is most significant. >> >> It might be worth using a more descriptive name here, since "syscon10" is >> pretty much meaningless at the board level. >> >> Robin. >> > Previously I though other bits might be able to reference from syscon10, > other than GPIO_MUTE alone. > If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as > `<&gpio-mute 1>`. Yet other > bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which is > not good. > > I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which overrides > the properties > of bit_count, data_bit_offset and dir_bit_offset in the driver. For No. Once you are describing individual register bits, it is too low level for DT. > example: > > gpio_mute: gpio-mute { > compatible = "rockchip,gpio-syscon"; > gpio-controller; > #gpio-cells = <2>; > gpio,syscon-dev = <0 0x0428 0>; > gpio,syscon-bit = <1 1 0>; > }; > > That way, the mute pin is strictly specified as <&gpio_mute 0>, and > <&gpio_mute 1> will be invalid. > I think that is neat, and consistent with the gpio_mute name. > > Thanks > Levin > > > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-05-11 8:22 PM, Rob Herring wrote: > On Thu, May 10, 2018 at 4:16 AM, <djw@t-chip.com.cn> wrote: >> From: Levin Du <djw@t-chip.com.cn> >> >> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing >> access to the pins defined in the syscon GRF_SOC_CON10. >> >> Boards using these special pins to control regulators or LEDs, can now >> utilize existing drivers like gpio-regulator and leds-gpio. >> >> Signed-off-by: Levin Du <djw@t-chip.com.cn> >> >> --- >> >> Changes in v1: >> - Split from V0 and add to rk3328.dtsi for general use. >> >> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> index b8e9da1..73a822d 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> @@ -309,6 +309,12 @@ >> mode-loader = <BOOT_BL_DOWNLOAD>; >> }; >> >> + gpio_syscon10: gpio-syscon10 { > GPIO controller nodes should be named just 'gpio'. 'gpio' is a general name, and there're already gpio0~gpio3 for pinctrl GPIOs. >> + compatible = "rockchip,gpio-syscon"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + gpio,syscon-dev = <0 0x0428 0>; > This property is not documented and takes a phandle. See PATCH1 which allows fetching syscon from parent node . This is also documented in Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt in PATCH2. Thanks Levin
On 2018-05-11 8:24 PM, Rob Herring wrote: > On Thu, May 10, 2018 at 10:45 PM, Levin Du <djw@t-chip.com.cn> wrote: >> On 2018-05-10 8:50 PM, Robin Murphy wrote: >>> On 10/05/18 10:16, djw@t-chip.com.cn wrote: >>>> From: Levin Du <djw@t-chip.com.cn> >>>> >>>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing >>>> access to the pins defined in the syscon GRF_SOC_CON10. >>> >>> This is the GPIO_MUTE pin, right? The public TRM is rather vague, but >>> cross-referencing against the datasheet and schematics implies that it's the >>> "gpiomut_*" part of the GRF bit names which is most significant. >>> >>> It might be worth using a more descriptive name here, since "syscon10" is >>> pretty much meaningless at the board level. >>> >>> Robin. >>> >> Previously I though other bits might be able to reference from syscon10, >> other than GPIO_MUTE alone. >> If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as >> `<&gpio-mute 1>`. Yet other >> bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which is >> not good. >> >> I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which overrides >> the properties >> of bit_count, data_bit_offset and dir_bit_offset in the driver. For > No. Once you are describing individual register bits, it is too low > level for DT. Okay. So I'll rename it to gpio_mute, and reference the output pin as <&gpio_mute 1>: + // Use <&gpio_mute 1> to ref to GPIO_MUTE pin + gpio_mute: gpio-mute { + compatible = "rockchip,gpio-syscon"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <0 0x0428 0>; + }; }; Thanks Levin
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index b8e9da1..73a822d 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi @@ -309,6 +309,12 @@ mode-loader = <BOOT_BL_DOWNLOAD>; }; + gpio_syscon10: gpio-syscon10 { + compatible = "rockchip,gpio-syscon"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <0 0x0428 0>; + }; }; uart0: serial@ff110000 {