Re: [PATCH 2/3] iio: dac: Support ROHM BD79703 DAC

[Jonathan Cameron]

On Thu, 19 Dec 2024 13:39:37 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> The ROHM BD79703 is a 6 channel digital to analog converter.
> 
> Based on the data-sheet examples the hardware would support setting the
> DAC word without changing the actual output. The data-sheet is not too
> specific on how the enabling the output of new voltage set by DAC
> should be done - hence this is not implemented by the driver.

I took a quick look and that does seem rather mysterious!
Normally there is either a 'sync' register or pin for setups like this
but no sign of either.  Maybe it's legacy from some other use of the
same silicon.

> 
> The BD79703 would also support two specific "PULL_DOWN" modes. These
> aren't currently supported by the driver either.
> 
> Add a very basic support for controlling the channel outputs one-by-one.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

Short and sweet.  One totally trivial formatting thing I can fix whilst
applying and a passing comment below. will leave this a little while
to see what other feedback it gets and for DT folk to look at the binding.

Jonathan


> diff --git a/drivers/iio/dac/rohm-bd79703.c b/drivers/iio/dac/rohm-bd79703.c
> new file mode 100644
> index 000000000000..a3df31d81b7c
> --- /dev/null
> +++ b/drivers/iio/dac/rohm-bd79703.c
> @@ -0,0 +1,162 @@


> +
> +static int bd79703_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct bd79703_data *data;
> +	struct iio_dev *idev;
> +	int ret;
> +
> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(idev);
> +
> +	data->regmap = devm_regmap_init_spi(spi, &bd79703_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "Failed to initialize Regmap\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vcc");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable VCC\n");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vfs");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get Vfs\n");
> +
> +	data->vfs = ret;
> +	idev->channels = bd79703_channels;
> +	idev->num_channels = ARRAY_SIZE(bd79703_channels);
> +	idev->modes = INDIO_DIRECT_MODE;
> +	idev->info = &bd79703_info;
> +	idev->name = "bd79703";
> +
> +	/* Initialize all to output zero */

I can't remember what we tend to do for defaults but I guess this
is safe enough. Alternative would be start the device with HI impedance
(power down state)

> +	ret = regmap_write(data->regmap, BD79703_REG_OUT_ALL, 0);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, idev);
> +}
> +
> +static const struct spi_device_id bd79703_id[] = {
> +	{ "bd79703", },
> +	{}
	{ }

Keep it consistent. This is the style I prefer though not sure I'll do
a full tidy up of this any time soon. 
> +};
> +MODULE_DEVICE_TABLE(spi, bd79703_id);
> +
> +static const struct of_device_id bd79703_of_match[] = {
> +	{ .compatible = "rohm,bd79703", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bd79703_of_match);