On Thu, Jun 8, 2017 at 11:39 AM, Sudeep Holla <sudeep.ho...@arm.com> wrote:
>
>
> On 07/06/17 21:38, Arnd Bergmann wrote:
>> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.ho...@arm.com> 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.
I meant all of them, from my first look they all seem to have natural
alignment on all members anyway. If there is one that doesn't, I would
suggest annotating the individual unaligned members with __packed.
>>> +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.
Yes, I'm still thinking about it, but I think we can do better. If a function
has both allocation and initialization parts in it, I would probably name
it *_alloc() rather than *_init().
What is the relation between scmi_one_xfer_get() and
scmi_one_xfer_init()? Do we need both in some callers, or
just one of the two?
Arnd
