From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <50E24B24-6DC9-4ECB-8752-B974BD6C53E8@kernel.org> References: <1436880983-29017-1-git-send-email-gizero@gmail.com> <50E24B24-6DC9-4ECB-8752-B974BD6C53E8@kernel.org> Date: Wed, 15 Jul 2015 07:07:27 +0200 Message-ID: Subject: Re: [PATCH 1/4] iio: adc: mcp320x: add masking of unknown bits From: Andrea Galbusera To: Jonathan Cameron Cc: linux-iio@vger.kernel.org Content-Type: multipart/alternative; boundary=001a11c37f7e5b2432051ae2ee09 List-ID: --001a11c37f7e5b2432051ae2ee09 Content-Type: text/plain; charset=UTF-8 On Tue, Jul 14, 2015 at 11:04 PM, Jonathan Cameron wrote: > > > On 14 July 2015 14:36:20 BST, Andrea Galbusera wrote: > >According to the datasheet, the 2 MSBs for parts 3001 and 3201 are > >unspecified and should be masked out > Theoretical issue or one with observed bad effects? > Hi! Thanks for reviewing. While in the business of adding support for 3301, I couldn't figure out why no masking wasn't done for similar parts like 3001 and 3201. Further investigation of previous patches reviews [1], where masking was suggested and initially implemented, convinced me the bit was lost somewhere, with no explicit motivation. Nevertheless, 3301 is the only chip of the family I have at hands: hence, yes, this specific patch in the set is addressing a theoretical issue. [1] http://marc.info/?l=linux-iio&m=140748835509583&w=2 > Changes the path and timing of this getting applied. Patch itself is fine > either way! > > > >Signed-off-by: Andrea Galbusera > >--- > > > >This was already suggested but for some reason got lost during review > >of a > >previous patch. See http://marc.info/?l=linux-iio&m=140748835509583&w=2 > >and following discussion > > > > drivers/iio/adc/mcp320x.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c > >index 8d9c9b9..0673ee7 100644 > >--- a/drivers/iio/adc/mcp320x.c > >+++ b/drivers/iio/adc/mcp320x.c > >@@ -114,13 +114,13 @@ static int mcp320x_adc_conversion(struct mcp320x > >*adc, u8 channel, > > > > switch (device_index) { > > case mcp3001: > >- return (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3); > >+ return ((adc->rx_buf[0] & 0x1f) << 5 | adc->rx_buf[1] >> > 3); > > case mcp3002: > > case mcp3004: > > case mcp3008: > > return (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6); > > case mcp3201: > >- return (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1); > >+ return ((adc->rx_buf[0] & 0x1f) << 7 | adc->rx_buf[1] >> > 1); > > case mcp3202: > > case mcp3204: > > case mcp3208: > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. > --001a11c37f7e5b2432051ae2ee09 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On T= ue, Jul 14, 2015 at 11:04 PM, Jonathan Cameron <jic23@kernel.org> wrote:


On 14 July 2015 14:36:20 BST, Andrea Galbusera <gizero@gmail.com> wrote:
>According to the datasheet, the 2 MSBs for parts 3001 and 3201 are
>unspecified and should be masked out
Theoretical issue or one with observed bad effects?
=

Hi! Thanks for r= eviewing. While in the business of adding support for 3301, I couldn't = figure out why no masking wasn't done for similar parts like 3001 and 3= 201. Further investigation of previous patches reviews [1], where masking w= as suggested and initially implemented, convinced me the bit was lost somew= here, with no explicit motivation. Nevertheless, 3301 is the only chip of t= he family I have at hands: hence, yes, this specific patch in the set is ad= dressing a theoretical issue.=C2=A0
=C2=A0
=C2=A0
Changes the path and timing of this getting applied. Patch itself is fine e= ither way!
>
>Signed-off-by: Andrea Galbusera <gizero@gmail.com>
>---
>
>This was already suggested but for some reason got lost during review >of a
>previous patch. See http://marc= .info/?l=3Dlinux-iio&m=3D140748835509583&w=3D2
>and following discussion
>
> drivers/iio/adc/mcp320x.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
>index 8d9c9b9..0673ee7 100644
>--- a/drivers/iio/adc/mcp320x.c
>+++ b/drivers/iio/adc/mcp320x.c
>@@ -114,13 +114,13 @@ static int mcp320x_adc_conversion(struct mcp320x<= br> >*adc, u8 channel,
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0switch (device_index) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3001:
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (adc->rx_bu= f[0] << 5 | adc->rx_buf[1] >> 3);
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ((adc->rx_b= uf[0] & 0x1f) << 5 | adc->rx_buf[1] >> 3);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3002:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3004:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3008:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (adc->= rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3201:
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (adc->rx_bu= f[0] << 7 | adc->rx_buf[1] >> 1);
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ((adc->rx_b= uf[0] & 0x1f) << 7 | adc->rx_buf[1] >> 1);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3202:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3204:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case mcp3208:

--
Sent from my Android device with K-9 Mail. Please excuse my bre= vity.

--001a11c37f7e5b2432051ae2ee09--