Antwort: Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Wolfgang Wallner]

Hi Bin,

-----"Bin Meng" <bmeng.cn at gmail.com> schrieb: -----
> Hi Wolfgang,
>
> On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
> <wolfgang.wallner at br-automation.com> wrote:
> >
> > The function pcr_clrsetbits32() expects a device with a P2SB parent
> > device.
> >
> > The currently passed device 'dev' is a gpio-controller with a device
> > 'pinctrl' as parent. This does not match the expectations of
> > pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.
>
> typo: the 'pinctrl' device

Thanks, I will fix that in V2.

> >
> > Pass the 'pinctrl' device instead of the 'dev' device to
> > pcr_clrsetbits32().
> >
> > Signed-off-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
> > ---
> >
> >  drivers/gpio/intel_gpio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > index 4bf1c9ddc4..db63115628 100644
> > --- a/drivers/gpio/intel_gpio.c
> > +++ b/drivers/gpio/intel_gpio.c
> > @@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
> >         struct udevice *pinctrl = dev_get_parent(dev);
> >         uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
> >
> > -       pcr_clrsetbits32(dev, config_offset,
> > +       pcr_clrsetbits32(pinctrl, config_offset,
> >                          PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
> >                                   PAD_CFG0_TX_DISABLE,
> >                          PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
>
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
>
> But I think we should also fix intel_gpio_set_value() where dev is
> passed instead of pinctrl.

Good catch, I will also include a fix for that in V2.

Those errors are easy to make, hard to find, and currently they are not caught,
except when something goes wrong during runtime. Should we add additional
assert()-lines or checks to catch such errors? If so, where?
For the P2SB driver I would say _pcr_reg_address() would be a good place, as
this is the function that relies on the uclass of the parent device.

regards, Wolfgang