drivers:iio:dac: add max538x support #2151

mrcsosa · 2023-03-23T00:45:19Z

Added driver implementation for MAX5380/1/2 8-bit DAC.

More info: analog.com: MAX5380-MAX5382

Signed-off-by: Marc Paolo Sosa marcpaolo.sosa@analog.com

amiclaus

please send the patches upstream for further review.

mrcsosa · 2023-04-20T00:20:26Z

Hi, can I request an initial review of this PR before sending the patches upstream? Thank you.

nunojsa

I would suggest to make this driver a bit more interesting. The part is very simple and looking at the datasheet, I would say the driver should support all max5380 - max5382. That would turn the driver a bit more interesting. If I'm not missing something, you would only need to handle the different VREFs

nunojsa · 2023-04-20T13:00:07Z

Documentation/devicetree/bindings/iio/dac/maxim,max5380.yaml

+    const: 1
+
+  '#size-cells':
+    const: 0


the above two properties are not needed... On top of this, if you are not adding more into this schema, the part could go to:

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/trivial-devices.yaml

That said, you could already add support for the vdd regulator that would justify this bindings.

nunojsa · 2023-04-20T13:00:23Z

Documentation/devicetree/bindings/iio/dac/maxim,max5380.yaml

+         #address-cells = <1>;
+         #size-cells = <0>;
+
+         max5380: dac@31 {


just dac@31

nunojsa · 2023-04-20T13:01:10Z

drivers/iio/dac/Kconfig

+config MAX5380
+	tristate "Maxim MAX5380 DAC driver"
+	depends on I2C
+	depends on OF


there's no OF dependency...

nunojsa · 2023-04-20T13:04:06Z

drivers/iio/dac/max5380.c

+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define V_REF 2.0


use something like define MAX5380_VREF 2

nunojsa · 2023-04-20T13:05:19Z

drivers/iio/dac/max5380.c

+#include <linux/proc_fs.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>


please cleanup the includes... There are things not needed and others that are missing. Furthermore, they should be placed in alphabetical order

nunojsa · 2023-04-20T13:09:34Z

drivers/iio/dac/max5380.c

+	data = iio_priv(indio_dev);
+	data->client = client;
+
+	indio_dev->dev.parent = &client->dev;


nunojsa · 2023-04-20T13:09:57Z

drivers/iio/dac/max5380.c

+	return ret;
+	}
+
+return 0;


wrong indentation... furthermore, just do return devm_iio_device_register(). Note that you need the devm_ variant. Otherwise you would need a .remove hook defined in your struct i2c_driver

nunojsa · 2023-04-20T13:12:08Z

drivers/iio/dac/max5380.c

+
+static const struct i2c_device_id max5380_id[] = {
+	{ "max5380", 0 },
+	{},


no need to explicitly set .driver_data to 0.

nunojsa · 2023-04-20T13:17:12Z

drivers/iio/dac/max5380.c

+static struct i2c_driver max5380_driver = {
+		.driver = {
+				.name = "max5380",
+				.of_match_table = max5380_of_match,


this looks to have more indentation than needed...

nunojsa · 2023-04-20T13:20:50Z

drivers/iio/dac/max5380.c

+		if (val < 0 || val > 255)
+			return -EINVAL;
+		data->value = val;
+		ret = i2c_smbus_write_byte(data->client, data->value);


why smbus? from a quick grasp on the datasheet, I do not see any specific smbus mention... Can't we use i2c_master_send()?

Hi, I have added few updates to support other devices. I will also include the improvements you have mentioned on my next update. Thank you so much.

nunojsa · 2023-05-02T12:29:59Z

Documentation/devicetree/bindings/iio/dac/maxim,max538x.yaml

+    const: 1
+
+  '#size-cells':
+    const: 0


no need for the above two properties..

nunojsa · 2023-05-02T12:32:27Z

Documentation/devicetree/bindings/iio/dac/maxim,max538x.yaml

+      - maxim,max5382L
+      - maxim,max5382M
+      - maxim,max5382N
+      - maxim,max5382K


What is the L,M,N,K,L all about? Do we need a sepcific compatible? AFAICT, we have max5380/max5381/max53802

nunojsa · 2023-05-02T12:32:51Z

Documentation/devicetree/bindings/iio/dac/maxim,max538x.yaml

+                reg=<0x31>;
+          };
+    };
+...


as it stands, these could go to trivial devices... But I think you need to add the vdd regulator if supporting all three devices

nunojsa · 2023-05-02T12:33:24Z

drivers/iio/dac/max538x.c

+		return ret;
+	}
+
+	return 0;


just return devm_iio_device_register()

nunojsa · 2023-05-02T12:34:29Z

drivers/iio/dac/max538x.c

+	case MAX5382M:
+	case MAX5382N:
+	case MAX5382K:
+		data->vfactor = 0.9;


? There's no floating point in the kernel... Also, I do not think this is right. From the datasheet, the internal reference for mx5832 is 0.9 * VDD. Hence, you need to use a regulator and get it's voltage and then do the calculation accordingly.

Look for examples using devm_regulator_get() and regulator_get_voltage()

You can have a flag (most obvious solution) in your chip info structure to let you know when do you need to get the regulator voltage so that you can calculate the internal reference.

nunojsa · 2023-05-02T12:38:14Z

drivers/iio/dac/max538x.c

+		return -EINVAL;
+	}
+
+	indio_dev->dev.parent = &client->dev;


drop this. automatically done by IIO core

nunojsa · 2023-05-02T12:38:33Z

drivers/iio/dac/max538x.c

+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev) {
+		dev_err(&client->dev, "Failed to allocate IIO device\n");


remove this log

nunojsa · 2023-05-02T12:38:46Z

drivers/iio/dac/max538x.c

+		dev_err(&client->dev, "Invalid device type specified\n");
+
+		return -EINVAL;
+	}


remove the above... should never happen

nunojsa · 2023-05-02T12:39:22Z

drivers/iio/dac/max538x.c

+		}
+	} else {
+		return -EINVAL;
+	}


use a switch() case

nunojsa · 2023-05-02T12:40:28Z

drivers/iio/dac/max538x.c

+		.name = "max538x",
+		.of_match_table = max538x_of_match,
+	},
+	.probe = max538x_probe,


use probe_new()... You need to see when sending these upstream if probe_new() is still in use or not. If it is, you should use it (I think).

added documentation for max5380/1/2 support Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

Added driver implementation for MAX5380/1/2 8-bit DAC. More information: analog.com: MAX5380/1/2 Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

add entry for MAX5380/1/2 driver Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

mrcsosa · 2023-05-24T01:47:53Z

Removed: Multiple device list (adjusted to only max5380/1/2)
Removed: Unnecessary error checking and logs
Adjusted: if statements to switch case
Added: voltage regulator for external vref.

nunojsa · 2024-01-15T11:43:49Z

@mrcsosa, I completely failed to look again at this one... Can you rebase onto master?

For similar situations, just ping me for the review :)

mrcsosa marked this pull request as draft March 23, 2023 00:49

mrcsosa force-pushed the dev/max538x-master branch 15 times, most recently from d408cf8 to 257d322 Compare March 24, 2023 03:39

amiclaus reviewed Apr 10, 2023

View reviewed changes

mrcsosa force-pushed the dev/max538x-master branch 7 times, most recently from 23d5d4d to b7f38bc Compare April 11, 2023 00:58

mrcsosa marked this pull request as ready for review April 11, 2023 01:23

mrcsosa force-pushed the dev/max538x-master branch 3 times, most recently from 2c46209 to 141f290 Compare April 14, 2023 00:49

mrcsosa changed the title ~~drivers:iio:dac: add max538x support~~ drivers:iio:dac: add max5380 support Apr 19, 2023

nunojsa requested changes Apr 20, 2023

View reviewed changes

mrcsosa force-pushed the dev/max538x-master branch 2 times, most recently from b66beac to d2283cf Compare April 26, 2023 01:02

mrcsosa changed the title ~~drivers:iio:dac: add max5380 support~~ drivers:iio:dac: add max538x support Apr 26, 2023

mrcsosa force-pushed the dev/max538x-master branch 4 times, most recently from c85477d to d3149aa Compare May 2, 2023 03:53

nunojsa requested changes May 2, 2023

View reviewed changes

mrcsosa force-pushed the dev/max538x-master branch 3 times, most recently from a26e354 to c984413 Compare May 24, 2023 00:44

mrcsosa added 3 commits May 24, 2023 08:52

Documentation:devicetree:bindings: add maxim,max538x.yaml

415d2a2

added documentation for max5380/1/2 support Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

drivers:iio:dac: added max538x support

2e158c9

Added driver implementation for MAX5380/1/2 8-bit DAC. More information: analog.com: MAX5380/1/2 Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

iio: Kconfig.adi: imply MAX538X

c3bc1ae

add entry for MAX5380/1/2 driver Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@analog.com>

mrcsosa force-pushed the dev/max538x-master branch from c984413 to c3bc1ae Compare May 24, 2023 00:53

SRaus changed the base branch from master to main January 9, 2024 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers:iio:dac: add max538x support #2151

drivers:iio:dac: add max538x support #2151

mrcsosa commented Mar 23, 2023

amiclaus left a comment

mrcsosa commented Apr 20, 2023

nunojsa left a comment

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

nunojsa Apr 20, 2023

mrcsosa Apr 26, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

nunojsa May 2, 2023

mrcsosa commented May 24, 2023

nunojsa commented Jan 15, 2024

drivers:iio:dac: add max538x support #2151

Are you sure you want to change the base?

drivers:iio:dac: add max538x support #2151

Conversation

mrcsosa commented Mar 23, 2023

amiclaus left a comment

Choose a reason for hiding this comment

mrcsosa commented Apr 20, 2023

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcsosa commented May 24, 2023

nunojsa commented Jan 15, 2024