Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

From	Sekhar Nori <nsekhar@ti.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver
Date	2016-11-22 07:30 +0100
Message-ID	<sGcKC-3ym-11@gated-at.bofh.it> (permalink)
References	<sym4p-3HL-9@gated-at.bofh.it> <sym4p-3HL-7@gated-at.bofh.it> <sFZNn-3C2-7@gated-at.bofh.it> <sG8nD-xC-5@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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?

Thanks,
Sekhar

Thread

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Sekhar Nori <nsekhar@ti.com> - 2016-11-21 17:40 +0100
  Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-11-21 17:50 +0100
    Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Robin Murphy <robin.murphy@arm.com> - 2016-11-21 18:50 +0100
      Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Sudeep Holla <sudeep.holla@arm.com> - 2016-11-21 19:00 +0100
  Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Frank Rowand <frowand.list@gmail.com> - 2016-11-22 02:50 +0100
    Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Sekhar Nori <nsekhar@ti.com> - 2016-11-22 07:30 +0100
      Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Frank Rowand <frowand.list@gmail.com> - 2016-11-22 19:30 +0100
        Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Sekhar Nori <nsekhar@ti.com> - 2016-11-23 07:00 +0100
          Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Frank Rowand <frowand.list@gmail.com> - 2016-11-23 19:20 +0100
            Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Frank Rowand <frowand.list@gmail.com> - 2016-11-23 19:30 +0100
    Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver Sudeep Holla <sudeep.holla@arm.com> - 2016-11-23 11:20 +0100

csiph-web