Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280535 > unrolled thread

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management  driver Vinod Koul <vinod.koul@intel.com> - 2015-12-01 04:20 +0100
    Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management  driver Sinan Kaya <okaya@codeaurora.org> - 2015-12-02 06:00 +0100
      Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management  driver Vinod Koul <vinod.koul@intel.com> - 2015-12-05 09:00 +0100
        Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management  driver Sinan Kaya <okaya@codeaurora.org> - 2015-12-08 15:40 +0100

#1280535 — Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:

> >> +static int hidma_mgmt_probe(struct platform_device *pdev)
> >> +{
> >> +	struct hidma_mgmt_dev *mgmtdev;
> >> +	struct resource *res;
> >> +	void __iomem *virtaddr;
> >> +	int irq;
> >> +	int rc;
> >> +	u32 val;
> >> +
> >> +	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> >> +	pm_runtime_use_autosuspend(&pdev->dev);
> >> +	pm_runtime_set_active(&pdev->dev);
> >> +	pm_runtime_enable(&pdev->dev);
> > 
> > at this time pm core will treat device as fully enabled and pm methods can
> > be invoked, but you are not ready yet right. Typically these are done at the
> > end of the probe unless you have a reason... 
> 
> I need it here because the clocks are declared as ACPI power resources.
> The kernel is turning off all power resources during initialization. In
> order for this code to touch the hardware, I need to call enable so that
> clocks are enabled once again.

The question is are you ready in your driver routines to be invoked by pm
core?

> 
> > 
> >> +static ssize_t show_values(struct device *dev, struct device_attribute *attr,
> >> +				char *buf)
> > 
> > Please fix the coding style here and other places as well. 
> 
> what's the problem here?
> 
> > Specifically
> > please read Chapter 2
> > 
> 
> Why is checkpatch not complaining about any of the coding style issues?
> I'm checking my code with checkpatch before submitting. Is there any
> other tool that would catch this?

So did you read the Chapter 2.. Quoting here

"Statements longer than 80 columns will be broken into sensible chunks,
unless
exceeding 80 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent
and
are placed substantially to the right. The same applies to function headers
with a long argument list. However, never break user-visible strings such as
printk messages, because that breaks the ability to grep for them."

Your breaking lines is not placed substantially to the right..
I do not think checkpatch is checking this..

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

[toc] | [next] | [standalone]

#1281445

On 11/30/2015 10:17 PM, Vinod Koul wrote:
> On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
> 
>>>> +static int hidma_mgmt_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct hidma_mgmt_dev *mgmtdev;
>>>> +	struct resource *res;
>>>> +	void __iomem *virtaddr;
>>>> +	int irq;
>>>> +	int rc;
>>>> +	u32 val;
>>>> +
>>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
>>>> +	pm_runtime_use_autosuspend(&pdev->dev);
>>>> +	pm_runtime_set_active(&pdev->dev);
>>>> +	pm_runtime_enable(&pdev->dev);
>>>
>>> at this time pm core will treat device as fully enabled and pm methods can
>>> be invoked, but you are not ready yet right. Typically these are done at the
>>> end of the probe unless you have a reason... 
>>
>> I need it here because the clocks are declared as ACPI power resources.
>> The kernel is turning off all power resources during initialization. In
>> order for this code to touch the hardware, I need to call enable so that
>> clocks are enabled once again.
> 
> The question is are you ready in your driver routines to be invoked by pm
> core?
> 

I don't have any support for suspend and resume PM APIs. The only PM
interface I support is PM runtime. PM can turn on/off the clocks based
on the reference counts it maintains after get/set APIs. Since PM is
turning off the clocks during power up before my driver load, I do need
to grab this lock to re-enable it during HW initialization. Then, let PM
turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.

Is there any other interaction with the PM that I'm not aware of?

>>
>>>
>>>> +static ssize_t show_values(struct device *dev, struct device_attribute *attr,
>>>> +				char *buf)
>>>
>>> Please fix the coding style here and other places as well. 
>>
>> what's the problem here?
>>
>>> Specifically
>>> please read Chapter 2
>>>
>>
>> Why is checkpatch not complaining about any of the coding style issues?
>> I'm checking my code with checkpatch before submitting. Is there any
>> other tool that would catch this?
> 
> So did you read the Chapter 2.. Quoting here
> 

I did read the chapter 2. Maybe, my lack of native english speaking but
I don't get from this sentence that function parameters need to be
aligned to the opening paranthesis.

> Descendants are always substantially shorter than the parent
> and are placed substantially to the right. The same applies to function headers
> with a long argument list. 
> 

I ran Lindent and manually cleaned up the junk it introduced. The result
is this

static ssize_t show_values(struct device *dev, struct device_attribute
*attr,
			   char *buf)


> Your breaking lines is not placed substantially to the right..
> I do not think checkpatch is checking this..
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1284489

On Tue, Dec 01, 2015 at 11:57:34PM -0500, Sinan Kaya wrote:
> On 11/30/2015 10:17 PM, Vinod Koul wrote:
> > On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
> > 
> >>>> +static int hidma_mgmt_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct hidma_mgmt_dev *mgmtdev;
> >>>> +	struct resource *res;
> >>>> +	void __iomem *virtaddr;
> >>>> +	int irq;
> >>>> +	int rc;
> >>>> +	u32 val;
> >>>> +
> >>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> >>>> +	pm_runtime_use_autosuspend(&pdev->dev);
> >>>> +	pm_runtime_set_active(&pdev->dev);
> >>>> +	pm_runtime_enable(&pdev->dev);
> >>>
> >>> at this time pm core will treat device as fully enabled and pm methods can
> >>> be invoked, but you are not ready yet right. Typically these are done at the
> >>> end of the probe unless you have a reason... 
> >>
> >> I need it here because the clocks are declared as ACPI power resources.
> >> The kernel is turning off all power resources during initialization. In
> >> order for this code to touch the hardware, I need to call enable so that
> >> clocks are enabled once again.
> > 
> > The question is are you ready in your driver routines to be invoked by pm
> > core?
> > 
> 
> I don't have any support for suspend and resume PM APIs. The only PM
> interface I support is PM runtime. PM can turn on/off the clocks based
> on the reference counts it maintains after get/set APIs. Since PM is
> turning off the clocks during power up before my driver load, I do need
> to grab this lock to re-enable it during HW initialization. Then, let PM
> turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.
> 
> Is there any other interaction with the PM that I'm not aware of?

No this is fine. The the runtime_resume will be onvoked and it will request
resources are those set before you enable the device?

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

[toc] | [prev] | [next] | [standalone]

#1286541

On 12/5/2015 2:58 AM, Vinod Koul wrote:
> On Tue, Dec 01, 2015 at 11:57:34PM -0500, Sinan Kaya wrote:
>> On 11/30/2015 10:17 PM, Vinod Koul wrote:
>>> On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
>>>
>>>>>> +static int hidma_mgmt_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct hidma_mgmt_dev *mgmtdev;
>>>>>> +	struct resource *res;
>>>>>> +	void __iomem *virtaddr;
>>>>>> +	int irq;
>>>>>> +	int rc;
>>>>>> +	u32 val;
>>>>>> +
>>>>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
>>>>>> +	pm_runtime_use_autosuspend(&pdev->dev);
>>>>>> +	pm_runtime_set_active(&pdev->dev);
>>>>>> +	pm_runtime_enable(&pdev->dev);
>>>>>
>>>>> at this time pm core will treat device as fully enabled and pm methods can
>>>>> be invoked, but you are not ready yet right. Typically these are done at the
>>>>> end of the probe unless you have a reason... 
>>>>
>>>> I need it here because the clocks are declared as ACPI power resources.
>>>> The kernel is turning off all power resources during initialization. In
>>>> order for this code to touch the hardware, I need to call enable so that
>>>> clocks are enabled once again.
>>>
>>> The question is are you ready in your driver routines to be invoked by pm
>>> core?
>>>
>>
>> I don't have any support for suspend and resume PM APIs. The only PM
>> interface I support is PM runtime. PM can turn on/off the clocks based
>> on the reference counts it maintains after get/set APIs. Since PM is
>> turning off the clocks during power up before my driver load, I do need
>> to grab this lock to re-enable it during HW initialization. Then, let PM
>> turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.
>>
>> Is there any other interaction with the PM that I'm not aware of?
> 
> No this is fine. The the runtime_resume will be onvoked and it will request
> resources are those set before you enable the device?
> 

Yes, the only resource that this device needs for power management is
the ACPI power resources. The device does not support suspend/resume via
traditional _PS0 and _PS3 calls. ACPI power resources are initialized
during power up while ACPI is being enumerated.

The probing of the HIDMA driver happens much afterwards.


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web