On Wed, Apr 29, 2015 at 3:49 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Tuesday 28 April 2015 16:10:44 Loc Ho wrote:
>> +
>> + rc = platform_driver_register(&xgene_edac_mc_driver);
>> + if (rc) {
>> + edac_printk(KERN_ERR, EDAC_MOD_STR, "MCU fails to register\n");
>> + goto reg_mc_failed;
>> + }
>> + rc = platform_driver_register(&xgene_edac_pmd_driver);
>> + if (rc) {
>> + edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n");
>> + goto reg_pmd_failed;
>> + }
>> + rc = platform_driver_register(&xgene_edac_l3_driver);
>> + if (rc) {
>> + edac_printk(KERN_ERR, EDAC_MOD_STR, "L3 fails to register\n");
>> + goto reg_l3_failed;
>> + }
>> + rc = platform_driver_register(&xgene_edac_soc_driver);
>> + if (rc) {
>> + edac_printk(KERN_ERR, EDAC_MOD_STR, "SoC fails to register\n");
>> + goto reg_soc_failed;
>> + }
>>
>
> I had not looked at the driver before, but I have one comment now:
>
> I think this can be simplified to registering a single platform_driver
> that just matches all four compatible strings, with an appropriate data:
>
> +static struct of_device_id xgene_edac_of_match[] = {
> + { .compatible = "apm,xgene-edac-mcu", .data = &xgene_edac_mcu_data },
> + { .compatible = "apm,xgene-edac-pmd", .data = &xgene_edac_pmd_data },
> + { .compatible = "apm,xgene-edac-l3", .data = &xgene_edac_l3_data },
> + { .compatible = "apm,xgene-edac-soc", .data = &xgene_edac_soc_data },
> + {},
> +};
What exactly is shared here to combine all this into one driver? I
would argue these are all independent functions and should be separate
drivers. Add some library functions if there are common parts. The
bindings are still a bit broken IMO.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
