[PATCH 0/8] spi: Add file i/o to spidev_test

[PATCH 0/8] spi: Add file i/o to spidev_test

[Joshua Clayton]

On Tuesday, November 17, 2015 03:37:46 PM Mark Brown wrote:
> On Tue, Nov 17, 2015 at 07:24:20AM -0800, Joshua Clayton wrote:
> 
> >   Documentation/spi/spidev_test.c: use one rx buffer
> >   Documentation/spi/spidev_test.c: clean up input_tx
> >   Documentation/spi/spidev_test.c: accept input from a file
> >   Documentation/spi/spidev_test.c: output to a file
> >   Documentation/spi/spidev_test.c: check error
> >   Documentation/spi/spidev_test.c: fix whitespace
> 
> Please use subjct lines reflecting the style for the subsystem.
OK. Will do.
Assuming "spi: spidev_test: yadda..." 
It was a bit unclear due to the location of the file.

-- 
~Joshua Clayton

[PATCH 0/8] spi: Add file i/o to spidev_test

[Mark Brown]

On Tue, Nov 17, 2015 at 08:15:45AM -0800, Joshua Clayton wrote:
> On Tuesday, November 17, 2015 03:37:46 PM Mark Brown wrote:
> > On Tue, Nov 17, 2015 at 07:24:20AM -0800, Joshua Clayton wrote:

> > >   Documentation/spi/spidev_test.c: use one rx buffer
> > >   Documentation/spi/spidev_test.c: clean up input_tx
> > >   Documentation/spi/spidev_test.c: accept input from a file
> > >   Documentation/spi/spidev_test.c: output to a file
> > >   Documentation/spi/spidev_test.c: check error
> > >   Documentation/spi/spidev_test.c: fix whitespace

> > Please use subjct lines reflecting the style for the subsystem.

> OK. Will do.
> Assuming "spi: spidev_test: yadda..." 

Something like that.

> It was a bit unclear due to the location of the file.

It's not really the location that you should be focusing on, it's other
commits in the file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151117/a48258ac/attachment.sig>

[PATCH 1/8] Documentation/spi/spidev_test.c: use one rx buffer

[Joshua Clayton]

On Tuesday, November 17, 2015 05:41:56 PM Mark Brown wrote:
> On Tue, Nov 17, 2015 at 07:24:21AM -0800, Joshua Clayton wrote:
> 
> > default_rx and rx are needlessly different.
> > Use one buffer, local to transmit()
> 
> Why?  This isn't what I'd expect from black boxing the API, from a
> userspace point of view the transfer is atomic and in an ideal world
> we'd be able to do direct to/from memory transfers rather than memcpy()
> into kernel space which means that userspace should assume the transfers
> are going on simultaneously even if they don't currently.

The important thing here was to get rid of the default_rx buffer.
I just noticed that the output can be set up completely within the scope
of the transmit function, since the operands are global. But I would be
just as happy to set it up at the top level. I'll change this in V2

--
~Joshua Clayton

[PATCH 2/8] Documentation/spi/spidev_test.c: clean up input_tx

[Joshua Clayton]

On Tuesday, November 17, 2015 05:43:33 PM Mark Brown wrote:
> On Tue, Nov 17, 2015 at 07:24:22AM -0800, Joshua Clayton wrote:
> > Put input from string into its own function.
> 
> Again, why are we doing this?  I'm having a hard time seeing what the
> gain is, the amount of code being moved is tiny.

It takes some clutter out of main() whose scope is limited to that little block of code, 
and because in the next patch we add another (larger)function to the if/else block.

I don't know if it is valid to say "look at the next commit" for justification, but
That is the reason.

-- 
~Joshua Clayton

[PATCH 2/8] Documentation/spi/spidev_test.c: clean up input_tx

[Mark Brown]

On Tue, Nov 17, 2015 at 11:21:12AM -0800, Joshua Clayton wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> It takes some clutter out of main() whose scope is limited to that little block of code, 
> and because in the next patch we add another (larger)function to the if/else block.

> I don't know if it is valid to say "look at the next commit" for justification, but
> That is the reason.

That's totally fine - just say that it's to support future changes in
this area.  It's good to split out mechanical changes like this from the
more complex changes, you just need to say why they're happening.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151117/0a2f7f03/attachment.sig>

[PATCH 3/8] Documentation/spi/spidev_test.c: accept input from a file

[Joshua Clayton]

On Tuesday, November 17, 2015 07:26:54 PM Anton Bondarenko wrote:
> On 17.11.2015 16:24, Joshua Clayton wrote:
> > +	if (sb.st_size > 4096)
> > +		pabort("input file exceeds spidev's 4k limit");
> This is not a true. IIRC PAGE_SIZE is the default buffer size for 
> spidev, but can be changed using bufsiz module parameter.
> Just 'insmod spidev bufsiz=X', where X is number of bytes.

You are right. I will drop this. As Mark suggests.
The ioctl gives a nice error code if the buffer is too big.

...
> > +	tx = malloc(sb.st_size);
> It would be good to check new allocations for fail.

I will add a check for this. 

-- 
~Joshua Clayton

[PATCH 7/8] tools/Makefile: minor whitespace cleanup

[Joshua Clayton]

On Tuesday, November 17, 2015 06:09:24 PM Mark Brown wrote:
> On Tue, Nov 17, 2015 at 07:24:27AM -0800, Joshua Clayton wrote:
> > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > ---
> >  tools/Makefile | 37 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 30 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 9a617ad..428fb4d 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -80,10 +80,21 @@ turbostat_install x86_energy_perf_policy_install:
> >  tmon_install:
> >  	$(call descend,thermal/$(@:_install=),install)
> >  
> > -install: acpi_install cgroup_install cpupower_install hv_install firewire_install lguest_install \
> > -		perf_install selftests_install turbostat_install usb_install \
> > -		virtio_install vm_install net_install x86_energy_perf_policy_install \
> > -	tmon
> > +install: acpi_install \
> > +		cgroup_install \
> > +		cpupower_install \
> > +		hv_install \
> > +		firewire_install \
> > +		lguest_install \
> > +		perf_install \
> > +		selftests_install \
> > +		tmon \
> > +		turbostat_install \
> > +		usb_install \
> > +		virtio_install \
> > +		vm_install \
> > +		net_install \
> > +		x86_energy_perf_policy_install
> 
> This isn't a whitespace cleanup, this is a substantial reindentation of
> the file :(  Please ensure your changelogs are accurate and in general
> try to avoid mixing this sort of invasive stylistic change in with other
> patch serieses, it reduces the potential for conflicts.
Um.
Inability to resist Makefile cleanup is a weakness of mine. 
It is a tangent from the rest of the series so I'll drop it.

-- 
~Joshua Clayton