linux-kernel - Re: [PATCH RESEND] spi: spi-fsl-spi: Make spi-fsl-spi usable in cpu mode outside of FSL SOC environments and add a grlib variant normally running on sparc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5112340B.7030505@gaisler.com>
Date:	Wed, 06 Feb 2013 11:44:27 +0100
From:	Andreas Larsson <andreas@...sler.com>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>,
	software@...sler.com, linux-kernel@...r.kernel.org,
	Peter Korsgaard <jacmet@...site.dk>,
	spi-devel-general@...ts.sourceforge.net,
	Mingkai Hu <Mingkai.hu@...escale.com>,
	Anton Vorontsov <avorontsov@...mvista.com>
Subject: Re: [PATCH RESEND] spi: spi-fsl-spi: Make spi-fsl-spi usable in cpu
 mode outside of FSL SOC environments and add a grlib variant normally running
 on sparc

On 2013-02-05 18:56, Grant Likely wrote:
> On Wed, 30 Jan 2013 13:15:24 +0100, Andreas Larsson <andreas@...sler.com> wrote:
>> This patch relies upon parts of the "of, of_gpio, of_spi: Fix and
>> improve of_parse_phandle_with_args, of_gpio_named_count and
>> of_spi_register_master" patchset - https://lkml.org/lkml/2012/12/27/54
>> (v2 at https://lkml.org/lkml/2013/1/29/308).

For TYPE_GRLIB, the gpio chipselect parts relies on this series. It 
would be great to get some feedback on that patch series as well.

>> Maybe the different out/in_be32 vs iowrite/read32be in spi-fsl-lib.h is
>> over the top, but I'm not sure if there might be subtle differences
>> between those on powerpc and I don't have any fsl hardware to try things
>> out on.
>
> Changing to ioread/write globally should be fine. I would change it and get
> someone with an fsl board to try it out. That will reduce the diffstat a
> bit.

Will do.

> As is, this is quite an invasive patch, so I'm not going to be
> comfortable merging it without at least one 3rd party tester. (Breaking
> things up into discrete patches will make me less nervous and possible
> to merge parts while still revising others.

I'll be happy to do that. Was trying to adhere to the guide line to not 
introduce something separately from its usage, but in this case it is 
probably better to split things up a bit.

>> +#ifdef CONFIG_FSL_SOC
>>   	else if (prop && !strcmp(prop, "qe"))
>>   		pdata->flags = SPI_CPM_MODE | SPI_QE;
>>   	else if (of_device_is_compatible(np, "fsl,cpm2-spi"))
>>   		pdata->flags = SPI_CPM_MODE | SPI_CPM2;
>>   	else if (of_device_is_compatible(np, "fsl,cpm1-spi"))
>>   		pdata->flags = SPI_CPM_MODE | SPI_CPM1;
>> +#endif
>
> The ifdefs are ugly and these lines won't affect sparc. Just leave them
> in.

Will do.

>> +/* TYPE_GRLIB SPI Controller capability register definitions */
>> +#define SPCAP_SSEN(x)           (((x) >> 16) & 0x1)
>> +#define SPCAP_SSSZ(x)           (((x) >> 24) & 0xff)
>> +#define SPCAP_MAXWLEN(x)	(((x) >> 20) & 0xf)
>
> Nit: inconsistent whitespace (tabs vs. spaces)

Will fix.

>> +	ret = of_property_read_u32(dev->of_node, "bus-number", &bus_num);
>> +	if (!ret)
>> +		master->bus_num = bus_num;
>
> Drop these lines. Never set the bus number explicitly when using DT. If
> you really want to assign a particular bus number, then use an alias.

Sure, I'll check that out.

> That's all the comments I've got for now, but there are an awful lot of
> #defines that are added to the driver which is kind of ugly. I'm not
> going to nack over that, but it would be good to clean it up and do some
> better abstraction.

I agree that it is ugly. If I move the cpm-related things from 
spi-fsl-spi.c to a separate file that only gets linked on FSL_SOC and 
have dummy functions in an h-file I think that it can be made much less 
ugly.

Thanks for the feedback!

Cheers,
Andreas

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/