[PATCH v2 0/5] eeprom: at24: device tree support fixes

[PATCH v2 0/5] eeprom: at24: device tree support fixes

[Bartosz Golaszewski]

The first three patches fix certain issues with the DT binding
document.

The last two extend the device tree support in the driver with more
at24 EEPROM variants.

v1 -> v2:
- remove any implementation details from patch 1/5

Bartosz Golaszewski (5):
  dt-bindings: at24: consistently document the compatible property
  dt-bindings: at24: add a missing compatible
  dt-bindings: at24: fix formatting and style
  dt-bindings: at24: extend the list of supported chips
  eeprom: at24: extend the list of chips supported in DT

 Documentation/devicetree/bindings/eeprom/at24.txt | 82 +++++++++++++----------
 drivers/misc/eeprom/at24.c                        |  9 +++
 2 files changed, 57 insertions(+), 34 deletions(-)

-- 
2.15.1

[PATCH v2 0/5] eeprom: at24: device tree support fixes

[Bartosz Golaszewski]

The first three patches fix certain issues with the DT binding
document.

The last two extend the device tree support in the driver with more
at24 EEPROM variants.

v1 -> v2:
- remove any implementation details from patch 1/5

Bartosz Golaszewski (5):
  dt-bindings: at24: consistently document the compatible property
  dt-bindings: at24: add a missing compatible
  dt-bindings: at24: fix formatting and style
  dt-bindings: at24: extend the list of supported chips
  eeprom: at24: extend the list of chips supported in DT

 Documentation/devicetree/bindings/eeprom/at24.txt | 82 +++++++++++++----------
 drivers/misc/eeprom/at24.c                        |  9 +++
 2 files changed, 57 insertions(+), 34 deletions(-)

-- 
2.15.1

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

[PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Bartosz Golaszewski]

Current description of the compatible property for at24 is quite vague.

Specify an exact list of accepted compatibles and document the - now
deprecated - strings which were previously used in device tree files.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 48 ++++++++++++-----------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index cbc80e194ac6..16b687458b14 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -2,28 +2,32 @@ EEPROMs (I2C)
 
 Required properties:
 
-  - compatible : should be "<manufacturer>,<type>", like these:
-
-	"atmel,24c00", "atmel,24c01", "atmel,24c02", "atmel,24c04",
-	"atmel,24c08", "atmel,24c16", "atmel,24c32", "atmel,24c64",
-	"atmel,24c128", "atmel,24c256", "atmel,24c512", "atmel,24c1024"
-
-	"catalyst,24c32"
-
-	"microchip,24c128"
-
-	"ramtron,24c64"
-
-	"renesas,r1ex24002"
-
-	The following manufacturers values have been deprecated:
-	"at", "at24"
-
-	 If there is no specific driver for <manufacturer>, a generic
-	 device with <type> and manufacturer "atmel" should be used.
-	 Possible types are:
-	 "24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", "24c64",
-	 "24c128", "24c256", "24c512", "24c1024", "spd"
+  - compatible: must be one of the following:
+
+        "atmel,24c00",
+        "atmel,24c01",
+        "atmel,24c02",
+        "atmel,24c04",
+        "atmel,24c08",
+        "atmel,24c16",
+        "atmel,24c32",
+        "atmel,24c64",
+        "atmel,24c128",
+        "atmel,24c256",
+        "atmel,24c512",
+        "atmel,24c1024"
+
+  NOTE: old compatible strings, such as:
+
+        "catalyst,24c32",
+        "microchip,24c128",
+        "ramtron,24c64",
+        "renesas,r1ex24002",
+        "at,24c08",
+        "at24,24c08",
+        "24c32"
+
+  will still work, but are now deprecated.
 
   - reg : the I2C address of the EEPROM
 
-- 
2.15.1

[PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Bartosz Golaszewski]

Current description of the compatible property for at24 is quite vague.

Specify an exact list of accepted compatibles and document the - now
deprecated - strings which were previously used in device tree files.

Signed-off-by: Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org>
Reviewed-by: Javier Martinez Canillas <javierm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 48 ++++++++++++-----------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index cbc80e194ac6..16b687458b14 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -2,28 +2,32 @@ EEPROMs (I2C)
 
 Required properties:
 
-  - compatible : should be "<manufacturer>,<type>", like these:
-
-	"atmel,24c00", "atmel,24c01", "atmel,24c02", "atmel,24c04",
-	"atmel,24c08", "atmel,24c16", "atmel,24c32", "atmel,24c64",
-	"atmel,24c128", "atmel,24c256", "atmel,24c512", "atmel,24c1024"
-
-	"catalyst,24c32"
-
-	"microchip,24c128"
-
-	"ramtron,24c64"
-
-	"renesas,r1ex24002"
-
-	The following manufacturers values have been deprecated:
-	"at", "at24"
-
-	 If there is no specific driver for <manufacturer>, a generic
-	 device with <type> and manufacturer "atmel" should be used.
-	 Possible types are:
-	 "24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", "24c64",
-	 "24c128", "24c256", "24c512", "24c1024", "spd"
+  - compatible: must be one of the following:
+
+        "atmel,24c00",
+        "atmel,24c01",
+        "atmel,24c02",
+        "atmel,24c04",
+        "atmel,24c08",
+        "atmel,24c16",
+        "atmel,24c32",
+        "atmel,24c64",
+        "atmel,24c128",
+        "atmel,24c256",
+        "atmel,24c512",
+        "atmel,24c1024"
+
+  NOTE: old compatible strings, such as:
+
+        "catalyst,24c32",
+        "microchip,24c128",
+        "ramtron,24c64",
+        "renesas,r1ex24002",
+        "at,24c08",
+        "at24,24c08",
+        "24c32"
+
+  will still work, but are now deprecated.
 
   - reg : the I2C address of the EEPROM
 
-- 
2.15.1

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

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Current description of the compatible property for at24 is quite vague.
>
> Specify an exact list of accepted compatibles and document the - now
> deprecated - strings which were previously used in device tree files.

> +  will still work, but are now deprecated.

I would rather reprase it to make people not think that's okay to
still use deprecated strings.

Something like
"are now deprecated, please do not use in new code or drivers".

Supporting old but deprecated is implied by DT globally.
(We don't break user space, remember?)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
> Current description of the compatible property for at24 is quite vague.
>
> Specify an exact list of accepted compatibles and document the - now
> deprecated - strings which were previously used in device tree files.

> +  will still work, but are now deprecated.

I would rather reprase it to make people not think that's okay to
still use deprecated strings.

Something like
"are now deprecated, please do not use in new code or drivers".

Supporting old but deprecated is implied by DT globally.
(We don't break user space, remember?)

-- 
With Best Regards,
Andy Shevchenko
--
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

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Peter Rosin]

On 2017-12-21 14:48, Bartosz Golaszewski wrote:
> Current description of the compatible property for at24 is quite vague.
> 
> Specify an exact list of accepted compatibles and document the - now
> deprecated - strings which were previously used in device tree files.

Why is it suddenly deprecated to correctly specify what hardware you
have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
Sure, it happens to be compatible with "atmel,24c32", but that is
supposed to be written with a fallback as

	"nxp,24c32", "atmel,24c32"

if I understand correctly. So, why is that deprecated in this case?

What if (a few years down the line) it is discovered that some weird
quirk is needed that is only appropriate for nxp chips?

nxp is of course just an example, pick any manufacturer of eeproms
(supposedly) compatible with the atmel interface.

Cheers,
Peter

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[David Lechner]

On 12/21/2017 10:20 AM, Peter Rosin wrote:
> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>> Current description of the compatible property for at24 is quite vague.
>>
>> Specify an exact list of accepted compatibles and document the - now
>> deprecated - strings which were previously used in device tree files.
> 
> Why is it suddenly deprecated to correctly specify what hardware you
> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
> Sure, it happens to be compatible with "atmel,24c32", but that is
> supposed to be written with a fallback as
> 
> 	"nxp,24c32", "atmel,24c32"
> 
> if I understand correctly. So, why is that deprecated in this case?
> 
> What if (a few years down the line) it is discovered that some weird
> quirk is needed that is only appropriate for nxp chips?
> 
> nxp is of course just an example, pick any manufacturer of eeproms
> (supposedly) compatible with the atmel interface.
> 

So now I am supposed to change my device tree from "microchip,24c128" to 
"atmel,24c128"?

Also, at,24c08 is listed in 
Documentation/devicetree/bindings/trivial-devices.txt. It should 
probably be removed.

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[David Lechner]

On 12/21/2017 11:24 AM, David Lechner wrote:
> Also, at,24c08 is listed in 
> Documentation/devicetree/bindings/trivial-devices.txt. It should 
> probably be removed.

And st,24c256

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[David Lechner]

On 12/21/2017 11:24 AM, David Lechner wrote:
> Also, at,24c08 is listed in 
> Documentation/devicetree/bindings/trivial-devices.txt. It should 
> probably be removed.

And st,24c256

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

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Javier Martinez Canillas]

Hello Peter,

On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>> Current description of the compatible property for at24 is quite vague.
>>
>> Specify an exact list of accepted compatibles and document the - now
>> deprecated - strings which were previously used in device tree files.
>
> Why is it suddenly deprecated to correctly specify what hardware you
> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.

Sorry but I disagree.

When you specify a compatible string, you are not specifying a
particular hardware but a device programming model.

It's very common to use a compatible string that doesn't match exactly
the specific hardware used. That's why it's called _compatible_ BTW.

For example when a DTS define a UART node with an ns16550 compatible,
they don't really mean that have a UART IC manufactured by National
Semiconductor.

> Sure, it happens to be compatible with "atmel,24c32", but that is
> supposed to be written with a fallback as
>
>         "nxp,24c32", "atmel,24c32"

This isn't a requirement really, systems integrators are free to use
more than one <manufacturer,model> tuple in the compatible string if
they want the DTB to be future proof, just in case there's a need for
a more specific driver or if the programming model happened to not be
the same at the end. This is usually done for the boards compatible
string as an example, even when there isn't a struct machine_desc for
the specific board compatible and only for the SoC family.

So it's OK if you want to define the compatible as "nxp,24c32",
"atmel,24c32", but that's a general OF concept and not something
related to the at24 eeprom driver so I'm not sure if it should be
mentioned in the at24 DT binding doc.

> if I understand correctly. So, why is that deprecated in this case?
>
> What if (a few years down the line) it is discovered that some weird
> quirk is needed that is only appropriate for nxp chips?
>
> nxp is of course just an example, pick any manufacturer of eeproms
> (supposedly) compatible with the atmel interface.
>

That's indeed a possibility and the reason why most DT nodes have a
more specific <manufacturer,model> before the generic one matched by
drivers. I really doubt that in the future it will be found that a
more specific compatible string is needed for one manufacturer in this
case, specially since the driver didn't even care about the
manufacturers until recently.

So I think that makes no sense for a driver to support all possible
manufacturers as possible compatible strings if all the devices have
the same exact programming model.

> Cheers,
> Peter

Best regards,
Javier

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Peter Rosin]

On 2017-12-21 21:27, Javier Martinez Canillas wrote:
> Hello Peter,
> 
> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>>> Current description of the compatible property for at24 is quite vague.
>>>
>>> Specify an exact list of accepted compatibles and document the - now
>>> deprecated - strings which were previously used in device tree files.
>>
>> Why is it suddenly deprecated to correctly specify what hardware you
>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
> 
> Sorry but I disagree.
> 
> When you specify a compatible string, you are not specifying a
> particular hardware but a device programming model.

That's not what it says in https://elinux.org/Device_Tree_Usage
in the "Understanding the compatible Property" section. I quote:

	compatible is a list of strings. The first string in the
	list specifies the exact device that the node represents
	in the form "<manufacturer>,<model>". The following strings
	represent other devices that the device is compatible with.

Pretty clearly talks about devices and not programming models. But
maybe I shouldn't trust that reference? What should I be reading
instead?

> It's very common to use a compatible string that doesn't match exactly
> the specific hardware used. That's why it's called _compatible_ BTW.

That's not how I read the above.

> For example when a DTS define a UART node with an ns16550 compatible,
> they don't really mean that have a UART IC manufactured by National
> Semiconductor.

That just tells me that most people are a little bit lazy and ready
to cut a corner or two when they can get away with it. Or that there
is some form of misunderstanding at work...

>> Sure, it happens to be compatible with "atmel,24c32", but that is
>> supposed to be written with a fallback as
>>
>>         "nxp,24c32", "atmel,24c32"
> 
> This isn't a requirement really, systems integrators are free to use
> more than one <manufacturer,model> tuple in the compatible string if
> they want the DTB to be future proof, just in case there's a need for
> a more specific driver or if the programming model happened to not be
> the same at the end. This is usually done for the boards compatible
> string as an example, even when there isn't a struct machine_desc for
> the specific board compatible and only for the SoC family.
> 
> So it's OK if you want to define the compatible as "nxp,24c32",
> "atmel,24c32", but that's a general OF concept and not something
> related to the at24 eeprom driver so I'm not sure if it should be
> mentioned in the at24 DT binding doc.

One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
below) is not a valid compatible, the tooling will be correct to
complain about it. Currently, it is just a checkpatch deficiency that
it complains like this:

$ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
#249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
+               compatible = "nxp,24c02", "atmel,24c02";

>> if I understand correctly. So, why is that deprecated in this case?
>>
>> What if (a few years down the line) it is discovered that some weird
>> quirk is needed that is only appropriate for nxp chips?
>>
>> nxp is of course just an example, pick any manufacturer of eeproms
>> (supposedly) compatible with the atmel interface.
>>
> 
> That's indeed a possibility and the reason why most DT nodes have a
> more specific <manufacturer,model> before the generic one matched by
> drivers. I really doubt that in the future it will be found that a
> more specific compatible string is needed for one manufacturer in this
> case, specially since the driver didn't even care about the
> manufacturers until recently.

I'm not talking about the driver. But if what you say is true, that
change would have broken a number of existing DTBs. Is that really the
case?

> So I think that makes no sense for a driver to support all possible
> manufacturers as possible compatible strings if all the devices have
> the same exact programming model.

That's what the fallback is for. With that in place the driver can
start to care when there is a need but until then it can work just
as it always has. Without knowing the specific device, the driver has
less chance to adapt when the unexpected is discovered. But again,
I'm not really discussing driver details, I'm complaining about the
proposed changes to the bindings. I simply don't see how they make
sense.

Cheers,
Peter

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Javier Martinez Canillas]

Hello Peter,

On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
>> Hello Peter,
>>
>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>>>> Current description of the compatible property for at24 is quite vague.
>>>>
>>>> Specify an exact list of accepted compatibles and document the - now
>>>> deprecated - strings which were previously used in device tree files.
>>>
>>> Why is it suddenly deprecated to correctly specify what hardware you
>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
>>
>> Sorry but I disagree.
>>
>> When you specify a compatible string, you are not specifying a
>> particular hardware but a device programming model.
>
> That's not what it says in https://elinux.org/Device_Tree_Usage

I think the most up-to-date DT reference is at:

https://www.devicetree.org/

> in the "Understanding the compatible Property" section. I quote:
>
>         compatible is a list of strings. The first string in the
>         list specifies the exact device that the node represents
>         in the form "<manufacturer>,<model>". The following strings
>         represent other devices that the device is compatible with.
>
> Pretty clearly talks about devices and not programming models. But
> maybe I shouldn't trust that reference? What should I be reading
> instead?
>

For example the latest spec draft
(https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
says:

"The compatible property value consists of one or more strings that
define the specific programming model for the device. This list of
strings should be used by a client program for device driver
selection. The property value consists of a concatenated list of null
terminated strings, from most specific to most general. They allow a
device to express its compatibility with a family of similar devices,
potentially allowing a single device driver to match against several
devices."

The recommended format is "manufacturer,model", where manufacturer is
a string describing the name of the manufacturer (such as a stock
ticker symbol), and model specifies the model number."

Example:

compatible = "fsl,mpc8641", "ns16550";

In this example, an operating system would first try to locate a
device driver that supported fsl,mpc8641. If a driver was not found,
it would then try to locate a driver that supported the more general
ns16550 device type."

>> It's very common to use a compatible string that doesn't match exactly
>> the specific hardware used. That's why it's called _compatible_ BTW.
>
> That's not how I read the above.
>

That's not how I read it nor my experience with DT, but of course I
may be wrong on this.

>> For example when a DTS define a UART node with an ns16550 compatible,
>> they don't really mean that have a UART IC manufactured by National
>> Semiconductor.
>
> That just tells me that most people are a little bit lazy and ready
> to cut a corner or two when they can get away with it. Or that there
> is some form of misunderstanding at work...
>

For example, I usually see that different SoC families from the same
vendor use the same compatible string for integrated peripherals just
because are the same from a programming model point of view.

TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
and its similar on Exynos SoCs which are the two ARM SoCs I'm most
familiar with. Following your logic that's wrong and instead a new
compatible string should be added for the GPIO or pinctrl drivers even
when are the same because refer to different devices.

>>> Sure, it happens to be compatible with "atmel,24c32", but that is
>>> supposed to be written with a fallback as
>>>
>>>         "nxp,24c32", "atmel,24c32"
>>
>> This isn't a requirement really, systems integrators are free to use
>> more than one <manufacturer,model> tuple in the compatible string if
>> they want the DTB to be future proof, just in case there's a need for
>> a more specific driver or if the programming model happened to not be
>> the same at the end. This is usually done for the boards compatible
>> string as an example, even when there isn't a struct machine_desc for
>> the specific board compatible and only for the SoC family.
>>
>> So it's OK if you want to define the compatible as "nxp,24c32",
>> "atmel,24c32", but that's a general OF concept and not something
>> related to the at24 eeprom driver so I'm not sure if it should be
>> mentioned in the at24 DT binding doc.
>
> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
> below) is not a valid compatible, the tooling will be correct to
> complain about it. Currently, it is just a checkpatch deficiency that
> it complains like this:
>
> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
> +               compatible = "nxp,24c02", "atmel,24c02";
>

But isn't that a bug in checkpatch? as long as there's a valid
<manufacturer,model> tuple in the compatible string, it shouldn't
complain.

>>> if I understand correctly. So, why is that deprecated in this case?
>>>
>>> What if (a few years down the line) it is discovered that some weird
>>> quirk is needed that is only appropriate for nxp chips?
>>>
>>> nxp is of course just an example, pick any manufacturer of eeproms
>>> (supposedly) compatible with the atmel interface.
>>>
>>
>> That's indeed a possibility and the reason why most DT nodes have a
>> more specific <manufacturer,model> before the generic one matched by
>> drivers. I really doubt that in the future it will be found that a
>> more specific compatible string is needed for one manufacturer in this
>> case, specially since the driver didn't even care about the
>> manufacturers until recently.
>
> I'm not talking about the driver. But if what you say is true, that
> change would have broken a number of existing DTBs. Is that really the
> case?
>

Sorry, I didn't get which change you meant.

>> So I think that makes no sense for a driver to support all possible
>> manufacturers as possible compatible strings if all the devices have
>> the same exact programming model.
>
> That's what the fallback is for. With that in place the driver can
> start to care when there is a need but until then it can work just
> as it always has. Without knowing the specific device, the driver has
> less chance to adapt when the unexpected is discovered. But again,
> I'm not really discussing driver details, I'm complaining about the
> proposed changes to the bindings. I simply don't see how they make
> sense.
>

My point is that saying in a DT binding that you could have a more
specific <manufacturer,model> followed by a generic one is redundant.
That's already something one can do to make the DT future proof and
latter if is found that the more specific pair is needed, then
compatible can be added to the OF device ID table making the driver to
match that an the <manufacturer,model> added to the DT binding making
it formal.

That's my understanding at least, but I could be completely wrong too
since I'm not a DT expert.

> Cheers,
> Peter

Best regards,
Javier

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Peter Rosin]

On 2017-12-22 00:38, Javier Martinez Canillas wrote:
> Hello Peter,
> 
> On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda@axentia.se> wrote:
>> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
>>> Hello Peter,
>>>
>>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
>>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>>>>> Current description of the compatible property for at24 is quite vague.
>>>>>
>>>>> Specify an exact list of accepted compatibles and document the - now
>>>>> deprecated - strings which were previously used in device tree files.
>>>>
>>>> Why is it suddenly deprecated to correctly specify what hardware you
>>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
>>>
>>> Sorry but I disagree.
>>>
>>> When you specify a compatible string, you are not specifying a
>>> particular hardware but a device programming model.
>>
>> That's not what it says in https://elinux.org/Device_Tree_Usage
> 
> I think the most up-to-date DT reference is at:
> 
> https://www.devicetree.org/
> 
>> in the "Understanding the compatible Property" section. I quote:
>>
>>         compatible is a list of strings. The first string in the
>>         list specifies the exact device that the node represents
>>         in the form "<manufacturer>,<model>". The following strings
>>         represent other devices that the device is compatible with.
>>
>> Pretty clearly talks about devices and not programming models. But
>> maybe I shouldn't trust that reference? What should I be reading
>> instead?
>>
> 
> For example the latest spec draft
> (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
> says:
> 
> "The compatible property value consists of one or more strings that
> define the specific programming model for the device. This list of
> strings should be used by a client program for device driver
> selection. The property value consists of a concatenated list of null
> terminated strings, from most specific to most general. They allow a
> device to express its compatibility with a family of similar devices,
> potentially allowing a single device driver to match against several
> devices."
> 
> The recommended format is "manufacturer,model", where manufacturer is
> a string describing the name of the manufacturer (such as a stock
> ticker symbol), and model specifies the model number."
> 
> Example:
> 
> compatible = "fsl,mpc8641", "ns16550";
> 
> In this example, an operating system would first try to locate a
> device driver that supported fsl,mpc8641. If a driver was not found,
> it would then try to locate a driver that supported the more general
> ns16550 device type."

Yes, that's a bit different from the wording on the elinux site. Thanks
for the pointer! Google was not my friend on this occasion, since I
managed to miss that site in my searches. Maybe it has gotten a higher
rank now that 0.2 is out, because now I find it easily?

>>> It's very common to use a compatible string that doesn't match exactly
>>> the specific hardware used. That's why it's called _compatible_ BTW.
>>
>> That's not how I read the above.
>>
> 
> That's not how I read it nor my experience with DT, but of course I
> may be wrong on this.

Either way, it should not be wrong to specify the more specific binding
before the generic fallback, as is done in the at91-tse850-3.dts
example I gave below.

>>> For example when a DTS define a UART node with an ns16550 compatible,
>>> they don't really mean that have a UART IC manufactured by National
>>> Semiconductor.
>>
>> That just tells me that most people are a little bit lazy and ready
>> to cut a corner or two when they can get away with it. Or that there
>> is some form of misunderstanding at work...
>>
> 
> For example, I usually see that different SoC families from the same
> vendor use the same compatible string for integrated peripherals just
> because are the same from a programming model point of view.
> 
> TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
> and its similar on Exynos SoCs which are the two ARM SoCs I'm most
> familiar with. Following your logic that's wrong and instead a new
> compatible string should be added for the GPIO or pinctrl drivers even
> when are the same because refer to different devices.

Considering the above quote from the actual DT spec, I wouldn't say wrong.
But I'd still say that it should be preferred to list the actual device
before the fallback to some generic programming model compatible or some
previous version of the hardware/IP-block. Just in case an unintended
difference is discovered late in the game...

>>>> Sure, it happens to be compatible with "atmel,24c32", but that is
>>>> supposed to be written with a fallback as
>>>>
>>>>         "nxp,24c32", "atmel,24c32"
>>>
>>> This isn't a requirement really, systems integrators are free to use
>>> more than one <manufacturer,model> tuple in the compatible string if
>>> they want the DTB to be future proof, just in case there's a need for
>>> a more specific driver or if the programming model happened to not be
>>> the same at the end. This is usually done for the boards compatible
>>> string as an example, even when there isn't a struct machine_desc for
>>> the specific board compatible and only for the SoC family.
>>>
>>> So it's OK if you want to define the compatible as "nxp,24c32",
>>> "atmel,24c32", but that's a general OF concept and not something
>>> related to the at24 eeprom driver so I'm not sure if it should be
>>> mentioned in the at24 DT binding doc.
>>
>> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
>> below) is not a valid compatible, the tooling will be correct to
>> complain about it. Currently, it is just a checkpatch deficiency that
>> it complains like this:
>>
>> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
>> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
>> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
>> +               compatible = "nxp,24c02", "atmel,24c02";
>>
> 
> But isn't that a bug in checkpatch? as long as there's a valid
> <manufacturer,model> tuple in the compatible string, it shouldn't
> complain.

Then there is a significant risk that speeling mistakes are not
caught. So, I don't think it will be considered a bug unless a DT
guru says so of course. I think the checkpatch intention is to catch
all undocumented compatibles. But I don't know of course...

>>>> if I understand correctly. So, why is that deprecated in this case?
>>>>
>>>> What if (a few years down the line) it is discovered that some weird
>>>> quirk is needed that is only appropriate for nxp chips?
>>>>
>>>> nxp is of course just an example, pick any manufacturer of eeproms
>>>> (supposedly) compatible with the atmel interface.
>>>>
>>>
>>> That's indeed a possibility and the reason why most DT nodes have a
>>> more specific <manufacturer,model> before the generic one matched by
>>> drivers. I really doubt that in the future it will be found that a
>>> more specific compatible string is needed for one manufacturer in this
>>> case, specially since the driver didn't even care about the
>>> manufacturers until recently.
>>
>> I'm not talking about the driver. But if what you say is true, that
>> change would have broken a number of existing DTBs. Is that really the
>> case?
>>
> 
> Sorry, I didn't get which change you meant.

I meant whatever change you implied with "the driver didn't even care
about the manufacturers until recently". However, on second reading it
may be the case that "care" did not imply that a breaking change was
made, that was however my interpretation. Oh well...

>>> So I think that makes no sense for a driver to support all possible
>>> manufacturers as possible compatible strings if all the devices have
>>> the same exact programming model.
>>
>> That's what the fallback is for. With that in place the driver can
>> start to care when there is a need but until then it can work just
>> as it always has. Without knowing the specific device, the driver has
>> less chance to adapt when the unexpected is discovered. But again,
>> I'm not really discussing driver details, I'm complaining about the
>> proposed changes to the bindings. I simply don't see how they make
>> sense.
>>
> 
> My point is that saying in a DT binding that you could have a more
> specific <manufacturer,model> followed by a generic one is redundant.
> That's already something one can do to make the DT future proof and
> latter if is found that the more specific pair is needed, then
> compatible can be added to the OF device ID table making the driver to
> match that an the <manufacturer,model> added to the DT binding making
> it formal.
> 
> That's my understanding at least, but I could be completely wrong too
> since I'm not a DT expert.

My understanding is that in order to do that future proofing, you first
have to document the more specific binding. Note that documenting the
more specific binding will not add any burden whatsoever to the driver,
as long as the fallback is in place, at least until a difference
between the specific and the generic device or programming model is
discovered... So, until you need to use the specific compatible, it is
pure documentation overhead.

This 1/5 patch deprecates the more specific bindings, and that seems to
be the wrong direction. *More* of the specific compatibles should be
encouraged (and documented) instead of less.

But what do I know? Anyway, I'll shut up now, I think I've made my point...

Cheers,
Peter

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Peter Rosin]

On 2017-12-22 00:38, Javier Martinez Canillas wrote:
> Hello Peter,
> 
> On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
>>> Hello Peter,
>>>
>>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>>>>> Current description of the compatible property for at24 is quite vague.
>>>>>
>>>>> Specify an exact list of accepted compatibles and document the - now
>>>>> deprecated - strings which were previously used in device tree files.
>>>>
>>>> Why is it suddenly deprecated to correctly specify what hardware you
>>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
>>>
>>> Sorry but I disagree.
>>>
>>> When you specify a compatible string, you are not specifying a
>>> particular hardware but a device programming model.
>>
>> That's not what it says in https://elinux.org/Device_Tree_Usage
> 
> I think the most up-to-date DT reference is at:
> 
> https://www.devicetree.org/
> 
>> in the "Understanding the compatible Property" section. I quote:
>>
>>         compatible is a list of strings. The first string in the
>>         list specifies the exact device that the node represents
>>         in the form "<manufacturer>,<model>". The following strings
>>         represent other devices that the device is compatible with.
>>
>> Pretty clearly talks about devices and not programming models. But
>> maybe I shouldn't trust that reference? What should I be reading
>> instead?
>>
> 
> For example the latest spec draft
> (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
> says:
> 
> "The compatible property value consists of one or more strings that
> define the specific programming model for the device. This list of
> strings should be used by a client program for device driver
> selection. The property value consists of a concatenated list of null
> terminated strings, from most specific to most general. They allow a
> device to express its compatibility with a family of similar devices,
> potentially allowing a single device driver to match against several
> devices."
> 
> The recommended format is "manufacturer,model", where manufacturer is
> a string describing the name of the manufacturer (such as a stock
> ticker symbol), and model specifies the model number."
> 
> Example:
> 
> compatible = "fsl,mpc8641", "ns16550";
> 
> In this example, an operating system would first try to locate a
> device driver that supported fsl,mpc8641. If a driver was not found,
> it would then try to locate a driver that supported the more general
> ns16550 device type."

Yes, that's a bit different from the wording on the elinux site. Thanks
for the pointer! Google was not my friend on this occasion, since I
managed to miss that site in my searches. Maybe it has gotten a higher
rank now that 0.2 is out, because now I find it easily?

>>> It's very common to use a compatible string that doesn't match exactly
>>> the specific hardware used. That's why it's called _compatible_ BTW.
>>
>> That's not how I read the above.
>>
> 
> That's not how I read it nor my experience with DT, but of course I
> may be wrong on this.

Either way, it should not be wrong to specify the more specific binding
before the generic fallback, as is done in the at91-tse850-3.dts
example I gave below.

>>> For example when a DTS define a UART node with an ns16550 compatible,
>>> they don't really mean that have a UART IC manufactured by National
>>> Semiconductor.
>>
>> That just tells me that most people are a little bit lazy and ready
>> to cut a corner or two when they can get away with it. Or that there
>> is some form of misunderstanding at work...
>>
> 
> For example, I usually see that different SoC families from the same
> vendor use the same compatible string for integrated peripherals just
> because are the same from a programming model point of view.
> 
> TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
> and its similar on Exynos SoCs which are the two ARM SoCs I'm most
> familiar with. Following your logic that's wrong and instead a new
> compatible string should be added for the GPIO or pinctrl drivers even
> when are the same because refer to different devices.

Considering the above quote from the actual DT spec, I wouldn't say wrong.
But I'd still say that it should be preferred to list the actual device
before the fallback to some generic programming model compatible or some
previous version of the hardware/IP-block. Just in case an unintended
difference is discovered late in the game...

>>>> Sure, it happens to be compatible with "atmel,24c32", but that is
>>>> supposed to be written with a fallback as
>>>>
>>>>         "nxp,24c32", "atmel,24c32"
>>>
>>> This isn't a requirement really, systems integrators are free to use
>>> more than one <manufacturer,model> tuple in the compatible string if
>>> they want the DTB to be future proof, just in case there's a need for
>>> a more specific driver or if the programming model happened to not be
>>> the same at the end. This is usually done for the boards compatible
>>> string as an example, even when there isn't a struct machine_desc for
>>> the specific board compatible and only for the SoC family.
>>>
>>> So it's OK if you want to define the compatible as "nxp,24c32",
>>> "atmel,24c32", but that's a general OF concept and not something
>>> related to the at24 eeprom driver so I'm not sure if it should be
>>> mentioned in the at24 DT binding doc.
>>
>> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
>> below) is not a valid compatible, the tooling will be correct to
>> complain about it. Currently, it is just a checkpatch deficiency that
>> it complains like this:
>>
>> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
>> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
>> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
>> +               compatible = "nxp,24c02", "atmel,24c02";
>>
> 
> But isn't that a bug in checkpatch? as long as there's a valid
> <manufacturer,model> tuple in the compatible string, it shouldn't
> complain.

Then there is a significant risk that speeling mistakes are not
caught. So, I don't think it will be considered a bug unless a DT
guru says so of course. I think the checkpatch intention is to catch
all undocumented compatibles. But I don't know of course...

>>>> if I understand correctly. So, why is that deprecated in this case?
>>>>
>>>> What if (a few years down the line) it is discovered that some weird
>>>> quirk is needed that is only appropriate for nxp chips?
>>>>
>>>> nxp is of course just an example, pick any manufacturer of eeproms
>>>> (supposedly) compatible with the atmel interface.
>>>>
>>>
>>> That's indeed a possibility and the reason why most DT nodes have a
>>> more specific <manufacturer,model> before the generic one matched by
>>> drivers. I really doubt that in the future it will be found that a
>>> more specific compatible string is needed for one manufacturer in this
>>> case, specially since the driver didn't even care about the
>>> manufacturers until recently.
>>
>> I'm not talking about the driver. But if what you say is true, that
>> change would have broken a number of existing DTBs. Is that really the
>> case?
>>
> 
> Sorry, I didn't get which change you meant.

I meant whatever change you implied with "the driver didn't even care
about the manufacturers until recently". However, on second reading it
may be the case that "care" did not imply that a breaking change was
made, that was however my interpretation. Oh well...

>>> So I think that makes no sense for a driver to support all possible
>>> manufacturers as possible compatible strings if all the devices have
>>> the same exact programming model.
>>
>> That's what the fallback is for. With that in place the driver can
>> start to care when there is a need but until then it can work just
>> as it always has. Without knowing the specific device, the driver has
>> less chance to adapt when the unexpected is discovered. But again,
>> I'm not really discussing driver details, I'm complaining about the
>> proposed changes to the bindings. I simply don't see how they make
>> sense.
>>
> 
> My point is that saying in a DT binding that you could have a more
> specific <manufacturer,model> followed by a generic one is redundant.
> That's already something one can do to make the DT future proof and
> latter if is found that the more specific pair is needed, then
> compatible can be added to the OF device ID table making the driver to
> match that an the <manufacturer,model> added to the DT binding making
> it formal.
> 
> That's my understanding at least, but I could be completely wrong too
> since I'm not a DT expert.

My understanding is that in order to do that future proofing, you first
have to document the more specific binding. Note that documenting the
more specific binding will not add any burden whatsoever to the driver,
as long as the fallback is in place, at least until a difference
between the specific and the generic device or programming model is
discovered... So, until you need to use the specific compatible, it is
pure documentation overhead.

This 1/5 patch deprecates the more specific bindings, and that seems to
be the wrong direction. *More* of the specific compatibles should be
encouraged (and documented) instead of less.

But what do I know? Anyway, I'll shut up now, I think I've made my point...

Cheers,
Peter
--
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

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Rob Herring]

On Fri, Dec 22, 2017 at 05:58:29PM +0100, Peter Rosin wrote:
> On 2017-12-22 00:38, Javier Martinez Canillas wrote:
> > Hello Peter,
> > 
> > On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda@axentia.se> wrote:
> >> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
> >>> Hello Peter,
> >>>
> >>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
> >>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
> >>>>> Current description of the compatible property for at24 is quite vague.
> >>>>>
> >>>>> Specify an exact list of accepted compatibles and document the - now
> >>>>> deprecated - strings which were previously used in device tree files.
> >>>>
> >>>> Why is it suddenly deprecated to correctly specify what hardware you
> >>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
> >>>
> >>> Sorry but I disagree.
> >>>
> >>> When you specify a compatible string, you are not specifying a
> >>> particular hardware but a device programming model.
> >>
> >> That's not what it says in https://elinux.org/Device_Tree_Usage
> > 
> > I think the most up-to-date DT reference is at:
> > 
> > https://www.devicetree.org/
> > 
> >> in the "Understanding the compatible Property" section. I quote:
> >>
> >>         compatible is a list of strings. The first string in the
> >>         list specifies the exact device that the node represents
> >>         in the form "<manufacturer>,<model>". The following strings
> >>         represent other devices that the device is compatible with.
> >>
> >> Pretty clearly talks about devices and not programming models. But
> >> maybe I shouldn't trust that reference? What should I be reading
> >> instead?
> >>
> > 
> > For example the latest spec draft
> > (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
> > says:
> > 
> > "The compatible property value consists of one or more strings that
> > define the specific programming model for the device. This list of
> > strings should be used by a client program for device driver
> > selection. The property value consists of a concatenated list of null
> > terminated strings, from most specific to most general. They allow a
> > device to express its compatibility with a family of similar devices,
> > potentially allowing a single device driver to match against several
> > devices."
> > 
> > The recommended format is "manufacturer,model", where manufacturer is
> > a string describing the name of the manufacturer (such as a stock
> > ticker symbol), and model specifies the model number."
> > 
> > Example:
> > 
> > compatible = "fsl,mpc8641", "ns16550";
> > 
> > In this example, an operating system would first try to locate a
> > device driver that supported fsl,mpc8641. If a driver was not found,
> > it would then try to locate a driver that supported the more general
> > ns16550 device type."
> 
> Yes, that's a bit different from the wording on the elinux site. Thanks
> for the pointer! Google was not my friend on this occasion, since I
> managed to miss that site in my searches. Maybe it has gotten a higher
> rank now that 0.2 is out, because now I find it easily?

The wording may be different, but both (and Peter) are correct.

And ns16550 is a perfect example of why that string alone is not 
sufficient. Just go count the number of variants and quirks in the 
kernel 8250 driver.

> >>> It's very common to use a compatible string that doesn't match exactly
> >>> the specific hardware used. That's why it's called _compatible_ BTW.
> >>
> >> That's not how I read the above.
> >>
> > 
> > That's not how I read it nor my experience with DT, but of course I
> > may be wrong on this.
> 
> Either way, it should not be wrong to specify the more specific binding
> before the generic fallback, as is done in the at91-tse850-3.dts
> example I gave below.

Indeed. I don't think I've ever said anyone is being too specific.

> >>> For example when a DTS define a UART node with an ns16550 compatible,
> >>> they don't really mean that have a UART IC manufactured by National
> >>> Semiconductor.
> >>
> >> That just tells me that most people are a little bit lazy and ready
> >> to cut a corner or two when they can get away with it. Or that there
> >> is some form of misunderstanding at work...
> >>
> > 
> > For example, I usually see that different SoC families from the same
> > vendor use the same compatible string for integrated peripherals just
> > because are the same from a programming model point of view.
> > 
> > TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
> > and its similar on Exynos SoCs which are the two ARM SoCs I'm most
> > familiar with. Following your logic that's wrong and instead a new
> > compatible string should be added for the GPIO or pinctrl drivers even
> > when are the same because refer to different devices.
> 
> Considering the above quote from the actual DT spec, I wouldn't say wrong.
> But I'd still say that it should be preferred to list the actual device
> before the fallback to some generic programming model compatible or some
> previous version of the hardware/IP-block. Just in case an unintended
> difference is discovered late in the game...

Let me clear, "generic" compatibles alone are wrong.

> 
> >>>> Sure, it happens to be compatible with "atmel,24c32", but that is
> >>>> supposed to be written with a fallback as
> >>>>
> >>>>         "nxp,24c32", "atmel,24c32"
> >>>
> >>> This isn't a requirement really, systems integrators are free to use
> >>> more than one <manufacturer,model> tuple in the compatible string if
> >>> they want the DTB to be future proof, just in case there's a need for
> >>> a more specific driver or if the programming model happened to not be
> >>> the same at the end. This is usually done for the boards compatible
> >>> string as an example, even when there isn't a struct machine_desc for
> >>> the specific board compatible and only for the SoC family.
> >>>
> >>> So it's OK if you want to define the compatible as "nxp,24c32",
> >>> "atmel,24c32", but that's a general OF concept and not something
> >>> related to the at24 eeprom driver so I'm not sure if it should be
> >>> mentioned in the at24 DT binding doc.
> >>
> >> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
> >> below) is not a valid compatible, the tooling will be correct to
> >> complain about it. Currently, it is just a checkpatch deficiency that
> >> it complains like this:
> >>
> >> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
> >> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
> >> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
> >> +               compatible = "nxp,24c02", "atmel,24c02";
> >>
> > 
> > But isn't that a bug in checkpatch? as long as there's a valid
> > <manufacturer,model> tuple in the compatible string, it shouldn't
> > complain.
> 
> Then there is a significant risk that speeling mistakes are not
> caught. So, I don't think it will be considered a bug unless a DT
> guru says so of course. I think the checkpatch intention is to catch
> all undocumented compatibles. But I don't know of course...

checkpatch.pl is just plain stupid. Who wrote that check anyway? Because 
bindings are not machine parseable, compatible string checks are pretty 
much just a grep for the string in all binding docs. So "The" or 
"compatible" would be valid compatible strings for checkpatch.pl.

If you want to quiet checkpatch.pl, I'd suggest documenting compatible 
strings as "<vendor>,foo" and having checkpatch match any string for 
<vendor>. We already handle vendor,<soc>-block type compatible 
descriptions.

Rob

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Bartosz Golaszewski]

2017-12-26 22:32 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Fri, Dec 22, 2017 at 05:58:29PM +0100, Peter Rosin wrote:
>> On 2017-12-22 00:38, Javier Martinez Canillas wrote:
>> > Hello Peter,
>> >
>> > On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda@axentia.se> wrote:
>> >> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
>> >>> Hello Peter,
>> >>>
>> >>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda@axentia.se> wrote:
>> >>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>> >>>>> Current description of the compatible property for at24 is quite vague.
>> >>>>>
>> >>>>> Specify an exact list of accepted compatibles and document the - now
>> >>>>> deprecated - strings which were previously used in device tree files.
>> >>>>
>> >>>> Why is it suddenly deprecated to correctly specify what hardware you
>> >>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
>> >>>
>> >>> Sorry but I disagree.
>> >>>
>> >>> When you specify a compatible string, you are not specifying a
>> >>> particular hardware but a device programming model.
>> >>
>> >> That's not what it says in https://elinux.org/Device_Tree_Usage
>> >
>> > I think the most up-to-date DT reference is at:
>> >
>> > https://www.devicetree.org/
>> >
>> >> in the "Understanding the compatible Property" section. I quote:
>> >>
>> >>         compatible is a list of strings. The first string in the
>> >>         list specifies the exact device that the node represents
>> >>         in the form "<manufacturer>,<model>". The following strings
>> >>         represent other devices that the device is compatible with.
>> >>
>> >> Pretty clearly talks about devices and not programming models. But
>> >> maybe I shouldn't trust that reference? What should I be reading
>> >> instead?
>> >>
>> >
>> > For example the latest spec draft
>> > (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
>> > says:
>> >
>> > "The compatible property value consists of one or more strings that
>> > define the specific programming model for the device. This list of
>> > strings should be used by a client program for device driver
>> > selection. The property value consists of a concatenated list of null
>> > terminated strings, from most specific to most general. They allow a
>> > device to express its compatibility with a family of similar devices,
>> > potentially allowing a single device driver to match against several
>> > devices."
>> >
>> > The recommended format is "manufacturer,model", where manufacturer is
>> > a string describing the name of the manufacturer (such as a stock
>> > ticker symbol), and model specifies the model number."
>> >
>> > Example:
>> >
>> > compatible = "fsl,mpc8641", "ns16550";
>> >
>> > In this example, an operating system would first try to locate a
>> > device driver that supported fsl,mpc8641. If a driver was not found,
>> > it would then try to locate a driver that supported the more general
>> > ns16550 device type."
>>
>> Yes, that's a bit different from the wording on the elinux site. Thanks
>> for the pointer! Google was not my friend on this occasion, since I
>> managed to miss that site in my searches. Maybe it has gotten a higher
>> rank now that 0.2 is out, because now I find it easily?
>
> The wording may be different, but both (and Peter) are correct.
>
> And ns16550 is a perfect example of why that string alone is not
> sufficient. Just go count the number of variants and quirks in the
> kernel 8250 driver.
>
>> >>> It's very common to use a compatible string that doesn't match exactly
>> >>> the specific hardware used. That's why it's called _compatible_ BTW.
>> >>
>> >> That's not how I read the above.
>> >>
>> >
>> > That's not how I read it nor my experience with DT, but of course I
>> > may be wrong on this.
>>
>> Either way, it should not be wrong to specify the more specific binding
>> before the generic fallback, as is done in the at91-tse850-3.dts
>> example I gave below.
>
> Indeed. I don't think I've ever said anyone is being too specific.
>
>> >>> For example when a DTS define a UART node with an ns16550 compatible,
>> >>> they don't really mean that have a UART IC manufactured by National
>> >>> Semiconductor.
>> >>
>> >> That just tells me that most people are a little bit lazy and ready
>> >> to cut a corner or two when they can get away with it. Or that there
>> >> is some form of misunderstanding at work...
>> >>
>> >
>> > For example, I usually see that different SoC families from the same
>> > vendor use the same compatible string for integrated peripherals just
>> > because are the same from a programming model point of view.
>> >
>> > TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
>> > and its similar on Exynos SoCs which are the two ARM SoCs I'm most
>> > familiar with. Following your logic that's wrong and instead a new
>> > compatible string should be added for the GPIO or pinctrl drivers even
>> > when are the same because refer to different devices.
>>
>> Considering the above quote from the actual DT spec, I wouldn't say wrong.
>> But I'd still say that it should be preferred to list the actual device
>> before the fallback to some generic programming model compatible or some
>> previous version of the hardware/IP-block. Just in case an unintended
>> difference is discovered late in the game...
>
> Let me clear, "generic" compatibles alone are wrong.
>
>>
>> >>>> Sure, it happens to be compatible with "atmel,24c32", but that is
>> >>>> supposed to be written with a fallback as
>> >>>>
>> >>>>         "nxp,24c32", "atmel,24c32"
>> >>>
>> >>> This isn't a requirement really, systems integrators are free to use
>> >>> more than one <manufacturer,model> tuple in the compatible string if
>> >>> they want the DTB to be future proof, just in case there's a need for
>> >>> a more specific driver or if the programming model happened to not be
>> >>> the same at the end. This is usually done for the boards compatible
>> >>> string as an example, even when there isn't a struct machine_desc for
>> >>> the specific board compatible and only for the SoC family.
>> >>>
>> >>> So it's OK if you want to define the compatible as "nxp,24c32",
>> >>> "atmel,24c32", but that's a general OF concept and not something
>> >>> related to the at24 eeprom driver so I'm not sure if it should be
>> >>> mentioned in the at24 DT binding doc.
>> >>
>> >> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
>> >> below) is not a valid compatible, the tooling will be correct to
>> >> complain about it. Currently, it is just a checkpatch deficiency that
>> >> it complains like this:
>> >>
>> >> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
>> >> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
>> >> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
>> >> +               compatible = "nxp,24c02", "atmel,24c02";
>> >>
>> >
>> > But isn't that a bug in checkpatch? as long as there's a valid
>> > <manufacturer,model> tuple in the compatible string, it shouldn't
>> > complain.
>>
>> Then there is a significant risk that speeling mistakes are not
>> caught. So, I don't think it will be considered a bug unless a DT
>> guru says so of course. I think the checkpatch intention is to catch
>> all undocumented compatibles. But I don't know of course...
>
> checkpatch.pl is just plain stupid. Who wrote that check anyway? Because
> bindings are not machine parseable, compatible string checks are pretty
> much just a grep for the string in all binding docs. So "The" or
> "compatible" would be valid compatible strings for checkpatch.pl.
>
> If you want to quiet checkpatch.pl, I'd suggest documenting compatible
> strings as "<vendor>,foo" and having checkpatch match any string for
> <vendor>. We already handle vendor,<soc>-block type compatible
> descriptions.
>
> Rob

Hi Rob,

more than shutting up checkpatch, I care about cleaning up the device
tree support in at24 and extending it with other variants that are
currently supported as i2c devices.

While just listing the supported models with any vendor allowed would
be fine in most cases, there are also examples like
"renesas,r1ex24002" which is really just "atmel,24c02" but with a
different model name. So should we say in the document that 'any
"<manufacturer>,<model>" pair is allowed as long as the fallback is
one of "atmel,<model>" strings'?

Best regards,
Bartosz Golaszewski

Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property

[Bartosz Golaszewski]

2017-12-26 22:32 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Fri, Dec 22, 2017 at 05:58:29PM +0100, Peter Rosin wrote:
>> On 2017-12-22 00:38, Javier Martinez Canillas wrote:
>> > Hello Peter,
>> >
>> > On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>> >> On 2017-12-21 21:27, Javier Martinez Canillas wrote:
>> >>> Hello Peter,
>> >>>
>> >>> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>> >>>> On 2017-12-21 14:48, Bartosz Golaszewski wrote:
>> >>>>> Current description of the compatible property for at24 is quite vague.
>> >>>>>
>> >>>>> Specify an exact list of accepted compatibles and document the - now
>> >>>>> deprecated - strings which were previously used in device tree files.
>> >>>>
>> >>>> Why is it suddenly deprecated to correctly specify what hardware you
>> >>>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit.
>> >>>
>> >>> Sorry but I disagree.
>> >>>
>> >>> When you specify a compatible string, you are not specifying a
>> >>> particular hardware but a device programming model.
>> >>
>> >> That's not what it says in https://elinux.org/Device_Tree_Usage
>> >
>> > I think the most up-to-date DT reference is at:
>> >
>> > https://www.devicetree.org/
>> >
>> >> in the "Understanding the compatible Property" section. I quote:
>> >>
>> >>         compatible is a list of strings. The first string in the
>> >>         list specifies the exact device that the node represents
>> >>         in the form "<manufacturer>,<model>". The following strings
>> >>         represent other devices that the device is compatible with.
>> >>
>> >> Pretty clearly talks about devices and not programming models. But
>> >> maybe I shouldn't trust that reference? What should I be reading
>> >> instead?
>> >>
>> >
>> > For example the latest spec draft
>> > (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf)
>> > says:
>> >
>> > "The compatible property value consists of one or more strings that
>> > define the specific programming model for the device. This list of
>> > strings should be used by a client program for device driver
>> > selection. The property value consists of a concatenated list of null
>> > terminated strings, from most specific to most general. They allow a
>> > device to express its compatibility with a family of similar devices,
>> > potentially allowing a single device driver to match against several
>> > devices."
>> >
>> > The recommended format is "manufacturer,model", where manufacturer is
>> > a string describing the name of the manufacturer (such as a stock
>> > ticker symbol), and model specifies the model number."
>> >
>> > Example:
>> >
>> > compatible = "fsl,mpc8641", "ns16550";
>> >
>> > In this example, an operating system would first try to locate a
>> > device driver that supported fsl,mpc8641. If a driver was not found,
>> > it would then try to locate a driver that supported the more general
>> > ns16550 device type."
>>
>> Yes, that's a bit different from the wording on the elinux site. Thanks
>> for the pointer! Google was not my friend on this occasion, since I
>> managed to miss that site in my searches. Maybe it has gotten a higher
>> rank now that 0.2 is out, because now I find it easily?
>
> The wording may be different, but both (and Peter) are correct.
>
> And ns16550 is a perfect example of why that string alone is not
> sufficient. Just go count the number of variants and quirks in the
> kernel 8250 driver.
>
>> >>> It's very common to use a compatible string that doesn't match exactly
>> >>> the specific hardware used. That's why it's called _compatible_ BTW.
>> >>
>> >> That's not how I read the above.
>> >>
>> >
>> > That's not how I read it nor my experience with DT, but of course I
>> > may be wrong on this.
>>
>> Either way, it should not be wrong to specify the more specific binding
>> before the generic fallback, as is done in the at91-tse850-3.dts
>> example I gave below.
>
> Indeed. I don't think I've ever said anyone is being too specific.
>
>> >>> For example when a DTS define a UART node with an ns16550 compatible,
>> >>> they don't really mean that have a UART IC manufactured by National
>> >>> Semiconductor.
>> >>
>> >> That just tells me that most people are a little bit lazy and ready
>> >> to cut a corner or two when they can get away with it. Or that there
>> >> is some form of misunderstanding at work...
>> >>
>> >
>> > For example, I usually see that different SoC families from the same
>> > vendor use the same compatible string for integrated peripherals just
>> > because are the same from a programming model point of view.
>> >
>> > TI am33xx SoCs use a lot of omap3 compatible strings on their nodes
>> > and its similar on Exynos SoCs which are the two ARM SoCs I'm most
>> > familiar with. Following your logic that's wrong and instead a new
>> > compatible string should be added for the GPIO or pinctrl drivers even
>> > when are the same because refer to different devices.
>>
>> Considering the above quote from the actual DT spec, I wouldn't say wrong.
>> But I'd still say that it should be preferred to list the actual device
>> before the fallback to some generic programming model compatible or some
>> previous version of the hardware/IP-block. Just in case an unintended
>> difference is discovered late in the game...
>
> Let me clear, "generic" compatibles alone are wrong.
>
>>
>> >>>> Sure, it happens to be compatible with "atmel,24c32", but that is
>> >>>> supposed to be written with a fallback as
>> >>>>
>> >>>>         "nxp,24c32", "atmel,24c32"
>> >>>
>> >>> This isn't a requirement really, systems integrators are free to use
>> >>> more than one <manufacturer,model> tuple in the compatible string if
>> >>> they want the DTB to be future proof, just in case there's a need for
>> >>> a more specific driver or if the programming model happened to not be
>> >>> the same at the end. This is usually done for the boards compatible
>> >>> string as an example, even when there isn't a struct machine_desc for
>> >>> the specific board compatible and only for the SoC family.
>> >>>
>> >>> So it's OK if you want to define the compatible as "nxp,24c32",
>> >>> "atmel,24c32", but that's a general OF concept and not something
>> >>> related to the at24 eeprom driver so I'm not sure if it should be
>> >>> mentioned in the at24 DT binding doc.
>> >>
>> >> One problem is that if "nxp,24c32" (or "nxp,24c02" as in the example
>> >> below) is not a valid compatible, the tooling will be correct to
>> >> complain about it. Currently, it is just a checkpatch deficiency that
>> >> it complains like this:
>> >>
>> >> $ scripts/checkpatch.pl -f arch/arm/boot/dts/at91-tse850-3.dts
>> >> WARNING: DT compatible string "nxp,24c02" appears un-documented -- check ./Documentation/devicetree/bindings/
>> >> #249: FILE: arch/arm/boot/dts/at91-tse850-3.dts:249:
>> >> +               compatible = "nxp,24c02", "atmel,24c02";
>> >>
>> >
>> > But isn't that a bug in checkpatch? as long as there's a valid
>> > <manufacturer,model> tuple in the compatible string, it shouldn't
>> > complain.
>>
>> Then there is a significant risk that speeling mistakes are not
>> caught. So, I don't think it will be considered a bug unless a DT
>> guru says so of course. I think the checkpatch intention is to catch
>> all undocumented compatibles. But I don't know of course...
>
> checkpatch.pl is just plain stupid. Who wrote that check anyway? Because
> bindings are not machine parseable, compatible string checks are pretty
> much just a grep for the string in all binding docs. So "The" or
> "compatible" would be valid compatible strings for checkpatch.pl.
>
> If you want to quiet checkpatch.pl, I'd suggest documenting compatible
> strings as "<vendor>,foo" and having checkpatch match any string for
> <vendor>. We already handle vendor,<soc>-block type compatible
> descriptions.
>
> Rob

Hi Rob,

more than shutting up checkpatch, I care about cleaning up the device
tree support in at24 and extending it with other variants that are
currently supported as i2c devices.

While just listing the supported models with any vendor allowed would
be fine in most cases, there are also examples like
"renesas,r1ex24002" which is really just "atmel,24c02" but with a
different model name. So should we say in the document that 'any
"<manufacturer>,<model>" pair is allowed as long as the fallback is
one of "atmel,<model>" strings'?

Best regards,
Bartosz Golaszewski
--
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

[PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Bartosz Golaszewski]

"atmel,spd" is reported by checkpatch as undocumented in the device
tree bindings. Add it to the list of supported compatible strings.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index 16b687458b14..2702f2456971 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -7,6 +7,7 @@ Required properties:
         "atmel,24c00",
         "atmel,24c01",
         "atmel,24c02",
+        "atmel,spd",
         "atmel,24c04",
         "atmel,24c08",
         "atmel,24c16",
-- 
2.15.1

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> "atmel,spd" is reported by checkpatch as undocumented in the device
> tree bindings. Add it to the list of supported compatible strings.

>          "atmel,24c00",
>          "atmel,24c01",
>          "atmel,24c02",
> +        "atmel,spd",
>          "atmel,24c04",
>          "atmel,24c08",
>          "atmel,24c16",

Sounds alogical to me to make a split by this new record.
Can you find better line to inject?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
> "atmel,spd" is reported by checkpatch as undocumented in the device
> tree bindings. Add it to the list of supported compatible strings.

>          "atmel,24c00",
>          "atmel,24c01",
>          "atmel,24c02",
> +        "atmel,spd",
>          "atmel,24c04",
>          "atmel,24c08",
>          "atmel,24c16",

Sounds alogical to me to make a split by this new record.
Can you find better line to inject?

-- 
With Best Regards,
Andy Shevchenko
--
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

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Bartosz Golaszewski]

2017-12-21 15:08 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> "atmel,spd" is reported by checkpatch as undocumented in the device
>> tree bindings. Add it to the list of supported compatible strings.
>
>>          "atmel,24c00",
>>          "atmel,24c01",
>>          "atmel,24c02",
>> +        "atmel,spd",
>>          "atmel,24c04",
>>          "atmel,24c08",
>>          "atmel,24c16",
>
> Sounds alogical to me to make a split by this new record.
> Can you find better line to inject?
>
> --
> With Best Regards,
> Andy Shevchenko

They are actually ordered by memory size. I want to keep it like this
in the driver and I prefer that the DT reflect it.

Thanks,
Bartosz

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 4:18 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-12-21 15:08 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>> "atmel,spd" is reported by checkpatch as undocumented in the device
>>> tree bindings. Add it to the list of supported compatible strings.
>>
>>>          "atmel,24c00",
>>>          "atmel,24c01",
>>>          "atmel,24c02",
>>> +        "atmel,spd",
>>>          "atmel,24c04",
>>>          "atmel,24c08",
>>>          "atmel,24c16",
>>
>> Sounds alogical to me to make a split by this new record.
>> Can you find better line to inject?

> They are actually ordered by memory size. I want to keep it like this
> in the driver and I prefer that the DT reflect it.

So, I just disagree on the above. Rationale I described at one of the comment.

At the end it's your call, but from my p.o.v. it makes life harder to
read and catch the chips which are (un)supported.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 4:18 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
> 2017-12-21 15:08 GMT+01:00 Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
>>> "atmel,spd" is reported by checkpatch as undocumented in the device
>>> tree bindings. Add it to the list of supported compatible strings.
>>
>>>          "atmel,24c00",
>>>          "atmel,24c01",
>>>          "atmel,24c02",
>>> +        "atmel,spd",
>>>          "atmel,24c04",
>>>          "atmel,24c08",
>>>          "atmel,24c16",
>>
>> Sounds alogical to me to make a split by this new record.
>> Can you find better line to inject?

> They are actually ordered by memory size. I want to keep it like this
> in the driver and I prefer that the DT reflect it.

So, I just disagree on the above. Rationale I described at one of the comment.

At the end it's your call, but from my p.o.v. it makes life harder to
read and catch the chips which are (un)supported.

-- 
With Best Regards,
Andy Shevchenko
--
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

Re: [PATCH v2 2/5] dt-bindings: at24: add a missing compatible

[Bartosz Golaszewski]

2017-12-21 15:25 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Thu, Dec 21, 2017 at 4:18 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> 2017-12-21 15:08 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>> On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>> "atmel,spd" is reported by checkpatch as undocumented in the device
>>>> tree bindings. Add it to the list of supported compatible strings.
>>>
>>>>          "atmel,24c00",
>>>>          "atmel,24c01",
>>>>          "atmel,24c02",
>>>> +        "atmel,spd",
>>>>          "atmel,24c04",
>>>>          "atmel,24c08",
>>>>          "atmel,24c16",
>>>
>>> Sounds alogical to me to make a split by this new record.
>>> Can you find better line to inject?
>
>> They are actually ordered by memory size. I want to keep it like this
>> in the driver and I prefer that the DT reflect it.
>
> So, I just disagree on the above. Rationale I described at one of the comment.
>
> At the end it's your call, but from my p.o.v. it makes life harder to
> read and catch the chips which are (un)supported.
>
> --
> With Best Regards,
> Andy Shevchenko

Yes, I prefer this version. Another reason to keep it like this is to
group chip variants together. E.g: atmel,24cs02, atmel,spd,
atmel,24mac402, atmel,24mac602 are all variants of atmel,24c02.

Thanks,
Bartosz

[PATCH v2 3/5] dt-bindings: at24: fix formatting and style

[Bartosz Golaszewski]

Make formatting and style consistent for the entire document.

This patch doesn't change the content of the binding.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 24 +++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index 2702f2456971..1068b4309fed 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -2,7 +2,7 @@ EEPROMs (I2C)
 
 Required properties:
 
-  - compatible: must be one of the following:
+  - compatible: Must be one of the following:
 
         "atmel,24c00",
         "atmel,24c01",
@@ -30,23 +30,23 @@ Required properties:
 
   will still work, but are now deprecated.
 
-  - reg : the I2C address of the EEPROM
+  - reg: The I2C address of the EEPROM.
 
 Optional properties:
 
-  - pagesize : the length of the pagesize for writing. Please consult the
-               manual of your device, that value varies a lot. A wrong value
-	       may result in data loss! If not specified, a safety value of
-	       '1' is used which will be very slow.
+  - pagesize: The length of the pagesize for writing. Please consult the
+              manual of your device, that value varies a lot. A wrong value
+              may result in data loss! If not specified, a safety value of
+              '1' is used which will be very slow.
 
-  - read-only: this parameterless property disables writes to the eeprom
+  - read-only: This parameterless property disables writes to the eeprom.
 
-  - size: total eeprom size in bytes
+  - size: Total eeprom size in bytes.
 
-  - no-read-rollover:
-			This parameterless property indicates that the multi-address
-			eeprom does not automatically roll over reads to the next
-			slave address. Please consult the manual of your device.
+  - no-read-rollover: This parameterless property indicates that the
+                      multi-address eeprom does not automatically roll over
+                      reads to the next slave address. Please consult the
+                      manual of your device.
 
   - wp-gpios: GPIO to which the write-protect pin of the chip is connected.
 
-- 
2.15.1

[PATCH v2 4/5] dt-bindings: at24: extend the list of supported chips

[Bartosz Golaszewski]

Add other variants of at24 EEPROMs we support in the driver to the
list of allowed compatible strings.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index 1068b4309fed..ea2707f769d0 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -6,13 +6,22 @@ Required properties:
 
         "atmel,24c00",
         "atmel,24c01",
+        "atmel,24cs01",
         "atmel,24c02",
+        "atmel,24cs02",
+        "atmel,24mac402",
+        "atmel,24mac602",
         "atmel,spd",
         "atmel,24c04",
+        "atmel,24cs04",
         "atmel,24c08",
+        "atmel,24cs08",
         "atmel,24c16",
+        "atmel,24cs16",
         "atmel,24c32",
+        "atmel,24cs32",
         "atmel,24c64",
+        "atmel,24cs64",
         "atmel,24c128",
         "atmel,24c256",
         "atmel,24c512",
-- 
2.15.1

[PATCH v2 4/5] dt-bindings: at24: extend the list of supported chips

[Bartosz Golaszewski]

Add other variants of at24 EEPROMs we support in the driver to the
list of allowed compatible strings.

Signed-off-by: Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org>
Reviewed-by: Javier Martinez Canillas <javierm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt b/Documentation/devicetree/bindings/eeprom/at24.txt
index 1068b4309fed..ea2707f769d0 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.txt
+++ b/Documentation/devicetree/bindings/eeprom/at24.txt
@@ -6,13 +6,22 @@ Required properties:
 
         "atmel,24c00",
         "atmel,24c01",
+        "atmel,24cs01",
         "atmel,24c02",
+        "atmel,24cs02",
+        "atmel,24mac402",
+        "atmel,24mac602",
         "atmel,spd",
         "atmel,24c04",
+        "atmel,24cs04",
         "atmel,24c08",
+        "atmel,24cs08",
         "atmel,24c16",
+        "atmel,24cs16",
         "atmel,24c32",
+        "atmel,24cs32",
         "atmel,24c64",
+        "atmel,24cs64",
         "atmel,24c128",
         "atmel,24c256",
         "atmel,24c512",
-- 
2.15.1

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

Re: [PATCH v2 4/5] dt-bindings: at24: extend the list of supported chips

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Add other variants of at24 EEPROMs we support in the driver to the
> list of allowed compatible strings.

>          "atmel,24c00",
>          "atmel,24c01",
> +        "atmel,24cs01",
>          "atmel,24c02",
> +        "atmel,24cs02",
> +        "atmel,24mac402",
> +        "atmel,24mac602",
>          "atmel,spd",
>          "atmel,24c04",
> +        "atmel,24cs04",
>          "atmel,24c08",
> +        "atmel,24cs08",
>          "atmel,24c16",
> +        "atmel,24cs16",
>          "atmel,24c32",

Same comments as per spd case.

For me more naturally to have them grouped by letters inside, i.e.
          "atmel,24c00",
          "atmel,24c01",
...
        "atmel,24cs01",
        "atmel,24cs02",
...
       "atmel,24mac402",
       "atmel,24mac602",

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] dt-bindings: at24: extend the list of supported chips

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
> Add other variants of at24 EEPROMs we support in the driver to the
> list of allowed compatible strings.

>          "atmel,24c00",
>          "atmel,24c01",
> +        "atmel,24cs01",
>          "atmel,24c02",
> +        "atmel,24cs02",
> +        "atmel,24mac402",
> +        "atmel,24mac602",
>          "atmel,spd",
>          "atmel,24c04",
> +        "atmel,24cs04",
>          "atmel,24c08",
> +        "atmel,24cs08",
>          "atmel,24c16",
> +        "atmel,24cs16",
>          "atmel,24c32",

Same comments as per spd case.

For me more naturally to have them grouped by letters inside, i.e.
          "atmel,24c00",
          "atmel,24c01",
...
        "atmel,24cs01",
        "atmel,24cs02",
...
       "atmel,24mac402",
       "atmel,24mac602",

-- 
With Best Regards,
Andy Shevchenko
--
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

[PATCH v2 5/5] eeprom: at24: extend the list of chips supported in DT

[Bartosz Golaszewski]

Add all supported at24 variants to the of_match table.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/misc/eeprom/at24.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index e79833d62284..01f9c4921c50 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -209,13 +209,22 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
 static const struct of_device_id at24_of_match[] = {
 	{ .compatible = "atmel,24c00",		.data = &at24_data_24c00 },
 	{ .compatible = "atmel,24c01",		.data = &at24_data_24c01 },
+	{ .compatible = "atmel,24cs01",		.data = &at24_data_24cs01 },
 	{ .compatible = "atmel,24c02",		.data = &at24_data_24c02 },
+	{ .compatible = "atmel,24cs02",		.data = &at24_data_24cs02 },
+	{ .compatible = "atmel,24mac402",	.data = &at24_data_24mac402 },
+	{ .compatible = "atmel,24mac602",	.data = &at24_data_24mac602 },
 	{ .compatible = "atmel,spd",		.data = &at24_data_spd },
 	{ .compatible = "atmel,24c04",		.data = &at24_data_24c04 },
+	{ .compatible = "atmel,24cs04",		.data = &at24_data_24cs04 },
 	{ .compatible = "atmel,24c08",		.data = &at24_data_24c08 },
+	{ .compatible = "atmel,24cs08",		.data = &at24_data_24cs08 },
 	{ .compatible = "atmel,24c16",		.data = &at24_data_24c16 },
+	{ .compatible = "atmel,24cs16",		.data = &at24_data_24cs16 },
 	{ .compatible = "atmel,24c32",		.data = &at24_data_24c32 },
+	{ .compatible = "atmel,24cs32",		.data = &at24_data_24cs32 },
 	{ .compatible = "atmel,24c64",		.data = &at24_data_24c64 },
+	{ .compatible = "atmel,24cs64",		.data = &at24_data_24cs64 },
 	{ .compatible = "atmel,24c128",		.data = &at24_data_24c128 },
 	{ .compatible = "atmel,24c256",		.data = &at24_data_24c256 },
 	{ .compatible = "atmel,24c512",		.data = &at24_data_24c512 },
-- 
2.15.1

[PATCH v2 5/5] eeprom: at24: extend the list of chips supported in DT

[Bartosz Golaszewski]

Add all supported at24 variants to the of_match table.

Signed-off-by: Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org>
Reviewed-by: Javier Martinez Canillas <javierm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/misc/eeprom/at24.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index e79833d62284..01f9c4921c50 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -209,13 +209,22 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
 static const struct of_device_id at24_of_match[] = {
 	{ .compatible = "atmel,24c00",		.data = &at24_data_24c00 },
 	{ .compatible = "atmel,24c01",		.data = &at24_data_24c01 },
+	{ .compatible = "atmel,24cs01",		.data = &at24_data_24cs01 },
 	{ .compatible = "atmel,24c02",		.data = &at24_data_24c02 },
+	{ .compatible = "atmel,24cs02",		.data = &at24_data_24cs02 },
+	{ .compatible = "atmel,24mac402",	.data = &at24_data_24mac402 },
+	{ .compatible = "atmel,24mac602",	.data = &at24_data_24mac602 },
 	{ .compatible = "atmel,spd",		.data = &at24_data_spd },
 	{ .compatible = "atmel,24c04",		.data = &at24_data_24c04 },
+	{ .compatible = "atmel,24cs04",		.data = &at24_data_24cs04 },
 	{ .compatible = "atmel,24c08",		.data = &at24_data_24c08 },
+	{ .compatible = "atmel,24cs08",		.data = &at24_data_24cs08 },
 	{ .compatible = "atmel,24c16",		.data = &at24_data_24c16 },
+	{ .compatible = "atmel,24cs16",		.data = &at24_data_24cs16 },
 	{ .compatible = "atmel,24c32",		.data = &at24_data_24c32 },
+	{ .compatible = "atmel,24cs32",		.data = &at24_data_24cs32 },
 	{ .compatible = "atmel,24c64",		.data = &at24_data_24c64 },
+	{ .compatible = "atmel,24cs64",		.data = &at24_data_24cs64 },
 	{ .compatible = "atmel,24c128",		.data = &at24_data_24c128 },
 	{ .compatible = "atmel,24c256",		.data = &at24_data_24c256 },
 	{ .compatible = "atmel,24c512",		.data = &at24_data_24c512 },
-- 
2.15.1

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

Re: [PATCH v2 5/5] eeprom: at24: extend the list of chips supported in DT

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Add all supported at24 variants to the of_match table.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Same comment as for previous patch.

It would be really easier to read and catch if you order by inside letters.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] eeprom: at24: extend the list of chips supported in DT

[Andy Shevchenko]

On Thu, Dec 21, 2017 at 3:48 PM, Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote:
> Add all supported at24 variants to the of_match table.
>
> Signed-off-by: Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org>
> Reviewed-by: Javier Martinez Canillas <javierm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Same comment as for previous patch.

It would be really easier to read and catch if you order by inside letters.

-- 
With Best Regards,
Andy Shevchenko
--
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