[PATCH v2 0/2] pwm: export pwm_get_state_hw()

[PATCH v2 0/2] pwm: export pwm_get_state_hw()

[David Lechner]

Calling this v2 since it is a revision of the first version of the patch
from [1]. I have split it into a separate series since the SPI offload
work will likely take longer to finalize and there are other potential
users of this, like the ad7606 driver that I have also included in this
series.

[1]: https://lore.kernel.org/linux-iio/20241023-dlech-mainline-spi-engine-offload-2-v4-1-f8125b99f5a1@baylibre.com/

---
David Lechner (2):
      pwm: core: export pwm_get_state_hw()
      iio: adc: ad7606: finish pwm_get_state_hw() TODO

 drivers/iio/adc/ad7606.c |  8 +++-----
 drivers/pwm/core.c       | 40 +++++++++++++++++++++++++++-------------
 include/linux/pwm.h      |  1 +
 3 files changed, 31 insertions(+), 18 deletions(-)
---
base-commit: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
change-id: 20241029-pwm-export-pwm_get_state_hw-8bdf15d9a408

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v2 1/2] pwm: core: export pwm_get_state_hw()

[David Lechner]

Export the pwm_get_state_hw() function. This is useful in cases where
we want to know what the hardware is actually doing, rather than what
what we requested it should do.

Locking had to be rearranged to ensure that the chip is still
operational before trying to access ops now that this can be called
from outside the pwm core.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v2 changes:
* Dropped __pwm_get_state_hw() function.
* Reworded commit message.
---
 drivers/pwm/core.c  | 40 +++++++++++++++++++++++++++-------------
 include/linux/pwm.h |  1 +
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4399e793efaf..ccbdd6dd1410 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -718,40 +718,54 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
 
-static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
+/**
+ * pwm_get_state_hw() - get the current PWM state from hardware
+ * @pwm: PWM device
+ * @state: state to fill with the current PWM state
+ *
+ * Similar to pwm_get_state() but reads the current PWM state from hardware
+ * instead of the requested state.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ * Context: May sleep.
+ */
+int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
 {
 	struct pwm_chip *chip = pwm->chip;
 	const struct pwm_ops *ops = chip->ops;
 	int ret = -EOPNOTSUPP;
 
+	might_sleep();
+
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
 	if (ops->read_waveform) {
 		char wfhw[WFHWSIZE];
 		struct pwm_waveform wf;
 
 		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
 
-		scoped_guard(pwmchip, chip) {
+		ret = __pwm_read_waveform(chip, pwm, &wfhw);
+		if (ret)
+			return ret;
 
-			ret = __pwm_read_waveform(chip, pwm, &wfhw);
-			if (ret)
-				return ret;
-
-			ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
-			if (ret)
-				return ret;
-		}
+		ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
+		if (ret)
+			return ret;
 
 		pwm_wf2state(&wf, state);
 
 	} else if (ops->get_state) {
-		scoped_guard(pwmchip, chip)
-			ret = ops->get_state(chip, pwm, state);
-
+		ret = ops->get_state(chip, pwm, state);
 		trace_pwm_get(pwm, state, ret);
 	}
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pwm_get_state_hw);
 
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f1cb1e5b0a36..5bcbcf2911c3 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -370,6 +370,7 @@ int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf
 int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
 int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
+int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**

-- 
2.43.0

Re: [PATCH v2 1/2] pwm: core: export pwm_get_state_hw()

[Uwe Kleine-König]

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

Hello David,

On Tue, Oct 29, 2024 at 04:18:49PM -0500, David Lechner wrote:
> Export the pwm_get_state_hw() function. This is useful in cases where
> we want to know what the hardware is actually doing, rather than what
> what we requested it should do.
> 
> Locking had to be rearranged to ensure that the chip is still
> operational before trying to access ops now that this can be called
> from outside the pwm core.

Good point, I didn't notice that in my review of v1.
 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v2 changes:
> * Dropped __pwm_get_state_hw() function.
> * Reworded commit message.
> ---
>  drivers/pwm/core.c  | 40 +++++++++++++++++++++++++++-------------
>  include/linux/pwm.h |  1 +
>  2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4399e793efaf..ccbdd6dd1410 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -718,40 +718,54 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
>  }
>  EXPORT_SYMBOL_GPL(pwm_apply_atomic);
>  
> -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> +/**
> + * pwm_get_state_hw() - get the current PWM state from hardware
> + * @pwm: PWM device
> + * @state: state to fill with the current PWM state
> + *
> + * Similar to pwm_get_state() but reads the current PWM state from hardware
> + * instead of the requested state.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + * Context: May sleep.
> + */
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  {
>  	struct pwm_chip *chip = pwm->chip;
>  	const struct pwm_ops *ops = chip->ops;
>  	int ret = -EOPNOTSUPP;
>  
> +	might_sleep();

Maybe this should be better

	if (!chip->atomic)
		might_sleep();

but I'm open to keep it unconditional until someone wails about it.

> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +

Huh, that means that __pwm_read_waveform() et al were called before
without holding the chip lock. How embarrassing. I think nothing bad
happens (because at this stage the PWM wasn't exposed to its consumer
yet and so no concurrency could happen), but still.

>  	if (ops->read_waveform) {
>  		char wfhw[WFHWSIZE];
>  		struct pwm_waveform wf;
>  
>  		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
>  
> -		scoped_guard(pwmchip, chip) {
> +		ret = __pwm_read_waveform(chip, pwm, &wfhw);
> +		if (ret)
> +			return ret;
>  
> -			ret = __pwm_read_waveform(chip, pwm, &wfhw);
> -			if (ret)
> -				return ret;
> -
> -			ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> +		if (ret)
> +			return ret;
>  
>  		pwm_wf2state(&wf, state);
>  
>  	} else if (ops->get_state) {
> -		scoped_guard(pwmchip, chip)
> -			ret = ops->get_state(chip, pwm, state);
> -
> +		ret = ops->get_state(chip, pwm, state);
>  		trace_pwm_get(pwm, state, ret);
>  	}
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(pwm_get_state_hw);
>  
>  /**
>   * pwm_adjust_config() - adjust the current PWM config to the PWM arguments

I applied that patch to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[David Lechner]

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.0

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[Uwe Kleine-König]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[Jonathan Cameron]

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

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[kernel test robot]

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

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[Uwe Kleine-König]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[Uwe Kleine-König]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[David Lechner]

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!

Re: [PATCH v2 2/2] iio: adc: ad7606: finish pwm_get_state_hw() TODO

[kernel test robot]

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-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