On Mon, Jan 7, 2013 at 1:11 PM, Sujit Reddy Thumma
<sthumma@xxxxxxxxxxxxxx> wrote:
> Hi Vinayak,
>
> I have few comments below:
>
>
>>>>> +#ifdef CONFIG_PM
>>>>> +/**
>>>>> + * ufshcd_pltfrm_suspend - suspend power management function
>>>>> + * @pdev: pointer to Platform device handle
>>>>> + * @mesg: power state
>>>>> + *
>>>>> + * Returns -ENOSYS
>
> What breaks if you return 0 instead of return -ENOSYS? Returning error seems
> to break platform suspend/resume until all the TODO's are addressed. If the
> current s/w cannot make h/w suspend, it should be okay to let the rest of
> the system be suspended.
>
In that case how will the controller be in a working state once it resumes.
It does not make sense to return zero and to notify the OS that
everything is fine.
>>>>> + */
>>>>> +static int ufshcd_pltfrm_suspend(struct platform_device *pdev,
>>>>> + pm_message_t mesg)
>>>>> +{
>>>>> + /*
>>>>> + * TODO:
>>>>> + * 1. Call ufshcd_suspend
>>>>> + * 2. Do bus specific power management
>>>>> + */
>>>>> +
>>>>> + return -ENOSYS;
>
> Returning error doesn't allow entire system to be suspended. Perhaps, you
> can do disable_irq() and return 0?
>
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * ufshcd_pltfrm_resume - resume power management function
>>>>> + * @pdev: pointer to Platform device handle
>>>>> + *
>>>>> + * Returns -ENOSYS
>>>>> + */
>>>>> +static int ufshcd_pltfrm_resume(struct platform_device *pdev)
>>>>> +{
>>>>> + /*
>>>>> + * TODO:
>>>>> + * 1. Call ufshcd_resume.
>>>>> + * 2. Do bus specific wake up
>>>>> + */
>>>>> +
>>>>> + return -ENOSYS;
>
> enable_irq() and return 0?
>
>>>>> +}
>>>>> +#endif
>>>>> +
>
>>>>> +static int __devexit ufshcd_pltfrm_remove(struct platform_device
>>>>> *pdev)
>>>>> +{
>>>>> + struct resource *mem_res;
>>>>> + struct resource *irq_res;
>>>>> + resource_size_t mem_size;
>>>>> + struct ufs_hba *hba = platform_get_drvdata(pdev);
>>>>> +
>>>>> + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>
>>>> It would be better to save the irq number under "struct ufs_hba" during
>>>> probe. So here during remove you just need simply need to call the
>>>> "free_irq(irq_res->start, hba)"
>>>
>>> Will modify the code accordingly in the next patchset.
>>>>>
>>>>> +
>>>>> + if (!irq_res)
>>>>> + dev_err(&pdev->dev, "ufshcd: IRQ resource not
>>>>> available\n");
>>>>> + else
>>>>> + free_irq(irq_res->start, hba);
>
>
> The documentation of free_irq says:
> "... On a shared IRQ the caller must ensure the interrupt is disabled on the
> card it drives before calling this function. .." I don't see disable_irq()
> getting called either here or ufshcd_remove().
Why would you want to disable the entire IRQ line when it is shared?
Logical thing is to disable the interrupt on the controller.
>
>
>>>>> +
>>>>> + ufshcd_remove(hba);
>>>>
>>>> Remove should be exactly opposite of probe(). So shouldn't you call the
>>>> ufshcd_remove() first and then free_irq() after that.
>>>
>>> Some bugging controllers might raise the interrupt after resources are
>>> removed.
>>> this sequence will prevent it.
>>
>>
>> Could you please add the same as comment in above code sequence?
>>
>>>>> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>
>>>> You might want to save the pointer to mem_res in "struct ufs_hba" during
>>>> probe and may use the same here.
>>>>>
>>>>> + if (!mem_res)
>>>>> + dev_err(&pdev->dev, "ufshcd: Memory resource not
>>>>> available\n");
>>>>> + else {
>>>>> + mem_size = resource_size(mem_res);
>>>>> + release_mem_region(mem_res->start, mem_size);
>>>>> + }
>>>>> + platform_set_drvdata(pdev, NULL);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id ufs_of_match[] = {
>>>>> + { .compatible = "jedec,ufs-1.1"},
>>>>> +};
>>>>> +
>>>>> +static struct platform_driver ufshcd_pltfrm_driver = {
>>>>> + .probe = ufshcd_pltfrm_probe,
>>>>> + .remove = __devexit_p(ufshcd_pltfrm_remove),
>>>>> +#ifdef CONFIG_PM
>
>
> CONFIG_PM_SLEEP would be better?
the current implementation looks fine.
>Also, can you move legacy suspend/resume
Ok,
> callbacks below to dev_pm_ops?
>
>>>>> + .suspend = ufshcd_pltfrm_suspend,
>>>>> + .resume = ufshcd_pltfrm_resume,
>>>>> +#endif
>>>>> + .driver = {
>>>>> + .name = "ufshcd",
>>>>> + .owner = THIS_MODULE,
>>>>> + .of_match_table = ufs_of_match,
>>>>> + },
>>>>> +};
>
>
> --
> Regards,
> Sujit Reddy Thumma
>
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
--
Regards,
Vinayak Holikatti
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
