| Message ID | 20200110073513.19472-2-wolfgang.wallner@br-automation.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | gpio: intel_gpio: Fix Intel gpio driver | expand |
On Fri, 10 Jan 2020 at 00:35, Wolfgang Wallner <wolfgang.wallner@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. > > Pass the 'pinctrl' device instead of the 'dev' device to > pcr_clrsetbits32(). > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > --- > > drivers/gpio/intel_gpio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Wolfgang, On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner <wolfgang.wallner@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 > > Pass the 'pinctrl' device instead of the 'dev' device to > pcr_clrsetbits32(). > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@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@gmail.com> But I think we should also fix intel_gpio_set_value() where dev is passed instead of pinctrl. Regards, Bin
Hi Bin, -----"Bin Meng" <bmeng.cn@gmail.com> schrieb: ----- > Hi Wolfgang, > > On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner > <wolfgang.wallner@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@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@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
Hi Wolfgang, On Mon, Feb 3, 2020 at 5:44 PM Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: > > Hi Bin, > > -----"Bin Meng" <bmeng.cn@gmail.com> schrieb: ----- > > Hi Wolfgang, > > > > On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner > > <wolfgang.wallner@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@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@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 That sounds good to me. Thanks! > this is the function that relies on the uclass of the parent device. Regards, Bin
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 |
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. Pass the 'pinctrl' device instead of the 'dev' device to pcr_clrsetbits32(). Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> --- drivers/gpio/intel_gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)