Re: [PATCH v5 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-02-28 14:30, Pankaj Bansal wrote:
> This adds device tree binding documentation for generic register based
> multiplexer controlled by a bitfields in a parent device's register range.
> 
> since MMIO mux is a special case of generic register based mux, the
> MMIO mux bindings have been subsumed in these bindings.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx>
> ---
> 
> Notes:
>     V5:
>     - Subsume the mmio-mux.txt bindings in reg-mux.txt
>     V4:
>     - No change
>     V3:
>     - Added the patch in series with the driver patch
>     - Fixed the node value out of bitfield error
>     - removed the "parent register r/w functions" line
>     V2:
>     - Removed syscon reference from txt file
>     - Removed loading zeroes from hex numbers
>     - Fixed the depth of dts nodes
>     - fixed minor formatting errors
> 
>  .../devicetree/bindings/mux/mmio-mux.txt     |  60 --------
>  .../devicetree/bindings/mux/reg-mux.txt      | 128 +++++++++++++++++
>  2 files changed, 128 insertions(+), 60 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> deleted file mode 100644
> index a9bfb4d8b6ac..000000000000
> --- a/Documentation/devicetree/bindings/mux/mmio-mux.txt
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -MMIO register bitfield-based multiplexer controller bindings
> -
> -Define register bitfields to be used to control multiplexers. The parent
> -device tree node must be a syscon node to provide register access.
> -
> -Required properties:
> -- compatible : "mmio-mux"
> -- #mux-control-cells : <1>
> -- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
> -                  pairs, each describing a single mux control.
> -* Standard mux-controller bindings as decribed in mux-controller.txt
> -
> -Optional properties:
> -- idle-states : if present, the state the muxes will have when idle. The
> -		special state MUX_IDLE_AS_IS is the default.
> -
> -The multiplexer state of each multiplexer is defined as the value of the
> -bitfield described by the corresponding register offset and bitfield mask pair
> -in the mux-reg-masks array, accessed through the parent syscon.
> -
> -Example:
> -
> -	syscon {
> -		compatible = "syscon";
> -
> -		mux: mux-controller {
> -			compatible = "mmio-mux";
> -			#mux-control-cells = <1>;
> -
> -			mux-reg-masks = <0x3 0x30>, /* 0: reg 0x3, bits 5:4 */
> -					<0x3 0x40>, /* 1: reg 0x3, bit 6 */
> -			idle-states = <MUX_IDLE_AS_IS>, <0>;
> -		};
> -	};
> -
> -	video-mux {
> -		compatible = "video-mux";
> -		mux-controls = <&mux 0>;
> -
> -		ports {
> -			/* inputs 0..3 */
> -			port@0 {
> -				reg = <0>;
> -			};
> -			port@1 {
> -				reg = <1>;
> -			};
> -			port@2 {
> -				reg = <2>;
> -			};
> -			port@3 {
> -				reg = <3>;
> -			};
> -
> -			/* output */
> -			port@4 {
> -				reg = <4>;
> -			};
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt b/Documentation/devicetree/bindings/mux/reg-mux.txt
> new file mode 100644
> index 000000000000..320eed948caa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
> @@ -0,0 +1,128 @@
> +Generic register bitfield-based multiplexer controller bindings
> +
> +Define register bitfields to be used to control multiplexers. The parent
> +device tree node must be a device node to provide register r/w access.
> +
> +Required properties:
> +- compatible : should be one of
> +	"reg-mux" : if parent device of mux producer is not syscon device
> +	"mmio-mux" : if parent device of mux producer is syscon device
> +- #mux-control-cells : <1>
> +- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
> +                  pairs, each describing a single mux control.
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-states : if present, the state the muxes will have when idle. The
> +		special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state of each multiplexer is defined as the value of the
> +bitfield described by the corresponding register offset and bitfield mask
> +pair in the mux-reg-masks array.
> +
> +Example 1:
> +The parent device of mux producer is not a syscon device.
> +
> +&i2c0 {
> +	fpga@66 { // fpga connected to i2c
> +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c",
> +			     "simple-mfd";
> +		reg = <0x66>;
> +
> +		mux: mux-controller { // Mux Producer

"Mux Producer" is not a concept that exists in any documentation besides
your examples. I do not think the "Mux Producer" / "Mux consumer" comments
adds any value, and think they can just be removed. I felt this from the
start, but it seemed like such a small issue that I tried to ignore it.
But, now you are adding it to the syscon example too, and before I know
it, it will be copied further. And, since there is the issue below in the
example, you need another iteration anyway, so I don't feel toooo bad for
nit-picking this late...

> +			compatible = "reg-mux";
> +			#mux-control-cells = <1>;
> +			mux-reg-masks = <0x54 0xf8>, /* 0: reg 0x54, bits 7:3 */
> +					<0x54 0x07>; /* 1: reg 0x54, bits 2:0 */
> +		};
> +	};
> +};
> +
> +mdio-mux-1 { // Mux consumer
> +	compatible = "mdio-mux-multiplexer";
> +	mux-controls = <&mux 0>;
> +	mdio-parent-bus = <&emdio1>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	mdio@0 {
> +		reg = <0x0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	mdio@8 {
> +		reg = <0x8>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	..
> +	..
> +};
> +
> +mdio-mux-2 { // Mux consumer
> +	compatible = "mdio-mux-multiplexer";
> +	mux-controls = <&mux 1>;
> +	mdio-parent-bus = <&emdio2>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	mdio@0 {
> +		reg = <0x0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	mdio@1 {
> +		reg = <0x1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	..
> +	..
> +};
> +
> +Example 2:
> +The parent device of mux producer is syscon device.
> +
> +syscon {
> +	compatible = "syscon";
> +
> +	mux: mux-controller { // Mux Producer
> +		compatible = "mmio-mux";
> +		#mux-control-cells = <1>;
> +
> +		mux-reg-masks = <0x3 0x30>, /* 0: reg 0x3, bits 5:4 */
> +				<0x3 0x40>, /* 1: reg 0x3, bit 6 */
> +		idle-states = <MUX_IDLE_AS_IS>, <0>;
> +	};
> +};
> +
> +video-mux { // Mux consumer
> +	compatible = "video-mux";
> +	mux-controls = <&mux 0>;
> +
> +	ports {

Not entirely your fault, I know, but this node needs:

		#address-cells = <1>;
		#size-cells = <0>;

Cheers,
Peter

> +		/* inputs 0..3 */
> +		port@0 {
> +			reg = <0>;
> +		};
> +		port@1 {
> +			reg = <1>;
> +		};
> +		port@2 {
> +			reg = <2>;
> +		};
> +		port@3 {
> +			reg = <3>;
> +		};
> +
> +		/* output */
> +		port@4 {
> +			reg = <4>;
> +		};
> +	};
> +};
> +
> 





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux