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
Date: Mon, 10 Jan 2022 14:05:55 +0000 [thread overview]
Message-ID: <20220110140555.GB27767@realwakka> (raw)
In-Reply-To: <20220109190552.GA2928@mail.google.com>
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.
>
next prev parent reply other threads:[~2022-01-10 14:06 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-08 22:26 [PATCH] staging: pi433: enforce tx_cfg to be set before any message can be sent Paulo Miguel Almeida
2022-01-09 15:49 ` Sidong Yang
2022-01-09 19:05 ` Paulo Miguel Almeida
2022-01-10 14:05 ` Sidong Yang [this message]
2022-01-10 9:17 ` Dan Carpenter
2022-01-12 8:40 ` Paulo Miguel Almeida
2022-01-14 22:16 ` [PATCH v2] " Paulo Miguel Almeida
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220110140555.GB27767@realwakka \
--to=realwakka@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=paulo.miguel.almeida.rodenas@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).