lkml.org 
[lkml]   [2016]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver
    From
    Date
    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

    \
     
     \ /
      Last update: 2016-11-22 07:27    [W:3.465 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site