Re: [PATCH v3] clk: zynqmp: use structs for clk query responses

[Stephen Boyd <sboyd@kernel.org>]

Quoting Michael Tretter (2019-04-23 06:54:18)
> On Fri, 19 Apr 2019 14:22:19 -0700, Stephen Boyd wrote:
> > Also, I was hoping to see the patch go further and make structures
> > instead of doing FIELD_GET on u32s. For example:
> > 
> > #define CLK_PARENTS_ID                  GENMASK(15, 0)
> > #define CLK_PARENTS_FLAGS               GENMASK(31, 16)
> > 
> > This could be a struct of two u16 in them (and quite possibly __le16).
> > 
> >       u16 id;
> >       u16 flags;
> 
> The error codes also do not fit into this scheme. For example, if no
> further parents are available, the response will contain the value
> 0xFFFFFFFF in the response. The value may occur at any position in the
> parents array and should not be interpreted as a struct with two u16 in
> it, but should be seen as a single value on its own.
> 
> Therefore, I think the abstraction using a u32[] and bitfields is better
> than defining a struct for unmarshalling the response.

We have union for that?

	union {
		struct resp {
			u16 id;
			u16 flags;
		};
		u32 error;
	};

> 
> > 
> > with __packed on it. This one is weird though:
> > 
> > #define CLK_ATTR_NODE_INDEX             GENMASK(13, 0)
> > #define CLK_ATTR_NODE_TYPE              GENMASK(19, 14)
> > #define CLK_ATTR_NODE_SUBCLASS          GENMASK(25, 20)
> > #define CLK_ATTR_NODE_CLASS             GENMASK(31, 26)
> > 
> > The bits are really packed in there. I guess we can't win this one.
> 
> Actually, the CLK_ATTR_NODE_INDEX is not a field in the response, but a
> field in the clock id. The driver needs to use the index from the query
> to set the index field in the clock id. Only the bits 2 and 0 in bits
> 13..0 carry actual meaning.
> 
> Not sure if I should amend this patch or have a separate patch for
> that, because this is in fact an issue in the "drivers: clk: Update
> clock driver to handle clock attribute" patch.
> 

I'd just send a follow-up patch if something is wrong.