[PATCH v2 0/2] pattern generation and gain update

[PATCH v2 0/2] pattern generation and gain update

[Giuliano Belinassi]

This series of patches fixes a bug in ad717x chips where the PAT2 bit
was wrongly read as a GAIN bit. It also refactors the pattern_mask
generation with the PAT bits.

Changelog:
* v2
	- Squashed is_add778x flag commit with the gain update fix
	- Changed u8 is_add778x to bool is_add778x
	- Set pattern and pattern_mask as macros
	- Aligned identation of macros

Giuliano Belinassi (2):
  staging: iio: ad7780: check if ad778x before gain update
  staging: iio: ad7780: generates pattern_mask from PAT bits

 drivers/staging/iio/adc/ad7780.c | 55 ++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 20 deletions(-)

-- 
2.19.1

[PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Giuliano Belinassi]

Only the ad778x have the 'gain' status bit. Check it before updating
through a new variable is_ad778x in chip_info.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v2:
	- Squashed is_ad778x declaration commit with the ad778x checkage
	- Changed is_ad778x type to bool
 
 drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 91e016d534ed..9ec2b002539e 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -35,6 +35,7 @@ struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
 	unsigned int		pattern;
+	bool			is_ad778x;
 };
 
 struct ad7780_state {
@@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
 		return -EIO;
 
-	if (raw_sample & AD7780_GAIN)
-		st->gain = 1;
-	else
-		st->gain = 128;
+	if (chip_info->is_ad778x) {
+		if (raw_sample & AD7780_GAIN)
+			st->gain = 1;
+		else
+			st->gain = 128;
+	}
 
 	return 0;
 }
@@ -135,21 +138,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 		.channel = AD7780_CHANNEL(12, 24),
 		.pattern = 0x5,
 		.pattern_mask = 0x7,
+		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
 		.channel = AD7780_CHANNEL(16, 24),
 		.pattern = 0x5,
 		.pattern_mask = 0x7,
+		.is_ad778x = false,
 	},
 	[ID_AD7780] = {
 		.channel = AD7780_CHANNEL(24, 32),
 		.pattern = 0x1,
 		.pattern_mask = 0x3,
+		.is_ad778x = true,
 	},
 	[ID_AD7781] = {
 		.channel = AD7780_CHANNEL(20, 32),
 		.pattern = 0x1,
 		.pattern_mask = 0x3,
+		.is_ad778x = true,
 	},
 };
 
-- 
2.19.1

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

T24gVGh1LCAyMDE4LTExLTA4IGF0IDExOjAzIC0wMjAwLCBHaXVsaWFubyBCZWxpbmFzc2kgd3Jv
dGU6DQo+IE9ubHkgdGhlIGFkNzc4eCBoYXZlIHRoZSAnZ2Fpbicgc3RhdHVzIGJpdC4gQ2hlY2sg
aXQgYmVmb3JlIHVwZGF0aW5nDQo+IHRocm91Z2ggYSBuZXcgdmFyaWFibGUgaXNfYWQ3Nzh4IGlu
IGNoaXBfaW5mby4NCj4gDQoNCkxvb2tzIGdvb2QuDQoNCkFsZXgNCg0KPiBTaWduZWQtb2ZmLWJ5
OiBHaXVsaWFubyBCZWxpbmFzc2kgPGdpdWxpYW5vLmJlbGluYXNzaUB1c3AuYnI+DQo+IC0tLQ0K
PiBDaGFuZ2VzIGluIHYyOg0KPiAJLSBTcXVhc2hlZCBpc19hZDc3OHggZGVjbGFyYXRpb24gY29t
bWl0IHdpdGggdGhlIGFkNzc4eCBjaGVja2FnZQ0KPiAJLSBDaGFuZ2VkIGlzX2FkNzc4eCB0eXBl
IHRvIGJvb2wNCj4gIA0KPiAgZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMgfCAxNSAr
KysrKysrKysrKy0tLS0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxMSBpbnNlcnRpb25zKCspLCA0IGRl
bGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2Fk
Nzc4MC5jDQo+IGIvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gaW5kZXggOTFl
MDE2ZDUzNGVkLi45ZWMyYjAwMjUzOWUgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvc3RhZ2luZy9p
aW8vYWRjL2FkNzc4MC5jDQo+ICsrKyBiL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5j
DQo+IEBAIC0zNSw2ICszNSw3IEBAIHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvIHsNCj4gIAlzdHJ1
Y3QgaWlvX2NoYW5fc3BlYwljaGFubmVsOw0KPiAgCXVuc2lnbmVkIGludAkJcGF0dGVybl9tYXNr
Ow0KPiAgCXVuc2lnbmVkIGludAkJcGF0dGVybjsNCj4gKwlib29sCQkJaXNfYWQ3Nzh4Ow0KPiAg
fTsNCj4gIA0KPiAgc3RydWN0IGFkNzc4MF9zdGF0ZSB7DQo+IEBAIC0xMTMsMTAgKzExNCwxMiBA
QCBzdGF0aWMgaW50IGFkNzc4MF9wb3N0cHJvY2Vzc19zYW1wbGUoc3RydWN0DQo+IGFkX3NpZ21h
X2RlbHRhICpzaWdtYV9kZWx0YSwNCj4gIAkgICAgKChyYXdfc2FtcGxlICYgY2hpcF9pbmZvLT5w
YXR0ZXJuX21hc2spICE9IGNoaXBfaW5mby0+cGF0dGVybikpDQo+ICAJCXJldHVybiAtRUlPOw0K
PiAgDQo+IC0JaWYgKHJhd19zYW1wbGUgJiBBRDc3ODBfR0FJTikNCj4gLQkJc3QtPmdhaW4gPSAx
Ow0KPiAtCWVsc2UNCj4gLQkJc3QtPmdhaW4gPSAxMjg7DQo+ICsJaWYgKGNoaXBfaW5mby0+aXNf
YWQ3Nzh4KSB7DQo+ICsJCWlmIChyYXdfc2FtcGxlICYgQUQ3NzgwX0dBSU4pDQo+ICsJCQlzdC0+
Z2FpbiA9IDE7DQo+ICsJCWVsc2UNCj4gKwkJCXN0LT5nYWluID0gMTI4Ow0KPiArCX0NCj4gIA0K
PiAgCXJldHVybiAwOw0KPiAgfQ0KPiBAQCAtMTM1LDIxICsxMzgsMjUgQEAgc3RhdGljIGNvbnN0
IHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvDQo+IGFkNzc4MF9jaGlwX2luZm9fdGJsW10gPSB7DQo+
ICAJCS5jaGFubmVsID0gQUQ3NzgwX0NIQU5ORUwoMTIsIDI0KSwNCj4gIAkJLnBhdHRlcm4gPSAw
eDUsDQo+ICAJCS5wYXR0ZXJuX21hc2sgPSAweDcsDQo+ICsJCS5pc19hZDc3OHggPSBmYWxzZSwN
Cj4gIAl9LA0KPiAgCVtJRF9BRDcxNzFdID0gew0KPiAgCQkuY2hhbm5lbCA9IEFENzc4MF9DSEFO
TkVMKDE2LCAyNCksDQo+ICAJCS5wYXR0ZXJuID0gMHg1LA0KPiAgCQkucGF0dGVybl9tYXNrID0g
MHg3LA0KPiArCQkuaXNfYWQ3Nzh4ID0gZmFsc2UsDQo+ICAJfSwNCj4gIAlbSURfQUQ3NzgwXSA9
IHsNCj4gIAkJLmNoYW5uZWwgPSBBRDc3ODBfQ0hBTk5FTCgyNCwgMzIpLA0KPiAgCQkucGF0dGVy
biA9IDB4MSwNCj4gIAkJLnBhdHRlcm5fbWFzayA9IDB4MywNCj4gKwkJLmlzX2FkNzc4eCA9IHRy
dWUsDQo+ICAJfSwNCj4gIAlbSURfQUQ3NzgxXSA9IHsNCj4gIAkJLmNoYW5uZWwgPSBBRDc3ODBf
Q0hBTk5FTCgyMCwgMzIpLA0KPiAgCQkucGF0dGVybiA9IDB4MSwNCj4gIAkJLnBhdHRlcm5fbWFz
ayA9IDB4MywNCj4gKwkJLmlzX2FkNzc4eCA9IHRydWUsDQo+ICAJfSwNCj4gIH07DQo+ICANCg==

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Jonathan Cameron]

On Thu, 8 Nov 2018 13:44:17 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > Only the ad778x have the 'gain' status bit. Check it before updating
> > through a new variable is_ad778x in chip_info.
> >   
> 
> Looks good.
Alex, formal tags definitely preferred!  It's not as though a
looks good is any less of a review than an Ack, it's just better
hidden as people need to look at mailing list archives...

Jonathan

> 
> Alex
> 
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > Changes in v2:
> > 	- Squashed is_ad778x declaration commit with the ad778x checkage
> > 	- Changed is_ad778x type to bool
> >  
> >  drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..9ec2b002539e 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> >  	struct iio_chan_spec	channel;
> >  	unsigned int		pattern_mask;
> >  	unsigned int		pattern;
> > +	bool			is_ad778x;
> >  };
> >  
> >  struct ad7780_state {
> > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > ad_sigma_delta *sigma_delta,
> >  	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> >  		return -EIO;
> >  
> > -	if (raw_sample & AD7780_GAIN)
> > -		st->gain = 1;
> > -	else
> > -		st->gain = 128;
> > +	if (chip_info->is_ad778x) {
> > +		if (raw_sample & AD7780_GAIN)
> > +			st->gain = 1;
> > +		else
> > +			st->gain = 128;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] = {
> >  		.channel = AD7780_CHANNEL(12, 24),
> >  		.pattern = 0x5,
> >  		.pattern_mask = 0x7,
> > +		.is_ad778x = false,
> >  	},
> >  	[ID_AD7171] = {
> >  		.channel = AD7780_CHANNEL(16, 24),
> >  		.pattern = 0x5,
> >  		.pattern_mask = 0x7,
> > +		.is_ad778x = false,
> >  	},
> >  	[ID_AD7780] = {
> >  		.channel = AD7780_CHANNEL(24, 32),
> >  		.pattern = 0x1,
> >  		.pattern_mask = 0x3,
> > +		.is_ad778x = true,
> >  	},
> >  	[ID_AD7781] = {
> >  		.channel = AD7780_CHANNEL(20, 32),
> >  		.pattern = 0x1,
> >  		.pattern_mask = 0x3,
> > +		.is_ad778x = true,
> >  	},
> >  };
> >    

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

T24gU3VuLCAyMDE4LTExLTExIGF0IDEyOjU4ICswMDAwLCBKb25hdGhhbiBDYW1lcm9uIHdyb3Rl
Og0KPiBPbiBUaHUsIDggTm92IDIwMTggMTM6NDQ6MTcgKzAwMDANCj4gIkFyZGVsZWFuLCBBbGV4
YW5kcnUiIDxhbGV4YW5kcnUuQXJkZWxlYW5AYW5hbG9nLmNvbT4gd3JvdGU6DQo+IA0KPiA+IE9u
IFRodSwgMjAxOC0xMS0wOCBhdCAxMTowMyAtMDIwMCwgR2l1bGlhbm8gQmVsaW5hc3NpIHdyb3Rl
Og0KPiA+ID4gT25seSB0aGUgYWQ3Nzh4IGhhdmUgdGhlICdnYWluJyBzdGF0dXMgYml0LiBDaGVj
ayBpdCBiZWZvcmUgdXBkYXRpbmcNCj4gPiA+IHRocm91Z2ggYSBuZXcgdmFyaWFibGUgaXNfYWQ3
Nzh4IGluIGNoaXBfaW5mby4NCj4gPiA+ICAgDQo+ID4gDQo+ID4gTG9va3MgZ29vZC4NCj4gDQo+
IEFsZXgsIGZvcm1hbCB0YWdzIGRlZmluaXRlbHkgcHJlZmVycmVkISAgSXQncyBub3QgYXMgdGhv
dWdoIGENCj4gbG9va3MgZ29vZCBpcyBhbnkgbGVzcyBvZiBhIHJldmlldyB0aGFuIGFuIEFjaywg
aXQncyBqdXN0IGJldHRlcg0KPiBoaWRkZW4gYXMgcGVvcGxlIG5lZWQgdG8gbG9vayBhdCBtYWls
aW5nIGxpc3QgYXJjaGl2ZXMuLi4NCj4gDQo+IEpvbmF0aGFuDQo+IA0KDQpBY2tlZC1ieTogQWxl
eGFuZHJ1IEFyZGVsZWFuIDxhbGV4YW5kcnUuYXJkZWxlYW5AYW5hbG9nLmNvbT4NCg0KLy8gV2ls
bCByZW1lbWJlciB0aGF0IG5leHQgdGltZSA6KQ0KDQpUaGFua3MNCkFsZXgNCg0KPiA+IA0KPiA+
IEFsZXgNCj4gPiANCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEdpdWxpYW5vIEJlbGluYXNzaSA8Z2l1
bGlhbm8uYmVsaW5hc3NpQHVzcC5icj4NCj4gPiA+IC0tLQ0KPiA+ID4gQ2hhbmdlcyBpbiB2MjoN
Cj4gPiA+IAktIFNxdWFzaGVkIGlzX2FkNzc4eCBkZWNsYXJhdGlvbiBjb21taXQgd2l0aCB0aGUg
YWQ3Nzh4IGNoZWNrYWdlDQo+ID4gPiAJLSBDaGFuZ2VkIGlzX2FkNzc4eCB0eXBlIHRvIGJvb2wN
Cj4gPiA+ICANCj4gPiA+ICBkcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYyB8IDE1ICsr
KysrKysrKysrLS0tLQ0KPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCAxMSBpbnNlcnRpb25zKCspLCA0
IGRlbGV0aW9ucygtKQ0KPiA+ID4gDQo+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5n
L2lpby9hZGMvYWQ3NzgwLmMNCj4gPiA+IGIvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3Nzgw
LmMNCj4gPiA+IGluZGV4IDkxZTAxNmQ1MzRlZC4uOWVjMmIwMDI1MzllIDEwMDY0NA0KPiA+ID4g
LS0tIGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gPiA+ICsrKyBiL2RyaXZl
cnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jDQo+ID4gPiBAQCAtMzUsNiArMzUsNyBAQCBzdHJ1
Y3QgYWQ3NzgwX2NoaXBfaW5mbyB7DQo+ID4gPiAgCXN0cnVjdCBpaW9fY2hhbl9zcGVjCWNoYW5u
ZWw7DQo+ID4gPiAgCXVuc2lnbmVkIGludAkJcGF0dGVybl9tYXNrOw0KPiA+ID4gIAl1bnNpZ25l
ZCBpbnQJCXBhdHRlcm47DQo+ID4gPiArCWJvb2wJCQlpc19hZDc3OHg7DQo+ID4gPiAgfTsNCj4g
PiA+ICANCj4gPiA+ICBzdHJ1Y3QgYWQ3NzgwX3N0YXRlIHsNCj4gPiA+IEBAIC0xMTMsMTAgKzEx
NCwxMiBAQCBzdGF0aWMgaW50IGFkNzc4MF9wb3N0cHJvY2Vzc19zYW1wbGUoc3RydWN0DQo+ID4g
PiBhZF9zaWdtYV9kZWx0YSAqc2lnbWFfZGVsdGEsDQo+ID4gPiAgCSAgICAoKHJhd19zYW1wbGUg
JiBjaGlwX2luZm8tPnBhdHRlcm5fbWFzaykgIT0gY2hpcF9pbmZvLT5wYXR0ZXJuKSkNCj4gPiA+
ICAJCXJldHVybiAtRUlPOw0KPiA+ID4gIA0KPiA+ID4gLQlpZiAocmF3X3NhbXBsZSAmIEFENzc4
MF9HQUlOKQ0KPiA+ID4gLQkJc3QtPmdhaW4gPSAxOw0KPiA+ID4gLQllbHNlDQo+ID4gPiAtCQlz
dC0+Z2FpbiA9IDEyODsNCj4gPiA+ICsJaWYgKGNoaXBfaW5mby0+aXNfYWQ3Nzh4KSB7DQo+ID4g
PiArCQlpZiAocmF3X3NhbXBsZSAmIEFENzc4MF9HQUlOKQ0KPiA+ID4gKwkJCXN0LT5nYWluID0g
MTsNCj4gPiA+ICsJCWVsc2UNCj4gPiA+ICsJCQlzdC0+Z2FpbiA9IDEyODsNCj4gPiA+ICsJfQ0K
PiA+ID4gIA0KPiA+ID4gIAlyZXR1cm4gMDsNCj4gPiA+ICB9DQo+ID4gPiBAQCAtMTM1LDIxICsx
MzgsMjUgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvDQo+ID4gPiBhZDc3
ODBfY2hpcF9pbmZvX3RibFtdID0gew0KPiA+ID4gIAkJLmNoYW5uZWwgPSBBRDc3ODBfQ0hBTk5F
TCgxMiwgMjQpLA0KPiA+ID4gIAkJLnBhdHRlcm4gPSAweDUsDQo+ID4gPiAgCQkucGF0dGVybl9t
YXNrID0gMHg3LA0KPiA+ID4gKwkJLmlzX2FkNzc4eCA9IGZhbHNlLA0KPiA+ID4gIAl9LA0KPiA+
ID4gIAlbSURfQUQ3MTcxXSA9IHsNCj4gPiA+ICAJCS5jaGFubmVsID0gQUQ3NzgwX0NIQU5ORUwo
MTYsIDI0KSwNCj4gPiA+ICAJCS5wYXR0ZXJuID0gMHg1LA0KPiA+ID4gIAkJLnBhdHRlcm5fbWFz
ayA9IDB4NywNCj4gPiA+ICsJCS5pc19hZDc3OHggPSBmYWxzZSwNCj4gPiA+ICAJfSwNCj4gPiA+
ICAJW0lEX0FENzc4MF0gPSB7DQo+ID4gPiAgCQkuY2hhbm5lbCA9IEFENzc4MF9DSEFOTkVMKDI0
LCAzMiksDQo+ID4gPiAgCQkucGF0dGVybiA9IDB4MSwNCj4gPiA+ICAJCS5wYXR0ZXJuX21hc2sg
PSAweDMsDQo+ID4gPiArCQkuaXNfYWQ3Nzh4ID0gdHJ1ZSwNCj4gPiA+ICAJfSwNCj4gPiA+ICAJ
W0lEX0FENzc4MV0gPSB7DQo+ID4gPiAgCQkuY2hhbm5lbCA9IEFENzc4MF9DSEFOTkVMKDIwLCAz
MiksDQo+ID4gPiAgCQkucGF0dGVybiA9IDB4MSwNCj4gPiA+ICAJCS5wYXR0ZXJuX21hc2sgPSAw
eDMsDQo+ID4gPiArCQkuaXNfYWQ3Nzh4ID0gdHJ1ZSwNCj4gPiA+ICAJfSwNCj4gPiA+ICB9Ow0K
PiA+ID4gICAgDQo+IA0KPiANCg==

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Jonathan Cameron]

On Mon, 12 Nov 2018 07:57:58 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2018-11-11 at 12:58 +0000, Jonathan Cameron wrote:
> > On Thu, 8 Nov 2018 13:44:17 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >   
> > > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:  
> > > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > > through a new variable is_ad778x in chip_info.
> > > >     
> > > 
> > > Looks good.  
> > 
> > Alex, formal tags definitely preferred!  It's not as though a
> > looks good is any less of a review than an Ack, it's just better
> > hidden as people need to look at mailing list archives...
> > 
> > Jonathan
> >   
> 
> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I haven't pushed out togreg yet so can still rebase.

Added. Thanks,

Jonathan

> 
> // Will remember that next time :)
> 
> Thanks
> Alex
> 
> > > 
> > > Alex
> > >   
> > > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > > > ---
> > > > Changes in v2:
> > > > 	- Squashed is_ad778x declaration commit with the ad778x checkage
> > > > 	- Changed is_ad778x type to bool
> > > >  
> > > >  drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 91e016d534ed..9ec2b002539e 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > > >  	struct iio_chan_spec	channel;
> > > >  	unsigned int		pattern_mask;
> > > >  	unsigned int		pattern;
> > > > +	bool			is_ad778x;
> > > >  };
> > > >  
> > > >  struct ad7780_state {
> > > > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > > > ad_sigma_delta *sigma_delta,
> > > >  	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > > >  		return -EIO;
> > > >  
> > > > -	if (raw_sample & AD7780_GAIN)
> > > > -		st->gain = 1;
> > > > -	else
> > > > -		st->gain = 128;
> > > > +	if (chip_info->is_ad778x) {
> > > > +		if (raw_sample & AD7780_GAIN)
> > > > +			st->gain = 1;
> > > > +		else
> > > > +			st->gain = 128;
> > > > +	}
> > > >  
> > > >  	return 0;
> > > >  }
> > > > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > > > ad7780_chip_info_tbl[] = {
> > > >  		.channel = AD7780_CHANNEL(12, 24),
> > > >  		.pattern = 0x5,
> > > >  		.pattern_mask = 0x7,
> > > > +		.is_ad778x = false,
> > > >  	},
> > > >  	[ID_AD7171] = {
> > > >  		.channel = AD7780_CHANNEL(16, 24),
> > > >  		.pattern = 0x5,
> > > >  		.pattern_mask = 0x7,
> > > > +		.is_ad778x = false,
> > > >  	},
> > > >  	[ID_AD7780] = {
> > > >  		.channel = AD7780_CHANNEL(24, 32),
> > > >  		.pattern = 0x1,
> > > >  		.pattern_mask = 0x3,
> > > > +		.is_ad778x = true,
> > > >  	},
> > > >  	[ID_AD7781] = {
> > > >  		.channel = AD7780_CHANNEL(20, 32),
> > > >  		.pattern = 0x1,
> > > >  		.pattern_mask = 0x3,
> > > > +		.is_ad778x = true,
> > > >  	},
> > > >  };
> > > >      
> > 
> >   

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Tomasz Duszynski]

Hi Giuliano,

Comment inline.

On 11/8/18 2:03 PM, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating
> through a new variable is_ad778x in chip_info.
>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
> Changes in v2:
> 	- Squashed is_ad778x declaration commit with the ad778x checkage
> 	- Changed is_ad778x type to bool
>  
>  drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..9ec2b002539e 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
>  	unsigned int		pattern;
> +	bool			is_ad778x;
>  };
>  
>  struct ad7780_state {
> @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>  	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>  		return -EIO;
>  
> -	if (raw_sample & AD7780_GAIN)
> -		st->gain = 1;
> -	else
> -		st->gain = 128;
> +	if (chip_info->is_ad778x) {
> +		if (raw_sample & AD7780_GAIN)
> +			st->gain = 1;
> +		else
> +			st->gain = 128;
> +	}

Just some random though. Instead of introducing extra level of indentation you
can simply check whether is_ad778x is asserted and simply return.

>  
>  	return 0;
>  }
> @@ -135,21 +138,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>  		.channel = AD7780_CHANNEL(12, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,

Any reason for setting this explicitly? That's going to be false anyway.

>  	},
>  	[ID_AD7171] = {
>  		.channel = AD7780_CHANNEL(16, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,
>  	},
>  	[ID_AD7780] = {
>  		.channel = AD7780_CHANNEL(24, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  	[ID_AD7781] = {
>  		.channel = AD7780_CHANNEL(20, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  };
>  

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Giuliano Augusto Faulin Belinassi]

> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.

I agree that the patch would be smaller if I do that, but is it really
an issue? If yes, then I will update the patch with this change

> Any reason for setting this explicitly? That's going to be false anyway

It seems clearer to me :-)
On Thu, Nov 8, 2018 at 4:26 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> Hi Giuliano,
>
> Comment inline.
>
> On 11/8/18 2:03 PM, Giuliano Belinassi wrote:
> > Only the ad778x have the 'gain' status bit. Check it before updating
> > through a new variable is_ad778x in chip_info.
> >
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > Changes in v2:
> >       - Squashed is_ad778x declaration commit with the ad778x checkage
> >       - Changed is_ad778x type to bool
> >
> >  drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..9ec2b002539e 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> >       struct iio_chan_spec    channel;
> >       unsigned int            pattern_mask;
> >       unsigned int            pattern;
> > +     bool                    is_ad778x;
> >  };
> >
> >  struct ad7780_state {
> > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >           ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> >               return -EIO;
> >
> > -     if (raw_sample & AD7780_GAIN)
> > -             st->gain = 1;
> > -     else
> > -             st->gain = 128;
> > +     if (chip_info->is_ad778x) {
> > +             if (raw_sample & AD7780_GAIN)
> > +                     st->gain = 1;
> > +             else
> > +                     st->gain = 128;
> > +     }
>
> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.
>
> >
> >       return 0;
> >  }
> > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> >               .channel = AD7780_CHANNEL(12, 24),
> >               .pattern = 0x5,
> >               .pattern_mask = 0x7,
> > +             .is_ad778x = false,
>
> Any reason for setting this explicitly? That's going to be false anyway.
>
> >       },
> >       [ID_AD7171] = {
> >               .channel = AD7780_CHANNEL(16, 24),
> >               .pattern = 0x5,
> >               .pattern_mask = 0x7,
> > +             .is_ad778x = false,
> >       },
> >       [ID_AD7780] = {
> >               .channel = AD7780_CHANNEL(24, 32),
> >               .pattern = 0x1,
> >               .pattern_mask = 0x3,
> > +             .is_ad778x = true,
> >       },
> >       [ID_AD7781] = {
> >               .channel = AD7780_CHANNEL(20, 32),
> >               .pattern = 0x1,
> >               .pattern_mask = 0x3,
> > +             .is_ad778x = true,
> >       },
> >  };
> >
>
> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> To post to this group, send email to kernel-usp@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/55b5de74-a607-94b9-8c85-40658e38fbb5%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Jonathan Cameron]

On Fri, 9 Nov 2018 20:15:45 -0200
Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote:

> > Just some random though. Instead of introducing extra level of indentation you
> > can simply check whether is_ad778x is asserted and simply return.  
> 
> I agree that the patch would be smaller if I do that, but is it really
> an issue? If yes, then I will update the patch with this change
> 
> > Any reason for setting this explicitly? That's going to be false anyway  
> 
> It seems clearer to me :-)

Definitely marginal, but not a strong reason either way so I've
applied this as is.  If there were lots of instances of it I would
have agreed with Tomasz (both suggestions were good but Tomasz said,
minor!)

Jonathan

> On Thu, Nov 8, 2018 at 4:26 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >
> > Hi Giuliano,
> >
> > Comment inline.
> >
> > On 11/8/18 2:03 PM, Giuliano Belinassi wrote:  
> > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > through a new variable is_ad778x in chip_info.
> > >
> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > > ---
> > > Changes in v2:
> > >       - Squashed is_ad778x declaration commit with the ad778x checkage
> > >       - Changed is_ad778x type to bool
> > >
> > >  drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> > > index 91e016d534ed..9ec2b002539e 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > >       struct iio_chan_spec    channel;
> > >       unsigned int            pattern_mask;
> > >       unsigned int            pattern;
> > > +     bool                    is_ad778x;
> > >  };
> > >
> > >  struct ad7780_state {
> > > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> > >           ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > >               return -EIO;
> > >
> > > -     if (raw_sample & AD7780_GAIN)
> > > -             st->gain = 1;
> > > -     else
> > > -             st->gain = 128;
> > > +     if (chip_info->is_ad778x) {
> > > +             if (raw_sample & AD7780_GAIN)
> > > +                     st->gain = 1;
> > > +             else
> > > +                     st->gain = 128;
> > > +     }  
> >
> > Just some random though. Instead of introducing extra level of indentation you
> > can simply check whether is_ad778x is asserted and simply return.
> >  
> > >
> > >       return 0;
> > >  }
> > > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > >               .channel = AD7780_CHANNEL(12, 24),
> > >               .pattern = 0x5,
> > >               .pattern_mask = 0x7,
> > > +             .is_ad778x = false,  
> >
> > Any reason for setting this explicitly? That's going to be false anyway.
> >  
> > >       },
> > >       [ID_AD7171] = {
> > >               .channel = AD7780_CHANNEL(16, 24),
> > >               .pattern = 0x5,
> > >               .pattern_mask = 0x7,
> > > +             .is_ad778x = false,
> > >       },
> > >       [ID_AD7780] = {
> > >               .channel = AD7780_CHANNEL(24, 32),
> > >               .pattern = 0x1,
> > >               .pattern_mask = 0x3,
> > > +             .is_ad778x = true,
> > >       },
> > >       [ID_AD7781] = {
> > >               .channel = AD7780_CHANNEL(20, 32),
> > >               .pattern = 0x1,
> > >               .pattern_mask = 0x3,
> > > +             .is_ad778x = true,
> > >       },
> > >  };
> > >  
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> > To post to this group, send email to kernel-usp@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/55b5de74-a607-94b9-8c85-40658e38fbb5%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.  

[PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits

[Giuliano Belinassi]

Previously, all pattern_masks and patterns in the chip_info table were
hardcoded. Now they are generated using the PAT macros, as described in
the datasheets.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v2:
	- Added the PATTERN and PATTERN_MASK macros
	- Update BIT macros alignment to match with PATTERN
	- Generate pattern mask with PATTERN_MASK macros.

drivers/staging/iio/adc/ad7780.c | 40 +++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 9ec2b002539e..ff8e3b2d0efc 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -22,14 +22,22 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/adc/ad_sigma_delta.h>
 
-#define AD7780_RDY	BIT(7)
-#define AD7780_FILTER	BIT(6)
-#define AD7780_ERR	BIT(5)
-#define AD7780_ID1	BIT(4)
-#define AD7780_ID0	BIT(3)
-#define AD7780_GAIN	BIT(2)
-#define AD7780_PAT1	BIT(1)
-#define AD7780_PAT0	BIT(0)
+#define AD7780_RDY		BIT(7)
+#define AD7780_FILTER		BIT(6)
+#define AD7780_ERR		BIT(5)
+#define AD7780_ID1		BIT(4)
+#define AD7780_ID0		BIT(3)
+#define AD7780_GAIN		BIT(2)
+#define AD7780_PAT1		BIT(1)
+#define AD7780_PAT0		BIT(0)
+
+#define AD7780_PATTERN		(AD7780_PAT0)
+#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2		BIT(2)
+
+#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
@@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
 		.channel = AD7780_CHANNEL(12, 24),
-		.pattern = 0x5,
-		.pattern_mask = 0x7,
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
 		.channel = AD7780_CHANNEL(16, 24),
-		.pattern = 0x5,
-		.pattern_mask = 0x7,
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7780] = {
 		.channel = AD7780_CHANNEL(24, 32),
-		.pattern = 0x1,
-		.pattern_mask = 0x3,
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
 		.is_ad778x = true,
 	},
 	[ID_AD7781] = {
 		.channel = AD7780_CHANNEL(20, 32),
-		.pattern = 0x1,
-		.pattern_mask = 0x3,
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
 		.is_ad778x = true,
 	},
 };
-- 
2.19.1

Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits

[Ardelean, Alexandru]

T24gVGh1LCAyMDE4LTExLTA4IGF0IDExOjAzIC0wMjAwLCBHaXVsaWFubyBCZWxpbmFzc2kgd3Jv
dGU6DQo+IFByZXZpb3VzbHksIGFsbCBwYXR0ZXJuX21hc2tzIGFuZCBwYXR0ZXJucyBpbiB0aGUg
Y2hpcF9pbmZvIHRhYmxlIHdlcmUNCj4gaGFyZGNvZGVkLiBOb3cgdGhleSBhcmUgZ2VuZXJhdGVk
IHVzaW5nIHRoZSBQQVQgbWFjcm9zLCBhcyBkZXNjcmliZWQgaW4NCj4gdGhlIGRhdGFzaGVldHMu
DQoNCk9uZSBjb21tZW50IGFib3V0IGluZGVudGF0aW9uL3doaXRlc3BhY2UuDQoNClJlc3QgbG9v
a3MgZ29vZC4NCg0KQWxleA0KDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBHaXVsaWFubyBCZWxpbmFz
c2kgPGdpdWxpYW5vLmJlbGluYXNzaUB1c3AuYnI+DQo+IC0tLQ0KPiBDaGFuZ2VzIGluIHYyOg0K
PiAJLSBBZGRlZCB0aGUgUEFUVEVSTiBhbmQgUEFUVEVSTl9NQVNLIG1hY3Jvcw0KPiAJLSBVcGRh
dGUgQklUIG1hY3JvcyBhbGlnbm1lbnQgdG8gbWF0Y2ggd2l0aCBQQVRURVJODQo+IAktIEdlbmVy
YXRlIHBhdHRlcm4gbWFzayB3aXRoIFBBVFRFUk5fTUFTSyBtYWNyb3MuDQo+IA0KPiBkcml2ZXJz
L3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYyB8IDQwICsrKysrKysrKysrKysrKysrKystLS0tLS0t
LS0tLS0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgMjQgaW5zZXJ0aW9ucygrKSwgMTYgZGVsZXRpb25z
KC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMN
Cj4gYi9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBpbmRleCA5ZWMyYjAwMjUz
OWUuLmZmOGUzYjJkMGVmYyAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMv
YWQ3NzgwLmMNCj4gKysrIGIvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gQEAg
LTIyLDE0ICsyMiwyMiBAQA0KPiAgI2luY2x1ZGUgPGxpbnV4L2lpby9zeXNmcy5oPg0KPiAgI2lu
Y2x1ZGUgPGxpbnV4L2lpby9hZGMvYWRfc2lnbWFfZGVsdGEuaD4NCj4gIA0KPiAtI2RlZmluZSBB
RDc3ODBfUkRZCUJJVCg3KQ0KPiAtI2RlZmluZSBBRDc3ODBfRklMVEVSCUJJVCg2KQ0KPiAtI2Rl
ZmluZSBBRDc3ODBfRVJSCUJJVCg1KQ0KPiAtI2RlZmluZSBBRDc3ODBfSUQxCUJJVCg0KQ0KPiAt
I2RlZmluZSBBRDc3ODBfSUQwCUJJVCgzKQ0KPiAtI2RlZmluZSBBRDc3ODBfR0FJTglCSVQoMikN
Cj4gLSNkZWZpbmUgQUQ3NzgwX1BBVDEJQklUKDEpDQo+IC0jZGVmaW5lIEFENzc4MF9QQVQwCUJJ
VCgwKQ0KPiArI2RlZmluZSBBRDc3ODBfUkRZCQlCSVQoNykNCj4gKyNkZWZpbmUgQUQ3NzgwX0ZJ
TFRFUgkJQklUKDYpDQo+ICsjZGVmaW5lIEFENzc4MF9FUlIJCUJJVCg1KQ0KPiArI2RlZmluZSBB
RDc3ODBfSUQxCQlCSVQoNCkNCj4gKyNkZWZpbmUgQUQ3NzgwX0lEMAkJQklUKDMpDQo+ICsjZGVm
aW5lIEFENzc4MF9HQUlOCQlCSVQoMikNCj4gKyNkZWZpbmUgQUQ3NzgwX1BBVDEJCUJJVCgxKQ0K
PiArI2RlZmluZSBBRDc3ODBfUEFUMAkJQklUKDApDQoNCkNoYW5naW5nIGluZGVudGF0aW9uIGhl
cmUgZG9lc24ndCBhZGQgbXVjaCB2YWx1ZTsgaXQncyBtb3N0bHkNCnBhdGNoL3doaXRlc3BhY2Ug
bm9pc2UuDQoNCldoaWxlIEkgYWdyZWUgdGhhdCBpdCBsb29rcyBuaWNlciB0byBpbmRlbnQgYWxs
IHRoZXNlIHRvIHRoZSBzYW1lIGxldmVsLA0KeW91IGFsc28gbmVlZCB0byB0aGluayBhYm91dCB0
aGUgZmFjdCB0aGF0IHRoZSBrZXJuZWwgZ2l0IHJlcG8gaXMgYWxyZWFkeQ0KcHJldHR5IGJpZyBh
cy1pcywgc28gaXQncyBhIGdvb2QgaWRlYSBpZiBhIHBhdGNoIGFkZHMgYXMgbXVjaCBjb2RlL3Nl
bWFudGljDQp2YWx1ZSBbYXMgcG9zc2libGVdIHdpdGggYXMgbGl0dGxlIGNoYW5nZSBbYXMgcG9z
c2libGVdIGFuZCB3aXRoIGFzIGdvb2Qgb2YNCnJlYWRhYmlsaXR5IFthcyBwb3NzaWJsZV0uDQpb
IEtpbmQgb2Ygc291bmRzIGxpa2UgYSBiYWxhbmNlIGFjdCBiZXR3ZWVuIHRoZSAzIHRoaW5ncyBd
Lg0KDQoNCj4gKw0KPiArI2RlZmluZSBBRDc3ODBfUEFUVEVSTgkJKEFENzc4MF9QQVQwKQ0KPiAr
I2RlZmluZSBBRDc3ODBfUEFUVEVSTl9NQVNLCShBRDc3ODBfUEFUMCB8IEFENzc4MF9QQVQxKQ0K
PiArDQo+ICsjZGVmaW5lIEFENzE3MF9QQVQyCQlCSVQoMikNCj4gKw0KPiArI2RlZmluZSBBRDcx
NzBfUEFUVEVSTgkJKEFENzc4MF9QQVQwIHwgQUQ3MTcwX1BBVDIpDQo+ICsjZGVmaW5lIEFENzE3
MF9QQVRURVJOX01BU0sJKEFENzc4MF9QQVQwIHwgQUQ3NzgwX1BBVDEgfCBBRDcxNzBfUEFUMikN
Cj4gIA0KPiAgc3RydWN0IGFkNzc4MF9jaGlwX2luZm8gew0KPiAgCXN0cnVjdCBpaW9fY2hhbl9z
cGVjCWNoYW5uZWw7DQo+IEBAIC0xMzYsMjYgKzE0NCwyNiBAQCBzdGF0aWMgY29uc3Qgc3RydWN0
IGFkX3NpZ21hX2RlbHRhX2luZm8NCj4gYWQ3NzgwX3NpZ21hX2RlbHRhX2luZm8gPSB7DQo+ICBz
dGF0aWMgY29uc3Qgc3RydWN0IGFkNzc4MF9jaGlwX2luZm8gYWQ3NzgwX2NoaXBfaW5mb190Ymxb
XSA9IHsNCj4gIAlbSURfQUQ3MTcwXSA9IHsNCj4gIAkJLmNoYW5uZWwgPSBBRDc3ODBfQ0hBTk5F
TCgxMiwgMjQpLA0KPiAtCQkucGF0dGVybiA9IDB4NSwNCj4gLQkJLnBhdHRlcm5fbWFzayA9IDB4
NywNCj4gKwkJLnBhdHRlcm4gPSBBRDcxNzBfUEFUVEVSTiwNCj4gKwkJLnBhdHRlcm5fbWFzayA9
IEFENzE3MF9QQVRURVJOX01BU0ssDQo+ICAJCS5pc19hZDc3OHggPSBmYWxzZSwNCj4gIAl9LA0K
PiAgCVtJRF9BRDcxNzFdID0gew0KPiAgCQkuY2hhbm5lbCA9IEFENzc4MF9DSEFOTkVMKDE2LCAy
NCksDQo+IC0JCS5wYXR0ZXJuID0gMHg1LA0KPiAtCQkucGF0dGVybl9tYXNrID0gMHg3LA0KPiAr
CQkucGF0dGVybiA9IEFENzE3MF9QQVRURVJOLA0KPiArCQkucGF0dGVybl9tYXNrID0gQUQ3MTcw
X1BBVFRFUk5fTUFTSywNCj4gIAkJLmlzX2FkNzc4eCA9IGZhbHNlLA0KPiAgCX0sDQo+ICAJW0lE
X0FENzc4MF0gPSB7DQo+ICAJCS5jaGFubmVsID0gQUQ3NzgwX0NIQU5ORUwoMjQsIDMyKSwNCj4g
LQkJLnBhdHRlcm4gPSAweDEsDQo+IC0JCS5wYXR0ZXJuX21hc2sgPSAweDMsDQo+ICsJCS5wYXR0
ZXJuID0gQUQ3NzgwX1BBVFRFUk4sDQo+ICsJCS5wYXR0ZXJuX21hc2sgPSBBRDc3ODBfUEFUVEVS
Tl9NQVNLLA0KPiAgCQkuaXNfYWQ3Nzh4ID0gdHJ1ZSwNCj4gIAl9LA0KPiAgCVtJRF9BRDc3ODFd
ID0gew0KPiAgCQkuY2hhbm5lbCA9IEFENzc4MF9DSEFOTkVMKDIwLCAzMiksDQo+IC0JCS5wYXR0
ZXJuID0gMHgxLA0KPiAtCQkucGF0dGVybl9tYXNrID0gMHgzLA0KPiArCQkucGF0dGVybiA9IEFE
Nzc4MF9QQVRURVJOLA0KPiArCQkucGF0dGVybl9tYXNrID0gQUQ3NzgwX1BBVFRFUk5fTUFTSywN
Cj4gIAkJLmlzX2FkNzc4eCA9IHRydWUsDQo+ICAJfSwNCj4gIH07DQo=

Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits

[Giuliano Augusto Faulin Belinassi]

Hi

>While I agree that it looks nicer to indent all these to the same level,
>you also need to think about the fact that the kernel git repo is already
>pretty big as-is, so it's a good idea if a patch adds as much code/semantic
>value [as possible] with as little change [as possible] and with as good of
>readability [as possible].

Understood. But can't someone else submit another patch fixing these
indentation issues? That would be another commit and more stuff to the
repository.
On Thu, Nov 8, 2018 at 11:51 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
>
> On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > Previously, all pattern_masks and patterns in the chip_info table were
> > hardcoded. Now they are generated using the PAT macros, as described in
> > the datasheets.
>
> One comment about indentation/whitespace.
>
> Rest looks good.
>
> Alex
>
> >
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > Changes in v2:
> >       - Added the PATTERN and PATTERN_MASK macros
> >       - Update BIT macros alignment to match with PATTERN
> >       - Generate pattern mask with PATTERN_MASK macros.
> >
> > drivers/staging/iio/adc/ad7780.c | 40 +++++++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 9ec2b002539e..ff8e3b2d0efc 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -22,14 +22,22 @@
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/adc/ad_sigma_delta.h>
> >
> > -#define AD7780_RDY   BIT(7)
> > -#define AD7780_FILTER        BIT(6)
> > -#define AD7780_ERR   BIT(5)
> > -#define AD7780_ID1   BIT(4)
> > -#define AD7780_ID0   BIT(3)
> > -#define AD7780_GAIN  BIT(2)
> > -#define AD7780_PAT1  BIT(1)
> > -#define AD7780_PAT0  BIT(0)
> > +#define AD7780_RDY           BIT(7)
> > +#define AD7780_FILTER                BIT(6)
> > +#define AD7780_ERR           BIT(5)
> > +#define AD7780_ID1           BIT(4)
> > +#define AD7780_ID0           BIT(3)
> > +#define AD7780_GAIN          BIT(2)
> > +#define AD7780_PAT1          BIT(1)
> > +#define AD7780_PAT0          BIT(0)
>
> Changing indentation here doesn't add much value; it's mostly
> patch/whitespace noise.
>
> While I agree that it looks nicer to indent all these to the same level,
> you also need to think about the fact that the kernel git repo is already
> pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> value [as possible] with as little change [as possible] and with as good of
> readability [as possible].
> [ Kind of sounds like a balance act between the 3 things ].
>
>
> > +
> > +#define AD7780_PATTERN               (AD7780_PAT0)
> > +#define AD7780_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1)
> > +
> > +#define AD7170_PAT2          BIT(2)
> > +
> > +#define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
> > +#define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> >
> >  struct ad7780_chip_info {
> >       struct iio_chan_spec    channel;
> > @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> >       [ID_AD7170] = {
> >               .channel = AD7780_CHANNEL(12, 24),
> > -             .pattern = 0x5,
> > -             .pattern_mask = 0x7,
> > +             .pattern = AD7170_PATTERN,
> > +             .pattern_mask = AD7170_PATTERN_MASK,
> >               .is_ad778x = false,
> >       },
> >       [ID_AD7171] = {
> >               .channel = AD7780_CHANNEL(16, 24),
> > -             .pattern = 0x5,
> > -             .pattern_mask = 0x7,
> > +             .pattern = AD7170_PATTERN,
> > +             .pattern_mask = AD7170_PATTERN_MASK,
> >               .is_ad778x = false,
> >       },
> >       [ID_AD7780] = {
> >               .channel = AD7780_CHANNEL(24, 32),
> > -             .pattern = 0x1,
> > -             .pattern_mask = 0x3,
> > +             .pattern = AD7780_PATTERN,
> > +             .pattern_mask = AD7780_PATTERN_MASK,
> >               .is_ad778x = true,
> >       },
> >       [ID_AD7781] = {
> >               .channel = AD7780_CHANNEL(20, 32),
> > -             .pattern = 0x1,
> > -             .pattern_mask = 0x3,
> > +             .pattern = AD7780_PATTERN,
> > +             .pattern_mask = AD7780_PATTERN_MASK,
> >               .is_ad778x = true,
> >       },
> >  };
>
> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> To post to this group, send email to kernel-usp@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel%40analog.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits

[Jonathan Cameron]

On Fri, 9 Nov 2018 20:18:58 -0200
Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote:

> Hi
> 
> >While I agree that it looks nicer to indent all these to the same level,
> >you also need to think about the fact that the kernel git repo is already
> >pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> >value [as possible] with as little change [as possible] and with as good of
> >readability [as possible].  
> 
> Understood. But can't someone else submit another patch fixing these
> indentation issues? That would be another commit and more stuff to the
> repository.

Separating real changes from white space changes is much more important
from a reviewability point of view.  This patch is small enough that it
doesn't 'really matter' but I would have preferred it as a realignment patch
and a follow up patch making the real change.

Anyhow, it doesn't matter that much for such a small case, so applied to
the togreg branch of iio.git and pushed out as testing for the autobuilders
to play with it.

Thanks,

Jonathan


> On Thu, Nov 8, 2018 at 11:51 AM Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> wrote:
> >
> > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:  
> > > Previously, all pattern_masks and patterns in the chip_info table were
> > > hardcoded. Now they are generated using the PAT macros, as described in
> > > the datasheets.  
> >
> > One comment about indentation/whitespace.
> >
> > Rest looks good.
> >
> > Alex
> >  
> > >
> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > > ---
> > > Changes in v2:
> > >       - Added the PATTERN and PATTERN_MASK macros
> > >       - Update BIT macros alignment to match with PATTERN
> > >       - Generate pattern mask with PATTERN_MASK macros.
> > >
> > > drivers/staging/iio/adc/ad7780.c | 40 +++++++++++++++++++-------------
> > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index 9ec2b002539e..ff8e3b2d0efc 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -22,14 +22,22 @@
> > >  #include <linux/iio/sysfs.h>
> > >  #include <linux/iio/adc/ad_sigma_delta.h>
> > >
> > > -#define AD7780_RDY   BIT(7)
> > > -#define AD7780_FILTER        BIT(6)
> > > -#define AD7780_ERR   BIT(5)
> > > -#define AD7780_ID1   BIT(4)
> > > -#define AD7780_ID0   BIT(3)
> > > -#define AD7780_GAIN  BIT(2)
> > > -#define AD7780_PAT1  BIT(1)
> > > -#define AD7780_PAT0  BIT(0)
> > > +#define AD7780_RDY           BIT(7)
> > > +#define AD7780_FILTER                BIT(6)
> > > +#define AD7780_ERR           BIT(5)
> > > +#define AD7780_ID1           BIT(4)
> > > +#define AD7780_ID0           BIT(3)
> > > +#define AD7780_GAIN          BIT(2)
> > > +#define AD7780_PAT1          BIT(1)
> > > +#define AD7780_PAT0          BIT(0)  
> >
> > Changing indentation here doesn't add much value; it's mostly
> > patch/whitespace noise.
> >
> > While I agree that it looks nicer to indent all these to the same level,
> > you also need to think about the fact that the kernel git repo is already
> > pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> > value [as possible] with as little change [as possible] and with as good of
> > readability [as possible].
> > [ Kind of sounds like a balance act between the 3 things ].
> >
> >  
> > > +
> > > +#define AD7780_PATTERN               (AD7780_PAT0)
> > > +#define AD7780_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1)
> > > +
> > > +#define AD7170_PAT2          BIT(2)
> > > +
> > > +#define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
> > > +#define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > >
> > >  struct ad7780_chip_info {
> > >       struct iio_chan_spec    channel;
> > > @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> > > ad7780_sigma_delta_info = {
> > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > >       [ID_AD7170] = {
> > >               .channel = AD7780_CHANNEL(12, 24),
> > > -             .pattern = 0x5,
> > > -             .pattern_mask = 0x7,
> > > +             .pattern = AD7170_PATTERN,
> > > +             .pattern_mask = AD7170_PATTERN_MASK,
> > >               .is_ad778x = false,
> > >       },
> > >       [ID_AD7171] = {
> > >               .channel = AD7780_CHANNEL(16, 24),
> > > -             .pattern = 0x5,
> > > -             .pattern_mask = 0x7,
> > > +             .pattern = AD7170_PATTERN,
> > > +             .pattern_mask = AD7170_PATTERN_MASK,
> > >               .is_ad778x = false,
> > >       },
> > >       [ID_AD7780] = {
> > >               .channel = AD7780_CHANNEL(24, 32),
> > > -             .pattern = 0x1,
> > > -             .pattern_mask = 0x3,
> > > +             .pattern = AD7780_PATTERN,
> > > +             .pattern_mask = AD7780_PATTERN_MASK,
> > >               .is_ad778x = true,
> > >       },
> > >       [ID_AD7781] = {
> > >               .channel = AD7780_CHANNEL(20, 32),
> > > -             .pattern = 0x1,
> > > -             .pattern_mask = 0x3,
> > > +             .pattern = AD7780_PATTERN,
> > > +             .pattern_mask = AD7780_PATTERN_MASK,
> > >               .is_ad778x = true,
> > >       },
> > >  };  
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> > To post to this group, send email to kernel-usp@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel%40analog.com.
> > For more options, visit https://groups.google.com/d/optout.