Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 02/06/2013 07:20 PM, Jonathan Cameron wrote: > On 02/05/2013 02:07 PM, Grant Likely wrote: >> On Sun, 27 Jan 2013 03:33:59 +, Mark Brown >> wrote: >>> On Wed, Jan 09, 2013 at 06:31:09PM +0100, Lars-Peter Clausen wrote: >>> The second function spi_sync_transfer() takes a SPI device and an array of spi_transfers. It will allocate a new spi_message (on the stack) and add all transfers in the array to the message. Finally it will call spi_sync() on the message. >>> >>> Reviewed-by: Mark Brown >> >> Looks good to me also. Go ahead and merge this series through the iio >> tree since it is the first user. > > Lars, when you are back on your feet, could you respin this series against > the latest iio tree please. 2 drivers have moved which messes up patches > 2 and 3. Now I could fix it up, but it's your patch series and I'm lazy ;) I had a few spare minutes so I've respun the series with the driver moves. I have also, dropped the coccinelle script from the first patch. I was unclear on whether the questions about that had been resolved. Anyhow, I've temporarily pushed out to togreg branch of iio.git. If Grant wants to add an ack, I'll pop that in before sending upstream if not I'll just put a note in the tag message (or if he is too slow for my arbitrary definition fo too slow ;) Jonathan > > >> >> g. >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Free Next-Gen Firewall Hardware Offer Buy your Sophos next-gen firewall before the end March 2013 and get the hardware for free! Learn more. http://p.sf.net/sfu/sophos-d2d-feb ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 02/05/2013 02:07 PM, Grant Likely wrote: > On Sun, 27 Jan 2013 03:33:59 +, Mark Brown > wrote: >> On Wed, Jan 09, 2013 at 06:31:09PM +0100, Lars-Peter Clausen wrote: >> >>> The second function spi_sync_transfer() takes a SPI device and an array of >>> spi_transfers. It will allocate a new spi_message (on the stack) and add all >>> transfers in the array to the message. Finally it will call spi_sync() on >>> the >>> message. >> >> Reviewed-by: Mark Brown > > Looks good to me also. Go ahead and merge this series through the iio > tree since it is the first user. Grant, can I take that as an Acked-by? Its going to touch your tree so that would probably reduce questions as this goes up stream. > > g. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Free Next-Gen Firewall Hardware Offer Buy your Sophos next-gen firewall before the end March 2013 and get the hardware for free! Learn more. http://p.sf.net/sfu/sophos-d2d-feb ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 02/05/2013 02:07 PM, Grant Likely wrote: > On Sun, 27 Jan 2013 03:33:59 +, Mark Brown > wrote: >> On Wed, Jan 09, 2013 at 06:31:09PM +0100, Lars-Peter Clausen wrote: >> >>> The second function spi_sync_transfer() takes a SPI device and an array of >>> spi_transfers. It will allocate a new spi_message (on the stack) and add all >>> transfers in the array to the message. Finally it will call spi_sync() on >>> the >>> message. >> >> Reviewed-by: Mark Brown > > Looks good to me also. Go ahead and merge this series through the iio > tree since it is the first user. Lars, when you are back on your feet, could you respin this series against the latest iio tree please. 2 drivers have moved which messes up patches 2 and 3. Now I could fix it up, but it's your patch series and I'm lazy ;) > > g. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Free Next-Gen Firewall Hardware Offer Buy your Sophos next-gen firewall before the end March 2013 and get the hardware for free! Learn more. http://p.sf.net/sfu/sophos-d2d-feb ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On Sun, 27 Jan 2013 03:33:59 +, Mark Brown wrote: > On Wed, Jan 09, 2013 at 06:31:09PM +0100, Lars-Peter Clausen wrote: > > > The second function spi_sync_transfer() takes a SPI device and an array of > > spi_transfers. It will allocate a new spi_message (on the stack) and add all > > transfers in the array to the message. Finally it will call spi_sync() on > > the > > message. > > Reviewed-by: Mark Brown Looks good to me also. Go ahead and merge this series through the iio tree since it is the first user. g. -- Free Next-Gen Firewall Hardware Offer Buy your Sophos next-gen firewall before the end March 2013 and get the hardware for free! Learn more. http://p.sf.net/sfu/sophos-d2d-feb ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On Wed, Jan 09, 2013 at 06:31:09PM +0100, Lars-Peter Clausen wrote: > The second function spi_sync_transfer() takes a SPI device and an array of > spi_transfers. It will allocate a new spi_message (on the stack) and add all > transfers in the array to the message. Finally it will call spi_sync() on the > message. Reviewed-by: Mark Brown for the helpers, we should try to get them merged separately to the coccinelle rules if there's issue with those. -- Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. ON SALE this month only -- learn more at: http://p.sf.net/sfu/learnnow-d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On Thu, 10 Jan 2013, Lars-Peter Clausen wrote:
> On 01/10/2013 09:53 AM, Julia Lawall wrote:
> >> +@r1@
> >> +identifier fn;
> >> +identifier xfers;
> >> +@@
> >> +fn(...)
> >> +{
> >> + ...
> >> +(
> >> + struct spi_transfer xfers[...];
> >> +|
> >> + struct spi_transfer xfers[];
> >> +)
> >> + ...
> >> +}
> >
> > Can it happen that there would be more than one spi_transfer or spi_message
> > variable per function? This semantic patch will only treat the case where
> > there is only one, because the ... before an after the variable declaration
> > won't match another declaration of the same form.
> >
> > julia
>
> I guess it could happen, but I would consider it to be very rare. There are
> a few examples of multiple transfers in the kernel. But most of them look like
>
> struct spi_message msg;
> struct spi_transfer xfer_foo;
> struct spi_transfer xfer_bar;
>
> ...
> spi_message_add_tail(&xfer_foo, &msg);
> spi_message_add_tail(&xfer_bar, &msg);
>
> So the transformation can't be applied here anyway.
>
> Do you have an idea how to change the rule to work with multiple
> transfers/messages per function? If it would make the cocci file more
> complex I wouldn't bother to take care of it, since it basically has no
> practical use.
Probably the simplest thing is to put when any on all of the ...s
It might get slower, though.
Alternatively you could have a rule at the end that prints a warning for
any cases that are not transformed.
julia
--
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122712
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 01/10/2013 09:53 AM, Julia Lawall wrote:
>> +@r1@
>> +identifier fn;
>> +identifier xfers;
>> +@@
>> +fn(...)
>> +{
>> +...
>> +(
>> +struct spi_transfer xfers[...];
>> +|
>> +struct spi_transfer xfers[];
>> +)
>> +...
>> +}
>
> Can it happen that there would be more than one spi_transfer or spi_message
> variable per function? This semantic patch will only treat the case where
> there is only one, because the ... before an after the variable declaration
> won't match another declaration of the same form.
>
> julia
I guess it could happen, but I would consider it to be very rare. There are
a few examples of multiple transfers in the kernel. But most of them look like
struct spi_message msg;
struct spi_transfer xfer_foo;
struct spi_transfer xfer_bar;
...
spi_message_add_tail(&xfer_foo, &msg);
spi_message_add_tail(&xfer_bar, &msg);
So the transformation can't be applied here anyway.
Do you have an idea how to change the rule to work with multiple
transfers/messages per function? If it would make the cocci file more
complex I wouldn't bother to take care of it, since it basically has no
practical use.
- Lars
--
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122712
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
> +@r1@
> +identifier fn;
> +identifier xfers;
> +@@
> +fn(...)
> +{
> + ...
> +(
> + struct spi_transfer xfers[...];
> +|
> + struct spi_transfer xfers[];
> +)
> + ...
> +}
Can it happen that there would be more than one spi_transfer or spi_message
variable per function? This semantic patch will only treat the case where
there is only one, because the ... before an after the variable declaration
won't match another declaration of the same form.
julia
--
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122712
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 01/09/2013 08:56 PM, Lars-Peter Clausen wrote:
> On 01/09/2013 08:20 PM, Jonathan Cameron wrote:
>> On 01/09/2013 05:31 PM, Lars-Peter Clausen wrote:
>>> Quite often the pattern used for setting up and transferring a synchronous
>>> SPI
>>> transaction looks very much like the following:
>>>
>>> struct spi_message msg;
>>> struct spi_transfer xfers[] = {
>>> ...
>>> };
>>>
>>> spi_message_init(&msg);
>>> spi_message_add_tail(&xfers[0], &msg);
>>> ...
>>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>>
>>> ret = spi_sync(&msg);
>>>
>>> This patch adds two new helper functions for handling this case. The first
>>> helper function spi_message_init_with_transfers() takes a spi_message and an
>>> array of spi_transfers. It will initialize the message and then call
>>> spi_message_add_tail() for each transfer in the array. E.g. the following
>>>
>>> spi_message_init(&msg);
>>> spi_message_add_tail(&xfers[0], &msg);
>>> ...
>>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>>
>>> can be rewritten as
>>>
>>> spi_message_init_with_transfers(&msg, xfers, ARRAY_SIZE(xfers));
>>>
>>> The second function spi_sync_transfer() takes a SPI device and an array of
>>> spi_transfers. It will allocate a new spi_message (on the stack) and add all
>>> transfers in the array to the message. Finally it will call spi_sync() on
>>> the
>>> message.
>>>
>>> E.g. the follwing
>>>
>>> struct spi_message msg;
>>> struct spi_transfer xfers[] = {
>>> ...
>>> };
>>>
>>> spi_message_init(&msg);
>>> spi_message_add_tail(&xfers[0], &msg);
>>> ...
>>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>>
>>> ret = spi_sync(spi, &msg);
>>>
>>> can be rewritten as
>>>
>>> struct spi_transfer xfers[] = {
>>> ...
>>> };
>>>
>>> ret = spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
>>>
>>> The patch also adds a new cocci script which can detect such sequences as
>>> described above and transform them automatically to use the new helper
>>> functions.
>>>
>>> Signed-off-by: Lars-Peter Clausen
>>>
>> Principle looks good to me and some nice little duplication removal
>> savings.
>>
>> My coccinelle isn't really up to checking that, but for the functions
>> Acked-by: Jonathan Cameron
>>
>> When all comments are in on the code we'll have to think about how to
>> merge this. If you have much else planned that will hit those iio drivers
>> then things will get uggly unless it goes through that tree.
>>
>> Guess it all depends on whether others like the patch though ;)
>
> The IIO patches can easily wait another release until the spi has made it's
> way
> up into mainline. I just didn't want to send out the helper functions without
> any realworld examples on how they can be used.
>
Good point, though obviously send them again after this patch has merged
given the fine nature of my memory ;)
--
Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
and much more. Keep your Java skills current with LearnJavaNow -
200+ hours of step-by-step video tutorials by Java experts.
SALE $49.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122612
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 01/09/2013 08:20 PM, Jonathan Cameron wrote:
> On 01/09/2013 05:31 PM, Lars-Peter Clausen wrote:
>> Quite often the pattern used for setting up and transferring a synchronous
>> SPI
>> transaction looks very much like the following:
>>
>> struct spi_message msg;
>> struct spi_transfer xfers[] = {
>> ...
>> };
>>
>> spi_message_init(&msg);
>> spi_message_add_tail(&xfers[0], &msg);
>> ...
>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>
>> ret = spi_sync(&msg);
>>
>> This patch adds two new helper functions for handling this case. The first
>> helper function spi_message_init_with_transfers() takes a spi_message and an
>> array of spi_transfers. It will initialize the message and then call
>> spi_message_add_tail() for each transfer in the array. E.g. the following
>>
>> spi_message_init(&msg);
>> spi_message_add_tail(&xfers[0], &msg);
>> ...
>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>
>> can be rewritten as
>>
>> spi_message_init_with_transfers(&msg, xfers, ARRAY_SIZE(xfers));
>>
>> The second function spi_sync_transfer() takes a SPI device and an array of
>> spi_transfers. It will allocate a new spi_message (on the stack) and add all
>> transfers in the array to the message. Finally it will call spi_sync() on the
>> message.
>>
>> E.g. the follwing
>>
>> struct spi_message msg;
>> struct spi_transfer xfers[] = {
>> ...
>> };
>>
>> spi_message_init(&msg);
>> spi_message_add_tail(&xfers[0], &msg);
>> ...
>> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>>
>> ret = spi_sync(spi, &msg);
>>
>> can be rewritten as
>>
>> struct spi_transfer xfers[] = {
>> ...
>> };
>>
>> ret = spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
>>
>> The patch also adds a new cocci script which can detect such sequences as
>> described above and transform them automatically to use the new helper
>> functions.
>>
>> Signed-off-by: Lars-Peter Clausen
>>
> Principle looks good to me and some nice little duplication removal
> savings.
>
> My coccinelle isn't really up to checking that, but for the functions
> Acked-by: Jonathan Cameron
>
> When all comments are in on the code we'll have to think about how to
> merge this. If you have much else planned that will hit those iio drivers
> then things will get uggly unless it goes through that tree.
>
> Guess it all depends on whether others like the patch though ;)
The IIO patches can easily wait another release until the spi has made it's way
up into mainline. I just didn't want to send out the helper functions without
any realworld examples on how they can be used.
- Lars
--
Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
and much more. Keep your Java skills current with LearnJavaNow -
200+ hours of step-by-step video tutorials by Java experts.
SALE $49.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122612
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3] spi: Add helper functions for setting up transfers
On 01/09/2013 05:31 PM, Lars-Peter Clausen wrote:
> Quite often the pattern used for setting up and transferring a synchronous SPI
> transaction looks very much like the following:
>
> struct spi_message msg;
> struct spi_transfer xfers[] = {
> ...
> };
>
> spi_message_init(&msg);
> spi_message_add_tail(&xfers[0], &msg);
> ...
> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>
> ret = spi_sync(&msg);
>
> This patch adds two new helper functions for handling this case. The first
> helper function spi_message_init_with_transfers() takes a spi_message and an
> array of spi_transfers. It will initialize the message and then call
> spi_message_add_tail() for each transfer in the array. E.g. the following
>
> spi_message_init(&msg);
> spi_message_add_tail(&xfers[0], &msg);
> ...
> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>
> can be rewritten as
>
> spi_message_init_with_transfers(&msg, xfers, ARRAY_SIZE(xfers));
>
> The second function spi_sync_transfer() takes a SPI device and an array of
> spi_transfers. It will allocate a new spi_message (on the stack) and add all
> transfers in the array to the message. Finally it will call spi_sync() on the
> message.
>
> E.g. the follwing
>
> struct spi_message msg;
> struct spi_transfer xfers[] = {
> ...
> };
>
> spi_message_init(&msg);
> spi_message_add_tail(&xfers[0], &msg);
> ...
> spi_message_add_tail(&xfers[ARRAY_SIZE(xfers) - 1], &msg);
>
> ret = spi_sync(spi, &msg);
>
> can be rewritten as
>
> struct spi_transfer xfers[] = {
> ...
> };
>
> ret = spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
>
> The patch also adds a new cocci script which can detect such sequences as
> described above and transform them automatically to use the new helper
> functions.
>
> Signed-off-by: Lars-Peter Clausen
>
Principle looks good to me and some nice little duplication removal
savings.
My coccinelle isn't really up to checking that, but for the functions
Acked-by: Jonathan Cameron
When all comments are in on the code we'll have to think about how to
merge this. If you have much else planned that will hit those iio drivers
then things will get uggly unless it goes through that tree.
Guess it all depends on whether others like the patch though ;)
> ---
> I'm not entirely happy with names of the two new functions and I'm open for
> better suggestions.
> ---
> include/linux/spi/spi.h| 44
> scripts/coccinelle/api/spi_sync_transfer.cocci | 141
> +
> 2 files changed, 185 insertions(+)
> create mode 100644 scripts/coccinelle/api/spi_sync_transfer.cocci
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index f629189..7dbe586 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -591,6 +591,26 @@ spi_transfer_del(struct spi_transfer *t)
> list_del(&t->transfer_list);
> }
>
> +/**
> + * spi_message_init_with_transfers - Initialize spi_message and append
> transfers
> + * @m: spi_message to be initialized
> + * @xfers: An array of spi transfers
> + * @num_xfers: Number of items in the xfer array
> + *
> + * This function initializes the given spi_message and adds each
> spi_transfer in
> + * the given array to the message.
> + */
> +static inline void
> +spi_message_init_with_transfers(struct spi_message *m,
> +struct spi_transfer *xfers, unsigned int num_xfers)
> +{
> + unsigned int i;
> +
> + spi_message_init(m);
> + for (i = 0; i < num_xfers; ++i)
> + spi_message_add_tail(&xfers[i], m);
> +}
> +
> /* It's fine to embed message and transaction structures in other data
> * structures so long as you don't free them while they're in use.
> */
> @@ -683,6 +703,30 @@ spi_read(struct spi_device *spi, void *buf, size_t len)
> return spi_sync(spi, &m);
> }
>
> +/**
> + * spi_sync_transfer - synchronous SPI data transfer
> + * @spi: device with which data will be exchanged
> + * @xfers: An array of spi_transfers
> + * @num_xfers: Number of items in the xfer array
> + * Context: can sleep
> + *
> + * Does a synchronous SPI data transfer of the given spi_transfer array.
> + *
> + * For more specific semantics see spi_sync().
> + *
> + * It returns zero on success, else a negative error code.
> + */
> +static inline int
> +spi_sync_transfer(struct spi_device *spi, struct spi_transfer *xfers,
> + unsigned int num_xfers)
> +{
> + struct spi_message msg;
> +
> + spi_message_init_with_transfers(&msg, xfers, num_xfers);
> +
> + return spi_sync(spi, &msg);
> +}
> +
> /* this copies txbuf and rxbuf data; for small transfers only! */
> extern int spi_write_then_read(struct spi_device *spi,
> const void *txbuf, unsigned n_tx,
> diff --git a/scripts/coccinelle/api/spi_sync_transfer.cocci
> b/s
