Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Jan 2018 15:25:21 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     =?utf-8?B?c2h1ZmFuX2xlZSjmnY7mm7jluIYp?= <shufan_lee@richtek.com>
Cc:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        'Jun Li' <jun.li@nxp.com>, ShuFanLee <leechu729@gmail.com>,
        =?utf-8?B?Y3lfaHVhbmco6buD5ZWf5Y6fKQ==?= <cy_huang@richtek.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
Message-ID: <20180130232521.GA19569@roeck-us.net>
References: <eb1dfcd6f98545acac817db3289b1295@ex1.rt.l>
 <20180119082218.GA22976@kuha.fi.intel.com>
 <25ced79e8ea84908bf6110a613ed81a2@ex1.rt.l>
 <20180119092413.GB22976@kuha.fi.intel.com>
 <20180119160235.GA21066@roeck-us.net>
 <dddc2060d19b47c69b582c9de51a85b1@ex1.rt.l>
 <20180122185034.GA26058@roeck-us.net>
 <b7e6482007584692972ffbdfd4238e6c@ex1.rt.l>
 <20180129195752.GA24352@roeck-us.net>
 <be2a92a6c60e40b190bc2b3b0981533e@ex1.rt.l>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <be2a92a6c60e40b190bc2b3b0981533e@ex1.rt.l>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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