Re: [PATCH v3] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 06/06/2014 03:12 AM, Michael Welling wrote:
[...]
> +enum mcp49x2_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp49x2_state {
> +	struct spi_device *spi;
> +	unsigned int value[MCP49X2_NUM_CHANNELS];
> +	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +};
> +
> +#define MCP49X2_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = IIO_ST('u', bits, 16, 12-bits),	\

The IIO_ST macro was removed a while ago, this will not compile with the 
latest IIO sources.

> +}
> +
> +static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
> +{
> +	u8 mosi[2] ____cacheline_aligned;

It is also important that the transfer buffer is not on the stack and 
strictly speaking the buffer should also be allocated with GFP_DMA.

> +
> +	mosi[1] = val & 0xff;
> +	mosi[0] = (addr == 0) ? 0x00 : 0x80;
> +	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(spi, mosi, 2);
> +}
[...]
> +
> +static int mcp49x2_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp49x2_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (!IS_ERR(state->vref_reg)) {
> +		ret = regulator_enable(state->vref_reg);
> +		if (ret) {
> +			dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = regulator_get_voltage(state->vref_reg);
> +		if (ret < 0) {
> +			dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +		state->vref_mv = ret / 1000;
> +	} else {
> +		dev_info(&spi->dev, "Vref regulator not specified assuming 2.5V\n");
> +		state->vref_mv = 2500;

This is not a good idea, unless there is a built-in vref with 2.5V. If that 
is the case use devm_regulator_get_optional() as devm_regulator_get() will 
return a dummy regulator if no regulator was specified for the device.

> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp49x2_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> +	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(state->vref_reg))
> +		regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}