On Fri, Nov 29, 2013 at 12:19:15PM +0000, Lee Jones wrote:
> When a read is issued by userspace the MFD framework calls back into
> the driver to conduct the actual command issue and data extraction.
> Here we provide the routines which do exactly that.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mtd/devices/st_spi_fsm.c | 110
> +++++++++++++++++++++++++++++++++++++++
> drivers/mtd/devices/st_spi_fsm.h | 3 ++
> 2 files changed, 113 insertions(+)
>
> diff --git a/drivers/mtd/devices/st_spi_fsm.c
> b/drivers/mtd/devices/st_spi_fsm.c
> index ad277f1..c76510e 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -538,6 +538,114 @@ static int stfsm_n25q_config(struct stfsm *fsm)
> return 0;
> }
>
> +static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,
Did you really mean uint8_t *const buf? This means that buf will always
hold the same address (which is fine). But we don't often enshrine that
in the function definition.
> + const uint32_t size, const uint32_t offset)
Similar. Is it really important to mark size and offset const?
I won't argue, if you think this is deserving, but I don't want to
encourage this practice everywhere, since it doesn't buy us much.
(Declaring a pointer argument as, e.g., 'const int *var' is useful in an
API, because it helps a developer understand externally that the routine
will not (in the absence of unsafe casts) modify the buffer contents.
But declaring 'int *const var' is less helpful, as it is only useful for
local reasoning.)
> +{
...
> +}
> +
> +/*
> + * Read an address range from the flash chip. The address range
> + * may be any size provided it is within the physical boundaries.
> + */
> +static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
> + size_t *retlen, u_char *buf)
> +{
> + struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
> + uint32_t bytes;
> +
> + dev_dbg(fsm->dev, "%s from 0x%08x, len %zd\n",
> + __func__, (u32)from, len);
> +
> + /* Initialise read length */
> + if (retlen)
> + *retlen = 0;
You're repeating the common code in mtd_read(). Kill this
initialization.
> +
> + if (!len) {
> + dev_warn(fsm->dev, "Zero byte read requested\n");
> + return 0;
> + }
Similar. You're duplicating the checks in mtd_read().
> +
> + if (from + len > mtd->size) {
> + dev_err(fsm->dev, "Can't read past end of chip\n");
> + return -EINVAL;
> + }
Ditto.
> +
> + mutex_lock(&fsm->lock);
> +
> + while (len > 0) {
> + bytes = min(len, (size_t)FLASH_PAGESIZE);
> +
> + stfsm_read(fsm, buf, bytes, from);
> +
> + buf += bytes;
> + from += bytes;
> + len -= bytes;
> +
> + if (retlen)
> + *retlen += bytes;
> + }
> +
> + mutex_unlock(&fsm->lock);
> +
> + return 0;
> +}
> +
> static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
> {
> const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
> diff --git a/drivers/mtd/devices/st_spi_fsm.h
> b/drivers/mtd/devices/st_spi_fsm.h
> index b5ce07d..e168296 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -229,6 +229,9 @@
> #define FLASH_CMD_READ4_1_1_4 0x6c
> #define FLASH_CMD_READ4_1_4_4 0xec
>
> +#define FLASH_PAGESIZE 256 /* In Bytes
> */
> +#define FLASH_PAGESIZE_32 FLASH_PAGESIZE / 4 /* In uint32_t */
checkpatch.pl warns that you need parentheses around FLASH_PAGESIZE / 4.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
