Re: [PATCH] iio: adc: add max1117/max1118/max1119 ADC driver

On 05/03/17 12:33, Akinobu Mita wrote:

This adds max1117/max1118/max1119 8-bit, dual-channel ADC driver.

Signed-off-by: Akinobu Mita <redacted>
Cc: Jonathan Cameron <redacted>
Cc: Hartmut Knaack <redacted>
Cc: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
Cc: Peter Meerwald-Stadler <redacted>
Cc: Rob Herring <redacted>
Cc: Mark Rutland <redacted>

Hi Akinobu,

The SPI usage is 'unusual' enough that I'd like Mark Brown to take a quick
look and give a view on whether it will always work or not.
cc'd Mark and linux-spi.

A few other comments inline.

Thanks,

Jonathan

Is this guaranteed to actually work?  I've no idea what the spi core
does with a zero length transfer.

+			.delay_usecs = 8,
+		},
+		{
+			.rx_buf = &adc->data,
+			.len = 1,
+		},
+	};
+	int ret;
+
+	if (channel == 0)
+		ret = spi_sync_transfer(spi, xfers + 1, 2);
+	else
+		ret = spi_sync_transfer(spi, xfers, 3);
+
+	if (ret)
+		return ret;
+
+	return adc->data;
+}
+
+static int max1118_get_vref_mV(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct max1118 *adc = iio_priv(indio_dev);
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int vref_uV;
+
+	switch (id->driver_data) {
+	case max1117:
+		return 2048;
+	case max1119:
+		return 4096;
+	case max1118:
+		if (IS_ERR_OR_NULL(adc->reg))
+			return -EINVAL;

Again, make it non optional and drop it.

If it really is optional, you should have two iio_info structures and
not have the scale available on one of them...

+
+		vref_uV = regulator_get_voltage(adc->reg);
+		if (vref_uV < 0)
+			return vref_uV;
+		return vref_uV / 1000;
+	}
+
+	return -ENODEV;
+}
+
+static int max1118_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask)
+{
+	struct max1118 *adc = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&adc->lock);
+		*val = max1118_read(adc->spi, chan->channel);
+		mutex_unlock(&adc->lock);
+		if (*val < 0)
+			return *val;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = max1118_get_vref_mV(adc->spi);
+		if (*val < 0)
+			return *val;
+		*val2 = 8;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info max1118_info = {
+	.read_raw = max1118_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int max1118_remove(struct spi_device *spi)

Code ordering is a little unusual.  Please move this to after the probe.

+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct max1118 *adc = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+	if (!IS_ERR_OR_NULL(adc->reg))

The regulator isn't optional so to have gotten here you have succesfully
acquired it or been passed a fake one.

+		return regulator_disable(adc->reg);
+
+	return 0;
+}
+
+static irqreturn_t max1118_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct max1118 *adc = iio_priv(indio_dev);
+	u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */
+	int scan_index;
+	int i = 0;
+
+	mutex_lock(&adc->lock);
+
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			indio_dev->masklength) {
+		const struct iio_chan_spec *scan_chan =
+				&indio_dev->channels[scan_index];
+		int ret = max1118_read(adc->spi, scan_chan->channel);
+
+		if (ret < 0) {
+			dev_warn(&adc->spi->dev,
+				"failed to get conversion data\n");
+			goto out;
+		}
+
+		data[i] = ret;
+		i++;
+	}
+	iio_push_to_buffers_with_timestamp(indio_dev, data,
+					   iio_get_time_ns(indio_dev));
+out:
+	mutex_unlock(&adc->lock);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int max1118_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max1118 *adc;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+	mutex_init(&adc->lock);
+
+	adc->reg = devm_regulator_get(&spi->dev, "vref");
+	if (PTR_ERR(adc->reg) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (IS_ERR_OR_NULL(adc->reg) && id->driver_data == max1118) {
+		dev_warn(&spi->dev, "max1118 requires vref-supply\n");

Not a warning as it's not optional.  Error out on this.
If one isn't specified, the regulator framework will feed you a fake
one anyway..

Looking further up, this should only be requested for the max1118, but
should be required for that one part.

+	} else {
+		ret = regulator_enable(adc->reg);
+		if (ret)
+			return ret;
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &max1118_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max1118_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max1118_channels);
+
+	/*
+	 * To reinitiate a conversion on CH0, it is necessary to allow for a
+	 * conversion to be complete and all of the data to be read out.  Once
+	 * a conversion has been completed, the MAX1117/MAX1118/MAX1119 will go
+	 * into AutoShutdown mode until the next conversion is initiated.
+	 */
+	max1118_read(spi, 0);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					max1118_trigger_handler, NULL);
+	if (ret)
+		goto err_reg_disable;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err_buffer_cleanup;
+
+	return 0;
+
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
+err_reg_disable:

Clean this up once you have made a failure to get the regulator drop out.
Needs to be dependent on the device type not whether there is a regulator
or not.

+	if (!IS_ERR_OR_NULL(adc->reg))
+		regulator_disable(adc->reg);
+
+	return ret;
+}
+
+static const struct spi_device_id max1118_id[] = {
+	{ "max1117", max1117 },
+	{ "max1118", max1118 },
+	{ "max1119", max1119 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, max1118_id);
+
+#ifdef CONFIG_OF
+
+static const struct of_device_id max1118_dt_ids[] = {
+	{ .compatible = "maxim,max1117" },
+	{ .compatible = "maxim,max1118" },
+	{ .compatible = "maxim,max1119" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max1118_dt_ids);
+
+#endif
+
+static struct spi_driver max1118_spi_driver = {
+	.driver = {
+		.name = "max1118",
+		.of_match_table = of_match_ptr(max1118_dt_ids),
+	},
+	.probe = max1118_probe,
+	.remove = max1118_remove,
+	.id_table = max1118_id,
+};
+module_spi_driver(max1118_spi_driver);
+
+MODULE_AUTHOR("Akinobu Mita [off-list ref]");
+MODULE_DESCRIPTION("MAXIM MAX1117/MAX1118/MAX1119 ADCs driver");
+MODULE_LICENSE("GPL v2");

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