Mark Rutland <mark.rutland@xxxxxxx> writes:
> On Sun, Jun 22, 2014 at 10:04:57AM +0100, Robert Jarzmik wrote:
>> +/**
>> + * pxa_udc_probe_dt - device tree specific probe
>> + * @pdev: platform data
>> + * @udc: pxa_udc structure to fill
>> + *
>> + * Fills udc from platform data out of device tree.
>> + *
>> + * Returns 0 if DT found, 1 if DT not found, and <0 on error
>
> That's impossible as this function is marked as returning bool.
> Make this an int and return negative error codes.
Yes, it was designed to be an int. I don't know why this "bool" appeared
... lack of coffee probably.
>
>> + */
>> +bool pxa_udc_probe_dt(struct platform_device *pdev, struct pxa_udc *udc)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + const struct of_device_id *of_id =
>> + of_match_device(udc_pxa_dt_ids, &pdev->dev);
>> + int ret;
>> + u32 gpio_pullup;
>> +
>> + if (!np || !of_id)
>> + return 1;
>> + ret = of_alias_get_id(np, "udc");
>> + pdev->id = (ret >= 0) ? ret : -1;
>
> The alias name wasn't mentioned in the binding...
Ah, now I'm thinking of it, it doesn't serve any purpose ... I will remove this
piece of code and replace by "pdev->id = -1".
>
>> +
>> + if (!of_property_read_u32(np, "udc-pullup-gpio", &gpio_pullup))
>> + udc->gpio_pullup = gpio_pullup;
>
> This number is a Linux internal detail. Use the GPIO bindings.
Yes, as in documentation. Agreed.
>
>> + udc->gpio_pullup_inverted =
>> + of_property_read_bool(np, "udc-pullup-gpio-inverted");
>
> The GPIO bindings can describe this.
Yes.
>
>> @@ -2415,7 +2469,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
>> {
>> struct resource *regs;
>> struct pxa_udc *udc = &memory;
>> - int retval = 0, gpio;
>> + int retval = 0;
>> +
>> + retval = pxa_udc_probe_dt(pdev, udc);
>> + if (retval < 0)
>> + return retval;
>
> This case can never happen given the prototype of pxa_udc_probe_dt.
>
>> @@ -2509,6 +2574,7 @@ static int pxa_udc_remove(struct platform_device *_dev)
>>
>> udc->transceiver = NULL;
>> the_controller = NULL;
>> + clk_unprepare(udc->clk);
>> clk_put(udc->clk);
>
> I don't see why these clock changes should be in the same patch as the
> DT support. They might be a prerequisite, but as far as I can tell they
> are required even without DT probing.
Ah they are another posted patch. I think I missed a rebase somewhere, and it
slipped in. It is as you say not this patch purpose, and I thought I had posted
a previous patch with this ... I will split it again.
Thanks for the review.
Cheers.
--
Robert
--
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
