Re: [cdi-devel] [PATCH v2 2/9] cdi/misc: Add CDI_UPCAST and endianness functions

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Thu, Apr 16, 2015 at 12:13:51AM +0200, Max Reitz wrote:
> + Type inheritance is a common pattern seen in CDI. The new CDI_UPCAST()
>   macro makes conversion to superclasses safer and allows for multiple
>   inheritance if the need arises.
> + Add some endianness conversion functions. Even though support for big
>   endian platforms is nowhere in sight, endianness conversion is still a
>   common operation which should not be implemented in the driver itself
>   but provided by the CDI implementation.
> 
> Signed-off-by: Max Reitz <max@xxxxxxxxxx>
> ---
>  include/cdi/misc.h | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/include/cdi/misc.h b/include/cdi/misc.h
> index 07967ec..3193505 100644
> --- a/include/cdi/misc.h
> +++ b/include/cdi/misc.h
> @@ -85,6 +85,84 @@ int cdi_ioports_free(uint16_t start, uint16_t count);
>   */
>  void cdi_sleep_ms(uint32_t ms);
>  
> +/**
> + * Casts @object (must be a pointer) to a pointer to an object of type @to which
> + * must be a baseclass of @object's type. @field is the entry in @to which
> + * contains @object.
> + *
> + * Example:
> + * struct cdi_driver *driver;
> + * struct cdi_storage_driver *storage_driver =
> + *     CDI_UPCAST(driver, drv, struct cdi_storage_driver);
> + */
> +#define CDI_UPCAST(object, field, to) \
> +    (to *)((uintptr_t)object - (uintptr_t)&((to *)0)->field)

With calculations on char* instead of uintptr_t, I think the result
would actually be defined.

container_of() used in Linux (and qemu) has a different argument order
(object, to, field), perhaps worth following suit? It also checks type
compatibility between object and to->field, which could perhaps be
portably achieved with comparing both pointers and the throwing away the
result using the comma operator.

> +
> +/**
> + * Functions for endianness conversion. Be aware that most if not all CDI
> + * drivers only support little endian machines as a host.
> + */
> +
> +static inline uint16_t cdi_be16_to_cpu(uint16_t x)
> +{
> +    return (x >> 8) | (x << 8);
> +}
> +
> +static inline uint32_t cdi_be32_to_cpu(uint32_t x)
> +{
> +    return ((uint32_t)cdi_be16_to_cpu(x) << 16) | (uint32_t)cdi_be16_to_cpu(x >> 16);
> +}
> +
> +static inline uint64_t cdi_be64_to_cpu(uint64_t x)
> +{
> +    return ((uint64_t)cdi_be32_to_cpu(x) << 32) | (uint64_t)cdi_be32_to_cpu(x >> 32);
> +}
> +
> +static inline uint16_t cdi_cpu_to_be16(uint16_t x)
> +{
> +    return cdi_be16_to_cpu(x);
> +}
> +
> +static inline uint32_t cdi_cpu_to_be32(uint32_t x)
> +{
> +    return cdi_be32_to_cpu(x);
> +}
> +
> +static inline uint64_t cdi_cpu_to_be64(uint64_t x)
> +{
> +    return cdi_be64_to_cpu(x);
> +}
> +
> +static inline uint16_t cdi_le16_to_cpu(uint16_t x)
> +{
> +    return x;
> +}
> +
> +static inline uint32_t cdi_le32_to_cpu(uint32_t x)
> +{
> +    return x;
> +}
> +
> +static inline uint64_t cdi_le64_to_cpu(uint64_t x)
> +{
> +    return x;
> +}
> +
> +static inline uint16_t cdi_cpu_to_le16(uint16_t x)
> +{
> +    return cdi_le16_to_cpu(x);
> +}
> +
> +static inline uint32_t cdi_cpu_to_le32(uint32_t x)
> +{
> +    return cdi_le32_to_cpu(x);
> +}
> +
> +static inline uint64_t cdi_cpu_to_le64(uint64_t x)
> +{
> +    return cdi_le64_to_cpu(x);
> +}

The endianess functions look good (enough for now).

Kevin