Re: [PATCH v2 01/13] trinity: Add base driver

[Jiho Chu <jiho.chu@samsung.com>]

On Sun, 18 Sep 2022 12:35:23 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Sep 17, 2022 at 04:23:44PM +0900, Jiho Chu wrote:
> > It contains the base codes for trinity driver. Minimal codes to load and
> > probe device is provided. The Trinity Family is controlled by the
> > Memory-Mapped Registers, the register addresses and offsets are
> > described. And user api interfaces are presented to control device under
> > ioctl manner.
> 
> Where is the documentation for how the userspace api works?  And where
> is a link to the userspace code that talks to these devices?  That
> belongs here in this commit changelog text please.
> 

Hi, Greg
Thanks for your review.

The user space library is published in:
https://review.tizen.org/gerrit/gitweb?p=platform/adaptation/npu/trix-engine.git;a=summary

It needs to login to tizen, access guide is:
https://docs.tizen.org/platform/get-started/open-source-project/

And, this information will be included in commit log.

> > +config TRINITY
> > +	bool "Samsung Neural Processing Unit"
> > +	depends on HAS_IOMEM
> > +	depends on HAS_DMA
> > +	help
> > +	  Select this option to enable driver support for Samsung
> > +	  Neural Processing Unit (NPU).
> > +
> > +	  This driver works as a base driver of the other drivers
> > +	  for Trinity device family.
> > +
> > +	  This option should be enabled to support Trinity
> > +	  Vision 2 (TRIV2), and Trinity Audio (TRIA).
> > +
> > +config TRINITY_VISION2
> > +	tristate "Samsung NPU Trinity Vision 2"
> > +	depends on TRINITY
> > +	help
> > +	  Select this option to enable driver support for a Samsung
> > +	  Neural Processing Unit (NPU), Trinity Vision 2.
> > +
> > +	  This driver enables userspace system library to access the
> > +	  device via /dev/triv2-N.
> 
> Why do you have 2 Kconfig entries for only a single driver?  Please just
> make it one.
> 

OK. It'll be merged.


> > diff --git a/drivers/misc/trinity/Makefile b/drivers/misc/trinity/Makefile
> > new file mode 100644
> > index 000000000000..a8e5697d6d85
> > --- /dev/null
> > +++ b/drivers/misc/trinity/Makefile
> > @@ -0,0 +1,7 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_TRINITY_VISION2) += trinity_vision2.o
> > +
> > +trinity-y := trinity.o
> > +
> > +trinity_vision2-objs := $(trinity-y) trinity_vision2_drv.o
> > diff --git a/drivers/misc/trinity/trinity.c b/drivers/misc/trinity/trinity.c
> > new file mode 100644
> > index 000000000000..1704eecfc439
> > --- /dev/null
> > +++ b/drivers/misc/trinity/trinity.c
> > @@ -0,0 +1,225 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Base device driver for Samsung NPU Trinity device family
> > + *
> > + * Copyright (C) 2020-2022 Samsung Electronics
> > + * Copyright (C) 2020 Dongju Chae <dongju.chae@samsung.com>
> > + * Copyright (C) 2020 Wook Song <wook16.song@samsung.com>
> > + * Copyright (C) 2022 MyungJoo Ham <myungjoo.ham@samsung.com>
> > + * Copyright (C) 2022 Yelin Jeong <yelini.jeong@samsung.com>
> > + * Copyright (C) 2022 Jiho Chu <jiho.chu@samsung.com>
> > + */
> > +
> > +#include <linux/of_address.h>
> > +
> > +#include "trinity_common.h"
> > +
> > +#define TRINITY_PADDR_BASE (0x0)
> > +
> > +static DEFINE_IDA(dev_nrs);
> > +static DEFINE_IDA(model_ids);
> > +
> > +/**
> > + * trinity_release() - A common callback for close() in file_operations for a
> > + *		Trinity	device node. If there are device-specific data to be
> > + *		cleaned-up, it is required to clean them up before invoke this
> > + *		callback.
> > + *
> > + * @inode: Inode to be closed
> > + * @file: File to be closed
> > + *
> > + * Returns 0 on success. Otherwise, returns negative error.
> > + */
> > +int trinity_release(struct inode *inode, struct file *file)
> > +{
> > +	return 0;
> 
> If a callback does nothing, odds are it is not needed at all.  Please
> just remove.
> 
> And why is this a global function?
> 
> 

Thanks. All empty functions will be removed.

> > +
> > +	/* get reg info for MMREG_BASE */
> > +	for (i = 0; i < TRINITY_MAX_MMREGS; i++) {
> > +		struct resource mmreg;
> > +
> > +		err = of_address_to_resource(np, i, &mmreg);
> > +		if (err < 0) {
> > +			dev_err(dev, "failed to get %d-th mmreg info", i);
> > +			goto err_cleanup;
> > +		}
> > +
> > +		drv->mmreg_vaddr[i] = devm_ioremap_resource(dev, &mmreg);
> > +		if (IS_ERR(drv->mmreg_vaddr[i])) {
> > +			dev_err(dev,
> > +				"failed to remap %d-th mmreg resource info", i);
> > +			err = PTR_ERR(drv->mmreg_vaddr[i]);
> > +			goto err_cleanup;
> > +		}
> > +		drv->mmreg_paddr[i] = mmreg.start;
> > +	}
> > +
> > +	/** get a TOPS property */
> 
> Why the odd "**" in comments?
> 
> thanks,
> 
> greg k-h
> 

It fixed.
Thanks for checking that. 

Thanks.
Jiho Chu