Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191007071610.65714-1-cychiang@chromium.org> <CA+Px+wWkr1xmSpgEkSaGS7UZu8TKUYvSnbjimBRH29=kDtcHKA@mail.gmail.com>
 <ebf9bc3f-a531-6c5b-a146-d80fe6c5d772@roeck-us.net> <CAFv8NwLuYKHJoG9YR3WvofwiMnXCgYv-Sk7t5jCvTZbST+Ctjw@mail.gmail.com>
 <5d9b5b3e.1c69fb81.7203c.1215@mx.google.com>
In-Reply-To: <5d9b5b3e.1c69fb81.7203c.1215@mx.google.com>
From:   Cheng-yi Chiang <cychiang@chromium.org>
Date:   Tue, 8 Oct 2019 02:50:31 +0800
Message-ID: <CAFv8Nw+x6V-995ijyws1Q36W1MpaP=kNJeiVtNakH-uC3Vgg9Q@mail.gmail.com>
Subject: Re: [PATCH] firmware: vpd: Add an interface to read VPD value
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Tzung-Bi Shih <tzungbi@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        ALSA development <alsa-devel@alsa-project.org>,
        Hung-Te Lin <hungte@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sean Paul <seanpaul@chromium.org>,
        Mark Brown <broonie@kernel.org>,
        Dylan Reid <dgreid@chromium.org>,
        Tzung-Bi Shih <tzungbi@chromium.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Oct 7, 2019 at 11:35 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Cheng-yi Chiang (2019-10-07 06:58:41)
> >
> > Hi Guenter,
> > Thanks for the quick review.
> > I'll update accordingly in v2.
>
> I'd prefer this use the nvmem framework which already handles many of
> the requirements discussed here. Implement an nvmem provider and figure
> out how to wire that up to the kernel users. Also, please include a user
> of the added support, otherwise it is impossible to understand how this
> code is used.
>
Hi Stephen,
Thanks for the suggestion.
My usage is for Intel machine driver to read a string for speaker calibration.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1838091/4/sound/soc/intel/boards/cml_rt1011_rt5682.c#325

Based on the comments in this thread, its usage would look like

#define DSM_CALIB_KEY "dsm_calib"
static int load_calibration_data(struct cml_card_private *ctx) {
          char *data = NULL;
          int ret;
          u32 value_len;

          /* Read calibration data from VPD. */
          ret = vpd_attribute_read(1, DSM_CALIB_KEY,
                                         (u8 **)&data, &value_len);

          /* Parsing of this string...*/
}

It is currently pending on unmerged machine driver cml_rt1011_rt5682.c
in ASoC so I can not post it for review for now.

As for nvmem approach, I looked into examples of nvmem usage, and have
a rough idea how to do this.

1) In vpd.c, as it parses key and value in the VPD section, add nvmem cell  with
{
.name=key,
.offset=consumed,   // need some change in vpd_decodec.c to get the
offset of value in the section.
.bytes=value
}
Implement read function with vpd_section as context.

2) In vpd.c, register an nvm_device using devm_nvmem_register in
coreboot_driver's probe function vpd_probe.

3) As my use case does not use device tree, it is hard for ASoC
machine to access nvmem device. I am wondering if I can use
nvm_cell_lookup so machine driver can find the nvmem device using a
con_id. But currently the cell lookup API requires a matched device,
which does not fit my usage because there will be different machine
drivers requesting the value.
I think I can still workaround this by adding the lookup table in
machine driver. This would seem to be a bit weird because I found that
most lookup table is added in provider side, not consumer side. Not
sure if this is logically correct.

IMO the nvmem approach would create more complexity to support this
simple usage. Plus, the underlying assumption of accessing data with
offset in a buffer does not fit well with the already parsed VPD
values in a list of vpd_attrib_info. But if you strongly feel that
this is a better approach I can work toward this.

Thanks!