[PATCH v1 0/2] LTC4286 and LTC4287 driver support

[PATCH v1 0/2] LTC4286 and LTC4287 driver support

[Delphine CC Chiu]

v1 - Add LTC4286 and LTC4287 binding document
   - Add LTC4286 and LTC4287 driver

Delphine CC Chiu (2):
  dt-bindings: hwmon: Add lltc ltc4286 driver bindings
  hwmon: pmbus: Add ltc4286 driver

 .../bindings/hwmon/lltc,ltc4286.yaml          |  47 ++++++
 MAINTAINERS                                   |   9 ++
 drivers/hwmon/pmbus/Kconfig                   |   9 ++
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/ltc4286.c                 | 142 ++++++++++++++++++
 5 files changed, 208 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
 create mode 100644 drivers/hwmon/pmbus/ltc4286.c

-- 
2.17.1

[PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine CC Chiu]

Add a device tree bindings for ltc4286 driver.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 .../bindings/hwmon/lltc,ltc4286.yaml          | 47 +++++++++++++++++++
 MAINTAINERS                                   |  9 ++++
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
new file mode 100644
index 000000000000..c1c8e310f9c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.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/lltc,ltc4286.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LTC4286 power monitors
+
+maintainers:
+  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+
+properties:
+  compatible:
+    enum:
+      - lltc,ltc4286
+      - lltc,ltc4287
+
+  reg:
+    maxItems: 1
+
+  vrange_select_25p6:
+    description:
+      This property is a bool parameter to represent the
+      voltage range is 25.6 or not for this chip.
+
+  rsense-micro-ohms:
+    description:
+      Resistor value in micro-Ohm
+
+required:
+  - compatible
+  - reg
+  - rsense-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        status = "okay";
+        ltc4286@40 {
+            compatible = "lltc,ltc4286";
+            reg = <0x40>;
+            vrange_select_25p6;
+            rsense-micro-ohms = <300>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c6545eb54104..930bdba0f73c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12183,6 +12183,15 @@ S:	Maintained
 F:	Documentation/hwmon/ltc4261.rst
 F:	drivers/hwmon/ltc4261.c
 
+LTC4286 HARDWARE MONITOR DRIVER
+M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
+F:	drivers/hwmon/pmbus/Kconfig
+F:	drivers/hwmon/pmbus/Makefile
+F:	drivers/hwmon/pmbus/ltc4286.c
+
 LTC4306 I2C MULTIPLEXER DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 L:	linux-i2c@vger.kernel.org
-- 
2.17.1

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Rob Herring]

On Mon, 24 Apr 2023 18:13:49 +0800, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 driver.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  .../bindings/hwmon/lltc,ltc4286.yaml          | 47 +++++++++++++++++++
>  MAINTAINERS                                   |  9 ++++
>  2 files changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dts:22.17-30: Warning (reg_format): /example-0/i2c/ltc4286@40:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dts:18.13-26.11: Warning (i2c_bus_bridge): /example-0/i2c: incorrect #address-cells for I2C bus
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dts:18.13-26.11: Warning (i2c_bus_bridge): /example-0/i2c: incorrect #size-cells for I2C bus
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'i2c_bus_bridge'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dts:20.24-25.15: Warning (avoid_default_addr_size): /example-0/i2c/ltc4286@40: Relying on default #address-cells value
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dts:20.24-25.15: Warning (avoid_default_addr_size): /example-0/i2c/ltc4286@40: Relying on default #size-cells value
Documentation/devicetree/bindings/hwmon/lltc,ltc4286.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230424101352.28117-2-Delphine_CC_Chiu@Wiwynn.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On Mon, Apr 24, 2023 at 06:13:49PM +0800, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 driver.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  .../bindings/hwmon/lltc,ltc4286.yaml          | 47 +++++++++++++++++++
>  MAINTAINERS                                   |  9 ++++
>  2 files changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> new file mode 100644
> index 000000000000..c1c8e310f9c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.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/lltc,ltc4286.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTC4286 power monitors
> +
> +maintainers:
> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lltc,ltc4286
> +      - lltc,ltc4287

There is no LTC4287, at least according to the Analog website.

> +
> +  reg:
> +    maxItems: 1
> +
> +  vrange_select_25p6:
> +    description:
> +      This property is a bool parameter to represent the
> +      voltage range is 25.6 or not for this chip.

Existing attributes are adi,vrange-high-enable, ti,extended-range-enable,
ti,bus-range-microvolt, adi,range-double. I would suggest to use
adi,vrange-high-enable.

> +
> +  rsense-micro-ohms:
> +    description:
> +      Resistor value in micro-Ohm

I would suggest either shunt-resistor-micro-ohms or
sense-resistor-micro-ohms as used by other bindings.

> +
> +required:
> +  - compatible
> +  - reg
> +  - rsense-micro-ohms
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        status = "okay";
> +        ltc4286@40 {
> +            compatible = "lltc,ltc4286";
> +            reg = <0x40>;
> +            vrange_select_25p6;
> +            rsense-micro-ohms = <300>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c6545eb54104..930bdba0f73c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12183,6 +12183,15 @@ S:	Maintained
>  F:	Documentation/hwmon/ltc4261.rst
>  F:	drivers/hwmon/ltc4261.c
>  
> +LTC4286 HARDWARE MONITOR DRIVER
> +M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> +F:	drivers/hwmon/pmbus/Kconfig
> +F:	drivers/hwmon/pmbus/Makefile
> +F:	drivers/hwmon/pmbus/ltc4286.c
> +
>  LTC4306 I2C MULTIPLEXER DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
>  L:	linux-i2c@vger.kernel.org
> -- 
> 2.17.1
> 

RE: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, April 24, 2023 9:50 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On Mon, Apr 24, 2023 at 06:13:49PM +0800, Delphine CC Chiu wrote:
> > Add a device tree bindings for ltc4286 driver.
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > ---
> >  .../bindings/hwmon/lltc,ltc4286.yaml          | 47
> +++++++++++++++++++
> >  MAINTAINERS                                   |  9 ++++
> >  2 files changed, 56 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > new file mode 100644
> > index 000000000000..c1c8e310f9c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > @@ -0,0 +1,47 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%7C
> 01%7C
> >
> +Wayne_SC_Liu%40wiwynn.com%7Cac3bc6cdf556435cfa0f08db44cabd06%7C
> da6e06
> >
> +28fc834caf9dd273061cbab167%7C0%7C0%7C638179409777006260%7CUnk
> nown%7CT
> >
> +WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVC
> >
> +I6Mn0%3D%7C3000%7C%7C%7C&sdata=8d9ycpebE%2FrhafDDmDL9sefo1xq
> m%2B9r%2F
> > +Wm0Cu1McEOs%3D&reserved=0
> > +$schema:
> > +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne_S
> C_Liu%
> >
> +40wiwynn.com%7Cac3bc6cdf556435cfa0f08db44cabd06%7Cda6e0628fc834c
> af9dd
> >
> +273061cbab167%7C0%7C0%7C638179409777006260%7CUnknown%7CTWFp
> bGZsb3d8ey
> >
> +JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C30
> >
> +00%7C%7C%7C&sdata=u4yJaVeL2xSUE2%2FaNpEbQ3KJiy%2BcxtKG95gGuaGI
> LFU%3D&
> > +reserved=0
> > +
> > +title: LTC4286 power monitors
> > +
> > +maintainers:
> > +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - lltc,ltc4286
> > +      - lltc,ltc4287
> 
> There is no LTC4287, at least according to the Analog website.
It has been announced on Analog Devices website.
Please refer to this link: https://www.analog.com/en/products/ltc2487.html#product-overview

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vrange_select_25p6:
> > +    description:
> > +      This property is a bool parameter to represent the
> > +      voltage range is 25.6 or not for this chip.
> 
> Existing attributes are adi,vrange-high-enable, ti,extended-range-enable,
> ti,bus-range-microvolt, adi,range-double. I would suggest to use
> adi,vrange-high-enable.
The vrange for this chip is either 102.4 or 25.6, and default is 102.4
So, we think vrange_select_25p6 may be more appropriate

> 
> > +
> > +  rsense-micro-ohms:
> > +    description:
> > +      Resistor value in micro-Ohm
> 
> I would suggest either shunt-resistor-micro-ohms or
> sense-resistor-micro-ohms as used by other bindings.
We will revise to shunt-resistor-micro-ohms.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - rsense-micro-ohms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        status = "okay";
> > +        ltc4286@40 {
> > +            compatible = "lltc,ltc4286";
> > +            reg = <0x40>;
> > +            vrange_select_25p6;
> > +            rsense-micro-ohms = <300>;
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > c6545eb54104..930bdba0f73c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12183,6 +12183,15 @@ S:   Maintained
> >  F:   Documentation/hwmon/ltc4261.rst
> >  F:   drivers/hwmon/ltc4261.c
> >
> > +LTC4286 HARDWARE MONITOR DRIVER
> > +M:   Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +L:   linux-i2c@vger.kernel.org
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > +F:   drivers/hwmon/pmbus/Kconfig
> > +F:   drivers/hwmon/pmbus/Makefile
> > +F:   drivers/hwmon/pmbus/ltc4286.c
> > +
> >  LTC4306 I2C MULTIPLEXER DRIVER
> >  M:   Michael Hennerich <michael.hennerich@analog.com>
> >  L:   linux-i2c@vger.kernel.org
> > --
> > 2.17.1
> >

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On 7/23/23 19:12, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:

>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - lltc,ltc4286
>>> +      - lltc,ltc4287
>>
>> There is no LTC4287, at least according to the Analog website.
> It has been announced on Analog Devices website.
> Please refer to this link: https://www.analog.com/en/products/ltc2487.html#product-overview
> 

No, that is wrong. You are pointing to ltc2487, which is something
completely different.

Guenter

RE: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, July 24, 2023 11:22 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 7/23/23 19:12, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> 
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - lltc,ltc4286
> >>> +      - lltc,ltc4287
> >>
> >> There is no LTC4287, at least according to the Analog website.
> > It has been announced on Analog Devices website.
> > Please refer to this link:
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> >
> analog.com%2Fen%2Fproducts%2Fltc2487.html%23product-overview&data=0
> 5%7
> >
> C01%7CWayne_SC_Liu%40wiwynn.com%7Cd97a86a696a54f28a26408db8bf52
> 23d%7Cd
> >
> a6e0628fc834caf9dd273061cbab167%7C0%7C0%7C638257657193005539%7C
> Unknown
> >
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJ
> >
> XVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XrdlzCyq0pcjfv3M6QNX73Ieux0w
> rfNKzNvv
> > HgVSH40%3D&reserved=0
> >
> 
> No, that is wrong. You are pointing to ltc2487, which is something completely
> different.
> 

We have sent e-mail to query about the release date for LTC4287 chip.
Analog Device reply that they will release this chip in last week of Sep, 2023.
Please refer to the attachment to review their reply.

> Guenter


[-- Attachment #2: Type: message/rfc822, Size: 10215 bytes --]

From: "Tai, Jason" <Jason.Tai@analog.com>
To: Wayne SC Liu/WYHQ/Wiwynn <Wayne_SC_Liu@wiwynn.com>
Cc: "openbmc_compute@meta.com" <openbmc_compute@meta.com>, Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
Subject: RE: The release date about LTC4287
Date: Mon, 31 Jul 2023 02:54:33 +0000
Message-ID: <PH0PR03MB706741CB9D9833AE006E39C39405A@PH0PR03MB7067.namprd03.prod.outlook.com>

  Security Reminder: Please be aware that this email is sent by an external sender.

Hi Wayne,

To be specific, last week of Sep, 2023. Thanks.



Regards,

Jason Tai
Senior Engineer, Field Applications
Mobile     (+886) (0) 922-226-527



-----Original Message-----
From: Wayne SC Liu/WYHQ/Wiwynn <Wayne_SC_Liu@wiwynn.com>
Sent: Tuesday, July 25, 2023 1:19 PM
To: Tai, Jason <Jason.Tai@analog.com>
Cc: openbmc_compute@meta.com; Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
Subject: RE: The release date about LTC4287

[External]

Loop more Wiwynn BMC member

> -----Original Message-----
> From: Wayne SC Liu/WYHQ/Wiwynn
> Sent: Tuesday, July 25, 2023 10:40 AM
> To: 'Jason.Tai@analog.com' <Jason.Tai@analog.com>
> Cc: 'openbmc_compute@meta.com' <openbmc_compute@meta.com>; Delphine
> Chiu/WYHQ/Wiwynn <DELPHINE_CHIU@wiwynn.com>; Bonnie Lo/WYHQ/Wiwynn
> <Bonnie_Lo@wiwynn.com>; Sara SY Lin/WYHQ/Wiwynn
> <Sara_SY_Lin@wiwynn.com>; Ricky CX Wu/WYHQ/Wiwynn
> <Ricky_CX_Wu@wiwynn.com>; Jeff Shih/WYHQ/Wiwynn <Jeff_Shih@wiwynn.com>
> Subject: The release date about LTC4287
>
> Hi Jason,
>
> I am Wiwynn BMC Wayne.
>
> We have contributed the Linux driver for LTC4286 and LTC4287 chips.
>
> However, Linux reviewer doesn't agree with our contribution because
> LTC4287 chip still not release on Aanlog Device website.
> (Please refer to the attachment)
>
> Moreover, our customer Meta plans to use LTC4287 in next generation
> product so we need driver to access this device.
>
> Do you have the exact release date about this chip?
>
> Thanks.
> Wayne
>

WIWYNN PROPRIETARY
This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately.

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On 8/1/23 22:31, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Monday, July 24, 2023 11:22 AM
>> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
>> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
>> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
>> bindings
>>
>>    Security Reminder: Please be aware that this email is sent by an external
>> sender.
>>
>> On 7/23/23 19:12, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - lltc,ltc4286
>>>>> +      - lltc,ltc4287
>>>>
>>>> There is no LTC4287, at least according to the Analog website.
>>> It has been announced on Analog Devices website.
>>> Please refer to this link:
>>> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
>>>
>> analog.com%2Fen%2Fproducts%2Fltc2487.html%23product-overview&data=0
>> 5%7
>>>
>> C01%7CWayne_SC_Liu%40wiwynn.com%7Cd97a86a696a54f28a26408db8bf52
>> 23d%7Cd
>>>
>> a6e0628fc834caf9dd273061cbab167%7C0%7C0%7C638257657193005539%7C
>> Unknown
>>>
>> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
>> wiLCJ
>>>
>> XVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XrdlzCyq0pcjfv3M6QNX73Ieux0w
>> rfNKzNvv
>>> HgVSH40%3D&reserved=0
>>>
>>
>> No, that is wrong. You are pointing to ltc2487, which is something completely
>> different.
>>
> 
> We have sent e-mail to query about the release date for LTC4287 chip.
> Analog Device reply that they will release this chip in last week of Sep, 2023.
> Please refer to the attachment to review their reply.
> 

At least according to the driver code, LTC4286 and the LTC4287 are functionally
identical. I am kind of puzzled why you insist mentioning the not-yet-existing
LTC4287; instantiating LTC4287 as LTC4286 should work perfectly fine. LTC4287
can always be added as devicetree reference if/when it officially exists.

Care to explain ?

Note: If the chips are _not_ functionally identical, and a follow-up patch will
be needed to add full LTC4287 support to the driver after the chip has been
published, it would be inappropriate to make partial/incomplete changes now.
With that possibility in mind, I am not inclined to accept a driver that is
even mentioning LTC4287 without access to its datasheet because I think it
is important for me to understand the differences and similarities between
the two chips.

Guenter

[PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine CC Chiu]

Add support for ltc4286 driver

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 drivers/hwmon/pmbus/Kconfig   |   9 +++
 drivers/hwmon/pmbus/Makefile  |   1 +
 drivers/hwmon/pmbus/ltc4286.c | 142 ++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/ltc4286.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 59d9a7430499..1230d910d681 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -218,6 +218,15 @@ config SENSORS_LTC3815
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc3815.
 
+config SENSORS_LTC4286
+	bool "Linear Technologies LTC4286"
+	help
+	  If you say yes here you get hardware monitoring support for Linear
+	  Technology LTC4286.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc4286.
+
 config SENSORS_MAX15301
 	tristate "Maxim MAX15301"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 3ae019916267..540265539580 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
 obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
 obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
 obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
+obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
 obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
 obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
 obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
new file mode 100644
index 000000000000..474f4ec9107c
--- /dev/null
+++ b/drivers/hwmon/pmbus/ltc4286.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for LTC4286 Hot-Swap Controller
+ *
+ * Copyright (c) 2023 Linear Technology
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+// LTC4286 register
+#define LTC4286_MFR_CONFIG1 (0xF2)
+
+// LTC4286 configuration
+#define VRANGE_SELECT (1 << 1)
+
+#define LTC4286_MFR_ID_SIZE 3
+
+enum chips { ltc4286, ltc4287 };
+
+static struct pmbus_driver_info ltc4286_info = {
+	.pages = 1,
+	.format[PSC_VOLTAGE_IN] = direct,
+	.format[PSC_VOLTAGE_OUT] = direct,
+	.format[PSC_CURRENT_OUT] = direct,
+	.format[PSC_POWER] = direct,
+	.format[PSC_TEMPERATURE] = direct,
+	.m[PSC_VOLTAGE_IN] = 32,
+	.b[PSC_VOLTAGE_IN] = 0,
+	.R[PSC_VOLTAGE_IN] = 1,
+	.m[PSC_VOLTAGE_OUT] = 32,
+	.b[PSC_VOLTAGE_OUT] = 0,
+	.R[PSC_VOLTAGE_OUT] = 1,
+	.m[PSC_CURRENT_OUT] = 1024,
+	.b[PSC_CURRENT_OUT] = 0,
+	.R[PSC_CURRENT_OUT] = 3 - 6,
+	.m[PSC_POWER] = 1,
+	.b[PSC_POWER] = 0,
+	.R[PSC_POWER] = 4 - 6,
+	.m[PSC_TEMPERATURE] = 1,
+	.b[PSC_TEMPERATURE] = 273.15,
+	.R[PSC_TEMPERATURE] = 0,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,
+};
+
+static int ltc4286_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	int ret;
+	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
+	struct device *dev = &client->dev;
+	struct pmbus_driver_info *info;
+	u32 rsense;
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer id\n");
+		return ret;
+	}
+
+	/* Refer to ltc4286 datasheet page 20
+	 * the default manufacturer id is LTC
+	 */
+	if (ret != LTC4286_MFR_ID_SIZE ||
+	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
+		dev_err(&client->dev, "unsupported manufacturer id\n");
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer model\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(client->dev.of_node, "rsense-micro-ohms",
+				   &rsense);
+	if (ret < 0)
+		return ret;
+
+	if (rsense == 0)
+		return -EINVAL;
+
+	info = &ltc4286_info;
+
+	/* Default of VRANGE_SELECT = 1 */
+	if (device_property_read_bool(dev, "vrange_select_25p6")) {
+		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
+		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"failed to read manufacturer configuration one\n");
+			return ret;
+		}
+
+		ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
+		i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);
+
+		info->m[PSC_VOLTAGE_IN] = 128;
+		info->m[PSC_VOLTAGE_OUT] = 128;
+		info->m[PSC_POWER] = 4 * rsense;
+	} else {
+		info->m[PSC_POWER] = rsense;
+	}
+
+	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
+
+	return pmbus_do_probe(client, info);
+}
+
+static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
+						   { "ltc4287", ltc4287 },
+						   {} };
+MODULE_DEVICE_TABLE(i2c, ltc4286_id);
+
+static const struct of_device_id ltc4286_of_match[] = {
+	{ .compatible = "lltc,ltc4286" },
+	{ .compatible = "lltc,ltc4287" },
+	{}
+};
+
+/* This is the driver that will be inserted */
+static struct i2c_driver ltc4286_driver = {
+	.driver = {
+		.name = "ltc4286",
+		.of_match_table = ltc4286_of_match,
+	},
+	.probe = ltc4286_probe,
+	.id_table = ltc4286_id,
+};
+
+module_i2c_driver(ltc4286_driver);
+
+MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
+MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On Mon, Apr 24, 2023 at 06:13:50PM +0800, Delphine CC Chiu wrote:
> Add support for ltc4286 driver

The patch does not add support for a driver, it adds support for a chip.

> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  drivers/hwmon/pmbus/Kconfig   |   9 +++
>  drivers/hwmon/pmbus/Makefile  |   1 +
>  drivers/hwmon/pmbus/ltc4286.c | 142 ++++++++++++++++++++++++++++++++++

Documentation is missing.

>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 59d9a7430499..1230d910d681 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -218,6 +218,15 @@ config SENSORS_LTC3815
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc3815.
>  
> +config SENSORS_LTC4286
> +	bool "Linear Technologies LTC4286"

Analog Devices ?

> +	help
> +	  If you say yes here you get hardware monitoring support for Linear
> +	  Technology LTC4286.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc4286.
> +
>  config SENSORS_MAX15301
>  	tristate "Maxim MAX15301"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 3ae019916267..540265539580 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>  obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
>  obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>  obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
> +obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
>  obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
>  obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
>  obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
> diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
> new file mode 100644
> index 000000000000..474f4ec9107c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ltc4286.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for LTC4286 Hot-Swap Controller
> + *
> + * Copyright (c) 2023 Linear Technology

Really ?

> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +// LTC4286 register

Please no C++ comments in the code.

> +#define LTC4286_MFR_CONFIG1 (0xF2)

Unnecessary ( )

> +
> +// LTC4286 configuration
> +#define VRANGE_SELECT (1 << 1)

#define<space>DEFINE<tab>value, please. Also, please use BIT() macros
where appropriate.
> +
> +#define LTC4286_MFR_ID_SIZE 3
> +
> +enum chips { ltc4286, ltc4287 };

There is no LTC4287 according to information available in public.
It has not even be announced.

Besides, the above enum is not really used anywhere and therefore
has zero value. Maybe the LTC4287 is not yet released. Even then,
there is no value listing it here because its parameters seem
to be identical to LTC4286.

> +
> +static struct pmbus_driver_info ltc4286_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.format[PSC_TEMPERATURE] = direct,
> +	.m[PSC_VOLTAGE_IN] = 32,
> +	.b[PSC_VOLTAGE_IN] = 0,
> +	.R[PSC_VOLTAGE_IN] = 1,
> +	.m[PSC_VOLTAGE_OUT] = 32,
> +	.b[PSC_VOLTAGE_OUT] = 0,
> +	.R[PSC_VOLTAGE_OUT] = 1,
> +	.m[PSC_CURRENT_OUT] = 1024,
> +	.b[PSC_CURRENT_OUT] = 0,
> +	.R[PSC_CURRENT_OUT] = 3 - 6,

This needs explanation.

> +	.m[PSC_POWER] = 1,
> +	.b[PSC_POWER] = 0,
> +	.R[PSC_POWER] = 4 - 6,

This needs explanation.

> +	.m[PSC_TEMPERATURE] = 1,
> +	.b[PSC_TEMPERATURE] = 273.15,

Assigning a float to an int doesn't make much sense.

> +	.R[PSC_TEMPERATURE] = 0,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> +		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,

The chip does have a number of status registers.

> +};
> +
> +static int ltc4286_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct device *dev = &client->dev;
> +	struct pmbus_driver_info *info;
> +	u32 rsense;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer id\n");

Kind of pointless to declare a local 'dev' variable and not use it.

> +		return ret;
> +	}
> +
> +	/* Refer to ltc4286 datasheet page 20
> +	 * the default manufacturer id is LTC

Why "default" ?

> +	 */

Please use standard multi-line comments.

> +	if (ret != LTC4286_MFR_ID_SIZE ||
> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> +		dev_err(&client->dev, "unsupported manufacturer id\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer model\n");
> +		return ret;
> +	}

Why read the model if you don't do anything with it ?

> +
> +	ret = of_property_read_u32(client->dev.of_node, "rsense-micro-ohms",
> +				   &rsense);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (rsense == 0)
> +		return -EINVAL;
> +

There should be a default for systems not supporting devicetree.

> +	info = &ltc4286_info;
> +
> +	/* Default of VRANGE_SELECT = 1 */
> +	if (device_property_read_bool(dev, "vrange_select_25p6")) {
> +		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> +		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"failed to read manufacturer configuration one\n");
> +			return ret;
> +		}
> +
> +		ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
> +		i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);

Error check missing.

> +
> +		info->m[PSC_VOLTAGE_IN] = 128;
> +		info->m[PSC_VOLTAGE_OUT] = 128;
> +		info->m[PSC_POWER] = 4 * rsense;
> +	} else {
> +		info->m[PSC_POWER] = rsense;

This just takes the current configuration, and assumes it is the default.
That may not be correct. The chip may have been configured by the BIOS,
or manually.

> +	}
> +

The code assumes that there is only a single chip in the system, or that
the configuration of all chips is identical. This is not necessarily
correct.

> +	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> +						   { "ltc4287", ltc4287 },

Even if LTC4287 existed, assigning the ID here ...

> +						   {} };
> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> +
> +static const struct of_device_id ltc4286_of_match[] = {
> +	{ .compatible = "lltc,ltc4286" },
> +	{ .compatible = "lltc,ltc4287" },

... but not here defeats having it in the first place.

> +	{}
> +};

MODULE_DEVOCE_TABLE() missing.

> +
> +/* This is the driver that will be inserted */

Useless comment

> +static struct i2c_driver ltc4286_driver = {
> +	.driver = {
> +		.name = "ltc4286",
> +		.of_match_table = ltc4286_of_match,
> +	},
> +	.probe = ltc4286_probe,
> +	.id_table = ltc4286_id,
> +};
> +
> +module_i2c_driver(ltc4286_driver);
> +
> +MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.1
> 

RE: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, April 24, 2023 10:14 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On Mon, Apr 24, 2023 at 06:13:50PM +0800, Delphine CC Chiu wrote:
> > Add support for ltc4286 driver
> 
> The patch does not add support for a driver, it adds support for a chip.
Add a driver to support ltc4286 chip

> 
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > ---
> >  drivers/hwmon/pmbus/Kconfig   |   9 +++
> >  drivers/hwmon/pmbus/Makefile  |   1 +
> >  drivers/hwmon/pmbus/ltc4286.c | 142
> > ++++++++++++++++++++++++++++++++++
> 
> Documentation is missing.
Documentation is in patch one ([PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings)

> 
> >  3 files changed, 152 insertions(+)
> >  create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> >
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 59d9a7430499..1230d910d681 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -218,6 +218,15 @@ config SENSORS_LTC3815
> >         This driver can also be built as a module. If so, the module will
> >         be called ltc3815.
> >
> > +config SENSORS_LTC4286
> > +     bool "Linear Technologies LTC4286"
> 
> Analog Devices ?
We will revise to "Analog Devices LTC4286"

> 
> > +     help
> > +       If you say yes here you get hardware monitoring support for Linear
> > +       Technology LTC4286.
> > +
> > +       This driver can also be built as a module. If so, the module will
> > +       be called ltc4286.
> > +
> >  config SENSORS_MAX15301
> >       tristate "Maxim MAX15301"
> >       help
> > diff --git a/drivers/hwmon/pmbus/Makefile
> > b/drivers/hwmon/pmbus/Makefile index 3ae019916267..540265539580
> 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)       +=
> lm25066.o
> >  obj-$(CONFIG_SENSORS_LT7182S)        += lt7182s.o
> >  obj-$(CONFIG_SENSORS_LTC2978)        += ltc2978.o
> >  obj-$(CONFIG_SENSORS_LTC3815)        += ltc3815.o
> > +obj-$(CONFIG_SENSORS_LTC4286)        += ltc4286.o
> >  obj-$(CONFIG_SENSORS_MAX15301)       += max15301.o
> >  obj-$(CONFIG_SENSORS_MAX16064)       += max16064.o
> >  obj-$(CONFIG_SENSORS_MAX16601)       += max16601.o
> > diff --git a/drivers/hwmon/pmbus/ltc4286.c
> > b/drivers/hwmon/pmbus/ltc4286.c new file mode 100644 index
> > 000000000000..474f4ec9107c
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/ltc4286.c
> > @@ -0,0 +1,142 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Hardware monitoring driver for LTC4286 Hot-Swap Controller
> > + *
> > + * Copyright (c) 2023 Linear Technology
> 
> Really ?
We will remove the copyright declaration as it was false reference from other drivers
Chip vendor doesn't support this driver, so we implement it for our own use and contribute it as Wiwynn is a system manufacturer, our server products use this chip.

> 
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pmbus.h>
> > +#include "pmbus.h"
> > +
> > +// LTC4286 register
> 
> Please no C++ comments in the code.
We will revise to /* LTC4286 register */

> 
> > +#define LTC4286_MFR_CONFIG1 (0xF2)
> 
> Unnecessary ( )
We will revise to #define LTC4286_MFR_CONFIG1 0xF2

> 
> > +
> > +// LTC4286 configuration
> > +#define VRANGE_SELECT (1 << 1)
> 
> #define<space>DEFINE<tab>value, please. Also, please use BIT() macros where
> appropriate.
We will revise to below.
#define VRANGE_SELECT_BIT	BIT(1)

> > +
> > +#define LTC4286_MFR_ID_SIZE 3
> > +
> > +enum chips { ltc4286, ltc4287 };
> 
> There is no LTC4287 according to information available in public.
> It has not even be announced.
> 
> Besides, the above enum is not really used anywhere and therefore has zero
> value. Maybe the LTC4287 is not yet released. Even then, there is no value
> listing it here because its parameters seem to be identical to LTC4286.
It has been announced on Analog Devices website.
Please refer to this link: https://www.analog.com/en/products/ltc2487.html#product-overview
enum chips { ltc4286 = 0, ltc4287 };
Use in v1 line 118 to list chip index instead of hardcoding

> 
> > +
> > +static struct pmbus_driver_info ltc4286_info = {
> > +     .pages = 1,
> > +     .format[PSC_VOLTAGE_IN] = direct,
> > +     .format[PSC_VOLTAGE_OUT] = direct,
> > +     .format[PSC_CURRENT_OUT] = direct,
> > +     .format[PSC_POWER] = direct,
> > +     .format[PSC_TEMPERATURE] = direct,
> > +     .m[PSC_VOLTAGE_IN] = 32,
> > +     .b[PSC_VOLTAGE_IN] = 0,
> > +     .R[PSC_VOLTAGE_IN] = 1,
> > +     .m[PSC_VOLTAGE_OUT] = 32,
> > +     .b[PSC_VOLTAGE_OUT] = 0,
> > +     .R[PSC_VOLTAGE_OUT] = 1,
> > +     .m[PSC_CURRENT_OUT] = 1024,
> > +     .b[PSC_CURRENT_OUT] = 0,
> > +     .R[PSC_CURRENT_OUT] = 3 - 6,
> 
> This needs explanation.
We will add comments as below
/* Initialize the MBR as default settings which is referred to LTC4286 datasheet(March 22, 2022 version) table 3 page 16 */

> 
> > +     .m[PSC_POWER] = 1,
> > +     .b[PSC_POWER] = 0,
> > +     .R[PSC_POWER] = 4 - 6,
> 
> This needs explanation.
We will add comments as below
/* To support small shunt resistor value */

> 
> > +     .m[PSC_TEMPERATURE] = 1,
> > +     .b[PSC_TEMPERATURE] = 273.15,
> 
> Assigning a float to an int doesn't make much sense.
We will revise to .b[PSC_TEMPERATURE] = 273,
However the value for this MBR is 273.15 in datasheet
We use 273 due to pmbus code limitation

> 
> > +     .R[PSC_TEMPERATURE] = 0,
> > +     .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT |
> PMBUS_HAVE_IOUT |
> > +                PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,
> 
> The chip does have a number of status registers.
We will add status registers here

> 
> > +};
> > +
> > +static int ltc4286_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *id) {
> > +     int ret;
> > +     u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> > +     struct device *dev = &client->dev;
> > +     struct pmbus_driver_info *info;
> > +     u32 rsense;
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer
> > + id\n");
> 
> Kind of pointless to declare a local 'dev' variable and not use it.
We will drop it

> 
> > +             return ret;
> > +     }
> > +
> > +     /* Refer to ltc4286 datasheet page 20
> > +      * the default manufacturer id is LTC
> 
> Why "default" ?
We will revise to 
/* 
 * Refer to ltc4286 datasheet page 20
 * the manufacturer id is LTC
 */

> 
> > +      */
> 
> Please use standard multi-line comments.
We will revise to
/* 
 * Refer to ltc4286 datasheet page 20
 * the manufacturer id is LTC
 */

> 
> > +     if (ret != LTC4286_MFR_ID_SIZE ||
> > +         strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> > +             dev_err(&client->dev, "unsupported manufacturer id\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer
> model\n");
> > +             return ret;
> > +     }
> 
> Why read the model if you don't do anything with it ?
We will add comaprision here.
for (mid = ltc4286_id; mid->name[0]; mid++) {
	if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
		break;
}

> 
> > +
> > +     ret = of_property_read_u32(client->dev.of_node,
> "rsense-micro-ohms",
> > +                                &rsense);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (rsense == 0)
> > +             return -EINVAL;
> > +
> 
> There should be a default for systems not supporting devicetree.
After confirming with vendor, they said there is no default rsesne for this chip
The value for rsense depends on hardware engineer in each manufacturer

> 
> > +     info = &ltc4286_info;
> > +
> > +     /* Default of VRANGE_SELECT = 1 */
> > +     if (device_property_read_bool(dev, "vrange_select_25p6")) {
> > +             /* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> > +             ret = i2c_smbus_read_word_data(client,
> LTC4286_MFR_CONFIG1);
> > +             if (ret < 0) {
> > +                     dev_err(&client->dev,
> > +                             "failed to read manufacturer
> configuration one\n");
> > +                     return ret;
> > +             }
> > +
> > +             ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
> > +             i2c_smbus_write_word_data(client,
> LTC4286_MFR_CONFIG1,
> > + ret);
> 
> Error check missing.
ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);
if (ret < 0) {
	dev_err(&client->dev, "failed to set vrange\n");
     return ret;
}

> 
> > +
> > +             info->m[PSC_VOLTAGE_IN] = 128;
> > +             info->m[PSC_VOLTAGE_OUT] = 128;
> > +             info->m[PSC_POWER] = 4 * rsense;
> > +     } else {
> > +             info->m[PSC_POWER] = rsense;
> 
> This just takes the current configuration, and assumes it is the default.
> That may not be correct. The chip may have been configured by the BIOS, or
> manually.
The MBR values are based on hardware design, so it must be set in initial stage

> 
> > +     }
> > +
> 
> The code assumes that there is only a single chip in the system, or that the
> configuration of all chips is identical. This is not necessarily correct.
If there are more than one LTC4286 or LTC4287 chips, user can add the configuration for different chips in dts file

> 
> > +     info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> > +
> > +     return pmbus_do_probe(client, info); }
> > +
> > +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> > +                                                { "ltc4287",
> ltc4287
> > +},
> 
> Even if LTC4287 existed, assigning the ID here ...
So do you recommend us to put this enum (enum chips { ltc4286, ltc4287 };) here?

> 
> > +                                                {} };
> > +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> > +
> > +static const struct of_device_id ltc4286_of_match[] = {
> > +     { .compatible = "lltc,ltc4286" },
> > +     { .compatible = "lltc,ltc4287" },
> 
> ... but not here defeats having it in the first place.
So do you recommend us to not put this enum (enum chips { ltc4286, ltc4287 };) here?

> 
> > +     {}
> > +};
> 
> MODULE_DEVOCE_TABLE() missing.
In v1 line 120

> 
> > +
> > +/* This is the driver that will be inserted */
> 
> Useless comment
We will drop it.

> 
> > +static struct i2c_driver ltc4286_driver = {
> > +     .driver = {
> > +             .name = "ltc4286",
> > +             .of_match_table = ltc4286_of_match,
> > +     },
> > +     .probe = ltc4286_probe,
> > +     .id_table = ltc4286_id,
> > +};
> > +
> > +module_i2c_driver(ltc4286_driver);
> > +
> > +MODULE_AUTHOR("Delphine CC Chiu
> <Delphine_CC_Chiu@wiwynn.com>");
> > +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.17.1
> >

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 7/23/23 23:03, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Monday, April 24, 2023 10:14 PM
>> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
>> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
>> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
>>
>>    Security Reminder: Please be aware that this email is sent by an external
>> sender.
>>
>> On Mon, Apr 24, 2023 at 06:13:50PM +0800, Delphine CC Chiu wrote:
>>> Add support for ltc4286 driver
>>
>> The patch does not add support for a driver, it adds support for a chip.
> Add a driver to support ltc4286 chip
> 
>>
>>>
>>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>> ---
>>>   drivers/hwmon/pmbus/Kconfig   |   9 +++
>>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>>   drivers/hwmon/pmbus/ltc4286.c | 142
>>> ++++++++++++++++++++++++++++++++++
>>
>> Documentation is missing.
> Documentation is in patch one ([PATCH v1 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings)
> 

Documentation/hwmon/ltc4286.rst is expected in this patch and not as
part of the bindings. The bindings do not document the driver,
they document the chip bindings.

>>
>>>   3 files changed, 152 insertions(+)
>>>   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
>>>
>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>> index 59d9a7430499..1230d910d681 100644
>>> --- a/drivers/hwmon/pmbus/Kconfig
>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>> @@ -218,6 +218,15 @@ config SENSORS_LTC3815
>>>          This driver can also be built as a module. If so, the module will
>>>          be called ltc3815.
>>>
>>> +config SENSORS_LTC4286
>>> +     bool "Linear Technologies LTC4286"
>>
>> Analog Devices ?
> We will revise to "Analog Devices LTC4286"
> 
>>
>>> +     help
>>> +       If you say yes here you get hardware monitoring support for Linear
>>> +       Technology LTC4286.
>>> +
>>> +       This driver can also be built as a module. If so, the module will
>>> +       be called ltc4286.
>>> +
>>>   config SENSORS_MAX15301
>>>        tristate "Maxim MAX15301"
>>>        help
>>> diff --git a/drivers/hwmon/pmbus/Makefile
>>> b/drivers/hwmon/pmbus/Makefile index 3ae019916267..540265539580
>> 100644
>>> --- a/drivers/hwmon/pmbus/Makefile
>>> +++ b/drivers/hwmon/pmbus/Makefile
>>> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)       +=
>> lm25066.o
>>>   obj-$(CONFIG_SENSORS_LT7182S)        += lt7182s.o
>>>   obj-$(CONFIG_SENSORS_LTC2978)        += ltc2978.o
>>>   obj-$(CONFIG_SENSORS_LTC3815)        += ltc3815.o
>>> +obj-$(CONFIG_SENSORS_LTC4286)        += ltc4286.o
>>>   obj-$(CONFIG_SENSORS_MAX15301)       += max15301.o
>>>   obj-$(CONFIG_SENSORS_MAX16064)       += max16064.o
>>>   obj-$(CONFIG_SENSORS_MAX16601)       += max16601.o
>>> diff --git a/drivers/hwmon/pmbus/ltc4286.c
>>> b/drivers/hwmon/pmbus/ltc4286.c new file mode 100644 index
>>> 000000000000..474f4ec9107c
>>> --- /dev/null
>>> +++ b/drivers/hwmon/pmbus/ltc4286.c
>>> @@ -0,0 +1,142 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Hardware monitoring driver for LTC4286 Hot-Swap Controller
>>> + *
>>> + * Copyright (c) 2023 Linear Technology
>>
>> Really ?
> We will remove the copyright declaration as it was false reference from other drivers
> Chip vendor doesn't support this driver, so we implement it for our own use and contribute it as Wiwynn is a system manufacturer, our server products use this chip.
> 
>>
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pmbus.h>
>>> +#include "pmbus.h"
>>> +
>>> +// LTC4286 register
>>
>> Please no C++ comments in the code.
> We will revise to /* LTC4286 register */
> 
>>
>>> +#define LTC4286_MFR_CONFIG1 (0xF2)
>>
>> Unnecessary ( )
> We will revise to #define LTC4286_MFR_CONFIG1 0xF2
> 
>>
>>> +
>>> +// LTC4286 configuration
>>> +#define VRANGE_SELECT (1 << 1)
>>
>> #define<space>DEFINE<tab>value, please. Also, please use BIT() macros where
>> appropriate.
> We will revise to below.
> #define VRANGE_SELECT_BIT	BIT(1)
> 
>>> +
>>> +#define LTC4286_MFR_ID_SIZE 3
>>> +
>>> +enum chips { ltc4286, ltc4287 };
>>
>> There is no LTC4287 according to information available in public.
>> It has not even be announced.
>>
>> Besides, the above enum is not really used anywhere and therefore has zero
>> value. Maybe the LTC4287 is not yet released. Even then, there is no value
>> listing it here because its parameters seem to be identical to LTC4286.
> It has been announced on Analog Devices website.

No, it has not.

> Please refer to this link: https://www.analog.com/en/products/ltc2487.html#product-overview
> enum chips { ltc4286 = 0, ltc4287 };
> Use in v1 line 118 to list chip index instead of hardcoding
> 

ltc2487 is _not_ ltc4287.
    ^^               ^^

Sure, a Google search for LTC4287 returns a link to LTC2487 (!) as first response,
but LTC2487 is an ADC and has nothing but the manufacturer in common with LTC4286.

LTC4286 High Power Positive Hot-Swap Controller with Power Monitor via PMBus
LTC2487 16-Bit 2-/4-Channel ∆Σ ADC with PGA, Easy Drive and I2C Interface

I _really_ do not understand what you are trying to do here or why.

>>
>>> +
>>> +static struct pmbus_driver_info ltc4286_info = {
>>> +     .pages = 1,
>>> +     .format[PSC_VOLTAGE_IN] = direct,
>>> +     .format[PSC_VOLTAGE_OUT] = direct,
>>> +     .format[PSC_CURRENT_OUT] = direct,
>>> +     .format[PSC_POWER] = direct,
>>> +     .format[PSC_TEMPERATURE] = direct,
>>> +     .m[PSC_VOLTAGE_IN] = 32,
>>> +     .b[PSC_VOLTAGE_IN] = 0,
>>> +     .R[PSC_VOLTAGE_IN] = 1,
>>> +     .m[PSC_VOLTAGE_OUT] = 32,
>>> +     .b[PSC_VOLTAGE_OUT] = 0,
>>> +     .R[PSC_VOLTAGE_OUT] = 1,
>>> +     .m[PSC_CURRENT_OUT] = 1024,
>>> +     .b[PSC_CURRENT_OUT] = 0,
>>> +     .R[PSC_CURRENT_OUT] = 3 - 6,
>>
>> This needs explanation.
> We will add comments as below
> /* Initialize the MBR as default settings which is referred to LTC4286 datasheet(March 22, 2022 version) table 3 page 16 */
> 

That does not explain "3 - 6".

>>
>>> +     .m[PSC_POWER] = 1,
>>> +     .b[PSC_POWER] = 0,
>>> +     .R[PSC_POWER] = 4 - 6,
>>
>> This needs explanation.
> We will add comments as below
> /* To support small shunt resistor value */
> 

That does not explain "4 - 6".


>>
>>> +     .m[PSC_TEMPERATURE] = 1,
>>> +     .b[PSC_TEMPERATURE] = 273.15,
>>
>> Assigning a float to an int doesn't make much sense.
> We will revise to .b[PSC_TEMPERATURE] = 273,
> However the value for this MBR is 273.15 in datasheet
> We use 273 due to pmbus code limitation
> 
>>
>>> +     .R[PSC_TEMPERATURE] = 0,
>>> +     .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT |
>> PMBUS_HAVE_IOUT |
>>> +                PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,
>>
>> The chip does have a number of status registers.
> We will add status registers here
> 
>>
>>> +};
>>> +
>>> +static int ltc4286_probe(struct i2c_client *client,
>>> +                      const struct i2c_device_id *id) {
>>> +     int ret;
>>> +     u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
>>> +     struct device *dev = &client->dev;
>>> +     struct pmbus_driver_info *info;
>>> +     u32 rsense;
>>> +
>>> +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
>> block_buffer);
>>> +     if (ret < 0) {
>>> +             dev_err(&client->dev, "failed to read manufacturer
>>> + id\n");
>>
>> Kind of pointless to declare a local 'dev' variable and not use it.
> We will drop it
> 
>>
>>> +             return ret;
>>> +     }
>>> +
>>> +     /* Refer to ltc4286 datasheet page 20
>>> +      * the default manufacturer id is LTC
>>
>> Why "default" ?
> We will revise to
> /*
>   * Refer to ltc4286 datasheet page 20
>   * the manufacturer id is LTC
>   */
> 
>>
>>> +      */
>>
>> Please use standard multi-line comments.
> We will revise to
> /*
>   * Refer to ltc4286 datasheet page 20
>   * the manufacturer id is LTC
>   */
> 
>>
>>> +     if (ret != LTC4286_MFR_ID_SIZE ||
>>> +         strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
>>> +             dev_err(&client->dev, "unsupported manufacturer id\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
>> block_buffer);
>>> +     if (ret < 0) {
>>> +             dev_err(&client->dev, "failed to read manufacturer
>> model\n");
>>> +             return ret;
>>> +     }
>>
>> Why read the model if you don't do anything with it ?
> We will add comaprision here.
> for (mid = ltc4286_id; mid->name[0]; mid++) {
> 	if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> 		break;
> }
> 
>>
>>> +
>>> +     ret = of_property_read_u32(client->dev.of_node,
>> "rsense-micro-ohms",
>>> +                                &rsense);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     if (rsense == 0)
>>> +             return -EINVAL;
>>> +
>>
>> There should be a default for systems not supporting devicetree.
> After confirming with vendor, they said there is no default rsesne for this chip
> The value for rsense depends on hardware engineer in each manufacturer
> 

The _driver_ needs a default.

>>
>>> +     info = &ltc4286_info;
>>> +
>>> +     /* Default of VRANGE_SELECT = 1 */
>>> +     if (device_property_read_bool(dev, "vrange_select_25p6")) {
>>> +             /* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
>>> +             ret = i2c_smbus_read_word_data(client,
>> LTC4286_MFR_CONFIG1);
>>> +             if (ret < 0) {
>>> +                     dev_err(&client->dev,
>>> +                             "failed to read manufacturer
>> configuration one\n");
>>> +                     return ret;
>>> +             }
>>> +
>>> +             ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
>>> +             i2c_smbus_write_word_data(client,
>> LTC4286_MFR_CONFIG1,
>>> + ret);
>>
>> Error check missing.
> ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);
> if (ret < 0) {
> 	dev_err(&client->dev, "failed to set vrange\n");
>       return ret;
> }
> 
>>
>>> +
>>> +             info->m[PSC_VOLTAGE_IN] = 128;
>>> +             info->m[PSC_VOLTAGE_OUT] = 128;
>>> +             info->m[PSC_POWER] = 4 * rsense;
>>> +     } else {
>>> +             info->m[PSC_POWER] = rsense;
>>
>> This just takes the current configuration, and assumes it is the default.
>> That may not be correct. The chip may have been configured by the BIOS, or
>> manually.
> The MBR values are based on hardware design, so it must be set in initial stage
> 
>>
>>> +     }
>>> +
>>
>> The code assumes that there is only a single chip in the system, or that the
>> configuration of all chips is identical. This is not necessarily correct.
> If there are more than one LTC4286 or LTC4287 chips, user can add the configuration for different chips in dts file
> 

"info" is a pointer to a static variable.

	info = &ltc4286_info;
	
In this context, what you are saying above does not make sense. The
second chip's configuration will override the first chip's configuration.

	info->m[PSC_POWER] = 4 * rsense;
	...


No, sorry, this still does not make sense.


>>
>>> +     info->m[PSC_CURRENT_OUT] = 1024 * rsense;
>>> +
>>> +     return pmbus_do_probe(client, info); }
>>> +
>>> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
>>> +                                                { "ltc4287",
>> ltc4287
>>> +},
>>
>> Even if LTC4287 existed, assigning the ID here ...
> So do you recommend us to put this enum (enum chips { ltc4286, ltc4287 };) here?
> 
>>
>>> +                                                {} };
>>> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
>>> +
>>> +static const struct of_device_id ltc4286_of_match[] = {
>>> +     { .compatible = "lltc,ltc4286" },
>>> +     { .compatible = "lltc,ltc4287" },
>>
>> ... but not here defeats having it in the first place.
> So do you recommend us to not put this enum (enum chips { ltc4286, ltc4287 };) here?
> 

Please read my comment. I am saying that it does not make sense to provide the chip ID
to i2c_device_id but not in of_device_id. Either add it do both if needed or not at
all. I am not telling which one, I am just asking for consistency.

However, as mentioned earlier, according to your code the chips supposedly have identical
functionality. Separate chip IDs (ltc4286 and ltc4287) therefore seem unnecessary (not
even counting the fact that ltc4287 still does not seem to exist).


>>
>>> +     {}
>>> +};
>>
>> MODULE_DEVOCE_TABLE() missing.
> In v1 line 120
> 
>>
>>> +
>>> +/* This is the driver that will be inserted */
>>
>> Useless comment
> We will drop it.
> 
>>
>>> +static struct i2c_driver ltc4286_driver = {
>>> +     .driver = {
>>> +             .name = "ltc4286",
>>> +             .of_match_table = ltc4286_of_match,
>>> +     },
>>> +     .probe = ltc4286_probe,
>>> +     .id_table = ltc4286_id,
>>> +};
>>> +
>>> +module_i2c_driver(ltc4286_driver);
>>> +
>>> +MODULE_AUTHOR("Delphine CC Chiu
>> <Delphine_CC_Chiu@wiwynn.com>");
>>> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.17.1
>>>

RE: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

[-- Attachment #1: Type: text/plain, Size: 18040 bytes --]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, July 24, 2023 2:57 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 7/23/23 23:03, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> >> -----Original Message-----
> >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> >> Sent: Monday, April 24, 2023 10:14 PM
> >> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> >> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> >> <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> >> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> >>
> >>    Security Reminder: Please be aware that this email is sent by an
> >> external sender.
> >>
> >> On Mon, Apr 24, 2023 at 06:13:50PM +0800, Delphine CC Chiu wrote:
> >>> Add support for ltc4286 driver
> >>
> >> The patch does not add support for a driver, it adds support for a chip.
> > Add a driver to support ltc4286 chip
> >
> >>
> >>>
> >>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >>> ---
> >>>   drivers/hwmon/pmbus/Kconfig   |   9 +++
> >>>   drivers/hwmon/pmbus/Makefile  |   1 +
> >>>   drivers/hwmon/pmbus/ltc4286.c | 142
> >>> ++++++++++++++++++++++++++++++++++
> >>
> >> Documentation is missing.
> > Documentation is in patch one ([PATCH v1 1/2] dt-bindings: hwmon: Add
> > lltc ltc4286 driver bindings)
> >
> 
> Documentation/hwmon/ltc4286.rst is expected in this patch and not as part of
> the bindings. The bindings do not document the driver, they document the chip
> bindings.

We will add ltc4286.rst

> 
> >>
> >>>   3 files changed, 152 insertions(+)
> >>>   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> >>>
> >>> diff --git a/drivers/hwmon/pmbus/Kconfig
> >>> b/drivers/hwmon/pmbus/Kconfig index 59d9a7430499..1230d910d681
> >>> 100644
> >>> --- a/drivers/hwmon/pmbus/Kconfig
> >>> +++ b/drivers/hwmon/pmbus/Kconfig
> >>> @@ -218,6 +218,15 @@ config SENSORS_LTC3815
> >>>          This driver can also be built as a module. If so, the module will
> >>>          be called ltc3815.
> >>>
> >>> +config SENSORS_LTC4286
> >>> +     bool "Linear Technologies LTC4286"
> >>
> >> Analog Devices ?
> > We will revise to "Analog Devices LTC4286"
> >
> >>
> >>> +     help
> >>> +       If you say yes here you get hardware monitoring support for
> Linear
> >>> +       Technology LTC4286.
> >>> +
> >>> +       This driver can also be built as a module. If so, the module will
> >>> +       be called ltc4286.
> >>> +
> >>>   config SENSORS_MAX15301
> >>>        tristate "Maxim MAX15301"
> >>>        help
> >>> diff --git a/drivers/hwmon/pmbus/Makefile
> >>> b/drivers/hwmon/pmbus/Makefile index 3ae019916267..540265539580
> >> 100644
> >>> --- a/drivers/hwmon/pmbus/Makefile
> >>> +++ b/drivers/hwmon/pmbus/Makefile
> >>> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)       +=
> >> lm25066.o
> >>>   obj-$(CONFIG_SENSORS_LT7182S)        += lt7182s.o
> >>>   obj-$(CONFIG_SENSORS_LTC2978)        += ltc2978.o
> >>>   obj-$(CONFIG_SENSORS_LTC3815)        += ltc3815.o
> >>> +obj-$(CONFIG_SENSORS_LTC4286)        += ltc4286.o
> >>>   obj-$(CONFIG_SENSORS_MAX15301)       += max15301.o
> >>>   obj-$(CONFIG_SENSORS_MAX16064)       += max16064.o
> >>>   obj-$(CONFIG_SENSORS_MAX16601)       += max16601.o
> >>> diff --git a/drivers/hwmon/pmbus/ltc4286.c
> >>> b/drivers/hwmon/pmbus/ltc4286.c new file mode 100644 index
> >>> 000000000000..474f4ec9107c
> >>> --- /dev/null
> >>> +++ b/drivers/hwmon/pmbus/ltc4286.c
> >>> @@ -0,0 +1,142 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +/*
> >>> + * Hardware monitoring driver for LTC4286 Hot-Swap Controller
> >>> + *
> >>> + * Copyright (c) 2023 Linear Technology
> >>
> >> Really ?
> > We will remove the copyright declaration as it was false reference
> > from other drivers Chip vendor doesn't support this driver, so we implement
> it for our own use and contribute it as Wiwynn is a system manufacturer, our
> server products use this chip.
> >
> >>
> >>> + */
> >>> +
> >>> +#include <linux/err.h>
> >>> +#include <linux/i2c.h>
> >>> +#include <linux/init.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/pmbus.h>
> >>> +#include "pmbus.h"
> >>> +
> >>> +// LTC4286 register
> >>
> >> Please no C++ comments in the code.
> > We will revise to /* LTC4286 register */
> >
> >>
> >>> +#define LTC4286_MFR_CONFIG1 (0xF2)
> >>
> >> Unnecessary ( )
> > We will revise to #define LTC4286_MFR_CONFIG1 0xF2
> >
> >>
> >>> +
> >>> +// LTC4286 configuration
> >>> +#define VRANGE_SELECT (1 << 1)
> >>
> >> #define<space>DEFINE<tab>value, please. Also, please use BIT() macros
> >> where appropriate.
> > We will revise to below.
> > #define VRANGE_SELECT_BIT     BIT(1)
> >
> >>> +
> >>> +#define LTC4286_MFR_ID_SIZE 3
> >>> +
> >>> +enum chips { ltc4286, ltc4287 };
> >>
> >> There is no LTC4287 according to information available in public.
> >> It has not even be announced.
> >>
> >> Besides, the above enum is not really used anywhere and therefore has
> >> zero value. Maybe the LTC4287 is not yet released. Even then, there
> >> is no value listing it here because its parameters seem to be identical to
> LTC4286.
> > It has been announced on Analog Devices website.
> 
> No, it has not.

We have sent e-mail to query about the release date for LTC4287 chip.
Analog Device reply that they will release this chip in last week of Sep, 2023.
Please refer to the attachment to review their reply.

> 
> > Please refer to this link:
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> >
> analog.com%2Fen%2Fproducts%2Fltc2487.html%23product-overview&data=05
> %7
> >
> C01%7CWayne_SC_Liu%40wiwynn.com%7Ca1151d861c5545c3407508db8c13
> 2746%7Cd
> >
> a6e0628fc834caf9dd273061cbab167%7C0%7C0%7C638257786161934465%7C
> Unknown
> >
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJ
> >
> XVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MuIiftfChWZwCRBtt%2B1%2BoPj
> ZGr1mE2Zt
> > fqqztvhjz6I%3D&reserved=0
> > enum chips { ltc4286 = 0, ltc4287 };
> > Use in v1 line 118 to list chip index instead of hardcoding
> >
> 
> ltc2487 is _not_ ltc4287.
>     ^^               ^^
> 
> Sure, a Google search for LTC4287 returns a link to LTC2487 (!) as first response,
> but LTC2487 is an ADC and has nothing but the manufacturer in common with
> LTC4286.
> 
> LTC4286 High Power Positive Hot-Swap Controller with Power Monitor via
> PMBus
> LTC2487 16-Bit 2-/4-Channel ∆Σ ADC with PGA, Easy Drive and I2C Interface
> 
> I _really_ do not understand what you are trying to do here or why.

We have sent e-mail to query about the release date for LTC4287 chip.
Analog Device reply that they will release this chip in last week of Sep, 2023.
Please refer to the attachment to review their reply.

> 
> >>
> >>> +
> >>> +static struct pmbus_driver_info ltc4286_info = {
> >>> +     .pages = 1,
> >>> +     .format[PSC_VOLTAGE_IN] = direct,
> >>> +     .format[PSC_VOLTAGE_OUT] = direct,
> >>> +     .format[PSC_CURRENT_OUT] = direct,
> >>> +     .format[PSC_POWER] = direct,
> >>> +     .format[PSC_TEMPERATURE] = direct,
> >>> +     .m[PSC_VOLTAGE_IN] = 32,
> >>> +     .b[PSC_VOLTAGE_IN] = 0,
> >>> +     .R[PSC_VOLTAGE_IN] = 1,
> >>> +     .m[PSC_VOLTAGE_OUT] = 32,
> >>> +     .b[PSC_VOLTAGE_OUT] = 0,
> >>> +     .R[PSC_VOLTAGE_OUT] = 1,
> >>> +     .m[PSC_CURRENT_OUT] = 1024,
> >>> +     .b[PSC_CURRENT_OUT] = 0,
> >>> +     .R[PSC_CURRENT_OUT] = 3 - 6,
> >>
> >> This needs explanation.
> > We will add comments as below
> > /* Initialize the MBR as default settings which is referred to LTC4286
> > datasheet(March 22, 2022 version) table 3 page 16 */
> >
> 
> That does not explain "3 - 6".

We will add explanation like below
/*
 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
 * However, the rsense value that user input is mirco ohm.
 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
 */

> 
> >>
> >>> +     .m[PSC_POWER] = 1,
> >>> +     .b[PSC_POWER] = 0,
> >>> +     .R[PSC_POWER] = 4 - 6,
> >>
> >> This needs explanation.
> > We will add comments as below
> > /* To support small shunt resistor value */
> >
> 
> That does not explain "4 - 6".

We will add explanation like below
/*
 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
 * However, the rsense value that user input is mirco ohm.
 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
 */

> 
> 
> >>
> >>> +     .m[PSC_TEMPERATURE] = 1,
> >>> +     .b[PSC_TEMPERATURE] = 273.15,
> >>
> >> Assigning a float to an int doesn't make much sense.
> > We will revise to .b[PSC_TEMPERATURE] = 273, However the value for
> > this MBR is 273.15 in datasheet We use 273 due to pmbus code
> > limitation
> >
> >>
> >>> +     .R[PSC_TEMPERATURE] = 0,
> >>> +     .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT |
> >> PMBUS_HAVE_IOUT |
> >>> +                PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,
> >>
> >> The chip does have a number of status registers.
> > We will add status registers here
> >
> >>
> >>> +};
> >>> +
> >>> +static int ltc4286_probe(struct i2c_client *client,
> >>> +                      const struct i2c_device_id *id) {
> >>> +     int ret;
> >>> +     u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> >>> +     struct device *dev = &client->dev;
> >>> +     struct pmbus_driver_info *info;
> >>> +     u32 rsense;
> >>> +
> >>> +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
> >> block_buffer);
> >>> +     if (ret < 0) {
> >>> +             dev_err(&client->dev, "failed to read manufacturer
> >>> + id\n");
> >>
> >> Kind of pointless to declare a local 'dev' variable and not use it.
> > We will drop it
> >
> >>
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     /* Refer to ltc4286 datasheet page 20
> >>> +      * the default manufacturer id is LTC
> >>
> >> Why "default" ?
> > We will revise to
> > /*
> >   * Refer to ltc4286 datasheet page 20
> >   * the manufacturer id is LTC
> >   */
> >
> >>
> >>> +      */
> >>
> >> Please use standard multi-line comments.
> > We will revise to
> > /*
> >   * Refer to ltc4286 datasheet page 20
> >   * the manufacturer id is LTC
> >   */
> >
> >>
> >>> +     if (ret != LTC4286_MFR_ID_SIZE ||
> >>> +         strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> >>> +             dev_err(&client->dev, "unsupported manufacturer id\n");
> >>> +             return -ENODEV;
> >>> +     }
> >>> +
> >>> +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
> >> block_buffer);
> >>> +     if (ret < 0) {
> >>> +             dev_err(&client->dev, "failed to read manufacturer
> >> model\n");
> >>> +             return ret;
> >>> +     }
> >>
> >> Why read the model if you don't do anything with it ?
> > We will add comaprision here.
> > for (mid = ltc4286_id; mid->name[0]; mid++) {
> >       if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> >               break;
> > }
> >
> >>
> >>> +
> >>> +     ret = of_property_read_u32(client->dev.of_node,
> >> "rsense-micro-ohms",
> >>> +                                &rsense);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +
> >>> +     if (rsense == 0)
> >>> +             return -EINVAL;
> >>> +
> >>
> >> There should be a default for systems not supporting devicetree.
> > After confirming with vendor, they said there is no default rsesne for
> > this chip The value for rsense depends on hardware engineer in each
> > manufacturer
> >
> 
> The _driver_ needs a default.

Here we provide two options
1. Do not set default rsense
As mentioned before, vendor said there is no default rsesne for this chip.
The value for rsense depends on hardware engineer in each manufacturer.
Thus, if user forgets to input rsesne, it will probe fail because user doesn't get the correct rsense value from hardware engineer.
Moreover, if user uses the default rsense, the sensor reading would be weird in his or her systems.
2. Set rsense as 300 micro ohm
We have queried our hardware engineer and they said that the min and max value are 500 micro ohm and 250 micro ohm in their experience.
Which one do you prefer?

> 
> >>
> >>> +     info = &ltc4286_info;
> >>> +
> >>> +     /* Default of VRANGE_SELECT = 1 */
> >>> +     if (device_property_read_bool(dev, "vrange_select_25p6")) {
> >>> +             /* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> >>> +             ret = i2c_smbus_read_word_data(client,
> >> LTC4286_MFR_CONFIG1);
> >>> +             if (ret < 0) {
> >>> +                     dev_err(&client->dev,
> >>> +                             "failed to read manufacturer
> >> configuration one\n");
> >>> +                     return ret;
> >>> +             }
> >>> +
> >>> +             ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V
> */
> >>> +             i2c_smbus_write_word_data(client,
> >> LTC4286_MFR_CONFIG1,
> >>> + ret);
> >>
> >> Error check missing.
> > ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret); if
> > (ret < 0) {
> >       dev_err(&client->dev, "failed to set vrange\n");
> >       return ret;
> > }
> >
> >>
> >>> +
> >>> +             info->m[PSC_VOLTAGE_IN] = 128;
> >>> +             info->m[PSC_VOLTAGE_OUT] = 128;
> >>> +             info->m[PSC_POWER] = 4 * rsense;
> >>> +     } else {
> >>> +             info->m[PSC_POWER] = rsense;
> >>
> >> This just takes the current configuration, and assumes it is the default.
> >> That may not be correct. The chip may have been configured by the
> >> BIOS, or manually.
> > The MBR values are based on hardware design, so it must be set in
> > initial stage
> >
> >>
> >>> +     }
> >>> +
> >>
> >> The code assumes that there is only a single chip in the system, or
> >> that the configuration of all chips is identical. This is not necessarily correct.
> > If there are more than one LTC4286 or LTC4287 chips, user can add the
> > configuration for different chips in dts file
> >
> 
> "info" is a pointer to a static variable.
> 
>         info = &ltc4286_info;
> 
> In this context, what you are saying above does not make sense. The second
> chip's configuration will override the first chip's configuration.
> 
>         info->m[PSC_POWER] = 4 * rsense;
>         ...
> 
> 
> No, sorry, this still does not make sense.

In our opinion, if there are multiple chips in one system, the LTC4286 driver would probe many times and kernel allocate these chips for different memory space.
Thus, the configuration wonn't be overrided

> 
> 
> >>
> >>> +     info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> >>> +
> >>> +     return pmbus_do_probe(client, info); }
> >>> +
> >>> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> >>> +                                                { "ltc4287",
> >> ltc4287
> >>> +},
> >>
> >> Even if LTC4287 existed, assigning the ID here ...
> > So do you recommend us to put this enum (enum chips { ltc4286, ltc4287 };)
> here?
> >
> >>
> >>> +                                                {} };
> >>> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> >>> +
> >>> +static const struct of_device_id ltc4286_of_match[] = {
> >>> +     { .compatible = "lltc,ltc4286" },
> >>> +     { .compatible = "lltc,ltc4287" },
> >>
> >> ... but not here defeats having it in the first place.
> > So do you recommend us to not put this enum (enum chips { ltc4286,
> ltc4287 };) here?
> >
> 
> Please read my comment. I am saying that it does not make sense to provide
> the chip ID to i2c_device_id but not in of_device_id. Either add it do both if
> needed or not at all. I am not telling which one, I am just asking for
> consistency.
> 
> However, as mentioned earlier, according to your code the chips supposedly
> have identical functionality. Separate chip IDs (ltc4286 and ltc4287) therefore
> seem unnecessary (not even counting the fact that ltc4287 still does not seem
> to exist).

We will revise to below and remove the enum
static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", 0 }, {} };

static const struct of_device_id ltc4286_of_match[] = { { .compatible = "lltc,ltc4286" }, {} };

> 
> 
> >>
> >>> +     {}
> >>> +};
> >>
> >> MODULE_DEVOCE_TABLE() missing.
> > In v1 line 120
> >
> >>
> >>> +
> >>> +/* This is the driver that will be inserted */
> >>
> >> Useless comment
> > We will drop it.
> >
> >>
> >>> +static struct i2c_driver ltc4286_driver = {
> >>> +     .driver = {
> >>> +             .name = "ltc4286",
> >>> +             .of_match_table = ltc4286_of_match,
> >>> +     },
> >>> +     .probe = ltc4286_probe,
> >>> +     .id_table = ltc4286_id,
> >>> +};
> >>> +
> >>> +module_i2c_driver(ltc4286_driver);
> >>> +
> >>> +MODULE_AUTHOR("Delphine CC Chiu
> >> <Delphine_CC_Chiu@wiwynn.com>");
> >>> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> >>> +MODULE_LICENSE("GPL");
> >>> --
> >>> 2.17.1
> >>>


[-- Attachment #2: Type: message/rfc822, Size: 10215 bytes --]

From: "Tai, Jason" <Jason.Tai@analog.com>
To: Wayne SC Liu/WYHQ/Wiwynn <Wayne_SC_Liu@wiwynn.com>
Cc: "openbmc_compute@meta.com" <openbmc_compute@meta.com>, Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
Subject: RE: The release date about LTC4287
Date: Mon, 31 Jul 2023 02:54:33 +0000
Message-ID: <PH0PR03MB706741CB9D9833AE006E39C39405A@PH0PR03MB7067.namprd03.prod.outlook.com>

  Security Reminder: Please be aware that this email is sent by an external sender.

Hi Wayne,

To be specific, last week of Sep, 2023. Thanks.



Regards,

Jason Tai
Senior Engineer, Field Applications
Mobile     (+886) (0) 922-226-527



-----Original Message-----
From: Wayne SC Liu/WYHQ/Wiwynn <Wayne_SC_Liu@wiwynn.com>
Sent: Tuesday, July 25, 2023 1:19 PM
To: Tai, Jason <Jason.Tai@analog.com>
Cc: openbmc_compute@meta.com; Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
Subject: RE: The release date about LTC4287

[External]

Loop more Wiwynn BMC member

> -----Original Message-----
> From: Wayne SC Liu/WYHQ/Wiwynn
> Sent: Tuesday, July 25, 2023 10:40 AM
> To: 'Jason.Tai@analog.com' <Jason.Tai@analog.com>
> Cc: 'openbmc_compute@meta.com' <openbmc_compute@meta.com>; Delphine
> Chiu/WYHQ/Wiwynn <DELPHINE_CHIU@wiwynn.com>; Bonnie Lo/WYHQ/Wiwynn
> <Bonnie_Lo@wiwynn.com>; Sara SY Lin/WYHQ/Wiwynn
> <Sara_SY_Lin@wiwynn.com>; Ricky CX Wu/WYHQ/Wiwynn
> <Ricky_CX_Wu@wiwynn.com>; Jeff Shih/WYHQ/Wiwynn <Jeff_Shih@wiwynn.com>
> Subject: The release date about LTC4287
>
> Hi Jason,
>
> I am Wiwynn BMC Wayne.
>
> We have contributed the Linux driver for LTC4286 and LTC4287 chips.
>
> However, Linux reviewer doesn't agree with our contribution because
> LTC4287 chip still not release on Aanlog Device website.
> (Please refer to the attachment)
>
> Moreover, our customer Meta plans to use LTC4287 in next generation
> product so we need driver to access this device.
>
> Do you have the exact release date about this chip?
>
> Thanks.
> Wayne
>

WIWYNN PROPRIETARY
This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately.

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.3 next-20230424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230424101352.28117-3-Delphine_CC_Chiu%40Wiwynn.com
patch subject: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
config: riscv-randconfig-r021-20230425 (https://download.01.org/0day-ci/archive/20230425/202304251257.oiVqQ5cl-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
        git checkout 318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/hwmon/pmbus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304251257.oiVqQ5cl-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/hwmon/pmbus/ltc4286.c:46:24: warning: implicit conversion from 'double' to 'int' changes value from 273.15 to 273 [-Wliteral-conversion]
           .b[PSC_TEMPERATURE] = 273.15,
                                 ^~~~~~
>> drivers/hwmon/pmbus/ltc4286.c:134:11: error: incompatible function pointer types initializing 'int (*)(struct i2c_client *)' with an expression of type 'int (struct i2c_client *, const struct i2c_device_id *)' [-Wincompatible-function-pointer-types]
           .probe = ltc4286_probe,
                    ^~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +134 drivers/hwmon/pmbus/ltc4286.c

    25	
    26	static struct pmbus_driver_info ltc4286_info = {
    27		.pages = 1,
    28		.format[PSC_VOLTAGE_IN] = direct,
    29		.format[PSC_VOLTAGE_OUT] = direct,
    30		.format[PSC_CURRENT_OUT] = direct,
    31		.format[PSC_POWER] = direct,
    32		.format[PSC_TEMPERATURE] = direct,
    33		.m[PSC_VOLTAGE_IN] = 32,
    34		.b[PSC_VOLTAGE_IN] = 0,
    35		.R[PSC_VOLTAGE_IN] = 1,
    36		.m[PSC_VOLTAGE_OUT] = 32,
    37		.b[PSC_VOLTAGE_OUT] = 0,
    38		.R[PSC_VOLTAGE_OUT] = 1,
    39		.m[PSC_CURRENT_OUT] = 1024,
    40		.b[PSC_CURRENT_OUT] = 0,
    41		.R[PSC_CURRENT_OUT] = 3 - 6,
    42		.m[PSC_POWER] = 1,
    43		.b[PSC_POWER] = 0,
    44		.R[PSC_POWER] = 4 - 6,
    45		.m[PSC_TEMPERATURE] = 1,
  > 46		.b[PSC_TEMPERATURE] = 273.15,
    47		.R[PSC_TEMPERATURE] = 0,
    48		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
    49			   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,
    50	};
    51	
    52	static int ltc4286_probe(struct i2c_client *client,
    53				 const struct i2c_device_id *id)
    54	{
    55		int ret;
    56		u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
    57		struct device *dev = &client->dev;
    58		struct pmbus_driver_info *info;
    59		u32 rsense;
    60	
    61		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
    62		if (ret < 0) {
    63			dev_err(&client->dev, "failed to read manufacturer id\n");
    64			return ret;
    65		}
    66	
    67		/* Refer to ltc4286 datasheet page 20
    68		 * the default manufacturer id is LTC
    69		 */
    70		if (ret != LTC4286_MFR_ID_SIZE ||
    71		    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
    72			dev_err(&client->dev, "unsupported manufacturer id\n");
    73			return -ENODEV;
    74		}
    75	
    76		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
    77		if (ret < 0) {
    78			dev_err(&client->dev, "failed to read manufacturer model\n");
    79			return ret;
    80		}
    81	
    82		ret = of_property_read_u32(client->dev.of_node, "rsense-micro-ohms",
    83					   &rsense);
    84		if (ret < 0)
    85			return ret;
    86	
    87		if (rsense == 0)
    88			return -EINVAL;
    89	
    90		info = &ltc4286_info;
    91	
    92		/* Default of VRANGE_SELECT = 1 */
    93		if (device_property_read_bool(dev, "vrange_select_25p6")) {
    94			/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
    95			ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
    96			if (ret < 0) {
    97				dev_err(&client->dev,
    98					"failed to read manufacturer configuration one\n");
    99				return ret;
   100			}
   101	
   102			ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
   103			i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);
   104	
   105			info->m[PSC_VOLTAGE_IN] = 128;
   106			info->m[PSC_VOLTAGE_OUT] = 128;
   107			info->m[PSC_POWER] = 4 * rsense;
   108		} else {
   109			info->m[PSC_POWER] = rsense;
   110		}
   111	
   112		info->m[PSC_CURRENT_OUT] = 1024 * rsense;
   113	
   114		return pmbus_do_probe(client, info);
   115	}
   116	
   117	static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
   118							   { "ltc4287", ltc4287 },
   119							   {} };
   120	MODULE_DEVICE_TABLE(i2c, ltc4286_id);
   121	
   122	static const struct of_device_id ltc4286_of_match[] = {
   123		{ .compatible = "lltc,ltc4286" },
   124		{ .compatible = "lltc,ltc4287" },
   125		{}
   126	};
   127	
   128	/* This is the driver that will be inserted */
   129	static struct i2c_driver ltc4286_driver = {
   130		.driver = {
   131			.name = "ltc4286",
   132			.of_match_table = ltc4286_of_match,
   133		},
 > 134		.probe = ltc4286_probe,
   135		.id_table = ltc4286_id,
   136	};
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Andi Shyti]

Hi Delphine,

On top of Guenter's comments,

[...]

> +config SENSORS_LTC4286
> +	bool "Linear Technologies LTC4286"
> +	help
> +	  If you say yes here you get hardware monitoring support for Linear
> +	  Technology LTC4286.

could you add a couple of words more here?

[...]

> +static int ltc4286_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct device *dev = &client->dev;
> +	struct pmbus_driver_info *info;
> +	u32 rsense;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer id\n");

you can use dev_err_probe() here:

	return dev_err_probe(&client->dev, err, "failed to read manufacturer id\n");

> +		return ret;
> +	}
> +
> +	/* Refer to ltc4286 datasheet page 20
> +	 * the default manufacturer id is LTC
> +	 */
> +	if (ret != LTC4286_MFR_ID_SIZE ||
> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> +		dev_err(&client->dev, "unsupported manufacturer id\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer model\n");
> +		return ret;
> +	}

Is this read really needed?

Andi

[...]

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 4/25/23 06:45, Andi Shyti wrote:
> Hi Delphine,
> 
> On top of Guenter's comments,
> 
> [...]
> 
>> +config SENSORS_LTC4286
>> +	bool "Linear Technologies LTC4286"
>> +	help
>> +	  If you say yes here you get hardware monitoring support for Linear
>> +	  Technology LTC4286.
> 
> could you add a couple of words more here?
> 
> [...]
> 
>> +static int ltc4286_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
>> +	struct device *dev = &client->dev;
>> +	struct pmbus_driver_info *info;
>> +	u32 rsense;
>> +
>> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "failed to read manufacturer id\n");
> 
> you can use dev_err_probe() here:
> 
> 	return dev_err_probe(&client->dev, err, "failed to read manufacturer id\n");
> 
>> +		return ret;
>> +	}
>> +
>> +	/* Refer to ltc4286 datasheet page 20
>> +	 * the default manufacturer id is LTC
>> +	 */
>> +	if (ret != LTC4286_MFR_ID_SIZE ||
>> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
>> +		dev_err(&client->dev, "unsupported manufacturer id\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "failed to read manufacturer model\n");
>> +		return ret;
>> +	}
> 
> Is this read really needed?
> 

It only makes sense if the returned string is actually validated.
Otherwise no.

Guenter

RE: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, April 25, 2023 10:09 PM
> To: Andi Shyti <andi.shyti@kernel.org>; Delphine_CC_Chiu/WYHQ/Wiwynn
> <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 4/25/23 06:45, Andi Shyti wrote:
> > Hi Delphine,
> >
> > On top of Guenter's comments,
> >
> > [...]
> >
> >> +config SENSORS_LTC4286
> >> +    bool "Linear Technologies LTC4286"
> >> +    help
> >> +      If you say yes here you get hardware monitoring support for Linear
> >> +      Technology LTC4286.
> >
> > could you add a couple of words more here?
> >
> > [...]
> >
> >> +static int ltc4286_probe(struct i2c_client *client,
> >> +                     const struct i2c_device_id *id) {
> >> +    int ret;
> >> +    u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> >> +    struct device *dev = &client->dev;
> >> +    struct pmbus_driver_info *info;
> >> +    u32 rsense;
> >> +
> >> +    ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
> block_buffer);
> >> +    if (ret < 0) {
> >> +            dev_err(&client->dev, "failed to read manufacturer
> >> + id\n");
> >
> > you can use dev_err_probe() here:
> >
> >       return dev_err_probe(&client->dev, err, "failed to read
> > manufacturer id\n");
> >
> >> +            return ret;
> >> +    }
> >> +
> >> +    /* Refer to ltc4286 datasheet page 20
> >> +     * the default manufacturer id is LTC
> >> +     */
> >> +    if (ret != LTC4286_MFR_ID_SIZE ||
> >> +        strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> >> +            dev_err(&client->dev, "unsupported manufacturer id\n");
> >> +            return -ENODEV;
> >> +    }
> >> +
> >> +    ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
> block_buffer);
> >> +    if (ret < 0) {
> >> +            dev_err(&client->dev, "failed to read manufacturer
> model\n");
> >> +            return ret;
> >> +    }
> >
> > Is this read really needed?
> >
> 
> It only makes sense if the returned string is actually validated.
> Otherwise no.
> 
> Guenter
We will add comaprision here.
for (mid = ltc4286_id; mid->name[0]; mid++) {
	if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
		break;
}

RE: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, April 25, 2023 9:46 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> <jdelvare@suse.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> Hi Delphine,
> 
> On top of Guenter's comments,
> 
> [...]
> 
> > +config SENSORS_LTC4286
> > +     bool "Linear Technologies LTC4286"
> > +     help
> > +       If you say yes here you get hardware monitoring support for Linear
> > +       Technology LTC4286.
> 
> could you add a couple of words more here?
We will revise as below
       config SENSORS_LTC4286
	        bool "Analog Devices LTC4286"
            help
			  LTC4286 is an integrated solution for hot swap applications that allows 
			  a board to be safely inserted and removed from a live backplane.
			  This chip could be used to monitor voltage, current, ...etc.
              If you say yes here you get hardware monitoring support for Analog
              Devices LTC4286.

> 
> [...]
> 
> > +static int ltc4286_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *id) {
> > +     int ret;
> > +     u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> > +     struct device *dev = &client->dev;
> > +     struct pmbus_driver_info *info;
> > +     u32 rsense;
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer
> > + id\n");
> 
> you can use dev_err_probe() here:
We will revise as below
dev_err_probe(&client->dev, err, "failed to read manufacturer id\n");

> 
>         return dev_err_probe(&client->dev, err, "failed to read manufacturer
> id\n");
> 
> > +             return ret;
> > +     }
> > +
> > +     /* Refer to ltc4286 datasheet page 20
> > +      * the default manufacturer id is LTC
> > +      */
> > +     if (ret != LTC4286_MFR_ID_SIZE ||
> > +         strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> > +             dev_err(&client->dev, "unsupported manufacturer id\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer
> model\n");
> > +             return ret;
> > +     }
> 
> Is this read really needed?
We use this to check manufacturer model.
And we will add comparison here.
for (mid = ltc4286_id; mid->name[0]; mid++) {
if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
	break;
} 

> 
> Andi
> 
> [...]

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.3 next-20230428]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230424101352.28117-3-Delphine_CC_Chiu%40Wiwynn.com
patch subject: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
config: arm-randconfig-s043-20230430 (https://download.01.org/0day-ci/archive/20230501/202305010110.yAHNljHe-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
        git checkout 318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwmon/pmbus/ drivers/i3c/master/ drivers/spi/ sound/soc/cirrus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305010110.yAHNljHe-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hwmon/pmbus/ltc4286.c:134:18: error: initialization of 'int (*)(struct i2c_client *)' from incompatible pointer type 'int (*)(struct i2c_client *, const struct i2c_device_id *)' [-Werror=incompatible-pointer-types]
     134 |         .probe = ltc4286_probe,
         |                  ^~~~~~~~~~~~~
   drivers/hwmon/pmbus/ltc4286.c:134:18: note: (near initialization for 'ltc4286_driver.<anonymous>.probe')
   cc1: some warnings being treated as errors


vim +134 drivers/hwmon/pmbus/ltc4286.c

   127	
   128	/* This is the driver that will be inserted */
   129	static struct i2c_driver ltc4286_driver = {
   130		.driver = {
   131			.name = "ltc4286",
   132			.of_match_table = ltc4286_of_match,
   133		},
 > 134		.probe = ltc4286_probe,
   135		.id_table = ltc4286_id,
   136	};
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.3 next-20230505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230424101352.28117-3-Delphine_CC_Chiu%40Wiwynn.com
patch subject: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230506/202305060716.5xAx86T8-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20230424-181521
        git checkout 318b8a252bb2d7430f1cf7b93bb5df8d0e4fee29
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305060716.5xAx86T8-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hwmon/pmbus/ltc4286.c:46:24: warning: implicit conversion from 'double' to 'int' changes value from 273.15 to 273 [-Wliteral-conversion]
           .b[PSC_TEMPERATURE] = 273.15,
                                 ^~~~~~
>> drivers/hwmon/pmbus/ltc4286.c:134:11: error: incompatible function pointer types initializing 'int (*)(struct i2c_client *)' with an expression of type 'int (struct i2c_client *, const struct i2c_device_id *)' [-Werror,-Wincompatible-function-pointer-types]
           .probe = ltc4286_probe,
                    ^~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +134 drivers/hwmon/pmbus/ltc4286.c

   127	
   128	/* This is the driver that will be inserted */
   129	static struct i2c_driver ltc4286_driver = {
   130		.driver = {
   131			.name = "ltc4286",
   132			.of_match_table = ltc4286_of_match,
   133		},
 > 134		.probe = ltc4286_probe,
   135		.id_table = ltc4286_id,
   136	};
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine CC Chiu]

Add a device tree bindings for ltc4286 driver.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 .../bindings/hwmon/lltc,ltc4286.yaml          | 49 +++++++++++++++++++
 MAINTAINERS                                   |  9 ++++
 2 files changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
new file mode 100644
index 000000000000..ad7f6ad888e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LTC4286 power monitors
+
+maintainers:
+  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+
+properties:
+  compatible:
+    enum:
+      - lltc,ltc4286
+      - lltc,ltc4287
+
+  reg:
+    maxItems: 1
+
+  vrange_select_25p6:
+    description:
+      This property is a bool parameter to represent the
+      voltage range is 25.6 or not for this chip.
+
+  shunt-resistor-micro-ohms:
+    description:
+      Resistor value in micro-Ohm
+
+required:
+  - compatible
+  - reg
+  - shunt-resistor-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "lltc,ltc4286";
+            reg = <0x40>;
+            vrange_select_25p6;
+            shunt-resistor-micro-ohms = <300>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d516295978a4..7c1cb9bd4f45 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12344,6 +12344,15 @@ S:	Maintained
 F:	Documentation/hwmon/ltc4261.rst
 F:	drivers/hwmon/ltc4261.c
 
+LTC4286 HARDWARE MONITOR DRIVER
+M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
+F:	drivers/hwmon/pmbus/Kconfig
+F:	drivers/hwmon/pmbus/Makefile
+F:	drivers/hwmon/pmbus/ltc4286.c
+
 LTC4306 I2C MULTIPLEXER DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 L:	linux-i2c@vger.kernel.org
-- 
2.25.1

[PATCH 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine CC Chiu]

Add a driver to support ltc4286 chip

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 drivers/hwmon/pmbus/Kconfig   |   9 +++
 drivers/hwmon/pmbus/Makefile  |   1 +
 drivers/hwmon/pmbus/ltc4286.c | 147 ++++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/ltc4286.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 270b6336b76d..7cb9cbff587d 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -226,6 +226,15 @@ config SENSORS_LTC3815
 
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc3815.
+config SENSORS_LTC4286
+	bool "Analog Devices LTC4286"
+	help
+	  LTC4286 is an integrated solution for hot swap applications that
+	  allows a board to be safely inserted and removed from a
+	  live backplane.
+	  This chip could be used to monitor voltage, current, ...etc.
+	  If you say yes here you get hardware monitoring support for Analog
+	  Devices LTC4286.
 
 config SENSORS_MAX15301
 	tristate "Maxim MAX15301"
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 84ee960a6c2d..94e28f6d6a61 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
 obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
 obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
 obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
+obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
 obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
 obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
 obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
new file mode 100644
index 000000000000..b86bf31cfbae
--- /dev/null
+++ b/drivers/hwmon/pmbus/ltc4286.c
@@ -0,0 +1,147 @@
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+/* LTC4286 register */
+#define LTC4286_MFR_CONFIG1	0xF2
+
+/* LTC4286 configuration */
+#define VRANGE_SELECT_BIT	BIT(1)
+
+#define LTC4286_MFR_ID_SIZE	3
+
+enum chips { ltc4286, ltc4287 };
+
+/*
+ * Initialize the MBR as default settings which is referred to LTC4286 datasheet
+ * (March 22, 2022 version) table 3 page 16
+ */
+static struct pmbus_driver_info ltc4286_info = {
+	.pages = 1,
+	.format[PSC_VOLTAGE_IN] = direct,
+	.format[PSC_VOLTAGE_OUT] = direct,
+	.format[PSC_CURRENT_OUT] = direct,
+	.format[PSC_POWER] = direct,
+	.format[PSC_TEMPERATURE] = direct,
+	.m[PSC_VOLTAGE_IN] = 32,
+	.b[PSC_VOLTAGE_IN] = 0,
+	.R[PSC_VOLTAGE_IN] = 1,
+	.m[PSC_VOLTAGE_OUT] = 32,
+	.b[PSC_VOLTAGE_OUT] = 0,
+	.R[PSC_VOLTAGE_OUT] = 1,
+	.m[PSC_CURRENT_OUT] = 1024,
+	.b[PSC_CURRENT_OUT] = 0,
+	.R[PSC_CURRENT_OUT] = 3 - 6, /* To support small shunt resistor value */
+	.m[PSC_POWER] = 1,
+	.b[PSC_POWER] = 0,
+	.R[PSC_POWER] = 4 - 6, /* To support small shunt resistor value */
+	.m[PSC_TEMPERATURE] = 1,
+	.b[PSC_TEMPERATURE] = 273,
+	.R[PSC_TEMPERATURE] = 0,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
+		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP,
+};
+
+static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
+						   { "ltc4287", ltc4287 },
+						   {} };
+MODULE_DEVICE_TABLE(i2c, ltc4286_id);
+
+static int ltc4286_probe(struct i2c_client *client)
+{
+	int ret;
+	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
+	struct pmbus_driver_info *info;
+	u32 rsense;
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer id\n");
+		return ret;
+	}
+
+	/*
+	 * Refer to ltc4286 datasheet page 20
+	 * the manufacturer id is LTC
+	 */
+	if (ret != LTC4286_MFR_ID_SIZE ||
+	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
+		return dev_err_probe(&client->dev, err,
+				     "failed to read manufacturer id\n");
+	}
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer model\n");
+		return ret;
+	}
+
+	for (mid = ltc4286_id; mid->name[0]; mid++) {
+		if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
+			break;
+	}
+
+	ret = of_property_read_u32(client->dev.of_node,
+				   "shunt-resistor-micro-ohms", &rsense);
+	if (ret < 0)
+		return ret;
+
+	if (rsense == 0)
+		return -EINVAL;
+
+	info = &ltc4286_info;
+
+	/* Default of VRANGE_SELECT = 1, 102.4V */
+	if (device_property_read_bool(client->dev, "vrange_select_25p6")) {
+		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
+		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"failed to read manufacturer configuration one\n");
+			return ret;
+		}
+
+		ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
+		ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1,
+						ret);
+		if (ret < 0) {
+			dev_err(&client->dev, "failed to set vrange\n");
+			return ret;
+		}
+
+		info->m[PSC_VOLTAGE_IN] = 128;
+		info->m[PSC_VOLTAGE_OUT] = 128;
+		info->m[PSC_POWER] = 4 * rsense;
+	} else
+		info->m[PSC_POWER] = rsense;
+
+	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
+
+	return pmbus_do_probe(client, info);
+}
+
+static const struct of_device_id ltc4286_of_match[] = {
+	{ .compatible = "lltc,ltc4286" },
+	{ .compatible = "lltc,ltc4287" },
+	{}
+};
+
+static struct i2c_driver ltc4286_driver = {
+	.driver = {
+		.name = "ltc4286",
+		.of_match_table = ltc4286_of_match,
+	},
+	.probe = ltc4286_probe,
+	.id_table = ltc4286_id,
+};
+
+module_i2c_driver(ltc4286_driver);
+
+MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
+MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 7/24/23 03:05, Delphine CC Chiu wrote:
> Add a driver to support ltc4286 chip
> 

No change log, no versioning, did not address all feedback,
sent as reply to previous version instead of independently.

I am not going to re-review this patch. Address all feedback
comments, provide change log and patch versions, and do not
send as reply to previous patch series.

Guenter

> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>   drivers/hwmon/pmbus/Kconfig   |   9 +++
>   drivers/hwmon/pmbus/Makefile  |   1 +
>   drivers/hwmon/pmbus/ltc4286.c | 147 ++++++++++++++++++++++++++++++++++
>   3 files changed, 157 insertions(+)
>   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 270b6336b76d..7cb9cbff587d 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -226,6 +226,15 @@ config SENSORS_LTC3815
>   
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc3815.
> +config SENSORS_LTC4286
> +	bool "Analog Devices LTC4286"
> +	help
> +	  LTC4286 is an integrated solution for hot swap applications that
> +	  allows a board to be safely inserted and removed from a
> +	  live backplane.
> +	  This chip could be used to monitor voltage, current, ...etc.
> +	  If you say yes here you get hardware monitoring support for Analog
> +	  Devices LTC4286.
>   
>   config SENSORS_MAX15301
>   	tristate "Maxim MAX15301"
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 84ee960a6c2d..94e28f6d6a61 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>   obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
>   obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>   obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
> +obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
>   obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
>   obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
>   obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
> diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
> new file mode 100644
> index 000000000000..b86bf31cfbae
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ltc4286.c
> @@ -0,0 +1,147 @@
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +/* LTC4286 register */
> +#define LTC4286_MFR_CONFIG1	0xF2
> +
> +/* LTC4286 configuration */
> +#define VRANGE_SELECT_BIT	BIT(1)
> +
> +#define LTC4286_MFR_ID_SIZE	3
> +
> +enum chips { ltc4286, ltc4287 };
> +
> +/*
> + * Initialize the MBR as default settings which is referred to LTC4286 datasheet
> + * (March 22, 2022 version) table 3 page 16
> + */
> +static struct pmbus_driver_info ltc4286_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.format[PSC_TEMPERATURE] = direct,
> +	.m[PSC_VOLTAGE_IN] = 32,
> +	.b[PSC_VOLTAGE_IN] = 0,
> +	.R[PSC_VOLTAGE_IN] = 1,
> +	.m[PSC_VOLTAGE_OUT] = 32,
> +	.b[PSC_VOLTAGE_OUT] = 0,
> +	.R[PSC_VOLTAGE_OUT] = 1,
> +	.m[PSC_CURRENT_OUT] = 1024,
> +	.b[PSC_CURRENT_OUT] = 0,
> +	.R[PSC_CURRENT_OUT] = 3 - 6, /* To support small shunt resistor value */
> +	.m[PSC_POWER] = 1,
> +	.b[PSC_POWER] = 0,
> +	.R[PSC_POWER] = 4 - 6, /* To support small shunt resistor value */
> +	.m[PSC_TEMPERATURE] = 1,
> +	.b[PSC_TEMPERATURE] = 273,
> +	.R[PSC_TEMPERATURE] = 0,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> +		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
> +		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP,
> +};
> +
> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> +						   { "ltc4287", ltc4287 },
> +						   {} };
> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> +
> +static int ltc4286_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct pmbus_driver_info *info;
> +	u32 rsense;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer id\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Refer to ltc4286 datasheet page 20
> +	 * the manufacturer id is LTC
> +	 */
> +	if (ret != LTC4286_MFR_ID_SIZE ||
> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> +		return dev_err_probe(&client->dev, err,
> +				     "failed to read manufacturer id\n");
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer model\n");
> +		return ret;
> +	}
> +
> +	for (mid = ltc4286_id; mid->name[0]; mid++) {
> +		if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> +			break;
> +	}
> +
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "shunt-resistor-micro-ohms", &rsense);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (rsense == 0)
> +		return -EINVAL;
> +
> +	info = &ltc4286_info;
> +
> +	/* Default of VRANGE_SELECT = 1, 102.4V */
> +	if (device_property_read_bool(client->dev, "vrange_select_25p6")) {
> +		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> +		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"failed to read manufacturer configuration one\n");
> +			return ret;
> +		}
> +
> +		ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
> +		ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1,
> +						ret);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "failed to set vrange\n");
> +			return ret;
> +		}
> +
> +		info->m[PSC_VOLTAGE_IN] = 128;
> +		info->m[PSC_VOLTAGE_OUT] = 128;
> +		info->m[PSC_POWER] = 4 * rsense;
> +	} else
> +		info->m[PSC_POWER] = rsense;
> +
> +	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct of_device_id ltc4286_of_match[] = {
> +	{ .compatible = "lltc,ltc4286" },
> +	{ .compatible = "lltc,ltc4287" },
> +	{}
> +};
> +
> +static struct i2c_driver ltc4286_driver = {
> +	.driver = {
> +		.name = "ltc4286",
> +		.of_match_table = ltc4286_of_match,
> +	},
> +	.probe = ltc4286_probe,
> +	.id_table = ltc4286_id,
> +};
> +
> +module_i2c_driver(ltc4286_driver);
> +
> +MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] hwmon: pmbus: Add ltc4286 driver

[Andi Shyti]

Hi Delphine,

On Mon, Jul 24, 2023 at 06:05:12PM +0800, Delphine CC Chiu wrote:
> Add a driver to support ltc4286 chip
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>

please resend this series withour using in-reply-to. Please read
carefully the comments in v1, add a good description in the
commit log, add versioning and changelog in the cover letter and
run checkpatch.pl before sending.

Thank you,
Andi

Re: [PATCH 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Krzysztof Kozlowski]

On 24/07/2023 12:05, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 driver.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  .../bindings/hwmon/lltc,ltc4286.yaml          | 49 +++++++++++++++++++
>  MAINTAINERS                                   |  9 ++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> 

You already sent it, so why this duplicate? I also don't understand why
this is attached to previous thread.

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Rob Herring]

On Mon, Jul 24, 2023 at 06:05:11PM +0800, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 driver.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  .../bindings/hwmon/lltc,ltc4286.yaml          | 49 +++++++++++++++++++
>  MAINTAINERS                                   |  9 ++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> new file mode 100644
> index 000000000000..ad7f6ad888e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTC4286 power monitors
> +
> +maintainers:
> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lltc,ltc4286
> +      - lltc,ltc4287
> +
> +  reg:
> +    maxItems: 1
> +
> +  vrange_select_25p6:

Needs a vendor prefix and don't use '_'.

> +    description:
> +      This property is a bool parameter to represent the

We have types, don't define the type in free form text.

> +      voltage range is 25.6 or not for this chip.
> +
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Resistor value in micro-Ohm
> +
> +required:
> +  - compatible
> +  - reg
> +  - shunt-resistor-micro-ohms
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        power-sensor@40 {
> +            compatible = "lltc,ltc4286";
> +            reg = <0x40>;
> +            vrange_select_25p6;
> +            shunt-resistor-micro-ohms = <300>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d516295978a4..7c1cb9bd4f45 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12344,6 +12344,15 @@ S:	Maintained
>  F:	Documentation/hwmon/ltc4261.rst
>  F:	drivers/hwmon/ltc4261.c
>  
> +LTC4286 HARDWARE MONITOR DRIVER
> +M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> +F:	drivers/hwmon/pmbus/Kconfig
> +F:	drivers/hwmon/pmbus/Makefile
> +F:	drivers/hwmon/pmbus/ltc4286.c
> +
>  LTC4306 I2C MULTIPLEXER DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
>  L:	linux-i2c@vger.kernel.org
> -- 
> 2.25.1
> 

RE: [PATCH 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, July 27, 2023 12:44 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Guenter Roeck
> <linux@roeck-us.net>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>;
> Conor Dooley <conor+dt@kernel.org>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On Mon, Jul 24, 2023 at 06:05:11PM +0800, Delphine CC Chiu wrote:
> > Add a device tree bindings for ltc4286 driver.
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > ---
> >  .../bindings/hwmon/lltc,ltc4286.yaml          | 49
> +++++++++++++++++++
> >  MAINTAINERS                                   |  9 ++++
> >  2 files changed, 58 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > new file mode 100644
> > index 000000000000..ad7f6ad888e4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%7C
> 01%7C
> >
> +Wayne_SC_Liu%40wiwynn.com%7Cd2e29aed8bdc47a743e508db8df786ac%7
> Cda6e06
> >
> +28fc834caf9dd273061cbab167%7C0%7C0%7C638259866495880680%7CUnkn
> own%7CT
> >
> +WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVC
> >
> +I6Mn0%3D%7C3000%7C%7C%7C&sdata=n%2FyZyYnSEoCOcUgQFI6BERAgkKa
> zYA%2FmAt
> > +2NAlr1Xdo%3D&reserved=0
> > +$schema:
> > +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne_S
> C_Liu%
> >
> +40wiwynn.com%7Cd2e29aed8bdc47a743e508db8df786ac%7Cda6e0628fc834
> caf9dd
> >
> +273061cbab167%7C0%7C0%7C638259866495880680%7CUnknown%7CTWFp
> bGZsb3d8ey
> >
> +JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C30
> >
> +00%7C%7C%7C&sdata=fAA2CUAxdK6MJugvQ1ZRZ1hvB%2FfS%2B8SEOwQ3R
> Z2HEXo%3D&
> > +reserved=0
> > +
> > +title: LTC4286 power monitors
> > +
> > +maintainers:
> > +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - lltc,ltc4286
> > +      - lltc,ltc4287
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vrange_select_25p6:
> 
> Needs a vendor prefix and don't use '_'.

We will revise as adi,vrange-select-25p6

> 
> > +    description:
> > +      This property is a bool parameter to represent the
> 
> We have types, don't define the type in free form text.

We will revise as below
description:
  Specifies which voltage range is used, 25.6 or 102.4.
type: boolean

> 
> > +      voltage range is 25.6 or not for this chip.
> > +
> > +  shunt-resistor-micro-ohms:
> > +    description:
> > +      Resistor value in micro-Ohm
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - shunt-resistor-micro-ohms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        power-sensor@40 {
> > +            compatible = "lltc,ltc4286";
> > +            reg = <0x40>;
> > +            vrange_select_25p6;
> > +            shunt-resistor-micro-ohms = <300>;
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > d516295978a4..7c1cb9bd4f45 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12344,6 +12344,15 @@ S:   Maintained
> >  F:   Documentation/hwmon/ltc4261.rst
> >  F:   drivers/hwmon/ltc4261.c
> >
> > +LTC4286 HARDWARE MONITOR DRIVER
> > +M:   Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +L:   linux-i2c@vger.kernel.org
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > +F:   drivers/hwmon/pmbus/Kconfig
> > +F:   drivers/hwmon/pmbus/Makefile
> > +F:   drivers/hwmon/pmbus/ltc4286.c
> > +
> >  LTC4306 I2C MULTIPLEXER DRIVER
> >  M:   Michael Hennerich <michael.hennerich@analog.com>
> >  L:   linux-i2c@vger.kernel.org
> > --
> > 2.25.1
> >