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/03/08 15:36:31 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #495:
SPI: Change spi_send() interface to support of 32-bit word transfer
pkarashchenko opened a new pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on issue #495: SPI:
Change spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596239017
@patacongo
Currently, almost all of the files related to SPI drivers do not comply with code style.
So should that compliance be introduced in this PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596233998
I can look at this next week.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull
request #495: SPI: Change spi_send() interface to support of 32-bit word
transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#discussion_r389380490
##########
File path: Documentation/NuttxPortingGuide.html
##########
@@ -1751,7 +1751,7 @@ <h2><a name="naming">4.1 Naming and Header File Conventions</a></h2>
The definitions in that header file provide the common interface between NuttX and the architecture-specific implementation in <code>arch/</code>.
</p>
<blockquote><small>
- <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>�</i>. So it would be <code>�P</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
+ <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>�</i>. So it would be <code>�P</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
Review comment:
```suggestion
<code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596542387
All chekcs pass. 2 of the 4 requested reviewers have approved (one cannot review until next week).
The current PR is current not reviewable further due to all of the nxstyle fixes.
All-in-all, I see no reason to hold thie change up any further.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596240014
> Currently, almost all of the files related to SPI drivers do not comply with code style.
> So should that compliance be introduced in this PR?
You need that remember that once we moved as an Apache project, no single person has any authority to set policies. Currently this is being discussed by Xaio Xiang and DavidS. I will defer to them. Normally, in the past, I would take the files and make the nxstyle fixes on your behalf. But I have been asked not to do that further. Although I am free to do that if I choose, I will follow the advice of Xiao Xiang.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596240351
In this case, we have to get past the nxstyle checks before we can get to the more important build tests. So, yes, someone will have to make the files conform to the coding standard before it can be merged.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull
request #495: SPI: Change spi_send() interface to support of 32-bit word
transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#discussion_r389380606
##########
File path: Documentation/NuttxPortingGuide.html
##########
@@ -1751,7 +1751,7 @@ <h2><a name="naming">4.1 Naming and Header File Conventions</a></h2>
The definitions in that header file provide the common interface between NuttX and the architecture-specific implementation in <code>arch/</code>.
</p>
<blockquote><small>
- <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>�</i>. So it would be <code>�P</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
+ <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
Review comment:
```suggestion
<code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull
request #495: SPI: Change spi_send() interface to support of 32-bit word
transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#discussion_r389380606
##########
File path: Documentation/NuttxPortingGuide.html
##########
@@ -1751,7 +1751,7 @@ <h2><a name="naming">4.1 Naming and Header File Conventions</a></h2>
The definitions in that header file provide the common interface between NuttX and the architecture-specific implementation in <code>arch/</code>.
</p>
<blockquote><small>
- <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>�</i>. So it would be <code>�P</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
+ <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
Review comment:
```suggestion
<code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596238546
I think that the keys to merging the PR are:
- We need to get through all checks without any failure.
- The impact to architecture-owners is simply to determine is you can live with the return value from SPI_SEND() being a uint32_t. That has more implact on some architectures than others.
- We need some assessment of the impact to end-users who have their own SPI driver implementations.
- We need approval from all reviewers.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on issue #495: SPI:
Change spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596268571
I have managed to resolve all nxstyle issues. But IMO now PR is extremely hard to review.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] Ouss4 commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596430256
Looking good, thanks @pkarashchenko.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull
request #495: SPI: Change spi_send() interface to support of 32-bit word
transfer
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#discussion_r389419371
##########
File path: Documentation/NuttxPortingGuide.html
##########
@@ -1751,7 +1751,8 @@ <h2><a name="naming">4.1 Naming and Header File Conventions</a></h2>
The definitions in that header file provide the common interface between NuttX and the architecture-specific implementation in <code>arch/</code>.
</p>
<blockquote><small>
- <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>�</i>. So it would be <code>�P</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
+ <code>up_</code> is supposed to stand for microprocessor; the <code>u</code> is like the Greek letter micron: <i>ľ</i>. So it would be <code>ľP</code> which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used a more obvious prefix like <code>arch_</code> instead -- then I would not have to answer this question so often.
+
Review comment:
```suggestion
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #495: SPI: Change
spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596219405
Two comments:
1) We need to get past all of the automated checks before we do anything. In this case, it is not getting far because most of the modified files do not conform completely to the coding standard. We need to decide what to do there because that blocks the other build tests.
2) I would like to get a broader community input. This does not have any effect on me personally, so I do not really have an opinion. The only rather small issue that I see is that for the majority of SPI drivers that only support short SPI transfers, this does add some overhead.
I think that the people who are really affected are all of the NuttX uses who have private SPI drivers. This will cause some code breakage for users of NuttX. We need to make sure that we consider the concerns of those other users who have SPI drivers that are not in the source tree.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #495: SPI:
Change spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596302702
> I have managed to resolve all nxstyle issues. But IMO now PR is extremely hard to review.
@pkarashchenko thanks for your hard work to address all nxstyle issue, the change looks good for me. I will send a VOTE to relex the precheck a little bit soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #495: SPI:
Change spi_send() interface to support of 32-bit word transfer
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #495: SPI: Change spi_send() interface to support of 32-bit word transfer
URL: https://github.com/apache/incubator-nuttx/pull/495#issuecomment-596240014
> Currently, almost all of the files related to SPI drivers do not comply with code style.
> So should that compliance be introduced in this PR?
You need that remember that once we moved as an Apache project, no single person has any authority to set policies. Currently this is being discussed by Xaio Xiang and DavidS. I will defer to them. Normally, in the past, I would take the files and make the nxstyle fixes on your behalf. But I have been asked not to do that further. Although I am free to do that if I choose since no individual has the authority to set policy, I will follow the advice of Xiao Xiang.
----------------------------------------------------------------
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
With regards,
Apache Git Services