linux-kernel - Re: [PATCH v2 04/11] pwm: Add Renesas TPU PWM driver

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] [day] [month] [year] [list]

Message-ID: <6125284.6vELCtAL9N@avalon>
Date:	Thu, 13 Jun 2013 17:32:22 +0200
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
	linux-sh@...r.kernel.org, linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org, Magnus Damm <magnus.damm@...il.com>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH v2 04/11] pwm: Add Renesas TPU PWM driver

Hi Thierry,

On Wednesday 12 June 2013 12:06:49 Thierry Reding wrote:
> On Wed, May 29, 2013 at 05:48:50PM +0200, Laurent Pinchart wrote:
> > On Thursday 23 May 2013 23:45:17 Thierry Reding wrote:
> > > On Wed, Apr 24, 2013 at 10:50:09PM +0200, Laurent Pinchart wrote:
> [...]
> 
> > > > +struct tpu_device {
> > > > +	struct platform_device *pdev;
> > > > +	struct pwm_chip chip;
> > > > +	spinlock_t lock;
> > > > +
> > > > +	void __iomem *base;
> > > > +	struct clk *clk;
> > > > +
> > > > +	struct tpu_pwm_device pwms[TPU_CHANNEL_MAX];
> > > > +};
> > > 
> > > Can't you reuse the infrastructure built into the PWM subsystem? You can
> > > associate chip-specific data with each PWM device. You can look at the
> > > pwm-atmel-tcb and pwm-bfin drivers for usage examples. In a nutshell you
> > > hook the .request() function and setup the driver-specific structure and
> > > associate them with the PWM using pwm_set_chip_data().
> > > 
> > > This has the advantage that you don't need the pwms array in tpu_device
> > > and you also don't need TPU_CHANNEL_MAX because only the pwm_chip.npwm
> > > field needs to contain the number of channels.
> > 
> > I've actually thought about that, but decided not to do so. It looked
> > pretty weird to allocate PWM devices at .request() time, so I decided to
> > allocate the devices once only at probe time. Is it considered better to
> > allocate/free PWM devices every time they're requested/released ?
> 
> Well, I consider it better because it postpones memory allocation until
> it is actually used. Typically requesting a PWM device happens at probe
> time of other drivers so it isn't actually as bad as it may sound. Also
> allocating at request time allows you to easily associate the data with
> the PWM device using pwm_set_chip_data(), which was intended to be used
> for exactly this purpose.
> 
> Doing so will keep the driver-specific data in a well-defined location
> instead of putting it somewhere driver-specific.

OK, I'll try to fix that. For now I'll still need to expose TPU_CHANNEL_MAX to 
boards for polarity configuration in platform data. That will go away when all 
users will be converted to DT.

-- 
Regards,

Laurent Pinchart

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)