| Message ID | 20200708202355.28507-4-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improvements for MAX77620 GPIO driver | expand |
08.07.2020 23:57, Andy Shevchenko пишет: > > > On Wednesday, July 8, 2020, Dmitry Osipenko <digetx@gmail.com > <mailto:digetx@gmail.com>> wrote: > > The gpiochip_add_data() takes care of setting the of_node to the > parent's > device of_node, hence there is no need to do it manually in the driver's > code. This patch corrects the parent's device pointer and removes the > unnecessary setting of the of_node. > > > I gave a second look and I think my suggestion is wrong. Here is an > interesting propagation of the parent device node to its grand son, > leaving son’s one untouched. Original code has intentions to do that way. The [1] says that gpio_chip.parent should point at the "device providing the GPIOs". That's the pdev->dev.parent in the case of this driver. MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO controller, PINCTRL and RTC. The MFD is the parent device that provides the GPIOs [2]. [1] https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276 [2] https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48 I think the old code was wrong and this patch is correct, please correct me if I'm missing something. > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com > <mailto:andy.shevchenko@gmail.com>> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com > <mailto:digetx@gmail.com>> > --- > drivers/gpio/gpio-max77620.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c > index 7f7e8d4bf0d3..39d431da2dbc 100644 > --- a/drivers/gpio/gpio-max77620.c > +++ b/drivers/gpio/gpio-max77620.c > @@ -279,7 +279,7 @@ static int max77620_gpio_probe(struct > platform_device *pdev) > mgpio->dev = &pdev->dev; > > mgpio->gpio_chip.label = pdev->name; > - mgpio->gpio_chip.parent = &pdev->dev; > + mgpio->gpio_chip.parent = pdev->dev.parent; > mgpio->gpio_chip.direction_input = max77620_gpio_dir_input; > mgpio->gpio_chip.get = max77620_gpio_get; > mgpio->gpio_chip.direction_output = max77620_gpio_dir_output; > @@ -288,9 +288,6 @@ static int max77620_gpio_probe(struct > platform_device *pdev) > mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR; > mgpio->gpio_chip.can_sleep = 1; > mgpio->gpio_chip.base = -1; > -#ifdef CONFIG_OF_GPIO > - mgpio->gpio_chip.of_node = pdev->dev.parent->of_node; > -#endif > > platform_set_drvdata(pdev, mgpio); > > -- > 2.26.0 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Jul 9, 2020 at 12:44 AM Dmitry Osipenko <digetx@gmail.com> wrote: > 08.07.2020 23:57, Andy Shevchenko пишет: > > On Wednesday, July 8, 2020, Dmitry Osipenko <digetx@gmail.com > > <mailto:digetx@gmail.com>> wrote: ... > > I gave a second look and I think my suggestion is wrong. Here is an > > interesting propagation of the parent device node to its grand son, > > leaving son’s one untouched. Original code has intentions to do that way. > > The [1] says that gpio_chip.parent should point at the "device providing > the GPIOs". Yes, physical device I believe. > That's the pdev->dev.parent in the case of this driver. > MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO > controller, PINCTRL and RTC. The MFD is the parent device that provides > the GPIOs [2]. > > [1] > https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276 > > [2] > https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48 > > I think the old code was wrong and this patch is correct, please correct > me if I'm missing something. Hmm... I have checked through GPIO drivers I have knowledge of / care about and PMIC ones do like you suggested in this patch, the rest (which are instantiated from MFD) take a virtual platform device. Looking at DT excerpt I think you're rather right than wrong, so I leave it to you and maintainers. Thanks!
09.07.2020 12:07, Andy Shevchenko пишет: > On Thu, Jul 9, 2020 at 12:44 AM Dmitry Osipenko <digetx@gmail.com> wrote: >> 08.07.2020 23:57, Andy Shevchenko пишет: >>> On Wednesday, July 8, 2020, Dmitry Osipenko <digetx@gmail.com >>> <mailto:digetx@gmail.com>> wrote: > > ... > >>> I gave a second look and I think my suggestion is wrong. Here is an >>> interesting propagation of the parent device node to its grand son, >>> leaving son’s one untouched. Original code has intentions to do that way. >> >> The [1] says that gpio_chip.parent should point at the "device providing >> the GPIOs". > > Yes, physical device I believe. > >> That's the pdev->dev.parent in the case of this driver. >> MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO >> controller, PINCTRL and RTC. The MFD is the parent device that provides >> the GPIOs [2]. >> >> [1] >> https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276 >> >> [2] >> https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48 >> >> I think the old code was wrong and this patch is correct, please correct >> me if I'm missing something. > > Hmm... I have checked through GPIO drivers I have knowledge of / care > about and PMIC ones do like you suggested in this patch, the rest > (which are instantiated from MFD) take a virtual platform device. > > Looking at DT excerpt I think you're rather right than wrong, so I > leave it to you and maintainers. > Thanks! Okay, waiting for the maintainers then :) Thank you very much for the review!
On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > The gpiochip_add_data() takes care of setting the of_node to the parent's > device of_node, hence there is no need to do it manually in the driver's > code. This patch corrects the parent's device pointer and removes the > unnecessary setting of the of_node. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Looks good to me. Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c index 7f7e8d4bf0d3..39d431da2dbc 100644 --- a/drivers/gpio/gpio-max77620.c +++ b/drivers/gpio/gpio-max77620.c @@ -279,7 +279,7 @@ static int max77620_gpio_probe(struct platform_device *pdev) mgpio->dev = &pdev->dev; mgpio->gpio_chip.label = pdev->name; - mgpio->gpio_chip.parent = &pdev->dev; + mgpio->gpio_chip.parent = pdev->dev.parent; mgpio->gpio_chip.direction_input = max77620_gpio_dir_input; mgpio->gpio_chip.get = max77620_gpio_get; mgpio->gpio_chip.direction_output = max77620_gpio_dir_output; @@ -288,9 +288,6 @@ static int max77620_gpio_probe(struct platform_device *pdev) mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR; mgpio->gpio_chip.can_sleep = 1; mgpio->gpio_chip.base = -1; -#ifdef CONFIG_OF_GPIO - mgpio->gpio_chip.of_node = pdev->dev.parent->of_node; -#endif platform_set_drvdata(pdev, mgpio);
The gpiochip_add_data() takes care of setting the of_node to the parent's device of_node, hence there is no need to do it manually in the driver's code. This patch corrects the parent's device pointer and removes the unnecessary setting of the of_node. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpio/gpio-max77620.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)