On 01/04/2024 22:45, David Lechner wrote: > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@xxxxxxxxxx> wrote: >> >> From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx> >> ... >> #define AD7175_2_ID 0x0cd0 >> #define AD7172_4_ID 0x2050 >> #define AD7173_ID 0x30d0 >> +#define AD4111_ID 0x30d0 >> +#define AD4112_ID 0x30d0 >> +#define AD4114_ID 0x30d0 > > It might make it a bit more obvious that not all chips have a unique > ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather > than introducing multiple macros with the same value. > > Or leave it as AD7173_ID to keep it short and add a comment where it > is used with 411x chips in ad7173_device_info[]. > Sure >> +#define AD4116_ID 0x34d0 >> +#define AD4115_ID 0x38d0 >> #define AD7175_8_ID 0x3cd0 >> #define AD7177_ID 0x4fd0 >> #define AD7173_ID_MASK GENMASK(15, 4) ... >> struct ad7173_device_info { >> const unsigned int *sinc5_data_rates; >> unsigned int num_sinc5_data_rates; >> unsigned int odr_start_value; >> + unsigned int num_inputs_with_divider; >> unsigned int num_channels; >> unsigned int num_configs; >> unsigned int num_inputs; > > Probably a good idea to change num_inputs to num_voltage_inputs so it > isn't confused with the total number of inputs. > > Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider. > > Also could use a comment to make it clear if num_voltage_inputs > includes num_voltage_inputs_with_divider or not. And that it doesn't > include VINCOM. > Alright for these 3 statements above. > Probably also need some flag here to differentiate ADCINxx voltage > inputs on AD4116. > That is the purpose of num_inputs_with_divider. Mangled some changes when splitting into individual patches. Will include in V2. " if (ain[1] == AD411X_VCOM_INPUT && ain[0] >= st->info->num_inputs_with_divider) return dev_err_probe(dev, -EINVAL, "VCOM must be paired with inputs having divider.\n"); " ... >> >> +static unsigned int ad4111_current_channel_config[] = { >> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8, >> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9, >> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA, >> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B, >> +}; > > As mentioned in the DT bindings review, it would make more sense to > just use the datasheet numbers for the current input channels in the > diff-channels DT property, then we don't need this lookup table. > Yet, the datasheet does not specify the numbers, just a single bitfield for each pair. It is too much of a churn to need to decode that bitfield into individual values when the user just wants to select a single pair. ... >> + case IIO_CURRENT: >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); >> + *val /= AD4111_SHUNT_RESISTOR_OHM; >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > > Static analysis tools like to complain about using bool as int. > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway. > Maybe it does not apply here, but i followed this advice: Andy Shevchenko V1 of AD7173 (named initially ad717x) " > + return (bool)(value & mask); This is weird. You have int which you get from bool, wouldn't be better to use !!(...) as other GPIO drivers do? " >> + case IIO_CURRENT: >> *val = -BIT(chan->scan_type.realbits - 1); > > Expecting a special case here, at least when ADCIN15 is configured for > pseudo-differential inputs. > And what configuration would that be? The only configurable part is the BI_UNIPOLARx bit in the channel register, which is addressed here. There seems to be a confusion similar to what we had with single-ended channels. The ADC is differential. Pseudo-differential in this datasheet just means that you wired a fixed voltage(higher than 0) to the negative analog input. Which you can also do on the other inputs with a divider. ... >> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]); >> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { >> + chan->type = IIO_CURRENT; >> + chan->channel = ain[0]; >> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; >> + } else { >> + chan->channel = ain[0]; >> + chan->channel2 = ain[1]; >> + chan->differential = true; > > Expecting chan->differential = false when ADCIN15 is configured for > pseudo-differential inputs. > > Also, perhaps missed in previous reviews, I would expect > chan->differential = false when channels are used as single-ended. > Why? Also, how would one detect if you are using single-ended channels? The ADC is still differential. Single ended is represented as connecting AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate. In the IIO framework the only difference this makes is in the naming of the channel: voltage0-voltage1 vs just voltage0 All channels are differential. Pseudo differential: still differential.