From: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
To: "József Horváth" <info@ministro.hu>
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: Fri, 11 Dec 2020 06:50:58 +0100 [thread overview]
Message-ID: <X9MIwqJBG69M5uHq@kroah.com> (raw)
In-Reply-To: <20201210194625.GA17516@dincontrollerdev>
On Thu, Dec 10, 2020 at 07:46:25PM +0000, József Horváth wrote:
> 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.
My point is, a serial driver should NOT have any custom ioctls.
> 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.
That is what a device tree is for, to configure the device to have the
correct system configuration, why can't that be the same here?
> 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.
So we have to take a "custom" userspace blob and send it to the device
to configure it properly? Like Jiri said, sounds like firmware, so just
use that interface instead.
thanks,
greg k-h
next prev parent reply other threads:[~2020-12-11 5:52 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
2020-12-11 5:33 ` Jiri Slaby
2020-12-11 5:50 ` 'Greg Kroah-Hartman' [this message]
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=X9MIwqJBG69M5uHq@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=devicetree@vger.kernel.org \
--cc=info@ministro.hu \
--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).