Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

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

* Stephen Warren wrote:
> On 03/20/2012 02:39 AM, Thierry Reding wrote:
> > * Stephen Warren wrote:
> >> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> >>> This commit adds very basic support for device tree probing. Currently,
> >>> only a PWM and a list of distinct brightness levels can be specified.
> >>> Enabling or disabling backlight power via GPIOs is not yet supported.
> >>>
> >>> A pointer to the exit() callback is stored in the driver data to keep it
> >>> around until the driver is unloaded.
> ...
> >>>  static int pwm_backlight_probe(struct platform_device *pdev)
> >> ...
> >>> -	pb->pwm = pwm_request(data->pwm_id, "backlight");
> >>> -	if (IS_ERR(pb->pwm)) {
> >>> -		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> >>> -		ret = PTR_ERR(pb->pwm);
> >>> -		goto err_alloc;
> >>> -	} else
> >>> -		dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >>> +	if (!pb->pwm) {
> >>> +		pb->pwm = pwm_request(data->pwm_id, "backlight");
> >>> +		if (IS_ERR(pb->pwm)) {
> >>> +			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> >>> +			ret = PTR_ERR(pb->pwm);
> >>> +			goto err_alloc;
> >>> +		} else
> >>> +			dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >>> +	}
> >>
> >> Hmmm. It'd be more consistent if pwm_backlight_parse_dt() called
> >> something like of_pwm_get() instead of of_pwm_request(), so that this
> >> code could always call pwm_request() on the PWM and hence operate the
> >> same irrespective of DT vs non-DT. GPIOs work that way at least.
> > 
> > That's actually what the initial patch had. Unfortunately that's pretty much
> > the opposite direction of where the PWM framework is headed because it would
> > involve getting a global index to request the PWM.
> 
> Not necessarily; get() could return a controller+index pair, which could
> then be passed to request().

This is pretty much what of_pwm_request() does internally (actually via the
of_xlate() function), and it goes one step further by requesting the
corresponding PWM.

I don't really like the fact that the DT parsing code needs to store a
requested PWM device in the platform data. The only other solution would be,
as you suggest, to obtain two indices and request based on them. However that
comes with a whole lot of problems of its own (race between getting the
controller index and the actual requesting of the PWM device, ...).

One other alternative could be to not pass the PWM device via platform data
but rather return it from the pwm_backlight_parse_dt() function (either via
the return value or an output parameter).

> > I think in the long run it
> > would be much better to get rid of pwm_request() altogether and unify by
> > having the non-DT case request the PWM device on a per-chip basis.
> 
> That might also work.

What I'm not sure about is how to represent this in the non-DT case. The
problem would be how to identify the individual PWM chips. From a quick
glance, pinctrl seems to do this purely on a name lookup basis, similar to
the clock framework.

What would be a good way to represent this in platform data?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]