On Wed, Jun 21, 2017 at 09:49:35AM +0900, AKASHI Takahiro wrote: > On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > > > > > perhaps a light > > > internal rework inside firmware_class might be more suitable towards the > > > extensibility goal while keeping driver API usage as simple as it is today > > > (even if you hate my patch for various reasons)? > > > > > > [1] - > > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319 > > > > What you did is but one step I took, your changes make it easier to shuffle > > data around internally. Its not sufficient to clean things up well enough, > > for > > instance look at the "firmware behavior options" which are cleaned up in > > this > > patch 1/5. That's been one pain on our side for a while, people > > understanding > > when a flag applies or a feature, and making sure we document it all. > > > > Then, one of the end goals is to provide extensibility, this is to allow > > users > > to *pass* similar type of struct for either a sync or async call. Without > > this > > the API remains unflexible and I predict we'll end up with the same > > situation > > later anyway. > > > > The approach I took uses an internal struct for passing required data for > > the > > private internal API use. Then it also provides a public struct which can be > > used to grow requirements to make only *new* API easily extensible. > > > > So we need all three things to move forward. > > Given the discussions here, it would be better to split your [1/5] and > [2/5] into more smaller pieces, say, > * re-factor the old APIs with introducing private fw_desc
So patch 1/5 introduces 3 structs: o struct driver_data_req_params - used for user specified parameters o struct driver_data_priv_params - used for internal use only o struct driver_data_params - stiches both of the above together, only for internal use I could certainly split the patch to introduce each separately. > * add new APIs with extra public arguments This was split out already, patch 2 is the first patch introducing new API. > * extend new APIs per-feature > - sync/async callbacks I suppose the base would be what we have today, only in new form. And sure, I can add the features one by one... > - API version, and so on Right. > This way, you can illustrate how your approach evolves and it may > mitigate people's concern about how complicated it is on the surface, > allowing for discussing each of aspects of new APIs separately. This makes sense. Greg, does this make sense to you? Or are you stone cold against all this? Luis
