Hi,

On Mon, Nov 9, 2020 at 6:44 AM Hans de Goede [off-list ref] wrote:

Hi,

On 11/9/20 3:29 PM, Benjamin Tissoires wrote:

quoted

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 [off-list ref] wrote:

quoted

Hi,

On 11/4/20 5:06 PM, Doug Anderson wrote:

quoted

Hi,

On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede [off-list ref] wrote:

quoted

quoted

+#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).

quoted

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

quoted

quoted

quoted

quoted

quoted

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

quoted

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