Re: [PATCH v9 1/5] firmware: add extensible driver data params

["Luis R. Rodriguez" <mcgrof@kernel.org>]

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