Re: [PATCH v2] Serial: silabs si4455 serial driver

[József Horváth]

On Fri, Dec 11, 2020 at 06:50:58AM +0100, 'Greg Kroah-Hartman' wrote:
> 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.
> > > > +
> > > > +#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.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch).
I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime.
SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.
SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information.
(Currently I'm the only one user, and I need this :) )

> 
> thanks,
> 
> greg k-h

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