2020-03-09 14:16:01

by Alexandru Tachici

[permalink] [raw]
Subject: [PATCH 0/2] hwmon: pmbus: adm1266: add initial support

Add initial support for the ADM1266 sequencer.
ADM1266 has 17 voltage input channels connected
to a 12 bit SAR ADC and supports standard VOUT
pmbus commands.

Alexandru Tachici (2):
hwmon: pmbus: adm1266: add initial support
dt-bindings: hwmon: Add bindings for ADM1266

.../bindings/hwmon/adi,adm1266.yaml | 47 ++++++
Documentation/hwmon/adm1266 | 34 ++++
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/adm1266.c | 146 ++++++++++++++++++
5 files changed, 238 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
create mode 100644 Documentation/hwmon/adm1266
create mode 100644 drivers/hwmon/pmbus/adm1266.c

--
2.20.1


2020-03-09 14:16:36

by Alexandru Tachici

[permalink] [raw]
Subject: [PATCH 1/2] hwmon: pmbus: adm1266: add initial support

Add initial pmbus probing driver for the adm1266 Cascadable
Super Sequencer with Margin Control and Fault Recording.
Driver is currently using the pmbus_core, creating sysfs
files under hwmon for inputs: vh1->vh4 and vp1->vp13.

Signed-off-by: Alexandru Tachici <[email protected]>
---
Documentation/hwmon/adm1266 | 34 ++++++++
drivers/hwmon/pmbus/Kconfig | 10 +++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/adm1266.c | 146 ++++++++++++++++++++++++++++++++++
4 files changed, 191 insertions(+)
create mode 100644 Documentation/hwmon/adm1266
create mode 100644 drivers/hwmon/pmbus/adm1266.c

diff --git a/Documentation/hwmon/adm1266 b/Documentation/hwmon/adm1266
new file mode 100644
index 000000000000..64651b5086a7
--- /dev/null
+++ b/Documentation/hwmon/adm1266
@@ -0,0 +1,34 @@
+Kernel driver adm1266
+=====================
+
+Supported chips:
+ * Analog Devices ADM1266
+ Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1266.pdf
+
+Author: Alexandru Tachici <[email protected]>
+
+
+Description
+-----------
+
+This driver supports hardware monitoring for Analog Devices ADM1266 sequencer.
+
+ADM1266 is a sequencer that feature voltage readback from 17 channels via an
+integrated 12 bit SAR ADC, accessed using a PMBus interface.
+
+The driver is a client driver to the core PMBus driver. Please see
+Documentation/hwmon/pmbus for details on PMBus client drivers.
+
+
+Sysfs entries
+-------------
+
+The following attributes are supported. Limits are read-write, history reset
+attributes are write-only, all other attributes are read-only.
+
+inX_label "voutx"
+inX_input Measured voltage.
+inX_min Minimum Voltage.
+inX_max Maximum voltage.
+inX_min_alarm Voltage low alarm.
+inX_max_alarm Voltage high alarm.
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index a9ea06204767..153e64febe98 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -26,6 +26,16 @@ config SENSORS_PMBUS
This driver can also be built as a module. If so, the module will
be called pmbus.

+config SENSORS_ADM1266
+ tristate "Analog Devices ADM1266"
+ default n
+ help
+ If you say yes here you get hardware monitoring support for Analog
+ Devices ADM1266.
+
+ This driver can also be built as a module. If so, the module will
+ be called adm1266.
+
config SENSORS_ADM1275
tristate "Analog Devices ADM1275 and compatibles"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 5feb45806123..ed38f6d6f845 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -5,6 +5,7 @@

obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
+obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
new file mode 100644
index 000000000000..3aa8262f9bd6
--- /dev/null
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADM1266 - Cascadable Super Sequencer with Margin
+ * Control and Fault Recording
+ *
+ * Copyright 2020 Analog Devices Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/fwnode.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "pmbus.h"
+
+enum chips { adm1266 };
+
+struct adm1266_data {
+ enum chips id;
+ struct pmbus_driver_info info;
+};
+
+static int adm1266_block_wr(struct i2c_client *client, u8 cmd, u8 wr_len,
+ u8 *data_w, u8 *data_r)
+{
+ union i2c_smbus_data smbus_data;
+ int ret;
+
+ smbus_data.block[0] = wr_len;
+ memcpy(smbus_data.block + 1, data_w, wr_len);
+ ret = i2c_smbus_xfer(client->adapter, client->addr,
+ client->flags, I2C_SMBUS_READ, cmd,
+ I2C_SMBUS_BLOCK_PROC_CALL, &smbus_data);
+ if (ret < 0)
+ return ret;
+
+ memcpy(data_r, &smbus_data.block[1], smbus_data.block[0]);
+
+ return 0;
+}
+
+static int adm1266_config(struct pmbus_driver_info *info)
+{
+ u32 funcs;
+ int i;
+
+ info->pages = 17;
+ info->format[PSC_VOLTAGE_OUT] = linear;
+
+ funcs = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+ for (i = 0; i < info->pages; i++)
+ info->func[i] = funcs;
+
+ return 0;
+}
+
+static const struct i2c_device_id adm1266_id[] = {
+ { "adm1266", adm1266 },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, adm1266_id);
+
+static int adm1266_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
+ u8 wr_buffer[I2C_SMBUS_BLOCK_MAX + 1];
+ const struct i2c_device_id *mid;
+ struct adm1266_data *data;
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BYTE_DATA
+ | I2C_FUNC_SMBUS_BLOCK_DATA))
+ return -ENODEV;
+
+ wr_buffer[0] = 32;
+ ret = adm1266_block_wr(client, PMBUS_MFR_ID, 1, wr_buffer,
+ block_buffer);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to read Manufacturer ID\n");
+ return ret;
+ }
+
+ if (strncmp(block_buffer, "Analog Devices, Inc", 19)) {
+ dev_err(&client->dev, "Unsupported Manufacturer ID\n");
+ return -ENODEV;
+ }
+
+ wr_buffer[0] = 32;
+ ret = adm1266_block_wr(client, PMBUS_MFR_MODEL, 1, wr_buffer,
+ block_buffer);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to read Manufacturer Model\n");
+ return ret;
+ }
+
+ for (mid = adm1266_id; mid->name[0]; mid++) {
+ if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
+ break;
+ }
+ if (!mid->name[0]) {
+ dev_err(&client->dev, "Unsupported device\n");
+ return -ENODEV;
+ }
+
+ data = devm_kzalloc(&client->dev, sizeof(struct adm1266_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ data->id = mid->driver_data;
+
+ ret = adm1266_config(&data->info);
+ if (ret < 0) {
+ dev_err(&client->dev, "Could not configure device.");
+ return ret;
+ }
+
+ return pmbus_do_probe(client, id, &data->info);
+}
+
+static const struct of_device_id adm1266_of_match[] = {
+ { .compatible = "adi,adm1266" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, adm1266_of_match);
+
+static struct i2c_driver adm1266_driver = {
+ .driver = {
+ .name = "adm1266",
+ .of_match_table = adm1266_of_match,
+ },
+ .probe = adm1266_probe,
+ .remove = pmbus_do_remove,
+ .id_table = adm1266_id,
+};
+
+module_i2c_driver(adm1266_driver);
+
+MODULE_AUTHOR("Alexandru Tachici <[email protected]>");
+MODULE_DESCRIPTION("PMBus driver for Analog Devices ADM1266");
+MODULE_LICENSE("GPL v2");
--
2.20.1

2020-03-09 14:17:38

by Alexandru Tachici

[permalink] [raw]
Subject: [PATCH 2/2] dt-bindings: hwmon: Add bindings for ADM1266

Add bindings for the Analog Devices ADM1266 sequencer.

Signed-off-by: Alexandru Tachici <[email protected]>
---
.../bindings/hwmon/adi,adm1266.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
new file mode 100644
index 000000000000..1e83883a4661
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/pmbus/adi,adm1266.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADM1266 Cascadable Super Sequencer with Margin
+Control and Fault Recording
+
+maintainers:
+ - Alexandru Tachici <[email protected]>
+
+description: |
+ Analog Devices ADM1266 Cascadable Super Sequencer with Margin
+ Control and Fault Recording
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1266.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,adm1266
+
+ reg:
+ maxItems: 1
+
+ avcc-supply:
+ description:
+ Phandle to the Avcc power supply
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ i2c0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adm1266@40 {
+ compatible = "adi,adm1266";
+ reg = <0x40>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
+...
--
2.20.1

2020-03-09 16:19:58

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/2] hwmon: pmbus: adm1266: add initial support

On Mon, Mar 09, 2020 at 04:14:21PM +0200, Alexandru Tachici wrote:
> Add initial pmbus probing driver for the adm1266 Cascadable

Does "initial" suggest planned further enhancements ? If yes, please
describe, and explain why this support is omitted at this time.
If no, there is no reason for "initial".

> Super Sequencer with Margin Control and Fault Recording.
> Driver is currently using the pmbus_core, creating sysfs

Why "currently" ?

> files under hwmon for inputs: vh1->vh4 and vp1->vp13.
>
> Signed-off-by: Alexandru Tachici <[email protected]>
> ---
> Documentation/hwmon/adm1266 | 34 ++++++++
> drivers/hwmon/pmbus/Kconfig | 10 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/adm1266.c | 146 ++++++++++++++++++++++++++++++++++
> 4 files changed, 191 insertions(+)
> create mode 100644 Documentation/hwmon/adm1266
> create mode 100644 drivers/hwmon/pmbus/adm1266.c
>
> diff --git a/Documentation/hwmon/adm1266 b/Documentation/hwmon/adm1266
> new file mode 100644
> index 000000000000..64651b5086a7
> --- /dev/null
> +++ b/Documentation/hwmon/adm1266
> @@ -0,0 +1,34 @@
> +Kernel driver adm1266
> +=====================
> +
> +Supported chips:
> + * Analog Devices ADM1266
> + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1266.pdf
> +
> +Author: Alexandru Tachici <[email protected]>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for Analog Devices ADM1266 sequencer.
> +
> +ADM1266 is a sequencer that feature voltage readback from 17 channels via an
> +integrated 12 bit SAR ADC, accessed using a PMBus interface.
> +
> +The driver is a client driver to the core PMBus driver. Please see
> +Documentation/hwmon/pmbus for details on PMBus client drivers.
> +
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. Limits are read-write, history reset
> +attributes are write-only, all other attributes are read-only.
> +
> +inX_label "voutx"
> +inX_input Measured voltage.
> +inX_min Minimum Voltage.
> +inX_max Maximum voltage.
> +inX_min_alarm Voltage low alarm.
> +inX_max_alarm Voltage high alarm.
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index a9ea06204767..153e64febe98 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -26,6 +26,16 @@ config SENSORS_PMBUS
> This driver can also be built as a module. If so, the module will
> be called pmbus.
>
> +config SENSORS_ADM1266
> + tristate "Analog Devices ADM1266"
> + default n

Unnecessary.

> + help
> + If you say yes here you get hardware monitoring support for Analog
> + Devices ADM1266.
> +
> + This driver can also be built as a module. If so, the module will
> + be called adm1266.
> +
> config SENSORS_ADM1275
> tristate "Analog Devices ADM1275 and compatibles"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 5feb45806123..ed38f6d6f845 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -5,6 +5,7 @@
>
> obj-$(CONFIG_PMBUS) += pmbus_core.o
> obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> +obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> new file mode 100644
> index 000000000000..3aa8262f9bd6
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADM1266 - Cascadable Super Sequencer with Margin
> + * Control and Fault Recording
> + *
> + * Copyright 2020 Analog Devices Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/fwnode.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "pmbus.h"
> +
> +enum chips { adm1266 };

Is this driver ever expected to support another chip ? If no, this is
unnecessary.

> +
> +struct adm1266_data {
> + enum chips id;
> + struct pmbus_driver_info info;
> +};
> +
> +static int adm1266_block_wr(struct i2c_client *client, u8 cmd, u8 wr_len,
> + u8 *data_w, u8 *data_r)
> +{
> + union i2c_smbus_data smbus_data;
> + int ret;
> +
> + smbus_data.block[0] = wr_len;
> + memcpy(smbus_data.block + 1, data_w, wr_len);
> + ret = i2c_smbus_xfer(client->adapter, client->addr,
> + client->flags, I2C_SMBUS_READ, cmd,
> + I2C_SMBUS_BLOCK_PROC_CALL, &smbus_data);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(data_r, &smbus_data.block[1], smbus_data.block[0]);
> +
> + return 0;
> +}
> +
> +static int adm1266_config(struct pmbus_driver_info *info)
> +{
> + u32 funcs;
> + int i;
> +
> + info->pages = 17;
> + info->format[PSC_VOLTAGE_OUT] = linear;
> +
> + funcs = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
> + for (i = 0; i < info->pages; i++)
> + info->func[i] = funcs;
> +
> + return 0;
> +}

I don't immediately see the benefit of allocating this dynamically.
Please consider using a static structure.

> +
> +static const struct i2c_device_id adm1266_id[] = {
> + { "adm1266", adm1266 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, adm1266_id);
> +
> +static int adm1266_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> + u8 wr_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> + const struct i2c_device_id *mid;
> + struct adm1266_data *data;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_BYTE_DATA
> + | I2C_FUNC_SMBUS_BLOCK_DATA))
> + return -ENODEV;
> +
> + wr_buffer[0] = 32;
> + ret = adm1266_block_wr(client, PMBUS_MFR_ID, 1, wr_buffer,
> + block_buffer);

Please replace with standard SMBus block read commands. If that doesn't
work, please explain.

Note that according to the datasheet, the MFR commands are writeable.
You might want to consider checking 0xAD—IC_DEVICE_ID instead.

> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to read Manufacturer ID\n");
> + return ret;
> + }
> +
> + if (strncmp(block_buffer, "Analog Devices, Inc", 19)) {
> + dev_err(&client->dev, "Unsupported Manufacturer ID\n");
> + return -ENODEV;
> + }
> +
> + wr_buffer[0] = 32;
> + ret = adm1266_block_wr(client, PMBUS_MFR_MODEL, 1, wr_buffer,
> + block_buffer);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to read Manufacturer Model\n");
> + return ret;
> + }
> +
> + for (mid = adm1266_id; mid->name[0]; mid++) {
> + if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> + break;
> + }
> + if (!mid->name[0]) {
> + dev_err(&client->dev, "Unsupported device\n");
> + return -ENODEV;
> + }
> +
Unless there is reason to believe that the driver will in the future support
other chips, this is unnecessarily complex.

> + data = devm_kzalloc(&client->dev, sizeof(struct adm1266_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + data->id = mid->driver_data;
> +
> + ret = adm1266_config(&data->info);
> + if (ret < 0) {
> + dev_err(&client->dev, "Could not configure device.");
> + return ret;
> + }
> +
> + return pmbus_do_probe(client, id, &data->info);
> +}
> +
> +static const struct of_device_id adm1266_of_match[] = {
> + { .compatible = "adi,adm1266" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, adm1266_of_match);
> +
> +static struct i2c_driver adm1266_driver = {
> + .driver = {
> + .name = "adm1266",
> + .of_match_table = adm1266_of_match,
> + },
> + .probe = adm1266_probe,
> + .remove = pmbus_do_remove,
> + .id_table = adm1266_id,
> +};
> +
> +module_i2c_driver(adm1266_driver);
> +
> +MODULE_AUTHOR("Alexandru Tachici <[email protected]>");
> +MODULE_DESCRIPTION("PMBus driver for Analog Devices ADM1266");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1
>

2020-03-09 16:22:24

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 2/2] dt-bindings: hwmon: Add bindings for ADM1266

On Mon, Mar 09, 2020 at 04:14:22PM +0200, Alexandru Tachici wrote:
> Add bindings for the Analog Devices ADM1266 sequencer.
>

Unless there are more bindings expected in the future, this could be
added as trivial device. If there _are_ more bindings expected in the
future, they should be part of this patch.

Thanks,
Guenter

> Signed-off-by: Alexandru Tachici <[email protected]>
> ---
> .../bindings/hwmon/adi,adm1266.yaml | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
> new file mode 100644
> index 000000000000..1e83883a4661
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1266.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/pmbus/adi,adm1266.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADM1266 Cascadable Super Sequencer with Margin
> +Control and Fault Recording
> +
> +maintainers:
> + - Alexandru Tachici <[email protected]>
> +
> +description: |
> + Analog Devices ADM1266 Cascadable Super Sequencer with Margin
> + Control and Fault Recording
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1266.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - adi,adm1266
> +
> + reg:
> + maxItems: 1
> +
> + avcc-supply:
> + description:
> + Phandle to the Avcc power supply
> +
> +required:
> + - compatible
> + - reg
> +
> +examples:
> + - |
> + i2c0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adm1266@40 {
> + compatible = "adi,adm1266";
> + reg = <0x40>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> + };
> +...
> --
> 2.20.1
>