lkml.org 
[lkml]   [2017]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol
From
Date


On 07/06/17 21:38, Arnd Bergmann wrote:
> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@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.

>> +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

\
 
 \ /
  Last update: 2017-06-12 01:25    [W:0.094 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site