Message-ID: <4dbc485f-599a-4b50-854c-c2e1c44d4810@email.android.com>
Date:	Mon, 15 Sep 2014 17:11:37 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Stanimir Varbanov <svarbanov@...sol.com>
CC:	Hartmut Knaack <knaack.h@....de>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	Grant Likely <grant.likely@...aro.org>,
	Arnd Bergmann <arnd@...db.de>, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Angelo Compagnucci <angelo.compagnucci@...il.com>,
	Doug Anderson <dianders@...omium.org>,
	Fugang Duan <B38611@...escale.com>,
	Johannes Thumshirn <johannes.thumshirn@....de>,
	Jean Delvare <jdelvare@...e.de>,
	Philippe Reynes <tremyfr@...oo.fr>,
	Lee Jones <lee.jones@...aro.org>,
	Josh Cartwright <joshc@...eaurora.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	David Collins <collinsd@...eaurora.org>,
	"Ivan T. Ivanov" <iivanov@...sol.com>
Subject: Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver



On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov <svarbanov@...sol.com> wrote:
>Hi Jonathan,
>
>Thanks for the review!
>
>On 09/13/2014 08:27 PM, Jonathan Cameron wrote:
>> On 13/09/14 00:27, Hartmut Knaack wrote:
>>> Stanimir Varbanov schrieb, Am 11.09.2014 17:13:
>>>> 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.
>> 
>> 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
>> tree reading code rather than doing it in lots of other places. 
>Hence
>> 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 :)
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@...sol.com>
>>>> Signed-off-by: Ivan T. Ivanov <iivanov@...sol.com>
>>>> ---
>>>>  drivers/iio/adc/Kconfig                       |   11 +
>>>>  drivers/iio/adc/Makefile                      |    1 +
>>>>  drivers/iio/adc/qcom-spmi-vadc.c              |  999
>+++++++++++++++++++++++++
>>>>  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>
>
>>>> +
>>>> +static int
>>> Don't wrap line here.
>>>> +vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec
>const *chan,
>>>> +	      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
>> 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?
>
>> 
>>>> +	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
>> all the channels?  Very handy if so, but seems a little unlikely with
>15 bit
>> 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.
>
>>>> +		rc = IIO_VAL_INT;
>>> return IIO_VAL_INT;
>>>> +		break;
>>>> +	default:
>>>> +		rc = -EINVAL;
>>>> +		break;
>>> Drop default case, or leave empty.
>>>> +	}
>>>> +
>>>> +	return rc;
>>> return -EINVAL;
>>>> +}
>>>> +
>>>> +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
>> 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.
>
>>>> +		.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)
>>>> +{
>>>> +	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?
>>>> +	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?
>> Hence move the lookups that are scattered elsewhere in the driver to
>here?
>> 
>
><snip>
>
>>>> +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?
>>>> +
>>>> +	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
>> might as well keep them in the same order and simplify various
>corners of
>> the code.
>>>> +	if (!vadc->channels)
>>>> +		return -ENOMEM;
>>>> +
>> I wonder if we can't vadc->channels rather more different from
>vadc_channels?
>> Perhaps vadc->channelspriv or similar? Confused me a little at first.
>>>> +	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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists