Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531745 > unrolled thread

[PATCH v2 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin <fancer.lancer@gmail.com> - 2016-11-28 23:40 +0100
    [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin <fancer.lancer@gmail.com> - 2016-11-28 23:40 +0100
      Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 20:40 +0100
        Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin <fancer.lancer@gmail.com> - 2016-11-29 22:20 +0100
      Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Rob Herring <robh@kernel.org> - 2016-12-05 15:50 +0100
        Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin <fancer.lancer@gmail.com> - 2016-12-05 16:30 +0100
          Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Rob Herring <robh@kernel.org> - 2016-12-05 18:30 +0100
            Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin <fancer.lancer@gmail.com> - 2016-12-05 20:10 +0100
              Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Serge Semin <fancer.lancer@gmail.com> - 2016-12-09 03:00 +0100
    Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 20:40 +0100
      Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin <fancer.lancer@gmail.com> - 2016-11-29 22:20 +0100
        Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 22:30 +0100
          Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin <fancer.lancer@gmail.com> - 2016-11-29 22:50 +0100
    Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 20:40 +0100
    [PATCH v3 2/2] eeprom: Add IDT 89HPESx driver dts-binding file Serge Semin <fancer.lancer@gmail.com> - 2016-11-29 23:30 +0100
    [PATCH v3 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Serge Semin <fancer.lancer@gmail.com> - 2016-11-29 23:30 +0100

#1531745 — [PATCH v2 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

Following changes are made in accordance with Greg KH notes as well as
fixing some found issues:
- Get rid of dev_*_idt() macros
- IDT CSR debug file is moved to debugfs
- BIN_ATTR is used to declare sysfs binary attribute
- Moved bindings file to a separate patch
- Need to create a specific bin_attribute structure for each device
- Perform a few read retries with delays if EEPROM is busy

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Serge Semin (2):
  MISC eeprom: Add IDT 89HPESx EEPROM/CSR driver
  MISC eeprom: Add IDT 89HPESx driver bindings file

 .../devicetree/bindings/misc/idt_89hpesx.txt       |   41 +
 drivers/misc/eeprom/Kconfig                        |   10 +
 drivers/misc/eeprom/Makefile                       |    1 +
 drivers/misc/eeprom/idt_89hpesx.c                  | 1577 ++++++++++++++++++++
 4 files changed, 1629 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
 create mode 100644 drivers/misc/eeprom/idt_89hpesx.c

-- 
2.6.6

[toc] | [next] | [standalone]

#1531748 — [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

See cover-letter for changelog

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
 .../devicetree/bindings/misc/idt_89hpesx.txt       | 41 ++++++++++++++++++++++
 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-
+		 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)
+
+Example:
+	idt_pcie_sw@60 {
+		compatible = "idt,89hpes12nt3";
+		reg = <0x60>;
+		read-only;
+		idt,eesize = <65536>;
+		idt,eeaddr = <0x50>;
+	};
-- 
2.6.6

[toc] | [prev] | [next] | [standalone]

#1532714 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote:
> See cover-letter for changelog

There is no cover letter in an individual patch when it gets committed
to the tree...

So please fix, personally, I never read cover letters, each patch should
be "obvious" on it's own :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1532788 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

On Tue, Nov 29, 2016 at 08:34:36PM +0100, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote:
> > See cover-letter for changelog
> 
> There is no cover letter in an individual patch when it gets committed
> to the tree...
> 
> So please fix, personally, I never read cover letters, each patch should
> be "obvious" on it's own :)
> 
> thanks,
> 
> greg k-h

Understood. I'll send the v3 of altered patchset over with individual messages
for each patch.

Thanks,
-Sergey

[toc] | [prev] | [next] | [standalone]

#1536143 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

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.

>  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.

> +		 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?

> +
> +Example:
> +	idt_pcie_sw@60 {

Don't use '_'.

> +		compatible = "idt,89hpes12nt3";
> +		reg = <0x60>;
> +		read-only;
> +		idt,eesize = <65536>;
> +		idt,eeaddr = <0x50>;
> +	};
> -- 
> 2.6.6
> 

[toc] | [prev] | [next] | [standalone]

#1536162 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

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

[toc] | [prev] | [next] | [standalone]

#1536263 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@gmail.com> wrote:
> 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.

Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
does. You don't want to be changing the binding doc when the driver
changes.

> 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.

NO! That's not at all what I'm suggesting. Specific compatible strings
are the right way to go for the reasons you give. You just don't need
to state why here (because it is true for all bindings).

>> > +            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:

You can grep thru bindings as easily as I can. I can't do that for
everyone's binding.

> "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.

Ah, this needs to be much different than I thought. You need to model
(i.e. use the same binding) the EEPROM node just like it was directly
attached to the host. So this means you need the 2nd i2c bus modeled
which means you need the PCIe switch modeled. A rough outline of the
nodes would look like this:

host-i2c: i2c {
  compatible ="host-i2c"
};

pcie {
    pcie-switch {
        i2c-bus = <&host-i2c>;
        i2c-bus {
            eeprom@50 {
            };
        };
    };
};

So this models the PCIe switch as a PCIe device, it has a phandle back
to it's controller since it's not a child of the i2c controller. Then
the devices on switches i2c bus are modeled as children of the switch.

Alternatively, it could be described all as children of host-i2c node.
It's common for i2c devices to have downstream i2c buses. I2C muxes
are one example and there are bindings defined for all this. There's
also chips like mpu-6050 that have slave buses.

Rob

[toc] | [prev] | [next] | [standalone]

#1536346 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh@kernel.org> wrote:
> On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@gmail.com> wrote:
> > 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.
> >

What about this comment? Shall the file be left at the path I placed it?

> >> >  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.
> 
> Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
> does. You don't want to be changing the binding doc when the driver
> changes.
> 
> > 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.
> 
> NO! That's not at all what I'm suggesting. Specific compatible strings
> are the right way to go for the reasons you give. You just don't need
> to state why here (because it is true for all bindings).
> 

Oh, I just misunderstood what you said. I'll discard the comment.

> >> > +            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:
> 
> You can grep thru bindings as easily as I can. I can't do that for
> everyone's binding.
> 

It won't be necessary due to the next comment.

> > "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.
> 
> Ah, this needs to be much different than I thought. You need to model
> (i.e. use the same binding) the EEPROM node just like it was directly
> attached to the host. So this means you need the 2nd i2c bus modeled
> which means you need the PCIe switch modeled. A rough outline of the
> nodes would look like this:
> 
> host-i2c: i2c {
>   compatible ="host-i2c"
> };
> 
> pcie {
>     pcie-switch {
>         i2c-bus = <&host-i2c>;
>         i2c-bus {
>             eeprom@50 {
>             };
>         };
>     };
> };
> 
> So this models the PCIe switch as a PCIe device, it has a phandle back
> to it's controller since it's not a child of the i2c controller. Then
> the devices on switches i2c bus are modeled as children of the switch.
> 
> Alternatively, it could be described all as children of host-i2c node.
> It's common for i2c devices to have downstream i2c buses. I2C muxes
> are one example and there are bindings defined for all this. There's
> also chips like mpu-6050 that have slave buses.
> 
> Rob

I think I understand what you says. However let me just bring some details
to make things clear.

First of all the driver doesn't do any PCI-Express-related work. The device
!IDT PCI Express switch! just has two additional i2c interfaces: i2c-slave
and i2c-master. As it is obvious from the bus-names i2c-slave is the interface,
where IDT PCIe-switch device is actually slave. This interface can be reached
from the host by ordinary i2c buses. i2c-master interface is connected to an
i2c-bus, where IDT PCIe-switch is single master. This bus can have just one
EEPROM device to store some initialization data. Host can send some specific
smbus-packets to i2c-slave interface of IDT PCIe-switch in order to
preinitialize EEPROM data, connected to i2c-master interface of the device.

Additionally IDT PCIe-switch handles some special smbus packets coming to it
i2c-slave interface to read/write its internal CSR. This interface can be
used to debug the device, when there are problems with it usual PCI Express
related functioning.

So to speak, it wouldn't be good to have PCIe-switch declared in dts as a
PCI-device, since PCI-bus is actually dynamically populated by PCI-core
subsystem.

According to what you said and the device/driver design I described, the
following bindings can be suggested:

i2c0: i2c@FFFF0000 {
	compatible = "vendor,i2c-adapter";
	#address-cells = <1>;
	#size-cells = <0>;

	idt_i2c_iface: idt@60 {
		compatible = "idt,89hpes32nt8ag2";
		reg = <0x60>;
		#address-cells = <1>;
		#size-cells = <0>;

		eeprom@51 {
			compatible = "at,24c64";
			reg = <0x51>;
			read-only;
		};
	};
};

Suppose there is some host-i2c adapter like "vendor,i2c-adapter" and
i2c-slave interface of IDT PCIe-switch is connected to it. In this way
i2c-slave interface will be visible like ordinary i2c-device with just
one subnode. This subnode explains the actual EEPROM connected to
IDT PCIe-switch i2c-master interface.

Does it look acceptable? It seems like your last suggestion. Is it?

Thanks,
-Sergey

[toc] | [prev] | [next] | [standalone]

#1539004 — Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

Rob,
Could you please respond on these comments? I've got some free time, so I wanna
rewrite the code until I've not got busy again.

Regards,
-Sergey

On Mon, Dec 05, 2016 at 10:04:56PM +0300, Serge Semin <fancer.lancer@gmail.com> wrote:
> On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh@kernel.org> wrote:
> > On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@gmail.com> wrote:
> > > 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.
> > >
> 
> What about this comment? Shall the file be left at the path I placed it?
> 
> > >> >  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.
> > 
> > Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
> > does. You don't want to be changing the binding doc when the driver
> > changes.
> > 
> > > 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.
> > 
> > NO! That's not at all what I'm suggesting. Specific compatible strings
> > are the right way to go for the reasons you give. You just don't need
> > to state why here (because it is true for all bindings).
> > 
> 
> Oh, I just misunderstood what you said. I'll discard the comment.
> 
> > >> > +            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:
> > 
> > You can grep thru bindings as easily as I can. I can't do that for
> > everyone's binding.
> > 
> 
> It won't be necessary due to the next comment.
> 
> > > "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.
> > 
> > Ah, this needs to be much different than I thought. You need to model
> > (i.e. use the same binding) the EEPROM node just like it was directly
> > attached to the host. So this means you need the 2nd i2c bus modeled
> > which means you need the PCIe switch modeled. A rough outline of the
> > nodes would look like this:
> > 
> > host-i2c: i2c {
> >   compatible ="host-i2c"
> > };
> > 
> > pcie {
> >     pcie-switch {
> >         i2c-bus = <&host-i2c>;
> >         i2c-bus {
> >             eeprom@50 {
> >             };
> >         };
> >     };
> > };
> > 
> > So this models the PCIe switch as a PCIe device, it has a phandle back
> > to it's controller since it's not a child of the i2c controller. Then
> > the devices on switches i2c bus are modeled as children of the switch.
> > 
> > Alternatively, it could be described all as children of host-i2c node.
> > It's common for i2c devices to have downstream i2c buses. I2C muxes
> > are one example and there are bindings defined for all this. There's
> > also chips like mpu-6050 that have slave buses.
> > 
> > Rob
> 
> I think I understand what you says. However let me just bring some details
> to make things clear.
> 
> First of all the driver doesn't do any PCI-Express-related work. The device
> !IDT PCI Express switch! just has two additional i2c interfaces: i2c-slave
> and i2c-master. As it is obvious from the bus-names i2c-slave is the interface,
> where IDT PCIe-switch device is actually slave. This interface can be reached
> from the host by ordinary i2c buses. i2c-master interface is connected to an
> i2c-bus, where IDT PCIe-switch is single master. This bus can have just one
> EEPROM device to store some initialization data. Host can send some specific
> smbus-packets to i2c-slave interface of IDT PCIe-switch in order to
> preinitialize EEPROM data, connected to i2c-master interface of the device.
> 
> Additionally IDT PCIe-switch handles some special smbus packets coming to it
> i2c-slave interface to read/write its internal CSR. This interface can be
> used to debug the device, when there are problems with it usual PCI Express
> related functioning.
> 
> So to speak, it wouldn't be good to have PCIe-switch declared in dts as a
> PCI-device, since PCI-bus is actually dynamically populated by PCI-core
> subsystem.
> 
> According to what you said and the device/driver design I described, the
> following bindings can be suggested:
> 
> i2c0: i2c@FFFF0000 {
> 	compatible = "vendor,i2c-adapter";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	idt_i2c_iface: idt@60 {
> 		compatible = "idt,89hpes32nt8ag2";
> 		reg = <0x60>;
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		eeprom@51 {
> 			compatible = "at,24c64";
> 			reg = <0x51>;
> 			read-only;
> 		};
> 	};
> };
> 
> Suppose there is some host-i2c adapter like "vendor,i2c-adapter" and
> i2c-slave interface of IDT PCIe-switch is connected to it. In this way
> i2c-slave interface will be visible like ordinary i2c-device with just
> one subnode. This subnode explains the actual EEPROM connected to
> IDT PCIe-switch i2c-master interface.
> 
> Does it look acceptable? It seems like your last suggestion. Is it?
> 
> Thanks,
> -Sergey
> 

[toc] | [prev] | [next] | [standalone]

#1532709 — Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

On Tue, Nov 29, 2016 at 01:38:20AM +0300, Serge Semin wrote:
> +struct idt_89hpesx_dev {
> +	u32 eesize;
> +	bool eero;
> +	u8 eeaddr;
> +
> +	u8 inieecmd;
> +	u8 inicsrcmd;
> +	u8 iniccode;
> +
> +	atomic_t csr;
> +
> +	int (*smb_write)(struct idt_89hpesx_dev *, const struct idt_smb_seq *);
> +	int (*smb_read)(struct idt_89hpesx_dev *, struct idt_smb_seq *);
> +	struct mutex smb_mtx;
> +
> +	struct i2c_client *client;
> +
> +	struct bin_attribute *ee_file;
> +	struct dentry *csr_dir;
> +	struct dentry *csr_file;
> +};
> +#define to_pdev_kobj(__kobj) \
> +	dev_get_drvdata(container_of(__kobj, struct device, kobj))

Is it a struct device, or a kobject?  This is totally confusing to me.

And can't you just use kobj_to_dev()?

> +/*
> + * eeprom_attribute - EEPROM sysfs-node attributes
> + *
> + * NOTE Size will be changed in compliance with OF node. EEPROM attribute will
> + * be read-only as well if the corresponding flag is specified in OF node.
> + */
> +BIN_ATTR(eeprom, 0644, idt_sysfs_eeprom_read, idt_sysfs_eeprom_write,
> +	 EEPROM_DEF_SIZE);

static?

And BIN_ATTR_RW()?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1532786 — Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

On Tue, Nov 29, 2016 at 08:37:50PM +0100, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 29, 2016 at 01:38:20AM +0300, Serge Semin wrote:
> > +struct idt_89hpesx_dev {
> > +	u32 eesize;
> > +	bool eero;
> > +	u8 eeaddr;
> > +
> > +	u8 inieecmd;
> > +	u8 inicsrcmd;
> > +	u8 iniccode;
> > +
> > +	atomic_t csr;
> > +
> > +	int (*smb_write)(struct idt_89hpesx_dev *, const struct idt_smb_seq *);
> > +	int (*smb_read)(struct idt_89hpesx_dev *, struct idt_smb_seq *);
> > +	struct mutex smb_mtx;
> > +
> > +	struct i2c_client *client;
> > +
> > +	struct bin_attribute *ee_file;
> > +	struct dentry *csr_dir;
> > +	struct dentry *csr_file;
> > +};
> > +#define to_pdev_kobj(__kobj) \
> > +	dev_get_drvdata(container_of(__kobj, struct device, kobj))
> 
> Is it a struct device, or a kobject?  This is totally confusing to me.
> 
> And can't you just use kobj_to_dev()?
> 

I just didn't know about kobj_to_dev() inline function. Totally agree that
container_of() should be replaced with it.
What does look confusing to you? Do you mean the name "to_pdev_kobj" of the
macro?

> > +/*
> > + * eeprom_attribute - EEPROM sysfs-node attributes
> > + *
> > + * NOTE Size will be changed in compliance with OF node. EEPROM attribute will
> > + * be read-only as well if the corresponding flag is specified in OF node.
> > + */
> > +BIN_ATTR(eeprom, 0644, idt_sysfs_eeprom_read, idt_sysfs_eeprom_write,
> > +	 EEPROM_DEF_SIZE);
> 
> static?
> 
> And BIN_ATTR_RW()?
> 
> thanks,
> 
> greg k-h

Of course it should be static. Thanks for noticing that.
But I intentionally utilized BIN_ATTR() instead of BIN_ATTR_RW(), because
the last one implies to define the read/write methods with names
"_name##_read"/"_name##_write", which totally get out of naming within the
driver source code. To tell the truth macro BIN_ATTR_RW() isn't that
popular in the kernel. Neither is BIN_ATTR() macro, but it suites my driver
better than the another one.

Thanks,
-Sergey

[toc] | [prev] | [next] | [standalone]

#1532789 — Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

On Wed, Nov 30, 2016 at 12:16:25AM +0300, Serge Semin wrote:
> On Tue, Nov 29, 2016 at 08:37:50PM +0100, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Nov 29, 2016 at 01:38:20AM +0300, Serge Semin wrote:
> > > +struct idt_89hpesx_dev {
> > > +	u32 eesize;
> > > +	bool eero;
> > > +	u8 eeaddr;
> > > +
> > > +	u8 inieecmd;
> > > +	u8 inicsrcmd;
> > > +	u8 iniccode;
> > > +
> > > +	atomic_t csr;
> > > +
> > > +	int (*smb_write)(struct idt_89hpesx_dev *, const struct idt_smb_seq *);
> > > +	int (*smb_read)(struct idt_89hpesx_dev *, struct idt_smb_seq *);
> > > +	struct mutex smb_mtx;
> > > +
> > > +	struct i2c_client *client;
> > > +
> > > +	struct bin_attribute *ee_file;
> > > +	struct dentry *csr_dir;
> > > +	struct dentry *csr_file;
> > > +};
> > > +#define to_pdev_kobj(__kobj) \
> > > +	dev_get_drvdata(container_of(__kobj, struct device, kobj))
> > 
> > Is it a struct device, or a kobject?  This is totally confusing to me.
> > 
> > And can't you just use kobj_to_dev()?
> > 
> 
> I just didn't know about kobj_to_dev() inline function. Totally agree that
> container_of() should be replaced with it.
> What does look confusing to you? Do you mean the name "to_pdev_kobj" of the
> macro?

Yes, the macro is odd.  As you are doing two different things here, just
spell it out in the code and use kobj_to_dev() to make it easier to
read please.

> > > +/*
> > > + * eeprom_attribute - EEPROM sysfs-node attributes
> > > + *
> > > + * NOTE Size will be changed in compliance with OF node. EEPROM attribute will
> > > + * be read-only as well if the corresponding flag is specified in OF node.
> > > + */
> > > +BIN_ATTR(eeprom, 0644, idt_sysfs_eeprom_read, idt_sysfs_eeprom_write,
> > > +	 EEPROM_DEF_SIZE);
> > 
> > static?
> > 
> > And BIN_ATTR_RW()?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Of course it should be static. Thanks for noticing that.
> But I intentionally utilized BIN_ATTR() instead of BIN_ATTR_RW(), because
> the last one implies to define the read/write methods with names
> "_name##_read"/"_name##_write", which totally get out of naming within the
> driver source code.

That's ok, use the names the macro wants you to, that's the best way,
and it ensures that I don't have to audit your permissions are correct
for the file.

> To tell the truth macro BIN_ATTR_RW() isn't that popular in the
> kernel.

Yes, but it should be, I have patches floating around somewhere to fix
almost all of these up.

> Neither is BIN_ATTR() macro, but it suites my driver better than the
> another one.

a "raw" BIN_ATTR() shouldn't be used either, please use the _RW()
variant.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1532803 — Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

On Tue, Nov 29, 2016 at 10:24:12PM +0100, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 30, 2016 at 12:16:25AM +0300, Serge Semin wrote:
> > On Tue, Nov 29, 2016 at 08:37:50PM +0100, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Tue, Nov 29, 2016 at 01:38:20AM +0300, Serge Semin wrote:
> > > > +struct idt_89hpesx_dev {
> > > > +	u32 eesize;
> > > > +	bool eero;
> > > > +	u8 eeaddr;
> > > > +
> > > > +	u8 inieecmd;
> > > > +	u8 inicsrcmd;
> > > > +	u8 iniccode;
> > > > +
> > > > +	atomic_t csr;
> > > > +
> > > > +	int (*smb_write)(struct idt_89hpesx_dev *, const struct idt_smb_seq *);
> > > > +	int (*smb_read)(struct idt_89hpesx_dev *, struct idt_smb_seq *);
> > > > +	struct mutex smb_mtx;
> > > > +
> > > > +	struct i2c_client *client;
> > > > +
> > > > +	struct bin_attribute *ee_file;
> > > > +	struct dentry *csr_dir;
> > > > +	struct dentry *csr_file;
> > > > +};
> > > > +#define to_pdev_kobj(__kobj) \
> > > > +	dev_get_drvdata(container_of(__kobj, struct device, kobj))
> > > 
> > > Is it a struct device, or a kobject?  This is totally confusing to me.
> > > 
> > > And can't you just use kobj_to_dev()?
> > > 
> > 
> > I just didn't know about kobj_to_dev() inline function. Totally agree that
> > container_of() should be replaced with it.
> > What does look confusing to you? Do you mean the name "to_pdev_kobj" of the
> > macro?
> 
> Yes, the macro is odd.  As you are doing two different things here, just
> spell it out in the code and use kobj_to_dev() to make it easier to
> read please.
> 
> > > > +/*
> > > > + * eeprom_attribute - EEPROM sysfs-node attributes
> > > > + *
> > > > + * NOTE Size will be changed in compliance with OF node. EEPROM attribute will
> > > > + * be read-only as well if the corresponding flag is specified in OF node.
> > > > + */
> > > > +BIN_ATTR(eeprom, 0644, idt_sysfs_eeprom_read, idt_sysfs_eeprom_write,
> > > > +	 EEPROM_DEF_SIZE);
> > > 
> > > static?
> > > 
> > > And BIN_ATTR_RW()?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Of course it should be static. Thanks for noticing that.
> > But I intentionally utilized BIN_ATTR() instead of BIN_ATTR_RW(), because
> > the last one implies to define the read/write methods with names
> > "_name##_read"/"_name##_write", which totally get out of naming within the
> > driver source code.
> 
> That's ok, use the names the macro wants you to, that's the best way,
> and it ensures that I don't have to audit your permissions are correct
> for the file.
> 
> > To tell the truth macro BIN_ATTR_RW() isn't that popular in the
> > kernel.
> 
> Yes, but it should be, I have patches floating around somewhere to fix
> almost all of these up.
> 
> > Neither is BIN_ATTR() macro, but it suites my driver better than the
> > another one.
> 
> a "raw" BIN_ATTR() shouldn't be used either, please use the _RW()
> variant.
> 
> thanks,
> 
> greg k-h

Agreed with all the notes. I will send patchset v3 within next hour.

Thanks,
-Sergey

[toc] | [prev] | [next] | [standalone]

#1532711 — Re: [PATCH v2 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

On Tue, Nov 29, 2016 at 01:38:20AM +0300, Serge Semin wrote:
> See cover-letter for changelog

Same here.

[toc] | [prev] | [next] | [standalone]

#1532819 — [PATCH v3 2/2] eeprom: Add IDT 89HPESx driver dts-binding file

IDT 89HPESx PCIe-switches exposes SMBus interface to have an access to
the device CSRs and EEPROM. So to properly utilize the interface
functionality, developer should declare a valid dts-file node, which
would refer to the corresponding 89HPESx device.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../devicetree/bindings/misc/idt_89hpesx.txt       | 41 ++++++++++++++++++++++
 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-
+		 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)
+
+Example:
+	idt_pcie_sw@60 {
+		compatible = "idt,89hpes12nt3";
+		reg = <0x60>;
+		read-only;
+		idt,eesize = <65536>;
+		idt,eeaddr = <0x50>;
+	};
-- 
2.6.6

[toc] | [prev] | [next] | [standalone]

#1532822 — [PATCH v3 0/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver

Changelog v3:
- Get rid of dev_*_idt() macros
- Replace to_pdev_kobj() macro with naked dev_get_drvdata() call
- Return naked 0 instead of SUCCESS macro
- IDT CSR debug file is moved to debugfs
- BIN_ATTR_RW is used to declare sysfs binary attribute
- Moved bindings file to a separate patch
- Need to create a specific bin_attribute structure for each device
- Perform a few read retries with delays if EEPROM is busy

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Serge Semin (2):
  eeprom: Add IDT 89HPESx EEPROM/CSR driver
  eeprom: Add IDT 89HPESx driver dts-binding file

 .../devicetree/bindings/misc/idt_89hpesx.txt       |   41 +
 drivers/misc/eeprom/Kconfig                        |   10 +
 drivers/misc/eeprom/Makefile                       |    1 +
 drivers/misc/eeprom/idt_89hpesx.c                  | 1574 ++++++++++++++++++++
 4 files changed, 1626 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
 create mode 100644 drivers/misc/eeprom/idt_89hpesx.c

-- 
2.6.6

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web