Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lee,

On 14-11-2022 02:41 pm, Lee Jones wrote:
On Fri, 11 Nov 2022, Naresh Solanki wrote:
On 07-11-2022 03:06 pm, Lee Jones wrote:
On Thu, 03 Nov 2022, Naresh Solanki wrote:

From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>

Implement a regulator driver with IRQ support for fault management.

This is not a "regulator driver".

Written against documentation [1] and [2] and tested on real hardware.

Every channel has its own regulator supplies nammed 'vss1-supply' and
'vss2-supply'. The regulator supply is used to determine the output
voltage, as the smart switch provides no output regulation.
The driver requires the 'shunt-resistor-micro-ohms' property to be
present in Device Tree to properly calculate current related
values.

Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf

Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
Signed-off-by: Marcello Sylvester Bauer <sylv@xxxxxxx>
Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>

[...]

Patrick & Marcello worked on the patch. I'm upstreaming it. Will update it as:
Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
Co-Developed-by: Marcello Sylvester Bauer <sylv@xxxxxxx>
Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>

ok?

+static const struct of_device_id max597x_of_match[] = {
+	{ .compatible = "maxim,max5970", .data = (void *)MAX597x_TYPE_MAX5970 },
+	{ .compatible = "maxim,max5978", .data = (void *)MAX597x_TYPE_MAX5978 },

Where is .data used?
The .data isn't used.

Then why add it?
Yes. Will remove that in next version.

[...]

+#include <linux/device.h>
+#include <linux/regmap.h>
+
+/* Number of switch based on chip variant */

This comment is superfluous.
You mean this comment should be removed ?

I do.
Sure will remove.

+#define MAX5970_NUM_SWITCHES 2
+#define MAX5978_NUM_SWITCHES 1
+/* Both chip variant have 4 indication LEDs used by LED cell */

Here too I think.

+#define MAX597X_NUM_LEDS     4
+
+enum max597x_chip_type {
+	MAX597x_TYPE_MAX5978 = 1,

Why 1?
MAX5978 & single power switch wheres MAX5970 has two.

That's not what this enum means.

You are just describing the type to be matched on.

The value should be arbitrary, no?
Yes you are right. And thats how its being used.
Setting the first enum value to 1 was to avoid zero.

[...]

+#define  OC_STATUS_WARN(ch)				BIT(ch)
+
+#define MAX5970_REG_CHXEN				0x3b
+#define  CHXEN(ch)						(3 << (ch * 2))
+
+#define MAX5970_REG_LED_FLASH			0x43

Do these all need to be shared?

Or can they be isolated into, say, the Regulator driver?

Shared reg.

Where else are they used?

The respective cell regulator/iio & led driver.

Regards,
Naresh



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux