[v2,6/9] dt-bindings: i2c: at91: add binding for enable-ana-filt
diff mbox series

Message ID 1561449642-26956-7-git-send-email-eugen.hristev@microchip.com
State Changes Requested
Headers show
Series
  • i2c: at91: filters support for at91 SoCs
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Eugen Hristev June 25, 2019, 8:05 a.m. UTC
From: Eugen Hristev <eugen.hristev@microchip.com>

Add binding specification for analogic filter inside the i2c controller

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

Comments

Peter Rosin June 25, 2019, 8:55 a.m. UTC | #1
On 2019-06-25 10:05, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Add binding specification for analogic filter inside the i2c controller

s/analogic/the analog/

> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-at91.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> index 8268595..20d334c 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> @@ -23,6 +23,9 @@ Optional properties:
>  - enable-dig-filt: Enable the built-in digital filter on the i2c lines,
>    specifically required depending on the hardware PCB/board and if the
>    version of the controller includes it.
> +- enable-ana-filt: Enable the built-in analogic filter on the i2c lines,
> +  specifically required depending on the hardware PCB/board and if the
> +  version of the controller includes it.
>  - Child nodes conforming to i2c bus binding
>  
>  Examples :
> @@ -60,6 +63,7 @@ i2c0: i2c@f8034600 {
>  	atmel,fifo-size = <16>;
>  	i2c-sda-hold-time-ns = <336>;
>  	enable-dig-filt;
> +	enable-ana-filt;

Perhaps

	microchip,digital-filter;
	microchip,analog-filter;

?

Cheers,
Peter

>  
>  	wm8731: wm8731@1a {
>  		compatible = "wm8731";
>
Eugen Hristev June 25, 2019, 9:14 a.m. UTC | #2
On 25.06.2019 11:55, Peter Rosin wrote:

> 
> On 2019-06-25 10:05, Eugen.Hristev@microchip.com wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> Add binding specification for analogic filter inside the i2c controller
> 
> s/analogic/the analog/
> 
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   Documentation/devicetree/bindings/i2c/i2c-at91.txt | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
>> index 8268595..20d334c 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
>> @@ -23,6 +23,9 @@ Optional properties:
>>   - enable-dig-filt: Enable the built-in digital filter on the i2c lines,
>>     specifically required depending on the hardware PCB/board and if the
>>     version of the controller includes it.
>> +- enable-ana-filt: Enable the built-in analogic filter on the i2c lines,
>> +  specifically required depending on the hardware PCB/board and if the
>> +  version of the controller includes it.
>>   - Child nodes conforming to i2c bus binding
>>   
>>   Examples :
>> @@ -60,6 +63,7 @@ i2c0: i2c@f8034600 {
>>   	atmel,fifo-size = <16>;
>>   	i2c-sda-hold-time-ns = <336>;
>>   	enable-dig-filt;
>> +	enable-ana-filt;
> 
> Perhaps
> 
> 	microchip,digital-filter;
> 	microchip,analog-filter;
> 
> ?

Hi Peter,

Thanks for reviewing. The name of the property does not matter much to 
me, and we have properties prefixed with vendor, and some are not.

@Alexandre Belloni: which name you think it's best ?

Eugen

> 
> Cheers,
> Peter
> 
>>   
>>   	wm8731: wm8731@1a {
>>   		compatible = "wm8731";
>>
>
Alexandre Belloni June 25, 2019, 9:31 a.m. UTC | #3
On 25/06/2019 09:14:13+0000, Eugen.Hristev@microchip.com wrote:
> > Perhaps
> > 
> > 	microchip,digital-filter;
> > 	microchip,analog-filter;
> > 
> > ?
> 
> Hi Peter,
> 
> Thanks for reviewing. The name of the property does not matter much to 
> me, and we have properties prefixed with vendor, and some are not.
> 
> @Alexandre Belloni: which name you think it's best ?
> 

I'm not sure, it depends on whether Wolfram thinks it is generic enough
to be used without a vendor prefix.
Wolfram Sang June 25, 2019, 9:55 a.m. UTC | #4
On Tue, Jun 25, 2019 at 11:31:56AM +0200, Alexandre Belloni wrote:
> On 25/06/2019 09:14:13+0000, Eugen.Hristev@microchip.com wrote:
> > > Perhaps
> > > 
> > > 	microchip,digital-filter;
> > > 	microchip,analog-filter;
> > > 
> > > ?
> > 
> > Hi Peter,
> > 
> > Thanks for reviewing. The name of the property does not matter much to 
> > me, and we have properties prefixed with vendor, and some are not.
> > 
> > @Alexandre Belloni: which name you think it's best ?
> > 
> 
> I'm not sure, it depends on whether Wolfram thinks it is generic enough
> to be used without a vendor prefix.

I could imagine that we design a generic property for filters. The ones
above make me wonder, though, because they are bool. I'd think you can
configure the filters in some way, too?

I never used such filtering, so I am unaware of the parameters needed /
suitable. Quick grepping through I2C master drivers reveals that
i2c-stm32f7.c also handles filters, but only with default values. Maybe
DT configuration would be benefitial to that driver, too?

Adding some people to CC.
Alexandre Belloni June 27, 2019, 1:22 p.m. UTC | #5
On 25/06/2019 11:55:33+0200, Wolfram Sang wrote:
> On Tue, Jun 25, 2019 at 11:31:56AM +0200, Alexandre Belloni wrote:
> > On 25/06/2019 09:14:13+0000, Eugen.Hristev@microchip.com wrote:
> > > > Perhaps
> > > > 
> > > > 	microchip,digital-filter;
> > > > 	microchip,analog-filter;
> > > > 
> > > > ?
> > > 
> > > Hi Peter,
> > > 
> > > Thanks for reviewing. The name of the property does not matter much to 
> > > me, and we have properties prefixed with vendor, and some are not.
> > > 
> > > @Alexandre Belloni: which name you think it's best ?
> > > 
> > 
> > I'm not sure, it depends on whether Wolfram thinks it is generic enough
> > to be used without a vendor prefix.
> 
> I could imagine that we design a generic property for filters. The ones
> above make me wonder, though, because they are bool. I'd think you can
> configure the filters in some way, too?
> 

Apart from enabling the filter there is indeed one configuration
setting, the maximum pulse width of spikes to be suppressed by the input
filter.

> I never used such filtering, so I am unaware of the parameters needed /
> suitable. Quick grepping through I2C master drivers reveals that
> i2c-stm32f7.c also handles filters, but only with default values. Maybe
> DT configuration would be benefitial to that driver, too?
> 
> Adding some people to CC.
>
Eugen Hristev June 27, 2019, 1:31 p.m. UTC | #6
On 27.06.2019 16:22, Alexandre Belloni wrote:
> External E-Mail
> 
> 
> On 25/06/2019 11:55:33+0200, Wolfram Sang wrote:
>> On Tue, Jun 25, 2019 at 11:31:56AM +0200, Alexandre Belloni wrote:
>>> On 25/06/2019 09:14:13+0000, Eugen.Hristev@microchip.com wrote:
>>>>> Perhaps
>>>>>
>>>>> 	microchip,digital-filter;
>>>>> 	microchip,analog-filter;
>>>>>
>>>>> ?
>>>>
>>>> Hi Peter,
>>>>
>>>> Thanks for reviewing. The name of the property does not matter much to
>>>> me, and we have properties prefixed with vendor, and some are not.
>>>>
>>>> @Alexandre Belloni: which name you think it's best ?
>>>>
>>>
>>> I'm not sure, it depends on whether Wolfram thinks it is generic enough
>>> to be used without a vendor prefix.
>>
>> I could imagine that we design a generic property for filters. The ones
>> above make me wonder, though, because they are bool. I'd think you can
>> configure the filters in some way, too?
>>
> 
> Apart from enabling the filter there is indeed one configuration
> setting, the maximum pulse width of spikes to be suppressed by the input
> filter.

Hello,

This is a number 0 to 7 (3 bits) that represents the width of the spike 
in periph clock cycles.
I am looking to see what is PADFCFG , as it's related to the PAD analog 
filter configuration. It may be unused by the filter.

Eugen

> 
>> I never used such filtering, so I am unaware of the parameters needed /
>> suitable. Quick grepping through I2C master drivers reveals that
>> i2c-stm32f7.c also handles filters, but only with default values. Maybe
>> DT configuration would be benefitial to that driver, too?
>>
>> Adding some people to CC.
>>
> 
> 
>
Wolfram Sang June 27, 2019, 1:34 p.m. UTC | #7
> > Apart from enabling the filter there is indeed one configuration
> > setting, the maximum pulse width of spikes to be suppressed by the input
> > filter.

Yup, this is what I anticipated.

> This is a number 0 to 7 (3 bits) that represents the width of the spike 
> in periph clock cycles.

For a generic binding, we would need some time-value as a parameter and
convert it to clock cycles in the driver then, I'd think.

> I am looking to see what is PADFCFG , as it's related to the PAD analog 
> filter configuration. It may be unused by the filter.

Thanks!
Alexandre Belloni June 27, 2019, 1:39 p.m. UTC | #8
On 27/06/2019 15:34:40+0200, Wolfram Sang wrote:
> 
> > > Apart from enabling the filter there is indeed one configuration
> > > setting, the maximum pulse width of spikes to be suppressed by the input
> > > filter.
> 
> Yup, this is what I anticipated.
> 
> > This is a number 0 to 7 (3 bits) that represents the width of the spike 
> > in periph clock cycles.
> 
> For a generic binding, we would need some time-value as a parameter and
> convert it to clock cycles in the driver then, I'd think.
> 

Yes, that is what I was going to suggest.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index 8268595..20d334c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -23,6 +23,9 @@  Optional properties:
 - enable-dig-filt: Enable the built-in digital filter on the i2c lines,
   specifically required depending on the hardware PCB/board and if the
   version of the controller includes it.
+- enable-ana-filt: Enable the built-in analogic filter on the i2c lines,
+  specifically required depending on the hardware PCB/board and if the
+  version of the controller includes it.
 - Child nodes conforming to i2c bus binding
 
 Examples :
@@ -60,6 +63,7 @@  i2c0: i2c@f8034600 {
 	atmel,fifo-size = <16>;
 	i2c-sda-hold-time-ns = <336>;
 	enable-dig-filt;
+	enable-ana-filt;
 
 	wm8731: wm8731@1a {
 		compatible = "wm8731";