Skip to contentSkip to conversation

pattern generation and gain update

PATCH In review Giuliano Belinassi 5 participants 13 messages Nov 8, 2018 – Nov 16, 2018

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io
lore
Share & tools
lore

Giuliano Belinassi

author
#0 lore raw

v2 cover pattern generation and gain update

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(-)
Signature · 1 line
2.19.1
author
#1 lore raw

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

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 <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +11 -4
index 91e016d534ed..9ec2b002539e 100644
@@ -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,
 	},
 };
Signature · 1 line
2.19.1
#2 lore raw

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

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==

maintainer
#3 lore raw

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

Quoted 6 lines · Alexandru
Quoted 3 lines · Giuliano Belinassi

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

Quoted 70 lines

Alex

Quoted 67 lines
Signed-off-by Giuliano Belinassi <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +7 -4
index 91e016d534ed..9ec2b002539e 100644
@@ -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;

}

drivers/staging/iio/adc/ad7780.c +4 -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,
 	},
 };
Acked-by
#4 lore raw

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

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==

maintainer
#5 lore raw

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

Quoted 11 lines · Alexandru
Quoted 10 lines · Jonathan Cameron
Quoted 6 lines · Alexandru
Quoted 3 lines · Giuliano Belinassi

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

Looks good.

Quoted 8 lines
Quoted 6 lines

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 <[email protected]>

I haven't pushed out togreg yet so can still rebase.

Added. Thanks,

Jonathan

Quoted 78 lines

// Will remember that next time :)

Thanks
Alex

Quoted 72 lines
Quoted 70 lines

Alex

Quoted 67 lines
Signed-off-by Giuliano Belinassi <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +7 -4
index 91e016d534ed..9ec2b002539e 100644
@@ -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;

}

drivers/staging/iio/adc/ad7780.c +4 -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,
 	},
 };
#6 lore raw

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

In reply to Giuliano Belinassi

Hi Giuliano,

Comment inline.

Quoted 28 lines · 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 <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +1 -0
index 91e016d534ed..9ec2b002539e 100644
@@ -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;

drivers/staging/iio/adc/ad7780.c static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, · line 122

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +6 -4
-	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.

drivers/staging/iio/adc/ad7780.c static const struct ad7780_chip_info ad7780_chip_info_tbl[] = { · line 141

Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +1 -0
 	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.

Quoted 21 lines
drivers/staging/iio/adc/ad7780.c +3 -0
 	},
 	[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,
 	},
 };
#7 lore raw

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

Quoted 2 lines

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

Quoted 1 line

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

It seems clearer to me :-)

Quoted 87 lines · Tomasz Duszynski

Hi Giuliano,

Comment inline.

Quoted 28 lines · 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 <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +1 -0
index 91e016d534ed..9ec2b002539e 100644
@@ -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;

drivers/staging/iio/adc/ad7780.c static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, · line 122

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +6 -4
-     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.

drivers/staging/iio/adc/ad7780.c static const struct ad7780_chip_info ad7780_chip_info_tbl[] = { · line 141

Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +1 -0
      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.

Quoted 11 lines
drivers/staging/iio/adc/ad7780.c +1 -0
      },
      [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,

drivers/staging/iio/adc/ad7780.c static const struct ad7780_chip_info ad7780_chip_info_tbl[] = { · line 162

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+             .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 [email protected]. To post to this group, send email to [email protected].
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.

maintainer
#8 lore raw

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

Quoted 9 lines · On Fri, 9 Nov 2018 20:15:45 -0200 Giuliano Augusto Faulin Belinassi…
Quoted 2 lines

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

Quoted 1 line

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

Quoted 88 lines · Tomasz Duszynski
Quoted 87 lines

Hi Giuliano,

Comment inline.

Quoted 28 lines · 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 <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +1 -0
index 91e016d534ed..9ec2b002539e 100644
@@ -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;

drivers/staging/iio/adc/ad7780.c static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, · line 122

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +6 -4
-     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.

drivers/staging/iio/adc/ad7780.c static const struct ad7780_chip_info ad7780_chip_info_tbl[] = { · line 141

Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +1 -0
      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.

Quoted 11 lines
drivers/staging/iio/adc/ad7780.c +1 -0
      },
      [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,

drivers/staging/iio/adc/ad7780.c static const struct ad7780_chip_info ad7780_chip_info_tbl[] = { · line 162

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+             .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 [email protected]. To post to this group, send email to [email protected].
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.

author
#9 lore raw

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

In reply to 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 <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +24 -16
index 9ec2b002539e..ff8e3b2d0efc 100644
@@ -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,
 	},
 };
Signature · 1 line
2.19.1
#10 lore raw

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

Quoted 3 lines · 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.

One comment about indentation/whitespace.

Rest looks good.

Alex

Quoted 26 lines
Signed-off-by Giuliano Belinassi <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +0 -6
index 9ec2b002539e..ff8e3b2d0efc 100644
@@ -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)

drivers/staging/iio/adc/ad7780.c line 32

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +8 -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 ].

Quoted 47 lines
drivers/staging/iio/adc/ad7780.c +16 -8
+
+#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,
 	},

};

#11 lore raw

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

Hi

Quoted 5 lines

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.

Quoted 114 lines · Alexandru
Quoted 3 lines · 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.

One comment about indentation/whitespace.

Rest looks good.

Alex

Quoted 26 lines
Signed-off-by Giuliano Belinassi <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +0 -6
index 9ec2b002539e..ff8e3b2d0efc 100644
@@ -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)

drivers/staging/iio/adc/ad7780.c line 32

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +8 -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 ].

Quoted 46 lines
drivers/staging/iio/adc/ad7780.c +16 -8
+
+#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,
      },
Quoted 1 line

};

--
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 [email protected]. To post to this group, send email to [email protected].
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.

maintainer
#12 lore raw

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

Quoted 11 lines · On Fri, 9 Nov 2018 20:18:58 -0200 Giuliano Augusto Faulin Belinassi…

Hi

Quoted 5 lines

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

Quoted 116 lines
Quoted 114 lines · Alexandru
Quoted 3 lines · 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.

One comment about indentation/whitespace.

Rest looks good.

Alex

Quoted 26 lines
Signed-off-by Giuliano Belinassi <[email protected]>

---
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(-)
drivers/staging/iio/adc/ad7780.c +0 -6
index 9ec2b002539e..ff8e3b2d0efc 100644
@@ -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)

drivers/staging/iio/adc/ad7780.c line 32

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +8 -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 ].

Quoted 46 lines
drivers/staging/iio/adc/ad7780.c +16 -8
+
+#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,
      },
Quoted 1 line

};

--
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 [email protected]. To post to this group, send email to [email protected].
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.