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;
MIME-Version: 1.0
References: <20190225065044.11023-1-vaishali.thakkar@linaro.org>
 <20190225065044.11023-5-vaishali.thakkar@linaro.org> <155138953788.16805.6820097041346672619@swboyd.mtv.corp.google.com>
In-Reply-To: <155138953788.16805.6820097041346672619@swboyd.mtv.corp.google.com>
From:   Vaishali Thakkar <vaishali.thakkar@linaro.org>
Date:   Thu, 14 Mar 2019 16:55:16 +0530
Message-ID: <CANNG1HWDc-PraBwG=8TBcfJSFFe+X+k6oVhOqyU=i5fAjpuoLw@mail.gmail.com>
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Andy Gross <andy.gross@linaro.org>,
        David Brown <david.brown@linaro.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rafael@kernel.org, Bjorn Andersson <bjorn.andersson@linaro.org>,
        Vinod Koul <vkoul@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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.
>
> >  };
> >
> >  struct soc_of_id {
> > @@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *dev, unsigned int id)
> >         return NULL;
> >  }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +#define UINT_SHOW(name, attr)                                          \
> > +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> > +{                                                                      \
> > +       struct socinfo *socinfo = 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 = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = 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.

Thanks for the review. I've worked on the next version with all the
changes you suggested in the patchset but I'm kind of not sure
what would be the best solution in this case.

I'm bit skeptical about introducing debugfs_create_le32 as we don't
really have any endian specific functions in the debugfs core at the
moment. And if I do that, should I also introduce debugfs_create_be32
[and to an extent debugfs_create_le{16,64}]? More importantly, would
it be useful to introduce them in core?

In the case of converting it to cpu native during probe, I'll need to
declare an extra struct with u32 being the parsed version for it to be
correct. Wouldn't it add extra overhead?

> > +
> > +#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 = 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 = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = 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 = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = 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 = 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 = 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 = seq->private;
> > +       int subtype = 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 = seq->private;
> > +       int model = 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 = 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;
> > +}
> > +