| Message ID | 20241028160748.489596-6-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: adc: ad7124: Make it work on de10-nano | expand |
On 10/28/24 11:07 AM, Uwe Kleine-König wrote: > Hello, > > this is iteration v2 to make ad7124 work on de10-nano. (Implicit) v1 is > available at > https://lore.kernel.org/linux-iio/20241024171703.201436-5-u.kleine-koenig@baylibre.com. > > The changes since v1: > > - Write 0 instead of 0x0001 to disable channels. While 0x0001 is the > reset default value for these registers (apart from the channel 0 one) > there is no sensible reason to use that value (i.e. > AD7124_CHANNEL_AINP(0) | AD7124_CHANNEL_AINM(1)) as the value is > reprogrammed before use anyhow. This addresses the feedback that the > magic value 0x0001 should better be constructed using register bit > field defintions. > > - Add maxItems: 1 to the new property defined in the binding patch (Krzysztof) > > - Rename property to rdy-gpios (Rob) > > - Use rdy-gpios only for gpio reading and continue using the usual irq > defintion for the interrupt (Jonathan). I was surprised I can use both the > GPIO as input and the matching irq. > > - patch #1 is new, and use GPIO_ACTIVE_LOW in the gpio descriptor > instead of 2. > > Jonathan voiced concerns about the reliability of this solution and > proposed to implement polling. I'm convinced the solution implemented > here is robust, so I see no need to implement polling today. > > Still open questions: > > - Is rdy-gpios the right name. The line is named ̅R̅D̅Y, so maybe nrdy-gpios? Or > nRDY-gpios? The GPIO_ACTIVE_LOW indicates the "bar" for active low, so we don't typically add the "n" prefix to the name. So rdy-gpios looks correct to me. > - Jonathan wanted some input from ADI about this series and the > hardware details. > > Best regards > Uwe > > Uwe Kleine-König (4): > dt-bindings: iio: adc: adi,ad7124: Use symbolic name for interrupt > flag > dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for > irq line > iio: adc: ad_sigma_delta: Add support for reading irq status using a > GPIO > iio: adc: ad7124: Disable all channels at probe time > > .../bindings/iio/adc/adi,ad7124.yaml | 11 +++++- > drivers/iio/adc/ad7124.c | 3 ++ > drivers/iio/adc/ad_sigma_delta.c | 35 ++++++++++++++++--- > include/linux/iio/adc/ad_sigma_delta.h | 1 + > 4 files changed, 44 insertions(+), 6 deletions(-) > > base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
Hello, On Mon, Oct 28, 2024 at 05:07:49PM +0100, Uwe Kleine-König wrote: > this is iteration v2 to make ad7124 work on de10-nano. (Implicit) v1 is > available at > https://lore.kernel.org/linux-iio/20241024171703.201436-5-u.kleine-koenig@baylibre.com. > > The changes since v1: > > - Write 0 instead of 0x0001 to disable channels. While 0x0001 is the > reset default value for these registers (apart from the channel 0 one) > there is no sensible reason to use that value (i.e. > AD7124_CHANNEL_AINP(0) | AD7124_CHANNEL_AINM(1)) as the value is > reprogrammed before use anyhow. This addresses the feedback that the > magic value 0x0001 should better be constructed using register bit > field defintions. > > - Add maxItems: 1 to the new property defined in the binding patch (Krzysztof) > > - Rename property to rdy-gpios (Rob) > > - Use rdy-gpios only for gpio reading and continue using the usual irq > defintion for the interrupt (Jonathan). I was surprised I can use both the > GPIO as input and the matching irq. > > - patch #1 is new, and use GPIO_ACTIVE_LOW in the gpio descriptor > instead of 2. > > Jonathan voiced concerns about the reliability of this solution and > proposed to implement polling. I'm convinced the solution implemented > here is robust, so I see no need to implement polling today. > > Still open questions: > > - Is rdy-gpios the right name. The line is named ̅R̅D̅Y, so maybe nrdy-gpios? Or > nRDY-gpios? David said that rdy-gpios looks right in combination with the GPIO_ACTIVE_LOW flag. Makes sense to me to negate only in a single location. > - Jonathan wanted some input from ADI about this series and the > hardware details. I think the hardware is understood now reasonably well and from the discussion with tglx it's also clear that the issue is expected and fixed at the right place. Although probably not all hardware configurations can benefit from the modification, I still consider this a beneficial modification because it allows at least some (most?) machines to use the irq instead of polling. There is a patch series on the list for ad7124 (https://lore.kernel.org/linux-iio/cover.1731404695.git.u.kleine-koenig@baylibre.com/) that for now didn't get feedback, and I found another race condition in the sigma_delta driver helper and now wonder how to proceed here. If we agree in general that the rdy-gpios patches are ok to be applied, I'd base the fix for the latest race condition on top of these. Should I better collect all in-flight patches in a single series, or just post the new patches (with a proper --base= parameter to format-patch)? Best regards Uwe
On Mon, 18 Nov 2024 19:12:22 +0100 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > Hello, > > On Mon, Oct 28, 2024 at 05:07:49PM +0100, Uwe Kleine-König wrote: > > this is iteration v2 to make ad7124 work on de10-nano. (Implicit) v1 is > > available at > > https://lore.kernel.org/linux-iio/20241024171703.201436-5-u.kleine-koenig@baylibre.com. > > > > The changes since v1: > > > > - Write 0 instead of 0x0001 to disable channels. While 0x0001 is the > > reset default value for these registers (apart from the channel 0 one) > > there is no sensible reason to use that value (i.e. > > AD7124_CHANNEL_AINP(0) | AD7124_CHANNEL_AINM(1)) as the value is > > reprogrammed before use anyhow. This addresses the feedback that the > > magic value 0x0001 should better be constructed using register bit > > field defintions. > > > > - Add maxItems: 1 to the new property defined in the binding patch (Krzysztof) > > > > - Rename property to rdy-gpios (Rob) > > > > - Use rdy-gpios only for gpio reading and continue using the usual irq > > defintion for the interrupt (Jonathan). I was surprised I can use both the > > GPIO as input and the matching irq. > > > > - patch #1 is new, and use GPIO_ACTIVE_LOW in the gpio descriptor > > instead of 2. > > > > Jonathan voiced concerns about the reliability of this solution and > > proposed to implement polling. I'm convinced the solution implemented > > here is robust, so I see no need to implement polling today. > > > > Still open questions: > > > > - Is rdy-gpios the right name. The line is named ̅R̅D̅Y, so maybe nrdy-gpios? Or > > nRDY-gpios? > > David said that rdy-gpios looks right in combination with the > GPIO_ACTIVE_LOW flag. Makes sense to me to negate only in a single > location. > > > - Jonathan wanted some input from ADI about this series and the > > hardware details. > > I think the hardware is understood now reasonably well and from the > discussion with tglx it's also clear that the issue is expected and > fixed at the right place. Although probably not all hardware > configurations can benefit from the modification, I still consider this > a beneficial modification because it allows at least some (most?) > machines to use the irq instead of polling. Agreed. Sorry for slow response; day job got too busy for a while. > > There is a patch series on the list for ad7124 > (https://lore.kernel.org/linux-iio/cover.1731404695.git.u.kleine-koenig@baylibre.com/) > that for now didn't get feedback, and I found another race condition in > the sigma_delta driver helper and now wonder how to proceed here. If we > agree in general that the rdy-gpios patches are ok to be applied, I'd > base the fix for the latest race condition on top of these. Should I > better collect all in-flight patches in a single series, or just post > the new patches (with a proper --base= parameter to format-patch)? Subject to perhaps adding a little more docs to the DT patch to strongly encourage use of the GPIO binding if IRQ controller capable (or double wired) Make sure those patches were you feel a fixes tag is appropriate go first (which incidentally includes patch 4 from this series and some or all of other series). Given we are very early in the cycle I'll pick the fixes up and get them upstream soon after rc1 then we can queue up the bulk of this which is a little complex to consider a fix as material for next merge window. We can think about backporting after that. Jonathan > > Best regards > Uwe
Hello, this is iteration v2 to make ad7124 work on de10-nano. (Implicit) v1 is available at https://lore.kernel.org/linux-iio/20241024171703.201436-5-u.kleine-koenig@baylibre.com. The changes since v1: - Write 0 instead of 0x0001 to disable channels. While 0x0001 is the reset default value for these registers (apart from the channel 0 one) there is no sensible reason to use that value (i.e. AD7124_CHANNEL_AINP(0) | AD7124_CHANNEL_AINM(1)) as the value is reprogrammed before use anyhow. This addresses the feedback that the magic value 0x0001 should better be constructed using register bit field defintions. - Add maxItems: 1 to the new property defined in the binding patch (Krzysztof) - Rename property to rdy-gpios (Rob) - Use rdy-gpios only for gpio reading and continue using the usual irq defintion for the interrupt (Jonathan). I was surprised I can use both the GPIO as input and the matching irq. - patch #1 is new, and use GPIO_ACTIVE_LOW in the gpio descriptor instead of 2. Jonathan voiced concerns about the reliability of this solution and proposed to implement polling. I'm convinced the solution implemented here is robust, so I see no need to implement polling today. Still open questions: - Is rdy-gpios the right name. The line is named ̅R̅D̅Y, so maybe nrdy-gpios? Or nRDY-gpios? - Jonathan wanted some input from ADI about this series and the hardware details. Best regards Uwe Uwe Kleine-König (4): dt-bindings: iio: adc: adi,ad7124: Use symbolic name for interrupt flag dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO iio: adc: ad7124: Disable all channels at probe time .../bindings/iio/adc/adi,ad7124.yaml | 11 +++++- drivers/iio/adc/ad7124.c | 3 ++ drivers/iio/adc/ad_sigma_delta.c | 35 ++++++++++++++++--- include/linux/iio/adc/ad_sigma_delta.h | 1 + 4 files changed, 44 insertions(+), 6 deletions(-) base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc