On Wed, 24 Apr 2019 03:47:18 -0700
"Life is hard, and then you die" <ronald@xxxxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote:
> > On Sun, 21 Apr 2019 20:12:49 -0700
> > Ronald Tschalär <ronald@xxxxxxxxxxxxx> wrote:
> >
> > > The iBridge device provides access to several devices, including:
> > > - the Touch Bar
> > > - the iSight webcam
> > > - the light sensor
> > > - the fingerprint sensor
> > >
> > > This driver provides the core support for managing the iBridge device
> > > and the access to the underlying devices. In particular, since the
> > > functionality for the touch bar and light sensor is exposed via USB HID
> > > interfaces, and the same HID device is used for multiple functions, this
> > > driver provides a multiplexing layer that allows multiple HID drivers to
> > > be registered for a given HID device. This allows the touch bar and ALS
> > > driver to be separated out into their own modules.
> > >
> > > Signed-off-by: Ronald Tschalär <ronald@xxxxxxxxxxxxx
> > Hi Ronald,
> >
> > I've only taken a fairly superficial look at this. A few global
> > things to note though.
>
> Thanks for this review.
>
> > 1. Please either use kernel-doc style for function descriptions, or
> > do not. Right now you are sort of half way there.
>
> Apologies, on re-reading the docs I realize what you mean here. Should
> be fixed now (next rev).
>
> > 2. There is quite a complex nest of separate structures being allocated,
> > so think about whether they can be simplified. In particular
> > use of container_of macros can allow a lot of forwards and backwards
> > pointers to be dropped if you embed the various structures directly.
>
> Done (see also below).
>
> [snip]
> > > +#define call_void_driver_func(drv_info, fn, ...) \
> >
> > This sort of macro may seem like a good idea because it saves a few lines
> > of code. However, that comes at the cost of readability, so just
> > put the code inline.
> >
> > > + do { \
> > > + if ((drv_info)->driver->fn) \
> > > + (drv_info)->driver->fn(__VA_ARGS__); \
> > > + } while (0)
> > > +
> > > +#define call_driver_func(drv_info, fn, ret_type, ...) \
> > > + ({ \
> > > + ret_type rc = 0; \
> > > + \
> > > + if ((drv_info)->driver->fn) \
> > > + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> > > + \
> > > + rc; \
> > > + })
>
> Just to clarify, you're only talking about removing/inlining the
> call_void_driver_func() macro, not the call_driver_func() macro,
> right?
Both please. Neither adds much.
>
> [snip]
> > > +static struct appleib_hid_dev_info *
> > > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> > > + const struct hid_device_id *id)
> > > +{
> > > + struct appleib_hid_dev_info *dev_info;
> > > + struct appleib_hid_drv_info *drv_info;
> > > +
> > > + /* allocate device-info for this device */
> > > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> > > + if (!dev_info)
> > > + return NULL;
> > > +
> > > + INIT_LIST_HEAD(&dev_info->drivers);
> > > + dev_info->device = hdev;
> > > + dev_info->device_id = id;
> > > +
> > > + /* notify all our sub drivers */
> > > + mutex_lock(&ib_dev->update_lock);
> > > +
> > This is interesting. I'd like to see a comment here on what
> > this flag is going to do.
>
> I'm not sure I follow: update_lock is simply a mutex protecting all
> driver and device update (i.e. add/remove) functions. Are you
> therefore looking for something like:
That ended up in the wrong place...
It was the in_hid_probe just after here that I was referring to.
>
> /* protect driver and device lists against concurrent updates */
> mutex_lock(&ib_dev->update_lock);
>
> [snip]
> > > +static int appleib_probe(struct acpi_device *acpi)
> > > +{
> > > + struct appleib_device *ib_dev;
> > > + struct appleib_platform_data *pdata;
> > Platform_data has a lot of historical meaning in Linux.
> > Also you have things in here that are not platform related
> > at all, such as the dev pointer. Hence I would rename it
> > as device_data or private or something like that.
>
> Ok. I guess I called in platform_data because it's stored in the mfd
> cell's "platform_data" field. Anyway, changed it per your suggestion.
>
> > > + int i;
> > > + int ret;
> > > +
> > > + if (appleib_dev)
> > This singleton bothers me a bit. I'm really not sure why it
> > is necessary. You can just put a pointer to this in
> > the pdata for the subdevs and I think that covers most of your
> > usecases. It's generally a bad idea to limit things to one instance
> > of a device unless that actually major simplifications.
> > I'm not seeing them here.
>
> Yes, this one is quite ugly. appleib_dev is static so that
> appleib_hid_probe() can find it. I could not find any other way to
> pass the appleib_dev instance to that probe function.
>
> However, on looking at this again, I realized that hid_device_id has
> a driver_data field which can be used for this; so if I added the
> hid_driver and hid_device_id structs to the appleib_device (instead of
> making them static like now) I could fill in the driver_data and avoid
> this hack. This looks much cleaner.
>
> Thanks for pointing this uglyness out again.
>
> [snip]
> > > + if (!ib_dev->subdevs) {
> > > + ret = -ENOMEM;
> > > + goto free_dev;
> > > + }
> > > +
> > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> >
> > Might as well embed this in ib_dev as well.
>
> Agreed.
>
> > That would let
> > you used container_of to avoid having to carry the ib_dev pointer
> > around in side pdata.
>
> I see. I guess my main reservation is that the functions exported to
> the sub-drivers would now take a 'struct appleib_device_data *'
> argument instead of a 'struct appleib_device *', which just seems a
> bit unnatural. E.g.
>
> int appleib_register_hid_driver(struct appleib_device_data *ib_ddata,
> struct hid_driver *driver, void *data);
>
> instead of (the current)
>
> int appleib_register_hid_driver(struct appleib_device *ib_dev,
> struct hid_driver *driver, void *data)
I'm not totally sure I can see why. You can go from backwards and forwards
from any of the pointers...
>
> [snip]
> > > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> > > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> > > + NULL, 0, NULL);
> > > + if (ret) {
> > > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> > > + goto free_pdata;
> > > + }
> > > +
> > > + acpi->driver_data = ib_dev;
> > > + appleib_dev = ib_dev;
> > > +
> > > + ret = hid_register_driver(&appleib_hid_driver);
> > > + if (ret) {
> > > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> > > + ret);
> > > + goto rem_mfd_devs;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +rem_mfd_devs:
> > > + mfd_remove_devices(&acpi->dev);
> > > +free_pdata:
> > > + kfree(pdata);
> > > +free_subdevs:
> > > + kfree(ib_dev->subdevs);
> > > +free_dev:
> > > + appleib_dev = NULL;
> > > + acpi->driver_data = NULL;
> > Why at this point? It's not set to anything until much later in the
> > probe flow.
>
> If the hid_register_driver() call fails, we get here after driver_data
> has been assigned. However, looking at this again, acpi->driver_data
> is only used by the remove, suspend, and resume callbacks, and those
> will not be called until a successful return from probe; therefore I
> can safely move the setting of driver_data to after the
> hid_register_driver() call and avoid having to set it to NULL in the
> error cleanup.
>
> > May be worth thinking about devm_ managed allocations
> > to cleanup some of these allocations automatically and simplify
> > the error handling.
>
> Good point, thanks.
>
> [snip]
> > > +
> > > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> > > + if (ACPI_FAILURE(rc))
> > > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
> >
> > I can sort of see you might want to do the LOG_DEV for consistency
> > but here I'm fairly sure it's just dev which might be clearer.
>
> Sorry, you mean rename the macro LOG_DEV() to just DEV()?
No, just dev_warn(dev,....)
It's the same one I think at this particular location.
>
>
> Cheers,
>
> Ronald
>
