linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Vignesh Raghavendra <vigneshr@ti.com>
To: Boris Brezillon <boris.brezillon@collabora.com>,
	Zhuohao Lee <zhuohao@chromium.org>
Cc: "Nicolas Boichat" <drinkcat@chromium.org>,
	bbrezillon@kernel.org, richard@nod.at,
	"Brian Norris" <briannorris@chromium.org>,
	"Marek Vašut" <marek.vasut@gmail.com>,
	linux-mtd@lists.infradead.org,
	"Brian Norris" <computersforpeace@gmail.com>,
	"David Woodhouse" <dwmw2@infradead.org>
Subject: Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id
Date: Tue, 2 Apr 2019 13:57:25 +0530	[thread overview]
Message-ID: <7077290a-8acc-465b-6ed0-fddd35ab8fd6@ti.com> (raw)
In-Reply-To: <20190402095628.5d376263@collabora.com>



On 02/04/19 1:26 PM, Boris Brezillon wrote:
> On Tue, 2 Apr 2019 15:39:54 +0800
> Zhuohao Lee <zhuohao@chromium.org> wrote:
> 
>> Thanks Boris for the comment. Please take a look the reply at below.
>>
>> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
>> <boris.brezillon@collabora.com> wrote:
>>>
>>> On Thu, 28 Mar 2019 12:59:10 +0800
>>> Zhuohao Lee <zhuohao@chromium.org> wrote:
>>>  
>>>> Currently, we don't have sysfs nodes for querying the underlying flash
>>>> name and flash id. This information is important especially when we
>>>> want to know the flash detail of the defective system. In order to
>>>> support the query, we add two pointers (*flashname, *id) into the
>>>> mtd_info structure and create two sysfs nodes (flashname, id). This
>>>> patch is modified based on the SPI-NOR flash system as we only have
>>>> that system now. But the idea should be applied to the other flash
>>>> driver like NAND flash.
>>>>
>>>> The output of new sysfs nodes on my device are:
>>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
>>>> w25q64dw
>>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
>>>> ef6017  
>>>
>>> I'm not sure I like the idea of exposing this kind of info through
>>> sysfs as it then makes part of the ABI. Did you consider exposing that
>>> through debugfs?  
>>
>> Yes, i did consider the debugfs. I think the debugfs is depended on
>> CONFIG_DEBUG_FS.
>> If removing that config, the partname and partid will be lost. So, i
>> proposed to use
>> sysfs.
> 
> Then just enable debugfs if you need this information :P.
> 

Do we really need this info even in debug FS?
spi-nor.c prints following to kernel log which can be obtained by dmesg:

        dev_info(dev, "%s (%lld Kbytes)\n", info->name,
                        (long long)mtd->size >> 10);

You could probably expand that to print flash IDs as well.

Regards
Vignesh

>>
>>>  
>>>>
>>>> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
>>>> ---
>>>>  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
>>>>  drivers/mtd/spi-nor/spi-nor.c |  3 +++
>>>>  include/linux/mtd/mtd.h       |  3 +++
>>>>  3 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>> index 3ef01baef9b6..dcbe6719ad67 100644
>>>> --- a/drivers/mtd/mtdcore.c
>>>> +++ b/drivers/mtd/mtdcore.c
>>>> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
>>>>  }
>>>>  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
>>>>
>>>> +static ssize_t mtd_flashname_show(struct device *dev,
>>>> +             struct device_attribute *attr, char *buf)
>>>> +{
>>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
>>>> +
>>>> +     if (!mtd->flashname)
>>>> +             return 0;
>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
>>>> +}
>>>> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);  
>>>
>>> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
>>> about partname?  
>>
>> Thanks, i will change the name to partname.
>>
>>>  
>>>> +
>>>> +static ssize_t mtd_id_show(struct device *dev,
>>>> +             struct device_attribute *attr, char *buf)
>>>> +{
>>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
>>>> +
>>>> +     if (!mtd->id)
>>>> +             return 0;
>>>> +     return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);  
>>>
>>> I'd recommend making mtd->id a string so that each flash type can
>>> decide of the formatting, and maybe have a prefix that tells which kind  
>>
>> ok, i will create an array to store the formatted partid.
>>
>>> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".  
>>
>> We had a sysfs node, called 'type', which indicated the type of the
>> underlying device. We can query the 'type' to get the device type.
>> I think it is not necessary to add prefix. What do you think?
> 
> No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR
> and they probably don't use the same ID-scheme. Same for NANDs (parallel
> NANDs vs SPI NANDs).
> 
>>
>>>  
>>>> +}
>>>> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);  
>>>
>>> id is bit vague, how about partid.  
>>
>> Agree, i will change this.
> 
> I think you should wait for other reviews before you sending a new
> version. I'm still not convinced exposing that through sysfs is a good
> idea, and I'd like other MTD maintainers to give their opinion on this
> aspect.
> 
>>
>>>  
>>>> +
>>>>  static ssize_t mtd_ecc_strength_show(struct device *dev,
>>>>                                    struct device_attribute *attr, char *buf)
>>>>  {
>>>> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
>>>>       &dev_attr_oobavail.attr,
>>>>       &dev_attr_numeraseregions.attr,
>>>>       &dev_attr_name.attr,
>>>> +     &dev_attr_flashname.attr,
>>>> +     &dev_attr_id.attr,
>>>>       &dev_attr_ecc_strength.attr,
>>>>       &dev_attr_ecc_step_size.attr,
>>>>       &dev_attr_corrected_bits.attr,
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 6e13bbd1aaa5..0e10858e532c 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>>>
>>>>       if (!mtd->name)
>>>>               mtd->name = dev_name(dev);
>>>> +     mtd->flashname = info->name;
>>>> +     mtd->id = info->id;
>>>> +     mtd->id_size = info->id_len;
>>>>       mtd->priv = nor;
>>>>       mtd->type = MTD_NORFLASH;
>>>>       mtd->writesize = 1;
>>>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>>>> index 677768b21a1d..0a81569fa4f6 100644
>>>> --- a/include/linux/mtd/mtd.h
>>>> +++ b/include/linux/mtd/mtd.h
>>>> @@ -210,6 +210,9 @@ struct mtd_info {
>>>>       uint32_t flags;
>>>>       uint32_t orig_flags; /* Flags as before running mtd checks */
>>>>       uint64_t size;   // Total size of the MTD
>>>> +     const char *flashname; /* The underlying flash name */
>>>> +     const char *id; /* The ID of the flash */
>>>> +     int id_size; /* Number of bytes of id array */
>>>>
>>>>       /* "Major" erase size for the device. Naïve users may take this
>>>>        * to be the only erase size available, or may use the more detailed  
>>>  
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply	other threads:[~2019-04-02  8:26 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-28  4:59 [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id Zhuohao Lee
2019-04-01  8:43 ` Zhuohao Lee
2019-04-01  9:27 ` Boris Brezillon
2019-04-02  7:39   ` Zhuohao Lee
2019-04-02  7:56     ` Boris Brezillon
2019-04-02  8:27       ` Vignesh Raghavendra [this message]
2019-04-02 11:06         ` Zhuohao Lee
2019-04-02 12:01           ` Boris Brezillon
2019-04-02 13:03             ` Zhuohao Lee
2019-04-03  8:31               ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7077290a-8acc-465b-6ed0-fddd35ab8fd6@ti.com \
    --to=vigneshr@ti.com \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=briannorris@chromium.org \
    --cc=computersforpeace@gmail.com \
    --cc=drinkcat@chromium.org \
    --cc=dwmw2@infradead.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=richard@nod.at \
    --cc=zhuohao@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).