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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]