<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html lang="en" style='--code-editor-font: var(--default-mono-font, "GitLab Mono"), JetBrains Mono, Menlo, DejaVu Sans Mono, Liberation Mono, Consolas, Ubuntu Mono, Courier New, andale mono, lucida console, monospace;'>
<head>
<meta content="text/html; charset=US-ASCII" http-equiv="Content-Type">
<title>
GitLab
</title>
<style data-premailer="ignore" type="text/css">
a { color: #1068bf; }
</style>
<style>img {
max-width: 100%; height: auto;
}
body {
font-size: .875rem;
}
body {
-webkit-text-shadow: rgba(255,255,255,.01) 0 0 1px;
}
body {
font-family: "GitLab Sans",-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,"Noto Sans",Ubuntu,Cantarell,"Helvetica Neue",sans-serif,"Apple Color Emoji","Segoe UI Emoji","Segoe UI Symbol","Noto Color Emoji"; font-size: inherit;
}
</style>
</head>
<body style='font-size: inherit; -webkit-text-shadow: rgba(255,255,255,.01) 0 0 1px; font-family: "GitLab Sans",-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,"Noto Sans",Ubuntu,Cantarell,"Helvetica Neue",sans-serif,"Apple Color Emoji","Segoe UI Emoji","Segoe UI Symbol","Noto Color Emoji";'>
<div class="content">
<p style="color: #777777;">
<a href="https://gitlab.rtems.org/c-mauderer">Christian Mauderer</a>
<a href="https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_123059">commented</a>:
</p>
<div class="md" style="position: relative; z-index: 1; color: #28272d; word-wrap: break-word;">
<p dir="auto" style="color: #28272d; margin: 0 0 16px;" align="initial">The code looks OK for me. One point regarding the code: In the transfer function, you use some of the flags (like <code style='font-size: 90%; color: #18171d; word-wrap: break-word; background-color: #ececef; border-radius: .25rem; margin-top: 0; font-weight: inherit; font-family: "GitLab Mono","JetBrains Mono","Menlo","DejaVu Sans Mono","Liberation Mono","Consolas","Ubuntu Mono","Courier New","andale mono","lucida console",monospace; vertical-align: bottom; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>I2C_M_TEN</code>). But not everything is supported (like <code style='font-size: 90%; color: #18171d; word-wrap: break-word; background-color: #ececef; border-radius: .25rem; font-weight: inherit; font-family: "GitLab Mono","JetBrains Mono","Menlo","DejaVu Sans Mono","Liberation Mono","Consolas","Ubuntu Mono","Courier New","andale mono","lucida console",monospace; vertical-align: bottom; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>I2C_M_IGNORE_NACK</code>). I would suggest to somewhere check whether only flags that are supported are set. For example, I usually try to do that at the start of a transfer here:</p>
<p dir="auto" style="color: #28272d; margin: 0 0 16px;" align="initial"><a href="https://gitlab.rtems.org/rtems/rtos/rtems/-/blob/main/bsps/arm/imxrt/i2c/imxrt-lpi2c.c?ref_type=heads#L305" style="margin-top: 0;">https://gitlab.rtems.org/rtems/rtos/rtems/-/blob/main/bsps/arm/imxrt/i2c/imxrt-lpi2c.c?ref_type=heads#L305</a></p>
<p dir="auto" style="color: #28272d; margin: 0 0 16px;" align="initial">But doing that on the fly is also OK. It just should happen at some point so that an application programmer notes if he tries to use something that is not supported.</p>
<p dir="auto" style="color: #28272d; margin: 0 0 16px;" align="initial">A functional edge case that you might want to test: Some I2C controllers support an addressing phase with no data. That can be (for example) used to detect I2C devices connected to the bus. We have a <code style='font-size: 90%; color: #18171d; word-wrap: break-word; background-color: #ececef; border-radius: .25rem; margin-top: 0; font-weight: inherit; font-family: "GitLab Mono","JetBrains Mono","Menlo","DejaVu Sans Mono","Liberation Mono","Consolas","Ubuntu Mono","Courier New","andale mono","lucida console",monospace; vertical-align: bottom; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>i2cdetect</code> command for that:</p>
<p dir="auto" style="color: #28272d; margin: 0 0 16px;" align="initial"><a href="https://gitlab.rtems.org/rtems/rtos/rtems/-/blob/main/cpukit/libmisc/shell/main_i2cdetect.c?ref_type=heads" style="margin-top: 0;">https://gitlab.rtems.org/rtems/rtos/rtems/-/blob/main/cpukit/libmisc/shell/main_i2cdetect.c?ref_type=heads</a></p>
<p dir="auto" style="color: #28272d; margin: 0;" align="initial">Note that not all drivers and all hardware support it and that's OK. But you should make sure that your driver at least doesn't crash or hangs with these settings. I have roughly looked through your code and I would expect that it at least doesn't crash and maybe even work without changes. But it would be good if you could confirm that you tested it.</p>
</div>
</div>
<div class="footer" style="margin-top: 10px;">
<p style="font-size: small; color: #737278;">
—
<br>
<a href="https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_123059">View it on GitLab</a>.
<br>
You're receiving this email because of your account on <a target="_blank" rel="noopener noreferrer" href="https://gitlab.rtems.org">gitlab.rtems.org</a>. <a href="https://gitlab.rtems.org/-/sent_notifications/b62a463cfbc11dbca1c86ab3fa8dc659/unsubscribe" target="_blank" rel="noopener noreferrer">Unsubscribe</a> from this thread · <a href="https://gitlab.rtems.org/-/profile/notifications" target="_blank" rel="noopener noreferrer" class="mng-notif-link">Manage all notifications</a> · <a href="https://gitlab.rtems.org/help" target="_blank" rel="noopener noreferrer" class="help-link">Help</a>
<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","action":{"@type":"ViewAction","name":"View Merge request","url":"https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_123059"}}</script>
</p>
</div>
</body>
</html>