On Mon, Oct 7, 2019 at 8:27 PM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel