| Message ID | 1561449642-26956-7-git-send-email-eugen.hristev@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: at91: filters support for at91 SoCs | expand |
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"; >
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"; >> >
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.
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.
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. >
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. >> > > >
> > 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!
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.
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";