1. Patchew
  2. QEMU
  3. [PATCH 0/5] PMBus fixes and new functions
  4. View patch

[PATCH 1/5] hw/i2c: pmbus add support for block receive

Titus Rwantare posted 5 patches 2 years, 10 months ago
Maintainers: Titus Rwantare <titusr@google.com>
There is a newer version of this series
[PATCH 1/5] hw/i2c: pmbus add support for block receive
Posted by Titus Rwantare 2 years, 10 months ago 
Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
 include/hw/i2c/pmbus_device.h |  7 +++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index c3d6046784..02647769cd 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
 void pmbus_send_string(PMBusDevice *pmdev, const char *data)
 {
     size_t len = strlen(data);
-    g_assert(len > 0);
     g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
     pmdev->out_buf[len + pmdev->out_buf_len] = len;
 
@@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
     pmdev->out_buf_len += len + 1;
 }
 
+uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len)
+{
+    /* dest may contain data from previous writes */
+    memset(dest, 0, len);
+
+    /* Exclude command code from return value */
+    pmdev->in_buf++;
+    pmdev->in_buf_len--;
+
+    /* The byte after the command code denotes the length */
+    uint8_t sent_len = pmdev->in_buf[0];
+
+    if (sent_len != pmdev->in_buf_len - 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: length mismatch. Expected %d bytes, got %d bytes\n",
+                      __func__, sent_len, pmdev->in_buf_len - 1);
+    }
+
+    /* exclude length byte */
+    pmdev->in_buf++;
+    pmdev->in_buf_len--;
+
+    if (pmdev->in_buf_len < len) {
+        len = pmdev->in_buf_len;
+    }
+    memcpy(dest, pmdev->in_buf, len);
+    return len;
+}
+
 
 static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
 {
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 93f5d57c9d..7dc00cc4d9 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data);
  */
 void pmbus_send_string(PMBusDevice *state, const char *data);
 
+/**
+ * @brief Receive data sent with Block Write.
+ * @param dest - memory with enough capacity to receive the write
+ * @param len - the capacity of dest
+ */
+uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len);
+
 /**
  * @brief Receive data over PMBus
  * These methods help track how much data is being received over PMBus
-- 
2.40.0.rc1.284.g88254d51c5-goog
Re: [PATCH 1/5] hw/i2c: pmbus add support for block receive
Posted by Corey Minyard 2 years, 10 months ago 
It's generally frowned upon to have empty descriptions, some rationale
would be helpful.  For instance, you remove a length check from the send
string, why did you do that?

Any why is this being added?  What's it supporting?

-corey

On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote:
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
>  include/hw/i2c/pmbus_device.h |  7 +++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index c3d6046784..02647769cd 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
>  void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>  {
>      size_t len = strlen(data);
> -    g_assert(len > 0);
>      g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
>      pmdev->out_buf[len + pmdev->out_buf_len] = len;
>  
> @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>      pmdev->out_buf_len += len + 1;
>  }
>  
> +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len)
> +{
> +    /* dest may contain data from previous writes */
> +    memset(dest, 0, len);
> +
> +    /* Exclude command code from return value */
> +    pmdev->in_buf++;
> +    pmdev->in_buf_len--;
> +
> +    /* The byte after the command code denotes the length */
> +    uint8_t sent_len = pmdev->in_buf[0];
> +
> +    if (sent_len != pmdev->in_buf_len - 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected %d bytes, got %d bytes\n",
> +                      __func__, sent_len, pmdev->in_buf_len - 1);
> +    }
> +
> +    /* exclude length byte */
> +    pmdev->in_buf++;
> +    pmdev->in_buf_len--;
> +
> +    if (pmdev->in_buf_len < len) {
> +        len = pmdev->in_buf_len;
> +    }
> +    memcpy(dest, pmdev->in_buf, len);
> +    return len;
> +}
> +
>  
>  static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
>  {
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 93f5d57c9d..7dc00cc4d9 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data);
>   */
>  void pmbus_send_string(PMBusDevice *state, const char *data);
>  
> +/**
> + * @brief Receive data sent with Block Write.
> + * @param dest - memory with enough capacity to receive the write
> + * @param len - the capacity of dest
> + */
> +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len);
> +
>  /**
>   * @brief Receive data over PMBus
>   * These methods help track how much data is being received over PMBus
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>
Re: [PATCH 1/5] hw/i2c: pmbus add support for block receive
Posted by Titus Rwantare 2 years, 10 months ago 
Apologies. I've updated the commit descriptions and added a sensor using
this code in v2.

While doing block receive I discovered that it is valid behaviour to erase
a field and have it be an empty string.

-Titus

On Thu, 30 Mar 2023 at 09:18, Corey Minyard <minyard@acm.org> wrote:

> It's generally frowned upon to have empty descriptions, some rationale
> would be helpful.  For instance, you remove a length check from the send
> string, why did you do that?
>
> Any why is this being added?  What's it supporting?
>
> -corey
>
> On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote:
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Signed-off-by: Titus Rwantare <titusr@google.com>
> > ---
> >  hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
> >  include/hw/i2c/pmbus_device.h |  7 +++++++
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> > index c3d6046784..02647769cd 100644
> > --- a/hw/i2c/pmbus_device.c
> > +++ b/hw/i2c/pmbus_device.c
> > @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
> >  void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> >  {
> >      size_t len = strlen(data);
> > -    g_assert(len > 0);
> >      g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> >      pmdev->out_buf[len + pmdev->out_buf_len] = len;
> >
> > @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const
> char *data)
> >      pmdev->out_buf_len += len + 1;
> >  }
> >
> > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t
> len)
> > +{
> > +    /* dest may contain data from previous writes */
> > +    memset(dest, 0, len);
> > +
> > +    /* Exclude command code from return value */
> > +    pmdev->in_buf++;
> > +    pmdev->in_buf_len--;
> > +
> > +    /* The byte after the command code denotes the length */
> > +    uint8_t sent_len = pmdev->in_buf[0];
> > +
> > +    if (sent_len != pmdev->in_buf_len - 1) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: length mismatch. Expected %d bytes, got %d
> bytes\n",
> > +                      __func__, sent_len, pmdev->in_buf_len - 1);
> > +    }
> > +
> > +    /* exclude length byte */
> > +    pmdev->in_buf++;
> > +    pmdev->in_buf_len--;
> > +
> > +    if (pmdev->in_buf_len < len) {
> > +        len = pmdev->in_buf_len;
> > +    }
> > +    memcpy(dest, pmdev->in_buf, len);
> > +    return len;
> > +}
> > +
> >
> >  static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
> >  {
> > diff --git a/include/hw/i2c/pmbus_device.h
> b/include/hw/i2c/pmbus_device.h
> > index 93f5d57c9d..7dc00cc4d9 100644
> > --- a/include/hw/i2c/pmbus_device.h
> > +++ b/include/hw/i2c/pmbus_device.h
> > @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t
> data);
> >   */
> >  void pmbus_send_string(PMBusDevice *state, const char *data);
> >
> > +/**
> > + * @brief Receive data sent with Block Write.
> > + * @param dest - memory with enough capacity to receive the write
> > + * @param len - the capacity of dest
> > + */
> > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t
> len);
> > +
> >  /**
> >   * @brief Receive data over PMBus
> >   * These methods help track how much data is being received over PMBus
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.