Re: Re[6]: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 19 February 2013 18:54, Alexander Shiyan <shc_work@xxxxxxx> wrote:
> ...
>> >> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>> >> >> >>  {
>> >> >> >>       struct device_node *syscon_np;
>> >> >> >>       struct regmap *regmap;
>> >> >> >> +     struct syscon *syscon;
>> >> >> >> +     struct device *dev;
>> >> >> >>
>> >> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> >> >> >> -     if (!syscon_np)
>> >> >> >> +     if (syscon_np) {
>> >> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> +             of_node_put(syscon_np);
>> >> >> >> +
>> >> >> >> +             return regmap;
>> >> >> >> +     }
>> >> >> >> +
>> >> >> >> +     /* Fallback to search by id_entry.name string */
>> >> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> >> >> >> +                              syscon_match_id);
>> >> >> >> +     if (!dev)
>> >> >> >>               return ERR_PTR(-ENODEV);
>> >> >> >>
>> >> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> -     of_node_put(syscon_np);
>> >> >> >> +     syscon = dev_get_drvdata(dev);
>> >> >> >>
>> >> >> >> -     return regmap;
>> >> >> >> +     return syscon->regmap;
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>> >> >> >
>> >> >> > Since you are not actually comparing the "compatible" property here,
>> >> >> > I would suggest adding another function here,
>> >> >>
>> >> >> Yes, i also think like that.
>> >> >
>> >> > In this case we should provide two paths for drivers which can work as with DT
>> >> > and without DT.
>> >>
>> >> Yes.
>> >
>> > I still think the universal procedure is better for the driver.
>> >
>>
>> Why?
>> I did not see your reply on my other comments on the problems of using universal
>> procedure?
>> Please let me know if you think they're not issues.
>
> Yes, I do not see a problem here.
> I will try to show the code.
>
> In the driver:
> struct regmap *r;
> r = syscon_regmap_lookup_by_compatible("my_super_syscon");
>
> For DT case:
> syscon@123456 {
>   compatible = "my_super_syscon", "syscon";
>   ...
> };
>
> For non-DT case:
> struct platform_device_id id = { .name = "my_super_syscon", };
> struct platform_device syscon_pdev = {
>   .name = "syscon",
>   .id_entry = &id,

This is really strange to me and i've never seen such using.
Usually the id_table is provided by the driver and the match process then will
set the correct id_entry for the platform device once it matches.
Please see the platform_bus match process: drivers/base/platform.c

>   ...
> };
> platform_device_register(&syscon_pdev);
>
> Do I understand what you mean?
>

My understanding for non-dt case is something like:
In client driver:
struct regmap *r;
r = syscon_regmap_lookup_by_pdevname("my_super_syscon");

In board code:
struct platform_device syscon_pdev = {
.name = "my_super_syscon",
...
};
 platform_device_register(&syscon_pdev);

In syscon driver:
static struct platform_device_id syscon_device_ids[] = {
        {
                .name = "my_super_syscon",
        }, {
                /* sentinel */
        }
};
MODULE_DEVICE_TABLE(platform, syscon_device_ids);

static struct platform_driver syscon_driver = {
        .driver = {
                .name = "syscon",
                .owner = THIS_MODULE,
                .of_match_table = of_syscon_match,
        },
        .id_table       = syscon_device_ids,
        .probe          = syscon_probe,
        .remove         = syscon_remove,
};

One problem is that every user needs to add their syscon compatible device
support(platform_device_id) in syscon driver first before they can use it.
But it looks to me just like how other driver generally does.

Arnd,
Do you think this is an issue?

Regards
Dong Aisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/