On Sun, Mar 17, 2024 at 06:02:42PM PDT, Ban Feng wrote:
>HI Guenter and Zev,
>
>If there's no concern about supporting nct7362 in nct7363 driver,
>I'll add it to the of_device_id and i2c_device_id table in v5.
>
>Thanks,
>Ban
>
That sounds good to me, thanks.
Zev
Hi Radu, kernel test robot noticed the following build errors: [auto build test ERROR on groeck-staging/hwmon-next] [also build test ERROR on robh/for-next linus/master v6.8 next-20240318] [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/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 8f68022f8e6e54d1aeae4ed301f5a015963089b7) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403190800.h8cSGROp-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 584 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:20: In file included from include/linux/mm.h:2188: include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ >> drivers/hwmon/pmbus/adp1050.c:47:60: error: too few arguments to function call, expected at least 3, have 2 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ~~~~~~~~~~~~~ ^ include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/hwmon/pmbus/adp1050.c:53:63: error: too few arguments to function call, expected at least 3, have 2 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ~~~~~~~~~~~~~ ^ include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 7 warnings and 2 errors generated. vim +47 drivers/hwmon/pmbus/adp1050.c 32 33 static int adp1050_probe(struct i2c_client *client) 34 { 35 u32 vin_scale_monitor, iin_scale_monitor; 36 int ret; 37 38 if (!i2c_check_functionality(client->adapter, 39 I2C_FUNC_SMBUS_WRITE_WORD_DATA)) 40 return -ENODEV; 41 42 /* Unlock CHIP's password in order to be able to read/write to it's 43 * VIN_SCALE and IIN_SCALE registers. 44 */ 45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 46 if (ret < 0) { > 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n"); 48 return ret; 49 } 50 51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 52 if (ret < 0) { 53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); 54 return ret; 55 } 56 57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the 58 * VIN monitor isn't used, therefore 0 is used as scale in order 59 * for the readings to return 0. 60 */ 61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", 62 &vin_scale_monitor)) 63 vin_scale_monitor = 0; 64 65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the 66 * IIN monitor isn't used, therefore 0 is used as scale in order 67 * for the readings to return 0. 68 */ 69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", 70 &iin_scale_monitor)) 71 iin_scale_monitor = 0; 72 73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, 74 vin_scale_monitor); 75 if (ret < 0) 76 return ret; 77 78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, 79 iin_scale_monitor); 80 if (ret < 0) 81 return ret; 82 83 return pmbus_do_probe(client, &adp1050_info); 84 } 85 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On 3/18/24 14:57, Armin Wolf wrote: > Currently, the hp-wmi-sensors driver needs to be loaded manually > on supported machines. This however is unnecessary since the WMI > id table can be used to support autoloading. > > However the driver might conflict with the hp-wmi driver since both > seem to use the same WMI GUID for registering notify handler. > I have no idea how this is supposed to be handled. wmi_install_notify_handler() is marked as deprecated, suggesting that the entire driver structure may be outdated. > I am thus submitting this patch as an RFC for now. > .... and, given the above, I have no idea what to do with it, sorry. Guenter
Register the WMI id table with MODULE_DEVICE_TABLE() so that the driver can automatically be leaded on supported machines. Compile-tested only. Fixes: 23902f98f8d4 ("hwmon: add HP WMI Sensors driver") Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/hwmon/hp-wmi-sensors.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c index b5325d0e72b9..493a3b3e86f3 100644 --- a/drivers/hwmon/hp-wmi-sensors.c +++ b/drivers/hwmon/hp-wmi-sensors.c @@ -25,6 +25,7 @@ #include <linux/debugfs.h> #include <linux/hwmon.h> #include <linux/jiffies.h> +#include <linux/module.h> #include <linux/mutex.h> #include <linux/nls.h> #include <linux/units.h> @@ -2072,6 +2073,7 @@ static const struct wmi_device_id hp_wmi_sensors_id_table[] = { { HP_WMI_NUMERIC_SENSOR_GUID, NULL }, {}, }; +MODULE_DEVICE_TABLE(wmi, hp_wmi_sensors_id_table); static struct wmi_driver hp_wmi_sensors_driver = { .driver = { .name = "hp-wmi-sensors" }, -- 2.39.2
Currently, the hp-wmi-sensors driver needs to be loaded manually on supported machines. This however is unnecessary since the WMI id table can be used to support autoloading. However the driver might conflict with the hp-wmi driver since both seem to use the same WMI GUID for registering notify handler. I am thus submitting this patch as an RFC for now. Armin Wolf (1): hwmon: (hp-wmi-sensors) Add missing MODULE_DEVICE_TABLE() drivers/hwmon/hp-wmi-sensors.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.39.2
Hi Radu, kernel test robot noticed the following build errors: [auto build test ERROR on groeck-staging/hwmon-next] [also build test ERROR on robh/for-next linus/master v6.8 next-20240318] [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/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403190552.U4RHYvqc-lkp@intel.com/ All error/warnings (new ones prefixed by >>): drivers/hwmon/pmbus/adp1050.c: In function 'adp1050_probe': >> drivers/hwmon/pmbus/adp1050.c:47:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion] 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | char * In file included from include/linux/device.h:15, from include/linux/acpi.h:14, from include/linux/i2c.h:13, from drivers/hwmon/pmbus/adp1050.c:9: include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *' 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ~~~~^~~ >> drivers/hwmon/pmbus/adp1050.c:47:17: error: too few arguments to function 'dev_err_probe' 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ^~~~~~~~~~~~~ include/linux/dev_printk.h:277:20: note: declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^~~~~~~~~~~~~ drivers/hwmon/pmbus/adp1050.c:53:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion] 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | char * include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *' 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ~~~~^~~ drivers/hwmon/pmbus/adp1050.c:53:17: error: too few arguments to function 'dev_err_probe' 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ^~~~~~~~~~~~~ include/linux/dev_printk.h:277:20: note: declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^~~~~~~~~~~~~ vim +/dev_err_probe +47 drivers/hwmon/pmbus/adp1050.c > 9 #include <linux/i2c.h> 10 #include <linux/init.h> 11 #include <linux/kernel.h> 12 #include <linux/module.h> 13 #include <linux/of.h> 14 #include "pmbus.h" 15 16 #define ADP1050_CHIP_PASSWORD 0xD7 17 18 #define ADP1050_VIN_SCALE_MONITOR 0xD8 19 #define ADP1050_IIN_SCALE_MONITOR 0xD9 20 21 static struct pmbus_driver_info adp1050_info = { 22 .pages = 1, 23 .format[PSC_VOLTAGE_IN] = linear, 24 .format[PSC_VOLTAGE_OUT] = linear, 25 .format[PSC_CURRENT_IN] = linear, 26 .format[PSC_TEMPERATURE] = linear, 27 .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT 28 | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT 29 | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP 30 | PMBUS_HAVE_STATUS_TEMP, 31 }; 32 33 static int adp1050_probe(struct i2c_client *client) 34 { 35 u32 vin_scale_monitor, iin_scale_monitor; 36 int ret; 37 38 if (!i2c_check_functionality(client->adapter, 39 I2C_FUNC_SMBUS_WRITE_WORD_DATA)) 40 return -ENODEV; 41 42 /* Unlock CHIP's password in order to be able to read/write to it's 43 * VIN_SCALE and IIN_SCALE registers. 44 */ 45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 46 if (ret < 0) { > 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n"); 48 return ret; 49 } 50 51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 52 if (ret < 0) { 53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); 54 return ret; 55 } 56 57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the 58 * VIN monitor isn't used, therefore 0 is used as scale in order 59 * for the readings to return 0. 60 */ 61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", 62 &vin_scale_monitor)) 63 vin_scale_monitor = 0; 64 65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the 66 * IIN monitor isn't used, therefore 0 is used as scale in order 67 * for the readings to return 0. 68 */ 69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", 70 &iin_scale_monitor)) 71 iin_scale_monitor = 0; 72 73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, 74 vin_scale_monitor); 75 if (ret < 0) 76 return ret; 77 78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, 79 iin_scale_monitor); 80 if (ret < 0) 81 return ret; 82 83 return pmbus_do_probe(client, &adp1050_info); 84 } 85 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Convert adc128d818 bindings to dtschema to support validation. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- .../devicetree/bindings/hwmon/adc128d818.txt | 38 ------------- .../devicetree/bindings/hwmon/ti,adc128d818.yaml | 63 ++++++++++++++++++++++ 2 files changed, 63 insertions(+), 38 deletions(-) diff --git a/Documentation/devicetree/bindings/hwmon/adc128d818.txt b/Documentation/devicetree/bindings/hwmon/adc128d818.txt deleted file mode 100644 index d0ae46d7bac3..000000000000 --- a/Documentation/devicetree/bindings/hwmon/adc128d818.txt +++ /dev/null @@ -1,38 +0,0 @@ -TI ADC128D818 ADC System Monitor With Temperature Sensor --------------------------------------------------------- - -Operation modes: - - - Mode 0: 7 single-ended voltage readings (IN0-IN6), - 1 temperature reading (internal) - - Mode 1: 8 single-ended voltage readings (IN0-IN7), - no temperature - - Mode 2: 4 pseudo-differential voltage readings - (IN0-IN1, IN3-IN2, IN4-IN5, IN7-IN6), - 1 temperature reading (internal) - - Mode 3: 4 single-ended voltage readings (IN0-IN3), - 2 pseudo-differential voltage readings - (IN4-IN5, IN7-IN6), - 1 temperature reading (internal) - -If no operation mode is configured via device tree, the driver keeps the -currently active chip operation mode (default is mode 0). - - -Required node properties: - - - compatible: must be set to "ti,adc128d818" - - reg: I2C address of the device - -Optional node properties: - - - ti,mode: Operation mode (u8) (see above). - - -Example (operation mode 2): - - adc128d818@1d { - compatible = "ti,adc128d818"; - reg = <0x1d>; - ti,mode = /bits/ 8 <2>; - }; diff --git a/Documentation/devicetree/bindings/hwmon/ti,adc128d818.yaml b/Documentation/devicetree/bindings/hwmon/ti,adc128d818.yaml new file mode 100644 index 000000000000..b48a9841600e --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ti,adc128d818.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/hwmon/ti,adc128d818.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments ADC128D818 ADC System Monitor With Temperature Sensor + +maintainers: + - Javier Carrasco <javier.carrasco.cruz@gmail.com> + +description: | + The ADC128D818 is a 12-Bit, 8-Channel Analog to Digital Converter (ADC) + with a temperature sensor and an I2C interface. + + Datasheets: + https://www.ti.com/product/ADC128D818 + +properties: + compatible: + const: ti,adc128d818 + + reg: + maxItems: 1 + + ti,mode: + $ref: /schemas/types.yaml#/definitions/uint8 + description: + Operation mode. + Mode 0 - 7 single-ended voltage readings (IN0-IN6), 1 temperature + reading (internal). + Mode 1 - 8 single-ended voltage readings (IN0-IN7), no temperature. + Mode 2 - 4 pseudo-differential voltage readings + (IN0-IN1, IN3-IN2, IN4-IN5, IN7-IN6), 1 temperature reading (internal). + Mode 3 - 4 single-ended voltage readings (IN0-IN3), 2 pseudo-differential + voltage readings (IN4-IN5, IN7-IN6), 1 temperature reading (internal). + default: 0 + + vref-supply: + description: + The regulator to use as an external reference. If it does not exist, the + internal reference will be used. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + adc@1d { + compatible = "ti,adc128d818"; + reg = <0x1d>; + vref-supply = <&vref>; + ti,mode = /bits/ 8 <2>; + }; + }; --- base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400 change-id: 20240318-adc128d818_dtschema-02b78fc26f65 Best regards, -- Javier Carrasco <javier.carrasco.cruz@gmail.com>
On 3/18/24 11:29, Kousik Sanagavarapu wrote:
> On Mon, Mar 18, 2024 at 11:11:29AM -0700, Guenter Roeck wrote:
>> On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
>>> Update links in the documentation and in-code comments which point to
>>> the datasheet.
>>>
>>> The current links don't work because National Semiconductor (which is
>>> the manufacturer of this board and lm70) has been a part of Texas
>> ^^^^^^^^^^
>>
>> Is this a leftover from the other patch ? The lm70 driver supports
>> the LM70 chip, not a specific board.
>
> Yeah, it should be "the manufacturer of lm70". Thanks for spotting.
>
> Should I fix and resend this specific patch as v3 or would you edit it
> while pulling?
>
I'll edit it.
Thanks,
Guenter
On Mon, Mar 18, 2024 at 11:11:29AM -0700, Guenter Roeck wrote:
> On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
> > Update links in the documentation and in-code comments which point to
> > the datasheet.
> >
> > The current links don't work because National Semiconductor (which is
> > the manufacturer of this board and lm70) has been a part of Texas
> ^^^^^^^^^^
>
> Is this a leftover from the other patch ? The lm70 driver supports
> the LM70 chip, not a specific board.
Yeah, it should be "the manufacturer of lm70". Thanks for spotting.
Should I fix and resend this specific patch as v3 or would you edit it
while pulling?
On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
> Update links in the documentation and in-code comments which point to
> the datasheet.
>
> The current links don't work because National Semiconductor (which is
> the manufacturer of this board and lm70) has been a part of Texas
^^^^^^^^^^
Is this a leftover from the other patch ? The lm70 driver supports
the LM70 chip, not a specific board.
Guenter
On 18/03/2024 17:48, Guenter Roeck wrote:
> On 3/18/24 09:12, Krzysztof Kozlowski wrote:
>> On 18/03/2024 12:21, Radu Sabau wrote:
>>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>>> with PMBus interface Voltage, Current and Temperature Monitor.
>>>
>>
>> ...
>>
>>> +static int adp1050_probe(struct i2c_client *client)
>>> +{
>>> + u32 vin_scale_monitor, iin_scale_monitor;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + /* Unlock CHIP's password in order to be able to read/write to it's
>>> + * VIN_SCALE and IIN_SCALE registers.
>>> + */
>>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>>> + if (ret < 0) {
>>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>>
>> Syntax is: return dev_err_probe(). Same in other places.
>>
>
> dev_err_probe() expects the error number as second parameter, so I don't
> really understand how the above even compiles.
I did not explain the arguments, because they are obvious... but if you
need so:
return dev_err_probe(&client->dev, ret, "Device can't be unlocked.\n");
Best regards,
Krzysztof
On 3/18/24 10:44, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>
> The MP2971/MP2973 use a custom 16bit register format for
> SMBALERT_MASK which doesn't follow the PMBUS specification.
>
> Map the PMBUS defined bits used by the common code onto the custom
> format used by MPS and since the SMBALERT_MASK is currently never read
> by common code only implement the mapping for write transactions.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
>
One of those "getting burned either way" thingies. We are not
supposed to apply patches while the commit window is open, but then
we get resends if we don't. Oh well.
Yes, I'll apply your patch, but only after the commit window closed.
Guenter
From: Patrick Rudolph <patrick.rudolph@9elements.com> The MP2971/MP2973 use a custom 16bit register format for SMBALERT_MASK which doesn't follow the PMBUS specification. Map the PMBUS defined bits used by the common code onto the custom format used by MPS and since the SMBALERT_MASK is currently never read by common code only implement the mapping for write transactions. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> --- Changes in V3: 1. Avoid precedence issues in Macro 2. Optimise macro. Changes in V2: 1. Add/Update comment 2. Update SWAP define to include both variable. 3. Add defines for each bits of SMBALERT mask. --- drivers/hwmon/pmbus/mp2975.c | 77 ++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c index e5fa10b3b8bc..953c02a2aeb5 100644 --- a/drivers/hwmon/pmbus/mp2975.c +++ b/drivers/hwmon/pmbus/mp2975.c @@ -392,6 +392,82 @@ static int mp2973_read_word_data(struct i2c_client *client, int page, return ret; } +static int mp2973_write_word_data(struct i2c_client *client, int page, + int reg, u16 word) +{ + u8 target, mask; + int ret; + + if (reg != PMBUS_SMBALERT_MASK) + return -ENODATA; + + /* + * Vendor-specific SMBALERT_MASK register with 16 maskable bits. + */ + ret = pmbus_read_word_data(client, 0, 0, PMBUS_SMBALERT_MASK); + if (ret < 0) + return ret; + + target = word & 0xff; + mask = word >> 8; + +/* + * Set/Clear 'bit' in 'ret' based on condition followed by define for each bit in SMBALERT_MASK. + * Also bit 2 & 15 are reserved. + */ +#define SWAP(val, mask, cond, bit) (((mask) & (cond)) ? ((val) & ~BIT(bit)) : ((val) | BIT(bit))) + +#define MP2973_TEMP_OT 0 +#define MP2973_VIN_UVLO 1 +#define MP2973_VIN_OVP 3 +#define MP2973_MTP_FAULT 4 +#define MP2973_OTHER_COMM 5 +#define MP2973_MTP_BLK_TRIG 6 +#define MP2973_PACKET_ERROR 7 +#define MP2973_INVALID_DATA 8 +#define MP2973_INVALID_COMMAND 9 +#define MP2973_IOUT_OC_LV 10 +#define MP2973_IOUT_OC 11 +#define MP2973_VOUT_MAX_MIN_WARNING 12 +#define MP2973_VOLTAGE_UV 13 +#define MP2973_VOLTAGE_OV 14 + + switch (target) { + case PMBUS_STATUS_CML: + ret = SWAP(ret, mask, PB_CML_FAULT_INVALID_DATA, MP2973_INVALID_DATA); + ret = SWAP(ret, mask, PB_CML_FAULT_INVALID_COMMAND, MP2973_INVALID_COMMAND); + ret = SWAP(ret, mask, PB_CML_FAULT_OTHER_COMM, MP2973_OTHER_COMM); + ret = SWAP(ret, mask, PB_CML_FAULT_PACKET_ERROR, MP2973_PACKET_ERROR); + break; + case PMBUS_STATUS_VOUT: + ret = SWAP(ret, mask, PB_VOLTAGE_UV_FAULT, MP2973_VOLTAGE_UV); + ret = SWAP(ret, mask, PB_VOLTAGE_OV_FAULT, MP2973_VOLTAGE_OV); + break; + case PMBUS_STATUS_IOUT: + ret = SWAP(ret, mask, PB_IOUT_OC_FAULT, MP2973_IOUT_OC); + ret = SWAP(ret, mask, PB_IOUT_OC_LV_FAULT, MP2973_IOUT_OC_LV); + break; + case PMBUS_STATUS_TEMPERATURE: + ret = SWAP(ret, mask, PB_TEMP_OT_FAULT, MP2973_TEMP_OT); + break; + /* + * Map remaining bits to MFR specific to let the PMBUS core mask + * those bits by default. + */ + case PMBUS_STATUS_MFR_SPECIFIC: + ret = SWAP(ret, mask, BIT(1), MP2973_VIN_UVLO); + ret = SWAP(ret, mask, BIT(3), MP2973_VIN_OVP); + ret = SWAP(ret, mask, BIT(4), MP2973_MTP_FAULT); + ret = SWAP(ret, mask, BIT(6), MP2973_MTP_BLK_TRIG); + break; + default: + return 0; + } +#undef SWAP + + return pmbus_write_word_data(client, 0, PMBUS_SMBALERT_MASK, ret); +} + static int mp2975_read_word_data(struct i2c_client *client, int page, int phase, int reg) { @@ -907,6 +983,7 @@ static struct pmbus_driver_info mp2973_info = { PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT, .read_word_data = mp2973_read_word_data, + .write_word_data = mp2973_write_word_data, #if IS_ENABLED(CONFIG_SENSORS_MP2975_REGULATOR) .num_regulators = 1, .reg_desc = mp2975_reg_desc, base-commit: 8debe3c1295ef36958dae77487eed9cf6584c008 -- 2.42.0
On Mon, 18 Mar 2024 21:08:33 +0530, Kousik Sanagavarapu wrote: > Fix links from the documentation and in-code comments pointing to > datasheets. > > Changes since v1: > - I forgot to fix the links in in-code comments in spi, so do that. > - New commit to address the same issues in hwmon/lm70 since the spi eval board > is based on that. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: lm70llp: fix links in doc and comments commit: 7397175cb7b48f7a3fc699083aa46f1234904c7e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On 18/03/2024 17:02, Krzysztof Kozlowski wrote: > On 18/03/2024 10:48, Chanh Nguyen wrote: >> >> >> On 11/03/2024 23:56, Krzysztof Kozlowski wrote: >>> On 11/03/2024 12:13, Chanh Nguyen wrote: >>>> Add pwmout-pin-as-tach-input property. >>> >>> Why is this split from original binding? This does not make much >>> sense... Add complete hardware description. >>> >> >> Ok Krzysztof, I will merg the "[PATCH 1/3] dt-bindings: hwmon: Add maxim >> max31790 driver bindings" commit and "[PATCH 3/3] dt-bindings: hwmon: >> max31790: Add pwmout-pin-as-tach-input property" commit. > > Later I checked your driver code and this explains a bit. However first > patch should explain that instead. The split is fine, but just lack of > rationale is confusing. > Thank Krzysztof. I'll try to explain in detail each patch in v2. > >> >>>> >>>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>>> --- >>>> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> index 5a93e6bdebda..447cac17053a 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> @@ -25,6 +25,16 @@ properties: >>>> reg: >>>> maxItems: 1 >>>> >>>> + pwmout-pin-as-tach-input: >>>> + description: | >>>> + An array of six integers responds to six PWM channels for >>>> + configuring the pwm to tach mode. >>>> + When set to 0, the associated PWMOUT produces a PWM waveform for >>>> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >>> >>> No vendor prefix, so generic property... but where is it defined? >>> >> >> Thank Krzysztof! It is not generic property, I'll add the vendor prefix. >> >> I'll update the "pwmout-pin-as-tach-input" to >> "maxim,pwmout-pin-as-tach-input" at v2. > > Except that you should really look into common properties and use them. > Or explain why do you need new property? > > Best regards, > Krzysztof > I'm also discussing with Rob Herring that on patch 3/3, I checked in the Documentation/devicetree/bindings/hwmon/fan-common.yaml. I found the tach-ch property, but it seems to define the tach channel used for the fan. It does not match my purpose. I want to configure the PWM-OUT pin to become a TACH-IN pin. I wonder if I can use the tach-ch property for my purpose.
On 3/18/24 09:12, Krzysztof Kozlowski wrote:
> On 18/03/2024 12:21, Radu Sabau wrote:
>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>> with PMBus interface Voltage, Current and Temperature Monitor.
>>
>
> ...
>
>> +static int adp1050_probe(struct i2c_client *client)
>> +{
>> + u32 vin_scale_monitor, iin_scale_monitor;
>> + int ret;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>> + return -ENODEV;
>> +
>> + /* Unlock CHIP's password in order to be able to read/write to it's
>> + * VIN_SCALE and IIN_SCALE registers.
>> + */
>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>> + if (ret < 0) {
>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>
> Syntax is: return dev_err_probe(). Same in other places.
>
dev_err_probe() expects the error number as second parameter, so I don't
really understand how the above even compiles.
Guenter
On 3/18/24 09:09, Uwe Kleine-König wrote: > This prepares the aspeed-g6-pwm-tacho driver to further changes of the > pwm core outlined in the commit introducing devm_pwmchip_alloc(). There > is no intended semantical change and the driver should behave as before. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/aspeed-g6-pwm-tach.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c > index 64def5e4cdf6..332c02216668 100644 > --- a/drivers/hwmon/aspeed-g6-pwm-tach.c > +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c > @@ -136,7 +136,6 @@ struct aspeed_pwm_tach_data { > struct clk *clk; > struct reset_control *reset; > unsigned long clk_rate; > - struct pwm_chip chip; > bool tach_present[TACH_ASPEED_NR_TACHS]; > u32 tach_divisor; > }; > @@ -144,7 +143,7 @@ struct aspeed_pwm_tach_data { > static inline struct aspeed_pwm_tach_data * > aspeed_pwm_chip_to_data(struct pwm_chip *chip) > { > - return container_of(chip, struct aspeed_pwm_tach_data, chip); > + return pwmchip_get_drvdata(chip); > } > > static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > @@ -459,6 +458,7 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev) > int ret; > struct device_node *child; > struct aspeed_pwm_tach_data *priv; > + struct pwm_chip *chip; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -487,11 +487,14 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev) > if (ret) > return ret; > > - priv->chip.dev = dev; > - priv->chip.ops = &aspeed_pwm_ops; > - priv->chip.npwm = PWM_ASPEED_NR_PWMS; > + chip = devm_pwmchip_alloc(dev, PWM_ASPEED_NR_PWMS, 0); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > > - ret = devm_pwmchip_add(dev, &priv->chip); > + pwmchip_set_drvdata(chip, priv); > + chip->ops = &aspeed_pwm_ops; > + > + ret = devm_pwmchip_add(dev, chip); > if (ret) > return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); >
On 3/18/24 09:09, Uwe Kleine-König wrote: > struct pwm_chip::dev is about to change. To not have to touch this > driver in the same commit as struct pwm_chip::dev, use the accessor > function provided for exactly this purpose. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/aspeed-g6-pwm-tach.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c > index 597b3b019d49..64def5e4cdf6 100644 > --- a/drivers/hwmon/aspeed-g6-pwm-tach.c > +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c > @@ -195,7 +195,7 @@ static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > expect_period = div64_u64(ULLONG_MAX, (u64)priv->clk_rate); > expect_period = min(expect_period, state->period); > - dev_dbg(chip->dev, "expect period: %lldns, duty_cycle: %lldns", > + dev_dbg(pwmchip_parent(chip), "expect period: %lldns, duty_cycle: %lldns", > expect_period, state->duty_cycle); > /* > * Pick the smallest value for div_h so that div_l can be the biggest > @@ -218,12 +218,12 @@ static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (div_l > 255) > div_l = 255; > > - dev_dbg(chip->dev, "clk source: %ld div_h %lld, div_l : %lld\n", > + dev_dbg(pwmchip_parent(chip), "clk source: %ld div_h %lld, div_l : %lld\n", > priv->clk_rate, div_h, div_l); > /* duty_pt = duty_cycle * (PERIOD + 1) / period */ > duty_pt = div64_u64(state->duty_cycle * priv->clk_rate, > (u64)NSEC_PER_SEC * (div_l + 1) << div_h); > - dev_dbg(chip->dev, "duty_cycle = %lld, duty_pt = %d\n", > + dev_dbg(pwmchip_parent(chip), "duty_cycle = %lld, duty_pt = %d\n", > state->duty_cycle, duty_pt); > > /*
This prepares the aspeed-g6-pwm-tacho driver to further changes of the pwm core outlined in the commit introducing devm_pwmchip_alloc(). There is no intended semantical change and the driver should behave as before. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/hwmon/aspeed-g6-pwm-tach.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c index 64def5e4cdf6..332c02216668 100644 --- a/drivers/hwmon/aspeed-g6-pwm-tach.c +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c @@ -136,7 +136,6 @@ struct aspeed_pwm_tach_data { struct clk *clk; struct reset_control *reset; unsigned long clk_rate; - struct pwm_chip chip; bool tach_present[TACH_ASPEED_NR_TACHS]; u32 tach_divisor; }; @@ -144,7 +143,7 @@ struct aspeed_pwm_tach_data { static inline struct aspeed_pwm_tach_data * aspeed_pwm_chip_to_data(struct pwm_chip *chip) { - return container_of(chip, struct aspeed_pwm_tach_data, chip); + return pwmchip_get_drvdata(chip); } static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, @@ -459,6 +458,7 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev) int ret; struct device_node *child; struct aspeed_pwm_tach_data *priv; + struct pwm_chip *chip; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -487,11 +487,14 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev) if (ret) return ret; - priv->chip.dev = dev; - priv->chip.ops = &aspeed_pwm_ops; - priv->chip.npwm = PWM_ASPEED_NR_PWMS; + chip = devm_pwmchip_alloc(dev, PWM_ASPEED_NR_PWMS, 0); + if (IS_ERR(chip)) + return PTR_ERR(chip); - ret = devm_pwmchip_add(dev, &priv->chip); + pwmchip_set_drvdata(chip, priv); + chip->ops = &aspeed_pwm_ops; + + ret = devm_pwmchip_add(dev, chip); if (ret) return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); -- 2.43.0
Hello, there is a pending rework for the pwm framework[1] that requires a preparatory change for all pwm drivers. When creating them I wasn't aware of the aspeed-g6-pwm-tacho driver, just found this now while doing build tests with my series. To not delay application of my series, I'd like to take the two patches from this series via my pwm tree. To state the (maybe) obvious: This series depends on the following commits: 7e1449cd15d1 "hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach" 024913dbf99f pwm: Provide pwmchip_alloc() function and a devm variant of it 4e59267c7a20 pwm: Provide an inline function to get the parent device of a given chip The earliest commit containing all those is: 15223fdbdf4f "Merge tag 'hwmon-for-v6.9' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging" Best regards Uwe [1] https://lore.kernel.org/linux-pwm/cover.1710670958.git.u.kleine-koenig@pengutronix.de Uwe Kleine-König (2): hwmon: (aspeed-g6-pwm-tacho): Make use of pwmchip_parent() accessor hwmon: (aspeed-g6-pwm-tacho): Make use of devm_pwmchip_alloc() function drivers/hwmon/aspeed-g6-pwm-tach.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) base-commit: f6cef5f8c37f58a3bc95b3754c3ae98e086631ca -- 2.43.0
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, use the accessor function provided for exactly this purpose. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/hwmon/aspeed-g6-pwm-tach.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c index 597b3b019d49..64def5e4cdf6 100644 --- a/drivers/hwmon/aspeed-g6-pwm-tach.c +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c @@ -195,7 +195,7 @@ static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, expect_period = div64_u64(ULLONG_MAX, (u64)priv->clk_rate); expect_period = min(expect_period, state->period); - dev_dbg(chip->dev, "expect period: %lldns, duty_cycle: %lldns", + dev_dbg(pwmchip_parent(chip), "expect period: %lldns, duty_cycle: %lldns", expect_period, state->duty_cycle); /* * Pick the smallest value for div_h so that div_l can be the biggest @@ -218,12 +218,12 @@ static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (div_l > 255) div_l = 255; - dev_dbg(chip->dev, "clk source: %ld div_h %lld, div_l : %lld\n", + dev_dbg(pwmchip_parent(chip), "clk source: %ld div_h %lld, div_l : %lld\n", priv->clk_rate, div_h, div_l); /* duty_pt = duty_cycle * (PERIOD + 1) / period */ duty_pt = div64_u64(state->duty_cycle * priv->clk_rate, (u64)NSEC_PER_SEC * (div_l + 1) << div_h); - dev_dbg(chip->dev, "duty_cycle = %lld, duty_pt = %d\n", + dev_dbg(pwmchip_parent(chip), "duty_cycle = %lld, duty_pt = %d\n", state->duty_cycle, duty_pt); /* -- 2.43.0
On 18/03/2024 12:21, Radu Sabau wrote: > Add support for ADP1050 Digital Controller for Isolated Power Supplies > with PMBus interface Voltage, Current and Temperature Monitor. > ... > +static int adp1050_probe(struct i2c_client *client) > +{ > + u32 vin_scale_monitor, iin_scale_monitor; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) > + return -ENODEV; > + > + /* Unlock CHIP's password in order to be able to read/write to it's > + * VIN_SCALE and IIN_SCALE registers. > + */ > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device can't be unlocked.\n"); Syntax is: return dev_err_probe(). Same in other places. > + return ret; > + } > + > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); > + return ret; > + } > + > + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the > + * VIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ Please use Linux coding style comments. /* and aligned */. > + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", > + &vin_scale_monitor)) > + vin_scale_monitor = 0; > + > + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the > + * IIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ > + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", > + &iin_scale_monitor)) > + iin_scale_monitor = 0; > + > + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, > + vin_scale_monitor); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, > + iin_scale_monitor); > + if (ret < 0) > + return ret; > + > + return pmbus_do_probe(client, &adp1050_info); > +} > + > +static const struct i2c_device_id adp1050_id[] = { > + {"adp1050", 0}, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, adp1050_id); > + > +static const struct of_device_id adp1050_of_match[] = { > + { .compatible = "adi,adp1050"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, adp1050_of_match); > + > +static struct i2c_driver adp1050_driver = { > + .driver = { > + .name = "adp1050", > + .of_match_table = of_match_ptr(adp1050_of_match), Drop of_match_ptr, you will have here warnings. > + }, > + .probe = adp1050_probe, > + .id_table = adp1050_id, > +}; > +module_i2c_driver(adp1050_driver); > + > +MODULE_AUTHOR("Radu Sabau <radu.sabau@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices ADP1050 HWMON PMBus Driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(PMBUS); Best regards, Krzysztof
On 18/03/2024 12:21, Radu Sabau wrote: > Add dt-bindings for adp1050 digital controller for isolated power supply > with pmbus interface voltage, current and temperature monitor. > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> Subject: drop space before ':' > --- > .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++ > MAINTAINERS | 8 +++ > 2 files changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > new file mode 100644 > index 000000000000..e3162d0df0e2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > @@ -0,0 +1,65 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > + Drop > +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml# > +$schema: htpps://devicetree.org/meta-schemes/core.yaml# > + > +title: Analog Devices ADP1050 digital controller with PMBus interface > + > +maintainers: > + - Radu Sabau <radu.sabau@analog.com> > + > +description: | > + The ADP1050 is used to monitor system voltages, currents and temperatures. > + Through the PMBus interface, the ADP1050 targets isolated power supplies > + and has four individual monitors for input/output voltage, input current > + and temperature. > + Datasheet: > + https://www.analog.com/en/products/adp1050.html Missing blank line > +properties: > + compatbile: Typo. And you did not test it... > + const: adi,adp1050 > + > + reg: > + maxItems: 1 > + > + vcc-supply: true > + > + adi,vin-scale-monitor: > + description: > + The value of the input voltage scale used by the internal ADP1050 ADC in > + order to read correct voltage values. > + $ref: /schemas/typees.yaml#/definitions/uint16 Missing blank line. > + adi,iin-scale-monitor: > + description: > + The value of the input current scale used by the internal ADP1050 ADC in > + order to read carrect current values. > + $ref: /schemas/typees.yaml#/definitions/uint16 > + > +required: > + - compatible > + - reg > + - vcc-supply > + - adi,vin-scale-monitor > + - adi,iin-scale-monitor > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #adress-cells = <1>; Totally messed indentation. Use 4 spaces for example indentation. > + #size-cells = <0>; > + clock-frequency = <100000>; > + adp1050@70 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + #adress-cells = <1>; > + #size-cells = <0>; > + compatible = "adi,adp1050"; > + reg = <0x70>; > + adi,vin-scale-monitor = <0xB030>; > + adi,iin-scale-monitor = <0x1>; > + vcc-supply = <&vcc>; > + }; > +... > + > diff --git a/MAINTAINERS b/MAINTAINERS > index f4d7f7cb7577..c90140859988 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -479,6 +479,14 @@ L: linux-wireless@vger.kernel.org > S: Orphan > F: drivers/net/wireless/admtek/adm8211.* > > +ADP1050 HARDWARE MONITOR DRIVER > +M: Radu Sabau <radu.sabau@analog.com> > +L: linux-hwmon@vger.kernel.org > +S: Supported > +W: https://ez.analog.com/linux-software-drivers > +F: Dcumentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > +F: drivers/hwmon/pmbus/adp1050.c There is no such file... Best regards, Krzysztof
Update links in the documentation and in-code comments which point to the datasheet. The current links don't work because National Semiconductor (which is the manufacturer of this board and lm70) has been a part of Texas Instruments since 2011 and hence http://www.national.com/ doesn't work anymore. Fixes: e1a8e913f97e ("[PATCH] lm70: New hardware monitoring driver") Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/hwmon/lm70.rst | 2 +- drivers/hwmon/lm70.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/hwmon/lm70.rst b/Documentation/hwmon/lm70.rst index 11303a7e16a8..02ed60dddffb 100644 --- a/Documentation/hwmon/lm70.rst +++ b/Documentation/hwmon/lm70.rst @@ -5,7 +5,7 @@ Supported chips: * National Semiconductor LM70 - Datasheet: http://www.national.com/pf/LM/LM70.html + Datasheet: https://www.ti.com/product/LM70 * Texas Instruments TMP121/TMP123 diff --git a/drivers/hwmon/lm70.c b/drivers/hwmon/lm70.c index c20a749fc7f2..481e4e1f8f4f 100644 --- a/drivers/hwmon/lm70.c +++ b/drivers/hwmon/lm70.c @@ -6,9 +6,9 @@ * Copyright (C) 2006 Kaiwan N Billimoria <kaiwan@designergraphix.com> * * The LM70 communicates with a host processor via an SPI/Microwire Bus - * interface. The complete datasheet is available at National's website + * interface. The complete datasheet is available at TI's website * here: - * http://www.national.com/pf/LM/LM70.html + * https://www.ti.com/product/LM70 */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -- 2.44.0.273.g4bc5b65358.dirty
Update links in the documentation and in-code comments which point to the datasheet and schematic. The current links don't work because National Semiconductor (which is the manufacturer of this board and lm70) has been a part of Texas Instruments since 2011 and hence http://www.national.com/ doesn't work anymore. Fixes: 78961a574037 ("spi_lm70llp parport adapter driver") Fixes: 2b7300513b98 ("hwmon: (lm70) Code streamlining and cleanup") Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/spi/spi-lm70llp.rst | 4 ++-- drivers/spi/spi-lm70llp.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/spi/spi-lm70llp.rst b/Documentation/spi/spi-lm70llp.rst index 2f20e5b405e6..ff98ddc76a74 100644 --- a/Documentation/spi/spi-lm70llp.rst +++ b/Documentation/spi/spi-lm70llp.rst @@ -6,7 +6,7 @@ Supported board/chip: * National Semiconductor LM70 LLP evaluation board - Datasheet: http://www.national.com/pf/LM/LM70.html + Datasheet: https://www.ti.com/lit/gpn/lm70 Author: Kaiwan N Billimoria <kaiwan@designergraphix.com> @@ -28,7 +28,7 @@ Hardware Interfacing The schematic for this particular board (the LM70EVAL-LLP) is available (on page 4) here: - http://www.national.com/appinfo/tempsensors/files/LM70LLPEVALmanual.pdf + https://download.datasheets.com/pdfs/documentation/nat/kit&board/lm70llpevalmanual.pdf The hardware interfacing on the LM70 LLP eval board is as follows: diff --git a/drivers/spi/spi-lm70llp.c b/drivers/spi/spi-lm70llp.c index f982bdebd028..3c0c24ed1f3d 100644 --- a/drivers/spi/spi-lm70llp.c +++ b/drivers/spi/spi-lm70llp.c @@ -29,10 +29,10 @@ * * Datasheet and Schematic: * The LM70 is a temperature sensor chip from National Semiconductor; its - * datasheet is available at http://www.national.com/pf/LM/LM70.html + * datasheet is available at https://www.ti.com/lit/gpn/lm70 * The schematic for this particular board (the LM70EVAL-LLP) is * available (on page 4) here: - * http://www.national.com/appinfo/tempsensors/files/LM70LLPEVALmanual.pdf + * https://download.datasheets.com/pdfs/documentation/nat/kit&board/lm70llpevalmanual.pdf * * Also see Documentation/spi/spi-lm70llp.rst. The SPI<->parport code here is * (heavily) based on spi-butterfly by David Brownell. -- 2.44.0.273.g4bc5b65358.dirty