Hi,
I have a few comments on the devicetree binding and the way it's parsed.
I note many are similar to those I commented on for the mv_udc and ehci-mv
devicetree code.
On Wed, Feb 06, 2013 at 07:23:36AM +0000, Chao Xie wrote:
> The PHY is seperated from usb controller.
> The usb controller used in marvell pxa168/pxa910/mmp2 are same,
> but PHY initialization may be different.
> the usb controller can support udc/otg/ehci, and for each of
> the mode, it need PHY to initialized before use the controller.
> Direclty writing the phy driver will make the usb controller
> driver to be simple and portable.
> The PHY driver will be used by marvell udc/otg/ehci.
>
> Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
> ---
> drivers/usb/phy/Kconfig | 7 +
> drivers/usb/phy/Makefile | 1 +
> drivers/usb/phy/mv_usb2_phy.c | 454 ++++++++++++++++++++++++++++++++++
> include/linux/platform_data/mv_usb.h | 9 +-
> include/linux/usb/mv_usb2.h | 43 ++++
> 5 files changed, 511 insertions(+), 3 deletions(-)
> create mode 100644 drivers/usb/phy/mv_usb2_phy.c
> create mode 100644 include/linux/usb/mv_usb2.h
[...]
> +static struct of_device_id usb_phy_dt_ids[] = {
> + { .compatible = "mrvl,pxa168-usb-phy", .data = (void *)PXA168_USB},
> + { .compatible = "mrvl,pxa910-usb-phy", .data = (void *)PXA910_USB},
> + { .compatible = "mrvl,mmp2-usb-phy", .data = (void *)MMP2_USB},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, usb_phy_dt_ids);
The binding (including these compatible string) needs to be documented.
> +
> +static int usb_phy_parse_dt(struct platform_device *pdev,
> + struct mv_usb2_phy *mv_phy)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + const struct of_device_id *of_id =
> + of_match_device(usb_phy_dt_ids, &pdev->dev);
> + unsigned int clks_num;
> + int i, ret;
> + const char *clk_name;
> +
> + if (!np)
> + return 1;
An actual error code please.
-ENODEV? -EINVAL?
> +
> + clks_num = of_property_count_strings(np, "clocks");
> + if (clks_num < 0) {
> + dev_err(&pdev->dev, "failed to get clock number\n");
> + return clks_num;
> + }
The common clock bindings use "clocks" as a list of phandle and clock-specifier
pairs. It seems bad to reuse that name in a different sense for this binding.
I'd recommend you use the common clock binding. There doesn't seem to be an
of_clk_count, but it should be a relatively simple addition, and it'd make this
code clearer and more consistent with other drivers.
See Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> + mv_phy->clks = devm_kzalloc(&pdev->dev,
> + sizeof(struct clk *) * clks_num, GFP_KERNEL);
> + if (mv_phy->clks == NULL) {
> + dev_err(&pdev->dev,
> + "failed to allocate mempory for clocks");
s/mempory/memory/
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < clks_num; i++) {
> + ret = of_property_read_string_index(np, "clocks", i,
> + &clk_name);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to read clocks\n");
> + return ret;
> + }
> + mv_phy->clks[i] = devm_clk_get(&pdev->dev, clk_name);
> + if (IS_ERR(mv_phy->clks[i])) {
> + dev_err(&pdev->dev, "failed to get clock %s\n",
> + clk_name);
> + return PTR_ERR(mv_phy->clks[i]);
> + }
> + }
> +
> + mv_phy->clks_num = clks_num;
> + mv_phy->type = (enum mv_usb2_phy_type)(of_id->data);
> +
> + return 0;
> +}
There's probably a need for something like devm_of_clk_get to make it easier to
write of-backed drivers.
> +
> +static int usb_phy_probe(struct platform_device *pdev)
> +{
> + struct mv_usb2_phy *mv_phy;
> + struct resource *r;
> + int ret, i;
> +
> + mv_phy = devm_kzalloc(&pdev->dev, sizeof(*mv_phy), GFP_KERNEL);
> + if (mv_phy == NULL) {
> + dev_err(&pdev->dev, "failed to allocate memory\n");
> + return -ENOMEM;
> + }
> + mutex_init(&mv_phy->phy_lock);
> +
> + ret = usb_phy_parse_dt(pdev, mv_phy);
> + /* no CONFIG_OF */
> + if (ret > 0) {
Reorganise this so you test for pdev->dev.of_node, and based of that you either
call usb_phy_parse_dt or do this stuff in the block below. That way you don't need
the positive return code from usb_phy_parse_dt.
> + struct mv_usb_phy_platform_data *pdata
> + = pdev->dev.platform_data;
> + const struct platform_device_id *id
> + = platform_get_device_id(pdev);
> +
> + if (pdata == NULL || id == NULL) {
> + dev_err(&pdev->dev,
> + "missing platform_data or id_entry\n");
> + return -ENODEV;
> + }
> + mv_phy->type = (unsigned int)(id->driver_data);
> + mv_phy->clks_num = pdata->clknum;
> + mv_phy->clks = devm_kzalloc(&pdev->dev,
> + sizeof(struct clk *) * mv_phy->clks_num, GFP_KERNEL);
> + if (mv_phy->clks == NULL) {
> + dev_err(&pdev->dev,
> + "failed to allocate mempory for clocks");
s/mempory/memory/
> + return -ENOMEM;
> + }
> + for (i = 0; i < mv_phy->clks_num; i++)
> + mv_phy->clks[i] = devm_clk_get(&pdev->dev,
> + pdata->clkname[i]);
> + if (IS_ERR(mv_phy->clks[i])) {
> + dev_err(&pdev->dev, "failed to get clock %s\n",
> + pdata->clkname[i]);
> + return PTR_ERR(mv_phy->clks[i]);
> + }
> + } else if (ret < 0) {
> + dev_err(&pdev->dev, "error parse dt\n");
s/parse/parsing/
> + return ret;
> + }
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (r == NULL) {
> + dev_err(&pdev->dev, "no phy I/O memory resource defined\n");
> + return -ENODEV;
> + }
> + mv_phy->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> + if (mv_phy->base == NULL) {
> + dev_err(&pdev->dev, "error map register base\n");
s/map/mapping/
> + return -EBUSY;
> + }
> + platform_set_drvdata(pdev, mv_phy);
> + mv_phy->pdev = pdev;
> + mv_phy->init = usb_phy_init;
> + mv_phy->shutdown = usb_phy_shutdown;
> +
> + platform_set_drvdata(pdev, mv_phy);
> +
> + the_phy = mv_phy;
> +
> + dev_info(&pdev->dev, "mv usb2 phy initialized\n");
> +
> + return 0;
> +}
> +
> +static int usb_phy_remove(struct platform_device *pdev)
> +{
> + the_phy = NULL;
> +
> + return 0;
> +}
Is mv_usb2_phy large? Why does it need to be dynamically allocated / freed at
all given it's treated as a singleton?
[...]
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
