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

[Thierry Reding]

On Wed, May 29, 2013 at 05:48:50PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> 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.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature