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

From: Frank Rowand <frowand.list@gmail.com>
To: Sekhar Nori <nsekhar@ti.com>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Russell King <linux@armlinux.org.uk>
Cc: LKML <linux-kernel@vger.kernel.org>,
	arm-soc <linux-arm-kernel@lists.infradead.org>,
	linux-drm <dri-devel@lists.freedesktop.org>,
	linux-devicetree <devicetree@vger.kernel.org>,
	Jyri Sarha <jsarha@ti.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	David Airlie <airlied@linux.ie>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver
Date: Tue, 22 Nov 2016 10:21:44 -0800	[thread overview]
Message-ID: <58348CB8.7050304@gmail.com> (raw)
In-Reply-To: <800171a8-2e2c-2afb-f96d-800a17eaa17a@ti.com>

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
> 