Page MenuHomeFreeBSD

Add pinctrl_single driver
Needs ReviewPublic

Authored by oskar.holmlund_ohdata.se on Oct 8 2023, 7:50 AM.
Tags
Referenced Files
Unknown Object (File)
Jan 24 2024, 7:08 AM
Unknown Object (File)
Dec 30 2023, 9:51 AM
Unknown Object (File)
Dec 29 2023, 11:27 PM
Unknown Object (File)
Dec 10 2023, 7:20 AM
Unknown Object (File)
Nov 30 2023, 4:42 PM
Unknown Object (File)
Nov 30 2023, 7:10 AM
Unknown Object (File)
Nov 6 2023, 7:23 PM
Unknown Object (File)
Oct 13 2023, 5:48 PM
Subscribers

Details

Reviewers
manu
karels
to.karlzon_gmail.com
mmel
Group Reviewers
ARM
Summary

Driver according to sys/contrib/device-tree/Bindings/pinctrl/pinctrl-single.txt

There are still optional properties that is not implemented due to there is not present in any device-tree at all, and some are present but these SoCs (hisilicon, pxa etc) are not supported by the system.

pinctrl_single_pins_write/read handles the different register sizes according to pinctrl-single,register-width DT property. The syscon interface (sys/dev/extres/syscon/syscon_if.m) define only _4 version, should I internally handle the writes according to register-width or should i extend the syscon to _1 _2 versions or a general without specify the size?

Removal of sys/arm/ti/ti_pinmux.c will be handled by the review regarding implementing the device specific gpio driver for the Ti SoC

Test Plan

Its tested on TI AM3358.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

manu requested changes to this revision.Oct 8 2023, 2:14 PM
manu added inline comments.
sys/dev/pinctrl/pinctrl_single.c
185

Resources aren't free-ed in the error paths below, better just move this after reading the DT properties

197

syscon_unregister(dev); is missing. Also freeing sys->syscon is needed in the error paths below.

This revision now requires changes to proceed.Oct 8 2023, 2:14 PM

Thank you Manu. Followed your advice to move the bus_alloc_resource/syscon interface to the end of the attach function to avoid complicated error paths.

Looks mostly good to me.

sys/dev/pinctrl/pinctrl_single.c
237

Isn't the syscon needed for configuring pin ?
Should fdt_pinctrl_configure_tree be called after then ?

402

Is there plan to subclass this driver ?
If not no need for DEFINE_CLASS_0 then.

  • Replaced the DEFINE_CLASS_0 -> struct driver_t
  • removed pinctrl_single_pins_write / read
  • update pinctrl_single_syscon_unlocked_write_4/read_4 to honor pinctrl-single,register-width. (even if the functionnames will be misleading and give the impression its an 4byte version of read/write)
  • removed pinctrl_single_syscon_unlocked_modify_4
  • update error handling after fdt_pinctrl_register() call to release syscon in case of failure.

Once again, Thank you Emmanuel for the review!

sys/dev/pinctrl/pinctrl_single.c
385

It might be a little bit to early in BUS_PASS_BUS, maybe BUS_PASS_INTERRUPT or perhaps BUS_PASS_SUPPORTDEV?

manu added inline comments.
sys/dev/pinctrl/pinctrl_single.c
241

I think that syscon interface needs either a syscon_free or syscon_unregister needs to free the resource.
I find it rather ugly that we need to MALLOC_DECLARE and free the resource in a driver.

@kevans @mmel what do you think ?

sys/dev/pinctrl/pinctrl_single.c
241

IMO not syscon_unregister, but a new either syscon_free (or perhaps syscon_destroy for more symmetry with the name create / create_ofw_node)

sys/dev/pinctrl/pinctrl_single.c
241

I'm good with syscon_destroy, can you take care of that Oskar ?