On 00:40 Sun 02 Aug , Eric Chung wrote: > Fix drive strength configuration by: > - Using FIELD_PREP() to properly set the PAD_DRIVE mask bits > - Not overriding io_type with IO_TYPE_EXTERNAL for external pins > - Returning the pin number instead of 0 from pinmux_property_set > - Removing redundant zero-initialization of the mask variable > > Signed-off-by: Eric Chung <[email protected]> > Checked this patch, the logic looks ok to me (only few minor comments), And you can still add my R-B, thanks
Reviewed-by: Yixun Lan <[email protected]> > --- > v6: > - Fix drive strength configuration. > --- > drivers/pinctrl/spacemit/pinctrl-k1.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c > b/drivers/pinctrl/spacemit/pinctrl-k1.c > index a6a22eacac7b..6ed511879daf 100644 > --- a/drivers/pinctrl/spacemit/pinctrl-k1.c > +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c > @@ -9,6 +9,7 @@ > #include <dm/device_compat.h> > #include <dm/pinctrl.h> > #include <dm/read.h> > +#include <linux/bitfield.h> > #include <linux/bitops.h> > #include <linux/errno.h> > #include <linux/io.h> > @@ -386,10 +387,12 @@ static int spacemit_pinmux_set(struct udevice *dev, > unsigned int pin, > static int spacemit_pinmux_property_set(struct udevice *dev, u32 > pinmux_group) > { > u32 pin, mux; > + int ret; > > pin = spacemit_dt_get_pin(pinmux_group); > mux = spacemit_dt_get_pin_mux(pinmux_group); > - return spacemit_pinmux_set(dev, pin, mux); > + ret = spacemit_pinmux_set(dev, pin, mux); .. > + return ret ? ret : pin; I'd prefer doing as below, which would more explicitly tell that's an error handler if (ret < 0) return ret; return pin; > } > > static const struct pinconf_param spacemit_pinconf_params[] = { > @@ -406,7 +409,7 @@ static int spacemit_pinconf_set(struct udevice *dev, > unsigned int pin_selector, > struct spacemit_pinctrl_data *data; > struct spacemit_pinctrl_priv *priv = dev_get_priv(dev); > void __iomem *addr; > - u32 mask = 0; > + u32 mask; This isn't a necessary change, which doesn't alter any logic or fix anything, and more worse, the follow-up patch drop line of this variable I mean bringing in unrelated change only increase the review burden.. > unsigned int io_type; > u8 ds; > bool found; > @@ -433,7 +436,8 @@ static int spacemit_pinconf_set(struct udevice *dev, > unsigned int pin_selector, > for (i = 0; i < priv->nr_io_pins; i++) { > if (priv->io_pins[i].pin != pin_selector) > continue; > - io_type = priv->io_pins[i].io_type; > + if (priv->io_pins[i].io_type != IO_TYPE_EXTERNAL) > + io_type = priv->io_pins[i].io_type; > break; > } > if (io_type != IO_TYPE_3V3 && io_type != IO_TYPE_1V8) { > @@ -441,7 +445,7 @@ static int spacemit_pinconf_set(struct udevice *dev, > unsigned int pin_selector, > return -EINVAL; > } > ds = spacemit_get_drive_strength(io_type, argument); > - clrsetbits_le32(addr, PAD_DRIVE, ds); > + clrsetbits_le32(addr, PAD_DRIVE, FIELD_PREP(PAD_DRIVE, ds)); > break; > case PIN_CONFIG_POWER_SOURCE: > for (i = 0, found = false; i < priv->nr_io_pins; i++) { > > -- > 2.51.0 > -- Yixun Lan (dlan)
