On 11/21/16 22:25, Sekhar Nori wrote:
> Hi Frank,
>
> On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote:
>> On 11/21/16 08:33, Sekhar Nori wrote:
>>> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>>>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>>> +{
>>>> + const struct da8xx_ddrctl_config_knob *knob;
>>>> + const struct da8xx_ddrctl_setting *setting;
>>>> + struct device_node *node;
>>>> + struct resource *res;
>>>> + void __iomem *ddrctl;
>>>> + struct device *dev;
>>>> + u32 reg;
>>>> +
>>>> + dev = &pdev->dev;
>>>> + node = dev->of_node;
>>>> +
>>>> + setting = da8xx_ddrctl_get_board_settings();
>>>> + if (!setting) {
>>>> + dev_err(dev, "no settings for board '%s'\n",
>>>> + of_flat_dt_get_machine_name());
>>>> + return -EINVAL;
>>>> + }
>>>
>>> This causes a section mismatch because of_flat_dt_get_machine_name()
>>> has an __init annotation. I did not notice that before, sorry.
>>>
>>> It can be fixed with a patch like below:
>>>
>>> ---8<---
>>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>>> --- a/drivers/memory/da8xx-ddrctl.c
>>> +++ b/drivers/memory/da8xx-ddrctl.c
>>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting
>>> *da8xx_ddrctl_get_board_settings(void)
>>> return NULL;
>>> }
>>>
>>> +static const char* da8xx_ddrctl_get_machine_name(void)
>>> +{
>>> + const char *str;
>>> + int ret;
>>> +
>>> + ret = of_property_read_string(of_root, "model", &str);
>>> + if (ret)
>>> + ret = of_property_read_string(of_root, "compatible", &str);
>>> +
>>> + return str;
>>> +}
>>> +
>>> static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> {
>>> const struct da8xx_ddrctl_config_knob *knob;
>>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device
>>> *pdev)
>>> setting = da8xx_ddrctl_get_board_settings();
>>> if (!setting) {
>>> dev_err(dev, "no settings for board '%s'\n",
>>> - of_flat_dt_get_machine_name());
>>
>> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
>> property in the root node. The "model" property in the root node has
>> nothing to do with the failure to match. So creating and then using
>> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
>>
>> It should be sufficient to simply report that no compatible matched.
>
> I agree with you on this. Even if model name is printed, you will have
> to go back and check the compatible anyway. But I think it will be
> useful to print the compatible instead of just reporting that nothing
> matched.
>
> Bartosz, if you agree too, could you send a fix patch just printing the
> compatible?
Please note that the compatible property might contain several strings, not just
a single string.
>
> Thanks,
> Sekhar
>
