All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: Zijun Hu <zijuhu@codeaurora.org>,
	robh@kernel.org, jirislaby@kernel.org,
	linux-serial@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-bluetooth <linux-bluetooth@vger.kernel.org>,
	Zijun Hu <quic_zijuhu@quicinc.com>
Subject: Re: [PATCH v1] serdev: Add interface serdev_device_ioctl
Date: Mon, 1 Nov 2021 12:54:09 +0100	[thread overview]
Message-ID: <YX/VYUC3ngOf5bX5@kroah.com> (raw)
In-Reply-To: <DC399B43-DB1E-42AC-8A31-3A2C9407EE6D@holtmann.org>

On Mon, Nov 01, 2021 at 12:45:36PM +0100, Marcel Holtmann wrote:
> Hi Greg,
> 
> >>>>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
> >>>>>> maybe be used to make serdev_device ready to talk with tty port, so
> >>>>>> add interface serdev_device_ioctl().
> >>>>>> 
> >>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>>> ---
> >>>>>> drivers/tty/serdev/core.c           | 11 +++++++++++
> >>>>>> drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
> >>>>>> include/linux/serdev.h              |  9 +++++++++
> >>>>>> 3 files changed, 32 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >>>>>> index f1324fe99378..c0f6cd64716b 100644
> >>>>>> --- a/drivers/tty/serdev/core.c
> >>>>>> +++ b/drivers/tty/serdev/core.c
> >>>>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
> >>>>>> }
> >>>>>> EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> >>>>>> 
> >>>>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
> >>>>>> +{
> >>>>>> +	struct serdev_controller *ctrl = serdev->ctrl;
> >>>>>> +
> >>>>>> +	if (!ctrl || !ctrl->ops->ioctl)
> >>>>>> +		return -EOPNOTSUPP;
> >>>>> 
> >>>>> Wrong error for returning that an ioctl is not handled :(
> >>>> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
> >>>> do. so i change error code to EOPNOTSUPP.
> >>>> 
> >>>> #28: FILE: drivers/tty/serdev/core.c:412:
> >>>> +               return -ENOTSUPP;
> >>>> 
> >>>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> >>> 
> >>> Both of them are not the correct error to return when an ioctl is not
> >>> supported.
> >>> 
> >> is ENODEV okay?
> > 
> > No, -ENOTTY is the correct one as per the documentation, right?
> > 
> >>>>> Anyway, what in-tree driver needs this functionality?  Why does serdev
> >>>>> need any ioctl commands?
> >>>>> 
> >>>> i am developing driver for a special bluetooth controller which is integrated within SOC,
> >>>> and it does not connect with the BT HOST with UART as normal controller do, but it has very
> >>>> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
> >>>> driven by a tty driver developed. but it need to call tty ioctl to make the 
> >>>> special BT controller ready to talk with tty port. so i add this interface.
> >>> 
> >>> Please submit this change when you submit your driver that uses it at
> >>> the same time so we can review them all at once.  We do not add apis
> >>> that are not used in the kernel tree.
> >>> 
> >> okay
> >>>> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
> >>>> for these irregular devices.
> >>> 
> >>> For tty devices, "custom" ioctls are not ok, use the standard tty
> >>> commands and you should be fine for everything you need to do.
> >>> 
> >>> If not, then perhaps your design is incorrect?
> >>> 
> >> i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
> >> by serdev. so add this interface.
> > 
> > The 5.4 kernel is not relevant here, so I do not understand.
> > 
> >> are there any other good solution to advise?
> > 
> > Why not work with the bluetooth developers on this?
> 
> if this is just to have some hackish Bluetooth driver, then NAK from my side. Since we have serdev, we have no need for, or requirements for any ioctl anymore. If such thing is needed, it is a bad design.

Thanks for the confirmation, seems sane to me!

Zijun, please fix up your driver and submit it to be merged and all
should be fine, no need for any custom ioctls.

thanks,

greg k-h

  reply	other threads:[~2021-11-01 11:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-01  7:50 [PATCH v1] serdev: Add interface serdev_device_ioctl Zijun Hu
2021-11-01  7:59 ` Greg KH
2021-11-01  8:29   ` Zijun Hu
2021-11-01  8:32     ` Greg KH
2021-11-01  9:28       ` Zijun Hu
2021-11-01 11:18         ` Greg KH
2021-11-01 11:45           ` Marcel Holtmann
2021-11-01 11:54             ` Greg KH [this message]
2021-11-02  7:18               ` Zijun Hu

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=YX/VYUC3ngOf5bX5@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=quic_zijuhu@quicinc.com \
    --cc=robh@kernel.org \
    --cc=zijuhu@codeaurora.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.