2010-11-03 02:15:19

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver



--- On Tue, 10/26/10, Cyril Chemparathy <[email protected]> wrote:

> From: Cyril Chemparathy <[email protected]>
> Subject: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

On the assumptions you've tested this *AND* will
resubmit with the previously-requested change of
removing all references to non-existent
"virtual"ness in the driver ... I'll likely
add my ack to that resubmitted version

Also, chip2gpio seems an excessively generic name;
maybe chip2_ti_ssp_gpio or somesuch?

I'd still be happier seeing this "stack" packaged
as a hardware library called by various drivers
(like GPIO, SPI, etc) rather than a core driver
that's called by other drivers. That seems like
a better structure for various vendors' "SSP"
hardware (multifunction serial interface logic)
since most function drivers just need to poke
the registers a bit differently, and don't have
much to share with a "core driver" beyond a few
setup routines (if that).

- Dave


2010-11-03 10:18:42

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

2010/11/3 David Brownell <[email protected]>:

> That seems like
> a better structure for various vendors' "SSP"
> hardware (multifunction serial interface logic)

Incidentally the ARM PrimeCell PL022 is called SSP,
"Synchronous Serial Port" and is not multifunction at all.
Just to add to the confusion...

Yours,
Linus Walleij

2010-11-03 12:35:58

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver



--- On Wed, 11/3/10, Linus Walleij <[email protected]> wrote:
\
> Date: Wednesday, November 3, 2010, 3:18 AM
> 2010/11/3 David Brownell <[email protected]>:
>
> > That seems like
> > a better structure for various vendors' "SSP"
> > hardware (multifunction serial interface logic)
>
> Incidentally the ARM PrimeCell PL022 is called SSP,
> "Synchronous Serial Port" and is not multifunction at all.

ISTR coming across that IP module; are you sure
that it only supports a single serial protocol,
instead of just a "small" variety" (multi)?
Unless the hardware only supports one protocol,
my point holds.

I wasn't implying that modules named SSP have
more in common than tending to support
more than a single serial protocol. If they
only support one, they get names like"USART"
or sometimes CODEC.

- Dave

2010-11-03 16:24:23

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

2010/11/3 David Brownell <[email protected]>:
> --- On Wed, 11/3/10, Linus Walleij <[email protected]> wrote:
>>
>> Incidentally the ARM PrimeCell PL022 is called SSP,
>> "Synchronous Serial Port" and is not multifunction at all.
>
> ISTR coming across that IP module; are you sure
> that it only supports a single serial protocol,
> instead of just a "small" variety" (multi)?
> Unless the hardware only supports one protocol,
> my point holds.

Yeah well:

/**
* enum ssp_interface - interfaces allowed for this SSP Controller
* @SSP_INTERFACE_MOTOROLA_SPI: Motorola Interface
* @SSP_INTERFACE_TI_SYNC_SERIAL: Texas Instrument Synchronous Serial
* interface
* @SSP_INTERFACE_NATIONAL_MICROWIRE: National Semiconductor Microwire
* interface
* @SSP_INTERFACE_UNIDIRECTIONAL: Unidirectional interface (STn8810
* &STn8815 only)
*/
enum ssp_interface {
SSP_INTERFACE_MOTOROLA_SPI,
SSP_INTERFACE_TI_SYNC_SERIAL,
SSP_INTERFACE_NATIONAL_MICROWIRE,
SSP_INTERFACE_UNIDIRECTIONAL
};

If that is what you mean then yes. All of the protols are
"SPI type" though.

Yours,
Linus Walleij

2010-11-10 04:45:38

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

On Tue, Nov 02, 2010 at 07:15:16PM -0700, David Brownell wrote:
>
>
> --- On Tue, 10/26/10, Cyril Chemparathy <[email protected]> wrote:
>
> > From: Cyril Chemparathy <[email protected]>
> > Subject: [PATCH v4 08/12] gpio: add ti-ssp gpio driver
>
> On the assumptions you've tested this *AND* will
> resubmit with the previously-requested change of
> removing all references to non-existent
> "virtual"ness in the driver ... I'll likely
> add my ack to that resubmitted version
>
> Also, chip2gpio seems an excessively generic name;
> maybe chip2_ti_ssp_gpio or somesuch?
>
> I'd still be happier seeing this "stack" packaged
> as a hardware library called by various drivers
> (like GPIO, SPI, etc) rather than a core driver
> that's called by other drivers. That seems like
> a better structure for various vendors' "SSP"
> hardware (multifunction serial interface logic)
> since most function drivers just need to poke
> the registers a bit differently, and don't have
> much to share with a "core driver" beyond a few
> setup routines (if that).

I thought the point of this device was that a single device hosted a
pair of multi-function serial interfaces, with each implementing a
separate function. If so, then it makes sense for the base driver to
register child devices of the appropriate kinds.

g.

2010-11-10 06:16:28

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver


> I thought the point of this device was that a single [SSP] device
> hosted a
> pair of multi-function serial interfaces, with each
> implementing a
> separate function.

function chosen based on what the board needs.
Codec interface, SPI, GPIO, etc.

? If so, then it makes sense for the
> base driver to
> register child devices of the appropriate kinds.

I'd normally say board setup registers them; a
"core"driver can't know what children would be needed.

But the point I was making was about code factoring
not driver setup. When the functions don't have
much commonality, they might as well just write to
the relevant registers instead of expecting to have
a non-register programming interface (of dubious
generality of a "core" driver, but much complexity).

Easier just to have children use registers directly,
in several similar cases. Less overhead, too.

- Dave

2010-11-10 06:23:14

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

On Tue, Nov 09, 2010 at 10:16:22PM -0800, David Brownell wrote:
>
> > I thought the point of this device was that a single [SSP] device
> > hosted a
> > pair of multi-function serial interfaces, with each
> > implementing a
> > separate function.
>
> function chosen based on what the board needs.
> Codec interface, SPI, GPIO, etc.
>
> ? If so, then it makes sense for the
> > base driver to
> > register child devices of the appropriate kinds.
>
> I'd normally say board setup registers them; a
> "core"driver can't know what children would be needed.
>
> But the point I was making was about code factoring
> not driver setup. When the functions don't have
> much commonality, they might as well just write to
> the relevant registers instead of expecting to have
> a non-register programming interface (of dubious
> generality of a "core" driver, but much complexity).
>
> Easier just to have children use registers directly,
> in several similar cases. Less overhead, too.

I guess it depends on how much overlap/interlock there is between the
multiple channels. If there is shared context, then that is a
stronger argument for a shared api. Cyril, what say you?

g.

2010-11-10 14:35:11

by Cyril Chemparathy

[permalink] [raw]
Subject: Re: [PATCH v4 08/12] gpio: add ti-ssp gpio driver

On 11/10/2010 01:23 AM, Grant Likely wrote:
> On Tue, Nov 09, 2010 at 10:16:22PM -0800, David Brownell wrote:
>>
>>> I thought the point of this device was that a single [SSP] device
>>> hosted a
>>> pair of multi-function serial interfaces, with each
>>> implementing a
>>> separate function.
>>
>> function chosen based on what the board needs.
>> Codec interface, SPI, GPIO, etc.
>>
>> If so, then it makes sense for the
>>> base driver to
>>> register child devices of the appropriate kinds.
>>
>> I'd normally say board setup registers them; a
>> "core"driver can't know what children would be needed.
>>
>> But the point I was making was about code factoring
>> not driver setup. When the functions don't have
>> much commonality, they might as well just write to
>> the relevant registers instead of expecting to have
>> a non-register programming interface (of dubious
>> generality of a "core" driver, but much complexity).
>>
>> Easier just to have children use registers directly,
>> in several similar cases. Less overhead, too.
>
> I guess it depends on how much overlap/interlock there is between the
> multiple channels. If there is shared context, then that is a
> stronger argument for a shared api. Cyril, what say you?
>

The channels (or ports) in this case are not very well separated out.
The registers for these ports are interleaved, and in some cases
different bits of the same register are meant for different ports.

Second, with the exception of GPIO (which essentially bit bangs), all
other functions would follow the same flow, i.e. set stuff up (mode,
iosel), load up a sequence, kick off execution, and wait for completion.
I thought it made sense to provide these pieces in a shared driver.

Regards
Cyril.