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;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20190225065044.11023-5-vaishali.thakkar@linaro.org>
To:     Vaishali Thakkar <vaishali.thakkar@linaro.org>,
        andy.gross@linaro.org
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes
Message-ID: <155138953788.16805.6820097041346672619@swboyd.mtv.corp.google.com>
References: <20190225065044.11023-1-vaishali.thakkar@linaro.org> <20190225065044.11023-5-vaishali.thakkar@linaro.org>
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     david.brown@linaro.org, gregkh@linuxfoundation.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rafael@kernel.org, bjorn.andersson@linaro.org, vkoul@kernel.org,
        Vaishali Thakkar <vaishali.thakkar@linaro.org>
User-Agent: alot/0.8
Date:   Thu, 28 Feb 2019 13:32:17 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Vaishali Thakkar (2019-02-24 22:50:43)
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 02078049fac7..ccadeba69a81 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -70,6 +93,10 @@ struct socinfo {
>  struct qcom_socinfo {
>         struct soc_device *soc_dev;
>         struct soc_device_attribute attr;
> +#ifdef CONFIG_DEBUG_FS
> +       struct dentry *dbg_root;
> +#endif /* CONFIG_DEBUG_FS */
> +       struct socinfo *socinfo;

This doesn't look necessary, instead just pass it through to the
functions that need the pointer.

>  };
> =20
>  struct soc_of_id {
> @@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *d=
ev, unsigned int id)
>         return NULL;
>  }
> =20
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define UINT_SHOW(name, attr)                                          \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo =3D seq->private;                        =
 \
> +       seq_printf(seq, "%u\n", le32_to_cpu(socinfo->attr));            \
> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops =3D {           =
 \
> +       .open =3D qcom_open_##name,                                      =
 \
> +       .read =3D seq_read,                                              =
 \
> +       .llseek =3D seq_lseek,                                           =
 \
> +       .release =3D single_release,                                     =
 \
> +}

Why can't we use the debugfs_create_u32 function? It would make things
clearer if there was either a debugfs_create_le32() function or if the
socinfo structure stored in smem was unmarshalled from little endian
to the cpu native endian format during probe time and then all the rest
of the code can treat it as a native endian u32 values.

> +
> +#define DEBUGFS_UINT_ADD(name)                                         \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +#define HEX_SHOW(name, attr)                                           \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo =3D seq->private;                        =
 \
> +       seq_printf(seq, "0x%x\n", le32_to_cpu(socinfo->attr));          \

Use "%#x\n" format?

> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops =3D {           =
 \
> +       .open =3D qcom_open_##name,                                      =
 \
> +       .read =3D seq_read,                                              =
 \
> +       .llseek =3D seq_lseek,                                           =
 \
> +       .release =3D single_release,                                     =
 \
> +}
> +
> +#define DEBUGFS_HEX_ADD(name)                                          \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +#define QCOM_OPEN(name, _func)                                         \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, _func, inode->i_private);              \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops =3D {           =
 \
> +       .open =3D qcom_open_##name,                                      =
 \
> +       .read =3D seq_read,                                              =
 \
> +       .llseek =3D seq_lseek,                                           =
 \
> +       .release =3D single_release,                                     =
 \
> +}
> +
> +#define DEBUGFS_ADD(name)                                              \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +static int qcom_show_build_id(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo =3D seq->private;
> +
> +       seq_printf(seq, "%s\n", socinfo->build_id);
> +
> +       return 0;
> +}
> +
> +static int qcom_show_accessory_chip(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo =3D seq->private;
> +
> +       seq_printf(seq, "%d\n", le32_to_cpu(socinfo->accessory_chip));
> +
> +       return 0;
> +}
> +
> +static int qcom_show_platform_subtype(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo =3D seq->private;
> +       int subtype =3D le32_to_cpu(socinfo->hw_plat_subtype);
> +
> +       if (subtype < 0)
> +               return -EINVAL;

Can it ever be negative? Why is it assigned to int type at all?

> +
> +       seq_printf(seq, "%u\n", subtype);

And then we print it as an unsigned value? Why not use %d to match the
type?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo =3D seq->private;
> +       int model =3D SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> +
> +       if (model < 0)
> +               return -EINVAL;
> +
> +       seq_printf(seq, "%s\n", pmic_model[model]);

Is there a debugfs_create_file_string()?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo =3D seq->private;
> +
> +       seq_printf(seq, "%u.%u\n",
> +                  SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> +                  SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> +
> +       return 0;
> +}
> +