Delete gpio flags because of not support device. This is same as gpioiic. Add spi clock low delay. Reformat message. ar5315_spi.c spibus minor number fix.
ClosedPublic
Actions

Authored by yamori813_yahoo.co.jp on Dec 11 2016, 8:33 AM.

Details

Reviewers

ray
mizhka
loos
adrian

Commits

rS331506: MFC r310017, r310229, r312289, r327260, r329539, r329544-r329546, r329620,
rS315329: MFC r310017-r310018
rS310018: [gpiospi] add clock delay to avoid smashing of bits
rS310017: [spi] reformat message and ar5315_spi minor fix

Summary

gpiospi modification

Test Plan

on ar531x soc

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

yamori813_yahoo.co.jp updated this revision to Diff 22810.Dec 11 2016, 8:33 AM

yamori813_yahoo.co.jp retitled this revision from to Delete gpio flags because of not support device. This is same as gpioiic. Add spi clock low delay. Reformat message. ar5315_spi.c spibus minor number fix..

yamori813_yahoo.co.jp updated this object.

yamori813_yahoo.co.jp edited the test plan for this revision. (Show Details)

yamori813_yahoo.co.jp added reviewers: mizhka, ray, adrian.

Herald added a subscriber: loos. · View Herald TranscriptDec 11 2016, 8:33 AM

ray added inline comments.Dec 11 2016, 8:32 PM

sys/dev/gpio/gpiospi.c
159 ↗	(On Diff #22810)	If driver do not support PullUp/PullDown, then why it makes problem?

gpiobus_pin_setflags() in gpibus.c have check gpio_check_flags(). This code check pin capabirity. If low driver not support GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN return EINVAL.

In D8749#181338, @yamori813_yahoo.co.jp wrote:

gpiobus_pin_setflags() in gpibus.c have check gpio_check_flags(). This code check pin capabirity. If low driver not support GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN return EINVAL.

Then better to set it as supported by driver, and ignore them in driver. Otherwise other controllers who support pullup/pulldown will have unpredictable results on that lines, because nobody said to pull it.

I seem GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN not set in any sys/mips/ gpio code. And gpioiic.c not use GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN that is same bitbang code . I think better not use in gpiospi.c rather than low driver ignore.

If ignore GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN in low driver then GPIO_PIN_PULLUP/GPIO_PIN_PULLDOWN flag have no meaning in a system.

Thanks for your contribution!

Generally speaking, it looks fine.
To satisfy both cases (with and wo pulling) it may be worth to make pulling configurable (and make default is off).

This revision is now accepted and ready to land.Dec 12 2016, 6:48 AM

@yamori813_yahoo.co.jp with i2c the bus hardware must have pullups (required by standard). This is why I haven't added it to gpioiic (but maybe I should...).

It is safe to ignore pullup/pulldown in gpio driver, they will have no meaning for that specific gpio controller, if you have a different controller they can still be used.

The check in gpio_check_flags() is wrong, I going to fix it. Thanks!

loos requested changes to this revision.Dec 12 2016, 6:44 PM

loos added a reviewer: loos.

loos added inline comments.

sys/dev/gpio/gpiospi.c
316 ↗	(On Diff #22810)	The delays still seem wrong to me, see: https://en.wikipedia.org/wiki/Serial_Peripheral_Interface_Bus We have to take the phase into account.
sys/dev/spibus/spibus.c
120–121 ↗	(On Diff #22810)	Correct.
sys/mips/atheros/ar531x/ar5315_spi.c
125 ↗	(On Diff #22810)	Correct, please commit.

This revision now requires changes to proceed.Dec 12 2016, 6:44 PM

I check this code on my develop SPI module code that is ksz8995ma ether switch chip and FON2100. This case delay is fixed problem and work. I check signal enclose picture. If no delay clock is not to zero.

This is my setting hints.

hint.gpiospi.0.at="gpiobus0"
hint.gpiospi.0.pins=0x9a
hint.gpiospi.0.cs0=0
hint.gpiospi.0.sclk=3
hint.gpiospi.0.mosi=1
hint.gpiospi.0.miso=2
hint.gpiospi.0.freq=100000000

yeah, this delay() fixes the clock output, but what I am saying is that data and clock should come out with a 90o phase difference. your patch doesn't not fix this but it is surely an improvement (you can commit this change too).

gpio_check_flags() should be fixed after r310000: https://svnweb.freebsd.org/changeset/base/310000

thanks!

back to original code at GPIOBUS_PIN_SETFLAG. Because of gpiobus.c fixed

Thanks a lot, guys!

ray accepted this revision.Dec 13 2016, 8:08 AM

ray edited edge metadata.

Closed by commit rS310017: [spi] reformat message and ar5315_spi minor fix (authored by mizhka). · Explain WhyDec 13 2016, 9:54 AM

This revision was automatically updated to reflect the committed changes.

Why delete my added gpio_delay() in gpiospi.c ? My KSZ8995MA code don't work no gpio_delay() on my target. I think current code clock is wrong.

In D8749#182206, @yamori813_yahoo.co.jp wrote:

Why delete my added gpio_delay() in gpiospi.c ? My KSZ8995MA code don't work no gpio_delay() on my target. I think current code clock is wrong.

Hi Hiroki-san,

gpio_delay has been committed by next commit in sequence: https://github.com/freebsd/freebsd/commit/1ea50c0b5014d04d2b8586d6efd258abe411d87f

I suppose Phabricator doesn't track commits for already closed reviews (smells like possible improvement ;).