You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/06/05 01:54:39 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #1196: Refactor audio
masayuki2009 opened a new pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196
## Summary
- This PR consists 2 commits to refactor nuttx audio drivers.
- The first commit is audio.h so that we can use struct ap_buffer_info_s without CONFIG_AUDIO_DRIVER_SPECIFIC_BUFFERS
- The second commit is to return -EINVAL in xxx_ioctl() if not handled.
## Impact
- This PR affects all audio drivers.
## Testing
- I tested this PR with nxplayer on both lc823450-xgevk which has wm8776.c and spresense which has cxd56.c
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435657741
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
I've just pushed with -f.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435655958
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
> Also https://pubs.opengroup.org/onlinepubs/009695399/functions/ioctl.html
>
> But currently every other NuttX ioctl function returns ENOTTY in that case. This would be an exception.
OK, I will modify the code and follow the NuttX's error code convention.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435653888
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
> This NuttX convention is that if the ioctl functions does not recognize the command, it returns ENOTTY, not EINVAL.
>
> See https://en.wikipedia.org/wiki/Not_a_typewriter which is how NutX does things in ioctls too.
Hi @patacongo,
Thanks for the comments.
Actually, I checked it with 'man ioctl' on Ubuntu18.04 and it says that
```
ERRORS
EBADF fd is not a valid file descriptor.
EFAULT argp references an inaccessible memory area.
EINVAL request or argp is not valid.
ENOTTY fd is not associated with a character special device.
ENOTTY The specified request does not apply to the kind of object that the file descriptor fd references.
```
So I thought we should return EINVAL.
However in NuttX should we return ENOTTY instead of EINVAL?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435652052
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
This NuttX convention is that if the ioctl functions does not recognize the command, it returns ENOTTY, not EINVAL.
See https://en.wikipedia.org/wiki/Not_a_typewriter which is how NutX does things in ioctls too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435660758
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
Here is Linus Torvalds interpretation of that exact same language: https://lore.kernel.org/patchwork/patch/258361/ I agree with Linus
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1196: Refactor audio
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1196:
URL: https://github.com/apache/incubator-nuttx/pull/1196#discussion_r435654681
##########
File path: drivers/audio/audio_null.c
##########
@@ -771,11 +772,12 @@ static int null_ioctl(FAR struct audio_lowerhalf_s *dev, int cmd,
#endif
default:
+ ret = -EINVAL;
Review comment:
Also https://pubs.opengroup.org/onlinepubs/009695399/functions/ioctl.html
But currently every other NuttX ioctl function returns ENOTTY in that case. This would be an exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org