On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> +/**
> + * struct scmi_msg_hdr - Message(Tx/Rx) header
> + *
> + * @id: The identifier of the command being sent
> + * @protocol_id: The identifier of the protocol used to send @id command
> + * @seq: The token to identify the message. when a message/command returns,
> + * the platform returns the whole message header unmodified including
> + * the token.
> + */
> +struct scmi_msg_hdr {
> + u8 id;
> + u8 protocol_id;
> + u16 seq;
> + u32 status;
> + bool poll_completion;
> +};
Is this structure part of the protocol, or just part of the linux
implementation?
If this is in the protocol, you should not have a 'bool' member in there, which
does not have a well-defined binary representation across architectures.
> +/*
> + * The SCP firmware providing SCM interface to OSPM and other agents must
> + * execute only in little-endian mode as per SCMI specification, so any buffers
> + * shared through SCMI should have their contents converted to little-endian
> + */
That is a very odd thing to put into a specification, are you sure it requires
a specific runtime endian-mode? I would bet that it only requires the protocol
to use little-endian data, so better describe it like that.
> +struct scmi_shared_mem {
> + __le32 reserved;
> + __le32 channel_status;
> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
> + __le32 reserved1[2];
> + __le32 flags;
> +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
> + __le32 length;
> + __le32 msg_header;
> + u8 msg_payload[0];
> +};
> +
> +static int scmi_linux_errmap[] = {
> + /* better than switch case as long as return value is continuous */
> + 0, /* SCMI_SUCCESS */
> + -EOPNOTSUPP, /* SCMI_ERR_SUPPORT */
> + -EINVAL, /* SCMI_ERR_PARAM */
> + -EACCES, /* SCMI_ERR_ACCESS */
> + -ENOENT, /* SCMI_ERR_ENTRY */
> + -ERANGE, /* SCMI_ERR_RANGE */
> + -EBUSY, /* SCMI_ERR_BUSY */
> + -ECOMM, /* SCMI_ERR_COMMS */
> + -EIO, /* SCMI_ERR_GENERIC */
> + -EREMOTEIO, /* SCMI_ERR_HARDWARE */
> + -EPROTO, /* SCMI_ERR_PROTOCOL */
> +};
maybe make this 'const'.
> +static struct platform_driver scmi_driver = {
> + .driver = {
> + .name = "arm-scmi",
> + .of_match_table = of_match_ptr(scmi_of_match),
> + },
> + .probe = scmi_probe,
> + .remove = scmi_remove,
> +};
The 'of_match_ptr' annotation probably causes an 'unused variable'
warning, better just drop that.
> +#if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL)
> +int scmi_handle_put(const struct scmi_handle *handle);
> +const struct scmi_handle *scmi_handle_get(struct device *dev);
> +const struct scmi_handle *devm_scmi_handle_get(struct device *dev);
IS_REACHABLE() can easily lead to confusion when the driver is
a loadable module but never gets used by a built-in driver. Maybe use
IS_ENABLED() here, and add a Kconfig symbol that other drivers
can depend on if you want them to optionally use it, like:
config MAYBE_ARM_SCMI_PROTOCOL
default y if ARM_SCMI_PROTOCOL=n
default ARM_SCMI_PROTOCOL
Arnd
--
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
