Re: [PATCH v4 1/2] mfd: devicetree: add bindings for Atmel Flexcom

[Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>]

Le 21/07/2015 17:10, Lee Jones a écrit :
> On Tue, 21 Jul 2015, Cyrille Pitchen wrote:
> 
>> Hi Lee,
>>
>> Le 21/07/2015 11:09, Lee Jones a écrit :
>>> On Mon, 22 Jun 2015, Cyrille Pitchen wrote:
>> ...
>>>> +- atmel,flexcom-mode: shall be a string among { "spi", "usart", "i2c", "twi" }.
>>>> +  "i2c" and "twi" are synonymous.
>>>
>>> Please use a numerical value instead.  You can then define it in
>>> include/dt-bindings/*
>>>
>>> This is only required if you supply more than one child node.  Is that
>>> the plan?  I ask because you only have one child node in the example
>>> below.
>>>
>> The "atmel,flexcom-mode" property is always required. The reset value of the
>> Operating Mode bit field inside the Flexcom Mode Register is 0 for "no serial
>> device". So none of the 3 embedded serial devices is clocked and all of their
>> registers are inaccessible (read as zero).
>> Before using any of the 3 serial devices, the Operating Mode must be set
>> accordingly by updating the Mode Register. Once done, only the selected serial
>> device is clocked. Also the external I/O lines of the Flexcom are muxed to
>> reach the selected serial device.
>>
>> The muxing is given by the following table:
>>
>> Flexcom pin    USART pin    SPI pin    I2C pin
>>         IO0          TXD       MOSI        TWD
>>         IO1          RXD       MISO       TWCK
>>         IO2          SCK       SPCK          -
>>         IO3          CTS      NPCS0          - 
>>         IO4          RTS      NPCS1          -
>>
>>
>> Bus conflicting configuration:
>>  ______________________________________
>> |               Flexcom                |
>> |  _____         _____         _____   |
>> | |     |__CLK__| SPI |__I/O__|     |  |    ___________       ___________
>> | |     |       |_____|       |     |  |   | SPI Slave |     | I2C Slave |
>> | | CLK |                     | I/O |  |   |___________|     |___________|
>> |_| MUX |        _____        | MUX |__|_________|________________|
>> | |     |__CLK__| I2C |__I/O__|     |  |             shared bus
>> | |_____|       |_____|       |_____|  |
>> |______________________________________|
>>
>>
>> For instance, it is very unlikely to connect both I2C and SPI slaves to
>> the very same Flexcom because I/O lines are shared. The operating mode is
>> selected once for all during the boot and will never change during runtime.
>> That's why the Flexcom DT node should have exactly one child.
> 
> You have contradicted your self here.  Firstly you say that the
> "atmel,flexcom-mode" property is always required, then you say that
> the DT node should only ever have one child.  If the latter is true,
> then you can infer which mode Flexcom needs to be in by which node is
> present.
> 
OK, you're right. So on the next series (v6), I will simply remove the
"atmel,flexcom-mode" property from the DT bindings. Then, the driver will
look for the first available child node which "compatible" property contains
one of the substrings "-usart", "-spi" or "-i2c", setting the Flexcom operating
mode accordingly. This child node should be unique.

>>>> +Example:
>>>> +
>>>> +flx0: flexcom@f8034000 {
>>>
>>> What references this node?  If nothing, then you can remove the label.
>> OK, I will remove the label in the next series.
>>>
>>>> +	compatible = "atmel,sama5d2-flexcom";
>>>> +	reg = <0xf8034000 0x200>;
>>>> +	clocks = <&flx0_clk>;
>>>> +	#address-cells = <1>;
>>>> +	#size-cells = <1>;
>>>> +	ranges = <0x0 0xf8034000 0x800>;
>>>> +	atmel,flexcom-mode = "spi";
>>>> +
>>>> +	spi@f8034400 {
>>>> +		compatible = "atmel,at91rm9200-spi";
>>>> +		reg = <0x400 0x200>;
>>>> +		interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&pinctrl_flx0_ioset1>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +		clocks = <&flx0_clk>;
>>>> +		clock-names = "spi_clk";
>>>> +		atmel,fifo-size = <32>;
>>>> +
>>>> +		mtd_dataflash@0 {
>>>> +			compatible = "atmel,at25f512b";
>>>> +			reg = <0>;
>>>> +			spi-max-frequency = <20000000>;
>>>> +		};
>>>> +	};
>>>> +};
>>>
>>
>> Thanks for your review :)
>>
>> Best Regards,
>>
>> Cyrille
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html