Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
> On some SPI controllers it is not feasible to transfer arbitrary amount
> of data at once.
> 
> When the limit on transfer size is a few kilobytes at least it makes
> sense to use the SPI hardware rather than reverting to gpio driver.

> +	/*
> +	 * on some hardware transfer size may be constrained
> +	 * the limit may depend on device transfer settings
> +	 */
> +	size_t (*max_transfer_size)(struct spi_device *spi);

Heiner submitted a *very* similar patch just now with a straight
variable plus accessor instead of a function and using a name with _msg.
I'm ambivalent on the implementation but prefer the naming here since
that's more the limitation we're trying to express I think (some
hardware does have limiations about multple transfers too).  Can the two
of you come up with something that works for both of you?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Heiner Kallweit]

Am 01.12.2015 um 20:58 schrieb Mark Brown:
> On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
>> On some SPI controllers it is not feasible to transfer arbitrary amount
>> of data at once.
>>
>> When the limit on transfer size is a few kilobytes at least it makes
>> sense to use the SPI hardware rather than reverting to gpio driver.
> 
>> +	/*
>> +	 * on some hardware transfer size may be constrained
>> +	 * the limit may depend on device transfer settings
>> +	 */
>> +	size_t (*max_transfer_size)(struct spi_device *spi);
> 
> Heiner submitted a *very* similar patch just now with a straight
> variable plus accessor instead of a function and using a name with _msg.
> I'm ambivalent on the implementation but prefer the naming here since
> that's more the limitation we're trying to express I think (some
> hardware does have limiations about multple transfers too).  Can the two
> of you come up with something that works for both of you?
> 
Sure .. Just one inquiry:
When you say "the naming here" you refer to Michal's or my version?

Actually I like in Michal's hook that it directly takes a struct spi_device.
This saves the caller one level of indirection as the caller usually will
deal with a spi_device and not a spi_master.

If you're fine with Michal's version then this is also fine with me,
especially as the functionality is the same.

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

On Tue, Dec 01, 2015 at 10:07:55PM +0100, Heiner Kallweit wrote:

> Sure .. Just one inquiry:
> When you say "the naming here" you refer to Michal's or my version?

Michael's (transfer).

> Actually I like in Michal's hook that it directly takes a struct spi_device.
> This saves the caller one level of indirection as the caller usually will
> deal with a spi_device and not a spi_master.

> If you're fine with Michal's version then this is also fine with me,
> especially as the functionality is the same.

OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Michal Suchanek]

On 1 December 2015 at 20:58, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
>> On some SPI controllers it is not feasible to transfer arbitrary amount
>> of data at once.
>>
>> When the limit on transfer size is a few kilobytes at least it makes
>> sense to use the SPI hardware rather than reverting to gpio driver.
>
>> +     /*
>> +      * on some hardware transfer size may be constrained
>> +      * the limit may depend on device transfer settings
>> +      */
>> +     size_t (*max_transfer_size)(struct spi_device *spi);
>
> Heiner submitted a *very* similar patch just now with a straight
> variable plus accessor instead of a function and using a name with _msg.
> I'm ambivalent on the implementation but prefer the naming here since
> that's more the limitation we're trying to express I think (some
> hardware does have limiations about multple transfers too).  Can the two
> of you come up with something that works for both of you?

Sorry, missed there is a patch because it shows in the middle of the
discussion for me.

I don't really care which one it is so long as I can get the last
patch in this series based on it.

Thanks

Michal

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:

> +static inline size_t
> +spi_max_transfer_size(struct spi_device *spi)
> +{
> +	struct spi_master *master = spi->master;
> +	if (!master->max_transfer_size)
> +		return 0;
> +	return master->max_transfer_size(spi);
> +}

Can we change this to return SIZE_MAX instead (ie, the maximum value for
a size_t)?  That way callers don't need to worry if there is a limit, if
they want to handle it they can just unconditionally assume that a limit
will be provided.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Michal Suchanek]

On 2 December 2015 at 00:12, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
>
>> +static inline size_t
>> +spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +     struct spi_master *master = spi->master;
>> +     if (!master->max_transfer_size)
>> +             return 0;
>> +     return master->max_transfer_size(spi);
>> +}
>
> Can we change this to return SIZE_MAX instead (ie, the maximum value for
> a size_t)?  That way callers don't need to worry if there is a limit, if
> they want to handle it they can just unconditionally assume that a limit
> will be provided.

Yes, that sounds reasonable.

Thanks

Michal

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Martin Sperl]

> On 02.12.2015, at 00:12, Mark Brown <broonie@kernel.org> wrote:
> 
> On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
> 
>> +static inline size_t
>> +spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +	struct spi_master *master = spi->master;
>> +	if (!master->max_transfer_size)
>> +		return 0;
>> +	return master->max_transfer_size(spi);
>> +}
> 
> Can we change this to return SIZE_MAX instead (ie, the maximum value for
> a size_t)?  That way callers don't need to worry if there is a limit, if
> they want to handle it they can just unconditionally assume that a limit
> will be provided.

As I just came across: spi_master.max_dma_len, so I wonder how this 
value would differ from the proposed spi_master.max_transfer_size
on specific HW?

For all practical purposes I would assume both are identical.

Martin

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Michal Suchanek]

On 4 December 2015 at 15:30, Martin Sperl <martin@sperl.org> wrote:
>
>> On 02.12.2015, at 00:12, Mark Brown <broonie@kernel.org> wrote:
>>
>> On Tue, Dec 01, 2015 at 04:51:06PM -0000, Michal Suchanek wrote:
>>
>>> +static inline size_t
>>> +spi_max_transfer_size(struct spi_device *spi)
>>> +{
>>> +    struct spi_master *master = spi->master;
>>> +    if (!master->max_transfer_size)
>>> +            return 0;
>>> +    return master->max_transfer_size(spi);
>>> +}
>>
>> Can we change this to return SIZE_MAX instead (ie, the maximum value for
>> a size_t)?  That way callers don't need to worry if there is a limit, if
>> they want to handle it they can just unconditionally assume that a limit
>> will be provided.
>
> As I just came across: spi_master.max_dma_len, so I wonder how this
> value would differ from the proposed spi_master.max_transfer_size
> on specific HW?
>
> For all practical purposes I would assume both are identical.

They aren't.

Some SPI masters don't use DMA. Some SPI master drivers can drive CS
manually so they can glue multiple DMA (or whatever) transfers into
single logical SPI transfer transparently. And some cannot. Hence this
limit.

That said I don't know what is the purpose of spi_master.max_dma_len.
It is seldom used. It seems to be used as a hint for the DMA buffer
size in spi.c only.

It is set in 3 drivers.

Thanks

Michal

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Fri, Dec 04, 2015 at 03:30:50PM +0100, Martin Sperl wrote:
> > On 02.12.2015, at 00:12, Mark Brown <broonie@kernel.org> wrote:

> > Can we change this to return SIZE_MAX instead (ie, the maximum value for
> > a size_t)?  That way callers don't need to worry if there is a limit, if
> > they want to handle it they can just unconditionally assume that a limit
> > will be provided.

> As I just came across: spi_master.max_dma_len, so I wonder how this 
> value would differ from the proposed spi_master.max_transfer_size
> on specific HW?

> For all practical purposes I would assume both are identical.

Yeah, we should probably just remove the DMA specific one in time though
it may be useful for some devices with built in DMA engines if they're
oddly limited I'm not sure how practical their DMA would be if it
differed from the overall transfer length.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]