Re: [PATCH v1] serdev: Add interface serdev_device_ioctl

[Zijun Hu]

On 11/1/2021 4:32 PM, Greg KH wrote:
> On Mon, Nov 01, 2021 at 04:29:10PM +0800, Zijun Hu wrote:
>>
>>
>> On 11/1/2021 3:59 PM, Greg KH wrote:
>>> On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxx>
>>>> ---
>>>>  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?
>>> 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.
are there any other good solution to advise?
thanks 
> thanks,
> 
> greg k-h
>