Date:   Mon, 10 Jan 2022 14:05:55 +0000
From:   Sidong Yang <realwakka@gmail.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: enforce tx_cfg to be set before any
 message can be sent
Message-ID: <20220110140555.GB27767@realwakka>
References: <20220108222652.GA11883@mail.google.com>
 <20220109154950.GA27767@realwakka>
 <20220109190552.GA2928@mail.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220109190552.GA2928@mail.google.com>
Precedence: bulk

On Mon, Jan 10, 2022 at 08:05:52AM +1300, Paulo Miguel Almeida wrote:
> On Sun, Jan 09, 2022 at 03:49:50PM +0000, Sidong Yang wrote:
> > On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
> > 
> > Hi, Paulo.
> > Thanks for the patch.
> > I have some opinion below.
> 
> thanks for taking the time to review my patch :)
> 
> > > this driver relies on exposing a char device to userspace to tx
> > > messages. Every message can be sent using different trasmitter settings
> > > such so the tx_cfg must be written before sending any messages.
> > > Failing to do so will cause the message to fail silently depending on
> > > printk/dynamic_debug settings which makes it hard to troubleshoot.
> > > 
> > > This patch add a control variable that will get initialized once tx_cfg
> > > is set for the fd used when interacting with the char device.
> > 
> > I don't know that adding flag is good idea. It seems that initializing
> > to default is better than this.
> 
> the reasons why I thought that the flag was a good approach is because of
> these points:
> 
> 1 - it returns an error to userspace to help the developers 
> troubleshooting what is missing/required to make a message to be
> successfully tx'ed. Unfortunately, once the message is in the chip
> queue, there isn't much that the user can know so, from my humble point
> of view, any way we can avoid hard-to-debug problems, we should.

I understood that you mean that user has difficult to debug when it
write message before initializing. If so, user would get debug message
that some tx_cfg members are invalid. I think the point is which message
would be useful for debugging. 'Some member is invalid' or 'tx_cfg is
uninitialized'. I think the latter that you said is more accurate than
before one. I think it make sense.

> 
> 2 - rf69 work with multiple frequencies (315,433,868 and 915MHz) in which
> acceptance varies from region to region. Essentially, picking 1 default
> frequency may be ideal for one place but not right for another.

Actually, I don't have idea about this. I want to know that how to other
module like this handle this. 

Thanks,
Sidong

> 
> Let me know if you agree with my train of thought above and if not,
> share with me your point of view.
> 
> thanks,
> 
> Paulo A.
>