From: Cheng-yi Chiang <cychiang@chromium.org> To: Guenter Roeck <linux@roeck-us.net> Cc: 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>, Stephen Boyd <swboyd@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> Subject: Re: [PATCH] firmware: vpd: Add an interface to read VPD value Date: Mon, 7 Oct 2019 21:58:41 +0800 [thread overview] Message-ID: <CAFv8NwLuYKHJoG9YR3WvofwiMnXCgYv-Sk7t5jCvTZbST+Ctjw@mail.gmail.com> (raw) In-Reply-To: <ebf9bc3f-a531-6c5b-a146-d80fe6c5d772@roeck-us.net> On Mon, Oct 7, 2019 at 8:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/7/19 1:03 AM, Tzung-Bi Shih wrote: > > On Mon, Oct 7, 2019 at 3:16 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > >> > >> Add an interface for other driver to query VPD value. > >> This will be used for ASoC machine driver to query calibration > >> data stored in VPD for smart amplifier speaker resistor > >> calibration. > >> > >> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> > >> --- > >> drivers/firmware/google/vpd.c | 16 ++++++++++++++++ > >> include/linux/firmware/google/google_vpd.h | 18 ++++++++++++++++++ > >> 2 files changed, 34 insertions(+) > >> create mode 100644 include/linux/firmware/google/google_vpd.h > >> > >> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c > >> index db0812263d46..71e9d2da63be 100644 > >> --- a/drivers/firmware/google/vpd.c > >> +++ b/drivers/firmware/google/vpd.c > >> @@ -65,6 +65,22 @@ static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp, > >> info->bin_attr.size); > >> } > >> > >> +int vpd_attribute_read_value(bool ro, const char *key, > >> + char **value, u32 value_len) > > FWIW, I don't think the "_value" in this function name adds any value, > unless there is going to be some other read function. ACK > > The API should be documented, and state clearly that the caller must release > the returned value. ACK > > >> +{ > >> + struct vpd_attrib_info *info; > >> + struct vpd_section *sec = ro ? &ro_vpd : &rw_vpd; > >> + > >> + list_for_each_entry(info, &sec->attribs, list) { > >> + if (strcmp(info->key, key) == 0) { > >> + *value = kstrndup(info->value, value_len, GFP_KERNEL); > > > > Value is not necessary a NULL-terminated string. > > kmalloc(info->bin_attr.size) and memcpy(...) would make the most > > sense. > > > kmemdup() ? ACK > > > The value_len parameter makes less sense. It seems the caller knows > > the length of the value in advance. > > Suggest to change the value_len to report the length of value. I.e. > > *value_len = info->bin_attr.size; > > > Also please check the return value for memory allocation-like > > functions (e.g. kstrndup, kmalloc) so that *value won't be NULL but > > the function returned 0. > > > >> + return 0; > >> + } > >> + } > >> + return -EINVAL; > > Maybe something like -ENOENT would be more appropriate here. > ACK > >> +} > >> +EXPORT_SYMBOL(vpd_attribute_read_value); > >> + > > I would suggest to use EXPORT_SYMBOL_GPL(). > ACK Hi Guenter, Thanks for the quick review. I'll update accordingly in v2. > >> /* > >> * vpd_section_check_key_name() > >> * > >> diff --git a/include/linux/firmware/google/google_vpd.h b/include/linux/firmware/google/google_vpd.h > >> new file mode 100644 > >> index 000000000000..6f1160f28af8 > >> --- /dev/null > >> +++ b/include/linux/firmware/google/google_vpd.h > >> @@ -0,0 +1,18 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * Google VPD interface. > >> + * > >> + * Copyright 2019 Google Inc. > >> + */ > >> + > >> +/* Interface for reading VPD value on Chrome platform. */ > >> + > >> +#ifndef __GOOGLE_VPD_H > >> +#define __GOOGLE_VPD_H > >> + > >> +#include <linux/types.h> > >> + > >> +int vpd_attribute_read_value(bool ro, const char *key, > >> + char **value, u32 value_len); > >> + > >> +#endif /* __GOOGLE_VPD_H */ > >> -- > >> 2.23.0.581.g78d2f28ef7-goog > >> > > >
WARNING: multiple messages have this Message-ID (diff)
From: Cheng-yi Chiang <cychiang@chromium.org> To: Guenter Roeck <linux@roeck-us.net> Cc: ALSA development <alsa-devel@alsa-project.org>, Tzung-Bi Shih <tzungbi@chromium.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org>, Hung-Te Lin <hungte@chromium.org>, Tzung-Bi Shih <tzungbi@google.com>, Mark Brown <broonie@kernel.org>, Sean Paul <seanpaul@chromium.org>, Dylan Reid <dgreid@chromium.org> Subject: Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value Date: Mon, 7 Oct 2019 21:58:41 +0800 [thread overview] Message-ID: <CAFv8NwLuYKHJoG9YR3WvofwiMnXCgYv-Sk7t5jCvTZbST+Ctjw@mail.gmail.com> (raw) In-Reply-To: <ebf9bc3f-a531-6c5b-a146-d80fe6c5d772@roeck-us.net> On Mon, Oct 7, 2019 at 8:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/7/19 1:03 AM, Tzung-Bi Shih wrote: > > On Mon, Oct 7, 2019 at 3:16 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > >> > >> Add an interface for other driver to query VPD value. > >> This will be used for ASoC machine driver to query calibration > >> data stored in VPD for smart amplifier speaker resistor > >> calibration. > >> > >> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> > >> --- > >> drivers/firmware/google/vpd.c | 16 ++++++++++++++++ > >> include/linux/firmware/google/google_vpd.h | 18 ++++++++++++++++++ > >> 2 files changed, 34 insertions(+) > >> create mode 100644 include/linux/firmware/google/google_vpd.h > >> > >> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c > >> index db0812263d46..71e9d2da63be 100644 > >> --- a/drivers/firmware/google/vpd.c > >> +++ b/drivers/firmware/google/vpd.c > >> @@ -65,6 +65,22 @@ static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp, > >> info->bin_attr.size); > >> } > >> > >> +int vpd_attribute_read_value(bool ro, const char *key, > >> + char **value, u32 value_len) > > FWIW, I don't think the "_value" in this function name adds any value, > unless there is going to be some other read function. ACK > > The API should be documented, and state clearly that the caller must release > the returned value. ACK > > >> +{ > >> + struct vpd_attrib_info *info; > >> + struct vpd_section *sec = ro ? &ro_vpd : &rw_vpd; > >> + > >> + list_for_each_entry(info, &sec->attribs, list) { > >> + if (strcmp(info->key, key) == 0) { > >> + *value = kstrndup(info->value, value_len, GFP_KERNEL); > > > > Value is not necessary a NULL-terminated string. > > kmalloc(info->bin_attr.size) and memcpy(...) would make the most > > sense. > > > kmemdup() ? ACK > > > The value_len parameter makes less sense. It seems the caller knows > > the length of the value in advance. > > Suggest to change the value_len to report the length of value. I.e. > > *value_len = info->bin_attr.size; > > > Also please check the return value for memory allocation-like > > functions (e.g. kstrndup, kmalloc) so that *value won't be NULL but > > the function returned 0. > > > >> + return 0; > >> + } > >> + } > >> + return -EINVAL; > > Maybe something like -ENOENT would be more appropriate here. > ACK > >> +} > >> +EXPORT_SYMBOL(vpd_attribute_read_value); > >> + > > I would suggest to use EXPORT_SYMBOL_GPL(). > ACK Hi Guenter, Thanks for the quick review. I'll update accordingly in v2. > >> /* > >> * vpd_section_check_key_name() > >> * > >> diff --git a/include/linux/firmware/google/google_vpd.h b/include/linux/firmware/google/google_vpd.h > >> new file mode 100644 > >> index 000000000000..6f1160f28af8 > >> --- /dev/null > >> +++ b/include/linux/firmware/google/google_vpd.h > >> @@ -0,0 +1,18 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * Google VPD interface. > >> + * > >> + * Copyright 2019 Google Inc. > >> + */ > >> + > >> +/* Interface for reading VPD value on Chrome platform. */ > >> + > >> +#ifndef __GOOGLE_VPD_H > >> +#define __GOOGLE_VPD_H > >> + > >> +#include <linux/types.h> > >> + > >> +int vpd_attribute_read_value(bool ro, const char *key, > >> + char **value, u32 value_len); > >> + > >> +#endif /* __GOOGLE_VPD_H */ > >> -- > >> 2.23.0.581.g78d2f28ef7-goog > >> > > > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-10-07 13:59 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-07 7:16 [PATCH] firmware: vpd: Add an interface to read VPD value Cheng-Yi Chiang 2019-10-07 7:16 ` [alsa-devel] " Cheng-Yi Chiang 2019-10-07 8:03 ` Tzung-Bi Shih 2019-10-07 8:03 ` [alsa-devel] " Tzung-Bi Shih 2019-10-07 8:52 ` Cheng-yi Chiang 2019-10-07 8:52 ` [alsa-devel] " Cheng-yi Chiang 2019-10-07 12:27 ` Guenter Roeck 2019-10-07 12:27 ` [alsa-devel] " Guenter Roeck 2019-10-07 13:58 ` Cheng-yi Chiang [this message] 2019-10-07 13:58 ` Cheng-yi Chiang 2019-10-07 15:35 ` Stephen Boyd 2019-10-07 15:35 ` [alsa-devel] " Stephen Boyd 2019-10-07 18:50 ` Cheng-yi Chiang 2019-10-07 18:50 ` [alsa-devel] " Cheng-yi Chiang 2019-10-08 15:14 ` Stephen Boyd 2019-10-08 15:14 ` [alsa-devel] " Stephen Boyd 2019-10-09 13:37 ` Mark Brown 2019-10-09 13:37 ` [alsa-devel] " Mark Brown 2019-10-09 14:05 ` Srinivas Kandagatla 2019-10-09 14:05 ` [alsa-devel] " Srinivas Kandagatla 2019-10-14 3:21 ` Cheng-yi Chiang 2019-10-14 3:21 ` [alsa-devel] " Cheng-yi Chiang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAFv8NwLuYKHJoG9YR3WvofwiMnXCgYv-Sk7t5jCvTZbST+Ctjw@mail.gmail.com \ --to=cychiang@chromium.org \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=dgreid@chromium.org \ --cc=gregkh@linuxfoundation.org \ --cc=hungte@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=seanpaul@chromium.org \ --cc=swboyd@chromium.org \ --cc=tzungbi@chromium.org \ --cc=tzungbi@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.