[1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
diff mbox series

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
Related show

Commit Message

Wolfgang Wallner Jan. 10, 2020, 7:35 a.m. UTC
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(-)

Comments

Simon Glass Jan. 30, 2020, 2:16 a.m. UTC | #1
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>
Bin Meng Feb. 3, 2020, 5:20 a.m. UTC | #2
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
Wolfgang Wallner Feb. 3, 2020, 9:44 a.m. UTC | #3
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
Bin Meng Feb. 3, 2020, 9:51 a.m. UTC | #4
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

Patch
diff mbox series

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 |