From: Serge Semin <fancer.lancer@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: gregkh@linuxfoundation.org, srinivas.kandagatla@linaro.org,
andrew@lunn.ch, mark.rutland@arm.com,
Sergey.Semin@t-platforms.ru, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file
Date: Mon, 5 Dec 2016 18:25:02 +0300 [thread overview]
Message-ID: <20161205152502.GA17538@mobilestation> (raw)
In-Reply-To: <20161205144621.yxwr7spmsvyilan5@rob-hp-laptop>
On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh@kernel.org> 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@gmail.com>
> >
> > ---
> > .../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
next prev parent reply other threads:[~2016-12-05 15:25 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-02 23:13 [PATCH] Add IDT 89HPESx EEPROM/CSR driver Serge Semin
2016-10-30 13:53 ` Greg KH
2016-11-24 18:42 ` Serge Semin
2016-11-28 22:38 ` [PATCH v2 0/2] eeprom: " Serge Semin
2016-11-28 22:38 ` [PATCH v2 1/2] " Serge Semin
2016-11-29 19:34 ` Greg KH
2016-11-29 19:37 ` Greg KH
2016-11-29 21:16 ` Serge Semin
2016-11-29 21:24 ` Greg KH
2016-11-29 21:45 ` Serge Semin
2016-11-28 22:38 ` [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin
2016-11-29 19:34 ` Greg KH
2016-11-29 21:15 ` Serge Semin
2016-12-05 14:46 ` Rob Herring
2016-12-05 15:25 ` Serge Semin [this message]
2016-12-05 17:27 ` Rob Herring
2016-12-05 19:04 ` Serge Semin
2016-12-09 1:57 ` Serge Semin
2016-12-12 23:04 ` Rob Herring
2016-12-13 14:08 ` Serge Semin
2016-11-29 22:27 ` [PATCH v3 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin
2016-11-29 22:27 ` [PATCH v3 1/2] " Serge Semin
2016-11-29 22:27 ` [PATCH v3 2/2] eeprom: Add IDT 89HPESx driver dts-binding file Serge Semin
2016-12-13 14:22 ` [PATCH v4 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin
2016-12-13 14:22 ` [PATCH v4 1/2] " Serge Semin
2017-01-11 8:21 ` Greg KH
2017-01-12 22:54 ` Serge Semin
2017-01-13 7:22 ` Greg KH
2017-01-13 9:47 ` Serge Semin
2016-12-13 14:22 ` [PATCH v4 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin
2017-01-13 12:16 ` [PATCH v5 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin
2017-01-13 12:16 ` [PATCH v5 1/2] " Serge Semin
2017-01-13 12:16 ` [PATCH v5 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin
2017-01-18 22:34 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161205152502.GA17538@mobilestation \
--to=fancer.lancer@gmail.com \
--cc=Sergey.Semin@t-platforms.ru \
--cc=andrew@lunn.ch \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh@kernel.org \
--cc=srinivas.kandagatla@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).