On Tue, Apr 12, 2016 at 06:12:04AM -0700, Greg KH wrote:
> 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.
OK, makes sense.
> > +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.
Will do.
> > + 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...)
You are correct, there is no use for it. I'll get rid of it.
> > +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.
OK.
> > + 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.
Yes, I'll get rid of it.
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
