On Tue, Apr 12, 2016 at 02:43:36PM +0300, Heikki Krogerus wrote:
> + ucsi.usb_data_role=
Ick, this isn't the 1990's anymore, please don't use module parameters
for anything that is required for a driver. Make it automatic if at all
possible, and if not, then make it at the least, a per-device attribute.
> +struct ucsi {
> + struct device *dev;
> + struct ucsi_data __iomem *data;
> +
> + int status;
> + struct completion complete;
> + struct ucsi_capability cap;
> + struct ucsi_connector *connector;
> +
> + struct mutex ppm_lock;
What does this lock? Document it please.
> + atomic_t event_pending;
Your atomic_t is a poor-man's lock, I don't see why it's even needed.
If you feel you need to keep it around, you better document the heck out
of it proving that you used it correctly everywhere (hint, I saw a few
places it was not used correctly...)
> +};
> +
> +static char data_role[7];
> +module_param_string(usb_data_role, data_role, sizeof(data_role), 0644);
> +MODULE_PARM_DESC(usb_data_role, " USB Data Role - host or device");
Again, don't use module parameters.
> +static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
> +{
> + struct ucsi *ucsi = data;
> + u32 cci = ucsi->data->cci;
> +
> + dev_dbg(ucsi->dev, "%s: cci 0x%x\n", __func__, cci);
Delete all of your debugging code please, that's what ftrace is for.
> + if (!atomic_read(&ucsi->event_pending)) {
> + atomic_inc(&ucsi->event_pending);
Oh look, a bug :(
Again, document it if you think you need it, but really, I think you can
get rid of it entirely.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
