drivers: regulator: convert to gpio_dt_spec
#37689
Conversation
| uint32_t startup_delay_us; | ||
| uint32_t off_on_delay_us; | ||
| gpio_pin_t gpio_pin; | ||
| gpio_dt_flags_t gpio_flags; | ||
| struct gpio_dt_spec enable; |
There was a problem hiding this comment.
Please rename to struct gpio_dt_spec gpio; or struct gpio_dt_spec reg_gpio;
There was a problem hiding this comment.
Why is that? The devicetree property is called enable-gpios
There was a problem hiding this comment.
Maybe enable_pin or enable_gpio.
There was a problem hiding this comment.
Looking through the existing usage of struct gpio_dt_spec, it looks like a pretty even split between including gpio in the name and not, with an edge towards not (influenced by my merged PR's that don't #37442, #37514)
My position is that these variables are only used in functions where it is exceedingly obvious that the object is a gpio (gpio_configure_dt(), etc), so the added value of tacking on _gpio isn't that high. Types aren't duplicated in the names of other fields, why is it needed here?
There was a problem hiding this comment.
Why is that? The devicetree property is called
enable-gpios
That should probably just be 'gpio'.
My position is that these variables are only used in functions where it is exceedingly obvious that the object is a gpio (
gpio_configure_dt(), etc), so the added value of tacking on_gpioisn't that high. Types aren't duplicated in the names of other fields, why is it needed here?
I am also fine with: onoff, offon, or disable.
There was a problem hiding this comment.
Absent other considerations, I feel that an argument could be made for
output_enablevsenable. Theoeabbreviation is IMO too abbreviated.
No, 'oe' is perfect, especially since it is part of struct driver_config. Alternative is onoff, which is also great since it is a fixed regulator. NACK for output_enable, enable.
There was a problem hiding this comment.
I think output_enable is better in this particular case. I agree that oe is simply too abbreviated.
There was a problem hiding this comment.
@jfischer-no will you compromise to output_enable ?
There was a problem hiding this comment.
Shorter names should be preferable, I would like to see oe or oe_pin.
There was a problem hiding this comment.
I also think that oe is too abbreviated. It would not be clear to me what gpio_pin_set_dt(&cfg->oe, false); really does, without looking at the logic/context. I am okay however with enable, output_enable, enable_pin or enable_gpio.
If that is the consensus of a larger group I am happy to update it. In the meantime I am leaving it as the name of the devicetree property, not a two letter abbreviation or a term against which true and false have no meaning. |
|
@stephanosio @jfischer-no I won't be making a 2am meeting to discuss a variable name. |
|
dev-review (sept 2, 2021): defer to next week when @jfischer-no is hopefully around to discuss. |
Convert regulator_fixed GPIO usage to utilize `struct gpio_dt_spec`. Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
JordanYates
force-pushed
the
210814_regulator_dt_spec
branch
from
67296fa
to
3a8f03b
Compare
gmarull
requested review from
carlescufi,
carlocaione,
cfriedt and
jfischer-no
|
Dropping dev-review label. Please add if you want to raise this again for discussion there. |
|
I don't feel like there has been any resolution in terms of what needs to be done. |
Convert regulator_fixed GPIO usage to utilize
struct gpio_dt_spec.Signed-off-by: Jordan Yates jordan.yates@data61.csiro.au