Re: [PATCH v2 0/3] EDAC support for Calxeda Highbank

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Hi Rob,


Em 13-06-2012 11:57, Rob Herring escreveu:
> Mauro,
> 
> On 06/12/2012 08:24 AM, Mauro Carvalho Chehab wrote:
>> Hi Rob,
>>
>> Em 11-06-2012 23:32, Rob Herring escreveu:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> This series adds EDAC support for Calxeda Highbank platform L2 and
>>> memory ECC hardware.
>>>
>>> This version is rebased current edac next tree for 3.6. Changes in
>>> this version are the addition of a common edac debugfs directory and
>>> coverting the highbank error injection to use debugfs.
>>
>> Thanks for the patches.
>>
>> It looks OK on my eyes, with regards to the EDAC API usage. While this patch
>> touches at the arm/devicetree stuff, most of the changes belong to EDAC, so
>> I'll apply them with my SOB on my tree.
>>
> 
> I found a build error when EDAC_DEBUG is turned off.
> 
> drivers/edac/highbank_mc_edac.c: In function 'highbank_mc_probe':
> drivers/edac/highbank_mc_edac.c:215:9: error: 'struct mem_ctl_info' has no member named 'debugfs'
> drivers/edac/highbank_mc_edac.c:216:50: error: 'struct mem_ctl_info' has no member named 'debugfs'
> drivers/edac/highbank_mc_edac.c:218:10: error: 'highbank_mc_debug_inject_fops' undeclared (first use in this function)
> 
> 
> Do you prefer I respin the patch or do you want a follow on patch?

Both approaches work for me. As such patch fixes a compilation bug, fixing the existing
patch is preferred.

> This is what the fix looks like:
> 
> diff --git a/drivers/edac/highbank_mc_edac.c b/drivers/edac/highbank_mc_edac.c
> index d00dc0b..3185961 100644
> --- a/drivers/edac/highbank_mc_edac.c
> +++ b/drivers/edac/highbank_mc_edac.c
> @@ -124,6 +124,22 @@ static const struct file_operations highbank_mc_debug_inject_fops = {
>          .write = highbank_mc_err_inject_write,
>          .llseek = generic_file_llseek,
>   };
> +
> +static int __devinit highbank_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
> +{
> +       if (!mci->debugfs)
> +               return -ENODEV;
> +
> +       debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci,
> +                           &highbank_mc_debug_inject_fops);
> +
> +       return 0;
> +}
> +#else
> +static int __devinit highbank_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
> +{
> +       return -ENODEV;
> +}
>   #endif
>   
>   static int __devinit highbank_mc_probe(struct platform_device *pdev)
> @@ -212,10 +228,7 @@ static int __devinit highbank_mc_probe(struct platform_device *pdev)
>          if (res < 0)
>                  goto err;
>   
> -       if (mci->debugfs)
> -               debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs,
> -                                   mci,
> -                                   &highbank_mc_debug_inject_fops);
> +       highbank_mc_create_debugfs_nodes(mci);

I would just declare the function as void, as you're not using the returned argument.

>   
>          devres_close_group(&pdev->dev, NULL);
>          return 0;
> 
> 
> Rob
> 
>> Regards,
>> Mauro.
>>>
>>> Rob
>>>
>>> Rob Herring (3):
>>>     edac: create top-level debugfs directory
>>>     edac: add support for Calxeda highbank memory controller
>>>     edac: add support for Calxeda highbank L2 cache ecc
>>>
>>>    .../devicetree/bindings/arm/calxeda/l2ecc.txt      |   17 ++
>>>    .../devicetree/bindings/arm/calxeda/mem-ctrlr.txt  |   17 ++
>>>    arch/arm/boot/dts/highbank.dts                     |   12 +
>>>    drivers/edac/Kconfig                               |   16 +-
>>>    drivers/edac/Makefile                              |    4 +
>>>    drivers/edac/edac_mc_sysfs.c                       |   23 +-
>>>    drivers/edac/edac_module.c                         |    3 +
>>>    drivers/edac/edac_module.h                         |   14 ++
>>>    drivers/edac/highbank_l2_edac.c                    |  149 ++++++++++++
>>>    drivers/edac/highbank_mc_edac.c                    |  256 ++++++++++++++++++++
>>>    10 files changed, 509 insertions(+), 2 deletions(-)
>>>    create mode 100644 Documentation/devicetree/bindings/arm/calxeda/l2ecc.txt
>>>    create mode 100644 Documentation/devicetree/bindings/arm/calxeda/mem-ctrlr.txt
>>>    create mode 100644 drivers/edac/highbank_l2_edac.c
>>>    create mode 100644 drivers/edac/highbank_mc_edac.c
>>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>