devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "József Horváth" <info@ministro.hu>
To: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
Cc: 'Rob Herring' <robh+dt@kernel.org>,
	'Jiri Slaby' <jirislaby@kernel.org>,
	linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] Serial: silabs si4455 serial driver
Date: Thu, 10 Dec 2020 19:46:25 +0000	[thread overview]
Message-ID: <20201210194625.GA17516@dincontrollerdev> (raw)
In-Reply-To: <X9Jw+srprdT8tquZ@kroah.com>

On Thu, Dec 10, 2020 at 08:03:22PM +0100, 'Greg Kroah-Hartman' wrote:
> On Thu, Dec 10, 2020 at 05:04:46PM +0000, József Horváth wrote:
> > This is a serial port driver for
> > Silicon Labs Si4455 Sub-GHz transciver.
> > 
> > Signed-off-by: József Horváth <info@ministro.hu>
> > ---
> >  .../bindings/serial/silabs,si4455.yaml        |   53 +
> >  MAINTAINERS                                   |    7 +
> >  drivers/tty/serial/Kconfig                    |    8 +
> >  drivers/tty/serial/Makefile                   |    1 +
> >  drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
> >  drivers/tty/serial/si4455_api.h               |   56 +
> 
> First thing, a single .c file should not need a .h file.
> 
> But then I looked at the .h file and see:
> 
> > --- /dev/null
> > +++ b/drivers/tty/serial/si4455_api.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Copyright (C) 2020 József Horváth <info@ministro.hu>
> > + *
> > + */
> > +#ifndef SI4455_API_H_
> > +#define SI4455_API_H_
> > +
> > +struct si4455_iocbuff {
> > +	uint32_t length;
> > +	uint8_t	data[4096];
> 
> If you do have an ioctl, use proper data types.  These are not the
> correct ones (hint, __u32 and __u8).
> 
> > +};
> > +
> > +#define BASE_TTYIOC_PRIVATE		0xA0
> > +/* Set EZConfig.
> > + * After this ioctl call, the driver restarts the si4455,
> > + * then apply the new configuration and patch.
> > + */
> > +#define SI4455_IOC_SEZC		_IOW('T', \
> > +				     BASE_TTYIOC_PRIVATE + 0x01, \
> > +				     struct si4455_iocbuff)
> 
> Why does a serial driver have private ioctls?  Please no, don't do that.

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h.
In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

> Implement the basic serial driver first, and then we can talk about
> "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all.
The cofiguration for interface is provided by user for application.
It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

> 
> thanks,
> 
> greg k-h

Üdvözlettel / Best regards:
József Horváth


  reply	other threads:[~2020-12-10 19:48 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-10 17:04 [PATCH v2] Serial: silabs si4455 serial driver József Horváth
2020-12-10 19:03 ` 'Greg Kroah-Hartman'
2020-12-10 19:46   ` József Horváth [this message]
2020-12-11  5:33     ` Jiri Slaby
2020-12-11  5:50     ` 'Greg Kroah-Hartman'
2020-12-11  6:09       ` József Horváth
2020-12-11  6:20         ` 'Greg Kroah-Hartman'
2020-12-11  6:37           ` József Horváth
2020-12-11  7:33             ` 'Greg Kroah-Hartman'
2020-12-11  8:16               ` József Horváth
2020-12-11  8:43                 ` 'Greg Kroah-Hartman'
2020-12-11  9:18                   ` József Horváth
2020-12-11 12:09                     ` 'Greg Kroah-Hartman'
2020-12-11 12:24                       ` József Horváth

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=20201210194625.GA17516@dincontrollerdev \
    --to=info@ministro.hu \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    /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).