linux-kernel - Re: [PATCH v1] spi: master driver to enable RTC on ICPDAS LP-8841

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 22 Feb 2016 12:26:14 +0300
From:	Sergei Ianovich <ynvich@...il.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@...r.kernel.org>,
	"open list:SPI SUBSYSTEM" <linux-spi@...r.kernel.org>
Subject: Re: [PATCH v1] spi: master driver to enable RTC on ICPDAS LP-8841

On Mon, 2016-02-22 at 12:10 +0900, Mark Brown wrote:
> On Mon, Feb 22, 2016 at 04:47:06AM +0300, Sergei Ianovich wrote:
> > ICP DAS LP-8841 contains a DS-1302 RTC. This driver provides an SPI
> > master which makes the RTC usable. The driver is not supposed to
> > work
> > with anything else.
> 
> So this is something that is internal to a single chip or something?
> I'm slightly unclear from the above if this is or is not a SPI
> controller.  If it isn't generic the DT should probably just describe
> the IP and let the driver create the SPI controller and device if
> that
> is an expedient way of doing things.
> 
> > +spi0@...c {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	compatible = "icpdas,spi-lp8841-rtc";
> > +	reg = <0x901c 0x1>;
> > +
> > +	rtc@0 {
> > +		compatible = "maxim,rtc-ds1302";
> > +		reg = <0>;
> > +		spi-max-frequency = <500000>;
> > +		spi-3wire;
> > +		spi-lsb-first;
> > +		spi-cs-high;
> > +	};
> > +};
> 
> This example makes it seem like an actual SPI controller but just one
> that's broken/limited?

Yes. This is an actual SPI controller with minimal required
functionality. I didn't implement anything I don't need.

> > +config SPI_LP8841_RTC
> > +	tristate "ICP DAS LP-8841 SPI Controller for RTC"
> > +	depends on OF && (MACH_PXA27X_DT || COMPILE_TEST)
> 
> Does this need a strict DT dependency or can it build without DT?

The driver can only be useful on a single industrial PC. That PC
requires DT, so I made this a strict dependency. The dependency can be
removed by using ifdefs, but I see no point why.

> > +/*
> > + * REVISIT If there is support for SPI_3WIRE and SPI_LSB_FIRST in
> > SPI
> > + * GPIO driver, this SPI driver can be replaced by a simple GPIO
> > driver
> > + * providing 3 GPIO pins.
> > + */
> 
> What's the advantage of not doing that?  Overall the driver looks
> fairly
> good but it does seem to just implement a straight bitbanging driver
> with less flexibility.

MicroWire (SPI_3WIRE) mode is slightly different from the modes
implemented in spi-bitbang-txrx.h. I was getting junk from device in
both Mode0 and Mode2 until I changed the implementation. The change is
documented in the patch.

There will also need to be changes in bitbang.c. SPI_LSB_FIRST will
require a new flasg in txrx_word(). To keep overhead low will require
to grow txrx_word[] from 4 to 16 or even 32. CPOL, CPHA, LSB_FIRST,
3_WIRE each requires an additional power of 2.

While this change could be benefitial to both spi-gpio and spi-bitbang, 
it is very big.

> > +#ifndef DRIVER_NAME
> > +#define DRIVER_NAME	"spi_lp8841_rtc"
> > +#endif
> 
> If you want to use a define for this just use a define for it, don't
> do
> this ifdef stuff.

Yes, I'll fix this. This has been blindly copied from spi-gpio.c.

> > +static inline void
> > +spidelay(unsigned usec)
> > +{
> > +	usleep_range(usec, usec + 1);
> > +}
> 
> Just use usleep_range() directly.

Yes, I will.

> > +static void
> > +spi_lp8841_rtc_cleanup(struct spi_device *spi)
> > +{
> > +}
> 
> Remove empty functions, if they can be empty the core will support
> ignoring them if they are missing.

Yes, I will.

Thanks for a lightning fast review.
> >