diff for duplicates of <20181201151747.16522ff4@archlinux> diff --git a/a/1.txt b/N1/1.txt index 121bc97..c7a6942 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -4,24 +4,23 @@ On Thu, 29 Nov 2018 12:19:08 +0000 > On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote: > > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote: > > Hi, please see bellow -> > =20 ->=20 +> > +> > One note from me here. ->=20 +> > > > Hi, thank you for the review -> > > =20 -> > > >=20 +> > > +> > > > > > > > On Thu, 22 Nov 2018 11:01:00 +0000 -> > > > "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote: =20 -> > > > >=20 +> > > > "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote: +> > > > > > > > > > I think that instead of setting the gain directly, we should use > > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 > > > > > datasheet > > > > > there > > > > > is a formula from which the output code can be calculated: -> > > > > Code =3D 2^(N =E2=88=92 1) -> > > > > =C3=97 [(AIN =C3=97 Gain /VREF) + 1]. So, by setting the scale fr= -om user +> > > > > Code = 2^(N − 1) +> > > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user > > > > > space, > > > > > the > > > > > driver can calculate the correct gain by using the formula above. @@ -31,43 +30,36 @@ om user > > > > > ad7124 adc driver which does this exact thing. Take a look here: > > > > > http > > > > > s://gi -> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad712= -4. +> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124. > > > > > c# -> > > > > L337. =20 -> > >=20 +> > > > > L337. +> > > > > > We have some questions about the code you provided to us: -> > > 1-) What is exactly the inputs for the write_raw function? =20 -> >=20 +> > > 1-) What is exactly the inputs for the write_raw function? +> > > > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. -> > Setting the scale from user space looks something like this: =20 -> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale= - . =20 -> > Furthermore, in your write_raw() function, val=3D0 and val2=3D298. -> > Knowing that full_scale_voltage =3D Vref / (gain * scale), we can calcu= -late -> > the gain =3D Vref / (full_scale_voltage * scale). We only support two g= -ains +> > Setting the scale from user space looks something like this: +> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . +> > Furthermore, in your write_raw() function, val=0 and val2=298. +> > Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate +> > the gain = Vref / (full_scale_voltage * scale). We only support two gains > > (1 and 128), so we need to determine which one fits better with the > > desired -> > scale. Finally, all we have left to do is to set the gain.=20 -> > =20 +> > scale. Finally, all we have left to do is to set the gain. +> > > > > 2-) Could you give more details about the math around lines 346-348? > > > Is it correct to assume that the multiplication at line 346 won't -> > > overflow? (vref is an uint) =20 -> >=20 -> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V = - =3D -> > 2500000000uV. It won't overflow since we use the Vref as nominator, whi= -le +> > > overflow? (vref is an uint) +> > +> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V = +> > 2500000000uV. It won't overflow since we use the Vref as nominator, while > > full_scale_voltage and scale are the denominators. -> > =20 ->=20 +> > +> > [Regarding the AD7124] I guess I should be noted that the code can > overflow, but in this case it shouldn't, because (according to the device > datasheet) the maximum VREF can be 3600 mV. -> A user could specify (in the devicetree) something larger than 4300 mV (a= -nd +> A user could specify (in the devicetree) something larger than 4300 mV (and > that would overflow) but that would also make the readings useless as the > external VREF would be invalid ; and there is also the risk of frying the > chip in that case [something you really can't protect the user against]. @@ -86,49 +78,43 @@ I'm not going to review every driver to ensure they do this, but I'm keen on them being checked where a possible problem has been identified! Jonathan ->=20 +> > The internal VREF however is 2500 mV, so things should be fine from that > point of view. ->=20 -> Typically, in datasheets (at least from Analog Devices) it's good to take= - a +> +> Typically, in datasheets (at least from Analog Devices) it's good to take a > look at the specifications sections. > [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with -> approximation of +/-0.2%) and for external reference it goes all the way = -up +> approximation of +/-0.2%) and for external reference it goes all the way up > to AVDD, which has typical values of 2.9V - 3.6V. > So, for u32 this code should be fine and not overflow. ->=20 +> > One small thing that can be confusing about that code in AD7124 is that it -> gets multiplied with 1000000LL (which is signed long-long), but that shou= -ld +> gets multiplied with 1000000LL (which is signed long-long), but that should > be fine, since the operation should be converted to u32 (unsigned int) -> representation [by being assigned to vref], which ends up being fine in t= -he +> representation [by being assigned to vref], which ends up being fine in the > end. ->=20 -> > >=20 +> +> > > > > > And regarding our code: > > > 1-) The val in our write_raw function should be, in case of GAIN, a > > > number that best approximate the actual gain value of 1 or 128? For -> > > instance, if the user inputs 126, we should default to 128? =20 -> >=20 -> > We should not allow the the user to input the gain, he needs to input t= -he -> > scale (see the mail from Jonathan and the above explanation). However, = -if +> > > instance, if the user inputs 126, we should default to 128? +> > +> > We should not allow the the user to input the gain, he needs to input the +> > scale (see the mail from Jonathan and the above explanation). However, if > > the calculated gain is one that is not supported, such as 126, we will > > set > > the closest matching value, in this case 128. -> > =20 +> > > > > 2-) In the case of FILTER, is it the same? Is the user sending the -> > > value in mHz (milihertz)? =20 -> >=20 +> > > value in mHz (milihertz)? +> > > > Yes, it is the same with the FILTER. You need to add > > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user -> > space, the input value should be in Hz, something like this: =20 -> > root:/sys/bus/iio/devices/iio:device0> echo 10 > =20 +> > space, the input value should be in Hz, something like this: +> > root:/sys/bus/iio/devices/iio:device0> echo 10 > > > in_voltage_sampling_frequency -> > =20 -> > >=20 -> > > Thank you =20 +> > +> > > +> > > Thank you diff --git a/a/content_digest b/N1/content_digest index 6c4d34d..b49eada 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -28,24 +28,23 @@ "> On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote:\n" "> > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:\n" "> > Hi, please see bellow\n" - "> > =20\n" - ">=20\n" + "> > \n" + "> \n" "> One note from me here.\n" - ">=20\n" + "> \n" "> > > Hi, thank you for the review\n" - "> > > =20\n" - "> > > >=20\n" + "> > > \n" + "> > > > \n" "> > > > On Thu, 22 Nov 2018 11:01:00 +0000\n" - "> > > > \"Popa, Stefan Serban\" <StefanSerban.Popa@analog.com> wrote: =20\n" - "> > > > >=20\n" + "> > > > \"Popa, Stefan Serban\" <StefanSerban.Popa@analog.com> wrote: \n" + "> > > > > \n" "> > > > > I think that instead of setting the gain directly, we should use\n" "> > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780\n" "> > > > > datasheet\n" "> > > > > there\n" "> > > > > is a formula from which the output code can be calculated:\n" - "> > > > > Code =3D 2^(N =E2=88=92 1)\n" - "> > > > > =C3=97 [(AIN =C3=97 Gain /VREF) + 1]. So, by setting the scale fr=\n" - "om user\n" + "> > > > > Code = 2^(N \342\210\222 1)\n" + "> > > > > \303\227 [(AIN \303\227 Gain /VREF) + 1]. So, by setting the scale from user\n" "> > > > > space,\n" "> > > > > the\n" "> > > > > driver can calculate the correct gain by using the formula above.\n" @@ -55,43 +54,36 @@ "> > > > > ad7124 adc driver which does this exact thing. Take a look here:\n" "> > > > > http\n" "> > > > > s://gi\n" - "> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad712=\n" - "4.\n" + "> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.\n" "> > > > > c#\n" - "> > > > > L337. =20\n" - "> > >=20\n" + "> > > > > L337. \n" + "> > > \n" "> > > We have some questions about the code you provided to us:\n" - "> > > 1-) What is exactly the inputs for the write_raw function? =20\n" - "> >=20\n" + "> > > 1-) What is exactly the inputs for the write_raw function? \n" + "> > \n" "> > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case.\n" - "> > Setting the scale from user space looks something like this: =20\n" - "> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale=\n" - " . =20\n" - "> > Furthermore, in your write_raw() function, val=3D0 and val2=3D298.\n" - "> > Knowing that full_scale_voltage =3D Vref / (gain * scale), we can calcu=\n" - "late\n" - "> > the gain =3D Vref / (full_scale_voltage * scale). We only support two g=\n" - "ains\n" + "> > Setting the scale from user space looks something like this: \n" + "> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . \n" + "> > Furthermore, in your write_raw() function, val=0 and val2=298.\n" + "> > Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate\n" + "> > the gain = Vref / (full_scale_voltage * scale). We only support two gains\n" "> > (1 and 128), so we need to determine which one fits better with the\n" "> > desired\n" - "> > scale. Finally, all we have left to do is to set the gain.=20\n" - "> > =20\n" + "> > scale. Finally, all we have left to do is to set the gain. \n" + "> > \n" "> > > 2-) Could you give more details about the math around lines 346-348?\n" "> > > Is it correct to assume that the multiplication at line 346 won't\n" - "> > > overflow? (vref is an uint) =20\n" - "> >=20\n" - "> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V =\n" - " =3D\n" - "> > 2500000000uV. It won't overflow since we use the Vref as nominator, whi=\n" - "le\n" + "> > > overflow? (vref is an uint) \n" + "> > \n" + "> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V =\n" + "> > 2500000000uV. It won't overflow since we use the Vref as nominator, while\n" "> > full_scale_voltage and scale are the denominators.\n" - "> > =20\n" - ">=20\n" + "> > \n" + "> \n" "> [Regarding the AD7124] I guess I should be noted that the code can\n" "> overflow, but in this case it shouldn't, because (according to the device\n" "> datasheet) the maximum VREF can be 3600 mV.\n" - "> A user could specify (in the devicetree) something larger than 4300 mV (a=\n" - "nd\n" + "> A user could specify (in the devicetree) something larger than 4300 mV (and\n" "> that would overflow) but that would also make the readings useless as the\n" "> external VREF would be invalid ; and there is also the risk of frying the\n" "> chip in that case [something you really can't protect the user against].\n" @@ -110,51 +102,45 @@ "on them being checked where a possible problem has been identified!\n" "\n" "Jonathan\n" - ">=20\n" + "> \n" "> The internal VREF however is 2500 mV, so things should be fine from that\n" "> point of view.\n" - ">=20\n" - "> Typically, in datasheets (at least from Analog Devices) it's good to take=\n" - " a\n" + "> \n" + "> Typically, in datasheets (at least from Analog Devices) it's good to take a\n" "> look at the specifications sections.\n" "> [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with\n" - "> approximation of +/-0.2%) and for external reference it goes all the way =\n" - "up\n" + "> approximation of +/-0.2%) and for external reference it goes all the way up\n" "> to AVDD, which has typical values of 2.9V - 3.6V.\n" "> So, for u32 this code should be fine and not overflow.\n" - ">=20\n" + "> \n" "> One small thing that can be confusing about that code in AD7124 is that it\n" - "> gets multiplied with 1000000LL (which is signed long-long), but that shou=\n" - "ld\n" + "> gets multiplied with 1000000LL (which is signed long-long), but that should\n" "> be fine, since the operation should be converted to u32 (unsigned int)\n" - "> representation [by being assigned to vref], which ends up being fine in t=\n" - "he\n" + "> representation [by being assigned to vref], which ends up being fine in the\n" "> end.\n" - ">=20\n" - "> > >=20\n" + "> \n" + "> > > \n" "> > > And regarding our code:\n" "> > > 1-) The val in our write_raw function should be, in case of GAIN, a\n" "> > > number that best approximate the actual gain value of 1 or 128? For\n" - "> > > instance, if the user inputs 126, we should default to 128? =20\n" - "> >=20\n" - "> > We should not allow the the user to input the gain, he needs to input t=\n" - "he\n" - "> > scale (see the mail from Jonathan and the above explanation). However, =\n" - "if\n" + "> > > instance, if the user inputs 126, we should default to 128? \n" + "> > \n" + "> > We should not allow the the user to input the gain, he needs to input the\n" + "> > scale (see the mail from Jonathan and the above explanation). However, if\n" "> > the calculated gain is one that is not supported, such as 126, we will\n" "> > set\n" "> > the closest matching value, in this case 128.\n" - "> > =20\n" + "> > \n" "> > > 2-) In the case of FILTER, is it the same? Is the user sending the\n" - "> > > value in mHz (milihertz)? =20\n" - "> >=20\n" + "> > > value in mHz (milihertz)? \n" + "> > \n" "> > Yes, it is the same with the FILTER. You need to add\n" "> > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user\n" - "> > space, the input value should be in Hz, something like this: =20\n" - "> > root:/sys/bus/iio/devices/iio:device0> echo 10 > =20\n" + "> > space, the input value should be in Hz, something like this: \n" + "> > root:/sys/bus/iio/devices/iio:device0> echo 10 > \n" "> > in_voltage_sampling_frequency\n" - "> > =20\n" - "> > >=20\n" - > > > Thank you =20 + "> > \n" + "> > > \n" + > > > Thank you -018ebb59f9ec581238ecec352613cee32666e9d92c36efba5e2a27817d6581bb +7321190a30de0cbd80f472a7451db9bfc73bacacd23fff3c49e60a58d89db7ac
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox