Date: Fri, 23 Jun 2017 18:33:23 +0200
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: AKASHI Takahiro <takahiro.akashi@linaro.org>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>,
        Vikram Mulukutla <markivx@codeaurora.org>,
        Greg KH <gregkh@linuxfoundation.org>, wagi@monom.org,
        dwmw2@infradead.org, rafal@milecki.pl, arend.vanspriel@broadcom.com,
        rjw@rjwysocki.net, yi1.li@linux.intel.com, atull@opensource.altera.com,
        moritz.fischer@ettus.com, pmladek@suse.com, johannes.berg@intel.com,
        emmanuel.grumbach@intel.com, luciano.coelho@intel.com,
        kvalo@codeaurora.org, luto@kernel.org, torvalds@linux-foundation.org,
        keescook@chromium.org, dhowells@redhat.com, pjones@redhat.com,
        hdegoede@redhat.com, alan@linux.intel.com, tytso@mit.edu,
        linux-kernel@vger.kernel.org, linux-kernel-owner@vger.kernel.org
Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params
Message-ID: <20170623163323.GM21846@wotan.suse.de>
References: <20170605213314.GR8951@wotan.suse.de>
 <20170605213937.26215-1-mcgrof@kernel.org>
 <20170605213937.26215-2-mcgrof@kernel.org>
 <20170613090548.GA31421@kroah.com>
 <20170613194011.GI27288@wotan.suse.de>
 <20170617193815.GI2974@kroah.com>
 <20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org>
 <20170620172219.GQ21846@wotan.suse.de>
 <20170621004934.GH4820@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170621004934.GH4820@linaro.org>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2741
Lines: 63

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