Message ID | 20231124-ltc4282-support-v2-2-952bf926f83c@analog.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Add support for LTC4282 | expand |
Hi Nuno, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on linus/master v6.7-rc2 next-20231124] [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/Nuno-Sa-via-B4-Relay/hwmon-ltc4282-add-support-for-the-LTC4282-chip/20231124-231842 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231124-ltc4282-support-v2-2-952bf926f83c%40analog.com patch subject: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip config: i386-randconfig-r081-20231125 (https://download.01.org/0day-ci/archive/20231125/202311250203.ZhAso6Dj-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231125/202311250203.ZhAso6Dj-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/202311250203.ZhAso6Dj-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/ltc4282.c:347:6: warning: no previous prototype for function 'ltc4282_round_rate' [-Wmissing-prototypes] long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, ^ drivers/hwmon/ltc4282.c:347:1: note: declare 'static' if the function is not intended to be used outside of this translation unit long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, ^ static >> drivers/hwmon/ltc4282.c:356:15: warning: no previous prototype for function 'ltc4282_recalc_rate' [-Wmissing-prototypes] unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) ^ drivers/hwmon/ltc4282.c:356:1: note: declare 'static' if the function is not intended to be used outside of this translation unit unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) ^ static 2 warnings generated. vim +/ltc4282_round_rate +347 drivers/hwmon/ltc4282.c 346 > 347 long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, 348 unsigned long *parent_rate) 349 { 350 int idx = find_closest(rate, ltc4282_out_rates, 351 ARRAY_SIZE(ltc4282_out_rates)); 352 353 return ltc4282_out_rates[idx]; 354 } 355 > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) 357 { 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, 359 clk_hw); 360 u32 clkdiv; 361 int ret; 362 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); 364 if (ret) 365 return 0; 366 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); 368 if (!clkdiv) 369 return 0; 370 if (clkdiv == LTC4282_CLKOUT_INT) 371 return LTC4282_CLKOUT_SYSTEM; 372 373 return LTC4282_CLKOUT_CNV; 374 } 375
Hi Nuno, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on linus/master v6.7-rc2 next-20231124] [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/Nuno-Sa-via-B4-Relay/hwmon-ltc4282-add-support-for-the-LTC4282-chip/20231124-231842 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231124-ltc4282-support-v2-2-952bf926f83c%40analog.com patch subject: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip config: x86_64-randconfig-161-20231125 (https://download.01.org/0day-ci/archive/20231125/202311250548.lUn3bm1A-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231125/202311250548.lUn3bm1A-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/202311250548.lUn3bm1A-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/ltc4282.c:347:6: warning: no previous prototype for 'ltc4282_round_rate' [-Wmissing-prototypes] 347 | long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, | ^~~~~~~~~~~~~~~~~~ >> drivers/hwmon/ltc4282.c:356:15: warning: no previous prototype for 'ltc4282_recalc_rate' [-Wmissing-prototypes] 356 | unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) | ^~~~~~~~~~~~~~~~~~~ vim +/ltc4282_round_rate +347 drivers/hwmon/ltc4282.c 346 > 347 long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, 348 unsigned long *parent_rate) 349 { 350 int idx = find_closest(rate, ltc4282_out_rates, 351 ARRAY_SIZE(ltc4282_out_rates)); 352 353 return ltc4282_out_rates[idx]; 354 } 355 > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) 357 { 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, 359 clk_hw); 360 u32 clkdiv; 361 int ret; 362 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); 364 if (ret) 365 return 0; 366 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); 368 if (!clkdiv) 369 return 0; 370 if (clkdiv == LTC4282_CLKOUT_INT) 371 return LTC4282_CLKOUT_SYSTEM; 372 373 return LTC4282_CLKOUT_CNV; 374 } 375
On Sat, 2023-11-25 at 07:39 +0800, kernel test robot wrote: > Hi Nuno, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on groeck-staging/hwmon-next] > [also build test WARNING on linus/master v6.7-rc2 next-20231124] > [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/Nuno-Sa-via-B4-Relay/hwmon-ltc4282-add-support-for-the-LTC4282-chip/20231124-231842 > base: > https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next > patch link: > https://lore.kernel.org/r/20231124-ltc4282-support-v2-2-952bf926f83c%40analog.com > patch subject: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip > config: x86_64-randconfig-161-20231125 > (https://download.01.org/0day-ci/archive/20231125/202311250548.lUn3bm1A-lkp@intel.c > om/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20231125/202311250548.lUn3bm1A-lkp@intel.c > om/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/202311250548.lUn3bm1A-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > > > drivers/hwmon/ltc4282.c:347:6: warning: no previous prototype for > > > 'ltc4282_round_rate' [-Wmissing-prototypes] > 347 | long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, > | ^~~~~~~~~~~~~~~~~~ > > > drivers/hwmon/ltc4282.c:356:15: warning: no previous prototype for > > > 'ltc4282_recalc_rate' [-Wmissing-prototypes] > 356 | unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long > parent) > | ^~~~~~~~~~~~~~~~~~~ > > > vim +/ltc4282_round_rate +347 drivers/hwmon/ltc4282.c > > 346 > > 347 long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, > 348 unsigned long *parent_rate) > 349 { > 350 int idx = find_closest(rate, ltc4282_out_rates, > 351 ARRAY_SIZE(ltc4282_out_rates)); > 352 > 353 return ltc4282_out_rates[idx]; > 354 } > 355 > > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) > 357 { > 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, > 359 clk_hw); > 360 u32 clkdiv; > 361 int ret; > 362 > 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); > 364 if (ret) > 365 return 0; > 366 > 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); > 368 if (!clkdiv) > 369 return 0; > 370 if (clkdiv == LTC4282_CLKOUT_INT) > 371 return LTC4282_CLKOUT_SYSTEM; > 372 > 373 return LTC4282_CLKOUT_CNV; > 374 } > 375 > Arghh, I do need to see if I can add some test branch of my own to the test robot :/. Anyways, will wait for some more reviewing before sending v3 to address this. - Nuno Sá
On 27/11/2023 08:53, Nuno Sá wrote: >> 355 >> > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) >> 357 { >> 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, >> 359 clk_hw); >> 360 u32 clkdiv; >> 361 int ret; >> 362 >> 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); >> 364 if (ret) >> 365 return 0; >> 366 >> 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); >> 368 if (!clkdiv) >> 369 return 0; >> 370 if (clkdiv == LTC4282_CLKOUT_INT) >> 371 return LTC4282_CLKOUT_SYSTEM; >> 372 >> 373 return LTC4282_CLKOUT_CNV; >> 374 } >> 375 >> > > Arghh, I do need to see if I can add some test branch of my own to the test robot :/. > Anyways, will wait for some more reviewing before sending v3 to address this. It's easy to test your patches on your own machines... Just build few different configs. Best regards, Krzysztof
On 27/11/2023 09:10, Krzysztof Kozlowski wrote: > On 27/11/2023 08:53, Nuno Sá wrote: >>> 355 >>> > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) >>> 357 { >>> 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, >>> 359 clk_hw); >>> 360 u32 clkdiv; >>> 361 int ret; >>> 362 >>> 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); >>> 364 if (ret) >>> 365 return 0; >>> 366 >>> 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); >>> 368 if (!clkdiv) >>> 369 return 0; >>> 370 if (clkdiv == LTC4282_CLKOUT_INT) >>> 371 return LTC4282_CLKOUT_SYSTEM; >>> 372 >>> 373 return LTC4282_CLKOUT_CNV; >>> 374 } >>> 375 >>> >> >> Arghh, I do need to see if I can add some test branch of my own to the test robot :/. >> Anyways, will wait for some more reviewing before sending v3 to address this. > > It's easy to test your patches on your own machines... Just build few > different configs. > Wait, this was not even unusual test, just standard compile, which means you did not do basic tests on your end. You must build your new driver with W=1, smatch, sparse and coccinelle before sending upstream. Best regards, Krzysztof
On Mon, 2023-11-27 at 09:12 +0100, Krzysztof Kozlowski wrote: > On 27/11/2023 09:10, Krzysztof Kozlowski wrote: > > On 27/11/2023 08:53, Nuno Sá wrote: > > > > 355 > > > > > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long > > > > parent) > > > > 357 { > > > > 358 struct ltc4282_state *st = container_of(hw, struct > > > > ltc4282_state, > > > > 359 clk_hw); > > > > 360 u32 clkdiv; > > > > 361 int ret; > > > > 362 > > > > 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); > > > > 364 if (ret) > > > > 365 return 0; > > > > 366 > > > > 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); > > > > 368 if (!clkdiv) > > > > 369 return 0; > > > > 370 if (clkdiv == LTC4282_CLKOUT_INT) > > > > 371 return LTC4282_CLKOUT_SYSTEM; > > > > 372 > > > > 373 return LTC4282_CLKOUT_CNV; > > > > 374 } > > > > 375 > > > > > > > > > > Arghh, I do need to see if I can add some test branch of my own to the test > > > robot :/. > > > Anyways, will wait for some more reviewing before sending v3 to address this. > > > > It's easy to test your patches on your own machines... Just build few > > different configs. > > > > Wait, this was not even unusual test, just standard compile, which means > you did not do basic tests on your end. You must build your new driver > with W=1, smatch, sparse and coccinelle before sending upstream. > > Best regards, > Krzysztof > Well, I do agree that a sparse build would easily caught this and I'm mad with myself because that much (together with checkpatch) I usually run (I confess that coccinelle and smatch is something that I don't usually do. "you did not do basic tests on your end" However I do think this is a bit unfair. I did got a lot of things to do in v2 with major refactors on some of the features. So "basic test" to me is actually making sure that all of these changes don't break the driver and the device still works as expected. Anyways, I don't want to find any execuses and start an argument. Will try to do better in next iterations. - Nuno Sá
Hi Nuno, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on linus/master v6.7-rc3 next-20231127] [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/Nuno-Sa-via-B4-Relay/hwmon-ltc4282-add-support-for-the-LTC4282-chip/20231124-231842 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231124-ltc4282-support-v2-2-952bf926f83c%40analog.com patch subject: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip config: nios2-randconfig-r111-20231127 (https://download.01.org/0day-ci/archive/20231127/202311271611.huVhrU16-lkp@intel.com/config) compiler: nios2-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20231127/202311271611.huVhrU16-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/202311271611.huVhrU16-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/hwmon/ltc4282.c:347:6: sparse: sparse: symbol 'ltc4282_round_rate' was not declared. Should it be static? >> drivers/hwmon/ltc4282.c:356:15: sparse: sparse: symbol 'ltc4282_recalc_rate' was not declared. Should it be static? >> drivers/hwmon/ltc4282.c:967:34: sparse: sparse: dubious: x & !y drivers/hwmon/ltc4282.c:1057:34: sparse: sparse: dubious: x & !y drivers/hwmon/ltc4282.c: note: in included file (through include/uapi/linux/swab.h, include/linux/swab.h, include/uapi/linux/byteorder/little_endian.h, ...): arch/nios2/include/uapi/asm/swab.h:25:24: sparse: sparse: too many arguments for function __builtin_custom_ini arch/nios2/include/uapi/asm/swab.h:25:24: sparse: sparse: too many arguments for function __builtin_custom_ini arch/nios2/include/uapi/asm/swab.h:31:24: sparse: sparse: too many arguments for function __builtin_custom_ini arch/nios2/include/uapi/asm/swab.h:31:24: sparse: sparse: too many arguments for function __builtin_custom_ini arch/nios2/include/uapi/asm/swab.h:25:24: sparse: sparse: too many arguments for function __builtin_custom_ini arch/nios2/include/uapi/asm/swab.h:25:24: sparse: sparse: too many arguments for function __builtin_custom_ini vim +/ltc4282_round_rate +347 drivers/hwmon/ltc4282.c 346 > 347 long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, 348 unsigned long *parent_rate) 349 { 350 int idx = find_closest(rate, ltc4282_out_rates, 351 ARRAY_SIZE(ltc4282_out_rates)); 352 353 return ltc4282_out_rates[idx]; 354 } 355 > 356 unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) 357 { 358 struct ltc4282_state *st = container_of(hw, struct ltc4282_state, 359 clk_hw); 360 u32 clkdiv; 361 int ret; 362 363 ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); 364 if (ret) 365 return 0; 366 367 clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); 368 if (!clkdiv) 369 return 0; 370 if (clkdiv == LTC4282_CLKOUT_INT) 371 return LTC4282_CLKOUT_SYSTEM; 372 373 return LTC4282_CLKOUT_CNV; 374 } 375
On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: > On 27/11/2023 09:10, Krzysztof Kozlowski wrote: ... > Wait, this was not even unusual test, just standard compile, which means > you did not do basic tests on your end. You must build your new driver > with W=1, smatch, sparse and coccinelle before sending upstream. Well, sparse is lagging in development, for the last year it's at least two times it broke kernel builds because of being not ready for the new stuff used in the kernel. Do we have anybody to sync this? I don't think so, hence requiring this from developer is doubtful. Otherwise I agree, that basic compilation with GCC/LLVM must be done.
On 27/11/2023 17:03, Andy Shevchenko wrote: > On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: >> On 27/11/2023 09:10, Krzysztof Kozlowski wrote: > > ... > >> Wait, this was not even unusual test, just standard compile, which means >> you did not do basic tests on your end. You must build your new driver >> with W=1, smatch, sparse and coccinelle before sending upstream. > > Well, sparse is lagging in development, for the last year it's at least two > times it broke kernel builds because of being not ready for the new stuff used > in the kernel. Do we have anybody to sync this? I don't think so, hence > requiring this from developer is doubtful. Otherwise I agree, that basic > compilation with GCC/LLVM must be done. Sparse still detects several issues and handles lock annotations, so it is useful. But if you disagree with that part, I still insist on Smatch (which is actively developed and works great) and Coccinelle (also actively developed). Best regards, Krzysztof
On Tue, Nov 28, 2023 at 6:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 27/11/2023 17:03, Andy Shevchenko wrote: > > On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: > >> On 27/11/2023 09:10, Krzysztof Kozlowski wrote: ... > >> Wait, this was not even unusual test, just standard compile, which means > >> you did not do basic tests on your end. You must build your new driver > >> with W=1, smatch, sparse and coccinelle before sending upstream. > > > > Well, sparse is lagging in development, for the last year it's at least two > > times it broke kernel builds because of being not ready for the new stuff used > > in the kernel. Do we have anybody to sync this? I don't think so, hence > > requiring this from developer is doubtful. Otherwise I agree, that basic > > compilation with GCC/LLVM must be done. > > Sparse still detects several issues and handles lock annotations, so it > is useful. Absolutely. > But if you disagree with that part, I still insist on Smatch > (which is actively developed and works great) and Coccinelle (also > actively developed). No disagreement on the usefulness of the sparse, my point is that its development is lagging and hence diminishes the value of the tool to the developer.
On 11/28/23 08:50, Krzysztof Kozlowski wrote: > On 27/11/2023 17:03, Andy Shevchenko wrote: >> On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: >>> On 27/11/2023 09:10, Krzysztof Kozlowski wrote: >> >> ... >> >>> Wait, this was not even unusual test, just standard compile, which means >>> you did not do basic tests on your end. You must build your new driver >>> with W=1, smatch, sparse and coccinelle before sending upstream. >> >> Well, sparse is lagging in development, for the last year it's at least two >> times it broke kernel builds because of being not ready for the new stuff used >> in the kernel. Do we have anybody to sync this? I don't think so, hence >> requiring this from developer is doubtful. Otherwise I agree, that basic >> compilation with GCC/LLVM must be done. > > Sparse still detects several issues and handles lock annotations, so it > is useful. But if you disagree with that part, I still insist on Smatch > (which is actively developed and works great) and Coccinelle (also > actively developed). > Quite frankly, for my part I would be more than happy if people would read and follow Documentation/hwmon/submitting-patches.rst. Most submitters don't bother. That doesn't even mention building with W=1 (the much more optimistic me who wrote that document several years ago thought that would be obvious), much less running any source code analysis tools . Feel free to submit a patch to strengthen the wording there. If you do that, it would have to be more explicit then "run smatch" or "run coccinelle" because hardly anyone would know how to do that. Until then, there isn't really anything to insist on. I even had submitters react angry when I asked them to follow the guidance in that document, so I won't insist on anything that isn't documented as requirement. Quite frankly, most of the time I'd probably fix up issues such as missing "static" myself because I am tired having to argue about it with submitters who don't care. Guenter
On Tue, 2023-11-28 at 10:03 -0800, Guenter Roeck wrote: > On 11/28/23 08:50, Krzysztof Kozlowski wrote: > > On 27/11/2023 17:03, Andy Shevchenko wrote: > > > On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: > > > > On 27/11/2023 09:10, Krzysztof Kozlowski wrote: > > > > > > ... > > > > > > > Wait, this was not even unusual test, just standard compile, which means > > > > you did not do basic tests on your end. You must build your new driver > > > > with W=1, smatch, sparse and coccinelle before sending upstream. > > > > > > Well, sparse is lagging in development, for the last year it's at least two > > > times it broke kernel builds because of being not ready for the new stuff used > > > in the kernel. Do we have anybody to sync this? I don't think so, hence > > > requiring this from developer is doubtful. Otherwise I agree, that basic > > > compilation with GCC/LLVM must be done. > > > > Sparse still detects several issues and handles lock annotations, so it > > is useful. But if you disagree with that part, I still insist on Smatch > > (which is actively developed and works great) and Coccinelle (also > > actively developed). > > > > Quite frankly, for my part I would be more than happy if people would read > and follow Documentation/hwmon/submitting-patches.rst. Most submitters don't > bother. That doesn't even mention building with W=1 (the much more optimistic > me who wrote that document several years ago thought that would be obvious), > much less running any source code analysis tools . Feel free to submit a patch > to strengthen the wording there. If you do that, it would have to be more explicit > then "run smatch" or "run coccinelle" because hardly anyone would know how > to do that. > IMO, submitting patches to linux is already not the most straightforward thing in the world. If we are now going to ask to run smatch, cocci, sparse and so on, we will scare even more developers from the community... I mean, the bots are also in place to help with these kind of more advanced analysis, right? > Until then, there isn't really anything to insist on. I even had submitters > react angry when I asked them to follow the guidance in that document, > so I won't insist on anything that isn't documented as requirement. > Quite frankly, most of the time I'd probably fix up issues such as missing > "static" myself because I am tired having to argue about it with submitters > who don't care. > For the record, I do care about the code I submit and missing 'static' is quite embarrassing, yes. The only reason why I still dind't send the v3 fixing that is because I'm giving more time to see if you can review some of the other changes. I'm pretty sure I'll be asked to change more things in v2 (as stated in the cover, there are still some shady things in the driver). - Nuno Sá
On 29/11/2023 09:35, Nuno Sá wrote: > On Tue, 2023-11-28 at 10:03 -0800, Guenter Roeck wrote: >> On 11/28/23 08:50, Krzysztof Kozlowski wrote: >>> On 27/11/2023 17:03, Andy Shevchenko wrote: >>>> On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: >>>>> On 27/11/2023 09:10, Krzysztof Kozlowski wrote: >>>> >>>> ... >>>> >>>>> Wait, this was not even unusual test, just standard compile, which means >>>>> you did not do basic tests on your end. You must build your new driver >>>>> with W=1, smatch, sparse and coccinelle before sending upstream. >>>> >>>> Well, sparse is lagging in development, for the last year it's at least two >>>> times it broke kernel builds because of being not ready for the new stuff used >>>> in the kernel. Do we have anybody to sync this? I don't think so, hence >>>> requiring this from developer is doubtful. Otherwise I agree, that basic >>>> compilation with GCC/LLVM must be done. >>> >>> Sparse still detects several issues and handles lock annotations, so it >>> is useful. But if you disagree with that part, I still insist on Smatch >>> (which is actively developed and works great) and Coccinelle (also >>> actively developed). >>> >> >> Quite frankly, for my part I would be more than happy if people would read >> and follow Documentation/hwmon/submitting-patches.rst. Most submitters don't >> bother. That doesn't even mention building with W=1 (the much more optimistic >> me who wrote that document several years ago thought that would be obvious), >> much less running any source code analysis tools . Feel free to submit a patch >> to strengthen the wording there. If you do that, it would have to be more explicit >> then "run smatch" or "run coccinelle" because hardly anyone would know how >> to do that. >> > > IMO, submitting patches to linux is already not the most straightforward thing in the True... > world. If we are now going to ask to run smatch, cocci, sparse and so on, we will > scare even more developers from the community... I mean, the bots are also in place This is not related to Linux at all. When you develop any C or C++ code, you run these tools. Upstream or downstream, does not matter. Why would you not use automated, free and easy tools to detect errors in your code? It's just a matter of professional approach to your code. > to help with these kind of more advanced analysis, right? They do not come for free (someone is paying for them even if they are for free to you) and they have delays in responses. > >> Until then, there isn't really anything to insist on. I even had submitters >> react angry when I asked them to follow the guidance in that document, >> so I won't insist on anything that isn't documented as requirement. >> Quite frankly, most of the time I'd probably fix up issues such as missing >> "static" myself because I am tired having to argue about it with submitters >> who don't care. >> > > For the record, I do care about the code I submit and missing 'static' is quite > embarrassing, yes. The only reason why I still dind't send the v3 fixing that is > because I'm giving more time to see if you can review some of the other changes. I'm > pretty sure I'll be asked to change more things in v2 (as stated in the cover, there > are still some shady things in the driver). Sure, mistakes happen. I did my share as well. I only brought the point that contributor should use automated tools available for free to them before using community resources. Community resources, including the most expensive one - review by maintainers, are scarce. Best regards, Krzysztof
On Wed, 2023-11-29 at 09:45 +0100, Krzysztof Kozlowski wrote: > On 29/11/2023 09:35, Nuno Sá wrote: > > On Tue, 2023-11-28 at 10:03 -0800, Guenter Roeck wrote: > > > On 11/28/23 08:50, Krzysztof Kozlowski wrote: > > > > On 27/11/2023 17:03, Andy Shevchenko wrote: > > > > > On Mon, Nov 27, 2023 at 09:12:14AM +0100, Krzysztof Kozlowski wrote: > > > > > > On 27/11/2023 09:10, Krzysztof Kozlowski wrote: > > > > > > > > > > ... > > > > > > > > > > > Wait, this was not even unusual test, just standard compile, which means > > > > > > you did not do basic tests on your end. You must build your new driver > > > > > > with W=1, smatch, sparse and coccinelle before sending upstream. > > > > > > > > > > Well, sparse is lagging in development, for the last year it's at least two > > > > > times it broke kernel builds because of being not ready for the new stuff > > > > > used > > > > > in the kernel. Do we have anybody to sync this? I don't think so, hence > > > > > requiring this from developer is doubtful. Otherwise I agree, that basic > > > > > compilation with GCC/LLVM must be done. > > > > > > > > Sparse still detects several issues and handles lock annotations, so it > > > > is useful. But if you disagree with that part, I still insist on Smatch > > > > (which is actively developed and works great) and Coccinelle (also > > > > actively developed). > > > > > > > > > > Quite frankly, for my part I would be more than happy if people would read > > > and follow Documentation/hwmon/submitting-patches.rst. Most submitters don't > > > bother. That doesn't even mention building with W=1 (the much more optimistic > > > me who wrote that document several years ago thought that would be obvious), > > > much less running any source code analysis tools . Feel free to submit a patch > > > to strengthen the wording there. If you do that, it would have to be more > > > explicit > > > then "run smatch" or "run coccinelle" because hardly anyone would know how > > > to do that. > > > > > > > IMO, submitting patches to linux is already not the most straightforward thing in > > the > > True... > > > world. If we are now going to ask to run smatch, cocci, sparse and so on, we will > > scare even more developers from the community... I mean, the bots are also in > > place > > This is not related to Linux at all. When you develop any C or C++ code, > you run these tools. Upstream or downstream, does not matter. Why would > you not use automated, free and easy tools to detect errors in your > code? It's just a matter of professional approach to your code. > That's true but still are too many things to remember for every single change/driver one sends upstream. Yeah, I might just wrap b4 in a script to run more advanced checks on my patches before 'send'. > > to help with these kind of more advanced analysis, right? > > They do not come for free (someone is paying for them even if they are > for free to you) and they have delays in responses. > Yeah, but actually thanks to you, I discovered I can have my private branches covered by lkp (and I got the PR merged already) and I do not mind having 1/2 day delay for sending patches. So maybe that will help me avoid these kind of mistakes. - Nuno Sá >
On Wed, Nov 29, 2023 at 9:45 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 29/11/2023 09:35, Nuno Sá wrote: > > world. If we are now going to ask to run smatch, cocci, sparse and so on, we will > > scare even more developers from the community... I mean, the bots are also in place > > This is not related to Linux at all. Smatch, main author Dan Carpenter Coccinelle, main author Julia Lawall Sparse, main author Linus Torvalds To be fair I think these tools has quite a lot to do with Linux, being developed for the kernel as the primary use case, by Linux kernel contributors. > When you develop any C or C++ code, > you run these tools. Upstream or downstream, does not matter. Why would > you not use automated, free and easy tools to detect errors in your > code? It's just a matter of professional approach to your code. This I agree with. We just happen to have some especially talented C developers who write some novel tooling. (I think developers should be able to rely on robots to run them though, we all seem to agree on that.) Yours, Linus Walleij
On 29/11/2023 15:10, Linus Walleij wrote: > On Wed, Nov 29, 2023 at 9:45 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 29/11/2023 09:35, Nuno Sá wrote: > >>> world. If we are now going to ask to run smatch, cocci, sparse and so on, we will >>> scare even more developers from the community... I mean, the bots are also in place >> >> This is not related to Linux at all. > > Smatch, main author Dan Carpenter By default Smatch checks regular C code and you need to enable kernel-specific tweaks. > Coccinelle, main author Julia Lawall True. > Sparse, main author Linus Torvalds Git, main author Linus Torvalds. :) Best regards, Krzysztof
On Wed, 2023-11-29 at 15:10 +0100, Linus Walleij wrote: > On Wed, Nov 29, 2023 at 9:45 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 29/11/2023 09:35, Nuno Sá wrote: > > > > world. If we are now going to ask to run smatch, cocci, sparse and so on, we > > > will > > > scare even more developers from the community... I mean, the bots are also in > > > place > > > > This is not related to Linux at all. > > Smatch, main author Dan Carpenter > Coccinelle, main author Julia Lawall > Sparse, main author Linus Torvalds > > To be fair I think these tools has quite a lot to do with Linux, being developed > for the kernel as the primary use case, by Linux kernel contributors. > > > When you develop any C or C++ code, > > you run these tools. Upstream or downstream, does not matter. Why would > > you not use automated, free and easy tools to detect errors in your > > code? It's just a matter of professional approach to your code. > > This I agree with. We just happen to have some especially > talented C developers who write some novel tooling. > > (I think developers should be able to rely on robots to run them though, > we all seem to agree on that.) > > Yours, > Linus Walleij Hi Linus, Since I have your attention, please give a look at the gpio stuff when you have the time for it. On the v1 cover [1], I have there my concerns. I'm really not sure if I'm providing the right interfaces for the device capabilities. [1]: https://lore.kernel.org/linux-hwmon/20231110151905.1659873-1-nuno.sa@analog.com/ Thanks! - Nuno Sá
On 11/29/23 00:35, Nuno Sá wrote: [ ... ] > > For the record, I do care about the code I submit and missing 'static' is quite > embarrassing, yes. The only reason why I still dind't send the v3 fixing that is > because I'm giving more time to see if you can review some of the other changes. I'm > pretty sure I'll be asked to change more things in v2 (as stated in the cover, there > are still some shady things in the driver). > NP, but you (and others) will have to accept that your patches end up at the end of my lowest-priority review queue if 0-day or checkpatch or builds with W=1 end up reporting problems. My time is limited, and I am extremely wary of having to keep telling people to run checkpatch or builds with W=1. Guenter
Hi Nuno, GPIO-related review as requested! Thanks for your patch! On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > +config SENSORS_LTC4282 > + tristate "Analog Devices LTC4282" > + depends on I2C > + select REGMAP_I2C select GPIOLIB potentially also select GPIO_REGMAP, see below. > +struct ltc4282_gpio { > + const char * const *funcs; > + u32 out_reg; > + u32 out_mask; > + u32 in_reg; > + u32 in_mask; > + bool active_high; > + u8 n_funcs; > +}; So pretty simple dedicated bits. > +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + > + /* we can only control this for GPIO_1 */ > + if (offset != LTC4282_GPIO_1) > + return 0; > + > + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, > + LTC4282_GPIO_1_CONFIG_MASK); > +} > + > +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset, > + int val) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > + > + guard(mutex)(&st->lock); > + /* > + * Explicitly setting the pin as output can only be done for GPIO_1. For > + * the other pins we just pull the line down or high-z. > + */ > + if (offset == LTC4282_GPIO_1) { > + int ret; > + > + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > + LTC4282_GPIO_1_CONFIG_MASK, > + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2)); > + if (ret) > + return ret; > + } > + > + /* > + * GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down > + * the line > + */ > + if (!gpio->active_high) > + val = !val; > + > + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > + field_prep(gpio->out_mask, val)); > +} > + > +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, > + int val) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > + > + if (!gpio->active_high) > + val = !val; > + > + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > + field_prep(gpio->out_mask, val)); > +} > + > +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > + int ret; > + u32 val; > + > + ret = regmap_read(st->map, gpio->in_reg, &val); > + if (ret) > + return ret; > + > + return !!(val & gpio->in_mask); > +} > + > +static int ltc4282_gpio_valid_mask(struct gpio_chip *chip, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + > + *valid_mask = st->valid_mask; > + return 0; > +} Some of this looks like it could use GPIO_REGMAP, look into other drivers using these helpers such as drivers/gpio/gpio-ds4520.c and see how small it becomes. It may or may not help you. But take a look. Other than that it looks fine. Yours, Linus Walleij
On Wed, 2023-11-29 at 15:49 +0100, Linus Walleij wrote: > Hi Nuno, > > GPIO-related review as requested! Thanks for your patch! > > On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay > <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > +config SENSORS_LTC4282 > > + tristate "Analog Devices LTC4282" > > + depends on I2C > > + select REGMAP_I2C > > select GPIOLIB > Hmm alright, the only reason why I didn't do this is because gpiochip is an optional feature for the driver. So I have an '!IS_ENABLED(CONFOG_GPIOLIB)' guard in the beginning of the function. But yeah, will just do this. Odds are that gpio is already enabled anyways. > potentially also > > select GPIO_REGMAP, see below. > > > +struct ltc4282_gpio { > > + const char * const *funcs; > > + u32 out_reg; > > + u32 out_mask; > > + u32 in_reg; > > + u32 in_mask; > > + bool active_high; > > + u8 n_funcs; > > +}; > > So pretty simple dedicated bits. > > > +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + > > + /* we can only control this for GPIO_1 */ > > + if (offset != LTC4282_GPIO_1) > > + return 0; > > + > > + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, > > + LTC4282_GPIO_1_CONFIG_MASK); > > +} > > + > > +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset, > > + int val) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > > + > > + guard(mutex)(&st->lock); > > + /* > > + * Explicitly setting the pin as output can only be done for GPIO_1. For > > + * the other pins we just pull the line down or high-z. > > + */ > > + if (offset == LTC4282_GPIO_1) { > > + int ret; > > + > > + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > > + LTC4282_GPIO_1_CONFIG_MASK, > > + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, > > 2)); > > + if (ret) > > + return ret; > > + } > > + > > + /* > > + * GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down > > + * the line > > + */ > > + if (!gpio->active_high) > > + val = !val; > > + > > + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > > + field_prep(gpio->out_mask, val)); > > +} > > + > > +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, > > + int val) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > > + > > + if (!gpio->active_high) > > + val = !val; > > + > > + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > > + field_prep(gpio->out_mask, val)); > > +} > > + > > +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; > > + int ret; > > + u32 val; > > + > > + ret = regmap_read(st->map, gpio->in_reg, &val); > > + if (ret) > > + return ret; > > + > > + return !!(val & gpio->in_mask); > > +} > > + > > +static int ltc4282_gpio_valid_mask(struct gpio_chip *chip, > > + unsigned long *valid_mask, > > + unsigned int ngpios) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + > > + *valid_mask = st->valid_mask; > > + return 0; > > +} > > Some of this looks like it could use GPIO_REGMAP, look into other > drivers using these helpers such as > drivers/gpio/gpio-ds4520.c and see how small it becomes. > > It may or may not help you. But take a look. > Ok, will look at it. > Other than that it looks fine. > Cool, I actually thought that having the direction + get/set stuff would be weird given the fact that we can only PULL_LOW or HIGH_Z the pins. Thanks! - Nuno Sá
On Wed, 2023-11-29 at 06:47 -0800, Guenter Roeck wrote: > On 11/29/23 00:35, Nuno Sá wrote: > [ ... ] > > > > > For the record, I do care about the code I submit and missing 'static' is quite > > embarrassing, yes. The only reason why I still dind't send the v3 fixing that is > > because I'm giving more time to see if you can review some of the other changes. > > I'm > > pretty sure I'll be asked to change more things in v2 (as stated in the cover, > > there > > are still some shady things in the driver). > > > > NP, but you (and others) will have to accept that your patches end up at the end > of my lowest-priority review queue if 0-day or checkpatch or builds with W=1 > end up reporting problems. My time is limited, and I am extremely wary of having > to keep telling people to run checkpatch or builds with W=1. > > Guenter > Hmm I see... So I might just send a v3 with the feedback I got so far plus getting 0- day fixed. That way, I might get out of the lowest-priority queue :) - Nuno Sá
On Wed, Nov 29, 2023 at 05:08:41PM +0100, Nuno Sá wrote: > On Wed, 2023-11-29 at 15:49 +0100, Linus Walleij wrote: > > On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay > > <devnull+nuno.sa.analog.com@kernel.org> wrote: ... > > Other than that it looks fine. > > Cool, I actually thought that having the direction + get/set stuff would be > weird given the fact that we can only PULL_LOW or HIGH_Z the pins. Oh, then it's probably should be a pin config involved.
On Wed, 2023-11-29 at 18:18 +0200, Andy Shevchenko wrote: > On Wed, Nov 29, 2023 at 05:08:41PM +0100, Nuno Sá wrote: > > On Wed, 2023-11-29 at 15:49 +0100, Linus Walleij wrote: > > > On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay > > > <devnull+nuno.sa.analog.com@kernel.org> wrote: > > ... > > > > Other than that it looks fine. > > > > Cool, I actually thought that having the direction + get/set stuff would be > > weird given the fact that we can only PULL_LOW or HIGH_Z the pins. > > Oh, then it's probably should be a pin config involved. > Well, that was what I stated in the cover [1]. Please give a look and see if what I'm saying in there makes any sense. [1]: https://lore.kernel.org/linux-hwmon/20231110151905.1659873-1-nuno.sa@analog.com/
On Wed, Nov 29, 2023 at 05:21:28PM +0100, Nuno Sá wrote: > On Wed, 2023-11-29 at 18:18 +0200, Andy Shevchenko wrote: > > On Wed, Nov 29, 2023 at 05:08:41PM +0100, Nuno Sá wrote: > > > On Wed, 2023-11-29 at 15:49 +0100, Linus Walleij wrote: > > > > On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote: ... > > > > Other than that it looks fine. > > > > > > Cool, I actually thought that having the direction + get/set stuff would be > > > weird given the fact that we can only PULL_LOW or HIGH_Z the pins. > > > > Oh, then it's probably should be a pin config involved. > > Well, that was what I stated in the cover [1]. Please give a look and see if what I'm > saying in there makes any sense. I could tell only WRT GPIO/pin control. And I think what you would benefit from is the pin configuration and "pin control as basic GPIO" (note, latter is not in the upstream yet, but was submitted for review, interesting idea nevertheless). > [1]: https://lore.kernel.org/linux-hwmon/20231110151905.1659873-1-nuno.sa@analog.com/
On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: > Cool, I actually thought that having the direction + get/set stuff would be weird > given the fact that we can only PULL_LOW or HIGH_Z the pins. There are several drivers in the kernel that implement .set_config(), it's existing and should be enabled if it has uses. As Andy points out: when the driver reaches a certain complexity, such as a huge table of muxable pins (that need to be configured to a certain muxing from device tree), and numerous complicated pin config options (also needing to be set up from device tree), it may be worth to implement a separate pin control driver that act as "backend" for the GPIO driver. I think a separate pin control driver would be overkill in this case, it's a PWM driver with some smallish GPIO portions AFAICT, but you get to decide. Yours, Linus Walleij
On Wed, 2023-11-29 at 21:55 +0100, Linus Walleij wrote: > On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > > Cool, I actually thought that having the direction + get/set stuff would be weird > > given the fact that we can only PULL_LOW or HIGH_Z the pins. > > There are several drivers in the kernel that implement .set_config(), > it's existing and should be enabled if it has uses. > Yeah, it might make sense to support it specially for the input case. AFAICT, if I use the .set_config() (but from a quick look I think we will need to add support for it in gpiolib for the high-z configuration), then I can't use the gpio_regmap stuff. As the driver stands I don't think I could do it anyways because setting gpio2-3 and alert requires to write 0 on the register rather than 1. But again, I'm still very suspicious about the whole thing. The datasheet states: "GPIO1-GPIO3 and ALERT all have comparators monitoring the voltage on these pins with a threshold of 1.28V even when the pins are configured as outputs." But we can't really set the direction for gpio2-3 and the alert pins (only getting the level and setting it as PULL_LOW or HIGH_Z. gpio1 is the only one where we can configure it as input or open drain ouput. Bah, I'll try to see if someone internally can shed some light on this. > As Andy points out: when the driver reaches a certain complexity, > such as a huge table of muxable pins (that need to be configured to > a certain muxing from device tree), and numerous complicated > pin config options (also needing to be set up from device tree), > it may be worth to implement a separate pin control driver that > act as "backend" for the GPIO driver. > > I think a separate pin control driver would be overkill in this case, > it's a PWM driver with some smallish GPIO portions AFAICT, > but you get to decide. > Agreed, the chip only supports 4 pins and it is an optional feature. The main usage for the chip iis to act as an hot swap controller (which maps into hwmon). - Nuno Sá
On Thu, Nov 30, 2023 at 11:20:32AM +0100, Nuno Sá wrote: > On Wed, 2023-11-29 at 21:55 +0100, Linus Walleij wrote: > > On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > Cool, I actually thought that having the direction + get/set stuff would be weird > > > given the fact that we can only PULL_LOW or HIGH_Z the pins. > > > > There are several drivers in the kernel that implement .set_config(), > > it's existing and should be enabled if it has uses. > > Yeah, it might make sense to support it specially for the input case. AFAICT, if I > use the .set_config() (but from a quick look I think we will need to add support for > it in gpiolib for the high-z configuration), then I can't use the gpio_regmap stuff. > As the driver stands I don't think I could do it anyways because setting gpio2-3 and > alert requires to write 0 on the register rather than 1. But again, I'm still very > suspicious about the whole thing. The datasheet states: > > "GPIO1-GPIO3 and ALERT all have comparators monitoring > the voltage on these pins with a threshold of 1.28V even when > the pins are configured as outputs." > > But we can't really set the direction for gpio2-3 and the alert pins (only getting > the level and setting it as PULL_LOW or HIGH_Z. gpio1 is the only one where we can > configure it as input or open drain ouput. Bah, I'll try to see if someone internally > can shed some light on this. I have better proposal. If these GPIOs are not needed for the main functionality of the hardware, can we just left it out for now and implement later if required?
On 11/30/23 05:36, Andy Shevchenko wrote: > On Thu, Nov 30, 2023 at 11:20:32AM +0100, Nuno Sá wrote: >> On Wed, 2023-11-29 at 21:55 +0100, Linus Walleij wrote: >>> On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: >>> >>>> Cool, I actually thought that having the direction + get/set stuff would be weird >>>> given the fact that we can only PULL_LOW or HIGH_Z the pins. >>> >>> There are several drivers in the kernel that implement .set_config(), >>> it's existing and should be enabled if it has uses. >> >> Yeah, it might make sense to support it specially for the input case. AFAICT, if I >> use the .set_config() (but from a quick look I think we will need to add support for >> it in gpiolib for the high-z configuration), then I can't use the gpio_regmap stuff. >> As the driver stands I don't think I could do it anyways because setting gpio2-3 and >> alert requires to write 0 on the register rather than 1. But again, I'm still very >> suspicious about the whole thing. The datasheet states: >> >> "GPIO1-GPIO3 and ALERT all have comparators monitoring >> the voltage on these pins with a threshold of 1.28V even when >> the pins are configured as outputs." >> >> But we can't really set the direction for gpio2-3 and the alert pins (only getting >> the level and setting it as PULL_LOW or HIGH_Z. gpio1 is the only one where we can >> configure it as input or open drain ouput. Bah, I'll try to see if someone internally >> can shed some light on this. > > I have better proposal. If these GPIOs are not needed for the main > functionality of the hardware, can we just left it out for now and implement > later if required? > I very much agree. Guenter
On Thu, 2023-11-30 at 15:36 +0200, Andy Shevchenko wrote: > On Thu, Nov 30, 2023 at 11:20:32AM +0100, Nuno Sá wrote: > > On Wed, 2023-11-29 at 21:55 +0100, Linus Walleij wrote: > > > On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > > > Cool, I actually thought that having the direction + get/set stuff would be > > > > weird > > > > given the fact that we can only PULL_LOW or HIGH_Z the pins. > > > > > > There are several drivers in the kernel that implement .set_config(), > > > it's existing and should be enabled if it has uses. > > > > Yeah, it might make sense to support it specially for the input case. AFAICT, if > > I > > use the .set_config() (but from a quick look I think we will need to add support > > for > > it in gpiolib for the high-z configuration), then I can't use the gpio_regmap > > stuff. > > As the driver stands I don't think I could do it anyways because setting gpio2-3 > > and > > alert requires to write 0 on the register rather than 1. But again, I'm still > > very > > suspicious about the whole thing. The datasheet states: > > > > "GPIO1-GPIO3 and ALERT all have comparators monitoring > > the voltage on these pins with a threshold of 1.28V even when > > the pins are configured as outputs." > > > > But we can't really set the direction for gpio2-3 and the alert pins (only > > getting > > the level and setting it as PULL_LOW or HIGH_Z. gpio1 is the only one where we > > can > > configure it as input or open drain ouput. Bah, I'll try to see if someone > > internally > > can shed some light on this. > > I have better proposal. If these GPIOs are not needed for the main > functionality of the hardware, can we just left it out for now and implement > later if required? > Well, I did spent some time on the gpio thing so I would like to have it in but yeah, no hard feelings if it does not go in. So, I actually talk with some hw guys and the pull_low is not really like a pull_low resistor. These pins are effectively an open drain. Which means, setting them as input means setting them in high-z (turning off the mosffet) - and I do have a bug in my code regarding this - Or if you want them as outputs you can set the level low (and it will always be low - just turn on the mosffet) or you can also set high-z which means it will be either low or high depending on your external circuitry. The point is, you can still have your pin acting as a normal gpo if you accommodate your circuitry for it (can also use these pins for things like buses). Also got me thinking if a gpi vs gpo devicetree property would make sense. But I would likely leave it more generic/relaxed for now (even though I think you would need to be creative and actually use more HW to have the possibility of using these pins as GPIs and GPOs at the same time). - Nuno Sá
On 11/30/23 07:20, Nuno Sá wrote: > On Thu, 2023-11-30 at 15:36 +0200, Andy Shevchenko wrote: >> On Thu, Nov 30, 2023 at 11:20:32AM +0100, Nuno Sá wrote: >>> On Wed, 2023-11-29 at 21:55 +0100, Linus Walleij wrote: >>>> On Wed, Nov 29, 2023 at 5:08 PM Nuno Sá <noname.nuno@gmail.com> wrote: >>>> >>>>> Cool, I actually thought that having the direction + get/set stuff would be >>>>> weird >>>>> given the fact that we can only PULL_LOW or HIGH_Z the pins. >>>> >>>> There are several drivers in the kernel that implement .set_config(), >>>> it's existing and should be enabled if it has uses. >>> >>> Yeah, it might make sense to support it specially for the input case. AFAICT, if >>> I >>> use the .set_config() (but from a quick look I think we will need to add support >>> for >>> it in gpiolib for the high-z configuration), then I can't use the gpio_regmap >>> stuff. >>> As the driver stands I don't think I could do it anyways because setting gpio2-3 >>> and >>> alert requires to write 0 on the register rather than 1. But again, I'm still >>> very >>> suspicious about the whole thing. The datasheet states: >>> >>> "GPIO1-GPIO3 and ALERT all have comparators monitoring >>> the voltage on these pins with a threshold of 1.28V even when >>> the pins are configured as outputs." >>> >>> But we can't really set the direction for gpio2-3 and the alert pins (only >>> getting >>> the level and setting it as PULL_LOW or HIGH_Z. gpio1 is the only one where we >>> can >>> configure it as input or open drain ouput. Bah, I'll try to see if someone >>> internally >>> can shed some light on this. >> >> I have better proposal. If these GPIOs are not needed for the main >> functionality of the hardware, can we just left it out for now and implement >> later if required? >> > > Well, I did spent some time on the gpio thing so I would like to have it in but yeah, > no hard feelings if it does not go in. > Problem with that is that it may end up not being used at all ... and thus start bit-rotting immediately. Guenter
On Thu, Nov 30, 2023 at 4:20 PM Nuno Sá <noname.nuno@gmail.com> wrote: > Well, I did spent some time on the gpio thing so I would like to have it in but yeah, > no hard feelings if it does not go in. I think it's a good idea to include it, especially if you can, in the commit message, illustrate how you test it with the libgpiod toolset. If you can test it all the way such that you have real hardware connected to real electronics where you can observe the values on these pins or read them: even better. GPIOs tend to get used, and then we are prepared. > So, I actually talk with some hw guys and the pull_low is not really like a pull_low > resistor. We usually call it pull down, so the PIN_CONFIG_BIAS_PULL_DOWN config property. This is vital to e.g. create a bit-banged I2C link, which is something I suspect could be useful on this hardware. >These pins are effectively an open drain. Which means, setting them as > input means setting them in high-z (turning off the mosffet) - and I do have a bug in > my code regarding this - The gpiolib framework assumes we can do open drain emulation by setting lines as input. It is used as fallback unless the hardware has an explicit open drain setting. > Or if you want them as outputs you can set the level low > (and it will always be low - just turn on the mosffet) or you can also set high-z > which means it will be either low or high depending on your external circuitry. The > point is, you can still have your pin acting as a normal gpo if you accommodate your > circuitry for it (can also use these pins for things like buses). Yeah that is just standard open drain behaviour, by the book. Also documented in https://docs.kernel.org/driver-api/gpio/driver.html under the heading "GPIO lines with open drain/source support". > Also got me thinking if a gpi vs gpo devicetree property would make sense. But I > would likely leave it more generic/relaxed for now (even though I think you would > need to be creative and actually use more HW to have the possibility of using these > pins as GPIs and GPOs at the same time). We don't define that in the device tree currently, we just make the driver not support output on input-only pins and vice versa, by returning error codes on the .set_direction() callbacks. Yours, Linus Walleij
On Thu, 2023-11-30 at 21:15 +0100, Linus Walleij wrote: > On Thu, Nov 30, 2023 at 4:20 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > > Well, I did spent some time on the gpio thing so I would like to have it in but > > yeah, > > no hard feelings if it does not go in. > > I think it's a good idea to include it, especially if you can, in the > commit message, > illustrate how you test it with the libgpiod toolset. If you can test it all the > way such that you have real hardware connected to real electronics where > you can observe the values on these pins or read them: even better. > > GPIOs tend to get used, and then we are prepared. > Yeah, I would also like to have this in but since I'm adding the driver to hwmon, I'll leave the final word to Guenter. I did not used libgpiod but I did tested it with gpio-sysfs. Well, I could effectively see the pull down behaviour but since my eval board has no pull-ups I could not drive the line high. > > So, I actually talk with some hw guys and the pull_low is not really like a > > pull_low > > resistor. > > We usually call it pull down, so the PIN_CONFIG_BIAS_PULL_DOWN > config property. > > This is vital to e.g. create a bit-banged I2C link, which is something I > suspect could be useful on this hardware. > > > These pins are effectively an open drain. Which means, setting them as > > input means setting them in high-z (turning off the mosffet) - and I do have a > > bug in > > my code regarding this - > > The gpiolib framework assumes we can do open drain emulation by > setting lines as input. It is used as fallback unless the hardware has > an explicit open drain setting. Yeah, I did look at that after you pointed that out. There's just something I'm still not getting. This HW has no explicit open drain setting because open drain is all that it is. So, I guess we could just specify the flag in devicetree so gpiolib could use the emulation but I wonder how would we have things in case we have the HW setup to drive the pin high (so having this as GPOs)? > > > Or if you want them as outputs you can set the level low > > (and it will always be low - just turn on the mosffet) or you can also set high-z > > which means it will be either low or high depending on your external circuitry. > > The > > point is, you can still have your pin acting as a normal gpo if you accommodate > > your > > circuitry for it (can also use these pins for things like buses). > > Yeah that is just standard open drain behaviour, by the book. > Also documented in > https://docs.kernel.org/driver-api/gpio/driver.html > under the heading "GPIO lines with open drain/source support". > > > Also got me thinking if a gpi vs gpo devicetree property would make sense. But I > > would likely leave it more generic/relaxed for now (even though I think you would > > need to be creative and actually use more HW to have the possibility of using > > these > > pins as GPIs and GPOs at the same time). > > We don't define that in the device tree currently, we just make the driver > not support output on input-only pins and vice versa, by returning error > codes on the .set_direction() callbacks. > I see, but in this case, the pins could be outputs depending on the HW setup but there's no way for us to know that in the driver. And given the fact that (I think) it's highly unlikely for pins like this to ever be GPIs and GPOs at the same time, I brought the devicetree property to define input and output only. So, roughly, what I have in mind now for the chip is; .set_config() -> with PULL_DOWN and HIGH_IMPEDANCE support .direction_input() -> This is important for gpio1 where we do have an hw setting to set the direction. On the other pins I was thinking in just forcing high-z. Or maybe can I just rely on gpio_set_bias()? .direction_ouput() -> Would only matter for gpio1 .get/set_value() -> And in this case we just assume that high value might or might not be possible (depending on the hw setup). Note that reading the pin state is always possible. This means that I assume we can have both directions but that is not really case and one needs to know what it is doing :). Or in cases like this, we just ignore the possibility of having GPO's and we let gpiolib do the emulation? Sounds reasonable or not really how I should handle this open-drain only pins? Thanks for the help! - Nuno Sá
On Fri, Dec 1, 2023 at 1:34 PM Nuno Sá <noname.nuno@gmail.com> wrote: > On Thu, 2023-11-30 at 21:15 +0100, Linus Walleij wrote: > I did not used libgpiod but I did tested it with gpio-sysfs. Well, I could > effectively see the pull down behaviour but since my eval board has no pull-ups I > could not drive the line high. libgpiod has the upside of offering you to set the pull down and open drain behaviour from userspace. > > The gpiolib framework assumes we can do open drain emulation by > > setting lines as input. It is used as fallback unless the hardware has > > an explicit open drain setting. > > Yeah, I did look at that after you pointed that out. There's just something I'm still > not getting. This HW has no explicit open drain setting because open drain is all > that it is. So, I guess we could just specify the flag in devicetree so gpiolib could > use the emulation > but I wonder how would we have things in case we have the HW setup > to drive the pin high (so having this as GPOs)? If another device tree node uses: foo-gpios = <&gpio0 5 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; The result will be that gpiolib will emulate open drain. From userspace libgpiod can do the same request. > > > Also got me thinking if a gpi vs gpo devicetree property would make sense. But I > > > would likely leave it more generic/relaxed for now (even though I think you would > > > need to be creative and actually use more HW to have the possibility of using > > > these > > > pins as GPIs and GPOs at the same time). > > > > We don't define that in the device tree currently, we just make the driver > > not support output on input-only pins and vice versa, by returning error > > codes on the .set_direction() callbacks. > > I see, but in this case, the pins could be outputs depending on the HW setup but > there's no way for us to know that in the driver. We just specify the line in the device tree, and we just use it as intended in the driver, if it is present, whether that is as input or output. We do not try to over-protect users from misusing GPIO lines that have just one possible (electronic defined) mode. It would be over-engineering IMO. > And given the fact that (I think) > it's highly unlikely for pins like this to ever be GPIs and GPOs at the same time, I > brought the devicetree property to define input and output only. So, roughly, what I > have in mind now for the chip is; > > .set_config() -> with PULL_DOWN and HIGH_IMPEDANCE support > .direction_input() -> This is important for gpio1 where we do have an hw setting to > set the direction. On the other pins I was thinking in just forcing high-z. Or maybe > can I just rely on gpio_set_bias()? No just write some default set-up into the registers, that's fine. Or leave the power-on defaults. > .direction_ouput() -> Would only matter for gpio1 The just return an error code for any other GPIO where this is called. > .get/set_value() -> And in this case we just assume that high value might or might > not be possible (depending on the hw setup). Note that reading the pin state is > always possible. If a pins .direction_output() fails, .set_value() will not be called on it either. > This means that I assume we can have both directions but that is not really case and > one needs to know what it is doing :). Or in cases like this, we just ignore the > possibility of having GPO's and we let gpiolib do the emulation? > > Sounds reasonable or not really how I should handle this open-drain only pins? Open drain-only pins would be pins that can be set to electric LOW (grounded) or High-Z. Is this what we have? That's indeed a bit of an oddity... If you implement .set_config and handle PIN_CONFIG_DRIVE_OPEN_DRAIN for these lines then I think gpiolib will do the right thing for you. Yours, Linus Walleij
On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > On Fri, Dec 1, 2023 at 1:34 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Thu, 2023-11-30 at 21:15 +0100, Linus Walleij wrote: > > > I did not used libgpiod but I did tested it with gpio-sysfs. Well, I could > > effectively see the pull down behaviour but since my eval board has no pull-ups I > > could not drive the line high. > > libgpiod has the upside of offering you to set the pull down and open > drain behaviour from userspace. > Yeah, I can also just come up with a minimal test driver and devicetree. > > > The gpiolib framework assumes we can do open drain emulation by > > > setting lines as input. It is used as fallback unless the hardware has > > > an explicit open drain setting. > > > > Yeah, I did look at that after you pointed that out. There's just something I'm > > still > > not getting. This HW has no explicit open drain setting because open drain is all > > that it is. So, I guess we could just specify the flag in devicetree so gpiolib > > could > > use the emulation > > but I wonder how would we have things in case we have the HW setup > > to drive the pin high (so having this as GPOs)? > > If another device tree node uses: > > foo-gpios = <&gpio0 5 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > > The result will be that gpiolib will emulate open drain. > > From userspace libgpiod can do the same request. > > > > > Also got me thinking if a gpi vs gpo devicetree property would make sense. > > > > But I > > > > would likely leave it more generic/relaxed for now (even though I think you > > > > would > > > > need to be creative and actually use more HW to have the possibility of using > > > > these > > > > pins as GPIs and GPOs at the same time). > > > > > > We don't define that in the device tree currently, we just make the driver > > > not support output on input-only pins and vice versa, by returning error > > > codes on the .set_direction() callbacks. > > > > I see, but in this case, the pins could be outputs depending on the HW setup but > > there's no way for us to know that in the driver. > > We just specify the line in the device tree, and we just use it as > intended in the > driver, if it is present, whether that is as input or output. > > We do not try to over-protect users from misusing GPIO lines that have just > one possible (electronic defined) mode. It would be over-engineering IMO. > Fair enough... > > And given the fact that (I think) > > it's highly unlikely for pins like this to ever be GPIs and GPOs at the same > > time, I > > brought the devicetree property to define input and output only. So, roughly, > > what I > > have in mind now for the chip is; > > > > .set_config() -> with PULL_DOWN and HIGH_IMPEDANCE support > > .direction_input() -> This is important for gpio1 where we do have an hw setting > > to > > set the direction. On the other pins I was thinking in just forcing high-z. Or > > maybe > > can I just rely on gpio_set_bias()? > > No just write some default set-up into the registers, that's fine. > Or leave the power-on defaults. > > > .direction_ouput() -> Would only matter for gpio1 > > The just return an error code for any other GPIO where this is called. > > > .get/set_value() -> And in this case we just assume that high value might or > > might > > not be possible (depending on the hw setup). Note that reading the pin state is > > always possible. > > If a pins .direction_output() fails, .set_value() will not be called > on it either. This is where I lost you :(. So, I'm might be overcomplicating things but... Again, the case where someone wired up HW so that we can actually use the pin to drive the line high (having an external pull up). In that case, If I return error, then I won't be able to effectively set the line high (as you said, set_value will not be called on it either). Now, I do understand that if we have the line flagged as GPIO_OPEN_DRAIN, then gpiolib will switch the line to input which means we will set the line in high-z which means that if we have a pull up, then the line will be high. I mean, it works but it would be strange if someone wants to have the line as output high and after trying to set the it high, it sees the pin moving to input. But if this is how it should be, fine by me. I do understand this is the definition of open drain so I guess someone should know what to expect when operating with pins like this. > > > This means that I assume we can have both directions but that is not really case > > and > > one needs to know what it is doing :). Or in cases like this, we just ignore the > > possibility of having GPO's and we let gpiolib do the emulation? > > > > Sounds reasonable or not really how I should handle this open-drain only pins? > > Open drain-only pins would be pins that can be set to electric LOW (grounded) > or High-Z. Is this what we have? > Yes, that is the only thing we have. Meaning that there is no hw setting to set the pins to open drain. Open drain is what they are. That is why I'm not seeing the point in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. Anyways, I'll try to have something cooked next week. I'll be slow since the winter (not even there yet) in Germany already got me! - Nuno Sá
On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: ... > Yes, that is the only thing we have. Meaning that there is no hw setting to set the > pins to open drain. Open drain is what they are. That is why I'm not seeing the point > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. At least you have to implement error for PUSH_PULL mode and other modes, so from the (core) software point of view the user should be able to ask for anything and get an answer from the certain driver that "hey, i do support OD", or "hey, push-pull can't be supported with this hw".
On 12/1/23 07:47, Andy Shevchenko wrote: > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: >> On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > > ... > >> Yes, that is the only thing we have. Meaning that there is no hw setting to set the >> pins to open drain. Open drain is what they are. That is why I'm not seeing the point >> in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > At least you have to implement error for PUSH_PULL mode and other modes, > so from the (core) software point of view the user should be able to ask for > anything and get an answer from the certain driver that "hey, i do support OD", > or "hey, push-pull can't be supported with this hw". > It seems to me that this is heading towards a mfd driver. I don't feel comfortable with all that gpio specific code in the hwmon subsystem. Maybe I should request that all hwmon chips with gpio support must be implemented as mfd drivers. I'll have to think about that. Guenter
On Fri, 2023-12-01 at 17:47 +0200, Andy Shevchenko wrote: > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > > ... > > > Yes, that is the only thing we have. Meaning that there is no hw setting to set > > the > > pins to open drain. Open drain is what they are. That is why I'm not seeing the > > point > > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > At least you have to implement error for PUSH_PULL mode and other modes, > so from the (core) software point of view the user should be able to ask for > anything and get an answer from the certain driver that "hey, i do support OD", > or "hey, push-pull can't be supported with this hw". > Yeah, that makes total sense. But I guess that the 'default' should already -EOPNOTSUPP right? - Nuno Sá
On Fri, Dec 01, 2023 at 05:19:33PM +0100, Nuno Sá wrote: > On Fri, 2023-12-01 at 17:47 +0200, Andy Shevchenko wrote: > > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > > > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: ... > > > Yes, that is the only thing we have. Meaning that there is no hw setting to set > > > the > > > pins to open drain. Open drain is what they are. That is why I'm not seeing the > > > point > > > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > > > At least you have to implement error for PUSH_PULL mode and other modes, > > so from the (core) software point of view the user should be able to ask for > > anything and get an answer from the certain driver that "hey, i do support OD", > > or "hey, push-pull can't be supported with this hw". > > Yeah, that makes total sense. But I guess that the 'default' should already > -EOPNOTSUPP right? Without OP part (as internal Linux error code), yes.
On Fri, Dec 01, 2023 at 08:04:12AM -0800, Guenter Roeck wrote: > On 12/1/23 07:47, Andy Shevchenko wrote: > > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > > > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: ... > > > Yes, that is the only thing we have. Meaning that there is no hw setting to set the > > > pins to open drain. Open drain is what they are. That is why I'm not seeing the point > > > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > > > At least you have to implement error for PUSH_PULL mode and other modes, > > so from the (core) software point of view the user should be able to ask for > > anything and get an answer from the certain driver that "hey, i do support OD", > > or "hey, push-pull can't be supported with this hw". > > > > It seems to me that this is heading towards a mfd driver. I don't feel comfortable > with all that gpio specific code in the hwmon subsystem. > > Maybe I should request that all hwmon chips with gpio support must be implemented > as mfd drivers. I'll have to think about that. Or auxiliary bus?
On Fri, 2023-12-01 at 08:04 -0800, Guenter Roeck wrote: > On 12/1/23 07:47, Andy Shevchenko wrote: > > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > > > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > > > > ... > > > > > Yes, that is the only thing we have. Meaning that there is no hw setting to set > > > the > > > pins to open drain. Open drain is what they are. That is why I'm not seeing the > > > point > > > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > > > At least you have to implement error for PUSH_PULL mode and other modes, > > so from the (core) software point of view the user should be able to ask for > > anything and get an answer from the certain driver that "hey, i do support OD", > > or "hey, push-pull can't be supported with this hw". > > > > It seems to me that this is heading towards a mfd driver. I don't feel comfortable > with all that gpio specific code in the hwmon subsystem. > > Maybe I should request that all hwmon chips with gpio support must be implemented > as mfd drivers. I'll have to think about that. > > Guenter > Hopefully you don't ask that already for this driver... I mean, I guess It could be done but having this is as mfd will make it more difficult to handle the pins. Because if we look at the bindings we can see that these pins might only be relevant to the hwmon driver and not even be used as gpios. But being an mfd, I don't think there's any requirement like one of the drivers can only exist if the other is also there. So making the gpio driver depend on the hwmon would be weird. I might be overcomplicating but things can get messy if we want to make it right in the gpio driver: * If the hwmon is not reachable then we could just assume we can use the 4 pins as gpios freely. * If the hwmon is reachable then we need some kind of syncup to make sure we dont use a pin as gpio if the hwmon driver wants to use it. Things can get even more shady if we think one driver being a module and the other being built-in. Or maybe we can also just have an array property in the gpio driver to explicitly set the pins we want as gpios. Then if the user messes up and we have a config overlaps one of the driver fails probing... Or maybe Andy's auxiliary bus suggestion might be more suitable (not really familiar with it). - Nuno Sá
On 12/1/23 08:24, Andy Shevchenko wrote: > On Fri, Dec 01, 2023 at 08:04:12AM -0800, Guenter Roeck wrote: >> On 12/1/23 07:47, Andy Shevchenko wrote: >>> On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: >>>> On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > > ... > >>>> Yes, that is the only thing we have. Meaning that there is no hw setting to set the >>>> pins to open drain. Open drain is what they are. That is why I'm not seeing the point >>>> in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. >>> >>> At least you have to implement error for PUSH_PULL mode and other modes, >>> so from the (core) software point of view the user should be able to ask for >>> anything and get an answer from the certain driver that "hey, i do support OD", >>> or "hey, push-pull can't be supported with this hw". >>> >> >> It seems to me that this is heading towards a mfd driver. I don't feel comfortable >> with all that gpio specific code in the hwmon subsystem. >> >> Maybe I should request that all hwmon chips with gpio support must be implemented >> as mfd drivers. I'll have to think about that. > > Or auxiliary bus? > I don't know. All I can see it that it is getting way too complicated for a hwmon driver with embedded gpio support. Guenter
On 12/1/23 08:29, Nuno Sá wrote: > On Fri, 2023-12-01 at 08:04 -0800, Guenter Roeck wrote: >> On 12/1/23 07:47, Andy Shevchenko wrote: >>> On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: >>>> On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: >>> >>> ... >>> >>>> Yes, that is the only thing we have. Meaning that there is no hw setting to set >>>> the >>>> pins to open drain. Open drain is what they are. That is why I'm not seeing the >>>> point >>>> in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. >>> >>> At least you have to implement error for PUSH_PULL mode and other modes, >>> so from the (core) software point of view the user should be able to ask for >>> anything and get an answer from the certain driver that "hey, i do support OD", >>> or "hey, push-pull can't be supported with this hw". >>> >> >> It seems to me that this is heading towards a mfd driver. I don't feel comfortable >> with all that gpio specific code in the hwmon subsystem. >> >> Maybe I should request that all hwmon chips with gpio support must be implemented >> as mfd drivers. I'll have to think about that. >> >> Guenter >> > > Hopefully you don't ask that already for this driver... > Yes, I am, because the gpio part is getting way to complicated for embedding it into a hwmon driver. Guenter
On Fri, 2023-12-01 at 08:46 -0800, Guenter Roeck wrote: > On 12/1/23 08:29, Nuno Sá wrote: > > On Fri, 2023-12-01 at 08:04 -0800, Guenter Roeck wrote: > > > On 12/1/23 07:47, Andy Shevchenko wrote: > > > > On Fri, Dec 01, 2023 at 04:24:35PM +0100, Nuno Sá wrote: > > > > > On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote: > > > > > > > > ... > > > > > > > > > Yes, that is the only thing we have. Meaning that there is no hw setting to > > > > > set > > > > > the > > > > > pins to open drain. Open drain is what they are. That is why I'm not seeing > > > > > the > > > > > point > > > > > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. > > > > > > > > At least you have to implement error for PUSH_PULL mode and other modes, > > > > so from the (core) software point of view the user should be able to ask for > > > > anything and get an answer from the certain driver that "hey, i do support > > > > OD", > > > > or "hey, push-pull can't be supported with this hw". > > > > > > > > > > It seems to me that this is heading towards a mfd driver. I don't feel > > > comfortable > > > with all that gpio specific code in the hwmon subsystem. > > > > > > Maybe I should request that all hwmon chips with gpio support must be > > > implemented > > > as mfd drivers. I'll have to think about that. > > > > > > Guenter > > > > > > > Hopefully you don't ask that already for this driver... > > > > Yes, I am, because the gpio part is getting way to complicated for embedding it > into a hwmon driver. > Well, fair enough... I will then drop it for now. The priority is the hwmon part as that was the request from a customer. I'll only leave the pin functions that might be relevant from a monitoring point of view. Hopefully, I'll get into the gpio stuff later on. From a brief look, the auxiliary bus might feet and easier than mfd. - Nuno Sá
On Fri, Dec 1, 2023 at 4:24 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > If a pins .direction_output() fails, .set_value() will not be called > > on it either. > > This is where I lost you :( devm_gpiod_get() (and similar interfaces) will set up the default mode for the line, as input or output (with value, calling .direction_output) so most likely it will fail already there, and the driver will not probe or userspace client will fail. > So, I'm might be overcomplicating things but... Again, > the case where someone wired up HW so that we can actually use the pin to drive the > line high (having an external pull up). In that case, If I return error, then I won't > be able to effectively set the line high (as you said, set_value will not be called > on it either). > > Now, I do understand that if we have the line flagged as GPIO_OPEN_DRAIN, then > gpiolib will switch the line to input which means we will set the line in high-z > which means that if we have a pull up, then the line will be high. I mean, it works > but it would be strange if someone wants to have the line as output high and after > trying to set the it high, it sees the pin moving to input. But if this is how it > should be, fine by me. What do you mean by "sees the pin moving to input". If you mean electrically then yes, it goes to high-Z. If you mean logically, as seen by software and GPIO and debugfs, not really. I think a good exercise to see how it works is to just walk through the code in drivers/gpio/gpiolib.c for e.g. gpiod_set_value() gpiod_set_value_nocheck() gpio_set_open_drain_value_commit() >direction_input() is indeed called, but that is just a way of using the hardware, logically, inside gpiolib, the line is handled as high. > Yes, that is the only thing we have. Meaning that there is no hw setting to set the > pins to open drain. Open drain is what they are. That is why I'm not seeing the point > in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented. For satisfying the logic. We have several cases where callbacks are just returning a 0 error code for such corner cases, and as Andy points out push-pull requests should return an error. Yours, Linus Walleij
On Sat, Dec 2, 2023 at 10:42 AM Nuno Sá <noname.nuno@gmail.com> wrote:
Hopefully, I'll get into the gpio stuff later on. From a brief look,
the auxiliary
> bus might feet and easier than mfd.
You can also just spawn a random platform_device from the HWMON driver
and let that probe a driver down in drivers/gpio/*.
static struct platform_device my_gpio_device = {
.name = "my-gpio",
.id = -1,
};
my_gpio_device.dev.platform_data = ... ;
my_gpio_device.dev.parent = dev;
return platform_device_register(&my_gpio_device);
You can then pass any accessors as platform data. This in a way is what
the MFD or aux buses do just more organized.
Yours
Linus Walleij
On Mon, Dec 4, 2023 at 12:09 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sat, Dec 2, 2023 at 10:42 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > Hopefully, I'll get into the gpio stuff later on. From a brief look, > the auxiliary > > bus might feet and easier than mfd. > > You can also just spawn a random platform_device from the HWMON driver > and let that probe a driver down in drivers/gpio/*. > Please don't. A "random platform_device" even reads like a bad idea. Conceptually the GPIO part is not a child but one of the modules. It should be an MFD device IMO. Bart > static struct platform_device my_gpio_device = { > .name = "my-gpio", > .id = -1, > }; > > my_gpio_device.dev.platform_data = ... ; > my_gpio_device.dev.parent = dev; > return platform_device_register(&my_gpio_device); > > You can then pass any accessors as platform data. This in a way is what > the MFD or aux buses do just more organized. > > Yours > Linus Walleij
On Mon, 2023-12-04 at 00:03 +0100, Linus Walleij wrote: > On Fri, Dec 1, 2023 at 4:24 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > > > If a pins .direction_output() fails, .set_value() will not be called > > > on it either. > > > > This is where I lost you :( > > devm_gpiod_get() (and similar interfaces) will set up the default mode for > the line, as input or output (with value, calling .direction_output) so most > likely it will fail already there, and the driver will not probe or > userspace client > will fail. > > > So, I'm might be overcomplicating things but... Again, > > the case where someone wired up HW so that we can actually use the pin to drive > > the > > line high (having an external pull up). In that case, If I return error, then I > > won't > > be able to effectively set the line high (as you said, set_value will not be > > called > > on it either). > > > > Now, I do understand that if we have the line flagged as GPIO_OPEN_DRAIN, then > > gpiolib will switch the line to input which means we will set the line in high-z > > which means that if we have a pull up, then the line will be high. I mean, it > > works > > but it would be strange if someone wants to have the line as output high and > > after > > trying to set the it high, it sees the pin moving to input. But if this is how it > > should be, fine by me. > > What do you mean by "sees the pin moving to input". > > If you mean electrically then yes, it goes to high-Z. > Ohh, I know where my failure was!! I was reading gpiod_direction_output(desc, 1) and following it only till gpiod_direction_input(desc). I was completely missing the 'set_output_flag' jump... All understood now :) > If you mean logically, as seen by software and GPIO and debugfs, not > really. > > I think a good exercise to see how it works is to just walk through the > code in drivers/gpio/gpiolib.c for e.g. > gpiod_set_value() > gpiod_set_value_nocheck() > gpio_set_open_drain_value_commit() > Hmm, by looking into those, it made me think that I should not even need to implement the .set() callback... > Thanks for all your help! - Nuno Sá
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 095c36f5e8a1..b3961144b44a 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -128,6 +128,7 @@ Hardware Monitoring Kernel Drivers ltc4245 ltc4260 ltc4261 + ltc4282 max127 max15301 max16064 diff --git a/Documentation/hwmon/ltc4282.rst b/Documentation/hwmon/ltc4282.rst new file mode 100644 index 000000000000..0a6910703ba7 --- /dev/null +++ b/Documentation/hwmon/ltc4282.rst @@ -0,0 +1,108 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +Kernel drivers ltc4282 +========================================== + +Supported chips: + + * Analog Devices LTC4282 + + Prefix: 'ltc4282' + + Addresses scanned: - I2C 0x40 - 0x5A (7-bit) + Addresses scanned: - I2C 0x80 - 0xB4 with a step of 2 (8-bit) + + Datasheet: + + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf + +Author: Nuno Sá <nuno.sa@analog.com> + +Description +___________ + +The LTC4282 hot swap controller allows a board to be safely inserted and removed +from a live backplane. Using one or more external N-channel pass transistors, +board supply voltage and inrush current are ramped up at an adjustable rate. An +I2C interface and onboard ADC allows for monitoring of board current, voltage, +power, energy and fault status. The device features analog foldback current +limiting and supply monitoring for applications from 2.9V to 33V. Dual 12V gate +drive allows high power applications to either share safe operating area across +parallel MOSFETs or support a 2-stage start-up that first charges the load +capacitance followed by enabling a low on-resistance path to the load. The +LTC4282 is well suited to high power applications because the precise monitoring +capability and accurate current limiting reduce the extremes in which both loads +and power supplies must safely operate. Non-volatile configuration allows for +flexibility in the autonomous generation of alerts and response to faults. + +Sysfs entries +_____________ + +The following attributes are supported. Limits are read-write and all the other +attributes are read-only. Note that in0 and in1 are mutually exclusive. Enabling +one disables the other and disabling one enables the other. + +======================= ========================================== +in0_input Output voltage (mV). +in0_min Undervoltage threshold +in0_max Overvoltage threshold +in0_lowest Lowest measured voltage +in0_highest Highest measured voltage +in2_reset_history Write 1 to reset in0 history +in0_min_alarm Undervoltage alarm +in0_max_alarm Overvoltage alarm +in0_enable Enable/Disable in0 monitoring +in0_label Channel label (VSOURCE) + +in1_input Input voltage (mV). +in1_min Undervoltage threshold +in1_max Overvoltage threshold +in1_lowest Lowest measured voltage +in1_highest Highest measured voltage +in2_reset_history Write 1 to reset in1 history +in1_min_alarm Undervoltage alarm +in1_max_alarm Overvoltage alarm +in1_lcrit_alarm Critical Undervoltage alarm +in1_crit_alarm Critical Overvoltage alarm +in0_enable Enable/Disable in1 monitoring +in1_label Channel label (VDD) + +in2_input GPIO voltage (mV) +in2_min Undervoltage threshold +in2_max Overvoltage threshold +in2_lowest Lowest measured voltage +in2_highest Highest measured voltage +in2_reset_history Write 1 to reset in2 history +in2_min_alarm Undervoltage alarm +in2_max_alarm Overvoltage alarm +in2_label Channel label (VGPIO) + +curr1_input Sense current (mA) +curr1_min Undercurrent threshold +curr1_max Overcurrent threshold +curr1_lowest Lowest measured current +curr1_highest Highest measured current +in2_reset_history Write 1 to reset curr1 history +curr1_min_alarm Undercurrent alarm +curr1_max_alarm Overcurrent alarm +curr1_crit_alarm Critical Overcurrent alarm +curr1_label Channel label (ISENSE) + +power1_input Power (in uW) +power1_min Low power threshold +power1_max High power threshold +power1_input_lowest Historical minimum power use +power1_input_highest Historical maximum power use +in2_reset_history Write 1 to reset power1 history +power1_min_alarm Low power alarm +power1_max_alarm High power alarm +power1_good Power considered good +power1_label Channel label (Power) + +energy1_input Measured energy over time (in microJoule) +energy1_enable Enable/Disable Energy accumulation + +fet_short_fault FET short alarm +fet_bad_fault FET bad alarm +fault_logs_reset Clears all the Logged Faults +======================= ========================================== diff --git a/MAINTAINERS b/MAINTAINERS index 4a2b84ea1274..4cad1eba99eb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12634,6 +12634,8 @@ M: Nuno Sa <nuno.sa@analog.com> L: linux-hwmon@vger.kernel.org S: Supported F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml +F: Documentation/hwmon/ltc4282.rst +F: drivers/hwmon/ltc4282.c LTC4306 I2C MULTIPLEXER DRIVER M: Michael Hennerich <michael.hennerich@analog.com> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index cf27523eed5a..2f1a0cf2e1e4 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1027,6 +1027,17 @@ config SENSORS_LTC4261 This driver can also be built as a module. If so, the module will be called ltc4261. +config SENSORS_LTC4282 + tristate "Analog Devices LTC4282" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for Analog Devices LTC4282 + High Current Hot Swap Controller I2C interface. + + This driver can also be built as a module. If so, the module will + be called ltc4282. + config SENSORS_LTQ_CPUTEMP bool "Lantiq cpu temperature sensor driver" depends on SOC_XWAY diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e84bd9685b5c..cbf22d1d736f 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_LTC4222) += ltc4222.o obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o obj-$(CONFIG_SENSORS_LTC4260) += ltc4260.o obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o +obj-$(CONFIG_SENSORS_LTC4282) += ltc4282.o obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o obj-$(CONFIG_SENSORS_MAX1111) += max1111.o obj-$(CONFIG_SENSORS_MAX127) += max127.o diff --git a/drivers/hwmon/ltc4282.c b/drivers/hwmon/ltc4282.c new file mode 100644 index 000000000000..dd391a7e8f70 --- /dev/null +++ b/drivers/hwmon/ltc4282.c @@ -0,0 +1,1891 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C + * + * Copyright 2023 Analog Devices Inc. + */ +#include <linux/bitfield.h> +#include <linux/cleanup.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/debugfs.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/driver.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/i2c.h> +#include <linux/math.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/property.h> +#include <linux/string.h> +#include <linux/units.h> +#include <linux/util_macros.h> + +#define LTC4282_CTRL_LSB 0x00 + #define LTC4282_CTRL_OV_RETRY_MASK BIT(0) + #define LTC4282_CTRL_UV_RETRY_MASK BIT(1) + #define LTC4282_CTRL_OC_RETRY_MASK BIT(2) + #define LTC4282_CTRL_ON_ACTIVE_LOW_MASK BIT(5) + #define LTC4282_CTRL_ON_DELAY_MASK BIT(6) +#define LTC4282_CTRL_MSB 0x01 + #define LTC4282_CTRL_VIN_MODE_MASK GENMASK(1, 0) + #define LTC4282_CTRL_OV_MODE_MASK GENMASK(3, 2) + #define LTC4282_CTRL_UV_MODE_MASK GENMASK(5, 4) +#define LTC4282_FAULT_LOG 0x04 + #define LTC4282_OV_FAULT_MASK BIT(0) + #define LTC4282_UV_FAULT_MASK BIT(1) + #define LTC4282_OC_FAULT_MASK BIT(2) + #define LTC4282_POWER_BAD_FAULT_MASK BIT(3) + #define LTC4282_FET_SHORT_FAULT_MASK BIT(5) + #define LTC4282_FET_BAD_FAULT_MASK BIT(6) +#define LTC4282_ADC_ALERT_LOG 0x05 + #define LTC4282_GPIO_ALARM_L_MASK BIT(0) + #define LTC4282_GPIO_ALARM_H_MASK BIT(1) + #define LTC4282_VSOURCE_ALARM_L_MASK BIT(2) + #define LTC4282_VSOURCE_ALARM_H_MASK BIT(3) + #define LTC4282_VSENSE_ALARM_L_MASK BIT(4) + #define LTC4282_VSENSE_ALARM_H_MASK BIT(5) + #define LTC4282_POWER_ALARM_L_MASK BIT(6) + #define LTC4282_POWER_ALARM_H_MASK BIT(7) +#define LTC4282_FET_BAD_FAULT_TIMEOUT 0x06 + #define LTC4282_FET_BAD_MAX_TIMEOUT 255 +#define LTC4282_GPIO_CONFIG 0x07 + #define LTC4282_GPIO_2_FET_STRESS_MASK BIT(1) + #define LTC4282_GPIO_1_OUT_MASK BIT(3) + #define LTC4282_GPIO_1_CONFIG_MASK GENMASK(5, 4) + #define LTC4282_GPIO_2_OUT_MASK BIT(6) + #define LTC4282_GPIO_3_OUT_MASK BIT(7) +#define LTC4282_VGPIO_MIN 0x08 +#define LTC4282_VGPIO_MAX 0x09 +#define LTC4282_VSOURCE_MIN 0x0a +#define LTC4282_VSOURCE_MAX 0x0b +#define LTC4282_VSENSE_MIN 0x0c +#define LTC4282_VSENSE_MAX 0x0d +#define LTC4282_POWER_MIN 0x0e +#define LTC4282_POWER_MAX 0x0f +#define LTC4282_CLK_DIV 0x10 + #define LTC4282_CLK_DIV_MASK GENMASK(4, 0) + #define LTC4282_CLKOUT_MASK GENMASK(6, 5) +#define LTC4282_ILIM_ADJUST 0x11 + #define LTC4282_GPIO_MODE_MASK BIT(1) + #define LTC4282_VDD_MONITOR_MASK BIT(2) + #define LTC4282_FOLDBACK_MODE_MASK GENMASK(4, 3) + #define LTC4282_ILIM_ADJUST_MASK GENMASK(7, 5) +#define LTC4282_ENERGY 0x12 +#define LTC4282_TIME_COUNTER 0x18 +#define LTC4282_ALERT_CTRL 0x1c + #define LTC4282_ALERT_OUT_MASK BIT(6) +#define LTC4282_ADC_CTRL 0x1d + #define LTC4282_FAULT_LOG_EN_MASK BIT(2) + #define LTC4282_METER_HALT_MASK BIT(5) + #define LTC4282_METER_RESET_MASK BIT(6) + #define LTC4282_RESET_MASK BIT(7) +#define LTC4282_STATUS_LSB 0x1e + #define LTC4282_OV_STATUS_MASK BIT(0) + #define LTC4282_UV_STATUS_MASK BIT(1) + #define LTC4282_VDD_STATUS_MASK \ + (LTC4282_OV_STATUS_MASK | LTC4282_UV_STATUS_MASK) + #define LTC4282_OC_STATUS_MASK BIT(2) + #define LTC4282_POWER_GOOD_MASK BIT(3) + #define LTC4282_FET_SHORT_MASK BIT(5) + #define LTC4282_FET_BAD_STATUS_MASK BIT(6) +#define LTC4282_STATUS_MSB 0x1f + #define LTC4282_ALERT_STATUS_MASK BIT(4) + #define LTC4282_GPIO_1_STATUS_MASK BIT(5) + #define LTC4282_GPIO_2_STATUS_MASK BIT(6) + #define LTC4282_GPIO_3_STATUS_MASK BIT(7) +#define LTC4282_RESERVED_1 0x32 +#define LTC4282_RESERVED_2 0x33 +#define LTC4282_VGPIO 0x34 +#define LTC4282_VGPIO_LOWEST 0x36 +#define LTC4282_VGPIO_HIGHEST 0x38 +#define LTC4282_VSOURCE 0x3a +#define LTC4282_VSOURCE_LOWEST 0x3c +#define LTC4282_VSOURCE_HIGHEST 0x3e +#define LTC4282_VSENSE 0x40 +#define LTC4282_VSENSE_LOWEST 0x42 +#define LTC4282_VSENSE_HIGHEST 0x44 +#define LTC4282_POWER 0x46 +#define LTC4282_POWER_LOWEST 0x48 +#define LTC4282_POWER_HIGHEST 0x4a +#define LTC4282_RESERVED_3 0x50 + +#define LTC4282_CLKIN_MIN (250 * KILO) +#define LTC4282_CLKIN_MAX (15500 * KILO) +#define LTC4282_CLKIN_RANGE (LTC4282_CLKIN_MAX - LTC4282_CLKIN_MIN + 1) +#define LTC4282_CLKOUT_SYSTEM (250 * KILO) +#define LTC4282_CLKOUT_CNV 15 + +#define LTC4282_GPIO_NR 4 +/* + * relaxed version of FIELD_PREP() to be used when mask is not a compile time + * constant u32_encode_bits() can't also be used as the compiler needs to be + * able to evaluate mask at compile time. + */ +#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) + +enum { + LTC4282_CHAN_VSOURCE, + LTC4282_CHAN_VDD, + LTC4282_CHAN_VGPIO, +}; + +struct ltc4282_cache { + u32 in_max_raw; + u32 in_min_raw; + long in_highest; + long in_lowest; + bool en; +}; + +struct ltc4282_state { + struct regmap *map; + /* Protect against multiple accesses to the device registers */ + struct mutex lock; + struct gpio_chip gc; + struct clk_hw clk_hw; + /* + * Used to cache values for VDD/VSOURCE depending which will be used + * when hwmon is not enabled for that channel. Needed because they share + * the same registers. + */ + struct ltc4282_cache in0_1_cache[LTC4282_CHAN_VGPIO]; + unsigned long valid_mask; + long power_max; + u32 rsense; + u32 vin_mode; + u16 vfs_out; + bool energy_en; +}; + +struct ltc4282_gpio { + const char * const *funcs; + u32 out_reg; + u32 out_mask; + u32 in_reg; + u32 in_mask; + bool active_high; + u8 n_funcs; +}; + +enum { + LTC4282_VIN_3_3V, + LTC4282_VIN_5V, + LTC4282_VIN_12V, + LTC4282_VIN_24V, +}; + +enum { + LTC4282_GPIO_1, + LTC4282_GPIO_2, + LTC4282_GPIO_3, + LTC4282_ALERT, + LTC4282_GPIO_MAX +}; + +static const char * const ltc4282_gpio1_modes[] = { + "gpio", "power_bad", "power_good" +}; + +static const char * const ltc4282_gpio2_modes[] = { + "gpio", "adc_input", "stress_fet" +}; + +static const char * const ltc4282_gpio3_modes[] = { + "gpio", "adc_input" +}; + +static const struct ltc4282_gpio ltc4282_gpios[] = { + [LTC4282_GPIO_1] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_1_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_1_OUT_MASK, + .active_high = true, + .funcs = ltc4282_gpio1_modes, + .n_funcs = ARRAY_SIZE(ltc4282_gpio1_modes), + }, + [LTC4282_GPIO_2] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_2_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_2_OUT_MASK, + .funcs = ltc4282_gpio2_modes, + .n_funcs = ARRAY_SIZE(ltc4282_gpio2_modes), + }, + [LTC4282_GPIO_3] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_3_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_3_OUT_MASK, + .funcs = ltc4282_gpio3_modes, + .n_funcs = ARRAY_SIZE(ltc4282_gpio3_modes), + }, + [LTC4282_ALERT] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_ALERT_STATUS_MASK, + .out_reg = LTC4282_ALERT_CTRL, + .out_mask = LTC4282_ALERT_OUT_MASK, + }, +}; + +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + + /* we can only control this for GPIO_1 */ + if (offset != LTC4282_GPIO_1) + return 0; + + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_1_CONFIG_MASK); +} + +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset, + int val) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; + + guard(mutex)(&st->lock); + /* + * Explicitly setting the pin as output can only be done for GPIO_1. For + * the other pins we just pull the line down or high-z. + */ + if (offset == LTC4282_GPIO_1) { + int ret; + + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_1_CONFIG_MASK, + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2)); + if (ret) + return ret; + } + + /* + * GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down + * the line + */ + if (!gpio->active_high) + val = !val; + + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, + field_prep(gpio->out_mask, val)); +} + +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, + int val) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; + + if (!gpio->active_high) + val = !val; + + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, + field_prep(gpio->out_mask, val)); +} + +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + const struct ltc4282_gpio *gpio = <c4282_gpios[offset]; + int ret; + u32 val; + + ret = regmap_read(st->map, gpio->in_reg, &val); + if (ret) + return ret; + + return !!(val & gpio->in_mask); +} + +static int ltc4282_gpio_valid_mask(struct gpio_chip *chip, + unsigned long *valid_mask, + unsigned int ngpios) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + + *valid_mask = st->valid_mask; + return 0; +} + +enum { + LTC4282_CLKOUT_NONE, + LTC4282_CLKOUT_INT, + LTC4282_CLKOUT_TICK, +}; + +static int ltc4282_set_rate(struct clk_hw *hw, + unsigned long rate, unsigned long parent_rate) +{ + struct ltc4282_state *st = container_of(hw, struct ltc4282_state, + clk_hw); + u32 val = LTC4282_CLKOUT_INT; + + if (rate == LTC4282_CLKOUT_CNV) + val = LTC4282_CLKOUT_TICK; + + return regmap_update_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLKOUT_MASK, + FIELD_PREP(LTC4282_CLKOUT_MASK, val)); +} + +/* + * Note the 15HZ conversion rate assumes 12bit ADC which is what we are + * supporting for now. + */ +static unsigned int ltc4282_out_rates[] = { + LTC4282_CLKOUT_CNV, LTC4282_CLKOUT_SYSTEM +}; + +long ltc4282_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + int idx = find_closest(rate, ltc4282_out_rates, + ARRAY_SIZE(ltc4282_out_rates)); + + return ltc4282_out_rates[idx]; +} + +unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long parent) +{ + struct ltc4282_state *st = container_of(hw, struct ltc4282_state, + clk_hw); + u32 clkdiv; + int ret; + + ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv); + if (ret) + return 0; + + clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv); + if (!clkdiv) + return 0; + if (clkdiv == LTC4282_CLKOUT_INT) + return LTC4282_CLKOUT_SYSTEM; + + return LTC4282_CLKOUT_CNV; +} + +static void ltc4282_disable(struct clk_hw *clk_hw) +{ + struct ltc4282_state *st = container_of(clk_hw, struct ltc4282_state, + clk_hw); + + regmap_clear_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLKOUT_MASK); +} + +static int ltc4282_read_voltage_word(const struct ltc4282_state *st, u32 reg, + u32 fs, long *val) +{ + __be16 in; + int ret; + + ret = regmap_bulk_read(st->map, reg, &in, sizeof(in)); + if (ret) + return ret; + + /* + * This is also used to calculate current in which case fs comes in + * 10 * uV. Hence the ULL usage. + */ + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); + return 0; +} + +static int ltc4282_read_voltage_byte_cached(const struct ltc4282_state *st, + u32 reg, u32 fs, long *val, + u32 *cached_raw) +{ + int ret; + u32 in; + + if (cached_raw) { + in = *cached_raw; + } else { + ret = regmap_read(st->map, reg, &in); + if (ret) + return ret; + } + + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); + return 0; +} + +static int ltc4282_read_voltage_byte(const struct ltc4282_state *st, u32 reg, + u32 fs, long *val) +{ + return ltc4282_read_voltage_byte_cached(st, reg, fs, val, NULL); +} + +static int __ltc4282_read_alarm(struct ltc4282_state *st, u32 reg, u32 mask, + long *val) +{ + u32 alarm; + int ret; + + ret = regmap_read(st->map, reg, &alarm); + if (ret) + return ret; + + *val = !!(alarm & mask); + + /* if not status/fault logs, clear the alarm after reading it */ + if (reg != LTC4282_STATUS_LSB && reg != LTC4282_FAULT_LOG) + return regmap_clear_bits(st->map, reg, mask); + + return 0; +} + +static int ltc4282_read_alarm(struct ltc4282_state *st, u32 reg, u32 mask, + long *val) +{ + guard(mutex)(&st->lock); + return __ltc4282_read_alarm(st, reg, mask, val); +} + +static int ltc4282_vdd_source_read_in(struct ltc4282_state *st, u32 channel, + long *val) +{ + guard(mutex)(&st->lock); + if (!st->in0_1_cache[channel].en) + return -ENODATA; + + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, val); +} + +static int ltc4282_vdd_source_read_hist(struct ltc4282_state *st, u32 reg, + u32 channel, long *cached, long *val) +{ + int ret; + + guard(mutex)(&st->lock); + if (!st->in0_1_cache[channel].en) { + *val = *cached; + return 0; + } + + ret = ltc4282_read_voltage_word(st, reg, st->vfs_out, val); + if (ret) + return ret; + + *cached = *val; + return 0; +} + +static int ltc4282_vdd_source_read_lim(struct ltc4282_state *st, u32 reg, + u32 channel, u32 *cached, long *val) +{ + guard(mutex)(&st->lock); + if (!st->in0_1_cache[channel].en) + return ltc4282_read_voltage_byte_cached(st, reg, st->vfs_out, + val, cached); + + return ltc4282_read_voltage_byte(st, reg, st->vfs_out, val); +} + +static int ltc4282_vdd_source_read_alm(struct ltc4282_state *st, u32 mask, + u32 channel, long *val) +{ + guard(mutex)(&st->lock); + if (!st->in0_1_cache[channel].en) { + /* + * Do this otherwise alarms can get confused because we clear + * them after reading them. So, if someone mistakenly reads + * VSOURCE right before VDD (or the other way around), we might + * get no alarm just because it was cleared when reading VSOURCE + * and had no time for a new conversion and thus having the + * alarm again. + */ + *val = 0; + return 0; + } + + return __ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, mask, val); +} + +static int ltc4282_read_in(struct ltc4282_state *st, u32 attr, long *val, + u32 channel) +{ + switch (attr) { + case hwmon_in_input: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_read_voltage_word(st, LTC4282_VGPIO, + 1280, val); + + return ltc4282_vdd_source_read_in(st, channel, val); + case hwmon_in_highest: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_read_voltage_word(st, + LTC4282_VGPIO_HIGHEST, + 1280, val); + + return ltc4282_vdd_source_read_hist(st, LTC4282_VSOURCE_HIGHEST, + channel, + &st->in0_1_cache[channel].in_highest, val); + case hwmon_in_lowest: + if (channel == LTC4282_CHAN_VGPIO) + ltc4282_read_voltage_word(st, LTC4282_VGPIO_LOWEST, + 1280, val); + + return ltc4282_vdd_source_read_hist(st, LTC4282_VSOURCE_LOWEST, + channel, + &st->in0_1_cache[channel].in_lowest, val); + case hwmon_in_max_alarm: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_GPIO_ALARM_H_MASK, + val); + + return ltc4282_vdd_source_read_alm(st, + LTC4282_VSOURCE_ALARM_H_MASK, + channel, val); + case hwmon_in_min_alarm: + if (channel == LTC4282_CHAN_VGPIO) + ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_GPIO_ALARM_L_MASK, val); + + return ltc4282_vdd_source_read_alm(st, + LTC4282_VSOURCE_ALARM_L_MASK, + channel, val); + case hwmon_in_crit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, + LTC4282_OV_STATUS_MASK, val); + case hwmon_in_lcrit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, + LTC4282_UV_STATUS_MASK, val); + case hwmon_in_max: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MAX, + 1280, val); + + return ltc4282_vdd_source_read_lim(st, LTC4282_VSOURCE_MAX, + channel, + &st->in0_1_cache[channel].in_max_raw, val); + case hwmon_in_min: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MIN, + 1280, val); + + return ltc4282_vdd_source_read_lim(st, LTC4282_VSOURCE_MIN, + channel, + &st->in0_1_cache[channel].in_min_raw, val); + case hwmon_in_enable: + scoped_guard(mutex, &st->lock) { + *val = st->in0_1_cache[channel].en; + } + return 0; + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_read_current_word(const struct ltc4282_state *st, u32 reg, + long *val) +{ + long in; + int ret; + + /* + * We pass in full scale in 10 * micro (note that 40 is already + * millivolt) so we have better approximations to calculate current. + */ + ret = ltc4282_read_voltage_word(st, reg, DECA * 40 * MILLI, &in); + if (ret) + return ret; + + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); + + return 0; +} + +static int ltc4282_read_current_byte(const struct ltc4282_state *st, u32 reg, + long *val) +{ + long in; + int ret; + + ret = ltc4282_read_voltage_byte(st, reg, DECA * 40 * MILLI, &in); + if (ret) + return ret; + + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); + + return 0; +} + +static int ltc4282_read_curr(struct ltc4282_state *st, const u32 attr, + long *val) +{ + switch (attr) { + case hwmon_curr_input: + return ltc4282_read_current_word(st, LTC4282_VSENSE, val); + case hwmon_curr_highest: + return ltc4282_read_current_word(st, LTC4282_VSENSE_HIGHEST, + val); + case hwmon_curr_lowest: + return ltc4282_read_current_word(st, LTC4282_VSENSE_LOWEST, + val); + case hwmon_curr_max: + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MAX, val); + case hwmon_curr_min: + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MIN, val); + case hwmon_curr_max_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_VSENSE_ALARM_H_MASK, val); + case hwmon_curr_min_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_VSENSE_ALARM_L_MASK, val); + case hwmon_curr_crit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, + LTC4282_OC_STATUS_MASK, val); + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, + long *val) +{ + u64 temp = DECA * 40ULL * st->vfs_out * BIT(16), temp_2; + __be16 raw; + u16 power; + int ret; + + ret = regmap_bulk_read(st->map, reg, &raw, sizeof(raw)); + if (ret) + return ret; + + power = be16_to_cpu(raw); + /* + * Power is given by: + * P = CODE(16b) * 0.040 * Vfs(out) * 2^16 / ((2^16 - 1)^2 * Rsense) + */ + if (check_mul_overflow(power * temp, MICRO, &temp_2)) { + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, + U16_MAX * (u64)st->rsense); + return 0; + } + + *val = DIV64_U64_ROUND_CLOSEST(temp_2, + st->rsense * int_pow(U16_MAX, 2)); + + return 0; +} + +static int ltc4282_read_power_byte(const struct ltc4282_state *st, u32 reg, + long *val) +{ + u32 power; + u64 temp; + int ret; + + ret = regmap_read(st->map, reg, &power); + if (ret) + return ret; + + temp = power * 40 * DECA * st->vfs_out * BIT_ULL(8); + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, + int_pow(U8_MAX, 2) * st->rsense); + + return 0; +} + +static int ltc4282_read_energy(const struct ltc4282_state *st, u64 *val) +{ + u64 temp, energy; + __be64 raw; + int ret; + + ret = regmap_bulk_read(st->map, LTC4282_ENERGY, &raw, 6); + if (ret) + return ret; + + energy = be64_to_cpu(raw) >> 16; + /* + * The formula for energy is given by: + * E = CODE(48b) * 0.040 * Vfs(out) * Tconv * 256 / + * ((2^16 - 1)^2 * Rsense) + * + * Since we only support 12bit ADC, Tconv = 0.065535s. Passing Vfs(out) + * and 0.040 to mV and Tconv to us, we can simplify the formula to: + * E = CODE(48b) * 40 * Vfs(out) * 256 / (U16_MAX * Rsense) + * + * As Rsense can have tenths of micro-ohm resolution, we need to + * multiply by DECA to get microujoule. + */ + if (check_mul_overflow(DECA * st->vfs_out * 40 * BIT(8), energy, &temp)) { + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * BIT(8), U16_MAX); + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); + return 0; + } + + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); + + return 0; +} + +static int ltc4282_read_power(struct ltc4282_state *st, const u32 attr, + long *val) +{ + switch (attr) { + case hwmon_power_input: + return ltc4282_read_power_word(st, LTC4282_POWER, val); + case hwmon_power_input_highest: + return ltc4282_read_power_word(st, LTC4282_POWER_HIGHEST, val); + case hwmon_power_input_lowest: + return ltc4282_read_power_word(st, LTC4282_POWER_LOWEST, val); + case hwmon_power_max_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_POWER_ALARM_H_MASK, val); + case hwmon_power_min_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_POWER_ALARM_L_MASK, val); + case hwmon_power_max: + return ltc4282_read_power_byte(st, LTC4282_POWER_MAX, val); + case hwmon_power_min: + return ltc4282_read_power_byte(st, LTC4282_POWER_MIN, val); + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (type) { + case hwmon_in: + return ltc4282_read_in(st, attr, val, channel); + case hwmon_curr: + return ltc4282_read_curr(st, attr, val); + case hwmon_power: + return ltc4282_read_power(st, attr, val); + case hwmon_energy: + scoped_guard(mutex, &st->lock) { + *val = st->energy_en; + } + return 0; + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_write_power_byte(const struct ltc4282_state *st, u32 reg, + long val) +{ + u32 power; + u64 temp; + + if (val > st->power_max) + val = st->power_max; + + temp = val * int_pow(U8_MAX, 2) * st->rsense; + power = DIV64_U64_ROUND_CLOSEST(temp, + MICRO * DECA * 256ULL * st->vfs_out * 40); + + return regmap_write(st->map, reg, power); +} + +static int __ltc4282_write_history(const struct ltc4282_state *st, u32 reg, + long lowest, long highest) +{ + __be16 __raw; + u16 tmp; + int ret; + + tmp = DIV_ROUND_CLOSEST(U16_MAX * lowest, st->vfs_out); + + __raw = cpu_to_be16(tmp); + + ret = regmap_bulk_write(st->map, reg, &__raw, 2); + if (ret) + return ret; + + tmp = DIV_ROUND_CLOSEST(U16_MAX * highest, st->vfs_out); + + __raw = cpu_to_be16(tmp); + + return regmap_bulk_write(st->map, reg + 2, &__raw, 2); +} + +static int ltc4282_write_history(struct ltc4282_state *st, u32 reg, long lowest, + long highest) +{ + guard(mutex)(&st->lock); + return __ltc4282_write_history(st, reg, lowest, highest); +} + +static int ltc4282_write_power(struct ltc4282_state *st, u32 attr, + long val) +{ + switch (attr) { + case hwmon_power_max: + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, val); + case hwmon_power_min: + return ltc4282_write_power_byte(st, LTC4282_POWER_MIN, val); + case hwmon_power_reset_history: + return ltc4282_write_history(st, LTC4282_POWER_LOWEST, 0, 0); + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_write_voltage_byte_cached(const struct ltc4282_state *st, + u32 reg, u32 fs, long val, + u32 *cache_raw) +{ + u32 in; + + if (val >= fs) + in = U8_MAX; + else + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); + + if (cache_raw) { + *cache_raw = in; + return 0; + } + + return regmap_write(st->map, reg, in); +} + +static int ltc4282_write_voltage_byte(const struct ltc4282_state *st, u32 reg, + u32 fs, long val) +{ + return ltc4282_write_voltage_byte_cached(st, reg, fs, val, NULL); +} + +static int ltc4282_cache_history(struct ltc4282_state *st, u32 channel) +{ + long val; + int ret; + + ret = ltc4282_read_voltage_word(st, LTC4282_VSOURCE_LOWEST, st->vfs_out, + &val); + if (ret) + return ret; + + st->in0_1_cache[channel].in_lowest = val; + + ret = ltc4282_read_voltage_word(st, LTC4282_VSOURCE_HIGHEST, + st->vfs_out, &val); + if (ret) + return ret; + + st->in0_1_cache[channel].in_highest = val; + + ret = regmap_read(st->map, LTC4282_VSOURCE_MIN, + &st->in0_1_cache[channel].in_min_raw); + if (ret) + return ret; + + return regmap_read(st->map, LTC4282_VSOURCE_MAX, + &st->in0_1_cache[channel].in_max_raw); +} + +static int ltc4282_cache_sync(struct ltc4282_state *st, u32 channel) +{ + int ret; + + ret = __ltc4282_write_history(st, LTC4282_VSOURCE_LOWEST, + st->in0_1_cache[channel].in_lowest, + st->in0_1_cache[channel].in_highest); + if (ret) + return ret; + + ret = regmap_write(st->map, LTC4282_VSOURCE_MIN, + st->in0_1_cache[channel].in_min_raw); + + return regmap_write(st->map, LTC4282_VSOURCE_MAX, + st->in0_1_cache[channel].in_max_raw); +} + +static int ltc4282_vdd_source_write_lim(struct ltc4282_state *st, u32 reg, + int channel, u32 *cache, long val) +{ + int ret; + + guard(mutex)(&st->lock); + if (st->in0_1_cache[channel].en) + ret = ltc4282_write_voltage_byte(st, reg, st->vfs_out, val); + else + ret = ltc4282_write_voltage_byte_cached(st, reg, st->vfs_out, + val, cache); + + return ret; +} + +static int ltc4282_vdd_source_write_hist(struct ltc4282_state *st, int channel) +{ + int ret; + + guard(mutex)(&st->lock); + if (st->in0_1_cache[channel].en) { + ret = __ltc4282_write_history(st, LTC4282_VSOURCE_LOWEST, 0, 0); + if (ret) + return ret; + } + + st->in0_1_cache[channel].in_lowest = 0; + st->in0_1_cache[channel].in_highest = 0; + + return 0; +} + +/* + * We need to mux between VSOURCE and VDD which means they are mutually + * exclusive. Moreover, we can't really disable both VDD and VSOURCE as the ADC + * is continuously running (we cannot independently halt it without also + * stopping VGPIO). Hence, the logic is that disabling or enabling VDD will + * automatically have the reverse effect on VSOURCE and vice-versa. + */ +static int ltc4282_vdd_source_enable(struct ltc4282_state *st, int channel, + long val) +{ + int ret, other_chan = ~channel & 0x1; + u8 __val = val; + + guard(mutex)(&st->lock); + if (st->in0_1_cache[channel].en == !!val) + return 0; + + /* clearing the bit makes the ADC to monitor VDD */ + if (channel == LTC4282_CHAN_VDD) + __val = !__val; + + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, + LTC4282_VDD_MONITOR_MASK, + FIELD_PREP(LTC4282_VDD_MONITOR_MASK, !!__val)); + if (ret) + return ret; + + st->in0_1_cache[channel].en = !!val; + st->in0_1_cache[other_chan].en = !val; + + if (st->in0_1_cache[channel].en) { + /* + * Then, we are disabling @other_chan. Let's save it's current + * history. + */ + ret = ltc4282_cache_history(st, other_chan); + if (ret) + return ret; + + return ltc4282_cache_sync(st, channel); + } + /* + * Then, we are enabling @other_chan. We need to do the opposite from + * above. + */ + ret = ltc4282_cache_history(st, channel); + if (ret) + return ret; + + return ltc4282_cache_sync(st, other_chan); +} + +static int ltc4282_write_in(struct ltc4282_state *st, u32 attr, long val, + int channel) +{ + switch (attr) { + case hwmon_in_max: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MAX, + 1280, val); + + return ltc4282_vdd_source_write_lim(st, LTC4282_VSOURCE_MAX, + channel, + &st->in0_1_cache[channel].in_max_raw, val); + case hwmon_in_min: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MIN, + 1280, val); + + return ltc4282_vdd_source_write_lim(st, LTC4282_VSOURCE_MIN, + channel, + &st->in0_1_cache[channel].in_min_raw, val); + case hwmon_in_reset_history: + if (channel == LTC4282_CHAN_VGPIO) + return ltc4282_write_history(st, LTC4282_VGPIO_LOWEST, + 0, 0); + + return ltc4282_vdd_source_write_hist(st, channel); + case hwmon_in_enable: + return ltc4282_vdd_source_enable(st, channel, val); + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_write_curr(struct ltc4282_state *st, u32 attr, + long val) +{ + /* need to pass it in millivolt */ + u32 in = DIV_ROUND_CLOSEST_ULL((u64)val * st->rsense, DECA * MICRO); + + switch (attr) { + case hwmon_curr_max: + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40, + in); + case hwmon_curr_min: + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MIN, 40, + in); + case hwmon_curr_reset_history: + return ltc4282_write_history(st, LTC4282_VSENSE_LOWEST, 0, 0); + default: + return -EOPNOTSUPP; + } +} + +static int ltc4282_energy_enable_set(struct ltc4282_state *st, long val) +{ + int ret; + + guard(mutex)(&st->lock); + /* setting the bit halts the meter */ + ret = regmap_update_bits(st->map, LTC4282_ADC_CTRL, + LTC4282_METER_HALT_MASK, + FIELD_PREP(LTC4282_METER_HALT_MASK, !val)); + if (ret) + return ret; + + st->energy_en = !!val; + + return 0; +} + +static int ltc4282_write(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (type) { + case hwmon_power: + return ltc4282_write_power(st, attr, val); + case hwmon_in: + return ltc4282_write_in(st, attr, val, channel); + case hwmon_curr: + return ltc4282_write_curr(st, attr, val); + case hwmon_energy: + return ltc4282_energy_enable_set(st, val); + default: + return -EOPNOTSUPP; + } +} + +static umode_t ltc4282_in_is_visible(const struct ltc4282_state *st, u32 attr) +{ + switch (attr) { + case hwmon_in_input: + case hwmon_in_highest: + case hwmon_in_lowest: + case hwmon_in_max_alarm: + case hwmon_in_min_alarm: + case hwmon_in_label: + case hwmon_in_lcrit_alarm: + case hwmon_in_crit_alarm: + return 0444; + case hwmon_in_max: + case hwmon_in_min: + case hwmon_in_enable: + case hwmon_in_reset_history: + return 0644; + default: + return 0; + } +} + +static umode_t ltc4282_curr_is_visible(u32 attr) +{ + switch (attr) { + case hwmon_curr_input: + case hwmon_curr_highest: + case hwmon_curr_lowest: + case hwmon_curr_max_alarm: + case hwmon_curr_min_alarm: + case hwmon_curr_crit_alarm: + case hwmon_curr_label: + return 0444; + case hwmon_curr_max: + case hwmon_curr_min: + case hwmon_curr_reset_history: + return 0644; + default: + return 0; + } +} + +static umode_t ltc4282_power_is_visible(u32 attr) +{ + switch (attr) { + case hwmon_power_input: + case hwmon_power_input_highest: + case hwmon_power_input_lowest: + case hwmon_power_label: + case hwmon_power_max_alarm: + case hwmon_power_min_alarm: + return 0444; + case hwmon_power_max: + case hwmon_power_min: + case hwmon_power_reset_history: + return 0644; + default: + return 0; + } +} + +static umode_t ltc4282_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_in: + return ltc4282_in_is_visible(data, attr); + case hwmon_curr: + return ltc4282_curr_is_visible(attr); + case hwmon_power: + return ltc4282_power_is_visible(attr); + case hwmon_energy: + /* hwmon_energy_enable */ + return 0644; + default: + return 0; + } +} + +static const char * const ltc4282_in_strs[] = { + "VSOURCE", "VDD", "VGPIO" +}; + +static int ltc4282_read_labels(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + switch (type) { + case hwmon_in: + *str = ltc4282_in_strs[channel]; + return 0; + case hwmon_curr: + *str = "ISENSE"; + return 0; + case hwmon_power: + *str = "Power"; + return 0; + default: + return -EOPNOTSUPP; + } +} + +static ssize_t ltc4282_energy_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + u64 energy; + int ret; + + guard(mutex)(&st->lock); + if (!st->energy_en) + return -ENODATA; + + ret = ltc4282_read_energy(st, &energy); + if (ret < 0) + return ret; + + return sysfs_emit(buf, "%llu\n", energy); +} + +static ssize_t ltc4282_status_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); + long alarm; + int ret; + + ret = ltc4282_read_alarm(st, LTC4282_STATUS_LSB, attr->index, &alarm); + if (ret) + return ret; + + return sysfs_emit(buf, "%lu\n", alarm); +} + +static ssize_t ltc4282_clear_faults_store(struct device *dev, + struct device_attribute *da, + const char *buf, size_t len) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + int ret; + + ret = regmap_write(st->map, LTC4282_FAULT_LOG, 0); + if (ret) + return ret; + + return len; +} + +static const struct clk_ops ltc4282_ops = { + .recalc_rate = ltc4282_recalc_rate, + .round_rate = ltc4282_round_rate, + .set_rate = ltc4282_set_rate, + .disable = ltc4282_disable, +}; + +static int ltc428_clk_provider_setup(struct ltc4282_state *st, + struct device *dev) +{ + struct clk_init_data init; + int ret; + + if (!IS_ENABLED(CONFIG_COMMON_CLK)) + return 0; + + init.name = "ltc4282-clk"; + init.ops = <c4282_ops; + init.flags = CLK_GET_RATE_NOCACHE; + st->clk_hw.init = &init; + + ret = devm_clk_hw_register(dev, &st->clk_hw); + if (ret) + return ret; + + return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, + &st->clk_hw); +} + +static int ltc428_clks_setup(struct ltc4282_state *st, struct device *dev) +{ + unsigned long rate; + struct clk *clkin; + u32 val; + int ret; + + ret = ltc428_clk_provider_setup(st, dev); + if (ret) + return ret; + + clkin = devm_clk_get_optional_enabled(dev, NULL); + if (IS_ERR(clkin)) + return dev_err_probe(dev, PTR_ERR(clkin), + "Failed to get clkin"); + if (!clkin) + return 0; + + rate = clk_get_rate(clkin); + if (in_range(rate, LTC4282_CLKIN_MIN, LTC4282_CLKIN_RANGE)) + return dev_err_probe(dev, -EINVAL, + "Invalid clkin range(%lu) [%lu %lu]\n", + rate, LTC4282_CLKIN_MIN, + LTC4282_CLKIN_MAX); + + /* + * Clocks faster than 250KHZ should be reduced to 250KHZ. The clock + * frequency is divided by twice the value in the register. + */ + val = rate / (2 * LTC4282_CLKIN_MIN); + + return regmap_update_bits(st->map, LTC4282_CLK_DIV, + LTC4282_CLK_DIV_MASK, + FIELD_PREP(LTC4282_CLK_DIV_MASK, val)); +} + +static const int ltc4282_curr_lim_uv[] = { + 12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375 +}; + +static int ltc4282_get_defaults(struct ltc4282_state *st, u32 *curr_lim_uv) +{ + u32 reg_val, ilm_adjust; + int ret; + + ret = regmap_read(st->map, LTC4282_ADC_CTRL, ®_val); + if (ret) + return ret; + + st->energy_en = !FIELD_GET(LTC4282_METER_HALT_MASK, reg_val); + + ret = regmap_read(st->map, LTC4282_CTRL_MSB, ®_val); + if (ret) + return ret; + + st->vin_mode = FIELD_GET(LTC4282_CTRL_VIN_MODE_MASK, reg_val); + + ret = regmap_read(st->map, LTC4282_ILIM_ADJUST, ®_val); + if (ret) + return ret; + + ilm_adjust = FIELD_GET(LTC4282_ILIM_ADJUST_MASK, reg_val); + *curr_lim_uv = ltc4282_curr_lim_uv[ilm_adjust]; + + st->in0_1_cache[LTC4282_CHAN_VSOURCE].en = FIELD_GET(LTC4282_VDD_MONITOR_MASK, + ilm_adjust); + if (!st->in0_1_cache[LTC4282_CHAN_VSOURCE].en) { + st->in0_1_cache[LTC4282_CHAN_VDD].en = true; + return regmap_read(st->map, LTC4282_VSOURCE_MAX, + &st->in0_1_cache[LTC4282_CHAN_VSOURCE].in_max_raw); + } + + return regmap_read(st->map, LTC4282_VSOURCE_MAX, + &st->in0_1_cache[LTC4282_CHAN_VDD].in_max_raw); +} + +/* + * Set max limits for ISENSE and Power as that depends on the max voltage on + * rsense that is defined in ILIM_ADJUST. This is specially important for power + * because for some rsense and vfsout values, if we allow the default raw 255 + * value, that would overflow long in 32bit archs when reading back the max + * power limit. + */ +static int ltc4282_set_max_limits(struct ltc4282_state *st, u32 val_uv) +{ + int ret; + + ret = ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40 * MILLI, + val_uv); + if (ret) + return ret; + + /* Power is given by ISENSE * Vout. */ + st->power_max = DIV_ROUND_CLOSEST(val_uv * DECA * MILLI, st->rsense) * st->vfs_out; + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, st->power_max); +} + +static int ltc4282_non_gpio_setup(struct ltc4282_state *st, + const struct device *dev, u32 pin, + const char *func, bool *adc_in) +{ + if (pin == LTC4282_GPIO_1) { + /* power_good */ + u32 val = 0; + + if (!strcmp(func, "power_bad")) + val = 1; + + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_1_CONFIG_MASK, + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, val)); + } + + if (!strcmp(func, "stress_fet")) + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_2_FET_STRESS_MASK, + FIELD_PREP(LTC4282_GPIO_2_FET_STRESS_MASK, 1)); + + /* + * Then, let's point the given GPIO to the ADC input. We need to ensure that + * function is only given once. + */ + if (*adc_in) + return dev_err_probe(dev, -EINVAL, + "Only one gpio can be given to the ADC input\n"); + + *adc_in = true; + + /* setting the bit to 1 cause the ADC to monitor GPIO2 */ + return regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, + LTC4282_GPIO_MODE_MASK, + FIELD_PREP(LTC4282_GPIO_MODE_MASK, pin == LTC4282_GPIO_2)); +} + +static const char * const ltc4282_gpio_prop[] = { + "adi,gpio1-mode", "adi,gpio2-mode", "adi,gpio3-mode" +}; + +static int ltc4282_gpio_setup(struct ltc4282_state *st, struct device *dev) +{ + bool adc_in = false, has_gpio = false; + const char *func; + u32 gpio; + int ret; + + if (!IS_ENABLED(CONFIG_GPIOLIB)) + return 0; + + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { + ret = device_property_read_string(dev, ltc4282_gpio_prop[gpio], + &func); + if (ret) + continue; + if (match_string(ltc4282_gpios[gpio].funcs, + ltc4282_gpios[gpio].n_funcs, func) < 0) + return dev_err_probe(dev, ret, + "Invalid func(%s) for gpio%u\n", + func, gpio + 1); + if (!strcmp(func, "gpio")) { + __set_bit(gpio, &st->valid_mask); + has_gpio = true; + if (gpio == LTC4282_GPIO_1) { + /* default to input GPIO */ + ret = regmap_set_bits(st->map, + LTC4282_GPIO_CONFIG, + LTC4282_GPIO_1_CONFIG_MASK); + if (ret) + return ret; + } + + continue; + } + + ret = ltc4282_non_gpio_setup(st, dev, gpio, func, &adc_in); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,gpio-alert")) { + __set_bit(LTC4282_ALERT, &st->valid_mask); + has_gpio = true; + } + + if (!has_gpio) + return 0; + + st->gc.parent = dev; + st->gc.base = -1; + st->gc.ngpio = LTC4282_GPIO_MAX; + st->gc.can_sleep = true; + st->gc.label = "ltc4282"; + st->gc.direction_input = ltc4282_gpio_input_set; + st->gc.direction_output = ltc4282_gpio_output_set; + st->gc.set = ltc4282_gpio_set; + st->gc.get = ltc4282_gpio_get; + st->gc.init_valid_mask = ltc4282_gpio_valid_mask; + + return devm_gpiochip_add_data(dev, &st->gc, st); +} + +static const char * const ltc4282_dividers[] = { + "external", "vdd_5_percent", "vdd_10_percent", "vdd_15_percent" +}; + +/* This maps the Vout full scale for the given Vin mode */ +static const u16 ltc4282_vfs_milli[] = { 5540, 8320, 16640, 33280 }; + +static int ltc4282_setup(struct ltc4282_state *st, struct device *dev) +{ + u32 val, curr_lim_uv; + const char *divider; + int ret; + + /* The part has an eeprom so let's get the needed defaults from it */ + ret = ltc4282_get_defaults(st, &curr_lim_uv); + if (ret) + return ret; + + ret = device_property_read_u32(dev, "adi,rsense-nano-ohms", + &st->rsense); + if (ret) + return dev_err_probe(dev, ret, + "Failed to read adi,rsense-nano-ohms\n"); + if (st->rsense < CENTI) + return dev_err_probe(dev, -EINVAL, + "adi,rsense-nano-ohms too small (< %lu)\n", + CENTI); + + /* + * The resolution for rsense is tenths of micro (eg: 62.5 uOhm) which + * means we need nano in the bindings. However, to make things easier to + * handle (with respect to overflows) we divide it by 100 as we don't + * really need the last two digits. + */ + st->rsense /= CENTI; + + ret = device_property_read_u32(dev, "adi,vin-mode-microvolt", &val); + if (!ret) { + switch (val) { + case 3300000: + st->vin_mode = LTC4282_VIN_3_3V; + break; + case 5000000: + st->vin_mode = LTC4282_VIN_5V; + break; + case 12000000: + st->vin_mode = LTC4282_VIN_12V; + break; + case 24000000: + st->vin_mode = LTC4282_VIN_24V; + break; + default: + return dev_err_probe(dev, -EINVAL, + "Invalid val(%u) for vin-mode-microvolt\n", + val); + } + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, + LTC4282_CTRL_VIN_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_VIN_MODE_MASK, st->vin_mode)); + if (ret) + return ret; + + /* Foldback mode should also be set to the input voltage */ + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, + LTC4282_FOLDBACK_MODE_MASK, + FIELD_PREP(LTC4282_FOLDBACK_MODE_MASK, st->vin_mode)); + if (ret) + return ret; + } + + st->vfs_out = ltc4282_vfs_milli[st->vin_mode]; + + ret = device_property_read_u32(dev, "adi,current-limit-sense-microvolt", + &curr_lim_uv); + if (!ret) { + int reg_val; + + switch (val) { + case 12500: + reg_val = 0; + break; + case 15625: + reg_val = 1; + break; + case 18750: + reg_val = 2; + break; + case 21875: + reg_val = 3; + break; + case 25000: + reg_val = 4; + break; + case 28125: + reg_val = 5; + break; + case 31250: + reg_val = 6; + break; + case 34375: + reg_val = 7; + break; + default: + return dev_err_probe(dev, -EINVAL, + "Invalid val(%u) for adi,current-limit-microvolt\n", + val); + } + + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, + LTC4282_ILIM_ADJUST_MASK, + FIELD_PREP(LTC4282_ILIM_ADJUST_MASK, reg_val)); + if (ret) + return ret; + } + + ret = ltc4282_set_max_limits(st, curr_lim_uv); + if (ret) + return ret; + + ret = device_property_read_string(dev, "adi,overvoltage-dividers", + ÷r); + if (!ret) { + int div = match_string(ltc4282_dividers, + ARRAY_SIZE(ltc4282_dividers), divider); + if (div < 0) + return dev_err_probe(dev, -EINVAL, + "Invalid val(%s) for adi,overvoltage-divider\n", + divider); + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, + LTC4282_CTRL_OV_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_OV_MODE_MASK, div)); + } + + ret = device_property_read_string(dev, "adi,undervoltage-dividers", + ÷r); + if (!ret) { + int div = match_string(ltc4282_dividers, + ARRAY_SIZE(ltc4282_dividers), divider); + if (div < 0) + return dev_err_probe(dev, -EINVAL, + "Invalid val(%s) for adi,undervoltage-divider\n", + divider); + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, + LTC4282_CTRL_UV_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_UV_MODE_MASK, div)); + } + + if (device_property_read_bool(dev, "adi,overcurrent-retry")) { + ret = regmap_set_bits(st->map, LTC4282_CTRL_LSB, + LTC4282_CTRL_OC_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,overvoltage-retry-disable")) { + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, + LTC4282_CTRL_OV_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,undervoltage-retry-disable")) { + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, + LTC4282_CTRL_UV_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,fault-log-enable")) { + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, + LTC4282_FAULT_LOG_EN_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,fault-log-enable")) { + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_FAULT_LOG_EN_MASK); + if (ret) + return ret; + } + + ret = device_property_read_u32(dev, "adi,fet-bad-timeout-ms", &val); + if (!ret) { + if (val > LTC4282_FET_BAD_MAX_TIMEOUT) + return dev_err_probe(dev, -EINVAL, + "Invalid value(%u) for adi,fet-bad-timeout-ms", + val); + + ret = regmap_write(st->map, LTC4282_FET_BAD_FAULT_TIMEOUT, val); + if (ret) + return ret; + } + + return ltc4282_gpio_setup(st, dev); +} + +static bool ltc4282_readable_reg(struct device *dev, unsigned int reg) +{ + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) + return false; + + return true; +} + +static bool ltc4282_writable_reg(struct device *dev, unsigned int reg) +{ + if (reg == LTC4282_STATUS_LSB || reg == LTC4282_STATUS_MSB) + return false; + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) + return false; + + return true; +} + +static const struct regmap_config ltc4282_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = LTC4282_RESERVED_3, + .readable_reg = ltc4282_readable_reg, + .writeable_reg = ltc4282_writable_reg, +}; + +static const struct hwmon_channel_info * const ltc4282_info[] = { + HWMON_CHANNEL_INFO(in, + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | + HWMON_I_MAX_ALARM | HWMON_I_ENABLE | + HWMON_I_RESET_HISTORY | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | + HWMON_I_MAX_ALARM | HWMON_I_LCRIT_ALARM | + HWMON_I_CRIT_ALARM | HWMON_I_ENABLE | + HWMON_I_RESET_HISTORY | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | + HWMON_I_MAX_ALARM | HWMON_I_LABEL), + HWMON_CHANNEL_INFO(curr, + HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST | + HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM | + HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM | + HWMON_I_RESET_HISTORY | HWMON_C_LABEL), + HWMON_CHANNEL_INFO(power, + HWMON_P_INPUT | HWMON_P_INPUT_LOWEST | + HWMON_P_INPUT_HIGHEST | HWMON_P_MAX | HWMON_P_MIN | + HWMON_P_MAX_ALARM | HWMON_P_MIN_ALARM | + HWMON_I_RESET_HISTORY | HWMON_P_LABEL), + HWMON_CHANNEL_INFO(energy, + HWMON_E_ENABLE), + NULL +}; + +static const struct hwmon_ops ltc4282_hwmon_ops = { + .read = ltc4282_read, + .write = ltc4282_write, + .is_visible = ltc4282_is_visible, + .read_string = ltc4282_read_labels, +}; + +static const struct hwmon_chip_info ltc2947_chip_info = { + .ops = <c4282_hwmon_ops, + .info = ltc4282_info, +}; + +/* energy attributes are 6bytes wide so we need u64 */ +static SENSOR_DEVICE_ATTR_RO(energy1_input, ltc4282_energy, 0); +/* power1_fault */ +static SENSOR_DEVICE_ATTR_RO(power1_good, ltc4282_status, + LTC4282_POWER_GOOD_MASK); +/* FET faults */ +static SENSOR_DEVICE_ATTR_RO(fet_short_fault, ltc4282_status, + LTC4282_FET_SHORT_MASK); +static SENSOR_DEVICE_ATTR_RO(fet_bad_fault, ltc4282_status, + LTC4282_FET_BAD_STATUS_MASK); +/* + * Fault log failures. These faults might be important in systems where + * auto-retry is not enabled since they will cause the part to latch off until + * they are cleared. Typically that happens when the system admin is close + * enough so he can check what happened and manually clear the faults. Hence, we + * provide an attribute to clear all fauls at once while still capable of + * checking individual faults in debugfs. + */ +static SENSOR_DEVICE_ATTR_WO(fault_logs_reset, ltc4282_clear_faults, 0); + +static struct attribute *ltc4282_attrs[] = { + &sensor_dev_attr_energy1_input.dev_attr.attr, + &sensor_dev_attr_power1_good.dev_attr.attr, + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, + &sensor_dev_attr_fet_short_fault.dev_attr.attr, + &sensor_dev_attr_fault_logs_reset.dev_attr.attr, + NULL +}; +ATTRIBUTE_GROUPS(ltc4282); + +#define LTC4282_DEBUGFS_FAULT_LOG(_name, _mask) \ +static int ltc4282_show_##_name##_fault_log(void *arg, u64 *val) \ +{ \ + struct ltc4282_state *st = arg; \ + long alarm; \ + int ret; \ + \ + ret = ltc4282_read_alarm(st, LTC4282_FAULT_LOG, _mask, &alarm); \ + if (ret) \ + return ret; \ + \ + *val = alarm; \ + \ + return 0; \ +} \ +DEFINE_DEBUGFS_ATTRIBUTE(ltc4282_##_name##_fault_log, \ + ltc4282_show_##_name##_fault_log, NULL, "%llu\n") + +LTC4282_DEBUGFS_FAULT_LOG(curr1_crit, LTC4282_OC_FAULT_MASK); +LTC4282_DEBUGFS_FAULT_LOG(in1_lcrit, LTC4282_UV_FAULT_MASK); +LTC4282_DEBUGFS_FAULT_LOG(in1_crit, LTC4282_OV_FAULT_MASK); +LTC4282_DEBUGFS_FAULT_LOG(fet_bad, LTC4282_FET_BAD_FAULT_MASK); +LTC4282_DEBUGFS_FAULT_LOG(fet_short, LTC4282_FET_SHORT_FAULT_MASK); +LTC4282_DEBUGFS_FAULT_LOG(power1, LTC4282_POWER_BAD_FAULT_MASK); + +static void ltc4282_debugfs_remove(void *dir) +{ + debugfs_remove_recursive(dir); +} + +static void ltc4282_debugfs_init(struct ltc4282_state *st, + struct i2c_client *i2c, + const struct device *hwmon) +{ + const char *debugfs_name; + struct dentry *dentry; + int ret; + + if (!IS_ENABLED(CONFIG_DEBUG_FS)) + return; + + debugfs_name = devm_kasprintf(&i2c->dev, GFP_KERNEL, "%s-%s", + i2c->name, dev_name(hwmon)); + if (!debugfs_name) + return; + + dentry = debugfs_create_dir(debugfs_name, NULL); + if (IS_ERR(dentry)) + return; + + ret = devm_add_action_or_reset(&i2c->dev, ltc4282_debugfs_remove, + dentry); + if (ret) + return; + + debugfs_create_file_unsafe("power1_fault_log", 0400, dentry, st, + <c4282_power1_fault_log); + debugfs_create_file_unsafe("fet_short_fault_log", 0400, dentry, st, + <c4282_fet_short_fault_log); + debugfs_create_file_unsafe("fet_bad_fault_log", 0400, dentry, st, + <c4282_fet_bad_fault_log); + debugfs_create_file_unsafe("in1_crit_fault_log", 0400, dentry, st, + <c4282_in1_crit_fault_log); + debugfs_create_file_unsafe("in1_lcrit_fault_log", 0400, dentry, st, + <c4282_in1_lcrit_fault_log); + debugfs_create_file_unsafe("curr1_crit_fault_log", 0400, dentry, st, + <c4282_curr1_crit_fault_log); +} + +static int ltc4282_probe(struct i2c_client *i2c) +{ + struct device *dev = &i2c->dev, *hwmon; + struct ltc4282_state *st; + int ret; + + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); + if (!st) + return dev_err_probe(dev, -ENOMEM, + "Failed to allocate memory\n"); + + st->map = devm_regmap_init_i2c(i2c, <c4282_regmap_config); + if (IS_ERR(st->map)) + return dev_err_probe(dev, PTR_ERR(st->map), + "failed regmap init\n"); + + /* Soft reset */ + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_RESET_MASK); + if (ret) + return ret; + + /* Yes, it's big but it is as specified in the datasheet */ + msleep(3200); + + ret = ltc428_clks_setup(st, dev); + if (ret) + return ret; + + ret = ltc4282_setup(st, dev); + if (ret) + return ret; + + mutex_init(&st->lock); + hwmon = devm_hwmon_device_register_with_info(dev, "ltc4282", st, + <c2947_chip_info, + ltc4282_groups); + if (IS_ERR(hwmon)) + return PTR_ERR(hwmon); + + ltc4282_debugfs_init(st, i2c, hwmon); + + return 0; +} + +static const struct of_device_id ltc4282_of_match[] = { + { .compatible = "adi,ltc4282" }, + {} +}; +MODULE_DEVICE_TABLE(of, ltc4282_of_match); + +static struct i2c_driver ltc4282_driver = { + .driver = { + .name = "ltc4282", + .of_match_table = ltc4282_of_match, + }, + .probe = ltc4282_probe, +}; +module_i2c_driver(ltc4282_driver); + +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); +MODULE_DESCRIPTION("LTC4282 I2C High Current Hot Swap Controller"); +MODULE_LICENSE("GPL");