On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh@xxxxxxxxxx> wrote: > On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote: > > See cover-letter for changelog > > > > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx> > > > > --- > > .../devicetree/bindings/misc/idt_89hpesx.txt | 41 ++++++++++++++++++++++ > > There's not a better location for this? I can't tell because you don't > describe what the device is. > The device is PCIe-switch EEPROM driver with additional debug-interface to access the switch CSRs. EEPROM is accesses via a separate i2c-slave interface of the switch. There might be another place to put the binding file in. There is a special location for EEPROM drivers bindings - Documentation/devicetree/bindings/eeprom/ . But as far as I understood from the files put in there, it's intended for pure EEPROM drivers only. On the other hand the directory I've chosen: Documentation/devicetree/bindings/misc/ mostly intended for some unusual devices. My device isn't usual, since it has CSRs debug-interface as well. Additionally I've found eeprom-93xx46.txt binding file there, which describes EEPROM bindings. Anyway if you find the file should be placed in Documentation/devicetree/bindings/eeprom/ instead, I'll move it, it's not that a big problem. > > 1 file changed, 41 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt > > > > diff --git a/Documentation/devicetree/bindings/misc/idt_89hpesx.txt b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt > > index 0000000..469cc93 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt > > @@ -0,0 +1,41 @@ > > +EEPROM / CSR SMBus-slave interface of IDT 89HPESx devices > > + > > +Required properties: > > + - compatible : should be "<manufacturer>,<type>" > > + Basically there is only one manufacturer: idt, but some > > + compatible devices may be produced in future. Following devices > > + are supported: 89hpes8nt2, 89hpes12nt3, 89hpes24nt6ag2, > > + 89hpes32nt8ag2, 89hpes32nt8bg2, 89hpes12nt12g2, 89hpes16nt16g2, > > + 89hpes24nt24g2, 89hpes32nt24ag2, 89hpes32nt24bg2; > > + 89hpes12n3, 89hpes12n3a, 89hpes24n3, 89hpes24n3a; > > + 89hpes32h8, 89hpes32h8g2, 89hpes48h12, 89hpes48h12g2, > > + 89hpes48h12ag2, 89hpes16h16, 89hpes22h16, 89hpes22h16g2, > > + 89hpes34h16, 89hpes34h16g2, 89hpes64h16, 89hpes64h16g2, > > + 89hpes64h16ag2; > > + 89hpes12t3g2, 89hpes24t3g2, 89hpes16t4, 89hpes4t4g2, > > + 89hpes10t4g2, 89hpes16t4g2, 89hpes16t4ag2, 89hpes5t5, > > + 89hpes6t5, 89hpes8t5, 89hpes8t5a, 89hpes24t6, 89hpes6t6g2, > > + 89hpes24t6g2, 89hpes16t7, 89hpes32t8, 89hpes32t8g2, > > + 89hpes48t12, 89hpes48t12g2. > > + Current implementation of the driver doesn't have any device- > > Driver capabilties are irrelevant to bindings. > Why? I've told in the comment, that the devices actually differ by the CSRs map. Even though it's not reflected in the code at the moment, the CSRs read/write restrictions can be added by some concerned programmer in future. But If I left something like "compatible : idt,89hpesx" device only, it will be problematic to add that functionality. Howbeit If you think it's not necessary and "compatible = idt,89hpesx" is ok, it's perfectly fine for me to make it this way. The property will be even simpler, than current approach. > > + specific functionalities. But since each of them differs > > + by registers mapping, CSRs read/write restrictions can be > > + added in future. > > + - reg : I2C address of the IDT 89HPES device. > > + > > +Optional properties: > > + - read-only : Parameterless property disables writes to the EEPROM > > + - idt,eesize : Size of EEPROM device connected to IDT 89HPES i2c-master bus > > + (default value is 4096 bytes if option isn't specified) > > + - idt,eeaddr : Custom address of EEPROM device > > + (If not specified IDT 89HPESx device will try to communicate > > + with EEPROM sited by default address - 0x50) > > Don't we already have standard EEPROM properties that could be used > here? > If we do, just tell me which one. There are standard options: "compatible, reg, pagesize, read-only". There isn't any connected with EEPROM actual size. Why so? Because standard EEPROM-drivers determine the device size from the compatible-string name. Such approach won't work in this case, because PCIe-switch and it EEPROM are actually two different devices. Look at the chain of the usual platform board design: Host <--- i2c ----> i2c-slave iface |PCIe-switch| i2c-master iface <--- i2c ---> EEPROM As you cas see the Host reaches EEPROM through the set of PCIe-switch i2c-interfaces. In order to properly get data from it my driver needs actual EEPROM size and it address in the i2c-master bus of the PCIe-switch, in addition to the standard reg-field, which is address of PCIe-switch i2c-slave interface and read-only parameter if EEPROM-device has got WP pin asserted. > > + > > +Example: > > + idt_pcie_sw@60 { > > Don't use '_'. > Ok, I won't. > > + compatible = "idt,89hpes12nt3"; > > + reg = <0x60>; > > + read-only; > > + idt,eesize = <65536>; > > + idt,eeaddr = <0x50>; > > + }; > > -- > > 2.6.6 > > Waiting for the respond. Thanks -Sergey -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html