[PATCH v3] mtd: lpddr: add driver for LPDDR2-NVM PCM memories

[Vincenzo Aliberti]

Hi Brian,
   your feedback looks great! Thanks!

Only one concern: I have been forced to test it on a 3.10_rc4 version
of the Kernel and in order to use
IS_ERR and PTR_ERR functions I had to add:

#include <linux/err.h>

On the 3.14 I expect it should be the same, which is your feeling?

Do you wnat I send a v4 of this patch?

Regards,
   Vincenzo

On Mon, May 12, 2014 at 8:30 PM, Brian Norris
<computersforpeace at gmail.com> wrote:
> Hi Vincenzo,
>
> On Fri, May 09, 2014 at 11:19:57PM +0200, Vincenzo Aliberti wrote:
>> MTD: LPDDR: Add Driver for LPDDR2-NVM PCM memories
>>
>> Signed-off-by: Vincenzo Aliberti <vincenzo.aliberti at gmail.com>
>>
>> ---
>> Changes in v3:
>>       - BusWidth no longer passed as argument: PCM devices available as x32
>>       - Partitions no longer defined in the driver
>>       - Added a remove callback
>
> OK, so you handled a lot of my requests. But you're still missing some
> error handling and a few other things. Rather than go through this whole
> patch again, I just made my own changes. Can you please review and test
> my diff, appended below?
>
>>  drivers/mtd/lpddr/Kconfig      |   12 +-
>>  drivers/mtd/lpddr/Makefile     |    1 +
>>  drivers/mtd/lpddr/lpddr2_nvm.c |  515 ++++++++++++++++++++++++++++++++++++++++
>>  include/uapi/mtd/mtd-abi.h     |    1 +
>>  4 files changed, 527 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/mtd/lpddr/lpddr2_nvm.c
> [snip]
>
> diff --git a/drivers/mtd/lpddr/lpddr2_nvm.c b/drivers/mtd/lpddr/lpddr2_nvm.c
> index 4ab9adb515eb..4e76f889c9c6 100644
> --- a/drivers/mtd/lpddr/lpddr2_nvm.c
> +++ b/drivers/mtd/lpddr/lpddr2_nvm.c
> @@ -17,10 +17,6 @@
>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc.
>   */
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
> @@ -35,7 +31,6 @@
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/ioport.h>
> -#include <linux/sched.h>
>
>  /* Parameters */
>  #define ERASE_BLOCKSIZE                        (0x00020000/2)  /* in Word */
> @@ -419,17 +414,15 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)
>  {
>         struct map_info *map;
>         struct mtd_info *mtd;
> -       struct resource *add_range = platform_get_resource(pdev,
> -                                               IORESOURCE_MEM, 0);
> -       struct resource *control_regs = platform_get_resource(pdev,
> -                                               IORESOURCE_MEM, 1);
> +       struct resource *add_range;
> +       struct resource *control_regs;
>         struct pcm_int_data *pcm_data;
>
>         /* Allocate memory control_regs data structures */
>         pcm_data = devm_kzalloc(&pdev->dev, sizeof(*pcm_data), GFP_KERNEL);
>         if (!pcm_data)
>                 return -ENOMEM;
> -       pcm_data->ctl_regs = devm_ioremap_resource(&pdev->dev, control_regs);
> +
>         pcm_data->bus_width = BUS_WIDTH;
>
>         /* Allocate memory for map_info & mtd_info data structures */
> @@ -441,34 +434,35 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)
>         if (!mtd)
>                 return -ENOMEM;
>
> -       /* Reserve lpddr2_nvm address range */
> -       if (devm_request_mem_region(&pdev->dev, add_range->start,
> -               resource_size(add_range), "lpddr2_nvm") == NULL) {
> -               pr_err("Unable to reserve address range\n");
> -               return -EBUSY;
> -       }
> +       /* lpddr2_nvm address range */
> +       add_range = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
>         /* Populate map_info data structure */
>         *map = (struct map_info) {
> -               .virt           = devm_ioremap_nocache(&pdev->dev,
> -                               add_range->start, (add_range->end -
> -                               add_range->start + 1)),
> +               .virt           = devm_ioremap_resource(&pdev->dev, add_range),
>                 .name           = pdev->dev.init_name,
>                 .phys           = add_range->start,
> -               .size           = add_range->end - add_range->start + 1,
> -               .bankwidth      = pcm_data->bus_width/2,
> +               .size           = resource_size(add_range),
> +               .bankwidth      = pcm_data->bus_width / 2,
>                 .pfow_base      = OW_BASE_ADDRESS,
>                 .fldrv_priv     = pcm_data,
>         };
> +       if (IS_ERR(map->virt))
> +               return PTR_ERR(map->virt);
>
>         simple_map_init(map);   /* fill with default methods */
>
> +       control_regs = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       pcm_data->ctl_regs = devm_ioremap_resource(&pdev->dev, control_regs);
> +       if (IS_ERR(pcm_data->ctl_regs))
> +               return PTR_ERR(pcm_data->ctl_regs);
> +
>         /* Populate mtd_info data structure */
>         *mtd = (struct mtd_info) {
>                 .name           = pdev->dev.init_name,
>                 .type           = MTD_RAM,
>                 .priv           = map,
> -               .size           = add_range->end - add_range->start + 1,
> +               .size           = resource_size(add_range),
>                 .erasesize      = ERASE_BLOCKSIZE * pcm_data->bus_width,
>                 .writesize      = 1,
>                 .writebufsize   = WRITE_BUFFSIZE * pcm_data->bus_width,
> @@ -494,10 +488,7 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)
>   */
>  static int lpddr2_nvm_remove(struct platform_device *pdev)
>  {
> -       /* Unregister the MTD device */
> -       mtd_device_unregister(dev_get_drvdata(&pdev->dev));
> -
> -       return 0;
> +       return mtd_device_unregister(dev_get_drvdata(&pdev->dev));
>  }
>
>  /* Initialize platform_driver data structure for lpddr2_nvm */