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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180130232521.GA19569@roeck-us.net>
Date:   Tue, 30 Jan 2018 15:25:21 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     shufan_lee(李書帆) <shufan_lee@...htek.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        'Jun Li' <jun.li@....com>, ShuFanLee <leechu729@...il.com>,
        cy_huang(黃啟原) <cy_huang@...htek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

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