Message ID | 20230330102100.17590-1-paul@crapouillou.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1023154vqo; Thu, 30 Mar 2023 03:29:37 -0700 (PDT) X-Google-Smtp-Source: AK7set/N11Y0gKNUVBw1zMPoiyeio15ym06CF0E/9S4I5qQbo/0P0lcVzvA2S3OVuFXtP4Z6aMhg X-Received: by 2002:a05:6a20:7924:b0:da:17b4:461a with SMTP id b36-20020a056a20792400b000da17b4461amr19073302pzg.32.1680172177069; Thu, 30 Mar 2023 03:29:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680172177; cv=none; d=google.com; s=arc-20160816; b=lb8itUKRkihsi5QsnjwATp04FGzJoQGx2YlWpO54siOqql3l3nxonuZPBFnkswi/P+ QuDNwy1DWd1dyJlIlB06n4DsnICdzGwCB+V/ijF+EegBFqtSTDS1Gi95CRMYsW4nfOnY A357uvt+WQJuqWCexhr6f6f3RgOdI2zOEN0PQ0eEpH/z7CSLVnP1HXNSv63D3MWDifnN jAvzvmTkYuBFYoopK7UG2GFFvmLXdW2fYFluwWFLSXiWM3wNkdFiAwoohgzwcNmLhqKr O/z4bPJpr/wysE3QpxnJrooR2SYD1iNeaf1ssMqJfnviNM6zKuIqwXQSwBzyGj5Zm1Pi JmgA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=wiu86Bom9SiZH9wS7LEFisDVIvTWugztSoKHUIOyNN4=; b=mGOfrGzSA8O0l9R+HLVwEu4luRQHoRi6Mf+PmtG5MxlkaP2lNkuk2eMGdUSPMFhjY7 38jNTHR1tp0IhiO/w6BgliOjZeon9hKoe5vunUeI/prd3gBJCJRqYyjK1q0rEFTX7qQh FGTUPHJ8fDeh05EWwPzg3MV+xs6wBI1dXyAH1JPdWOLg9b8YNfT4nyCz0QxGb6DTyh/P 1dI4iao+7OMbB7fd+rHUsdw45PFU+8MR0YqqojPvxKnY1xUPZ7MIiVgC7Ypy24E3h8S8 3AvjtbC7OJdC/45/XnL7lN7BY6KZ7aoOqRhyWHuzK0vUvVHL1RqE02GdbCbzq7ClBSHy s+WQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@crapouillou.net header.s=mail header.b=s1e1N0U5; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=crapouillou.net Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f20-20020a63f114000000b005073e334327si24984395pgi.779.2023.03.30.03.29.23; Thu, 30 Mar 2023 03:29:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@crapouillou.net header.s=mail header.b=s1e1N0U5; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=crapouillou.net Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231318AbjC3KVV (ORCPT <rfc822;rua109.linux@gmail.com> + 99 others); Thu, 30 Mar 2023 06:21:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39482 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231314AbjC3KVR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 30 Mar 2023 06:21:17 -0400 Received: from aposti.net (aposti.net [89.234.176.197]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8046C8A47; Thu, 30 Mar 2023 03:21:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1680171667; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=wiu86Bom9SiZH9wS7LEFisDVIvTWugztSoKHUIOyNN4=; b=s1e1N0U5Qd95DMQsIoN0TqQUu/34vQG6++4q+0/2CfJ2U+asCFThFCFpmr6BkklDJ9cZ54 T9LkxLwIuFLyYddgPrwAyOTXSiwdC+r/6rC+6y/acw9uDVi5nhzRNDInNpYEYSwG2foP9n QNc2+NYXIKFlNZoYObEWlAE8zwcw4uo= From: Paul Cercueil <paul@crapouillou.net> To: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich <Michael.Hennerich@analog.com> Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Paul Cercueil <paul@crapouillou.net>, Alisa Roman <alisa.roman@analog.com> Subject: [PATCH] iio: adc: ad7192: Change "shorted" channels to differential Date: Thu, 30 Mar 2023 12:21:00 +0200 Message-Id: <20230330102100.17590-1-paul@crapouillou.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761788220585142880?= X-GMAIL-MSGID: =?utf-8?q?1761788220585142880?= |
Series |
iio: adc: ad7192: Change "shorted" channels to differential
|
|
Commit Message
Paul Cercueil
March 30, 2023, 10:21 a.m. UTC
The AD7192 provides a specific channel configuration where both negative and positive inputs are connected to AIN2. This was represented in the ad7192 driver as a IIO channel with .channel = 2 and .extended_name set to "shorted". The problem with this approach, is that the driver provided two IIO channels with the identifier .channel = 2; one "shorted" and the other not. This goes against the IIO ABI, as a channel identifier should be unique. Address this issue by changing "shorted" channels to being differential instead, with channel 2 vs. itself, as we're actually measuring AIN2 vs. itself. Note that the fix tag is for the commit that moved the driver out of staging. The bug existed before that, but backporting would become very complex further down and unlikely to happen. Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging") Signed-off-by: Paul Cercueil <paul@crapouillou.net> Co-developed-by: Alisa Roman <alisa.roman@analog.com> Signed-off-by: Alisa Roman <alisa.roman@analog.com> --- drivers/iio/adc/ad7192.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Comments
Le jeudi 30 mars 2023 à 12:21 +0200, Paul Cercueil a écrit : > The AD7192 provides a specific channel configuration where both > negative > and positive inputs are connected to AIN2. This was represented in > the > ad7192 driver as a IIO channel with .channel = 2 and .extended_name > set > to "shorted". > > The problem with this approach, is that the driver provided two IIO > channels with the identifier .channel = 2; one "shorted" and the > other > not. This goes against the IIO ABI, as a channel identifier should be > unique. > > Address this issue by changing "shorted" channels to being > differential > instead, with channel 2 vs. itself, as we're actually measuring AIN2 > vs. > itself. > > Note that the fix tag is for the commit that moved the driver out of > staging. The bug existed before that, but backporting would become > very > complex further down and unlikely to happen. > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of > staging") Forgot to Cc: stable@vger.kernel.org D'oh. -Paul > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > Signed-off-by: Alisa Roman <alisa.roman@analog.com> > --- > drivers/iio/adc/ad7192.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index 55a6ab591016..99bb604b78c8 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { > __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, > IIO_VOLTAGE, \ > BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ > - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", > IIO_VOLTAGE, \ > - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > - > #define AD719x_TEMP_CHANNEL(_si, _address) \ > __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, > NULL) > > @@ -908,7 +904,7 @@ static const struct iio_chan_spec > ad7192_channels[] = { > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), > + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > @@ -922,7 +918,7 @@ static const struct iio_chan_spec > ad7193_channels[] = { > AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), > + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), > AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), > AD719x_CHANNEL(8, 3, AD7193_CH_AIN3),
On Thu, 2023-03-30 at 12:21 +0200, Paul Cercueil wrote: > The AD7192 provides a specific channel configuration where both > negative > and positive inputs are connected to AIN2. This was represented in > the > ad7192 driver as a IIO channel with .channel = 2 and .extended_name > set > to "shorted". > > The problem with this approach, is that the driver provided two IIO > channels with the identifier .channel = 2; one "shorted" and the > other > not. This goes against the IIO ABI, as a channel identifier should be > unique. > > Address this issue by changing "shorted" channels to being > differential > instead, with channel 2 vs. itself, as we're actually measuring AIN2 > vs. > itself. > > Note that the fix tag is for the commit that moved the driver out of > staging. The bug existed before that, but backporting would become > very > complex further down and unlikely to happen. > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of > staging") > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > Signed-off-by: Alisa Roman <alisa.roman@analog.com> > --- Reviewed-by: Nuno Sa <nuno.sa@analog.com>
On Thu, 30 Mar 2023 12:21:00 +0200 Paul Cercueil <paul@crapouillou.net> wrote: > The AD7192 provides a specific channel configuration where both negative > and positive inputs are connected to AIN2. This was represented in the > ad7192 driver as a IIO channel with .channel = 2 and .extended_name set > to "shorted". > > The problem with this approach, is that the driver provided two IIO > channels with the identifier .channel = 2; one "shorted" and the other > not. This goes against the IIO ABI, as a channel identifier should be > unique. > > Address this issue by changing "shorted" channels to being differential > instead, with channel 2 vs. itself, as we're actually measuring AIN2 vs. > itself. > > Note that the fix tag is for the commit that moved the driver out of > staging. The bug existed before that, but backporting would become very > complex further down and unlikely to happen. > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging") > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > Signed-off-by: Alisa Roman <alisa.roman@analog.com> +CC Fabrizio who has a fix series under review for the same driver. I'm going to let this one sit on the list for a little while. It is a breaking ABI change (that hopefully no one will notice - given the first fix from Fabrizio shows the driver crashes on probe currently we should be safe on that). Arguably just changing the index would also have been an ABI change, but that would have gotten past any code that didn't take much notice of the channel index whereas this won't. Anyhow, will give it a little while for comments then pick this up on top of Fabrizio's fixes series. Give me a poke in 2-3 weeks if I seem to have lost it. Jonathan > --- > drivers/iio/adc/ad7192.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index 55a6ab591016..99bb604b78c8 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { > __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, IIO_VOLTAGE, \ > BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ > - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", IIO_VOLTAGE, \ > - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > - > #define AD719x_TEMP_CHANNEL(_si, _address) \ > __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, NULL) > > @@ -908,7 +904,7 @@ static const struct iio_chan_spec ad7192_channels[] = { > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), > + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > @@ -922,7 +918,7 @@ static const struct iio_chan_spec ad7193_channels[] = { > AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), > + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), > AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), > AD719x_CHANNEL(8, 3, AD7193_CH_AIN3),
On Sat, Apr 1, 2023 at 4:27 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 30 Mar 2023 12:21:00 +0200 > Paul Cercueil <paul@crapouillou.net> wrote: > > > The AD7192 provides a specific channel configuration where both negative > > and positive inputs are connected to AIN2. This was represented in the > > ad7192 driver as a IIO channel with .channel = 2 and .extended_name set > > to "shorted". > > > > The problem with this approach, is that the driver provided two IIO > > channels with the identifier .channel = 2; one "shorted" and the other > > not. This goes against the IIO ABI, as a channel identifier should be > > unique. > > > > Address this issue by changing "shorted" channels to being differential > > instead, with channel 2 vs. itself, as we're actually measuring AIN2 vs. > > itself. > > > > Note that the fix tag is for the commit that moved the driver out of > > staging. The bug existed before that, but backporting would become very > > complex further down and unlikely to happen. > > > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging") > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > > Signed-off-by: Alisa Roman <alisa.roman@analog.com> > > +CC Fabrizio who has a fix series under review for the same driver. > > I'm going to let this one sit on the list for a little while. > It is a breaking ABI change (that hopefully no one will notice - given > the first fix from Fabrizio shows the driver crashes on probe currently we > should be safe on that). > > Arguably just changing the index would also have been an ABI change, but > that would have gotten past any code that didn't take much notice of the > channel index whereas this won't. > > Anyhow, will give it a little while for comments then pick this up > on top of Fabrizio's fixes series. Give me a poke in 2-3 weeks if I > seem to have lost it. The bug report in libiio related to this patch was from the company I work for. I guess this confirms there are few actual users of the driver (at least of its later revisions). I see a somewhat limited usage of the "input2 vs input2" configuration, it was just causing a libiio initialization issue, without impacting sysfs operation. The breaking ABI change would impact only users of this channel relying on sysfs without libiio. I tested this patch and I confirm it solves the issue with libiio. Fabrizio > > Jonathan > > > > --- > > drivers/iio/adc/ad7192.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > > index 55a6ab591016..99bb604b78c8 100644 > > --- a/drivers/iio/adc/ad7192.c > > +++ b/drivers/iio/adc/ad7192.c > > @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { > > __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, IIO_VOLTAGE, \ > > BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > > > -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ > > - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", IIO_VOLTAGE, \ > > - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > - > > #define AD719x_TEMP_CHANNEL(_si, _address) \ > > __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, NULL) > > > > @@ -908,7 +904,7 @@ static const struct iio_chan_spec ad7192_channels[] = { > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), > > + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > @@ -922,7 +918,7 @@ static const struct iio_chan_spec ad7193_channels[] = { > > AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > > AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > > - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), > > + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), > > AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), > > AD719x_CHANNEL(8, 3, AD7193_CH_AIN3), >
Hi Jonathan, Le samedi 01 avril 2023 à 15:42 +0100, Jonathan Cameron a écrit : > On Thu, 30 Mar 2023 12:21:00 +0200 > Paul Cercueil <paul@crapouillou.net> wrote: > > > The AD7192 provides a specific channel configuration where both > > negative > > and positive inputs are connected to AIN2. This was represented in > > the > > ad7192 driver as a IIO channel with .channel = 2 and .extended_name > > set > > to "shorted". > > > > The problem with this approach, is that the driver provided two IIO > > channels with the identifier .channel = 2; one "shorted" and the > > other > > not. This goes against the IIO ABI, as a channel identifier should > > be > > unique. > > > > Address this issue by changing "shorted" channels to being > > differential > > instead, with channel 2 vs. itself, as we're actually measuring > > AIN2 vs. > > itself. > > > > Note that the fix tag is for the commit that moved the driver out > > of > > staging. The bug existed before that, but backporting would become > > very > > complex further down and unlikely to happen. > > > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of > > staging") > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > > Signed-off-by: Alisa Roman <alisa.roman@analog.com> > > +CC Fabrizio who has a fix series under review for the same driver. > > I'm going to let this one sit on the list for a little while. > It is a breaking ABI change (that hopefully no one will notice - > given > the first fix from Fabrizio shows the driver crashes on probe > currently we > should be safe on that). > > Arguably just changing the index would also have been an ABI change, > but > that would have gotten past any code that didn't take much notice of > the > channel index whereas this won't. > > Anyhow, will give it a little while for comments then pick this up > on top of Fabrizio's fixes series. Give me a poke in 2-3 weeks if I > seem to have lost it. Friendly ping :) Cheers, -Paul > > Jonathan > > > > --- > > drivers/iio/adc/ad7192.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > > index 55a6ab591016..99bb604b78c8 100644 > > --- a/drivers/iio/adc/ad7192.c > > +++ b/drivers/iio/adc/ad7192.c > > @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { > > __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, > > IIO_VOLTAGE, \ > > BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > > > -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ > > - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", > > IIO_VOLTAGE, \ > > - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > - > > #define AD719x_TEMP_CHANNEL(_si, _address) \ > > __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, > > NULL) > > > > @@ -908,7 +904,7 @@ static const struct iio_chan_spec > > ad7192_channels[] = { > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), > > + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > @@ -922,7 +918,7 @@ static const struct iio_chan_spec > > ad7193_channels[] = { > > AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > > AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > > - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), > > + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > > AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), > > AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), > > AD719x_CHANNEL(8, 3, AD7193_CH_AIN3), >
On Tue, 25 Apr 2023 11:07:11 +0200 Paul Cercueil <paul@crapouillou.net> wrote: > Hi Jonathan, > > Le samedi 01 avril 2023 à 15:42 +0100, Jonathan Cameron a écrit : > > On Thu, 30 Mar 2023 12:21:00 +0200 > > Paul Cercueil <paul@crapouillou.net> wrote: > > > > > The AD7192 provides a specific channel configuration where both > > > negative > > > and positive inputs are connected to AIN2. This was represented in > > > the > > > ad7192 driver as a IIO channel with .channel = 2 and .extended_name > > > set > > > to "shorted". > > > > > > The problem with this approach, is that the driver provided two IIO > > > channels with the identifier .channel = 2; one "shorted" and the > > > other > > > not. This goes against the IIO ABI, as a channel identifier should > > > be > > > unique. > > > > > > Address this issue by changing "shorted" channels to being > > > differential > > > instead, with channel 2 vs. itself, as we're actually measuring > > > AIN2 vs. > > > itself. > > > > > > Note that the fix tag is for the commit that moved the driver out > > > of > > > staging. The bug existed before that, but backporting would become > > > very > > > complex further down and unlikely to happen. > > > > > > Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of > > > staging") > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > > Co-developed-by: Alisa Roman <alisa.roman@analog.com> > > > Signed-off-by: Alisa Roman <alisa.roman@analog.com> > > > > +CC Fabrizio who has a fix series under review for the same driver. > > > > I'm going to let this one sit on the list for a little while. > > It is a breaking ABI change (that hopefully no one will notice - > > given > > the first fix from Fabrizio shows the driver crashes on probe > > currently we > > should be safe on that). > > > > Arguably just changing the index would also have been an ABI change, > > but > > that would have gotten past any code that didn't take much notice of > > the > > channel index whereas this won't. > > > > Anyhow, will give it a little while for comments then pick this up > > on top of Fabrizio's fixes series. Give me a poke in 2-3 weeks if I > > seem to have lost it. > > Friendly ping :) Wise move :) Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > > Cheers, > -Paul > > > > > Jonathan > > > > > > > --- > > > drivers/iio/adc/ad7192.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > > > index 55a6ab591016..99bb604b78c8 100644 > > > --- a/drivers/iio/adc/ad7192.c > > > +++ b/drivers/iio/adc/ad7192.c > > > @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { > > > __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, > > > IIO_VOLTAGE, \ > > > BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > > > > > -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ > > > - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", > > > IIO_VOLTAGE, \ > > > - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) > > > - > > > #define AD719x_TEMP_CHANNEL(_si, _address) \ > > > __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, > > > NULL) > > > > > > @@ -908,7 +904,7 @@ static const struct iio_chan_spec > > > ad7192_channels[] = { > > > AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), > > > AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), > > > AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), > > > - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), > > > + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), > > > AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), > > > AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), > > > AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), > > > @@ -922,7 +918,7 @@ static const struct iio_chan_spec > > > ad7193_channels[] = { > > > AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), > > > AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), > > > AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), > > > - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), > > > + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), > > > AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), > > > AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), > > > AD719x_CHANNEL(8, 3, AD7193_CH_AIN3), > > >
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c index 55a6ab591016..99bb604b78c8 100644 --- a/drivers/iio/adc/ad7192.c +++ b/drivers/iio/adc/ad7192.c @@ -897,10 +897,6 @@ static const struct iio_info ad7195_info = { __AD719x_CHANNEL(_si, _channel1, -1, _address, NULL, IIO_VOLTAGE, \ BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) -#define AD719x_SHORTED_CHANNEL(_si, _channel1, _address) \ - __AD719x_CHANNEL(_si, _channel1, -1, _address, "shorted", IIO_VOLTAGE, \ - BIT(IIO_CHAN_INFO_SCALE), ad7192_calibsys_ext_info) - #define AD719x_TEMP_CHANNEL(_si, _address) \ __AD719x_CHANNEL(_si, 0, -1, _address, NULL, IIO_TEMP, 0, NULL) @@ -908,7 +904,7 @@ static const struct iio_chan_spec ad7192_channels[] = { AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M), AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M), AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP), - AD719x_SHORTED_CHANNEL(3, 2, AD7192_CH_AIN2P_AIN2M), + AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M), AD719x_CHANNEL(4, 1, AD7192_CH_AIN1), AD719x_CHANNEL(5, 2, AD7192_CH_AIN2), AD719x_CHANNEL(6, 3, AD7192_CH_AIN3), @@ -922,7 +918,7 @@ static const struct iio_chan_spec ad7193_channels[] = { AD719x_DIFF_CHANNEL(2, 5, 6, AD7193_CH_AIN5P_AIN6M), AD719x_DIFF_CHANNEL(3, 7, 8, AD7193_CH_AIN7P_AIN8M), AD719x_TEMP_CHANNEL(4, AD7193_CH_TEMP), - AD719x_SHORTED_CHANNEL(5, 2, AD7193_CH_AIN2P_AIN2M), + AD719x_DIFF_CHANNEL(5, 2, 2, AD7193_CH_AIN2P_AIN2M), AD719x_CHANNEL(6, 1, AD7193_CH_AIN1), AD719x_CHANNEL(7, 2, AD7193_CH_AIN2), AD719x_CHANNEL(8, 3, AD7193_CH_AIN3),