Re: [PATCH 20/61] input: keyboard: simplify getting .drvdata

2018-04-27 Thread Wolfram Sang 
Hi Dmitry,

> > Isn't it actually the other way around? platform_get_drvdata() is a
> > convenience function to access driver_data which is embedded in struct
> > device?
> 
> I guess it depends on how you read it. I always considered it separate
> because none (?) of the bus implementation assert this in comments to
> XXX_get_drvdata().

Well, even in the case somebody will implement a custom driver_data for
platform_devices, this person will need to convert all current users to
'dev_get_drvdata(&pdev->dev);' first in order to avoid regressions, I'd
think. This is what my patch does right now (but merely for overhead
reasons). Or?

> > > in the future, so I'd prefer keep using the proper accessors for the
> > > objects we are dealing with.
> > 
> > Exactly. I'd just argue, the object we are dealing with, declared in the
> > PM functions, is a struct device.
> 
> No, the driver does not create a generic device, it actually creates a
> platform device, or i2c client, or spi, or something else. The fact that

True.

> suspend and resume routines have generic device as their argument has
> more to do with the language limitation rather than reflection of true
> type of the objects we are dealing with.

Ok, can be argued. I'd personally still go for the gain, but I won't
push harder than this mail.

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 20/61] input: keyboard: simplify getting .drvdata

2018-04-26 Thread Dmitry Torokhov 
On Thu, Apr 26, 2018 at 10:04:01PM +0200, Wolfram Sang wrote:
> Hi Dmitry,
> 
> > > We should get drvdata from struct device directly. Going via
> > > platform_device is an unneeded step back and forth.
> > 
> > I consider the fact that platform device's driver data is accessible via
> > device driver data being implementation detail that may or may not change
> 
> Isn't it actually the other way around? platform_get_drvdata() is a
> convenience function to access driver_data which is embedded in struct
> device?

I guess it depends on how you read it. I always considered it separate
because none (?) of the bus implementation assert this in comments to
XXX_get_drvdata().

> 
> > in the future, so I'd prefer keep using the proper accessors for the
> > objects we are dealing with.
> 
> Exactly. I'd just argue, the object we are dealing with, declared in the
> PM functions, is a struct device.

No, the driver does not create a generic device, it actually creates a
platform device, or i2c client, or spi, or something else. The fact that
suspend and resume routines have generic device as their argument has
more to do with the language limitation rather than reflection of true
type of the objects we are dealing with.

Thanks.

-- 
Dmitry

Re: [PATCH 20/61] input: keyboard: simplify getting .drvdata

2018-04-26 Thread Wolfram Sang 
Hi Dmitry,

> > We should get drvdata from struct device directly. Going via
> > platform_device is an unneeded step back and forth.
> 
> I consider the fact that platform device's driver data is accessible via
> device driver data being implementation detail that may or may not change

Isn't it actually the other way around? platform_get_drvdata() is a
convenience function to access driver_data which is embedded in struct
device?

> in the future, so I'd prefer keep using the proper accessors for the
> objects we are dealing with.

Exactly. I'd just argue, the object we are dealing with, declared in the
PM functions, is a struct device.

Thanks for the comment,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 20/61] input: keyboard: simplify getting .drvdata

2018-04-26 Thread Dmitry Torokhov 
Hi Wolfram,

On Thu, Apr 19, 2018 at 04:05:50PM +0200, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.

I consider the fact that platform device's driver data is accessible via
device driver data being implementation detail that may or may not change
in the future, so I'd prefer keep using the proper accessors for the
objects we are dealing with.

Thanks.

> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> Build tested only. buildbot is happy. Please apply individually.
> 
>  drivers/input/keyboard/ep93xx_keypad.c   | 10 --
>  drivers/input/keyboard/imx_keypad.c  | 10 --
>  drivers/input/keyboard/lpc32xx-keys.c|  6 ++
>  drivers/input/keyboard/matrix_keypad.c   | 10 --
>  drivers/input/keyboard/omap4-keypad.c| 10 --
>  drivers/input/keyboard/pmic8xxx-keypad.c |  6 ++
>  drivers/input/keyboard/pxa27x_keypad.c   | 10 --
>  drivers/input/keyboard/samsung-keypad.c  | 12 
>  drivers/input/keyboard/snvs_pwrkey.c | 10 --
>  drivers/input/keyboard/spear-keyboard.c  | 10 --
>  drivers/input/keyboard/st-keyscan.c  |  6 ++
>  drivers/input/keyboard/tegra-kbc.c   | 10 --
>  12 files changed, 42 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/input/keyboard/ep93xx_keypad.c 
> b/drivers/input/keyboard/ep93xx_keypad.c
> index f77b295e0123..7584a03db4b3 100644
> --- a/drivers/input/keyboard/ep93xx_keypad.c
> +++ b/drivers/input/keyboard/ep93xx_keypad.c
> @@ -185,8 +185,7 @@ static void ep93xx_keypad_close(struct input_dev *pdev)
>  #ifdef CONFIG_PM_SLEEP
>  static int ep93xx_keypad_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct ep93xx_keypad *keypad = platform_get_drvdata(pdev);
> + struct ep93xx_keypad *keypad = dev_get_drvdata(dev);
>   struct input_dev *input_dev = keypad->input_dev;
>  
>   mutex_lock(&input_dev->mutex);
> @@ -198,7 +197,7 @@ static int ep93xx_keypad_suspend(struct device *dev)
>  
>   mutex_unlock(&input_dev->mutex);
>  
> - if (device_may_wakeup(&pdev->dev))
> + if (device_may_wakeup(dev))
>   enable_irq_wake(keypad->irq);
>  
>   return 0;
> @@ -206,11 +205,10 @@ static int ep93xx_keypad_suspend(struct device *dev)
>  
>  static int ep93xx_keypad_resume(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct ep93xx_keypad *keypad = platform_get_drvdata(pdev);
> + struct ep93xx_keypad *keypad = dev_get_drvdata(dev);
>   struct input_dev *input_dev = keypad->input_dev;
>  
> - if (device_may_wakeup(&pdev->dev))
> + if (device_may_wakeup(dev))
>   disable_irq_wake(keypad->irq);
>  
>   mutex_lock(&input_dev->mutex);
> diff --git a/drivers/input/keyboard/imx_keypad.c 
> b/drivers/input/keyboard/imx_keypad.c
> index 25d61d8d4fc4..56328ced81e2 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -532,8 +532,7 @@ static int imx_keypad_probe(struct platform_device *pdev)
>  
>  static int __maybe_unused imx_kbd_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct imx_keypad *kbd = platform_get_drvdata(pdev);
> + struct imx_keypad *kbd = dev_get_drvdata(dev);
>   struct input_dev *input_dev = kbd->input_dev;
>  
>   /* imx kbd can wake up system even clock is disabled */
> @@ -544,7 +543,7 @@ static int __maybe_unused imx_kbd_suspend(struct device 
> *dev)
>  
>   mutex_unlock(&input_dev->mutex);
>  
> - if (device_may_wakeup(&pdev->dev))
> + if (device_may_wakeup(dev))
>   enable_irq_wake(kbd->irq);
>  
>   return 0;
> @@ -552,12 +551,11 @@ static int __maybe_unused imx_kbd_suspend(struct device 
> *dev)
>  
>  static int __maybe_unused imx_kbd_resume(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct imx_keypad *kbd = platform_get_drvdata(pdev);
> + struct imx_keypad *kbd = dev_get_drvdata(dev);
>   struct input_dev *input_dev = kbd->input_dev;
>   int ret = 0;
>  
> - if (device_may_wakeup(&pdev->dev))
> + if (device_may_wakeup(dev))
>   disable_irq_wake(kbd->irq);
>  
>   mutex_lock(&input_dev->mutex);
> diff --git a/drivers/input/keyboard/lpc32xx-keys.c 
> b/drivers/input/keyboard/lpc32xx-keys.c
> index 1dd57ac0e7a2..0831a6f2a9d4 100644
> --- a/drivers/input/keyboard/lpc32xx-keys.c
> +++ b/drivers/input/keyboard/lpc32xx-keys.c
> @@ -279,8 +279,7 @@ static int lpc32xx_kscan_probe(struct platform_device 
> *pdev)
>  #ifdef CONFIG_PM_SLEEP
>  static int lpc32xx_kscan_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
> + struct lpc32xx_kscan_drv *kscandat = dev_get_drvdata