On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov [off-list ref] wrote:

Hi Jonathan,

Thanks for the review!

On 09/13/2014 08:27 PM, Jonathan Cameron wrote:

quoted

On 13/09/14 00:27, Hartmut Knaack wrote:

quoted

Stanimir Varbanov schrieb, Am 11.09.2014 17:13:

quoted

The voltage ADC is peripheral of Qualcomm SPMI PMIC chips. It has
15bits resolution and register space inside PMIC accessible across
SPMI bus.

The vadc driver registers itself through IIO interface.

Looks already pretty good. Things you should consider in regard of

common coding style are to use the variable name ret instead of rc,
since it is used in almost all adc drivers and thus makes reviewing a
bit easier. Besides that, you seem to use unsigned as well as unsigned
int, so to be consistent, please stick to one of them. Other comments
in line.

quoted

A few additional comments from me.  My biggest question is whether
you are actually making life difficult for yourself by having
vadc_channels and vadc->channels (don't like the similar naming btw!)
in different orders.  I think you can move the ordering into the

device

quoted

tree reading code rather than doing it in lots of other places. 

Hence

quoted

rather than an order based on the device tree description, put the
data into a fixed ofer in vadc->channels.

Entirely possible I'm missing something though :)

quoted

quoted

Signed-off-by: Stanimir Varbanov <redacted>
Signed-off-by: Ivan T. Ivanov <redacted>
---
 drivers/iio/adc/Kconfig                       |   11 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/qcom-spmi-vadc.c              |  999

+++++++++++++++++++++++++

quoted

quoted

quoted

 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h |  119 +++
 4 files changed, 1130 insertions(+), 0 deletions(-)
 create mode 100644 drivers/iio/adc/qcom-spmi-vadc.c
 create mode 100644 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h

<snip>

quoted

quoted

quoted

+
+static int

Don't wrap line here.

quoted

+vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec

const *chan,

quoted

quoted

quoted

+	      int *val, int *val2, long mask)
+{
+	struct vadc_priv *vadc = iio_priv(indio_dev);
+	struct vadc_channel *vchan;
+	struct vadc_result result;
+	int rc;
+

It is a bit of a pitty we can't avoid this lookup. Normally I'd

suggest

quoted

putting an index in chan->address but you've already used that.
The purpose of this is to (I think) allow you to have the private
data stored in a random order... What is the benefit of doing that?
(see various comments elsewhere)

So the vadc_channels array describe all possible vadc channels for all
supported PMICs from this driver. On the other side vadc->channels
pointer should contain only the channels described in DT.

Thus we need a below function to check is the current channel is active
for the current DT (current PMIC version). This is because we have in
vadc_channels the full set of channels but not every supported PMIC
have
support for them.

I agree that this peace of code is not so clear. So I will try to
rework
this and register to the IIO core only those channels that have channel
descriptions in DT.

Cool

Also I wonder can I use iio_chan_spec::address field as a pointer to
private structure with vadc channel properties like decimation,
prescale
etc. got from DT or the default values.

Yes or an index into an array of them perhaps?

quoted

quoted

quoted

+	vchan = vadc_find_channel(vadc, chan->channel);
+	if (!vchan)
+		return -EINVAL;
+
+	if (!vadc->is_ref_measured) {
+		rc = vadc_measure_reference_points(vadc);
+		if (rc)
+			return rc;
+
+		vadc->is_ref_measured = true;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		rc = vadc_do_conversion(vadc, vchan, &result.adc_code);
+		if (rc)
+			return rc;
+
+		vadc_calibrate(vadc, vchan, &result);
+
+		*val = result.physical;

I'm a little suspicious here.  Are the resulting values in milivolts

for

quoted

all the channels?  Very handy if so, but seems a little unlikely with

15 bit

quoted

ADC that you'd have no part of greater accuracy than a milivolt.

In fact *val is in microvolts. What is the expected unit from IIO ADC
users?

See Documentation/ABI/sysfs-bus-iio
Millivolts I think... We copied hwmon where possible.

quoted

quoted

quoted

+		rc = IIO_VAL_INT;

return IIO_VAL_INT;

quoted

+		break;
+	default:
+		rc = -EINVAL;
+		break;

Drop default case, or leave empty.

quoted

+	}
+
+	return rc;

return -EINVAL;

quoted

+}
+
+static const struct iio_info vadc_info = {
+	.read_raw = vadc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+#define VADC_CHAN(_id, _pre)						\
+	[VADC_##_id] = {						\
+		.type = IIO_VOLTAGE,					\

A few of the below look to be temp sensors.  If they are hardwired
in some way to this functionality (i.e. is it on chip) then it might

be

quoted

nice to reflect this in the channel type.

There are a dedicated channels to measure temperature. Those channels
have connected thermistor but I don't think it is on chip. So the
returned adc code is in microvolts and we have a translation table to
convert measured voltage to miliCelsius. I thought that if I mark the
channel type as IIO_TEMP then the user would expect the returned units
to be miliCelsius. If that assumption is not correct I can change the
type of those channels.

You are correct. Leave them as voltages.
If they were hardwired inside the chip it would be different.

quoted

quoted

quoted

+		.indexed = 1,						\
+		.channel = VADC_##_id,					\
+		.address = _pre,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),	\
+		.datasheet_name = __stringify(VADC_##_id),		\
+		.scan_type = {						\
+			.sign		= 's',				\
+			.realbits	= 15,				\
+			.storagebits	= 16,				\
+			.endianness	= IIO_CPU,			\
+		},							\
+	},
+
+static const struct iio_chan_spec vadc_channels[] = {
+	VADC_CHAN(USBIN, 4)				/* 0x00 */
+	VADC_CHAN(DCIN, 4)
+	VADC_CHAN(VCHG_SNS, 3)
+	VADC_CHAN(SPARE1_03, 1)
+	VADC_CHAN(USB_ID_MV, 1)
+	VADC_CHAN(VCOIN, 1)
+	VADC_CHAN(VBAT_SNS, 1)
+	VADC_CHAN(VSYS, 1)
+	VADC_CHAN(DIE_TEMP, 0)
+	VADC_CHAN(REF_625MV, 0)
+	VADC_CHAN(REF_1250MV, 0)
+	VADC_CHAN(CHG_TEMP, 0)
+	VADC_CHAN(SPARE1, 0)
+	VADC_CHAN(SPARE2, 0)
+	VADC_CHAN(GND_REF, 0)
+	VADC_CHAN(VDD_VADC, 0)				/* 0x0f */
+
+	VADC_CHAN(P_MUX1_1_1, 0)			/* 0x10 */
+	VADC_CHAN(P_MUX2_1_1, 0)
+	VADC_CHAN(P_MUX3_1_1, 0)
+	VADC_CHAN(P_MUX4_1_1, 0)
+	VADC_CHAN(P_MUX5_1_1, 0)
+	VADC_CHAN(P_MUX6_1_1, 0)
+	VADC_CHAN(P_MUX7_1_1, 0)
+	VADC_CHAN(P_MUX8_1_1, 0)
+	VADC_CHAN(P_MUX9_1_1, 0)
+	VADC_CHAN(P_MUX10_1_1, 0)
+	VADC_CHAN(P_MUX11_1_1, 0)
+	VADC_CHAN(P_MUX12_1_1, 0)
+	VADC_CHAN(P_MUX13_1_1, 0)
+	VADC_CHAN(P_MUX14_1_1, 0)
+	VADC_CHAN(P_MUX15_1_1, 0)
+	VADC_CHAN(P_MUX16_1_1, 0)			/* 0x1f */
+
+	VADC_CHAN(P_MUX1_1_3, 1)			/* 0x20 */
+	VADC_CHAN(P_MUX2_1_3, 1)
+	VADC_CHAN(P_MUX3_1_3, 1)
+	VADC_CHAN(P_MUX4_1_3, 1)
+	VADC_CHAN(P_MUX5_1_3, 1)
+	VADC_CHAN(P_MUX6_1_3, 1)
+	VADC_CHAN(P_MUX7_1_3, 1)
+	VADC_CHAN(P_MUX8_1_3, 1)
+	VADC_CHAN(P_MUX9_1_3, 1)
+	VADC_CHAN(P_MUX10_1_3, 1)
+	VADC_CHAN(P_MUX11_1_3, 1)
+	VADC_CHAN(P_MUX12_1_3, 1)
+	VADC_CHAN(P_MUX13_1_3, 1)
+	VADC_CHAN(P_MUX14_1_3, 1)
+	VADC_CHAN(P_MUX15_1_3, 1)
+	VADC_CHAN(P_MUX16_1_3, 1)			/* 0x2f */
+
+	VADC_CHAN(LR_MUX1_BAT_THERM, 0)			/* 0x30 */
+	VADC_CHAN(LR_MUX2_BAT_ID, 0)
+	VADC_CHAN(LR_MUX3_XO_THERM, 0)
+	VADC_CHAN(LR_MUX4_AMUX_THM1, 0)
+	VADC_CHAN(LR_MUX5_AMUX_THM2, 0)
+	VADC_CHAN(LR_MUX6_AMUX_THM3, 0)
+	VADC_CHAN(LR_MUX7_HW_ID, 0)
+	VADC_CHAN(LR_MUX8_AMUX_THM4, 0)
+	VADC_CHAN(LR_MUX9_AMUX_THM5, 0)
+	VADC_CHAN(LR_MUX10_USB_ID, 0)
+	VADC_CHAN(AMUX_PU1, 0)
+	VADC_CHAN(AMUX_PU2, 0)
+	VADC_CHAN(LR_MUX3_BUF_XO_THERM, 0)		/* 0x3c */
+
+	VADC_CHAN(LR_MUX1_PU1_BAT_THERM, 0)		/* 0x70 */
+	VADC_CHAN(LR_MUX2_PU1_BAT_ID, 0)
+	VADC_CHAN(LR_MUX3_PU1_XO_THERM, 0)
+	VADC_CHAN(LR_MUX4_PU1_AMUX_THM1, 0)
+	VADC_CHAN(LR_MUX5_PU1_AMUX_THM2, 0)
+	VADC_CHAN(LR_MUX6_PU1_AMUX_THM3, 0)
+	VADC_CHAN(LR_MUX7_PU1_AMUX_HW_ID, 0)
+	VADC_CHAN(LR_MUX8_PU1_AMUX_THM4, 0)
+	VADC_CHAN(LR_MUX9_PU1_AMUX_THM5, 0)
+	VADC_CHAN(LR_MUX10_PU1_AMUX_USB_ID, 0)		/* 0x79 */
+	VADC_CHAN(LR_MUX3_BUF_PU1_XO_THERM, 0)		/* 0x7c */
+
+	VADC_CHAN(LR_MUX1_PU2_BAT_THERM, 0)		/* 0xb0 */
+	VADC_CHAN(LR_MUX2_PU2_BAT_ID, 0)
+	VADC_CHAN(LR_MUX3_PU2_XO_THERM, 0)
+	VADC_CHAN(LR_MUX4_PU2_AMUX_THM1, 0)
+	VADC_CHAN(LR_MUX5_PU2_AMUX_THM2, 0)
+	VADC_CHAN(LR_MUX6_PU2_AMUX_THM3, 0)
+	VADC_CHAN(LR_MUX7_PU2_AMUX_HW_ID, 0)
+	VADC_CHAN(LR_MUX8_PU2_AMUX_THM4, 0)
+	VADC_CHAN(LR_MUX9_PU2_AMUX_THM5, 0)
+	VADC_CHAN(LR_MUX10_PU2_AMUX_USB_ID, 0)		/* 0xb9 */
+	VADC_CHAN(LR_MUX3_BUF_PU2_XO_THERM, 0)		/* 0xbc */
+
+	VADC_CHAN(LR_MUX1_PU1_PU2_BAT_THERM, 0)		/* 0xf0 */
+	VADC_CHAN(LR_MUX2_PU1_PU2_BAT_ID, 0)
+	VADC_CHAN(LR_MUX3_PU1_PU2_XO_THERM, 0)
+	VADC_CHAN(LR_MUX4_PU1_PU2_AMUX_THM1, 0)
+	VADC_CHAN(LR_MUX5_PU1_PU2_AMUX_THM2, 0)
+	VADC_CHAN(LR_MUX6_PU1_PU2_AMUX_THM3, 0)
+	VADC_CHAN(LR_MUX7_PU1_PU2_AMUX_HW_ID, 0)
+	VADC_CHAN(LR_MUX8_PU1_PU2_AMUX_THM4, 0)
+	VADC_CHAN(LR_MUX9_PU1_PU2_AMUX_THM5, 0)
+	VADC_CHAN(LR_MUX10_PU1_PU2_AMUX_USB_ID, 0)	/* 0xf9 */
+	VADC_CHAN(LR_MUX3_BUF_PU1_PU2_XO_THERM, 0)	/* 0xfc */
+};
+
+static int
+vadc_get_dt_channel_data(struct vadc_priv *vadc, struct

device_node *node)

quoted

quoted

quoted

+{
+	struct vadc_channel *vchan;
+	u32 channel, value, varr[2];
+	int rc, pre, time, avg, decim;

Drop pre, time, avg and decim and reuse rc instead?

quoted

+	const char *name = node->name;
+
+	rc = of_property_read_u32(node, "reg", &channel);
+	if (rc) {
+		dev_dbg(vadc->dev, "invalid channel number %s\n", name);
+		return -EINVAL;
+	}
+
+	if (channel >= vadc->nchannels) {
+		dev_dbg(vadc->dev, "%s invalid channel number %d\n", name,
+			channel);
+		return -EINVAL;
+	}
+
+	vchan = &vadc->channels[channel];

Could you not have these in the same order as the iio_chan_spec

array?

quoted

Hence move the lookups that are scattered elsewhere in the driver to

here?

quoted

<snip>

quoted

quoted

quoted

+static int vadc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct vadc_channel *vchan;
+	struct vadc_priv *vadc;
+	struct resource *res;
+	struct regmap *regmap;
+	int rc, irq_eoc, n;

unsigned int n?

quoted

+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*vadc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	vadc = iio_priv(indio_dev);
+	vadc->dev = dev;
+	vadc->regmap = regmap;
+	vadc->is_ref_measured = false;
+	init_completion(&vadc->complete);
+
+	vadc->nchannels = ARRAY_SIZE(vadc_channels);
+	vadc->channels = devm_kcalloc(dev, vadc->nchannels,
+				      sizeof(*vadc->channels), GFP_KERNEL);

Interesting.  This is always the same size as the vadc_channels so

you

quoted

might as well keep them in the same order and simplify various

corners of

quoted

the code.

quoted

quoted

+	if (!vadc->channels)
+		return -ENOMEM;
+

I wonder if we can't vadc->channels rather more different from

vadc_channels?

quoted

Perhaps vadc->channelspriv or similar? Confused me a little at first.

quoted

quoted

+	for (n = 0; n < vadc->nchannels; n++) {
+		vchan = &vadc->channels[n];
+		/* set default channel properties */
+		vchan->number = -1;	/* inactive */
+		vchan->prescaling = vadc_channels[n].address;
+		vchan->decimation = VADC_DEF_DECIMATION;
+		vchan->hw_settle_time = VADC_DEF_HW_SETTLE_TIME;
+		vchan->avg_samples = VADC_DEF_AVG_SAMPLES;
+		vchan->calibration = VADC_DEF_CALIB_TYPE;
+	}
+
+	platform_set_drvdata(pdev, vadc);
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (!res)
+		return -ENODEV;
+
+	vadc->base = res->start;
+
+	rc = vadc_version_check(vadc);
+	if (rc < 0)
+		return -ENODEV;
+
+	rc = vadc_get_dt_data(vadc, node);
+	if (rc < 0)
+		return rc;
+

Do we need an explicit flag to indicate poll mode rather than just
using the absense of the irq being specified to select that mode?

I will think about this. Maybe I will check the returned value from
platform_get_irq to see is the IRQ resource exist. If the IRQ doesn't
exist I will fallback to polling.

That was my thought.

<snip>

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.