Page MenuHomeFreeBSD

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

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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)
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.

ray edited edge metadata.Dec 11 2016, 11:00 PM

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.

mizhka accepted this revision.Dec 12 2016, 6:48 AM
mizhka edited edge metadata.

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
loos added a comment.Dec 12 2016, 6:32 PM

@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

loos added a comment.Dec 13 2016, 3:06 AM

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).

loos added a comment.Dec 13 2016, 3:39 AM

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

thanks!

yamori813_yahoo.co.jp edited edge metadata.
  • back to original code at GPIOBUS_PIN_SETFLAG. Because of gpiobus.c fixed
ray added a comment.Dec 13 2016, 8:07 AM

Thanks a lot, guys!

ray accepted this revision.Dec 13 2016, 8:08 AM
ray edited edge metadata.
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.

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 ;).