On 09/18/2013 06:47 AM, Gabriel FERNANDEZ wrote:
> On 18/09/2013 12:01, Maxime COQUELIN wrote:>> This patch adds support to SSC (Synchronous Serial Controller)>> I2C driver. This IP also supports SPI protocol, but this is not>> the aim of this driver.>>>> This IP is embedded in all ST SoCs for Set-top box platorms, and>> supports I2C Standard and Fast modes.> ... [entire patch quoted for a one-line comment] ...
Please trim down the patch so you only quote relevant lines. Quoting the
entire patch for a 1-line comment makes people wast time searching for
your reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On 09/18/2013 04:01 AM, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)> I2C driver. This IP also supports SPI protocol, but this is not> the aim of this driver.> > This IP is embedded in all ST SoCs for Set-top box platorms, and> supports I2C Standard and Fast modes.
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> +I2C for ST platforms
If the HW block supports both I2C and SPI, the DT binding ought to
support that too. It's be best to create a single DT binding that
represents the IP block, and include a property that indicates whether
the device should operate in I2C or SPI mode.
I suppose you could reasonably define different compatible values for
those two cases. However, the binding should be titled something more
like "ST SSC binding, for I2C mode operation" and "ST SSC binding, for
SPI mode operation", rather than "I2C for ST platforms", since the HW
includes an SSC block, not an I2C block.
> +Required properties :> +- compatible : Must be "st,comms-i2c"> +- reg : Offset and length of the register set for the device> +- interrupts : the interrupt number
It's an interrupt specifier, not an interrupt number. The format is
defined by the interrupt controller, not this binding.
> +- clocks : phandle to the I2C clock source
What about clock-names?
> +Recommended (non-standard) properties :
Usually you'd just say "Optional properties:", or perhaps "Recommended
properties:". I don't think adding "(non-standard)" serves any purpose.
> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise> + the default 100 kHz frequency will be used. As only Normal and Fast modes> + are supported, possible values are 100000 and 400000.> +> +Optional (non-standard) properties:
Same here.
> +- st,glitches : Enable timing glitch suppression support.
That property name doesn't really convey the "enables" that appears in
the property description to me...
> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.
s/timinig/timing/
> +- st,hw-glitches : Enable filter glitch suppression support.> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.
Those sound more like runtime configuration rather than HW description.
Can you rephrase the descriptions (and property names) more along the
lines of HW properties? Perhaps e.g.:
st,needs-glitch-suppression: The board design needs timing glitch
suppression enabled to operate reliably.
st,min-scl-pulse-width: The minimum valid SCL pulse width that is
allowed through the deglitch circuit. In units of ns.
(I just made up those descriptions to give a feel for the flavor of
description that I expect. They likely need some adjustment to reflect
whatever they're actually intended to represent in HW).
What is the difference between "glitch" and "hw-glitch"?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On 09/23/2013 11:06 PM, Stephen Warren wrote:
> On 09/18/2013 04:01 AM, Maxime COQUELIN wrote:>> This patch adds support to SSC (Synchronous Serial Controller)>> I2C driver. This IP also supports SPI protocol, but this is not>> the aim of this driver.>>>> This IP is embedded in all ST SoCs for Set-top box platorms, and>> supports I2C Standard and Fast modes.>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt>> +I2C for ST platforms> If the HW block supports both I2C and SPI, the DT binding ought to> support that too. It's be best to create a single DT binding that> represents the IP block, and include a property that indicates whether> the device should operate in I2C or SPI mode.>> I suppose you could reasonably define different compatible values for> those two cases. However, the binding should be titled something more> like "ST SSC binding, for I2C mode operation" and "ST SSC binding, for> SPI mode operation", rather than "I2C for ST platforms", since the HW> includes an SSC block, not an I2C block.
You are right Stephen.
I will rename the title and change the compatible value as per your
recommendation.
>> +Required properties :>> +- compatible : Must be "st,comms-i2c">> +- reg : Offset and length of the register set for the device>> +- interrupts : the interrupt number> It's an interrupt specifier, not an interrupt number. The format is> defined by the interrupt controller, not this binding.
Ok. I will change to "the interrupt specifier".
>> +- clocks : phandle to the I2C clock source> What about clock-names?
It will be added in next revision. It will be named "ssc"
>> +Recommended (non-standard) properties :> Usually you'd just say "Optional properties:", or perhaps "Recommended> properties:". I don't think adding "(non-standard)" serves any purpose.
Ok. then I will remove all the "non-standard" occurences.
>> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise>> + the default 100 kHz frequency will be used. As only Normal and Fast modes>> + are supported, possible values are 100000 and 400000.>> +>> +Optional (non-standard) properties:> Same here.>>> +- st,glitches : Enable timing glitch suppression support.> That property name doesn't really convey the "enables" that appears in> the property description to me...>>> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.>> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.> s/timinig/timing/>>> +- st,hw-glitches : Enable filter glitch suppression support.>> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.>> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.> Those sound more like runtime configuration rather than HW description.> Can you rephrase the descriptions (and property names) more along the> lines of HW properties? Perhaps e.g.:>> st,needs-glitch-suppression: The board design needs timing glitch> suppression enabled to operate reliably.>> st,min-scl-pulse-width: The minimum valid SCL pulse width that is> allowed through the deglitch circuit. In units of ns.>> (I just made up those descriptions to give a feel for the flavor of> description that I expect. They likely need some adjustment to reflect> whatever they're actually intended to represent in HW).>> What is the difference between "glitch" and "hw-glitch"?
"hw-glitch" is used to configure the anti-glitch filters.
It suppresses the pulses with a width lower than x microseconds.
"glitch" is is used to tune the I2C timing requirements, and has a
nanosecond granularity.
These values are added to default timing values.
I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the
i2c-s3c2410 driver.
I agree the names and descriptions are not clear, and even misleading.
I will come with a clearer implementation, as soon as I get
clarification from HW team.
Thanks for the review,
Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

> "glitch" is is used to tune the I2C timing requirements, and has a > nanosecond granularity.> These values are added to default timing values.> I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the > i2c-s3c2410 driver.
For that, we have the generic "i2c-sda-hold-time-ns" property. The s3c
driver hasn't been converted to use it, sadly.

On 09/24/2013 05:59 PM, Wolfram Sang wrote:
>> "glitch" is is used to tune the I2C timing requirements, and has a>> nanosecond granularity.>> These values are added to default timing values.>> I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the>> i2c-s3c2410 driver.> For that, we have the generic "i2c-sda-hold-time-ns" property. The s3c> driver hasn't been converted to use it, sadly.>
Ok, thank you for pointing this out.
After some checks, this property doesn't match with what is done in this
driver.
In the v2 series I am sending, I removed the I2C timing tuning as it
works on all
the boards I have tested with.
I will only keep the anti-glitch filtering, using private properties as
I didn't find any generic
ones that could fit.
Regards,
Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html