linux-kernel - Re: [PATCH v3] pwm: Fix compilation error when CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120912094151.GA21786@avionic-0098.mockup.avionic-design.de>
Date:	Wed, 12 Sep 2012 11:41:51 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Tushar Behera <tushar.behera@...aro.org>
Cc:	linux-kernel@...r.kernel.org, sachin.kamat@...aro.org,
	patches@...aro.org
Subject: Re: [PATCH v3] pwm: Fix compilation error when CONFIG_PWM is not
 defined

On Wed, Sep 12, 2012 at 02:30:12PM +0530, Tushar Behera wrote:
> Add dummy implemention of public symbols for compilation-safe inclusion
> of include/linux/pwm.h file when CONFIG_PWM is not defined.
> 
> Reported-by: Sachin Kamat <sachin.kamat@...aro.org>
> Signed-off-by: Tushar Behera <tushar.behera@...aro.org>
> ---
> Changes since v2:
> * #if condition for legacy functions modified
> * Reverted layout changes, can be taken up when HAVE_PWM is no longer
> required.
> 
> Changes since v1:
> * Incorporated Thierry's suggestions regarding adding dummy function
> implemention for all global functions
> * Reorganized header file to have structure definitions first and then the
> function definitions.
> 
>  include/linux/pwm.h |   69 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 66 insertions(+), 3 deletions(-)

This is starting to look real good. Two more things I forgot to mention
on the last round. And one nitpick.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 21d076c..2c5daa9 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -6,6 +6,7 @@
>  struct pwm_device;
>  struct seq_file;
>  
> +#if IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM)
>  /*
>   * pwm_request - request a PWM device
>   */
> @@ -30,8 +31,29 @@ int pwm_enable(struct pwm_device *pwm);
>   * pwm_disable - stop a PWM output toggling
>   */
>  void pwm_disable(struct pwm_device *pwm);
> +#else
> +static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
> +{
> +	return NULL;
> +}
> +
> +static inline void pwm_free(struct pwm_device *pwm)
> +{}

These should also go on separate lines. I should have been more clear
about that. So:

	static inline void pwm_free(struct pwm_device *pwm)
	{
	}

There are a couple more of these below.

> +
> +static inline int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int pwm_enable(struct pwm_device *pwm)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void pwm_disable(struct pwm_device *pwm)
> +{}
> +#endif /* !(IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM)) */

I don't think this comment is necessary. Most editors allow you to jump
to the matching #if or #else. Mostly these comments just confuse me.

>  
> -#ifdef CONFIG_PWM
>  struct pwm_chip;
>  
>  enum {
> @@ -113,6 +135,7 @@ struct pwm_chip {
>  	unsigned int		of_pwm_n_cells;
>  };
>  
> +#if IS_ENABLED(CONFIG_PWM)
>  int pwm_set_chip_data(struct pwm_device *pwm, void *data);
>  void *pwm_get_chip_data(struct pwm_device *pwm);
>  
> @@ -124,6 +147,43 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
>  
>  struct pwm_device *pwm_get(struct device *dev, const char *consumer);
>  void pwm_put(struct pwm_device *pwm);
> +#else
> +static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void *pwm_get_chip_data(struct pwm_device *pwm)
> +{
> +	return NULL;
> +}
> +
> +static inline int pwmchip_add(struct pwm_chip *chip)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int pwmchip_remove(struct pwm_chip *chip)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
> +		unsigned int index,
> +		const char *label)
> +{
> +	return NULL;
> +}
> +
> +static inline struct pwm_device *pwm_get(struct device *dev,
> +		const char *consumer)
> +{
> +	return NULL;
> +}

Can you align the split parameter list on subsequent lines with the
first parameter, please? Like so:

static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
						       unsigned int index,
						       const char *label)

static inline struct pwm_device *pwm_get(struct device *dev,
					 const char *consumer)

Thierry

Content of type "application/pgp-signature" skipped