From: Joe Burmeister <joe.burmeister@devtank.co.uk>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Arnd Bergmann <arnd@arndb.de>,
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
YueHaibing <yuehaibing@huawei.com>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.
Date: Fri, 9 Aug 2019 14:18:24 +0100 [thread overview]
Message-ID: <d6534808-aa41-0bf0-a516-cee9bbd8e97a@devtank.co.uk> (raw)
In-Reply-To: <20190809130005.GA13962@kroah.com>
Hi Greg,
On 09/08/2019 14:00, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
>> +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
>> +{
>> + unsigned long timeout, retries;
>> + int sr, status;
>> + u8 cp;
>> +
>> + cp = AT25_WREN;
>> + status = spi_write(at25->spi, &cp, 1);
>> + if (status < 0) {
>> + dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
>> + return;
>> + }
>> + cp = at25->erase_instr;
>> + status = spi_write(at25->spi, &cp, 1);
>> + if (status < 0) {
>> + dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
>> + return;
>> + }
>> + /* Wait for non-busy status */
>> + timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
>> + retries = 0;
>> + do {
>> + sr = spi_w8r8(at25->spi, AT25_RDSR);
>> + if (sr < 0 || (sr & AT25_SR_nRDY)) {
>> + dev_dbg(&at25->spi->dev,
>> + "rdsr --> %d (%02x)\n", sr, sr);
>> + /* at HZ=100, this is sloooow */
>> + msleep(1);
>> + continue;
>> + }
>> + if (!(sr & AT25_SR_nRDY))
>> + return;
>> + } while (retries++ < 200 || time_before_eq(jiffies, timeout));
>> +
>> + if ((sr < 0) || (sr & AT25_SR_nRDY)) {
>> + dev_err(&at25->spi->dev,
>> + "chip erase, timeout after %u msecs\n",
>> + jiffies_to_msecs(jiffies -
>> + (timeout - ERASE_TIMEOUT)));
>> + status = -ETIMEDOUT;
>> + return;
>> + }
>> +}
>> +
>> +
> No need for 2 lines :(
Sorry, other coding conventions I'm used to.
>> +static ssize_t eeprom_at25_store_erase(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct at25_data *at25 = dev_get_drvdata(dev);
>> + int erase = 0;
>> +
>> + sscanf(buf, "%d", &erase);
>> + if (erase) {
>> + mutex_lock(&at25->lock);
>> + _eeprom_at25_store_erase_locked(at25);
>> + mutex_unlock(&at25->lock);
>> + }
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
>> +
>> +
> Same here.
>
> Also, where is the Documentation/ABI/ update for the new sysfs file?
There isn't anything for the existing SPI EEPROM stuff I can see.
Would I have to document what was already there to add my bit?
>> static int at25_probe(struct spi_device *spi)
>> {
>> struct at25_data *at25 = NULL;
>> @@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
>> int err;
>> int sr;
>> int addrlen;
>> + int has_erase;
>>
>> /* Chip description */
>> if (!spi->dev.platform_data) {
>> @@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
>> spi_set_drvdata(spi, at25);
>> at25->addrlen = addrlen;
>>
>> + /* Optional chip erase instruction */
>> + device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
>> +
>> at25->nvmem_config.name = dev_name(&spi->dev);
>> at25->nvmem_config.dev = &spi->dev;
>> at25->nvmem_config.read_only = chip.flags & EE_READONLY;
>> @@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
>> if (IS_ERR(at25->nvmem))
>> return PTR_ERR(at25->nvmem);
>>
>> - dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
>> + has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
>> +
>> + dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
>> (chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
>> (chip.byte_len < 1024) ? "Byte" : "KByte",
>> at25->chip.name,
>> (chip.flags & EE_READONLY) ? " (readonly)" : "",
>> - at25->chip.page_size);
>> + at25->chip.page_size, (has_erase)?" <has erase>":"");
>> +
>> + if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
>> + dev_err(&spi->dev, "can't create erase interface\n");
> You just raced with userspace and lost :(
>
> Please create an attribute group and add it to the .dev_groups pointer
> in struct driver so it can be created properly in a race-free manner.
>
> See the thread at:
> https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
> for the details on how to do that.
Clearly I didn't know about that. I'll do some reading when I've got a
bit of time and try a again.
> thanks,
>
> greg k-h
Cheers,
Joe
next prev parent reply other threads:[~2019-08-09 13:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-09 12:53 [PATCH] Add optional chip erase functionality to AT25 EEPROM driver Joe Burmeister
2019-08-09 13:00 ` Greg Kroah-Hartman
2019-08-09 13:18 ` Joe Burmeister [this message]
2019-08-09 13:28 ` Greg Kroah-Hartman
2019-08-09 13:54 ` Mark Rutland
2019-08-09 16:28 ` Joe Burmeister
2019-08-12 15:51 ` David Laight
2019-08-12 21:03 ` Joe Burmeister
2019-08-21 21:21 ` Rob Herring
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=d6534808-aa41-0bf0-a516-cee9bbd8e97a@devtank.co.uk \
--to=joe.burmeister@devtank.co.uk \
--cc=arnd@arndb.de \
--cc=bgolaszewski@baylibre.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=yuehaibing@huawei.com \
/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).