Re: Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
Hi Wolfgang, On Mon, Feb 3, 2020 at 5:44 PM Wolfgang Wallner wrote: > > Hi Bin, > > -"Bin Meng" schrieb: - > > Hi Wolfgang, > > > > On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner > > 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 > > > --- > > > > > > 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 > > > > 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
Antwort: Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
Hi Bin, -"Bin Meng" schrieb: - > Hi Wolfgang, > > On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner > 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 > > --- > > > > 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 > > 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
Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
Hi Wolfgang, On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner 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 > --- > > 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 But I think we should also fix intel_gpio_set_value() where dev is passed instead of pinctrl. Regards, Bin
Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
On Fri, 10 Jan 2020 at 00:35, Wolfgang Wallner 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 > --- > > drivers/gpio/intel_gpio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass
[PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
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 --- 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 | -- 2.24.1
