| Message ID | 20170523004317.16908-6-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, May 23, 2017 at 12:43:17PM +1200, Chris Packham wrote: > The mchp23lcv1024 is software compatible with the mchp23k256, the > only difference (from a software point of view) is the size. This is not really true. The size of the address is also different, and the point of the v2 change. > There > is no way to detect the size so we must be told via a Device Tree. or defaults to 256K when device tree is not used. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 23/05/17 12:58, Andrew Lunn wrote: > On Tue, May 23, 2017 at 12:43:17PM +1200, Chris Packham wrote: >> The mchp23lcv1024 is software compatible with the mchp23k256, the >> only difference (from a software point of view) is the size. > > This is not really true. The size of the address is also different, > and the point of the v2 change. Agreed. How about this revised commit message --- 8< --- mtd: mchp23k256: Add support for mchp23lcv1024 The mchp23lcv1024 is similar to the mchp23k256, the differences (from a software point of view) are the capacity of the chip and the size of the addresses used. There is no way to detect the specific chip so we must be told via a Device Tree or default to mchp23k256 when device tree is not used. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Reviewed-by: Andrew Lunn <andrew@lunn.ch> --- >8 --- Can someone fixup the commit message or should I re-send? > >> There >> is no way to detect the size so we must be told via a Device Tree. > > or defaults to 256K when device tree is not used. > >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Andrew >
> Agreed. How about this revised commit message > > --- 8< --- > mtd: mchp23k256: Add support for mchp23lcv1024 > > The mchp23lcv1024 is similar to the mchp23k256, the differences (from a > software point of view) are the capacity of the chip and the size of the > addresses used. > > There is no way to detect the specific chip so we must be told via a > Device Tree or default to mchp23k256 when device tree is not used. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > --- >8 --- Yes, that is good. > Can someone fixup the commit message or should I re-send? The less the maintainer has to do, the more likely he will do it. But hold on a few days, and see if you get any more comments. Andrew
Le Tue, 23 May 2017 12:43:17 +1200, Chris Packham <chris.packham@alliedtelesis.co.nz> a écrit : > The mchp23lcv1024 is software compatible with the mchp23k256, the > only difference (from a software point of view) is the size. There > is no way to detect the size so we must be told via a Device Tree. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > Changes in v2: > - fix formatting in switch statement > - add support for 24-bit addressing > Changes in v3: > - None > > .../bindings/mtd/microchip,mchp23k256.txt | 2 +- > drivers/mtd/devices/mchp23k256.c | 53 ++++++++++++++++++---- > 2 files changed, 44 insertions(+), 11 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt b/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt > index 25e5ad38b0f0..7328eb92a03c 100644 > --- a/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt > +++ b/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt > @@ -3,7 +3,7 @@ > Required properties: > - #address-cells, #size-cells : Must be present if the device has sub-nodes > representing partitions. > -- compatible : Must be "microchip,mchp23k256" > +- compatible : Must be one of "microchip,mchp23k256" or "microchip,mchp23lcv1024" > - reg : Chip-Select number > - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at > > diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c > index 3e5feb454644..72ecf374a06a 100644 > --- a/drivers/mtd/devices/mchp23k256.c > +++ b/drivers/mtd/devices/mchp23k256.c > @@ -21,10 +21,14 @@ > #include <linux/spi/spi.h> > #include <linux/of_device.h> > > +#define MAX_CMD_SIZE 4 > +enum chips { mchp23k256, mchp23lcv1024 }; > + > struct mchp23k256_flash { > struct spi_device *spi; > struct mutex lock; > struct mtd_info mtd; > + u8 addr_width; > }; How about creating a struct mchp23_caps or mchp23_specs struct containing the SRAM specs. struct mchp23_caps { u8 addr_width; unsigned int size; } and then struct mchp23k256_flash { ... const struct mchp32_cap *caps; }; This way you can get rid of the enum and add new fields to the caps struct if needed. BTW, it's really weird to have the _flash extension in the struct name, while we're actually dealing with SRAMs. > > #define MCHP23K256_CMD_WRITE_STATUS 0x01 > @@ -34,22 +38,35 @@ struct mchp23k256_flash { > > #define to_mchp23k256_flash(x) container_of(x, struct mchp23k256_flash, mtd) > > +static void mchp23k256_addr2cmd(struct mchp23k256_flash *flash, > + unsigned int addr, u8 *cmd) > +{ > + /* cmd[0] has opcode */ > + cmd[1] = addr >> (flash->addr_width * 8 - 8); > + cmd[2] = addr >> (flash->addr_width * 8 - 16); > + cmd[3] = addr >> (flash->addr_width * 8 - 24); or int i; /* * Address is sent in big endian (MSB first) and we skip * the first entry of the cmd array which contains the cmd * opcode. */ for (i = flash->caps->addr_width; i--, addr >>= 8; i > 0) cmd[i] = addr; > +} > + > +static int mchp23k256_cmdsz(struct mchp23k256_flash *flash) > +{ > + return 1 + flash->addr_width; > +} > + > static int mchp23k256_write(struct mtd_info *mtd, loff_t to, size_t len, > size_t *retlen, const unsigned char *buf) > { > struct mchp23k256_flash *flash = to_mchp23k256_flash(mtd); > struct spi_transfer transfer[2] = {}; > struct spi_message message; > - unsigned char command[3]; > + unsigned char command[MAX_CMD_SIZE]; > > spi_message_init(&message); > > command[0] = MCHP23K256_CMD_WRITE; > - command[1] = to >> 8; > - command[2] = to; > + mchp23k256_addr2cmd(flash, to, command); > > transfer[0].tx_buf = command; > - transfer[0].len = sizeof(command); > + transfer[0].len = mchp23k256_cmdsz(flash); > spi_message_add_tail(&transfer[0], &message); > > transfer[1].tx_buf = buf; > @@ -73,17 +90,16 @@ static int mchp23k256_read(struct mtd_info *mtd, loff_t from, size_t len, > struct mchp23k256_flash *flash = to_mchp23k256_flash(mtd); > struct spi_transfer transfer[2] = {}; > struct spi_message message; > - unsigned char command[3]; > + unsigned char command[MAX_CMD_SIZE]; > > spi_message_init(&message); > > memset(&transfer, 0, sizeof(transfer)); > command[0] = MCHP23K256_CMD_READ; > - command[1] = from >> 8; > - command[2] = from; > + mchp23k256_addr2cmd(flash, from, command); > > transfer[0].tx_buf = command; > - transfer[0].len = sizeof(command); > + transfer[0].len = mchp23k256_cmdsz(flash); > spi_message_add_tail(&transfer[0], &message); > > transfer[1].rx_buf = buf; static const struct mchp23_caps mchp23k256_caps = { .size = SZ_32K, .addr_width = 2; }; static const struct mchp23_caps mchp23lcv1024_caps = { .size = SZ_128K, .addr_width = 3; }; > @@ -128,6 +144,7 @@ static int mchp23k256_probe(struct spi_device *spi) > struct mchp23k256_flash *flash; > struct flash_platform_data *data; > int err; > + enum chips chip; > > flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL); > if (!flash) > @@ -143,15 +160,30 @@ static int mchp23k256_probe(struct spi_device *spi) > > data = dev_get_platdata(&spi->dev); > > + if (spi->dev.of_node) > + chip = (enum chips)of_device_get_match_data(&spi->dev); > + else > + chip = mchp23k256; flash->caps = of_device_get_match_data(&spi->dev); if (!flash->caps) flash->caps = mchp23k256_caps; > + > mtd_set_of_node(&flash->mtd, spi->dev.of_node); > flash->mtd.dev.parent = &spi->dev; > flash->mtd.type = MTD_RAM; > flash->mtd.flags = MTD_CAP_RAM; > flash->mtd.writesize = 1; > - flash->mtd.size = SZ_32K; > flash->mtd._read = mchp23k256_read; > flash->mtd._write = mchp23k256_write; > > + switch (chip) { > + case mchp23lcv1024: > + flash->mtd.size = SZ_128K; > + flash->addr_width = 3; > + break; > + default: > + flash->mtd.size = SZ_32K; > + flash->addr_width = 2; > + break; > + } > + > err = mtd_device_register(&flash->mtd, data ? data->parts : NULL, > data ? data->nr_parts : 0); > if (err) > @@ -168,7 +200,8 @@ static int mchp23k256_remove(struct spi_device *spi) > } > > static const struct of_device_id mchp23k256_of_table[] = { > - { .compatible = "microchip,mchp23k256" }, > + { .compatible = "microchip,mchp23k256", .data = (void *)mchp23k256 }, > + { .compatible = "microchip,mchp23lcv1024", .data = (void *)mchp23lcv1024 }, { .compatible = "microchip,mchp23k256", .data = &mchp23k256_caps, }, { .compatible = "microchip,mchp23lcv1024", .data = &mchp23lcv1024_caps, }, > {} > }; > MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
On 23/05/17 19:19, Boris Brezillon wrote: > BTW, it's really weird to have the _flash extension in the struct name, > while we're actually dealing with SRAMs. Agreed. After the dust settles on this series I'll send a trivial patch to change "flash" to something sensible like "sram" or "chip".
diff --git a/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt b/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt index 25e5ad38b0f0..7328eb92a03c 100644 --- a/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt +++ b/Documentation/devicetree/bindings/mtd/microchip,mchp23k256.txt @@ -3,7 +3,7 @@ Required properties: - #address-cells, #size-cells : Must be present if the device has sub-nodes representing partitions. -- compatible : Must be "microchip,mchp23k256" +- compatible : Must be one of "microchip,mchp23k256" or "microchip,mchp23lcv1024" - reg : Chip-Select number - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c index 3e5feb454644..72ecf374a06a 100644 --- a/drivers/mtd/devices/mchp23k256.c +++ b/drivers/mtd/devices/mchp23k256.c @@ -21,10 +21,14 @@ #include <linux/spi/spi.h> #include <linux/of_device.h> +#define MAX_CMD_SIZE 4 +enum chips { mchp23k256, mchp23lcv1024 }; + struct mchp23k256_flash { struct spi_device *spi; struct mutex lock; struct mtd_info mtd; + u8 addr_width; }; #define MCHP23K256_CMD_WRITE_STATUS 0x01 @@ -34,22 +38,35 @@ struct mchp23k256_flash { #define to_mchp23k256_flash(x) container_of(x, struct mchp23k256_flash, mtd) +static void mchp23k256_addr2cmd(struct mchp23k256_flash *flash, + unsigned int addr, u8 *cmd) +{ + /* cmd[0] has opcode */ + cmd[1] = addr >> (flash->addr_width * 8 - 8); + cmd[2] = addr >> (flash->addr_width * 8 - 16); + cmd[3] = addr >> (flash->addr_width * 8 - 24); +} + +static int mchp23k256_cmdsz(struct mchp23k256_flash *flash) +{ + return 1 + flash->addr_width; +} + static int mchp23k256_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const unsigned char *buf) { struct mchp23k256_flash *flash = to_mchp23k256_flash(mtd); struct spi_transfer transfer[2] = {}; struct spi_message message; - unsigned char command[3]; + unsigned char command[MAX_CMD_SIZE]; spi_message_init(&message); command[0] = MCHP23K256_CMD_WRITE; - command[1] = to >> 8; - command[2] = to; + mchp23k256_addr2cmd(flash, to, command); transfer[0].tx_buf = command; - transfer[0].len = sizeof(command); + transfer[0].len = mchp23k256_cmdsz(flash); spi_message_add_tail(&transfer[0], &message); transfer[1].tx_buf = buf; @@ -73,17 +90,16 @@ static int mchp23k256_read(struct mtd_info *mtd, loff_t from, size_t len, struct mchp23k256_flash *flash = to_mchp23k256_flash(mtd); struct spi_transfer transfer[2] = {}; struct spi_message message; - unsigned char command[3]; + unsigned char command[MAX_CMD_SIZE]; spi_message_init(&message); memset(&transfer, 0, sizeof(transfer)); command[0] = MCHP23K256_CMD_READ; - command[1] = from >> 8; - command[2] = from; + mchp23k256_addr2cmd(flash, from, command); transfer[0].tx_buf = command; - transfer[0].len = sizeof(command); + transfer[0].len = mchp23k256_cmdsz(flash); spi_message_add_tail(&transfer[0], &message); transfer[1].rx_buf = buf; @@ -128,6 +144,7 @@ static int mchp23k256_probe(struct spi_device *spi) struct mchp23k256_flash *flash; struct flash_platform_data *data; int err; + enum chips chip; flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL); if (!flash) @@ -143,15 +160,30 @@ static int mchp23k256_probe(struct spi_device *spi) data = dev_get_platdata(&spi->dev); + if (spi->dev.of_node) + chip = (enum chips)of_device_get_match_data(&spi->dev); + else + chip = mchp23k256; + mtd_set_of_node(&flash->mtd, spi->dev.of_node); flash->mtd.dev.parent = &spi->dev; flash->mtd.type = MTD_RAM; flash->mtd.flags = MTD_CAP_RAM; flash->mtd.writesize = 1; - flash->mtd.size = SZ_32K; flash->mtd._read = mchp23k256_read; flash->mtd._write = mchp23k256_write; + switch (chip) { + case mchp23lcv1024: + flash->mtd.size = SZ_128K; + flash->addr_width = 3; + break; + default: + flash->mtd.size = SZ_32K; + flash->addr_width = 2; + break; + } + err = mtd_device_register(&flash->mtd, data ? data->parts : NULL, data ? data->nr_parts : 0); if (err) @@ -168,7 +200,8 @@ static int mchp23k256_remove(struct spi_device *spi) } static const struct of_device_id mchp23k256_of_table[] = { - { .compatible = "microchip,mchp23k256" }, + { .compatible = "microchip,mchp23k256", .data = (void *)mchp23k256 }, + { .compatible = "microchip,mchp23lcv1024", .data = (void *)mchp23lcv1024 }, {} }; MODULE_DEVICE_TABLE(of, mchp23k256_of_table);
The mchp23lcv1024 is software compatible with the mchp23k256, the only difference (from a software point of view) is the size. There is no way to detect the size so we must be told via a Device Tree. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Changes in v2: - fix formatting in switch statement - add support for 24-bit addressing Changes in v3: - None .../bindings/mtd/microchip,mchp23k256.txt | 2 +- drivers/mtd/devices/mchp23k256.c | 53 ++++++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-)