Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/9/20 3:29 PM, Benjamin Tissoires wrote:
> Hi,
> 
> sorry for the delay. I have been heavily sidetracked and have a bunch
> of internal deadlines coming in :/
> 
> On Mon, Nov 9, 2020 at 12:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/4/20 5:06 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>>> +#include "i2c-hid.h"
>>>>> +
>>>>> +struct i2c_hid_acpi {
>>>>> +     struct i2chid_subclass_data subclass;
>>>>
>>>> This feels a bit weird, we are the subclass so typically we would
>>>> be embedding a base_class data struct here ...
>>>>
>>>> (more remarks below, note just my 2 cents you may want to wait
>>>> for feedback from others).
>>>>
>>>>> +     struct i2c_client *client;
>>>>
>>>> You pass this to i2c_hid_core_probe which then stores it own
>>>> copy, why not just store it in the subclass (or even better
>>>> baseclass) data struct ?
>>>
>>> My goal was to avoid moving the big structure to the header file.
>>> Without doing that, I think you need something more like the setup I
>>> have.  I'll wait for Benjamin to comment on whether he'd prefer
>>> something like what I have here or if I should move the structure.
>>
>> Ok, if Benjamin decides to keep things this way, can you consider
>> renaming i2chid_subclass_data to i2chid_ops ?
>>
>> It just feels weird to have a struct with subclass in the name
>> embedded inside as a member in another struct, usualy the kobject model
>> works by having the the parent/base-class struct embedded inside
>> the subclass data struct.
>>
>> This also avoids the need for a callback_priv_data pointer to the ops,
>> as the ops get a pointer to the baseclass data struct as argument and
>> you can then use container_of to get your own subclassdata struct
>> since that encapsulates (contains) the baseclass struct.
>>
>> Note the dropping of the callback_priv_data pointer only works if you
>> do move the entire struct to the header.
> 
> I am not sure my opinion is the best in this case. However, the one
> thing I'd like us to do is knowing which use cases we are solving, and
> this should hopefully help us finding the best approach:
> 
> - use case 1: fully upstream driver (like this one)
>    -> the OEM sets up the DT associated with the embedded devices
>    -> the kernel is compiled with the proper flags/configs
>   -> the device works out of the box (yay!)
> 
> - use case 2: tinkerer in a garage
>   -> assembly of a generic SoC + Goodix v-next panel (that needs
> modifications in the driver)
>   -> use of a generic (arm?) distribution
>   -> the user compiles the new (changed) goodix driver
>   -> the DT is populated (with overloads)
>   -> the device works
>   -> do we want to keep compatibility across kernel versions (not
> recompile the custom module)
> 
> - use case 3: Google fixed kernel
>   -> the kernel is built once for all platforms
>   -> OEMs can recompile a few drivers if they need, but can not touch
> the core system
>   -> DT/goodix specific drivers are embedded
>   -> device works
>   -> do we want compatibility across major versions, and how "nice" we
> want to be with OEM?
> 
> I understand that use case 2 should in the end become use case 1, but
> having a possibility for casual/enthusiasts developers to fix their
> hardware is always nice.
> 
> So to me, having the base struct in an external header means we are
> adding a lot of ABI and putting a lot more weight to case 1.
> 
> Personally, I am not that much in favour of being too strict and I
> think we also want to help these external drivers. It is true that
> i2c-hid should be relatively stable from now on, but we can never
> predict the future, so maybe the external header is not so much a good
> thing (for me).
> 
> Anyway, if we were to extract the base struct, we would need to
> provide allocators to be able to keep forward compatibility (I think).
> 
> Does that help a bit?
> 
> [mode bikeshedding on]
> And to go back to Hans' suggestion, I really prefer i2chid_ops. This
> whole architecture makes me think of a bus, not a subclass hierarchy.
> In the same way we have the hid bus, we could have the i2c-hid bus,
> with separate drivers in it (acpi, of, goodix).
> 
> Note that I don't want the i2c-hid to be converted into an actual bus,
> but just rely on the concepts.
> [bikeshedding off]

Ok, so TL;DR: keep as is but rename subclass to i2chid_ops. That works
for me.

>>>>> @@ -156,10 +152,10 @@ struct i2c_hid {
>>>>>
>>>>>       wait_queue_head_t       wait;           /* For waiting the interrupt */
>>>>>
>>>>> -     struct i2c_hid_platform_data pdata;
>>>>> -
>>>>>       bool                    irq_wake_enabled;
>>>>>       struct mutex            reset_lock;
>>>>> +
>>>>> +     struct i2chid_subclass_data *subclass;
>>>>>  };
>>>>
>>>> Personally, I would do things a bit differently here:
>>>>
>>>> 1. Just add the
>>>>
>>>>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
>>>>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
>>>>
>>>> members which you put in the subclass struct here.
>>>>
>>>> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
>>>> and use this as the base-class which I described before (and store the client
>>>> pointer here).
>>>>
>>>> 3. And then kzalloc both this baseclass struct + the subclass-data
>>>> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
>>>> replacing 2 kzallocs (+ error checking with one, simplifying the code and
>>>> reducing memory fragmentation (by a tiny sliver).
>>>
>>> Sure, I'll do that if Benjamin likes moving the structure to the header.
>>>
>>>
>>>> About the power_*_device callbacks, I wonder if it would not be more consistent
>>>> to also have a shutdown callback and make i2c_driver.shutdown point to
>>>> a (modified) i2c_hid_core_shutdown() function.
>>>
>>> Personally this doesn't seem cleaner to me, but I'm happy to do it if
>>> folks like it better.  Coming up with a name for the callback would be
>>> a bit awkward, which is a sign that this isn't quite ideal?  For the
>>> power_up()/power_down() those are sane concepts to abstract out.  Here
>>> we'd be abstracting out "subclass_shutdown_tail()" or something?
>>> ...and if a subclass needs something at the head of shutdown, we'd
>>> need to add a "subclass_shutdown_head()"?
>>
>> I have no real preference here either way.
> 
> If we are using i2chid_ops, we could just have `shutdown_tail()`.
> Basically drop any "device" or "subclass" in the op name.
> This would lead to better code IMO: "ihid->dev_ops->shutdown()" for example


This also works for me.

Regards,

Hans