by Hans de Goede

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]> 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

2020-11-09 21:50:36

by Doug Anderson

[permalink] [raw]

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

Hi,

On Mon, Nov 9, 2020 at 6:44 AM Hans de Goede <[email protected]> wrote:
>
> 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 <[email protected]> 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 <[email protected]> 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.

Done in v5.

> >>>>> @@ -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.

I've done this part and called the callback "shutdown_tail()", which I
think was what was agreed upon above. If you want me to rename it to
"shutdown()" I can always send a v6.

NOTE: I haven't added a patch that makes shutdown do a "power off" by
default. That seems like it should wait until there's a use case
showing that it helps with something.

-Doug