linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
To: Sidong Yang <realwakka@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 08:05:52 +1300	[thread overview]
Message-ID: <20220109190552.GA2928@mail.google.com> (raw)
In-Reply-To: <20220109154950.GA27767@realwakka>

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.

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.

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.


  reply	other threads:[~2022-01-09 19: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 [this message]
2022-01-10 14:05     ` Sidong Yang
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=20220109190552.GA2928@mail.google.com \
    --to=paulo.miguel.almeida.rodenas@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=realwakka@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).