Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Greg Kroah-Hartman]

On Mon, Feb 01, 2021 at 06:09:25PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 01, 2021 at 04:19:38PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Feb 01, 2021 at 05:12:53PM +0200, Heikki Krogerus wrote:
> > > On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
> > > > The USB Communications Capable bit indicates if port
> > > > partner is capable of communication over the USB data lines
> > > > (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> > > > level drivers to perform chip specific operation.
> > > > For instance, low level driver enables USB switches on D+/D-
> > > > lines to set up data path when the bit is set.
> > > > 
> > > > Refactored from patch initially authored by
> > > > Kyle Tso <kyletso@xxxxxxxxxx>
> > > > 
> > > > Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
> > > >  include/linux/usb/tcpm.h      |  5 +++++
> > > >  2 files changed, 21 insertions(+)
> > > 
> > > ...
> > > 
> > > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > > > index 3af99f85e8b9..42fcfbe10590 100644
> > > > --- a/include/linux/usb/tcpm.h
> > > > +++ b/include/linux/usb/tcpm.h
> > > > @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
> > > >   *		is supported by TCPC, set this callback for TCPM to query
> > > >   *		whether vbus is at VSAFE0V when needed.
> > > >   *		Returns true when vbus is at VSAFE0V, false otherwise.
> > > > + * @set_partner_usb_comm_capable:
> > > > + *              Optional; The USB Communications Capable bit indicates if port
> > > > + *              partner is capable of communication over the USB data lines
> > > > + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> > > >   */
> > > >  struct tcpc_dev {
> > > >  	struct fwnode_handle *fwnode;
> > > > @@ -139,6 +143,7 @@ struct tcpc_dev {
> > > >  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> > > >  						 bool pps_active, u32 requested_vbus_voltage);
> > > >  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > > > +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> > > >  };
> > > >  
> > > >  struct tcpm_port;
> > > 
> > > There start to be a lot of callback there, separate for each function.
> > > And I guess flags too... Would it be possible to have a single
> > > notification callback instead, that would take the type of the
> > > notification as a parameter (we could have an enum for those), and
> > > then the specific object(s) for each type as another paramter (RDO I
> > > guess in this case)?
> > > 
> > > It would then be up to the TCPC driver to extract the detail it needs
> > > from that object. That would somehow feel more cleaner to me, but what
> > > do you guys think?
> > 
> > It's pretty much the same thing, a "mux" function vs. individual
> > function calls.  Personally, individual callbacks are much more
> > explicit, and I think make it easier to determine what is really going
> > on in each driver.
> > 
> > But it all does the same thing, if there's going to be loads of
> > callbacks needed, then a single one makes it easier to maintain over
> > time.
> > 
> > So it's up to the maintainer what they want to see :)
> 
> I understand your point, and I guess a "generic" notification callback
> for all that would not be a good idea. However, right now it looks
> like we are picking individual bits from various PD objects with those
> callbacks, and that does not feel ideal to me either. After all, each of
> those bits has its own flag now, even though the details is just
> extracted from some PD object that we should also have access to.
> 
> I think there are ways we can improve this for example by attempting
> to create the notifications per transaction instead of for each
> individual result of those transactions. That way we would not need to
> store the flags at least because we could deliver the entire object
> that was the result of the specific transaction.
> 
> So basically, I fear that dealing with these individual bits will in
> many case only serve individual device drivers, and in the worst case
> start making the tcpm.c a bit more difficult to manage if we start to
> have more and more of these bit callbacks.
> 
> But on the other hand, I guess we are nowhere near that point, so
> let's forget about this for now.

If it gets unwieldy, we can always change it in the future, there's no
reason these types of in-kernel apis can not be modified and cleaned up
over time.

thanks,

greg k-h