Re: [PATCH] dt: pinctrl: Document device tree binding

[Stephen Warren]

On 03/14/2012 09:32 PM, Dong Aisheng wrote:
> On Wed, Mar 14, 2012 at 03:27:26AM +0800, Stephen Warren wrote:
>> On 03/12/2012 09:20 PM, Dong Aisheng wrote:
...
>>> I can't see why we still need a special compatible flag to tell driver.
>>> Just do not define pinctrl-* properties for that devices in device tree.
>>> Did i understand wrong?
>>
>> The compatible property would be the only way to implement anything like
>> is_soc_foo. However, it's a bad idea to overload the compatible property
>> in this way.
>
> I guess i might not describe my idea clearly.
> My idea is that the compatible string does that work.
> For example:
> static const struct of_device_id mxs_mmc_dt_ids[] = {
>         { .compatible = "fsl,imx23-mmc", .data = NULL, },
>         { .compatible = "fsl,imx28-mmc", .data = NULL, },
>         { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids);
>
> Replace NULL to some special data like SOC_MX23 or SOC_MX28 can let driver
> driver know which SoC it's running on.
> Then driver can use private macro like is_soc_foo.

OK, you could implement is_soc_foo based on the compatible flag.
However, it's best not to do that.

Instead of asking is_soc_foo(), you should really ask has_bug_xxx() or
has_feature_xxx(), and implement those based on either the compatible
flag, or perhaps other properties.

That way, when 10 of the 20 SoCs that contain the IP block all have the
same feature or need the same bugfix, the code that implements the
bugfix just calls a single has_bug_xxx() rather than having to sprinkle
many many is_soc_foo() calls through the code, which gets unmaintainable.

>>>> That also means that if you were the first user of an IP block in a
>>>> system which didn't need pin muxing for it, you'd have to modify the
>>>> kernel to support pinctrl being optional before you could use that device.
>>>
>>> Why need modify the kernel?
>>> Assuming above example.
>>> I'm a bit confused.
>>
>> If the driver contains code like:
>>
>> if (is_soc_a) {
>>     ...
>> } else if (is_soc_b) {
>>    ...
>> }
>> ...
>>
>> Then in order to support a new SoC, even if the driver doesn't need to
>> do anything different, you'd need to go and edit the code to add an "if
>> (is_soc_c)" condition into that list.
>
> No.
> No changes needed if the driver does not need to do anything different.
> Compatible string does that work

To make the driver support a new compatible property value, you have to
edit the driver to add it to the of_device_id table, and recompile it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/