Replace the call to pwm_get_state() with a call to pwm_get_state_hw() in
the ad7606 driver. This allows reading the sampling_frequency attribute
to return the rate the hardware is actually running at rather than the
rate that was requested. These may differ when the hardware isn't
capable of running at exactly the requested frequency.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
I went ahead and made this patch since it is trivial, but it would be
nice to get a Tested-by from Guillaume to make sure it actually works
as expected.
---
drivers/iio/adc/ad7606.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 8b2046baaa3e..1581eb31b8f9 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -762,11 +762,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
*val = st->oversampling;
return IIO_VAL_INT;
case IIO_CHAN_INFO_SAMP_FREQ:
- /*
- * TODO: return the real frequency intead of the requested one once
- * pwm_get_state_hw comes upstream.
- */
- pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+ ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
+ if (ret < 0)
+ return ret;
*val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period);
return IIO_VAL_INT;
}
--
2.43.0Hi David,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6fb2fa9805c501d9ade047fc511961f3273cdcb5]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/pwm-core-export-pwm_get_state_hw/20241030-052134
base: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
patch link: https://lore.kernel.org/r/20241029-pwm-export-pwm_get_state_hw-v2-2-03ba063a3230%40baylibre.com
patch subject: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO
config: i386-buildonly-randconfig-004-20241102 (https://download.01.org/0day-ci/archive/20241102/202411020416.BsBZy5XU-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241102/202411020416.BsBZy5XU-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/202411020416.BsBZy5XU-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/iio/adc/ad7606.c:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2225:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/adc/ad7606.c:765:9: error: call to undeclared function 'pwm_get_state_hw'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
765 | ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
| ^
drivers/iio/adc/ad7606.c:765:9: note: did you mean 'pwm_get_state'?
include/linux/pwm.h:127:20: note: 'pwm_get_state' declared here
127 | static inline void pwm_get_state(const struct pwm_device *pwm,
| ^
1 warning and 1 error generated.
vim +/pwm_get_state_hw +765 drivers/iio/adc/ad7606.c
733
734 static int ad7606_read_raw(struct iio_dev *indio_dev,
735 struct iio_chan_spec const *chan,
736 int *val,
737 int *val2,
738 long m)
739 {
740 int ret, ch = 0;
741 struct ad7606_state *st = iio_priv(indio_dev);
742 struct ad7606_chan_scale *cs;
743 struct pwm_state cnvst_pwm_state;
744
745 switch (m) {
746 case IIO_CHAN_INFO_RAW:
747 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
748 ret = ad7606_scan_direct(indio_dev, chan->address, val);
749 if (ret < 0)
750 return ret;
751 return IIO_VAL_INT;
752 }
753 unreachable();
754 case IIO_CHAN_INFO_SCALE:
755 if (st->sw_mode_en)
756 ch = chan->address;
757 cs = &st->chan_scales[ch];
758 *val = cs->scale_avail[cs->range][0];
759 *val2 = cs->scale_avail[cs->range][1];
760 return IIO_VAL_INT_PLUS_MICRO;
761 case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
762 *val = st->oversampling;
763 return IIO_VAL_INT;
764 case IIO_CHAN_INFO_SAMP_FREQ:
> 765 ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
766 if (ret < 0)
767 return ret;
768 *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period);
769 return IIO_VAL_INT;
770 }
771 return -EINVAL;
772 }
773
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi David,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6fb2fa9805c501d9ade047fc511961f3273cdcb5]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/pwm-core-export-pwm_get_state_hw/20241030-052134
base: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
patch link: https://lore.kernel.org/r/20241029-pwm-export-pwm_get_state_hw-v2-2-03ba063a3230%40baylibre.com
patch subject: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO
config: i386-randconfig-141-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020101.5Hs6MkwQ-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/20241102/202411020101.5Hs6MkwQ-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/202411020101.5Hs6MkwQ-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/adc/ad7606.c: In function 'ad7606_read_raw':
>> drivers/iio/adc/ad7606.c:765:23: error: implicit declaration of function 'pwm_get_state_hw'; did you mean 'pwm_get_state'? [-Werror=implicit-function-declaration]
765 | ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
| ^~~~~~~~~~~~~~~~
| pwm_get_state
cc1: some warnings being treated as errors
vim +765 drivers/iio/adc/ad7606.c
733
734 static int ad7606_read_raw(struct iio_dev *indio_dev,
735 struct iio_chan_spec const *chan,
736 int *val,
737 int *val2,
738 long m)
739 {
740 int ret, ch = 0;
741 struct ad7606_state *st = iio_priv(indio_dev);
742 struct ad7606_chan_scale *cs;
743 struct pwm_state cnvst_pwm_state;
744
745 switch (m) {
746 case IIO_CHAN_INFO_RAW:
747 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
748 ret = ad7606_scan_direct(indio_dev, chan->address, val);
749 if (ret < 0)
750 return ret;
751 return IIO_VAL_INT;
752 }
753 unreachable();
754 case IIO_CHAN_INFO_SCALE:
755 if (st->sw_mode_en)
756 ch = chan->address;
757 cs = &st->chan_scales[ch];
758 *val = cs->scale_avail[cs->range][0];
759 *val2 = cs->scale_avail[cs->range][1];
760 return IIO_VAL_INT_PLUS_MICRO;
761 case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
762 *val = st->oversampling;
763 return IIO_VAL_INT;
764 case IIO_CHAN_INFO_SAMP_FREQ:
> 765 ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
766 if (ret < 0)
767 return ret;
768 *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period);
769 return IIO_VAL_INT;
770 }
771 return -EINVAL;
772 }
773
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hello David, On Sat, Nov 02, 2024 at 01:50:35AM +0800, kernel test robot wrote: > kernel test robot noticed the following build errors: > > [auto build test ERROR on 6fb2fa9805c501d9ade047fc511961f3273cdcb5] > > url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/pwm-core-export-pwm_get_state_hw/20241030-052134 > base: 6fb2fa9805c501d9ade047fc511961f3273cdcb5 > patch link: https://lore.kernel.org/r/20241029-pwm-export-pwm_get_state_hw-v2-2-03ba063a3230%40baylibre.com > patch subject: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO > config: i386-randconfig-141-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020101.5Hs6MkwQ-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/20241102/202411020101.5Hs6MkwQ-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/202411020101.5Hs6MkwQ-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > drivers/iio/adc/ad7606.c: In function 'ad7606_read_raw': > >> drivers/iio/adc/ad7606.c:765:23: error: implicit declaration of function 'pwm_get_state_hw'; did you mean 'pwm_get_state'? [-Werror=implicit-function-declaration] > 765 | ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > | ^~~~~~~~~~~~~~~~ > | pwm_get_state > cc1: some warnings being treated as errors The problem here is that there is no declaration (and implementation) of pwm_get_state_hw() with CONFIG_PWM=n. Does it make sense to enable the ad7606 driver without enabling PWM support? If yes, we should add a dummy implementation of pwm_get_state_hw(), if not, a depends on PWM should be introduced. Best regards Uwe
On Sun, Nov 03, 2024 at 03:00:14PM +0100, Uwe Kleine-König wrote: > Hello David, > > On Sat, Nov 02, 2024 at 01:50:35AM +0800, kernel test robot wrote: > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on 6fb2fa9805c501d9ade047fc511961f3273cdcb5] > > > > url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/pwm-core-export-pwm_get_state_hw/20241030-052134 > > base: 6fb2fa9805c501d9ade047fc511961f3273cdcb5 > > patch link: https://lore.kernel.org/r/20241029-pwm-export-pwm_get_state_hw-v2-2-03ba063a3230%40baylibre.com > > patch subject: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO > > config: i386-randconfig-141-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020101.5Hs6MkwQ-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/20241102/202411020101.5Hs6MkwQ-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/202411020101.5Hs6MkwQ-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > drivers/iio/adc/ad7606.c: In function 'ad7606_read_raw': > > >> drivers/iio/adc/ad7606.c:765:23: error: implicit declaration of function 'pwm_get_state_hw'; did you mean 'pwm_get_state'? [-Werror=implicit-function-declaration] > > 765 | ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > > | ^~~~~~~~~~~~~~~~ > > | pwm_get_state > > cc1: some warnings being treated as errors > > The problem here is that there is no declaration (and implementation) of > pwm_get_state_hw() with CONFIG_PWM=n. Does it make sense to enable the > ad7606 driver without enabling PWM support? If yes, we should add a > dummy implementation of pwm_get_state_hw(), if not, a depends on PWM > should be introduced. Looking at the driver, the PWM is optional. So I rewrote the commit from patch 1/2 in this series and added a dummy. Best regards Uwe
On 11/3/24 2:20 PM, Uwe Kleine-König wrote: > On Sun, Nov 03, 2024 at 03:00:14PM +0100, Uwe Kleine-König wrote: >> Hello David, >> >> On Sat, Nov 02, 2024 at 01:50:35AM +0800, kernel test robot wrote: >>> kernel test robot noticed the following build errors: >>> >>> [auto build test ERROR on 6fb2fa9805c501d9ade047fc511961f3273cdcb5] >>> >>> url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/pwm-core-export-pwm_get_state_hw/20241030-052134 >>> base: 6fb2fa9805c501d9ade047fc511961f3273cdcb5 >>> patch link: https://lore.kernel.org/r/20241029-pwm-export-pwm_get_state_hw-v2-2-03ba063a3230%40baylibre.com >>> patch subject: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO >>> config: i386-randconfig-141-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020101.5Hs6MkwQ-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/20241102/202411020101.5Hs6MkwQ-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/202411020101.5Hs6MkwQ-lkp@intel.com/ >>> >>> All errors (new ones prefixed by >>): >>> >>> drivers/iio/adc/ad7606.c: In function 'ad7606_read_raw': >>>>> drivers/iio/adc/ad7606.c:765:23: error: implicit declaration of function 'pwm_get_state_hw'; did you mean 'pwm_get_state'? [-Werror=implicit-function-declaration] >>> 765 | ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); >>> | ^~~~~~~~~~~~~~~~ >>> | pwm_get_state >>> cc1: some warnings being treated as errors >> >> The problem here is that there is no declaration (and implementation) of >> pwm_get_state_hw() with CONFIG_PWM=n. Does it make sense to enable the >> ad7606 driver without enabling PWM support? If yes, we should add a >> dummy implementation of pwm_get_state_hw(), if not, a depends on PWM >> should be introduced. > > Looking at the driver, the PWM is optional. So I rewrote the commit from > patch 1/2 in this series and added a dummy. > > Best regards > Uwe Thanks!
On Tue, Oct 29, 2024 at 04:18:50PM -0500, David Lechner wrote: > Replace the call to pwm_get_state() with a call to pwm_get_state_hw() in > the ad7606 driver. This allows reading the sampling_frequency attribute > to return the rate the hardware is actually running at rather than the > rate that was requested. These may differ when the hardware isn't > capable of running at exactly the requested frequency. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > I went ahead and made this patch since it is trivial, but it would be > nice to get a Tested-by from Guillaume to make sure it actually works > as expected. > --- > drivers/iio/adc/ad7606.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > index 8b2046baaa3e..1581eb31b8f9 100644 > --- a/drivers/iio/adc/ad7606.c > +++ b/drivers/iio/adc/ad7606.c > @@ -762,11 +762,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > *val = st->oversampling; > return IIO_VAL_INT; > case IIO_CHAN_INFO_SAMP_FREQ: > - /* > - * TODO: return the real frequency intead of the requested one once > - * pwm_get_state_hw comes upstream. > - */ > - pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state); > + ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > + if (ret < 0) > + return ret; > *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period); > return IIO_VAL_INT; > } Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> There is a slight inconsistency compared to ad7606_set_sampling_freq(): ad7606_set_sampling_freq uses cnvst_pwm_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq); . So if cnvst_pwm_state.period happens to be 3 ns then reading the freq value yields 333333333, but if you feed freq=333333333 into ad7606_set_sampling_freq() it sets period = 4. To fix that you'd better use a plain / here in ad7606_read_raw(). (Note that with using round-closest for both there are still corner cases, e.g. period = 31796 ns yields freq = 31450.496917851302 but setting freq = 31450 yields 31796.50238473768 and so 31797.) Best regards Uwe
On Wed, 30 Oct 2024 09:28:01 +0100 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Tue, Oct 29, 2024 at 04:18:50PM -0500, David Lechner wrote: > > Replace the call to pwm_get_state() with a call to pwm_get_state_hw() in > > the ad7606 driver. This allows reading the sampling_frequency attribute > > to return the rate the hardware is actually running at rather than the > > rate that was requested. These may differ when the hardware isn't > > capable of running at exactly the requested frequency. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > I went ahead and made this patch since it is trivial, but it would be > > nice to get a Tested-by from Guillaume to make sure it actually works > > as expected. > > --- > > drivers/iio/adc/ad7606.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > > index 8b2046baaa3e..1581eb31b8f9 100644 > > --- a/drivers/iio/adc/ad7606.c > > +++ b/drivers/iio/adc/ad7606.c > > @@ -762,11 +762,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > > *val = st->oversampling; > > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SAMP_FREQ: > > - /* > > - * TODO: return the real frequency intead of the requested one once > > - * pwm_get_state_hw comes upstream. > > - */ > > - pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state); > > + ret = pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > > + if (ret < 0) > > + return ret; > > *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period); > > return IIO_VAL_INT; > > } > > Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Probably not something we need to hurry so assuming it is fine, please send again after the merge window. > > There is a slight inconsistency compared to ad7606_set_sampling_freq(): > > ad7606_set_sampling_freq uses > > cnvst_pwm_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq); > > . So if cnvst_pwm_state.period happens to be 3 ns then reading > the freq value yields 333333333, but if you feed freq=333333333 into > ad7606_set_sampling_freq() it sets period = 4. > > To fix that you'd better use a plain / here in ad7606_read_raw(). > (Note that with using round-closest for both there are still corner > cases, e.g. period = 31796 ns yields freq = 31450.496917851302 but > setting freq = 31450 yields 31796.50238473768 and so 31797.) Ouch. > > Best regards > Uwe
© 2016 - 2024 Red Hat, Inc.