[PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[Ian Abbott]

Commit eddd2a4c675c ("staging: comedi: cb_pcidas: refactor
write_calibration_bitstream()") inadvertently removed one of the
`udelay(1)` calls when writing to the calibration register in
`cb_pcidas_calib_write()`.  Reinstate the delay.  It may seem strange
that the delay is placed before the register write, but this function is
called in a loop so the extra delay can make a difference.

This _might_ solve reported issues reading analog inputs on a
PCIe-DAS1602/16 card where the analog input values "were scaled in a
strange way that didn't make sense".  On the same hardware running a
system with a 3.13 kernel, and then a system with a 4.4 kernel, but with
the same application software, the system with the 3.13 kernel was fine,
but the one with the 4.4 kernel exhibited the problem.  Of the 90
changes to the driver between those kernel versions, this change looked
like the most likely culprit.

Fixes: eddd2a4c675c ("staging: comedi: cb_pcidas: refactor write_calibration_bitstream()")
Cc: <stable@vger.kernel.org>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/cb_pcidas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas.c b/drivers/staging/comedi/drivers/cb_pcidas.c
index 48ec2ee953dc..4f2ac39aa619 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas.c
@@ -529,6 +529,7 @@ static void cb_pcidas_calib_write(struct comedi_device *dev,
 	if (trimpot) {
 		/* select trimpot */
 		calib_bits |= PCIDAS_CALIB_TRIM_SEL;
+		udelay(1);
 		outw(calib_bits, devpriv->pcibar1 + PCIDAS_CALIB_REG);
 	}
 
-- 
2.28.0

Re: [PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[Ian Abbott]

On 29/10/2020 14:18, Ian Abbott wrote:
> Commit eddd2a4c675c ("staging: comedi: cb_pcidas: refactor
> write_calibration_bitstream()") inadvertently removed one of the
> `udelay(1)` calls when writing to the calibration register in
> `cb_pcidas_calib_write()`.  Reinstate the delay.  It may seem strange
> that the delay is placed before the register write, but this function is
> called in a loop so the extra delay can make a difference.
> 
> This _might_ solve reported issues reading analog inputs on a
> PCIe-DAS1602/16 card where the analog input values "were scaled in a
> strange way that didn't make sense".  On the same hardware running a
> system with a 3.13 kernel, and then a system with a 4.4 kernel, but with
> the same application software, the system with the 3.13 kernel was fine,
> but the one with the 4.4 kernel exhibited the problem.  Of the 90
> changes to the driver between those kernel versions, this change looked
> like the most likely culprit.

Actually, I've realized that this patch will have no effect on the 
PCIe-DAS1602/16 card because it uses a different driver - cb_pcimdas, 
not cb_pcidas.

Greg, you might as well drop this patch if you haven't already applied 
it, since it was only a hunch that it fixed a problem.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[Ian Abbott]

On 02/11/2020 10:25, Ian Abbott wrote:
> On 29/10/2020 14:18, Ian Abbott wrote:
>> Commit eddd2a4c675c ("staging: comedi: cb_pcidas: refactor
>> write_calibration_bitstream()") inadvertently removed one of the
>> `udelay(1)` calls when writing to the calibration register in
>> `cb_pcidas_calib_write()`.  Reinstate the delay.  It may seem strange
>> that the delay is placed before the register write, but this function is
>> called in a loop so the extra delay can make a difference.
>>
>> This _might_ solve reported issues reading analog inputs on a
>> PCIe-DAS1602/16 card where the analog input values "were scaled in a
>> strange way that didn't make sense".  On the same hardware running a
>> system with a 3.13 kernel, and then a system with a 4.4 kernel, but with
>> the same application software, the system with the 3.13 kernel was fine,
>> but the one with the 4.4 kernel exhibited the problem.  Of the 90
>> changes to the driver between those kernel versions, this change looked
>> like the most likely culprit.
> 
> Actually, I've realized that this patch will have no effect on the 
> PCIe-DAS1602/16 card because it uses a different driver - cb_pcimdas, 
> not cb_pcidas.

But that's also confusing because PCIe-DAS1602/16 was not supported 
until the 3.19 kernel!  I know the reported has both PCI-DAS1602/16 and 
PCIe-DAS1602/16 cards (supported by cb_pcidas and cb_pcimdas 
respectively), so there could have been some mix-up in the reporting.

> 
> Greg, you might as well drop this patch if you haven't already applied 
> it, since it was only a hunch that it fixed a problem.
> 


-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[Ian Abbott]

On 02/11/2020 11:16, Ian Abbott wrote:
> On 02/11/2020 10:25, Ian Abbott wrote:
>> On 29/10/2020 14:18, Ian Abbott wrote:
>>> Commit eddd2a4c675c ("staging: comedi: cb_pcidas: refactor
>>> write_calibration_bitstream()") inadvertently removed one of the
>>> `udelay(1)` calls when writing to the calibration register in
>>> `cb_pcidas_calib_write()`.  Reinstate the delay.  It may seem strange
>>> that the delay is placed before the register write, but this function is
>>> called in a loop so the extra delay can make a difference.
>>>
>>> This _might_ solve reported issues reading analog inputs on a
>>> PCIe-DAS1602/16 card where the analog input values "were scaled in a
>>> strange way that didn't make sense".  On the same hardware running a
>>> system with a 3.13 kernel, and then a system with a 4.4 kernel, but with
>>> the same application software, the system with the 3.13 kernel was fine,
>>> but the one with the 4.4 kernel exhibited the problem.  Of the 90
>>> changes to the driver between those kernel versions, this change looked
>>> like the most likely culprit.
>>
>> Actually, I've realized that this patch will have no effect on the 
>> PCIe-DAS1602/16 card because it uses a different driver - cb_pcimdas, 
>> not cb_pcidas.
> 
> But that's also confusing because PCIe-DAS1602/16 was not supported 
> until the 3.19 kernel!  I know the reported has both PCI-DAS1602/16 and 
> PCIe-DAS1602/16 cards (supported by cb_pcidas and cb_pcimdas 
> respectively), so there could have been some mix-up in the reporting.

Mystery solved.  The reporter had a mixture of PCIe-DAS1602/16 and 
PCIM-DAS1602/16 cards (not PCI-DAS1602/16).  Both of those are supported 
by the "cb_pcimdas" driver (not "cb_pcidas"), although the PCIe card was 
not supported until the 3.19 kernel (by commit 4e3d14af1286).  Testing 
with the 3.13 kernel was done with the PCIM card.

The "strange scaling" was due to a change in the ranges reported for the 
analog input subdevice in the 4.1 kernel (by commit c7549d770a27). 
Before then, it just reported a single dummy range [0, 1000000] with no 
units (converted to [0.0, 1.0] with no units by comedilib).  Afterwards, 
it reported four different voltage ranges (either unipolar or bipolar, 
depending in a status bit tied to a physical switch).  The reporter's 
application code was using the reported range to scale the raw values to 
a voltage (using comedilib functions), but because the reported range 
was bogus, the application code was performing additional scaling 
(outside of comedilib).  The application code can be changed to check 
whether the device is reporting a proper voltage range or the old, bogus 
range, and behave accordingly.

>> Greg, you might as well drop this patch if you haven't already applied 
>> it, since it was only a hunch that it fixed a problem.

That's still the case, although it won't do any harm if applied (apart 
from the incorrect patch description).

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH] staging: comedi: cb_pcidas: reinstate delay removed from trimpot setting

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 10:49:18AM +0000, Ian Abbott wrote:
> On 02/11/2020 11:16, Ian Abbott wrote:
> > On 02/11/2020 10:25, Ian Abbott wrote:
> > > On 29/10/2020 14:18, Ian Abbott wrote:
> > > > Commit eddd2a4c675c ("staging: comedi: cb_pcidas: refactor
> > > > write_calibration_bitstream()") inadvertently removed one of the
> > > > `udelay(1)` calls when writing to the calibration register in
> > > > `cb_pcidas_calib_write()`.  Reinstate the delay.  It may seem strange
> > > > that the delay is placed before the register write, but this function is
> > > > called in a loop so the extra delay can make a difference.
> > > > 
> > > > This _might_ solve reported issues reading analog inputs on a
> > > > PCIe-DAS1602/16 card where the analog input values "were scaled in a
> > > > strange way that didn't make sense".  On the same hardware running a
> > > > system with a 3.13 kernel, and then a system with a 4.4 kernel, but with
> > > > the same application software, the system with the 3.13 kernel was fine,
> > > > but the one with the 4.4 kernel exhibited the problem.  Of the 90
> > > > changes to the driver between those kernel versions, this change looked
> > > > like the most likely culprit.
> > > 
> > > Actually, I've realized that this patch will have no effect on the
> > > PCIe-DAS1602/16 card because it uses a different driver -
> > > cb_pcimdas, not cb_pcidas.
> > 
> > But that's also confusing because PCIe-DAS1602/16 was not supported
> > until the 3.19 kernel!  I know the reported has both PCI-DAS1602/16 and
> > PCIe-DAS1602/16 cards (supported by cb_pcidas and cb_pcimdas
> > respectively), so there could have been some mix-up in the reporting.
> 
> Mystery solved.  The reporter had a mixture of PCIe-DAS1602/16 and
> PCIM-DAS1602/16 cards (not PCI-DAS1602/16).  Both of those are supported by
> the "cb_pcimdas" driver (not "cb_pcidas"), although the PCIe card was not
> supported until the 3.19 kernel (by commit 4e3d14af1286).  Testing with the
> 3.13 kernel was done with the PCIM card.
> 
> The "strange scaling" was due to a change in the ranges reported for the
> analog input subdevice in the 4.1 kernel (by commit c7549d770a27). Before
> then, it just reported a single dummy range [0, 1000000] with no units
> (converted to [0.0, 1.0] with no units by comedilib).  Afterwards, it
> reported four different voltage ranges (either unipolar or bipolar,
> depending in a status bit tied to a physical switch).  The reporter's
> application code was using the reported range to scale the raw values to a
> voltage (using comedilib functions), but because the reported range was
> bogus, the application code was performing additional scaling (outside of
> comedilib).  The application code can be changed to check whether the device
> is reporting a proper voltage range or the old, bogus range, and behave
> accordingly.
> 
> > > Greg, you might as well drop this patch if you haven't already
> > > applied it, since it was only a hunch that it fixed a problem.
> 
> That's still the case, although it won't do any harm if applied (apart from
> the incorrect patch description).

I'll leave it dropped :)

thanks,

greg k-h