RE: [PATCH resend v9 1/8] i3c: add target mode support

From: Joshua Yeong <joshua.yeong@starfivetech.com>
To: Frank Li <Frank.Li@nxp.com>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>
Cc: "conor.culhane@silvaco.com" <conor.culhane@silvaco.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
	"imx@lists.linux.dev" <imx@lists.linux.dev>,
	"jirislaby@kernel.org" <jirislaby@kernel.org>,
	"joe@perches.com" <joe@perches.com>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"krzysztof.kozlowski@linaro.org" <krzysztof.kozlowski@linaro.org>,
	"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
	"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
	"robh@kernel.org" <robh@kernel.org>,
	"zbigniew.lukwinski@linux.intel.com"
	<zbigniew.lukwinski@linux.intel.com>
Subject: RE: [PATCH resend v9 1/8] i3c: add target mode support
Date: Mon, 13 May 2024 03:16:05 +0000	[thread overview]
Message-ID: <SH0PR01MB084187FBA1E4EE786A8996C9F9E22@SH0PR01MB0841.CHNPR01.prod.partner.outlook.cn> (raw)
In-Reply-To: <20240507204311.2898714-2-Frank.Li@nxp.com>

Joshua Yeong wrote:
> 
> Introduce a new target core layer in order to support target functions in linux
> kernel. This comprises the controller library and function library.
> Controller library implements functions specific to an target controller and
> function library implements functions specific to an target function.
> 
> Introduce a new configfs entry to configure the target function configuring
> and bind the target function with target controller.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> Notes:
>     Change from v8 to v9
>     -none
> 
>     Change from v7 to v8
>     Added missed head files
> 
>     Change from v5 to v6
>     - fixed build error when have not CONFIG_TARGET_CONFIGFS
>     | Reported-by: kernel test robot <lkp@intel.com>
>     | Closes: https://lore.kernel.org/oe-kbuild-all/202402030437.GdGCrKeK-
> lkp@intel.com/
> 
>     Chagne from v4 to v5
>     - add include <linux/slab.h> to fix build error
>     | Reported-by: kernel test robot <lkp@intel.com>
>     | Closes: https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> lkp@intel.com/
> 
>     Chagne from v4 to v8
>     - add include <linux/slab.h> to fix build error
>     | Reported-by: kernel test robot <lkp@intel.com>
>     | Closes: https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> lkp@intel.com/
> 
>  drivers/i3c/Kconfig        |  28 +-
>  drivers/i3c/Makefile       |   2 +
>  drivers/i3c/i3c-cfs.c      | 389 ++++++++++++++++++++++++++
>  drivers/i3c/target.c       | 453 ++++++++++++++++++++++++++++++
>  include/linux/i3c/target.h | 548
> +++++++++++++++++++++++++++++++++++++
>
> ...
>
> diff --git a/include/linux/i3c/target.h b/include/linux/i3c/target.h new file
> mode 100644 index 0000000000000..b0bf06685834c
> --- /dev/null
> +++ b/include/linux/i3c/target.h
>
> ...
>
> +/**
> + * struct i3c_target_ctrl_ops - set of function pointers for performing
> +controller operations
> + * @set_config: set I3C controller configuration
> + * @enable: enable I3C controller
> + * @disable: disable I3C controller
> + * @raise_ibi: raise IBI interrupt to master
> + * @alloc_request: allocate a i3c_request, optional, default use
> +kmalloc
> + * @free_request: free a i3c_request, default use kfree
> + * @queue: queue an I3C transfer
> + * @dequeue: dequeue an I3C transfer
> + * @cancel_all_reqs: call all pending requests
> + * @fifo_status: current FIFO status
> + * @fifo_flush: flush hardware FIFO
> + * @hotjoin: raise hotjoin request to master
> + * @set_status_format1: set i3c status format1
> + * @get_status_format1: get i3c status format1
> + * @get_addr: get i3c dynmatic address
> + * @get_features: ops to get the features supported by the I3C target
> +controller
> + * @owner: the module owner containing the ops  */ struct
> +i3c_target_ctrl_ops {
> +	int (*set_config)(struct i3c_target_ctrl *ctrl, struct i3c_target_func
> *func);
> +	int (*enable)(struct i3c_target_ctrl *ctrl);
> +	int (*disable)(struct i3c_target_ctrl *ctrl);
> +	int (*raise_ibi)(struct i3c_target_ctrl *ctrl, void *p, u8 size);
> +
> +	struct i3c_request *(*alloc_request)(struct i3c_target_ctrl *ctrl, gfp_t
> gfp_flags);
> +	void (*free_request)(struct i3c_request *req);
> +
> +	int (*queue)(struct i3c_request *req, gfp_t gfp_flags);
> +	int (*dequeue)(struct i3c_request *req);
> +
> +	void (*cancel_all_reqs)(struct i3c_target_ctrl *ctrl, bool tx);
> +
> +	int (*fifo_status)(struct i3c_target_ctrl *ctrl, bool tx);
> +	void (*fifo_flush)(struct i3c_target_ctrl *ctrl, bool tx);
> +	int (*hotjoin)(struct i3c_target_ctrl *ctrl);
> +	int (*set_status_format1)(struct i3c_target_ctrl *ctrl, u16 status);
> +	u16 (*get_status_format1)(struct i3c_target_ctrl *ctrl);
> +	u8  (*get_addr)(struct i3c_target_ctrl *ctrl);
> +	const struct i3c_target_ctrl_features *(*get_features)(struct
> i3c_target_ctrl *ctrl);
> +	struct module *owner;
> +};

This structure looks very different from the master controller ops 'i3c_master_controller_ops'.
I think you could probably combine 'set_config, enable' into 'bus_init', 'disable' to 'bus_cleanup'.
Also using the 'struct i3c_priv_xfer' rather redefining another 'struct i3c_request' would be much cleaner.

In the master i3c side they don't abbreviate i3c_master_controller to i3c_master_ctrl. I think we should
do the same here to standardize it. Thanks

Regards,
Joshua

> --
> 2.34.1
> 
> 
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c