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

Groups > linux.kernel > #1410806 > unrolled thread

Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing

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 7/8] mwifiex: don't print an error if an optional DT  property is missing Julian Calaby <julian.calaby@gmail.com> - 2016-06-01 06:30 +0200
    Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT  property is missing Javier Martinez Canillas <javier@osg.samsung.com> - 2016-06-01 16:00 +0200
      Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT  property is missing Julian Calaby <julian.calaby@gmail.com> - 2016-06-02 01:20 +0200
        RE: [PATCH 7/8] mwifiex: don't print an error if an optional DT  property is missing Amitkumar Karwar <akarwar@marvell.com> - 2016-06-09 16:00 +0200

#1410806 — Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
> binding document say that the "interrupts" property in the child node is
> optional. So the property being missed shouldn't be treated as an error.

Have you checked whether it is truly optional? I.e. nothing else
breaks if this property isn't set?

> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

Other than that, this looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[toc] | [next] | [standalone]

#1411242

Hello Julian,

Thanks a lot for your feedback and reviews.

On 06/01/2016 12:20 AM, Julian Calaby wrote:
> Hi All,
> 
> On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
>> binding document say that the "interrupts" property in the child node is
>> optional. So the property being missed shouldn't be treated as an error.
> 
> Have you checked whether it is truly optional? I.e. nothing else
> breaks if this property isn't set?
>

That's what the DT binding says and the IRQ is only used as a wakeup source
during system suspend, it is not used during runtime. And that is why the
mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.

Now, I just got to that conclusion by reading the binding docs, the message
in the commits that introduced this and the driver code. Xinming Hu should
comment on how critical this feature is for systems that needs to be wakeup.

In any case I think that the code should be consistent with what the binding
doc says and also the function does (i.e: dev_err only if returns an error).
 
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Other than that, this looks sensible to me.
> 
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

#1411653

Hi Javier,

On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> Hello Julian,
>
> Thanks a lot for your feedback and reviews.
>
> On 06/01/2016 12:20 AM, Julian Calaby wrote:
>> Hi All,
>>
>> On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
>> <javier@osg.samsung.com> wrote:
>>> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
>>> binding document say that the "interrupts" property in the child node is
>>> optional. So the property being missed shouldn't be treated as an error.
>>
>> Have you checked whether it is truly optional? I.e. nothing else
>> breaks if this property isn't set?
>>
>
> That's what the DT binding says and the IRQ is only used as a wakeup source
> during system suspend, it is not used during runtime. And that is why the
> mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.

Awesome, that's what I wanted to know.

> Now, I just got to that conclusion by reading the binding docs, the message
> in the commits that introduced this and the driver code. Xinming Hu should
> comment on how critical this feature is for systems that needs to be wakeup.

Xinming, could you review this also?

> In any case I think that the code should be consistent with what the binding
> doc says and also the function does (i.e: dev_err only if returns an error).
>
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> Other than that, this looks sensible to me.
>>
>> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
>>
>
> Best regards,

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

#1418358

> From: Julian Calaby [mailto:julian.calaby@gmail.com]
> Sent: Thursday, June 02, 2016 4:44 AM
> To: Javier Martinez Canillas; Xinming Hu
> Cc: linux-kernel@vger.kernel.org; Amitkumar Karwar; Kalle Valo; netdev;
> linux-wireless; Nishant Sarmukadam
> Subject: Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT
> property is missing
> 
> Hi Javier,
> 
> On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
> > Hello Julian,
> >
> > Thanks a lot for your feedback and reviews.
> >
> > On 06/01/2016 12:20 AM, Julian Calaby wrote:
> >> Hi All,
> >>
> >> On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
> >> <javier@osg.samsung.com> wrote:
> >>> The
> >>> Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
> >>> binding document say that the "interrupts" property in the child
> node is optional. So the property being missed shouldn't be treated as
> an error.
> >>
> >> Have you checked whether it is truly optional? I.e. nothing else
> >> breaks if this property isn't set?
> >>
> >
> > That's what the DT binding says and the IRQ is only used as a wakeup
> > source during system suspend, it is not used during runtime. And that
> > is why the
> > mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.
> 
> Awesome, that's what I wanted to know.
> 
> > Now, I just got to that conclusion by reading the binding docs, the
> > message in the commits that introduced this and the driver code.
> > Xinming Hu should comment on how critical this feature is for systems
> that needs to be wakeup.
> 
> Xinming, could you review this also?
> 

Yes. IRQ is the optional parameter. System has a flexibility to not use it, but it still can configure other device tree parameters. The patch looks good.

Regards,
Amitkumar

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web