On 07/06/17 21:38, Arnd Bergmann wrote: > On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote: > >> +struct scmi_msg_resp_power_attributes { >> + __le16 num_domains; >> + __le16 reserved; >> + __le32 stats_addr_low; >> + __le32 stats_addr_high; >> + __le32 stats_size; >> +} __packed; >> + >> +struct scmi_msg_resp_power_domain_attributes { >> + __le32 flags; >> +#define SUPPORTS_STATE_SET_NOTIFY(x) ((x) & BIT(31)) >> +#define SUPPORTS_STATE_SET_ASYNC(x) ((x) & BIT(30)) >> +#define SUPPORTS_STATE_SET_SYNC(x) ((x) & BIT(29)) >> + u8 name[SCMI_MAX_STR_SIZE]; >> +} __packed; > > I think it would be better to leave out the __packed here, which > can lead to rather inefficient code. It's only really a problem when > building with -mstrict-align, but it's better to write code in a way that > doesn't rely on that. > I assume you are referring to above structure only and not general across all the structures ? I will have a look at this one. >> +static int >> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain, >> + struct power_dom_info *dom_info) >> +{ >> + int ret; >> + struct scmi_xfer *t; >> + struct scmi_msg_resp_power_domain_attributes *attr; >> + >> + ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES, >> + SCMI_PROTOCOL_POWER, sizeof(domain), >> + sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + *(__le32 *)t->tx.buf = cpu_to_le32(domain); >> + attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf; > > It seems you require a similar pattern in each caller of scmi_one_xfer_init(), > but it seems a little clumsy to always require those casts, so maybe there > is a nicer way to do this. How about making scmi_one_xfer_init() act > as an allocation function and having it return the buffer or a PTR_ERR? > Yes I agree it doesn't looks all nice. I have changed these few times while developing and then thought it's better to get some suggestions. I am open to any suggestions that will help to make these nicer. > It also seems odd to have it named 'init' but actually allocate the scmi_xfer > structure rather than filling a local variable that gets passed by reference. > It does initialise but partially. scmi_one_xfer_get does pure allocation while scmi_one_xfer_init initialise header variables and also tx/rx size. But if you think it's odd, I will looks at ways to make it better. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html