RE: [PATCH 1/3] ti-st: use device handles and add device tree binding

From: "Reizer, Eyal" <eyalr-l0cyMroinI0@public.gmane.org>
To: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"pawel.moll-5wv7dgnIgG8@public.gmane.org"
	<pawel.moll-5wv7dgnIgG8@public.gmane.org>,
	"mark.rutland-5wv7dgnIgG8@public.gmane.org"
	<mark.rutland-5wv7dgnIgG8@public.gmane.org>,
	"ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org"
	<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
	"galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org"
	<galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
	"tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org"
	<tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>,
	"linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org"
	<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>,
	"Balbi, Felipe" <balbi-l0cyMroinI0@public.gmane.org>
Subject: RE: [PATCH 1/3] ti-st: use device handles and add device tree binding
Date: Tue, 26 Jan 2016 14:38:54 +0000	[thread overview]
Message-ID: <8665E2433BC68541A24DFFCA87B70F5B237793A1@DFRE01.ent.ti.com> (raw)
In-Reply-To: 20151229183534.GM12450@rob-hp-laptop

Hi Rob,

Ping on this. Have you seen my comments below?
I am trying to understand if fixing the smaller comments discussed below would allow this 
patch-set to be accepted so we at least have a working support as with previous kernels?
Or do we basically drop support for ti_st until it is implemented as a slave device?

Can you please let me know your thoughts?

Best Regards,
Eyal

> -----Original Message-----
> From: Reizer, Eyal
> Sent: Sunday, January 17, 2016 9:57 AM
> To: 'Rob Herring'
> Cc: devicetree@vger.kernel.org; linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org; tony@atomide.com;
> linux@arm.linux.org.uk; Balbi, Felipe
> Subject: RE: [PATCH 1/3] ti-st: use device handles and add device tree binding
> 
> Sorry for the delayed response.
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, December 29, 2015 8:36 PM
> > To: Reizer, Eyal
> > Cc: devicetree@vger.kernel.org; linux-omap@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; tony@atomide.com;
> > linux@arm.linux.org.uk
> > Subject: Re: [PATCH 1/3] ti-st: use device handles and add device tree
> > binding
> >
> > On Wed, Dec 23, 2015 at 11:38:29AM +0000, Reizer, Eyal wrote:
> > > - Add support for getting the platform data which includes the uart
> > >   used and gpio pin used for enable from device tree.
> > >
> > > - Fix the implementation for using device handle for the uart and
> > >   gpiod for the enable pin, instead of device name (as string) used
> > >   for the uart and pio number which are both bad practice.
> > >
> > > Signed-off-by: Eyal Reizer <eyalr@ti.com>
> > > ---
> > >  Documentation/devicetree/bindings/misc/ti-st.txt |   42 ++++++
> > >  arch/arm/mach-omap2/pdata-quirks.c               |   16 ++-
> > >  drivers/misc/ti-st/st_kim.c                      |  159 ++++++++++++++++------
> > >  drivers/misc/ti-st/st_ll.c                       |   16 ++-
> > >  include/linux/ti_wilink_st.h                     |   13 +-
> >
> > I'd suggest you look at commit c0bd1b9e58959c5 (Revert "ti-st: add
> > device tree support") first.
> >
> > >  5 files changed, 190 insertions(+), 56 deletions(-)  create mode
> > > 100644 Documentation/devicetree/bindings/misc/ti-st.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/misc/ti-st.txt
> > > b/Documentation/devicetree/bindings/misc/ti-st.txt
> > > new file mode 100644
> > > index 0000000..4490da6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/misc/ti-st.txt
> > > @@ -0,0 +1,42 @@
> > > +TI Wilink 6/7/8 (wl12xx/wl18xx) Shared transport driver
> >
> > Bindings shouldn't be describing drivers...
> >
> 
> OK, understood
> 
> > > +
> > > +TI’s Wireless Connectivity chips support Bluetooth (BT), WiFi, and
> > > +GPS technology cores in a single die.
> > > +
> > > +Such a multi-core combo chip will be interfaced to the application
> > > +processor using a single physical port (like UART).
> > > +
> > > +Shared Transport (ST) software enables BT and GPS protocols or
> > > +software components to interact with their respective cores over
> > > +single
> > physical port.
> > > +ST uses logical channels, over physical transport, to communicate
> > > +with individual cores.
> > > +
> > > +Logical channels 1, 2, 3, and 4 are used for BT packets, channel 8
> > > +for FM, channel 9 for GPS and channels 30, 31, 32, and 33 are used
> > > +for Chip Power Management (PM).
> >
> > All this is irrelevant for a binding.
> >
> 
> OK, understood
> 
> > > +
> > > +This node provides properties for passing parameters to the ti
> > > +shared transport driver.
> > > +
> > > +Required properties:
> > > + - compatible: should be the following:
> > > +    * "kim" - ti-st parameters
> >
> > Who is kim? Certainly not a description of a h/w block.
> 
> Not sure about the origin of this name but according to the following link:
> http://processors.wiki.ti.com/index.php/Shared_Transport_Driver
> KIM is "Kernel Initialization Manager" that enables communication with BT and
> GPS cores.
> 
> >
> > > +
> > > +Optional properties:
> > > + - nshutdown-gpios : specifies attributes for gpio ping used for enabling
> > > +	the bluetooth,gps and FM sub systems
> > > + - serial-device : the phandle for the phisical uart used for interacting
> > > + 	with the wilink device
> >
> > There have been multiple discussions on serial slave devices recently.
> > I'm not going to accept any device binding without a common uart slave
> > device binding first.
> >
> 
> Perhaps I am reading it wrong but I think this is a different discussion.
> The shared transport driver is already in the kernel for pretty long time.
> AFAIK the original author is not around to maintain it.
> Currently it is useless  as no bindings exist for it and all customers I have seen
> using ti_st that upgrade to newer kernels have broken support and have to
> manually patch the kernel for adding bindings for it.
> 
> Having a new uart slave device that may provide similar functionality is a
> different discussion as it would require a total different implementation.
> But what do we do now with the implementation that is already there.
> Don't we want it to work and at least have working bindings for it?
> 
> > > + - flow_cntrl : Indicates if uart flow control is used
> > > + - flow_cntrl : uart baud rate in BPS
> >
> > Typo here, but these should be part of a common serial slave binding.
> >
> > Don't use '_' in property names.
> >
> 
> Will fix this
> 
> > > +
> > > +Example:
> > > +
> > > +kim {
> > > +	compatible = "kim";
> > > +	nshutdown-gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> > > +	serial-device = <&uart1>;
> > > +	flow_cntrl = <1>;
> > > +	flow_cntrl = <3000000>;
> > > +};
> > > +
> 
> Best Regards,
> Eyal