linux-kernel - Re: [PATCH 1/2] PWM: add pwm framework support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <201106281427.04373.arnd@arndb.de>
Date:	Tue, 28 Jun 2011 14:27:04 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Sascha Hauer <s.hauer@...gutronix.de>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	viresh kumar <viresh.kumar@...com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Ryan Mallon <ryan@...ewatersys.com>
Subject: Re: [PATCH 1/2] PWM: add pwm framework support

On Tuesday 28 June 2011, Sascha Hauer wrote:
> This patch adds framework support for PWM (pulse width modulation) devices.
> 
> The is a barebone PWM API already in the kernel under include/linux/pwm.h,
> but it does not allow for multiple drivers as each of them implements the
> pwm_*() functions.

Hi Sascha,

This looks very nice.
I have only trivial comments, except for the suggestion to use idr.
 
> +PWM core
> +M:	Sascha Hauer <s.hauer@...gutronix.de>
> +L:	linux-kernel@...r.kernel.org
> +S:	Maintained

I would add

F:	drivers/pwm/

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> new file mode 100644
> index 0000000..93c1052
> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig PWM
> +	bool "PWM Support"
> +	help
> +	  This enables PWM support through the generic PWM framework.
> +	  You only need to enable this, if you also want to enable
> +	  one or more of the PWM drivers below.

Remove the comma.

> +
> +/*
> + * The next pwm id to assign. We do not bother to fill gaps which
> + * occur during dynamic registering/deregistering of pwms, but
> + * instead assign a uniq id to each new pwm.
                       unique

> + */
> +static int next_pwm_id;

How about using idr? It provides you a fast lookup and handles giving
out unique numbers.

> +
> +/**
> + * pwmchip_reserve() - reserve range of pwms to use with platform code only
> + * @npwms: number of pwms to reserve
> + * Context: platform init
> + *
> + * Maybe called only once. It reserves the first pwm_ids for platform use so

I assume you mean "May only be called once".

> +/**
> + * struct pwm_ops - PWM operations
> + * @request: optional hook for requesting a PWM
> + * @free: optional hook for freeing a PWM
> + * @config: configure duty cycles and period length for this PWM
> + * @enable: enable PWM output toggling
> + * @disable: disable PWM output toggling
> + */
> +struct pwm_ops {
> +	int			(*request)(struct pwm_chip *chip);
> +	void			(*free)(struct pwm_chip *chip);
> +	int			(*config)(struct pwm_chip *chip, int duty_ns,
> +						int period_ns);
> +	int			(*enable)(struct pwm_chip *chip);
> +	void			(*disable)(struct pwm_chip *chip);
> +};
>
> +/**
> + * struct pwm_chip - abstract a PWM
> + * @label: for diagnostics
> + * @owner: helps prevent removal of modules exporting active PWMs
> + * @ops: The callbacks for this PWM
> + */
> +struct pwm_chip {
> +	int			pwm_id;
> +	const char		*label;
> +	struct module		*owner;
> +	struct pwm_ops		*ops;
> +};

I think the "owner" field should be in the pwm_ops, not in the pwm_chip,
because the pwm_ops is likely to be statically allocated, while the
pwm_chip is probably runtime allocated and cannot be preinitialized.

Should a pwm_chip contain a "struct device" so you can refer to it in
sysfs and add attributes?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/