Re: [PATCH v4 0/8] i2c-atr and FPDLink

Vaittinen, Matti
2022-11-07T14:38:07Z

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! ~~