Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
@ 2019-10-07  7:16 Cheng-Yi Chiang
  2019-10-07  8:03 ` Tzung-Bi Shih
  0 siblings, 1 reply; 11+ messages in thread
From: Cheng-Yi Chiang @ 2019-10-07  7:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: alsa-devel, tzungbi, Greg Kroah-Hartman, Stephen Boyd,
	Hung-Te Lin, Mark Brown, Sean Paul, dgreid, Guenter Roeck,
	Cheng-Yi Chiang

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)
+{
+	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);
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(vpd_attribute_read_value);
+
 /*
  * 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07  7:16 [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value Cheng-Yi Chiang
@ 2019-10-07  8:03 ` Tzung-Bi Shih
  2019-10-07  8:52   ` Cheng-yi Chiang
  2019-10-07 12:27   ` Guenter Roeck
  0 siblings, 2 replies; 11+ messages in thread
From: Tzung-Bi Shih @ 2019-10-07  8:03 UTC (permalink / raw)
  To: Cheng-Yi Chiang
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Stephen Boyd, Hung-Te Lin, Mark Brown,
	Sean Paul, dgreid, Guenter Roeck

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)
> +{
> +       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.

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;
> +}
> +EXPORT_SYMBOL(vpd_attribute_read_value);
> +
>  /*
>   * 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07  8:03 ` Tzung-Bi Shih
@ 2019-10-07  8:52   ` Cheng-yi Chiang
  2019-10-07 12:27   ` Guenter Roeck
  1 sibling, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2019-10-07  8:52 UTC (permalink / raw)
  To: Tzung-Bi Shih
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Stephen Boyd, Hung-Te Lin, Mark Brown,
	Sean Paul, Dylan Reid, Guenter Roeck

On Mon, Oct 7, 2019 at 4:03 PM Tzung-Bi Shih <tzungbi@google.com> 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)
> > +{
> > +       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.
>
> 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.

Thanks for the review.
I will them in v2.

>
> > +                       return 0;
> > +               }
> > +       }
> > +       return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(vpd_attribute_read_value);
> > +
> >  /*
> >   * 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07  8:03 ` Tzung-Bi Shih
  2019-10-07  8:52   ` Cheng-yi Chiang
@ 2019-10-07 12:27   ` Guenter Roeck
  2019-10-07 13:58     ` Cheng-yi Chiang
  1 sibling, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2019-10-07 12:27 UTC (permalink / raw)
  To: Tzung-Bi Shih, Cheng-Yi Chiang
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Stephen Boyd, Hung-Te Lin, Mark Brown,
	Sean Paul, dgreid

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.

The API should be documented, and state clearly that the caller must release
the returned value.

>> +{
>> +       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() ?

> 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.

>> +}
>> +EXPORT_SYMBOL(vpd_attribute_read_value);
>> +

I would suggest to use EXPORT_SYMBOL_GPL().

>>   /*
>>    * 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07 12:27   ` Guenter Roeck
@ 2019-10-07 13:58     ` Cheng-yi Chiang
  2019-10-07 15:35       ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Cheng-yi Chiang @ 2019-10-07 13:58 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Stephen Boyd, Hung-Te Lin,
	Tzung-Bi Shih, Mark Brown, Sean Paul, Dylan Reid

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07 13:58     ` Cheng-yi Chiang
@ 2019-10-07 15:35       ` Stephen Boyd
  2019-10-07 18:50         ` Cheng-yi Chiang
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2019-10-07 15:35 UTC (permalink / raw)
  To: Cheng-yi Chiang, Guenter Roeck
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hung-Te Lin, Tzung-Bi Shih,
	Mark Brown, Sean Paul, Srinivas Kandagatla, Dylan Reid

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.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07 15:35       ` Stephen Boyd
@ 2019-10-07 18:50         ` Cheng-yi Chiang
  2019-10-08 15:14           ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Cheng-yi Chiang @ 2019-10-07 18:50 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hung-Te Lin, Tzung-Bi Shih,
	Mark Brown, Sean Paul, Srinivas Kandagatla, Dylan Reid,
	Guenter Roeck

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!
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-07 18:50         ` Cheng-yi Chiang
@ 2019-10-08 15:14           ` Stephen Boyd
  2019-10-09 13:37             ` Mark Brown
  2019-10-09 14:05             ` Srinivas Kandagatla
  0 siblings, 2 replies; 11+ messages in thread
From: Stephen Boyd @ 2019-10-08 15:14 UTC (permalink / raw)
  To: Cheng-yi Chiang
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hung-Te Lin, Tzung-Bi Shih,
	Mark Brown, Sean Paul, Srinivas Kandagatla, Dylan Reid,
	Guenter Roeck

Quoting Cheng-yi Chiang (2019-10-07 11:50:31)
> 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.

Maybe Srini has some input here. It looks like your main concern is
consumer to provider mapping?

> 
> 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.
> 

I'm not sure how an ACPI system like this would work because my exposure
to ACPI is extremely limited. I would expect there to be some sort of
firmware property indicating that an nvmem should be used and it's
provided by VPD or for firmware to parse VPD itself and put the
information into the ACPI table for this device.

Has either of those things been done? If it is a reference/property in
firmware then it should be possible to connect consumer devices like the
audio device you mention to VPD via the nvmem APIs with some changes to
the nvmem framework assuming there's an approach for nvmem in ACPI in
some "standard" way. 

I'd like to use nvmem for two reasons. First, it is a kernel framework
for reading non-volatile memories, which is fairly close to what VPD is
for. Second, it makes a standard, i.e. non-vpd/coreboot specific, API
that drivers can use to read memories. Of course in ASoC the machine
driver is already very platform specific, but the idea is to avoid
platform specific APIs so that drivers are loosely coupled with the rest
of the kernel. We shouldn't push against that goal by introducing more
platform specific APIs.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-08 15:14           ` Stephen Boyd
@ 2019-10-09 13:37             ` Mark Brown
  2019-10-09 14:05             ` Srinivas Kandagatla
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Brown @ 2019-10-09 13:37 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hung-Te Lin, Tzung-Bi Shih, Sean Paul,
	Srinivas Kandagatla, Dylan Reid, Guenter Roeck, Cheng-yi Chiang

[-- Attachment #1.1: Type: text/plain, Size: 1129 bytes --]

On Tue, Oct 08, 2019 at 08:14:43AM -0700, Stephen Boyd wrote:
> Quoting Cheng-yi Chiang (2019-10-07 11:50:31)

> > 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.

> I'm not sure how an ACPI system like this would work because my exposure
> to ACPI is extremely limited. I would expect there to be some sort of
> firmware property indicating that an nvmem should be used and it's
> provided by VPD or for firmware to parse VPD itself and put the
> information into the ACPI table for this device.

I fear this is optimistic.  It's fairly idiomatic for ACPI for stuff
like this to be keyed off DMI information rather than integrated with
anything, basically once you get out of the bits that are explictly
standardized you're into board file territory.  There is the _DSD stuff
for using DT properties in ACPI but it's had limited use AFAICT.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-08 15:14           ` Stephen Boyd
  2019-10-09 13:37             ` Mark Brown
@ 2019-10-09 14:05             ` Srinivas Kandagatla
  2019-10-14  3:21               ` Cheng-yi Chiang
  1 sibling, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-10-09 14:05 UTC (permalink / raw)
  To: Stephen Boyd, Cheng-yi Chiang
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hung-Te Lin, Tzung-Bi Shih,
	Mark Brown, Sean Paul, Dylan Reid, Guenter Roeck



On 08/10/2019 16:14, Stephen Boyd wrote:
>> 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.
> Maybe Srini has some input here. It looks like your main concern is
> consumer to provider mapping?
> 

In non-DT setup, there are various ways to lookup nvmem provider.

1> nvmem_device_get()/put() using provider devid/name. I think you 
should be able to use this in your case.
2> nvmem_register_notifier() which notifies when nvmem provider is added 
to system.
3> nvmem_device_find() with own match function this will be merged in 
next window (https://lkml.org/lkml/2019/10/3/215)


If none of these are of any help, could explain what exactly are you 
looking for w.r.t nvmem to be able to move to what Stephen Boyd suggested?

--srini

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value
  2019-10-09 14:05             ` Srinivas Kandagatla
@ 2019-10-14  3:21               ` Cheng-yi Chiang
  0 siblings, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2019-10-14  3:21 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: ALSA development, Tzung-Bi Shih, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Stephen Boyd, Hung-Te Lin,
	Tzung-Bi Shih, Mark Brown, Sean Paul, Dylan Reid, Guenter Roeck

On Wed, Oct 9, 2019 at 10:05 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 08/10/2019 16:14, Stephen Boyd wrote:
> >> 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.
> > Maybe Srini has some input here. It looks like your main concern is
> > consumer to provider mapping?
> >
>
> In non-DT setup, there are various ways to lookup nvmem provider.
>
> 1> nvmem_device_get()/put() using provider devid/name. I think you
> should be able to use this in your case.
> 2> nvmem_register_notifier() which notifies when nvmem provider is added
> to system.
> 3> nvmem_device_find() with own match function this will be merged in
> next window (https://lkml.org/lkml/2019/10/3/215)
>
>
> If none of these are of any help, could explain what exactly are you
> looking for w.r.t nvmem to be able to move to what Stephen Boyd suggested?
>
> --srini
>

Hi Stephen, Mark and Srinivas,
Thank you all for the suggestions.
In my non-DT setup, I have been working on coreboot changes to prepare
data in _DSD following suggestion in
https://patchwork.kernel.org/patch/11179237
The basic idea is that codec driver should just get data it needs from
device property.
The coreboot approach works in my local setup, but I have not sent the
change to coreboot upstream yet.
If that path work, then the change needed in kernel will be much simpler.

In the future, there might be DT setup use case, and I think it should
be doable for VPD to register a nvmem device, and let codec driver
gets the property.
But I would drop this path for now.
Thanks!
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07  7:16 [alsa-devel] [PATCH] firmware: vpd: Add an interface to read VPD value Cheng-Yi Chiang
2019-10-07  8:03 ` Tzung-Bi Shih
2019-10-07  8:52   ` Cheng-yi Chiang
2019-10-07 12:27   ` Guenter Roeck
2019-10-07 13:58     ` Cheng-yi Chiang
2019-10-07 15:35       ` Stephen Boyd
2019-10-07 18:50         ` Cheng-yi Chiang
2019-10-08 15:14           ` Stephen Boyd
2019-10-09 13:37             ` Mark Brown
2019-10-09 14:05             ` Srinivas Kandagatla
2019-10-14  3:21               ` Cheng-yi Chiang

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org alsa-devel@archiver.kernel.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox