Re: [PATCH] pwm: pxa: add device tree support to pwm driver

[Mike Dunn <mikedunn@newsguy.com>]

On 09/04/2013 07:35 AM, Marek Vasut wrote:
> Dear Mike Dunn,
> 
>> On 09/03/2013 03:20 PM, Marek Vasut wrote:
>>
>> [...]
>>
>>>> +#ifdef CONFIG_OF
>>>> +/* use the platform_device id table for OF match table data */
>>>> +static struct of_device_id pwm_of_match[] = {
>>>> +	{ .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
>>>> +	{ .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
>>>> +	{ .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
>>>> +	{ .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
>>>
>>> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why
>>> not just stick with pxa25x-pwm only for all of the CPUs (aka. the lowest
>>> CPU model). Then the table would have but a single entry.
>>
>> I'm just echoing the existing platform_device_id table...
>>
>> static const struct platform_device_id pwm_id_table[] = {
>> 	/*   PWM    has_secondary_pwm? */
>> 	{ "pxa25x-pwm", 0 },
>> 	{ "pxa27x-pwm", HAS_SECONDARY_PWM },
>> 	{ "pxa168-pwm", 0 },
>> 	{ "pxa910-pwm", 0 },
>> 	{ },
>> };
>> MODULE_DEVICE_TABLE(platform, pwm_id_table);
>>
>> ... so that my changes to the driver are minimal.  Yes, apparently the only
>> difference is the existance of a "secondary" pwm for pxa27x.
>>
>> BTW, the pxa27x actually has four pwms, which is why the addition I made to
>> pxa27x.dtsi has two nodes (the driver handles two pwms for each device
>> instance in the pxa27x case).
>>
> 
> What's that "secondary PWM" there? I no longer remember, sorry. 


If pdev->id_entry->driver_data == HAS_SECONDARY_PWM, then pwm_chip->npwm=2 when
pwmchip_add() is called.  Otherwise pwm_chip->npwm=1.  The driver knows that the
second pwm's registers are at a fixed offset from the first.  For compatibility,
the pxa27x maps the registers for the third pwm at a distant offset, and makes
the offset between 3 and 4 the same as between 1 and 2.  Yes, the driver mkes
this unnecessarily complicated.  There should just be one device instance per
pwm, and dispense with the whole driver_data thing.  I guess there's some
history there.


> The question 
> remains still, we can have two entries there (pxa25x and pxa27x) ORR have one 
> entry (pxa25x) + mrvl,has-secondary-pwm entry.


It looks like defining "compatible" properties that mirror the old
platform_device_id names won't fly... wildcards are verboten (see Sergei's
comment).  So your inclination to use one value for the "compatible" property is
correct.  I think the way to go is to forget the whole HAS_SECONDARY_PWM in the
DT case, have one device instance per pwm, and use "compatible=marvell,pwm".
Other suggestions welcome.

Thanks,
Mike