Add pinctrl_single driver
Needs ReviewPublic
Actions

Authored by oskar.holmlund_ohdata.se on Oct 8 2023, 7:50 AM.

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

oskar.holmlund_ohdata.se created this revision.Oct 8 2023, 7:50 AM

Herald added subscribers: loos, imp. · View Herald TranscriptOct 8 2023, 7:50 AM

oskar.holmlund_ohdata.se requested review of this revision.Oct 8 2023, 7:50 AM

oskar.holmlund_ohdata.se mentioned this in D42126: AM335x: update ti_gpio & am335x_gpio to use pinctrl-single driver.Oct 8 2023, 10:12 AM

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

oskar.holmlund_ohdata.se mentioned this in D41896: AM335x: ti_pinmux.c add pinconf-single to set of compatible strings.Oct 9 2023, 6:31 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!

oskar.holmlund_ohdata.se added inline comments.Oct 18 2023, 8:35 PM

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?

oskar.holmlund_ohdata.se mentioned this in D27889: TI SoC rework clk support.Nov 30 2023, 10:21 PM

manu added a subscriber: kevans.Dec 25 2023, 4:58 PM

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 ?

kevans added inline comments.Dec 25 2023, 8:36 PM

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