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?
[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:
/* 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);
[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()?
Cheers,
Ronald
