[v4,2/9] dt-bindings: i2c: add bindings for i2c analog and digital filter
diff mbox series

Message ID 1567418773-2427-3-git-send-email-eugen.hristev@microchip.com
State Changes Requested
Headers show
Series
  • i2c: add support for filters
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 17 lines checked"

Commit Message

Eugen Hristev Sept. 2, 2019, 10:12 a.m. UTC
From: Eugen Hristev <eugen.hristev@microchip.com>

Some i2c controllers have a built-in digital or analog filter.
This is specifically required depending on the hardware PCB/board.
Some controllers also allow specifying the maximum width of the
spikes that can be filtered. The width length can be specified in nanoseconds.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Peter Rosin Sept. 2, 2019, 10:49 a.m. UTC | #1
On 2019-09-02 12:12, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Some i2c controllers have a built-in digital or analog filter.
> This is specifically required depending on the hardware PCB/board.
> Some controllers also allow specifying the maximum width of the
> spikes that can be filtered. The width length can be specified in nanoseconds.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index 44efafd..8dbff67 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -55,6 +55,17 @@ wants to support one of the below features, it should adapt the bindings below.
>  	Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
>  	specification.
>  
> +- i2c-analog-filter
> +	Enable analog filter for i2c lines.
> +
> +- i2c-digital-filter
> +	Enable digital filter for i2c lines.
> +
> +- i2c-filter-width-ns
> +	Width of spikes which can be filtered by either digital or analog
> +	filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified

filtr -> filter (two instances)

What if you want/need to have different bandwidth for the digital and analog
filters? After all, this is a generic binding...

Cheers,
Peter

> +	in nanoseconds.
> +
>  - interrupts
>  	interrupts used by the device.
>  
>
Eugen Hristev Sept. 2, 2019, 2:15 p.m. UTC | #2
On 02.09.2019 13:49, Peter Rosin wrote:

> On 2019-09-02 12:12, Eugen.Hristev@microchip.com wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> Some i2c controllers have a built-in digital or analog filter.
>> This is specifically required depending on the hardware PCB/board.
>> Some controllers also allow specifying the maximum width of the
>> spikes that can be filtered. The width length can be specified in nanoseconds.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
>> index 44efafd..8dbff67 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
>> @@ -55,6 +55,17 @@ wants to support one of the below features, it should adapt the bindings below.
>>   	Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
>>   	specification.
>>   
>> +- i2c-analog-filter
>> +	Enable analog filter for i2c lines.
>> +
>> +- i2c-digital-filter
>> +	Enable digital filter for i2c lines.
>> +
>> +- i2c-filter-width-ns
>> +	Width of spikes which can be filtered by either digital or analog
>> +	filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified
> 
> filtr -> filter (two instances)
> 
> What if you want/need to have different bandwidth for the digital and analog
> filters? After all, this is a generic binding...

Hi Peter,

For our needs, this is enough: the purpose of the filters is to avoid 
noise on the lines, the noise is as big as it is for the digital and for 
the analog filters, since we use an absolute measurement for them. So I 
do not know how useful it would be to make a difference.

Wolfram, what do you think ?

Eugen


> 
> Cheers,
> Peter
> 
>> +	in nanoseconds.
>> +
>>   - interrupts
>>   	interrupts used by the device.
>>   
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>
Alexandre Belloni Sept. 2, 2019, 2:22 p.m. UTC | #3
Eugen,

On 02/09/2019 14:15:14+0000, Eugen.Hristev@microchip.com wrote:
> On 02.09.2019 13:49, Peter Rosin wrote:
> 
> > On 2019-09-02 12:12, Eugen.Hristev@microchip.com wrote:
> >> From: Eugen Hristev <eugen.hristev@microchip.com>
> >>
> >> Some i2c controllers have a built-in digital or analog filter.
> >> This is specifically required depending on the hardware PCB/board.
> >> Some controllers also allow specifying the maximum width of the
> >> spikes that can be filtered. The width length can be specified in nanoseconds.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >> ---
> >>   Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> >> index 44efafd..8dbff67 100644
> >> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> >> @@ -55,6 +55,17 @@ wants to support one of the below features, it should adapt the bindings below.
> >>   	Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
> >>   	specification.
> >>   
> >> +- i2c-analog-filter
> >> +	Enable analog filter for i2c lines.
> >> +
> >> +- i2c-digital-filter
> >> +	Enable digital filter for i2c lines.
> >> +
> >> +- i2c-filter-width-ns
> >> +	Width of spikes which can be filtered by either digital or analog
> >> +	filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified
> > 
> > filtr -> filter (two instances)
> > 
> > What if you want/need to have different bandwidth for the digital and analog
> > filters? After all, this is a generic binding...
> 
> For our needs, this is enough: the purpose of the filters is to avoid 
> noise on the lines, the noise is as big as it is for the digital and for 
> the analog filters, since we use an absolute measurement for them. So I 
> do not know how useful it would be to make a difference.
> 

You are adding generic properties so they have to be generic and not
tied to your particular use case.
Peter Rosin Sept. 2, 2019, 2:40 p.m. UTC | #4
On 2019-09-02 16:15, Eugen.Hristev@microchip.com wrote:
> 
> 
> On 02.09.2019 13:49, Peter Rosin wrote:
> 
>> On 2019-09-02 12:12, Eugen.Hristev@microchip.com wrote:
>>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>>
>>> Some i2c controllers have a built-in digital or analog filter.
>>> This is specifically required depending on the hardware PCB/board.
>>> Some controllers also allow specifying the maximum width of the
>>> spikes that can be filtered. The width length can be specified in nanoseconds.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>> ---
>>>   Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> index 44efafd..8dbff67 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> @@ -55,6 +55,17 @@ wants to support one of the below features, it should adapt the bindings below.
>>>   	Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
>>>   	specification.
>>>   
>>> +- i2c-analog-filter
>>> +	Enable analog filter for i2c lines.
>>> +
>>> +- i2c-digital-filter
>>> +	Enable digital filter for i2c lines.
>>> +
>>> +- i2c-filter-width-ns
>>> +	Width of spikes which can be filtered by either digital or analog
>>> +	filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified
>>
>> filtr -> filter (two instances)
>>
>> What if you want/need to have different bandwidth for the digital and analog
>> filters? After all, this is a generic binding...
> 
> Hi Peter,
> 
> For our needs, this is enough: the purpose of the filters is to avoid 
> noise on the lines, the noise is as big as it is for the digital and for 
> the analog filters, since we use an absolute measurement for them. So I 
> do not know how useful it would be to make a difference.
I think my gripe is that the description also seems non-generic. Analog
filters never (ok, usually, but I have a hard time seeing how a simple
analog filter can) work in terms of some "width of spikes". That phrasing
seems like something inherent to trivial digital filters. For analog
filters, specifying the bandwidth or cut-off frequency seems much more
appropriate. And bandwidth would work equally well for digital filters,
methinks.

I also think it should be mentioned explicitly that this binding is for
LP filters. I don't think anything else would be useful, but better safe
than sorry...

Hmm, would it be good or bad to specify the bandwidth relative to the
current maximum bus speed?

Cheers,
Peter

> Wolfram, what do you think ?
> 
> Eugen
> 
> 
>>
>> Cheers,
>> Peter
>>
>>> +	in nanoseconds.
>>> +
>>>   - interrupts
>>>   	interrupts used by the device.
>>>   
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
Wolfram Sang Sept. 2, 2019, 3:47 p.m. UTC | #5
Hi Eugen,

> Wolfram, what do you think ?

Yes, the bindings should be generic. Peter's reasoning makes much sense
to me. I am quite sure if the two of you can work things out, I'll have
nothing to add.

Thanks,

   Wolfram

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 44efafd..8dbff67 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -55,6 +55,17 @@  wants to support one of the below features, it should adapt the bindings below.
 	Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
 	specification.
 
+- i2c-analog-filter
+	Enable analog filter for i2c lines.
+
+- i2c-digital-filter
+	Enable digital filter for i2c lines.
+
+- i2c-filter-width-ns
+	Width of spikes which can be filtered by either digital or analog
+	filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified
+	in nanoseconds.
+
 - interrupts
 	interrupts used by the device.