<snip>

quoted

quoted

+static int iadc_poll_wait_eoc(struct iadc_chip *iadc, int interval_us)
+{
+	int ret, count, retry;
+	u8 sta1;
+
+	retry = interval_us / IADC_CONV_TIME_MIN_US;
+
+	for (count = 0; count < retry; count++) {
+		ret = iadc_read(iadc, IADC_STATUS1, &sta1);
+		if (ret < 0)
+			return ret;
+
+		sta1 &= IADC_STATUS1_REQ_STS_EOC_MASK;
+		if (sta1 == IADC_STATUS1_EOC)
+			return 0;
+
+		usleep_range(IADC_CONV_TIME_MIN_US, IADC_CONV_TIME_MAX_US);
+	}
+
+	iadc_status_show(iadc);

What is this for?  Left over from debugging the driver?

This should not generally happen, but will be useful to see what was 
ADC configuration and statuses, when conversion time outs.

Hmm. Fine I guess, just a lot of code for an unexpected condition
Should probably be as dev_err if it occurs.

quoted

quoted

+
+	return -ETIMEDOUT;
+}
+

<snip>

quoted

quoted

+static int iadc_read_raw(struct iio_dev *indio_dev,
+			 struct iio_chan_spec const *chan,
+			 int *val, int *val2, long mask)
+{
+	struct iadc_chip *iadc = iio_priv(indio_dev);
+	long rsense_ua, rsense_uv, rsense_raw;
+	int ret = -EINVAL, negative;
+	u16 adc_raw;
+
+	mutex_lock(&iadc->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = iadc_do_conversion(iadc, chan->channel, &adc_raw);
+		if (ret < 0)
+			goto exit;
+
+		rsense_raw = adc_raw - iadc->offset_raw;
+
+		rsense_uv = (rsense_raw * IADC_REF_GAIN_MICRO_VOLTS);
+		rsense_uv /= iadc->gain_raw - iadc->offset_raw;
+
+		negative = 0;
+		if (rsense_uv < 0) {
+			negative = 1;
+			rsense_uv = -rsense_uv;
+		}
+
+		rsense_ua = rsense_uv;
+
+		do_div(rsense_ua, iadc->rsense);
+
+		if (negative)
+			rsense_ua = -rsense_ua;
+
+		dev_dbg(iadc->dev, "offset %d, gain %d, adc %d, V=%ld,uV, I=%ld,uA\n",
+			iadc->offset_raw, iadc->gain_raw, adc_raw,
+			rsense_uv, rsense_ua);
+
+		*val = rsense_ua;

Given the naming this seems unlikely to be in millamps?

Yes, it is micro amps. I was unable to find what should be correct
dimension. There is nothing in sysfs-bus-iio or bindings/iio.

There is now (in the togreg branch of iio.git;)  Wasn't until very recently though.
Somehow it got lost in the move out of staging...  The scaling comes form hwmon,
With hindsight we should probably have ditched it in favour of using Volts and Amps
as our base scaling.  Less confusing, but too late now!

quoted

quoted

+		ret = IIO_VAL_INT;
+		break;
+	default:
+		break;
+	}
+
+exit:
+	mutex_unlock(&iadc->lock);
+
+	return ret;
+}
+

<snip>

quoted

quoted

+
+static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
+{
+	unsigned int trim_val;
+	u8  rsense, const_rds;
+	int ret, negative;
+	u32 trim_type;
+
+	ret = of_property_read_u32(node, "qcom,rsense", &iadc->rsense);
+	if (!ret) {
+		iadc->ext_sense = true;
+		return 0;
+	}

User specify external sense resistor value, which means external
channel should be used.

quoted

quoted

+
+	ret = iadc_read(iadc, IADC_NOMINAL_RSENSE, &rsense);
+	if (ret < 0)
+		return ret;
+
+	negative = 0;
+	if (rsense & IADC_NOMINAL_RSENSE_SIGN_MASK)
+		negative = 1;

I'm a little confused.  The docs say that rsense is a resistor value in
ohms (u32).  Why does bit 7 allow encode other information?

External sense resistor value not specified, driver will use internal
ADC channel and internal sense resistor, so read what was trimmed
during manufacturing process. Sense register value is read from
register. If bit 7 of value is set value in register is negative.
Register itself didn't contain value in ohms, but difference between
desired and actual value.

Ah got it.  Thanks for the explanation.
Perhaps ensure that iadc->rsense is clearly in ohms wherease this local
rsense is the register value (rename one of them!)

quoted

quoted

+
+	rsense &= ~IADC_NOMINAL_RSENSE_SIGN_MASK;
+
+	iadc->rsense = IADC_INTERNAL_RSENSE_OHMS;
+	if (negative)
+		iadc->rsense -=	rsense * IADC_RSENSE_OHMS_PER_BIT;
+	else
+		iadc->rsense +=	rsense * IADC_RSENSE_OHMS_PER_BIT;

internal rsense = 10000000 + sign * 15625 * value;

quoted

quoted

+
+	if (rsense < IADC_TRIM2_FULLSCALE)
+		return 0;
+	/*
+	 * Trim register is "saturated", check for specific trim value
+	 * based on manufacturer and RDS constant
+	 */
+	ret = of_property_read_u32(node, "qcom,rds-trim", &trim_type);
+	if (ret)
+		return 0;
+
+	ret = regmap_read(iadc->regmap, iadc->trim_rds, &trim_val);
+	if (ret < 0)
+		return ret;
+
+	const_rds = trim_val & IADC_TRIM_CNST_RDS_MASK;
+
+	dev_dbg(iadc->dev, "factory %d trim type %d rsense %d const %d\n",
+		iadc->factory, trim_type, rsense, const_rds);
+
+	switch (trim_type) {
+	case 0:
+		if (const_rds == 2)
+			iadc->rsense = IADC_RSENSE_DEFAULT_VALUE;

So this is overwriting rsense if properties are obeyed.  Would it not
make sense to do this before using the rsense value above (if these
constraints are not met?)

We are coming here only if value trimmed into IADC_NOMINAL_RSENSE
register is saturated i.e, u8 value is 127, bit 7 is sign bit. Which 
means that internal resistor is far from desired value. And this 
depends on the factory used for manufacturing the chip. The whole 
logic here is simplified version, I believe, on the downstream 
implementation, which could be found here[1].

[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/hwmon/qpnp-adc-current.c?h=msm-3.10

quoted

quoted

+		break;
+	case 1:
+		if (const_rds >= 2) {
+			iadc->rsense = IADC_RSENSE_DEFAULT_VALUE;
+		} else if (const_rds < 2) {
+			if (iadc->factory == IADC_FACTORY_GF)
+				iadc->rsense = IADC_RSENSE_DEFAULT_GF;
+			else if (iadc->factory == IADC_FACTORY_SMIC)
+				iadc->rsense = IADC_RSENSE_DEFAULT_SMIC;
+		}
+		break;
+	case 2:
+		if (const_rds > 0 && const_rds <= 2)
+			iadc->rsense = IADC_RSENSE_DEFAULT_VALUE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int iadc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct iio_chan_spec *iio_chan;
+	struct iio_dev *indio_dev;
+	struct iadc_chip *iadc;
+	struct resource *res;
+	struct regmap *regmap;
+	int ret, irq_eoc;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*iadc));
+	if (!indio_dev)
+		return -ENOMEM;

Spinning the order of the regmap get and iio_device_alloc would perhaps
give a cleaner result as you could then fill iadc->regmap directly...
(marginal!)

No local variables, right? :-)

Well only when they actually make the code cleaner!

Thank you for review.

You are welcome

Jonathan

Regards,
Ivan

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html