diff mbox series

[3/5] sysinfo: Require that sysinfo_detect be called before other methods

Message ID 20210301204603.2730666-4-sean.anderson@seco.com
State Superseded
Delegated to: Tom Rini
Headers show
Series sysinfo: Add gpio sysinfo driver | expand

Commit Message

Sean Anderson March 1, 2021, 8:46 p.m. UTC 
This updates sysinfo documentation to document that detect() must be
called first. This allows drivers to cache information in detect() and
perform (cheaper) retrieval in the other accessors. This also modifies
the only instance where this sequencing was not followed.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 common/spl/spl_fit.c |  4 ++++
 include/sysinfo.h    | 29 ++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

Comments

Simon Glass March 5, 2021, 4:08 a.m. UTC | #1 
Hi Sean,

On Mon, 1 Mar 2021 at 15:46, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This updates sysinfo documentation to document that detect() must be
> called first. This allows drivers to cache information in detect() and
> perform (cheaper) retrieval in the other accessors. This also modifies
> the only instance where this sequencing was not followed.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  common/spl/spl_fit.c |  4 ++++
>  include/sysinfo.h    | 29 ++++++++++++++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Is it possible to enforce this in the uclass? Also I think -EPERM or
-EACCES might be better than -ENOENT.
Sean Anderson March 5, 2021, 3:12 p.m. UTC | #2 
On 3/4/21 11:08 PM, Simon Glass wrote:
 > Hi Sean,
 >
 > On Mon, 1 Mar 2021 at 15:46, Sean Anderson <sean.anderson@seco.com> wrote:
 >>
 >> This updates sysinfo documentation to document that detect() must be
 >> called first. This allows drivers to cache information in detect() and
 >> perform (cheaper) retrieval in the other accessors. This also modifies
 >> the only instance where this sequencing was not followed.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>   common/spl/spl_fit.c |  4 ++++
 >>   include/sysinfo.h    | 29 ++++++++++++++++++++---------
 >>   2 files changed, 24 insertions(+), 9 deletions(-)
 >
 > Reviewed-by: Simon Glass <sjg@chromium.org>
 >
 > Is it possible to enforce this in the uclass?

So have the uclass call detect() if it hasn't been called? Should there
be a way to re-detect?

 > Also I think -EPERM or -EACCES might be better than -ENOENT.

Hm, I thought I saw gazerbeam using ENOENT for this purpose, but on
further review it looks like it does not. I think EPERM is good for
this.

--Sean
Simon Glass March 5, 2021, 4:39 p.m. UTC | #3 
Hi Sean,

On Fri, 5 Mar 2021 at 08:12, Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 3/4/21 11:08 PM, Simon Glass wrote:
>  > Hi Sean,
>  >
>  > On Mon, 1 Mar 2021 at 15:46, Sean Anderson <sean.anderson@seco.com> wrote:
>  >>
>  >> This updates sysinfo documentation to document that detect() must be
>  >> called first. This allows drivers to cache information in detect() and
>  >> perform (cheaper) retrieval in the other accessors. This also modifies
>  >> the only instance where this sequencing was not followed.
>  >>
>  >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>  >> ---
>  >>
>  >>   common/spl/spl_fit.c |  4 ++++
>  >>   include/sysinfo.h    | 29 ++++++++++++++++++++---------
>  >>   2 files changed, 24 insertions(+), 9 deletions(-)
>  >
>  > Reviewed-by: Simon Glass <sjg@chromium.org>
>  >
>  > Is it possible to enforce this in the uclass?
>
> So have the uclass call detect() if it hasn't been called? Should there
> be a way to re-detect?

I mean have a check function (that all off the uclass methods call)
that checks it has been called and returns an error immediately if
not.

I don't think automatically detecting is a great idea - as you say it
makes it hard to re-detect. But just a bool flag in the device's
uclass-private data should be enough.

>
>  > Also I think -EPERM or -EACCES might be better than -ENOENT.
>
> Hm, I thought I saw gazerbeam using ENOENT for this purpose, but on
> further review it looks like it does not. I think EPERM is good for
> this.

OK.

Regards,
Simon
diff mbox series

Patch

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index a6ad094e91..4d17582af5 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -116,6 +116,10 @@  static int spl_fit_get_image_name(const void *fit, int images,
 		 * no string in the property for this index. Check if the
 		 * sysinfo-level code can supply one.
 		 */
+		rc = sysinfo_detect(sysinfo);
+		if (rc)
+			return rc;
+
 		rc = sysinfo_get_fit_loadable(sysinfo, index - i - 1, type,
 					      &str);
 		if (rc && rc != -ENOENT)
diff --git a/include/sysinfo.h b/include/sysinfo.h
index 9386bdf49a..14c25fa15c 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -54,7 +54,8 @@  struct sysinfo_ops {
 	 * This operation might take a long time (e.g. read from EEPROM,
 	 * check the presence of a device on a bus etc.), hence this is not
 	 * done in the probe() method, but later during operation in this
-	 * dedicated method.
+	 * dedicated method. This method must be called before any other
+	 * methods.
 	 *
 	 * Return: 0 if OK, -ve on error.
 	 */
@@ -67,7 +68,8 @@  struct sysinfo_ops {
 	 * @id:		A unique identifier for the bool value to be read.
 	 * @val:	Pointer to a buffer that receives the value read.
 	 *
-	 * Return: 0 if OK, -ve on error.
+	 * Return: 0 if OK, -ENOENT if called before @detect, else -ve on
+	 * error.
 	 */
 	int (*get_bool)(struct udevice *dev, int id, bool *val);
 
@@ -78,7 +80,8 @@  struct sysinfo_ops {
 	 * @id:        A unique identifier for the int value to be read.
 	 * @val:       Pointer to a buffer that receives the value read.
 	 *
-	 * Return: 0 if OK, -ve on error.
+	 * Return: 0 if OK, -ENOENT if called before @detect, else -ve on
+	 * error.
 	 */
 	int (*get_int)(struct udevice *dev, int id, int *val);
 
@@ -90,7 +93,8 @@  struct sysinfo_ops {
 	 * @size:	The size of the buffer to receive the string data.
 	 * @val:	Pointer to a buffer that receives the value read.
 	 *
-	 * Return: 0 if OK, -ve on error.
+	 * Return: 0 if OK, -ENOENT if called before @detect, else -ve on
+	 * error.
 	 */
 	int (*get_str)(struct udevice *dev, int id, size_t size, char *val);
 
@@ -98,7 +102,7 @@  struct sysinfo_ops {
 	 * get_fit_loadable - Get the name of an image to load from FIT
 	 * This function can be used to provide the image names based on runtime
 	 * detection. A classic use-case would when DTBOs are used to describe
-	 * additionnal daughter cards.
+	 * additional daughter cards.
 	 *
 	 * @dev:	The sysinfo instance to gather the data.
 	 * @index:	Index of the image. Starts at 0 and gets incremented
@@ -120,6 +124,9 @@  struct sysinfo_ops {
  *
  * @dev:	The device containing the information
  *
+ * This function must be called before any other accessor function for this
+ * device.
+ *
  * Return: 0 if OK, -ve on error.
  */
 int sysinfo_detect(struct udevice *dev);
@@ -131,7 +138,8 @@  int sysinfo_detect(struct udevice *dev);
  * @id:		A unique identifier for the bool value to be read.
  * @val:	Pointer to a buffer that receives the value read.
  *
- * Return: 0 if OK, -ve on error.
+ * Return: 0 if OK, -ENOENT if called before sysinfo_detect(), else -ve on
+ * error.
  */
 int sysinfo_get_bool(struct udevice *dev, int id, bool *val);
 
@@ -142,7 +150,8 @@  int sysinfo_get_bool(struct udevice *dev, int id, bool *val);
  * @id:		A unique identifier for the int value to be read.
  * @val:	Pointer to a buffer that receives the value read.
  *
- * Return: 0 if OK, -ve on error.
+ * Return: 0 if OK, -ENOENT if called before sysinfo_detect(), else -ve on
+ * error.
  */
 int sysinfo_get_int(struct udevice *dev, int id, int *val);
 
@@ -154,7 +163,8 @@  int sysinfo_get_int(struct udevice *dev, int id, int *val);
  * @size:	The size of the buffer to receive the string data.
  * @val:	Pointer to a buffer that receives the value read.
  *
- * Return: 0 if OK, -ve on error.
+ * Return: 0 if OK, -ENOENT if called before sysinfo_detect(), else -ve on
+ * error.
  */
 int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
 
@@ -166,7 +176,8 @@  int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
  * function that returns the unique device. This is especially useful for use
  * in sysinfo files.
  *
- * Return: 0 if OK, -ve on error.
+ * Return: 0 if OK, -ENOENT if called before sysinfo_detect(), else -ve on
+ * error.
  */
 int sysinfo_get(struct udevice **devp);