Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[Guenter Roeck]

On Tue, Jan 30, 2018 at 01:21:01PM +0000, shufan_lee(ææå) wrote:
> Hi Guenter,
> 
>   For now, it looks like there are two ways to implement vendor data. It would be nice to hear your suggestion.
> 
>   1. Set vendor data in the data field of of_device_id.
> If I understand correctly, this would be the one more like you mentioned before.
> In this case, tcpci_rt1711h_data needs to be defined inside tcpci.c or defined by other file(tcpci_rt1711h.c) but extern in tcpci.c.
> 
> For example:
> static struct tcpci_vendor_data tcpci_rt1711h_data = {
> .init = rt1711h_init;
> .irq_handler = rt1711h_irq_handler
> };
> OR
> extern struct tcpci_vendor_data tcpci_rt1711h_data;
> 
> Then, put this structure here
> static const struct of_device_id tcpci_of_match[] = {
> { .compatible = "usb,tcpci", },
> { .compatible = "richtek,rt1711h", .data = (void *)&tcpci_rt1711h_data },
> {},
> };
> 
> For other vendors who want to handle vendor data also need to add these code inside tcpci.c.
> We are not sure that's what you expect or not.
> 

I would not say expect, but it is one possibility. Sure,
it requires rt1711h_init and rt1711h_irq_handler to be public,
and a bit of ifdefery, but it is simpler than option #2.

Another option would be to instantiate tcpci from vendor drivers.
In this case, there would be an exported registration function which
would be called from tcpci_rt1711h.c:rt1711h_init(), similar to
tcpm_register_port(). In that case, tcpci_rt1711h.c would have its
own init function and compatible property.

To do that, you would effectively split tcpci_probe() into two functions,
tcpci_probe() and tcpci_register_port(), and call tcpci_register_port()
from the probe function.

int tcpci_register_port(struct i2c_client *client,
			const struct tcpci_vendor_data *data)
{
	/* pretty much verything currently done in the probe function */
}
EXPORT_SYMBOL(tcpci_register_port);

static int tcpci_probe(struct i2c_client *client,
                       const struct i2c_device_id *i2c_id)
{
	return tcpci_register_port(client, NULL);
}

Maybe you can experiment with this and see if it makes sense.
If not, you can still fall back to option #1.

Thanks,
Guenter