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 <svarba...@mm-sol.com>
>>> Signed-off-by: Ivan T. Ivanov <iiva...@mm-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.
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.
>
>>> + 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?
>>> + 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.
>>> + .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.
<snip>
--
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
