Re: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

Re: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Krzysztof Kozlowski]

On 28.10.2015 16:57, Pavel Fedin wrote:
> Add documentation for new properties, allowing bank configuration.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  .../bindings/arm/samsung/exynos-srom.txt           | 24 +++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)

You missed here a lot of potential Cc. Please use get_maintainer.pl
script. It *must* be sent to devicetree list.

Please send it also to DeviceTree maintainers because you are adding
quite generic names for bindings so they may have interesting thoughts
on this.

LKML list is also missing.

> 
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> index 33886d5..9e4a40b 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> @@ -5,8 +5,30 @@ Required properties:
>  
>  - reg: offset and length of the register set
>  
> -Example:
> +Bank configurations can be defined in optional subnodes. Each subnode
> +describes one bank and contains the following:
> +
> +Required properties:
> +- bank : bank number (0 - 3)

I wonder whether this should be maybe "reg"?

> +
> +- srom-timing : array of 7 integers: Tacp, Tcah, Tcoh, Tacc, Tcos, Tacs

Missing "pmc"? Where is the seventh?

This looks vendor specific, so prefix with "samsung,exynos-".

> +
> +Optional properties:
> +- width : data width in bytes (1 or 2). If omitted, default of 1 is used.

This is not bank-width? So data-width? Any vendor prefix here? How
generic is this?

These are first things which came to my mind. Maybe DT maintainers will
come with something more...

Best regards,
Krzysztof

> +
> +Example: basic definition, no banks are configured
> +	sromc@12570000 {
> +		compatible = "samsung,exynos-srom";
> +		reg = <0x12570000 0x10>;
> +	};
> +
> +Example: SROMc with bank3 configuration
>  	sromc@12570000 {
>  		compatible = "samsung,exynos-srom";
>  		reg = <0x12570000 0x10>;
> +		bank@3 {
> +			bank = <3>;
> +			width = <2>;
> +			srom-timing = <1 9 12 1 9 1 1>;
> +		};
>  	};
> 

RE: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Pavel Fedin]

 Hello!

> You missed here a lot of potential Cc. Please use get_maintainer.pl
> script. It *must* be sent to devicetree list.
> 
> Please send it also to DeviceTree maintainers because you are adding
> quite generic names for bindings so they may have interesting thoughts
> on this.
> 
> LKML list is also missing.

 Ok. Next version will go there too.

> Any vendor prefix here? How generic is this?

 I just don't know... Does *everything* really need a vendor prefix? How readable would that be? "compatible" property already says
that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-specific device are automatically
vendor-specific.
 Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree ML.

 P.S. I intentionally omit the rest of the text in order to avoid overquoting, since we are already focused on some specific things.
If someone wants to get the whole picture, he/she could just browse back to the start of the thread. Or is it considered wrong
approach in this particular ML?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Krzysztof Kozlowski]

2015-10-29 16:45 GMT+09:00 Pavel Fedin <p.fedin@samsung.com>:
>  Hello!
>
>> You missed here a lot of potential Cc. Please use get_maintainer.pl
>> script. It *must* be sent to devicetree list.
>>
>> Please send it also to DeviceTree maintainers because you are adding
>> quite generic names for bindings so they may have interesting thoughts
>> on this.
>>
>> LKML list is also missing.
>
>  Ok. Next version will go there too.
>
>> Any vendor prefix here? How generic is this?
>
>  I just don't know... Does *everything* really need a vendor prefix? How readable would that be? "compatible" property already says
> that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-specific device are automatically
> vendor-specific.
>  Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree ML.

Which my reply you are referring to? You stripped part of some
sentence and put it without *any context*. Just random sentence.
I asked for vendor prefix in few places... srom-timing? width? And I
do not remember where I used exactly these words. This is why
*context* is needed.

>
>  P.S. I intentionally omit the rest of the text in order to avoid overquoting, since we are already focused on some specific things.
> If someone wants to get the whole picture, he/she could just browse back to the start of the thread. Or is it considered wrong
> approach in this particular ML?

Yep, selecting some sentences and replying only to them, leaving the
context away, is highly confusing. I reply in different places,
sometimes asking for similar things. Removing context messes this up.
In the same time all of my other questions are being removed without
any acknowledgment so I do not know if they were ignored or just
silently acked.

I show you example:

>  I just don't know...
I don't know neither.

> If someone wants to get the whole picture, he/she could just browse back to the start of the thread.
Yes, I would just scroll up and see the context. No, wait. There is no
context. I have to find mine original email and compare which part was
ignored, which was applied and which quoted sentence you are referring
to.

Best regards,
Krzysztof

RE: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Pavel Fedin]

 Hello!

> >> Any vendor prefix here? How generic is this?
> >
> >  I just don't know... Does *everything* really need a vendor prefix? How readable would that
> be? "compatible" property already says
> > that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-
> specific device are automatically
> > vendor-specific.
> >  Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree
> ML.
> 
> Which my reply you are referring to? You stripped part of some
> sentence and put it without *any context*. Just random sentence.
> I asked for vendor prefix in few places... srom-timing? width? And I
> do not remember where I used exactly these words.

 Ok, sorry, i promise to improve. :)
 Anyway, i have figured out how to add sub-devices, and heavily modified the whole thing. And indeed, vendor prefix is now very useful, so i added it to all three properties. Making v4...

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v3 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Krzysztof Kozlowski]

On 29.10.2015 20:43, Pavel Fedin wrote:
>  Hello!
> 
>>>> Any vendor prefix here? How generic is this?
>>>
>>>  I just don't know... Does *everything* really need a vendor prefix? How readable would that
>> be? "compatible" property already says
>>> that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-
>> specific device are automatically
>>> vendor-specific.
>>>  Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree
>> ML.
>>
>> Which my reply you are referring to? You stripped part of some
>> sentence and put it without *any context*. Just random sentence.
>> I asked for vendor prefix in few places... srom-timing? width? And I
>> do not remember where I used exactly these words.
> 
>  Ok, sorry, i promise to improve. :)
>  Anyway, i have figured out how to add sub-devices, and heavily modified the whole thing. And indeed, vendor prefix is now very useful, so i added it to all three properties. Making v4...

Actually now I found:
Documentation/devicetree/bindings/net/gpmc-eth.txt

Aren't you duplicating this work? This looks very, very similar.

Best regards,
Krzysztof