Message-ID: <b0319f7c-54af-3132-2775-fba7dcad6bbe@fi.rohmeurope.com>
Date:   Mon, 7 Nov 2022 14:37:58 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>
CC:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Luca Ceresoli <luca@...aceresoli.net>,
        Mark Rutland <mark.rutland@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Rob Herring <robh+dt@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        "satish.nagireddy@...cruise.com" <satish.nagireddy@...cruise.com>
Subject: Re: [PATCH v4 0/8] i2c-atr and FPDLink

On 11/1/22 15:20, Tomi Valkeinen wrote:
> Hi,
> 
> Intro
> -----
> 
> This is, kind of, v4 of Luca's i2c-atr and FPDLink series, v3 of which
> you can find from:
> 
> https://lore.kernel.org/all/20220206115939.3091265-1-luca@lucaceresoli.net/
> 

//snip

First of all, I like this series.

There is just one thing that slightly bothers me here. It is putting 
everything the deserializer has inside one driver. I think I mentioned 
this already earlier - but to me these devices seem like MFD ICs.

Hence, my attempt to support one of SerDes devices resembling the TI 
SerDes devices had following split. MFD I2C drivers for SER and DES. Own 
platform drivers for DES ATR, V4L2, pinctrl/GPIO. Also own platform 
drivers for SER V4L2, pinctrl/GPIO.

Below you can see the relevant pieces of simplified (pseudo-)code 
explaining the (optional?) design which attempts creating somewhat 
smaller and re-usable individual drivers for DES/SER blocks. Please, do 
not treat this as a requirement - please treat it as a food for thoughts ;)


DES MFD-core: as I2C - device. Registers regmap and IRQ-chip for 
sub-devices. Creates the sub-devices for DES. Parses the required SerDes 
topology from device-tree and makes the minimum required configuration 
to get the actual link (FPDLink in TI's case) run in a level that I2C 
connection is possible when ATR is configured. (Actually, I noticed this 
could be put in ATR driver, but in my opinion it's better to parse the 
overall DT in MFD and put it in the MFD driver-data so all sub-devices 
can get the parsed bits of DT data from parent w/o duplicating the 
parsing code).

static struct mfd_cell bu18rm84_mfd_cells[] = {
	{ .name = "bu18rm84-i2c", },
	{ .name = "bu18rm84-video", },
	{ .name = "bu18rm84-pinctrl", },
};

...

static int link_init(struct device *dev, struct regmap *regmap,
		    struct bu18rm84_core_data *data,
		    struct bu18rm84_ser_conf *ser, int ser_id)
{
	...
}

static int prepare_hw_for_subdevs(struct device *dev, struct regmap *regmap,
				  struct bu18rm84_core_data *data)
{
	int ret, i;

	ret = bu18rm84_common_init_pre(dev, regmap, data);
	if (ret)
		return ret;

	for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) {
		if (data->ser[i].ser_ep) {
			/*
			 * We initialize link between DES => SERsin MFD so
			 * the sub-devices have working link to SER devices.
			 */
			ret = link_init(dev, regmap, data, &data->ser[i], i);
			if (ret)
				return ret;
		}
	}

	...

	return ret;
}

static int connected_devices(struct device *dev, struct 
bu18rm84_core_data *data)
{
	int i;

	for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) {
		/*
		 * TODO: See where/when/if we should drop the reference to
		 * the node
		 */
		data->ser[i].ser_ep = fwnode_graph_get_endpoint_by_id(data->fw, i,
								  0, 0);
		if (data->ser[i].ser_ep) {
			dev_dbg(dev, "Found node for SER %d\n", i);
			data->num_ser++;
		}
	}


	for (; i < BU18RM84_MAX_NUM_SER_NODES + BU18RM84_MAX_NUM_CSI_NODES; i++) {
		int csi = i - BU18RM84_MAX_NUM_SER_NODES;

		data->csi_ep[csi] = fwnode_graph_get_endpoint_by_id(data->fw, i,
								    0, 0);
		if (data->csi_ep[csi])
			data->num_csi++;
	}
	dev_dbg(dev, "found %d ser, %d csi\n", data->num_ser, data->num_csi);

	return (!data->num_ser) ? -ENODEV : 0;
}

static int bu18rm84_i2c_probe(struct i2c_client *i2c)
{
	data = devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL);
	regmap = devm_regmap_init_i2c(i2c, &bu18rm84_regmap);

	ret = connected_devices(&i2c->dev, data);

	/*
	 * We need to set-up the link between DES and SER(s) before
	 * the I2C to SER(s) is operational. Sub-devices like the SER GPIO, pinmux
	 * or media do need this I2C so we'd better to prepare things up-to-the
	 * point where I2C works prior registering the subdev cells.
	 *
	 * Unfortunately, this means the MFD needs to go and read & understand
	 * the DT for SER topology - and do some magic settings to get things
	 * up & running.
	 */
	ret = prepare_hw_for_subdevs(&i2c->dev, regmap, data);

	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, i2c->irq,
				       IRQF_SHARED, 0, &bu18rm84_irq_chip,
				       &irq_data);

	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
				   bu18rm84_mfd_cells,
				   ARRAY_SIZE(bu18rm84_mfd_cells),
				   NULL, 0, regmap_irq_get_domain(irq_data));

}

===============

DES ATR platform driver: Kicked by DES MFD. Uses I2C-atr to configure 
address translation. Creates SER devices and creates the I2C adapter for 
the remote SER buses. This in-turn launches SER MFD driver + remote I2C 
slave devices. And yes. I think the remote I2C slave devices must not be 
created before SER side link configurations are done by SER MFD driver. 
Thanks Tomi - I guess I've been lucky this far (or there is something I 
didn't spot quite yet ;] )

static int bu18rm84_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
				   const struct i2c_board_info *info,
				   const struct i2c_client *client,
				   u16 *alias_id)
{
	/* Configure the ATR for slave to DES registers */
}

static void bu18rm84_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
				    const struct i2c_client *client)
{
	/* Clear the configuration for the ATR for slave at DES registers */
}

static const struct i2c_atr_ops bu18rm84_atr_ops = {
	.attach_client = bu18rm84_atr_attach_client,
	.detach_client = bu18rm84_atr_detach_client,
};

static int bu18rm84_populate_ser(struct bu18rm84_i2c_atr *data, u32 *addrs,
				 int num_addrs, struct fwnode_handle *des_node,
				 int ser_id)
{
	/*
	 * Configure the I2C alias for SER devices in DES registers based on
	 * reg-names in the DT
	 */
	idx = fwnode_property_match_string(des_node, "reg-names",
					   ser_names[ser_id]);
	...
}

static int setup_ser_i2c(struct bu18rm84_i2c_atr *data, int ser_id,
			 struct fwnode_handle *ser_ep)
{
	int ret;
	const char *name;
	struct i2c_client **new = &data->ser_client[ser_id];
	struct i2c_board_info ser_info = { 0 };

	ser_info.fwnode = fwnode_get_named_child_node(ser_ep, "remote-chip");
	name = fwnode_get_name(ser_info.fwnode);

	ser_info.addr = data->ser_alias[ser_id];
	*new = i2c_new_client_device(data->parent_i2c->adapter, &ser_info);

	/* Add adapter to correctly enable i2c to devices behind SER */
	/* We need to have ATR done before this */
	ret = i2c_atr_add_adapter(data->atr, ser_id);
	...

	return ret;
}

static int bu18rm84_setup_ser_i2cs(const struct bu18rm84_core_data *core,
				   struct bu18rm84_i2c_atr *data)
{
	for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) {
		if (core->ser[i].ser_ep) {
			ret = setup_ser_i2c(data, i, core->ser[i].ser_ep);
			...
		}
	}
}

static int bu18rm84_atr_probe(struct platform_device *pdev)
{
	parent = dev->parent;
	core = dev_get_drvdata(parent);

	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);

	data->parent_i2c = to_i2c_client(parent);

	data->regmap = dev_get_regmap(parent, NULL);

	dev_set_drvdata(&pdev->dev, data);

	/* Go read the I2C addresses from DT */
	ret = bu18rm84_i2c_parse_dt(data, core);

	/*
	 * Create device for SER. We won't add SER to ATR in order to avoid
	 * messing with the SER specific aliases in ATR.
	 *
	 * Also, add adapter.
	 */
	for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) {
		if (core->ser[i].ser_ep) {
			ret = bu18rm84_populate_ser(data, &addrs[0], num_addrs,
						    core->fw, i);
		}
	}

	data->atr = i2c_atr_new(data->parent_i2c->adapter, parent,
				&bu18rm84_atr_ops, 4);

	i2c_atr_set_clientdata(data->atr, data);

	return bu18rm84_setup_ser_i2cs(core, data);

=============================

Ser MFD (i2c)driver. Kicked by DES ATR platform device when creating the 
I2C devices with the SER MFD fwnode. Finalizes the SER <=> DES I2C 
connection and kicks up the SER slave devices for DES pinctrl / V4L2 or 
others.


static struct mfd_cell bu18tm41_mfd_cells[] = {
	{ .name = "bu18tm41-video", },	/* Video forwarding */
	{ .name = "bu18tm41-pinctrl", }, /* pinmux, pinconf and gpio */
};

static int bu18tm41_conf_ser(struct device *dev, struct regmap *regmap,
			     struct bu18tm41_core *core)
{
	fw = dev_fwnode(dev);

	ret = fwnode_property_read_u64(fw, "compulsory stuff to get link up",
				       &foo);

	ret = bu18tm41_enable_link( ... );

	...

	return 0;
}

static int bu18tm41_i2c_probe(struct i2c_client *i2c)
{
	regmap = devm_regmap_init_i2c(i2c, &bu18tm41_regmap);
	core = devm_kzalloc(&i2c->dev, sizeof(*core), GFP_KERNEL);
	dev_set_drvdata(&i2c->dev, core);

	/*
  	 * Do the minimum required link config on SER config to get I2C
  	 * between DES <=> SER up
	 *
	 * Unfortunately, this means the MFD needs to do some magic settings
	 * to get things up & running.
	 */
	ret = bu18tm41_conf_ser(&i2c->dev, regmap, core);

	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
				   bu18tm41_mfd_cells,
				   ARRAY_SIZE(bu18tm41_mfd_cells),
				   NULL, 0, /*regmap_irq_get_domain(irq_data) */ NULL);

	return ret;
}

===========================

Des media platform driver. V4L2/media stuff.

...

static int bu18rm84_media_probe(struct platform_device *pdev)
{

	core = dev_get_drvdata(dev->parent);

	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);

	data->dev = dev;
	data->parent_i2c = to_i2c_client(dev->parent);
	data->regmap = dev_get_regmap(dev->parent, NULL);

	platform_set_drvdata(pdev, data);


	ret = bu18rm84_parse_csi_dt(core, data);

	ret = bu18rm84_config_csi2(data);

	ret = bu18rm84_add_ser_data(core, data);

	...

	/* Create the V4L2 subdevices */
	ret = bu18rm84_create_subdev(core, data);
	if (ret)
		goto err_subdev;

	...
}

SER video, Des/Ser pinctrl/GPIO platform drivers:
...
  (Like usual)


I think it was Tomi who asked me the benefit of using MFD. In some cases 
the digital interface towards pinctrl/GPIO or other functional blocks in 
SER/DES is re-used from other products - or the blocks are re-used on 
other products. Separating them in own platform-drivers is a nice way to 
re-use drivers and avoid code duplication.

Whether it is worth or correct can be evaluated by brighter minds :) As 
I said, I am in a no way demanding a change if you see the current 
proposal being better! I am just presenting an optional way to feed your 
thoughts ;)

Ps. Sorry for _very_ stripped down code in this explanation. I am 
currently not allowed to publish any details of the ICs these drivers 
are written for.

Yours
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Powered by blists - more mailing lists