Re: linux-next: build failure after merge of the driver-core tree

[Uwe Kleine-König]

Hello Stephen,

On Fri, Jul 23, 2021 at 03:09:44PM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the driver-core tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
> 
> drivers/gpu/drm/drm_dp_aux_bus.c:106:13: error: initialization of 'void 
> (*)(struct device *)' from incompatible pointer type 'int (*)(struct device 
> *)' [-Werror=incompatible-pointer-types]
>   106 |  .remove  = dp_aux_ep_remove,
>   | ^~~~
> drivers/gpu/drm/drm_dp_aux_bus.c:106:13: note: (near initialization for 
> 'dp_aux_bus_type.remove')
> 
> Caused by commit
> 
>   aeb33699fc2c ("drm: Introduce the DP AUX bus")
> 
> from the drm tree interacting with commit
> 
>   fc7a6209d571 ("bus: Make remove callback return void")
> 
> from the driver-core tree.
> 
> I applied the following merge fix patch.
> 
> From: Stephen Rothwell 
> Date: Fri, 23 Jul 2021 14:58:25 +1000
> Subject: [PATCH] fix for "drm: Introduce the DP AUX bus"
> 
> interaction with "bus: Make remove callback return void"
> 
> Signed-off-by: Stephen Rothwell 
> ---
>  drivers/gpu/drm/drm_dp_aux_bus.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_bus.c 
> b/drivers/gpu/drm/drm_dp_aux_bus.c
> index e49a70f3691b..298ea7a49591 100644
> --- a/drivers/gpu/drm/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/drm_dp_aux_bus.c
> @@ -67,9 +67,8 @@ static int dp_aux_ep_probe(struct device *dev)
>   *
>   * Calls through to the endpoint driver remove.
>   *
> - * Return: 0 if no error or negative error code.
>   */
> -static int dp_aux_ep_remove(struct device *dev)
> +static void dp_aux_ep_remove(struct device *dev)
>  {
>   struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver);
>   struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
> @@ -77,8 +76,6 @@ static int dp_aux_ep_remove(struct device *dev)
>   if (aux_ep_drv->remove)
>   aux_ep_drv->remove(aux_ep);
>   dev_pm_domain_detach(dev, true);
> -
> - return 0;
>  }

This looks right.

Greg provided a tag containing fc7a6209d571 ("bus: Make remove callback
return void") at

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 
tags/bus_remove_return_void-5.15

(see https://lore.kernel.org/lkml/ypkwqwf0dukng...@kroah.com).

It would be great if this could be merged into the drm tree with the
above diff squashed into the merge commit.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH] pwm: Rename pwm_get_state() to better reflect its semantic

[Uwe Kleine-König]

Given that lowlevel drivers usually cannot implement exactly what a
consumer requests with pwm_apply_state() there is some rounding involved.

pwm_get_state() traditionally returned the setting that was requested most
recently by the consumer (opposed to what was actually implemented in
hardware in reply to the last request). To make this semantic obvious
rename the function.

Signed-off-by: Uwe Kleine-König 
---
 Documentation/driver-api/pwm.rst   |  6 +++-
 drivers/clk/clk-pwm.c  |  2 +-
 drivers/gpu/drm/i915/display/intel_panel.c |  4 +--
 drivers/input/misc/da7280.c|  2 +-
 drivers/input/misc/pwm-beeper.c|  2 +-
 drivers/input/misc/pwm-vibra.c |  4 +--
 drivers/pwm/core.c |  4 +--
 drivers/pwm/pwm-atmel-hlcdc.c  |  2 +-
 drivers/pwm/pwm-atmel.c|  2 +-
 drivers/pwm/pwm-imx27.c|  2 +-
 drivers/pwm/pwm-rockchip.c |  2 +-
 drivers/pwm/pwm-stm32-lp.c |  4 +--
 drivers/pwm/pwm-sun4i.c|  2 +-
 drivers/pwm/sysfs.c| 18 ++--
 drivers/regulator/pwm-regulator.c  |  4 +--
 drivers/video/backlight/pwm_bl.c   | 10 +++
 include/linux/pwm.h| 34 ++
 17 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index ab62f1bb0366..381f3c46cdac 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -55,7 +55,11 @@ several parameter at once. For example, if you see 
pwm_config() and
 pwm_{enable,disable}() calls in the same function, this probably means you
 should switch to pwm_apply_state().
 
-The PWM user API also allows one to query the PWM state with pwm_get_state().
+The PWM user API also allows one to query the last applied PWM state with
+pwm_get_last_applied_state(). Note this is different to what the driver has
+actually implemented if the request cannot be implemented exactly with the
+hardware in use. There is currently no way for consumers to get the actually
+implemented settings.
 
 In addition to the PWM state, the PWM API also exposes PWM arguments, which
 are the reference PWM config one should use on this PWM.
diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index da2c8eddfd9f..7cfaff32980b 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -49,7 +49,7 @@ static int clk_pwm_get_duty_cycle(struct clk_hw *hw, struct 
clk_duty *duty)
struct clk_pwm *clk_pwm = to_clk_pwm(hw);
struct pwm_state state;
 
-   pwm_get_state(clk_pwm->pwm, &state);
+   pwm_get_last_applied_state(clk_pwm->pwm, &state);
 
duty->num = state.duty_cycle;
duty->den = state.period;
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c 
b/drivers/gpu/drm/i915/display/intel_panel.c
index 5fdf52643150..3aebf9be6b6a 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -627,7 +627,7 @@ static u32 ext_pwm_get_backlight(struct intel_connector 
*connector, enum pipe un
struct intel_panel *panel = &connector->panel;
struct pwm_state state;
 
-   pwm_get_state(panel->backlight.pwm, &state);
+   pwm_get_last_applied_state(panel->backlight.pwm, &state);
return pwm_get_relative_duty_cycle(&state, 100);
 }
 
@@ -1915,7 +1915,7 @@ static int ext_pwm_setup_backlight(struct intel_connector 
*connector,
 
if (pwm_is_enabled(panel->backlight.pwm)) {
/* PWM is already enabled, use existing settings */
-   pwm_get_state(panel->backlight.pwm, 
&panel->backlight.pwm_state);
+   pwm_get_last_applied_state(panel->backlight.pwm, 
&panel->backlight.pwm_state);
 
level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
100);
diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index b08610d6e575..409670be1d2b 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -333,7 +333,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic 
*haptics, bool enabled)
return -EINVAL;
}
 
-   pwm_get_state(haptics->pwm_dev, &state);
+   pwm_get_last_applied_state(haptics->pwm_dev, &state);
state.enabled = enabled;
if (enabled) {
period_mag_multi = (u64)state.period * haptics->gain;
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index d6b12477748a..4c5f09201ecf 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -33,7 +33,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned 
long period)
struct pwm_state state;
int error;

Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic

[Uwe Kleine-König]

Hello Thierry,

On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote:
> > Given that lowlevel drivers usually cannot implement exactly what a
> > consumer requests with pwm_apply_state() there is some rounding involved.
> > 
> > pwm_get_state() traditionally returned the setting that was requested most
> > recently by the consumer (opposed to what was actually implemented in
> > hardware in reply to the last request). To make this semantic obvious
> > rename the function.
> > 
> > Signed-off-by: Uwe Kleine-König 
> > ---
> >  Documentation/driver-api/pwm.rst   |  6 +++-
> >  drivers/clk/clk-pwm.c  |  2 +-
> >  drivers/gpu/drm/i915/display/intel_panel.c |  4 +--
> >  drivers/input/misc/da7280.c|  2 +-
> >  drivers/input/misc/pwm-beeper.c|  2 +-
> >  drivers/input/misc/pwm-vibra.c |  4 +--
> >  drivers/pwm/core.c |  4 +--
> >  drivers/pwm/pwm-atmel-hlcdc.c  |  2 +-
> >  drivers/pwm/pwm-atmel.c|  2 +-
> >  drivers/pwm/pwm-imx27.c|  2 +-
> >  drivers/pwm/pwm-rockchip.c |  2 +-
> >  drivers/pwm/pwm-stm32-lp.c |  4 +--
> >  drivers/pwm/pwm-sun4i.c|  2 +-
> >  drivers/pwm/sysfs.c| 18 ++--
> >  drivers/regulator/pwm-regulator.c  |  4 +--
> >  drivers/video/backlight/pwm_bl.c   | 10 +++
> >  include/linux/pwm.h| 34 ++
> >  17 files changed, 59 insertions(+), 45 deletions(-)
> 
> Honestly, I don't think this is worth the churn. If you think people
> will easily get confused by this then a better solution might be to more
> explicitly document the pwm_get_state() function to say exactly what it
> returns.

I'm not so optimistic that people become aware of the semantic just
because there is documentation describing it and I strongly believe that
a good name for functions is more important than accurate documentation.

If you don't agree, what do you think about the updated wording in
Documentation/driver-api/pwm.rst?

> But there's no need to make life difficult for everyone by
> renaming this to something as cumbersome as this.

I don't expect any merge conflicts (and if still a problem occurs
resolving should be trivial enough). So I obviously don't agree to your
weighing.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 0/5] amba: minor fix and various cleanups

[Uwe Kleine-König]

From: Uwe Kleine-König https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/5] amba: Make the remove callback return void

[Uwe Kleine-König]

All amba drivers return 0 in their remove callback. Together with the
driver core ignoring the return value anyhow, it doesn't make sense to
return a value here.

Change the remove prototype to return void, which makes it explicit that
returning an error value doesn't work as expected. This simplifies changing
the core remove callback to return void, too.

Reviewed-by: Ulf Hansson 
Reviewed-by: Arnd Bergmann 
Acked-by: Alexandre Belloni 
Acked-by: Dmitry Torokhov 
Acked-by: Krzysztof Kozlowski  # for drivers/memory
Acked-by: Mark Brown 
Acked-by: Dmitry Torokhov 
Acked-by: Linus Walleij 
Signed-off-by: Uwe Kleine-König 
---
 drivers/amba/bus.c | 5 ++---
 drivers/char/hw_random/nomadik-rng.c   | 3 +--
 drivers/dma/pl330.c| 3 +--
 drivers/gpu/drm/pl111/pl111_drv.c  | 4 +---
 drivers/hwtracing/coresight/coresight-catu.c   | 3 +--
 drivers/hwtracing/coresight/coresight-cpu-debug.c  | 4 +---
 drivers/hwtracing/coresight/coresight-cti-core.c   | 4 +---
 drivers/hwtracing/coresight/coresight-etb10.c  | 4 +---
 drivers/hwtracing/coresight/coresight-etm3x-core.c | 4 +---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +---
 drivers/hwtracing/coresight/coresight-funnel.c | 4 ++--
 drivers/hwtracing/coresight/coresight-replicator.c | 4 ++--
 drivers/hwtracing/coresight/coresight-stm.c| 4 +---
 drivers/hwtracing/coresight/coresight-tmc-core.c   | 4 +---
 drivers/hwtracing/coresight/coresight-tpiu.c   | 4 +---
 drivers/i2c/busses/i2c-nomadik.c   | 4 +---
 drivers/input/serio/ambakmi.c  | 3 +--
 drivers/memory/pl172.c | 4 +---
 drivers/memory/pl353-smc.c | 4 +---
 drivers/mmc/host/mmci.c| 4 +---
 drivers/rtc/rtc-pl030.c| 4 +---
 drivers/rtc/rtc-pl031.c| 4 +---
 drivers/spi/spi-pl022.c| 5 ++---
 drivers/tty/serial/amba-pl010.c| 4 +---
 drivers/tty/serial/amba-pl011.c| 3 +--
 drivers/vfio/platform/vfio_amba.c  | 3 +--
 drivers/video/fbdev/amba-clcd.c| 4 +---
 drivers/watchdog/sp805_wdt.c   | 4 +---
 include/linux/amba/bus.h   | 2 +-
 sound/arm/aaci.c   | 4 +---
 30 files changed, 34 insertions(+), 80 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 8c4a42df47c6..48b5d4b4e889 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -300,11 +300,10 @@ static int amba_remove(struct device *dev)
 {
struct amba_device *pcdev = to_amba_device(dev);
struct amba_driver *drv = to_amba_driver(dev->driver);
-   int ret = 0;
 
pm_runtime_get_sync(dev);
if (drv->remove)
-   ret = drv->remove(pcdev);
+   drv->remove(pcdev);
pm_runtime_put_noidle(dev);
 
/* Undo the runtime PM settings in amba_probe() */
@@ -315,7 +314,7 @@ static int amba_remove(struct device *dev)
amba_put_disable_pclk(pcdev);
dev_pm_domain_detach(dev, true);
 
-   return ret;
+   return 0;
 }
 
 static void amba_shutdown(struct device *dev)
diff --git a/drivers/char/hw_random/nomadik-rng.c 
b/drivers/char/hw_random/nomadik-rng.c
index b0ded41eb865..67947a19aa22 100644
--- a/drivers/char/hw_random/nomadik-rng.c
+++ b/drivers/char/hw_random/nomadik-rng.c
@@ -69,11 +69,10 @@ static int nmk_rng_probe(struct amba_device *dev, const 
struct amba_id *id)
return ret;
 }
 
-static int nmk_rng_remove(struct amba_device *dev)
+static void nmk_rng_remove(struct amba_device *dev)
 {
amba_release_regions(dev);
clk_disable(rng_clk);
-   return 0;
 }
 
 static const struct amba_id nmk_rng_ids[] = {
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index bc0f66af0f11..fd8d2bc3be9f 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3195,7 +3195,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
return ret;
 }
 
-static int pl330_remove(struct amba_device *adev)
+static void pl330_remove(struct amba_device *adev)
 {
struct pl330_dmac *pl330 = amba_get_drvdata(adev);
struct dma_pl330_chan *pch, *_p;
@@ -3235,7 +3235,6 @@ static int pl330_remove(struct amba_device *adev)
 
if (pl330->rstc)
reset_control_assert(pl330->rstc);
-   return 0;
 }
 
 static const struct amba_id pl330_ids[] = {
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c 
b/drivers/gpu/drm/pl111/pl111_drv.c
index 40e6708fbbe2..1fb5eacefd2d 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -320,7 +320,7 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
return ret;
 }
 
-static int pl111_amba_remove(struct amba_device *amba_dev)
+s

Re: [PATCH v3 4/5] amba: Make the remove callback return void

[Uwe Kleine-König]

Hello,

On Tue, Jan 26, 2021 at 05:08:40PM +, Suzuki K Poulose wrote:
> On 1/26/21 4:58 PM, Uwe Kleine-König wrote:
> > All amba drivers return 0 in their remove callback. Together with the
> > driver core ignoring the return value anyhow, it doesn't make sense to
> > return a value here.
> > 
> > Change the remove prototype to return void, which makes it explicit that
> > returning an error value doesn't work as expected. This simplifies changing
> > the core remove callback to return void, too.
> > 
> > Reviewed-by: Ulf Hansson 
> > Reviewed-by: Arnd Bergmann 
> > Acked-by: Alexandre Belloni 
> > Acked-by: Dmitry Torokhov 
> > Acked-by: Krzysztof Kozlowski  # for drivers/memory
> > Acked-by: Mark Brown 
>  > Acked-by: Dmitry Torokhov 
> > Acked-by: Linus Walleij 
> > Signed-off-by: Uwe Kleine-König 
> 
> 
> >   drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +---
> 
> You are most likely to have a conflict for the above file, with what is
> in coresight/next. It should be easy to resolve.

I'm surprised to see that the remove callback introduced in 2952ecf5df33
("coresight: etm4x: Refactor probing routine") has an __exit annotation.

With .suppress_bind_attrs = true you don't need a remove callback at
all. (And without .suppress_bind_attrs = true the remove callback must
have no __exit annotation.)

This make me looking at commit 45fe7befe0db ("coresight: remove broken
__exit annotations") by Arnd. Unless I miss something the better change
would have been to remove the unused remove callbacks instead of dropping
their __exit annotation?!

Anyhow, my conflict resolution looks as follows:

diff --cc drivers/hwtracing/coresight/coresight-etm4x-core.c
index 82787cba537d,473ab7480a36..
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@@ -1703,6 -1903,28 +1903,27 @@@ static int __exit etm4_remove_dev(struc
cpus_read_unlock();
  
coresight_unregister(drvdata->csdev);
+ 
+   return 0;
+ }
+ 
 -static int __exit etm4_remove_amba(struct amba_device *adev)
++static void __exit etm4_remove_amba(struct amba_device *adev)
+ {
+   struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+ 
+   if (drvdata)
 -  return etm4_remove_dev(drvdata);
 -  return 0;
++  etm4_remove_dev(drvdata);
+ }
+ 
+ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
+ {
+   int ret = 0;
+   struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ 
+   if (drvdata)
+   ret = etm4_remove_dev(drvdata);
+   pm_runtime_disable(&pdev->dev);
+   return ret;
  }
  
  static const struct amba_id etm4_ids[] = {

If this series should make it in for 5.12 we probably need an immutable
branch between hwtracing and amba.

> Otherwise, the changes look good for the drivers/hwtracing/coresight/*
> 
> Acked-by: Suzuki K Poulose 

Thanks
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[GIT PULL] immutable branch for amba changes targeting v5.12-rc1

[Uwe Kleine-König]

Hello,

the following changes since commit 5c8fe583cce542aa0b84adc939ce85293de36e5e:

  Linux 5.11-rc1 (2020-12-27 15:30:22 -0800)

are available in the Git repository at:

  https://git.pengutronix.de/git/ukl/linux tags/amba-make-remove-return-void

for you to fetch changes up to f170b59fedd733b92f58c4d7c8357fbf7601d623:

  amba: Make use of bus_type functions (2021-02-02 14:26:02 +0100)

I expect this tag to be merged by Russell King as amba maintainer and by
Mathieu Poirier (or Greg Kroah-Hartman?) for coresight as there are some
pending conflicting changes. These are not hard to resolve but also
non-trivial. Tell me if you need assistance for resolving, also if it's only a
second pair of eyes to judge your resolution.

Best regards,
Uwe


Tag for adaptions to struct amba_driver::remove changing prototype

----
Uwe Kleine-König (5):
  amba: Fix resource leak for drivers without .remove
  amba: reorder functions
  vfio: platform: simplify device removal
  amba: Make the remove callback return void
  amba: Make use of bus_type functions

 drivers/amba/bus.c | 234 
+--
 drivers/char/hw_random/nomadik-rng.c   |   3 +-
 drivers/dma/pl330.c|   3 +-
 drivers/gpu/drm/pl111/pl111_drv.c  |   4 +-
 drivers/hwtracing/coresight/coresight-catu.c   |   3 +-
 drivers/hwtracing/coresight/coresight-cpu-debug.c  |   4 +-
 drivers/hwtracing/coresight/coresight-cti-core.c   |   4 +-
 drivers/hwtracing/coresight/coresight-etb10.c  |   4 +-
 drivers/hwtracing/coresight/coresight-etm3x-core.c |   4 +-
 drivers/hwtracing/coresight/coresight-etm4x-core.c |   4 +-
 drivers/hwtracing/coresight/coresight-funnel.c |   4 +-
 drivers/hwtracing/coresight/coresight-replicator.c |   4 +-
 drivers/hwtracing/coresight/coresight-stm.c|   4 +-
 drivers/hwtracing/coresight/coresight-tmc-core.c   |   4 +-
 drivers/hwtracing/coresight/coresight-tpiu.c   |   4 +-
 drivers/i2c/busses/i2c-nomadik.c   |   4 +-
 drivers/input/serio/ambakmi.c  |   3 +-
 drivers/memory/pl172.c |   4 +-
 drivers/memory/pl353-smc.c |   4 +-
 drivers/mmc/host/mmci.c|   4 +-
 drivers/rtc/rtc-pl030.c|   4 +-
 drivers/rtc/rtc-pl031.c|   4 +-
 drivers/spi/spi-pl022.c|   5 +-
 drivers/tty/serial/amba-pl010.c|   4 +-
 drivers/tty/serial/amba-pl011.c|   3 +-
 drivers/vfio/platform/vfio_amba.c  |  15 ++--
 drivers/video/fbdev/amba-clcd.c|   4 +-
 drivers/watchdog/sp805_wdt.c   |   4 +-
 include/linux/amba/bus.h   |   2 +-
 sound/arm/aaci.c   |   4 +-
 30 files changed, 157 insertions(+), 198 deletions(-)




signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] mailbox: arm_mhuv2: make remove callback return void

[Uwe Kleine-König]

My build tests failed to catch that amba driver that would have needed
adaption in commit 3fd269e74f2f ("amba: Make the remove callback return
void"). Change the remove function to make the driver build again.

Reported-by: kernel test robot 
Fixes: 3fd269e74f2f ("amba: Make the remove callback return void")
Signed-off-by: Uwe Kleine-König 
---
Hello,

I guess I missed that driver during rebase as it was only introduced in
the last merge window. Sorry for that.

I'm unsure what is the right thing to do now. Should I redo the pull
request (with this patch squashed into 3fd269e74f2f)? Or do we just
apply this patch on top?

FTR, the test robot report is at 
https://lore.kernel.org/r/202102030343.d9j1wukx-...@intel.com

Best regards
Uwe

 drivers/mailbox/arm_mhuv2.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mailbox/arm_mhuv2.c b/drivers/mailbox/arm_mhuv2.c
index 67fb10885bb4..6cf1991a5c9c 100644
--- a/drivers/mailbox/arm_mhuv2.c
+++ b/drivers/mailbox/arm_mhuv2.c
@@ -1095,14 +1095,12 @@ static int mhuv2_probe(struct amba_device *adev, const 
struct amba_id *id)
return ret;
 }
 
-static int mhuv2_remove(struct amba_device *adev)
+static void mhuv2_remove(struct amba_device *adev)
 {
struct mhuv2 *mhu = amba_get_drvdata(adev);
 
if (mhu->frame == SENDER_FRAME)
writel_relaxed(0x0, &mhu->send->access_request);
-
-   return 0;
 }
 
 static struct amba_id mhuv2_ids[] = {
-- 
2.29.2



signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1

[Uwe Kleine-König]

Hello,

On Tue, Feb 02, 2021 at 02:53:50PM +0100, Uwe Kleine-König wrote:
> the following changes since commit 5c8fe583cce542aa0b84adc939ce85293de36e5e:
> 
>   Linux 5.11-rc1 (2020-12-27 15:30:22 -0800)
> 
> are available in the Git repository at:
> 
>   https://git.pengutronix.de/git/ukl/linux tags/amba-make-remove-return-void
> 
> for you to fetch changes up to f170b59fedd733b92f58c4d7c8357fbf7601d623:
> 
>   amba: Make use of bus_type functions (2021-02-02 14:26:02 +0100)
> 
> I expect this tag to be merged by Russell King as amba maintainer and by
> Mathieu Poirier (or Greg Kroah-Hartman?) for coresight as there are some
> pending conflicting changes. These are not hard to resolve but also
> non-trivial. Tell me if you need assistance for resolving, also if it's only a
> second pair of eyes to judge your resolution.

Alternatively to my additional patch sent earlier today I prepared a v2
tag at

  https://git.pengutronix.de/git/ukl/linux tags/amba-make-remove-return-void-v2

with the build fix squashed in. Iff you prefer and both Russell and Greg
agree to pull this one instead of the (implicit) v1 we can maybe prevent
introducing this build regression in mainline. Please coordinate among
you two. And sorry again for breaking the build.

Best regards
Uwe

PS: The range-diff between the original
tags/amba-make-remove-return-void and
tags/amba-make-remove-return-void-v2 is:

1:  3fd269e74f2f ! 1:  481963c91284 amba: Make the remove callback return void
@@ Commit message
 Acked-by: Vladimir Zapolskiy  # for memory/pl172
 Acked-by: Greg Kroah-Hartman 
 Link: 
https://lore.kernel.org/r/20210126165835.687514-5-u.kleine-koe...@pengutronix.de
+[ukleinek: squashed in a build fix for drivers/mailbox/arm_mhuv2.c]
 Signed-off-by: Uwe Kleine-König 
 
  ## drivers/amba/bus.c ##
@@ drivers/input/serio/ambakmi.c: static int amba_kmi_remove(struct 
amba_device *de
  
  static int __maybe_unused amba_kmi_resume(struct device *dev)
 
+ ## drivers/mailbox/arm_mhuv2.c ##
+@@ drivers/mailbox/arm_mhuv2.c: static int mhuv2_probe(struct amba_device 
*adev, const struct amba_id *id)
+   return ret;
+ }
+ 
+-static int mhuv2_remove(struct amba_device *adev)
++static void mhuv2_remove(struct amba_device *adev)
+ {
+   struct mhuv2 *mhu = amba_get_drvdata(adev);
+ 
+   if (mhu->frame == SENDER_FRAME)
+   writel_relaxed(0x0, &mhu->send->access_request);
+-
+-  return 0;
+ }
+ 
+ static struct amba_id mhuv2_ids[] = {
+
  ## drivers/memory/pl172.c ##
 @@ drivers/memory/pl172.c: static int pl172_probe(struct amba_device 
*adev, const struct amba_id *id)
return ret;
2:  f170b59fedd7 = 2:  f30d22a7bfab amba: Make use of bus_type functions

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/5] amba: minor fix and various cleanups

[Uwe Kleine-König]

Hello,

we already talked about this via irc, but for the record and the benefit
of others:

On Tue, Feb 02, 2021 at 10:49:15AM +, Russell King - ARM Linux admin wrote:
> I think you need to have a 6th patch which moves the
> probe/remove/shutdown methods into the bus_type - if you're setting
> them for every struct device_driver, then there's no point doing that
> and they may as well be in the bus_type.

This is implemented in patch 5 already.

Best regards
Uwe

-- 
Pengutronix e.K.           | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1

[Uwe Kleine-König]

On Thu, Feb 04, 2021 at 04:59:51PM +, Russell King - ARM Linux admin wrote:
> On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 04, 2021 at 04:52:24PM +, Russell King - ARM Linux admin 
> > wrote:
> > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > other coresight changes flow through.  So if no one else objects, I will
> > > > do so...
> > > 
> > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > I haven't merged v2 yet as I don't know what you've done.
> > 
> > I thought you merged this?
> 
> I took v1, and put it in a branch I've promised in the past not to
> rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> on top.
> 
> The only reason to produce an "immutable" branch is if it's the basis
> for some dependent work and you need that branch merged into other
> people's trees... so the whole "lets produce a v2" is really odd
> workflow... I'm confused about what I should do, and who has to be
> informed which option I take.
> 
> I'm rather lost here too.

Sorry to have cause this confusion. After I saw that my initial tag
missed to adapt a driver I wanted to make it easy for you to fix the
situation.
So I created a patch to fix it and created a second tag with the patch
squashed in. Obviously only one of them have to be picked and I hoped
you (= Russell + Greg) would agree which option to pick.

My preference would be if you both pick up v2 of the tag to yield a
history that is bisectable without build problems, but if Russell (who
already picked up the broken tag) considers his tree immutable and so
isn't willing to rebase, then picking up the patch is the way to go.

I suggest that Russell descides which option he wants to pick and tells
Greg to do the same!?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1

[Uwe Kleine-König]

Hello Russell, hello Greg,

On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 04, 2021 at 04:59:51PM +, Russell King - ARM Linux admin 
> wrote:
> > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 04, 2021 at 04:52:24PM +, Russell King - ARM Linux admin 
> > > wrote:
> > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > > other coresight changes flow through.  So if no one else objects, I 
> > > > > will
> > > > > do so...
> > > > 
> > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > > I haven't merged v2 yet as I don't know what you've done.
> > > 
> > > I thought you merged this?
> > 
> > I took v1, and put it in a branch I've promised in the past not to
> > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > on top.
> > 
> > The only reason to produce an "immutable" branch is if it's the basis
> > for some dependent work and you need that branch merged into other
> > people's trees... so the whole "lets produce a v2" is really odd
> > workflow... I'm confused about what I should do, and who has to be
> > informed which option I take.
> > 
> > I'm rather lost here too.
> 
> Sorry to have cause this confusion. After I saw that my initial tag
> missed to adapt a driver I wanted to make it easy for you to fix the
> situation.
> So I created a patch to fix it and created a second tag with the patch
> squashed in. Obviously only one of them have to be picked and I hoped
> you (= Russell + Greg) would agree which option to pick.
> 
> My preference would be if you both pick up v2 of the tag to yield a
> history that is bisectable without build problems, but if Russell (who
> already picked up the broken tag) considers his tree immutable and so
> isn't willing to rebase, then picking up the patch is the way to go.

OK, the current state is that Russell applied the patch fixing
drivers/mailbox/arm_mhuv2.c on top of merging my first tag.

So the way forward now is that Greg pulls

git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable

which currently points to 

860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback 
return void")

, into his tree that contains the hwtracing changes that conflict with my
changes. @Greg: Is this good enough, or do you require a dedicated tag
to pull that?

I think these conflicting hwtracing changes are not yet in any of Greg's
trees (at least they are not in next).

When I pull

https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next

(currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
resolution looks as follows:

diff --cc drivers/hwtracing/coresight/coresight-etm4x-core.c
index 82787cba537d,5017d33ba4f5..
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@@ -1703,6 -1906,28 +1906,27 @@@ static int __exit etm4_remove_dev(struc
cpus_read_unlock();
  
coresight_unregister(drvdata->csdev);
+ 
+   return 0;
+ }
+ 
 -static int __exit etm4_remove_amba(struct amba_device *adev)
++static void __exit etm4_remove_amba(struct amba_device *adev)
+ {
+   struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+ 
+   if (drvdata)
 -  return etm4_remove_dev(drvdata);
 -  return 0;
++  etm4_remove_dev(drvdata);
+ }
+ 
+ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
+ {
+   int ret = 0;
+   struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ 
+   if (drvdata)
+   ret = etm4_remove_dev(drvdata);
+   pm_runtime_disable(&pdev->dev);
+   return ret;
  }
  
  static const struct amba_id etm4_ids[] = {

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1

[Uwe Kleine-König]

On Fri, Feb 05, 2021 at 11:18:17AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 05, 2021 at 10:37:44AM +0100, Uwe Kleine-König wrote:
> > Hello Russell, hello Greg,
> > 
> > On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 04, 2021 at 04:59:51PM +, Russell King - ARM Linux admin 
> > > wrote:
> > > > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Thu, Feb 04, 2021 at 04:52:24PM +, Russell King - ARM Linux 
> > > > > admin wrote:
> > > > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > > > I'm glad to take this through my char/misc tree, as that's where 
> > > > > > > the
> > > > > > > other coresight changes flow through.  So if no one else objects, 
> > > > > > > I will
> > > > > > > do so...
> > > > > > 
> > > > > > Greg, did you end up pulling this after all? If not, Uwe produced a 
> > > > > > v2.
> > > > > > I haven't merged v2 yet as I don't know what you've done.
> > > > > 
> > > > > I thought you merged this?
> > > > 
> > > > I took v1, and put it in a branch I've promised in the past not to
> > > > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > > > on top.
> > > > 
> > > > The only reason to produce an "immutable" branch is if it's the basis
> > > > for some dependent work and you need that branch merged into other
> > > > people's trees... so the whole "lets produce a v2" is really odd
> > > > workflow... I'm confused about what I should do, and who has to be
> > > > informed which option I take.
> > > > 
> > > > I'm rather lost here too.
> > > 
> > > Sorry to have cause this confusion. After I saw that my initial tag
> > > missed to adapt a driver I wanted to make it easy for you to fix the
> > > situation.
> > > So I created a patch to fix it and created a second tag with the patch
> > > squashed in. Obviously only one of them have to be picked and I hoped
> > > you (= Russell + Greg) would agree which option to pick.
> > > 
> > > My preference would be if you both pick up v2 of the tag to yield a
> > > history that is bisectable without build problems, but if Russell (who
> > > already picked up the broken tag) considers his tree immutable and so
> > > isn't willing to rebase, then picking up the patch is the way to go.
> > 
> > OK, the current state is that Russell applied the patch fixing
> > drivers/mailbox/arm_mhuv2.c on top of merging my first tag.
> > 
> > So the way forward now is that Greg pulls
> > 
> > git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable
> > 
> > which currently points to 
> > 
> > 860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback 
> > return void")
> > 
> > , into his tree that contains the hwtracing changes that conflict with my
> > changes. @Greg: Is this good enough, or do you require a dedicated tag
> > to pull that?
> > 
> > I think these conflicting hwtracing changes are not yet in any of Greg's
> > trees (at least they are not in next).
> > 
> > When I pull
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next
> > 
> > (currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
> > to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
> > drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
> > resolution looks as follows:
> 
> Ok, my resolution looked a bit different.
> 
> Can you pull my char-misc-testing branch and verify I got this all
> pulled in correctly?

minor side-note: mentioning the repo url would have simplified that test.

I looked at

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 
char-misc-testing

commit 0573d3fa48640f0fa6b105ff92dcb02b94d6c1ab now.

I didn't compile test, but I'm willing to bet your resolution is wrong.
You have no return statement in etm4_remove_dev() but its return type is
int and etm4_remove_amba() still returns int but should return void.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] coresight: etm4x: Fix merge resolution for amba rework

[Uwe Kleine-König]

This was non-trivial to get right because commits
c23bc382ef0e ("coresight: etm4x: Refactor probing routine") and
5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
changed the code flow considerably. With this change the driver can be
built again.

Fixes: 0573d3fa4864 ("Merge branch 'devel-stable' of 
git://git.armlinux.org.uk/~rmk/linux-arm into char-misc-next")
Signed-off-by: Uwe Kleine-König 
---
On Fri, Feb 05, 2021 at 12:07:09PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 05, 2021 at 11:56:15AM +0100, Uwe Kleine-König wrote:
> > I didn't compile test, but I'm willing to bet your resolution is wrong.
> > You have no return statement in etm4_remove_dev() but its return type is
> > int and etm4_remove_amba() still returns int but should return void.
> 
> Can you send a patch to fix this up?

Sure, here it comes. As I'm unsure if you want to squash it into the
merge or want to keep it separate I crafted a commit message. If you
prefer squashing feel free to do so.

This change corresponds to the merge resolution I suggested before.

Best regards
Uwe

 drivers/hwtracing/coresight/coresight-etm4x-core.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c 
b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index bc55b261af23..c8ecd91e289e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1906,15 +1906,16 @@ static int __exit etm4_remove_dev(struct etmv4_drvdata 
*drvdata)
cpus_read_unlock();
 
coresight_unregister(drvdata->csdev);
+
+   return 0;
 }
 
-static int __exit etm4_remove_amba(struct amba_device *adev)
+static void __exit etm4_remove_amba(struct amba_device *adev)
 {
struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
if (drvdata)
-   return etm4_remove_dev(drvdata);
-   return 0;
+   etm4_remove_dev(drvdata);
 }
 
 static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] video: fbdev: imxfb: Fix an error message

[Uwe Kleine-König]

Hello Christophe,

On Thu, May 06, 2021 at 08:57:05PM +0200, Christophe JAILLET wrote:
> 'ret' is known to be 0 here.
> No error code is available, so just remove it from the error message.
> 
> Fixes: 72330b0eeefc ("i.MX Framebuffer: Use readl/writel instead of direct 
> pointer deref")
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/video/fbdev/imxfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 7f8debd2da06..ad598257ab38 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -992,7 +992,7 @@ static int imxfb_probe(struct platform_device *pdev)
>   info->screen_buffer = dma_alloc_wc(&pdev->dev, fbi->map_size,
>  &fbi->map_dma, GFP_KERNEL);
>   if (!info->screen_buffer) {
> - dev_err(&pdev->dev, "Failed to allocate video RAM: %d\n", ret);
> + dev_err(&pdev->dev, "Failed to allocate video RAM\n");
>   ret = -ENOMEM;
>   goto failed_map;
>   }

Reviewed-by: Uwe Kleine-König 

Are you using this driver, or did you find that problem using some
static checker?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] video: fbdev: imxfb: Fix an error message

[Uwe Kleine-König]

Hello Christophe,

[dropping krzysztof...@poczta.fm from recipents, the address doesn't
seem to exist]

On Fri, May 07, 2021 at 09:11:19AM +0200, Christophe JAILLET wrote:
> Le 07/05/2021 à 07:05, Uwe Kleine-König a écrit :
> > On Thu, May 06, 2021 at 08:57:05PM +0200, Christophe JAILLET wrote:
> > > 'ret' is known to be 0 here.
> > > No error code is available, so just remove it from the error message.
> > > 
> > > Fixes: 72330b0eeefc ("i.MX Framebuffer: Use readl/writel instead of 
> > > direct pointer deref")
> > > Signed-off-by: Christophe JAILLET 
> > > ---
> > >   drivers/video/fbdev/imxfb.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> > > index 7f8debd2da06..ad598257ab38 100644
> > > --- a/drivers/video/fbdev/imxfb.c
> > > +++ b/drivers/video/fbdev/imxfb.c
> > > @@ -992,7 +992,7 @@ static int imxfb_probe(struct platform_device *pdev)
> > >   info->screen_buffer = dma_alloc_wc(&pdev->dev, fbi->map_size,
> > >  &fbi->map_dma, GFP_KERNEL);
> > >   if (!info->screen_buffer) {
> > > - dev_err(&pdev->dev, "Failed to allocate video RAM: %d\n", ret);
> > > + dev_err(&pdev->dev, "Failed to allocate video RAM\n");
> > >   ret = -ENOMEM;
> > >   goto failed_map;
> > >   }
> > 
> > Reviewed-by: Uwe Kleine-König 
> > 
> > Are you using this driver, or did you find that problem using some
> > static checker?
> 
> No, I'm not using this driver AFAIK.

OK, thanks. (We're working on a DRM driver for this hardware. So you
could have been an interested tester.)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH] drm: Only select I2C_ALGOBIT for drivers that actually need it

[Uwe Kleine-König]

While working on a drm driver that doesn't need the i2c algobit stuff I
noticed that DRM selects this code even tough only 8 drivers actually use
it. While also only some drivers use i2c, keep the select for I2C for the
next cleanup patch. Still prepare this already by also selecting I2C for
the individual drivers.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/Kconfig | 5 -
 drivers/gpu/drm/ast/Kconfig | 2 ++
 drivers/gpu/drm/gma500/Kconfig  | 2 ++
 drivers/gpu/drm/hisilicon/hibmc/Kconfig | 2 ++
 drivers/gpu/drm/i915/Kconfig| 2 ++
 drivers/gpu/drm/mgag200/Kconfig | 2 ++
 drivers/gpu/drm/nouveau/Kconfig | 2 ++
 7 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 3c16bd1afd87..351ea617c498 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,7 +12,6 @@ menuconfig DRM
select HDMI
select FB_CMDLINE
select I2C
-   select I2C_ALGOBIT
select DMA_SHARED_BUFFER
select SYNC_FILE
 # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
@@ -233,6 +232,8 @@ config DRM_RADEON
 select DRM_KMS_HELPER
 select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
select POWER_SUPPLY
select HWMON
select BACKLIGHT_CLASS_DEVICE
@@ -254,6 +255,8 @@ config DRM_AMDGPU
select DRM_SCHED
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
select POWER_SUPPLY
select HWMON
select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
index fbcf2f45cef5..bcc25decd485 100644
--- a/drivers/gpu/drm/ast/Kconfig
+++ b/drivers/gpu/drm/ast/Kconfig
@@ -6,6 +6,8 @@ config DRM_AST
select DRM_VRAM_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
 Say yes for experimental AST GPU driver. Do not enable
 this driver without having a working -modesetting,
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index 0cff20265f97..e26c3a24955d 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -3,6 +3,8 @@ config DRM_GMA500
tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
depends on DRM && PCI && X86 && MMU
select DRM_KMS_HELPER
+   select I2C
+   select I2C_ALGOBIT
# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
select ACPI_VIDEO if ACPI
select BACKLIGHT_CLASS_DEVICE if ACPI
diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig 
b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
index 43943e980203..ac8c42dc79f6 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
+++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
@@ -6,6 +6,8 @@ config DRM_HISI_HIBMC
select DRM_VRAM_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
  Choose this option if you have a Hisilicon Hibmc soc chipset.
  If M is selected the module will be called hibmc-drm.
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 69f57ca9c68d..b3bb6f7cfbbc 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -13,6 +13,8 @@ config DRM_I915
select DRM_PANEL
select DRM_MIPI_DSI
select RELAY
+   select I2C
+   select I2C_ALGOBIT
select IRQ_WORK
# i915 depends on ACPI_VIDEO when ACPI is enabled
# but for select to work, need to select ACPI_VIDEO's dependencies, ick
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index eec59658a938..b28c5e4828f4 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -4,6 +4,8 @@ config DRM_MGAG200
depends on DRM && PCI && MMU
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
 This is a KMS driver for Matrox G200 chips. It supports the original
 MGA G200 desktop chips and the server variants. It requires 0.3.0
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 9436310d0854..8823f0b24c73 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -7,6 +7,8 @@ config DRM_NOUVEAU
select DRM_KMS_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT
select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select X86_PLATFORM_DEVICES if ACPI && X86

base-commit: 315d99318179b9cd5077ccc9f7f26a164c9fa998
-- 
2.30.2

[PATCH] ARM: locomo: make locomo bus's remove callback return void

[Uwe Kleine-König]

The driver core ignores the return value of struct bus_type::remove
because there is only little that can be done. To simplify the quest to
make this function return void, let struct locomo_driver::remove return
void, too. All users already unconditionally return 0, this commit makes
it obvious that returning an error code is a bad idea and ensures future
users behave accordingly.

Signed-off-by: Uwe Kleine-König 
---
Hello,

if desired the change to arch/arm/mach-sa1100/collie.c can be split out
of this patch. The change of prototype then doesn't affect this driver
any more. There is one locomo-driver that is already now unaffected:
drivers/leds/leds-locomo.c. This driver doesn't have a remove callback.

Best regards
Uwe

 arch/arm/common/locomo.c   | 5 ++---
 arch/arm/include/asm/hardware/locomo.h | 2 +-
 arch/arm/mach-sa1100/collie.c  | 6 --
 drivers/input/keyboard/locomokbd.c | 4 +---
 drivers/video/backlight/locomolcd.c| 3 +--
 5 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
index 62f241b09fe3..e45f4e4e06b6 100644
--- a/arch/arm/common/locomo.c
+++ b/arch/arm/common/locomo.c
@@ -838,11 +838,10 @@ static int locomo_bus_remove(struct device *dev)
 {
struct locomo_dev *ldev = LOCOMO_DEV(dev);
struct locomo_driver *drv = LOCOMO_DRV(dev->driver);
-   int ret = 0;
 
if (drv->remove)
-   ret = drv->remove(ldev);
-   return ret;
+   drv->remove(ldev);
+   return 0;
 }
 
 struct bus_type locomo_bus_type = {
diff --git a/arch/arm/include/asm/hardware/locomo.h 
b/arch/arm/include/asm/hardware/locomo.h
index f8712e3c29cf..246a3de25931 100644
--- a/arch/arm/include/asm/hardware/locomo.h
+++ b/arch/arm/include/asm/hardware/locomo.h
@@ -188,7 +188,7 @@ struct locomo_driver {
struct device_driverdrv;
unsigned intdevid;
int (*probe)(struct locomo_dev *);
-   int (*remove)(struct locomo_dev *);
+   void (*remove)(struct locomo_dev *);
 };
 
 #define LOCOMO_DRV(_d) container_of((_d), struct locomo_driver, drv)
diff --git a/arch/arm/mach-sa1100/collie.c b/arch/arm/mach-sa1100/collie.c
index bd3a52fd09ce..f43beb7b25c7 100644
--- a/arch/arm/mach-sa1100/collie.c
+++ b/arch/arm/mach-sa1100/collie.c
@@ -204,18 +204,12 @@ static int collie_uart_probe(struct locomo_dev *dev)
return 0;
 }
 
-static int collie_uart_remove(struct locomo_dev *dev)
-{
-   return 0;
-}
-
 static struct locomo_driver collie_uart_driver = {
.drv = {
.name = "collie_uart",
},
.devid  = LOCOMO_DEVID_UART,
.probe  = collie_uart_probe,
-   .remove = collie_uart_remove,
 };
 
 static int __init collie_uart_init(void)
diff --git a/drivers/input/keyboard/locomokbd.c 
b/drivers/input/keyboard/locomokbd.c
index daf6a753ca61..dae053596572 100644
--- a/drivers/input/keyboard/locomokbd.c
+++ b/drivers/input/keyboard/locomokbd.c
@@ -304,7 +304,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
return err;
 }
 
-static int locomokbd_remove(struct locomo_dev *dev)
+static void locomokbd_remove(struct locomo_dev *dev)
 {
struct locomokbd *locomokbd = locomo_get_drvdata(dev);
 
@@ -318,8 +318,6 @@ static int locomokbd_remove(struct locomo_dev *dev)
release_mem_region((unsigned long) dev->mapbase, dev->length);
 
kfree(locomokbd);
-
-   return 0;
 }
 
 static struct locomo_driver keyboard_driver = {
diff --git a/drivers/video/backlight/locomolcd.c 
b/drivers/video/backlight/locomolcd.c
index 297ee2e1ab0b..0468ea82159f 100644
--- a/drivers/video/backlight/locomolcd.c
+++ b/drivers/video/backlight/locomolcd.c
@@ -208,7 +208,7 @@ static int locomolcd_probe(struct locomo_dev *ldev)
return 0;
 }
 
-static int locomolcd_remove(struct locomo_dev *dev)
+static void locomolcd_remove(struct locomo_dev *dev)
 {
unsigned long flags;
 
@@ -220,7 +220,6 @@ static int locomolcd_remove(struct locomo_dev *dev)
local_irq_save(flags);
locomolcd_dev = NULL;
local_irq_restore(flags);
-   return 0;
 }
 
 static struct locomo_driver poodle_lcd_driver = {
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Uwe Kleine-König]

The remove callback is only called for devices that were probed
successfully before. As the matching probe function cannot complete
without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
check this here.

Signed-off-by: Uwe Kleine-König 
---
 sound/ppc/snd_ps3.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c
index 58bb49fff184..6ab796a5d936 100644
--- a/sound/ppc/snd_ps3.c
+++ b/sound/ppc/snd_ps3.c
@@ -1053,8 +1053,6 @@ static int snd_ps3_driver_remove(struct 
ps3_system_bus_device *dev)
 {
int ret;
pr_info("%s:start id=%d\n", __func__,  dev->match_id);
-   if (dev->match_id != PS3_MATCH_ID_SOUND)
-   return -ENXIO;
 
/*
 * ctl and preallocate buffer will be freed in
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Uwe Kleine-König]

The driver core ignores the return value of struct device_driver::remove
because there is only little that can be done. For the shutdown callback
it's ps3_system_bus_shutdown() which ignores the return value.

To simplify the quest to make struct device_driver::remove return void,
let struct ps3_system_bus_driver::remove return void, too. All users
already unconditionally return 0, this commit makes it obvious that
returning an error code is a bad idea and ensures future users behave
accordingly.

Signed-off-by: Uwe Kleine-König 
---
 arch/powerpc/include/asm/ps3.h   |  4 ++--
 arch/powerpc/platforms/ps3/system-bus.c  |  5 ++---
 drivers/block/ps3disk.c  |  3 +--
 drivers/block/ps3vram.c  |  3 +--
 drivers/char/ps3flash.c  |  3 +--
 drivers/net/ethernet/toshiba/ps3_gelic_net.c |  3 +--
 drivers/ps3/ps3-lpm.c|  3 +--
 drivers/ps3/ps3-vuart.c  | 10 --
 drivers/scsi/ps3rom.c|  3 +--
 drivers/usb/host/ehci-ps3.c  |  4 +---
 drivers/usb/host/ohci-ps3.c  |  4 +---
 drivers/video/fbdev/ps3fb.c  |  4 +---
 sound/ppc/snd_ps3.c  |  3 +--
 13 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/include/asm/ps3.h b/arch/powerpc/include/asm/ps3.h
index cb89e4bf55ce..e646c7f218bc 100644
--- a/arch/powerpc/include/asm/ps3.h
+++ b/arch/powerpc/include/asm/ps3.h
@@ -378,8 +378,8 @@ struct ps3_system_bus_driver {
enum ps3_match_sub_id match_sub_id;
struct device_driver core;
int (*probe)(struct ps3_system_bus_device *);
-   int (*remove)(struct ps3_system_bus_device *);
-   int (*shutdown)(struct ps3_system_bus_device *);
+   void (*remove)(struct ps3_system_bus_device *);
+   void (*shutdown)(struct ps3_system_bus_device *);
 /* int (*suspend)(struct ps3_system_bus_device *, pm_message_t); */
 /* int (*resume)(struct ps3_system_bus_device *); */
 };
diff --git a/arch/powerpc/platforms/ps3/system-bus.c 
b/arch/powerpc/platforms/ps3/system-bus.c
index c62aaa29a9d5..b431f41c6cb5 100644
--- a/arch/powerpc/platforms/ps3/system-bus.c
+++ b/arch/powerpc/platforms/ps3/system-bus.c
@@ -382,7 +382,6 @@ static int ps3_system_bus_probe(struct device *_dev)
 
 static int ps3_system_bus_remove(struct device *_dev)
 {
-   int result = 0;
struct ps3_system_bus_device *dev = ps3_dev_to_system_bus_dev(_dev);
struct ps3_system_bus_driver *drv;
 
@@ -393,13 +392,13 @@ static int ps3_system_bus_remove(struct device *_dev)
BUG_ON(!drv);
 
if (drv->remove)
-   result = drv->remove(dev);
+   drv->remove(dev);
else
dev_dbg(&dev->core, "%s:%d %s: no remove method\n",
__func__, __LINE__, drv->core.name);
 
pr_debug(" <- %s:%d: %s\n", __func__, __LINE__, dev_name(&dev->core));
-   return result;
+   return 0;
 }
 
 static void ps3_system_bus_shutdown(struct device *_dev)
diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index 7b55811c2a81..ba3ece56cbb3 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -507,7 +507,7 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev)
return error;
 }
 
-static int ps3disk_remove(struct ps3_system_bus_device *_dev)
+static void ps3disk_remove(struct ps3_system_bus_device *_dev)
 {
struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
struct ps3disk_private *priv = ps3_system_bus_get_drvdata(&dev->sbd);
@@ -526,7 +526,6 @@ static int ps3disk_remove(struct ps3_system_bus_device 
*_dev)
kfree(dev->bounce_buf);
kfree(priv);
ps3_system_bus_set_drvdata(_dev, NULL);
-   return 0;
 }
 
 static struct ps3_system_bus_driver ps3disk = {
diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
index 1088798c8dd0..b71d28372ef3 100644
--- a/drivers/block/ps3vram.c
+++ b/drivers/block/ps3vram.c
@@ -797,7 +797,7 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev)
return error;
 }
 
-static int ps3vram_remove(struct ps3_system_bus_device *dev)
+static void ps3vram_remove(struct ps3_system_bus_device *dev)
 {
struct ps3vram_priv *priv = ps3_system_bus_get_drvdata(dev);
 
@@ -817,7 +817,6 @@ static int ps3vram_remove(struct ps3_system_bus_device *dev)
free_pages((unsigned long) priv->xdr_buf, get_order(XDR_BUF_SIZE));
kfree(priv);
ps3_system_bus_set_drvdata(dev, NULL);
-   return 0;
 }
 
 static struct ps3_system_bus_driver ps3vram = {
diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c
index 1a07fee33f66..23871cde41fb 100644
--- a/drivers/char/ps3flash.c
+++ b/drivers/char/ps3flash.c
@@ -403,7 +403,7 @@ static int ps3flash_probe(struct ps3_system_bus_de

Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Uwe Kleine-König]

On Fri, Nov 27, 2020 at 09:35:39AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König
>  wrote:
> > The remove callback is only called for devices that were probed
> > successfully before. As the matching probe function cannot complete
> > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> > check this here.
> >
> > Signed-off-by: Uwe Kleine-König 
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven 
> 
> Note that there are similar checks in snd_ps3_driver_probe(), which
> can be removed, too:
> 
> if (WARN_ON(!firmware_has_feature(FW_FEATURE_PS3_LV1)))
> return -ENODEV;
> if (WARN_ON(dev->match_id != PS3_MATCH_ID_SOUND))
> return -ENODEV;

I had to invest some brain cycles here. For the first:

Assuming firmware_has_feature(FW_FEATURE_PS3_LV1) always returns the
same value, snd_ps3_driver_probe is only used after this check succeeds
because the driver is registered only after this check in
snd_ps3_init().

The second is superflous because ps3_system_bus_match() yields false if
this doesn't match the driver's match_id.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Uwe Kleine-König]

Hello Michael,

On Sat, Nov 28, 2020 at 09:48:30AM +0100, Takashi Iwai wrote:
> On Thu, 26 Nov 2020 17:59:50 +0100,
> Uwe Kleine-König wrote:
> > 
> > The driver core ignores the return value of struct device_driver::remove
> > because there is only little that can be done. For the shutdown callback
> > it's ps3_system_bus_shutdown() which ignores the return value.
> > 
> > To simplify the quest to make struct device_driver::remove return void,
> > let struct ps3_system_bus_driver::remove return void, too. All users
> > already unconditionally return 0, this commit makes it obvious that
> > returning an error code is a bad idea and ensures future users behave
> > accordingly.
> > 
> > Signed-off-by: Uwe Kleine-König 
> 
> For the sound bit:
> Acked-by: Takashi Iwai 

assuming that you are the one who will apply this patch: Note that it
depends on patch 1 that Takashi already applied to his tree. So you
either have to wait untils patch 1 appears in some tree that you merge
before applying, or you have to take patch 1, too. (With Takashi
optinally dropping it then.)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

ight? And now your idea to "best suite the requested period" is
to select a small divider such that you can still use a big value in
SCALE to define the period and so have a fine separation for the
duty_cycle, right?

I will stop doing maths here now until you confirm my steps up to now
are right.

> + backlight = scale * state->duty_cycle / state->period;

This is an u64 division, you must use do_div for that. Also you're
losing precision here.

> + ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> + if (ret) {
> + dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
> + goto out;
> + }
> +
> + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);

How does the PWM behave in between these writes? Are the register values
shadowed until the third write happens (which would be the optimum), or
does this result in (maybe) emitting an output wave that doesn't
correspond to the requested setting (assuming the PWM is already enabled
of course)?

What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
regmap writes, is there a race, too?

> + }
> +
> + pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +  FIELD_PREP(BIT(0), state->polarity == 
> PWM_POLARITY_INVERSED);

Please introduce symbolic names for BIT(1) and BIT(0) here.

How does the hardware behave with the enable bit unset? Does it emit the
inactive level according to the polarity bit?

> + ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
> + if (ret) {
> + dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> + goto out;
> + }
> +
> + pdata->pwm_enabled = !!state->enabled;
> +out:
> +
> + if (!pdata->pwm_enabled)
> + pm_runtime_put_sync(pdata->dev);
> +
> + return ret;
> +}
> +
> [...]
> +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +  const struct of_phandle_args *args)
> +{
> + struct pwm_device *pwm;
> +
> + if (args->args_count != 1)
> + return ERR_PTR(-EINVAL);
> +
> + pwm = pwm_request_from_chip(pc, 0, NULL);
> + if (IS_ERR(pwm))
> + return pwm;
> +
> + pwm->args.period = args->args[0];
> +
> + return pwm;
> +}

This is done to optimise away the 0 needed in each phandle to implement
the "usual" pwm binding. IMHO this function should either move into the
pwm core, or you should stick to the usual binding.

Apropos binding: Is there already a binding document for the hardware?
You should expand it to describe your additions.

> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client 
> *client,
>   return ret;
>   }
>  
> + ret = ti_sn_setup_pwmchip(pdata);
> + if (ret)  {
> + pm_runtime_disable(pdata->dev);
> + return ret;
> + }

I'm not sure about the purpose of the containing hardware, but I wonder
if it would be saner to not break probing of the device if adding the
PWM functionality fails. Ideally the driver would provide an mfd driver
that allows its components to be probed independently.

>   i2c_set_clientdata(client, pdata);
>  
>   pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client 
> *client)
>  
>   drm_bridge_remove(&pdata->bridge);
>  
> + ti_sn_remove_pwmchip(pdata);
> +
>   return 0;

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

Hi Shawn,

On Thu, Dec 10, 2020 at 09:51:37AM +0800, Shawn Guo wrote:
> On Mon, Dec 07, 2020 at 10:40:22PM -0600, Bjorn Andersson wrote:
> > The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> > with the primary purpose of controlling the backlight of the attached
> > panel. Add an implementation that exposes this using the standard PWM
> > framework, to allow e.g. pwm-backlight to expose this to the user.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> > 
> > Signed-off-by: Bjorn Andersson 
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++
> >  1 file changed, 202 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f27306c51e4d..43c0acba57ab 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -4,6 +4,7 @@
> >   * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
> >   */
> >  
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -14,6 +15,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -89,6 +91,11 @@
> >  #define SN_ML_TX_MODE_REG  0x96
> >  #define  ML_TX_MAIN_LINK_OFF   0
> >  #define  ML_TX_NORMAL_MODE BIT(0)
> > +#define SN_PWM_PRE_DIV_REG 0xA0
> > +#define SN_BACKLIGHT_SCALE_REG 0xA1
> > +#define  BACKLIGHT_SCALE_MAX   0x
> > +#define SN_BACKLIGHT_REG   0xA3
> > +#define SN_PWM_EN_INV_REG  0xA5
> >  #define SN_AUX_CMD_STATUS_REG  0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT  BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT  BIT(5)
> > @@ -111,6 +118,8 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES 10
> >  
> > +#define SN_PWM_GPIO3
> 
> So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> wondering if it's more readable to define the following SHIFT constants
> (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> offset?
> 
> #define  GPIO_MUX_GPIO1_SHIFT 0
> #define  GPIO_MUX_GPIO2_SHIFT 2
> #define  GPIO_MUX_GPIO3_SHIFT 4
> #define  GPIO_MUX_GPIO4_SHIFT 6
> 
> If you agree, you may consider to integrate this patch beforehand:
> 
> https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586

My preferred way here would be to add a prefix for the other constants.
It (IMHO) looks nicer and

GPIO_INPUT_SHIFT

looks like a quite generic name for a hardware specific definition.
(Even if up to now there is no other code location using this name.)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

On Thu, Dec 10, 2020 at 10:40:36PM +0800, Shawn Guo wrote:
> Hi Uwe,
> 
> On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
>  wrote:
> > > > @@ -111,6 +118,8 @@
> > > >
> > > >  #define SN_LINK_TRAINING_TRIES 10
> > > >
> > > > +#define SN_PWM_GPIO3
> > >
> > > So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> > > wondering if it's more readable to define the following SHIFT constants
> > > (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> > > offset?
> > >
> > > #define  GPIO_MUX_GPIO1_SHIFT 0
> > > #define  GPIO_MUX_GPIO2_SHIFT 2
> > > #define  GPIO_MUX_GPIO3_SHIFT 4
> > > #define  GPIO_MUX_GPIO4_SHIFT 6
> > >
> > > If you agree, you may consider to integrate this patch beforehand:
> > >
> > > https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
> >
> > My preferred way here would be to add a prefix for the other constants.
> > It (IMHO) looks nicer and
> >
> > GPIO_INPUT_SHIFT
> >
> > looks like a quite generic name for a hardware specific definition.
> 
> While this looks like a reasonable argument, I also like the naming
> choice for these constants in the beginning for that distinction
> between registers and bits.  And changing the names the other way
> around means there will be a much bigger diffstat, which I would like
> to avoid.  I suggest let's just focus on what really matters here -
> keep the naming consistent, so that people do not get confused when
> they want to add more constants in there.

In my eyes the bigger diffstat is justified. As I wrote,
GPIO_INPUT_SHIFT isn't used in other files, but please look how many
definitions there are for RESET. The usefulness of ctags/cscope is quite
reduced if generic terms are used this way.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v8 08/16] drm/meson: dw-hdmi: Don't open code devm_clk_get_enabled()

[Uwe Kleine-König]

devm_clk_get_enabled() returns a clock prepared and enabled and already
registers a devm exit handler to disable (and unprepare) the clock.

There is slight change in behavior as a failure to enable the clock
now results in an error message, too. Also the actual error code is added
to the message.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 48 +--
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd2b2ebbbd3..6034e80c5b2e 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -670,29 +670,6 @@ static void meson_disable_regulator(void *data)
regulator_disable(data);
 }
 
-static void meson_disable_clk(void *data)
-{
-   clk_disable_unprepare(data);
-}
-
-static int meson_enable_clk(struct device *dev, char *name)
-{
-   struct clk *clk;
-   int ret;
-
-   clk = devm_clk_get(dev, name);
-   if (IS_ERR(clk)) {
-   dev_err(dev, "Unable to get %s pclk\n", name);
-   return PTR_ERR(clk);
-   }
-
-   ret = clk_prepare_enable(clk);
-   if (!ret)
-   ret = devm_add_action_or_reset(dev, meson_disable_clk, clk);
-
-   return ret;
-}
-
 static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
void *data)
 {
@@ -702,6 +679,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
struct drm_device *drm = data;
struct meson_drm *priv = drm->dev_private;
struct dw_hdmi_plat_data *dw_plat_data;
+   struct clk *clk;
int irq;
int ret;
 
@@ -763,17 +741,23 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
if (IS_ERR(meson_dw_hdmi->hdmitx))
return PTR_ERR(meson_dw_hdmi->hdmitx);
 
-   ret = meson_enable_clk(dev, "isfr");
-   if (ret)
-   return ret;
+   clk = devm_clk_get_enabled(dev, "isfr");
+   if (IS_ERR(clk)) {
+   dev_err(dev, "Failed to get enabled isfr pclk (%pe)\n", clk);
+   return PTR_ERR(clk);
+   }
 
-   ret = meson_enable_clk(dev, "iahb");
-   if (ret)
-   return ret;
+   clk = devm_clk_get_enabled(dev, "iahb");
+   if (IS_ERR(clk)) {
+   dev_err(dev, "Failed to get enabled iahb pclk (%pe)\n", clk);
+   return PTR_ERR(clk);
+   }
 
-   ret = meson_enable_clk(dev, "venci");
-   if (ret)
-   return ret;
+   clk = devm_clk_get_enabled(dev, "venci");
+   if (IS_ERR(clk)) {
+   dev_err(dev, "Failed to get enabled venci pclk (%pe)\n", clk);
+   return PTR_ERR(clk);
+   }
 
dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
  &meson_dw_hdmi_regmap_config);
-- 
2.35.1

[PATCH v8 01/16] clk: generalize devm_clk_get() a bit

[Uwe Kleine-König]

Allow to add an exit hook to devm managed clocks. Also use
clk_get_optional() in devm_clk_get_optional instead of open coding it.
The generalisation will be used in the next commit to add some more
devm_clk helpers.

Reviewed-by: Jonathan Cameron 
Reviewed-by: Alexandru Ardelean 
Signed-off-by: Uwe Kleine-König 
---
 drivers/clk/clk-devres.c | 67 ++--
 1 file changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index f9d5b7334341..fb7761888b30 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,39 +4,72 @@
 #include 
 #include 
 
+struct devm_clk_state {
+   struct clk *clk;
+   void (*exit)(struct clk *clk);
+};
+
 static void devm_clk_release(struct device *dev, void *res)
 {
-   clk_put(*(struct clk **)res);
+   struct devm_clk_state *state = *(struct devm_clk_state **)res;
+
+   if (state->exit)
+   state->exit(state->clk);
+
+   clk_put(state->clk);
 }
 
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id,
+ struct clk *(*get)(struct device *dev, const 
char *id),
+ int (*init)(struct clk *clk),
+ void (*exit)(struct clk *clk))
 {
-   struct clk **ptr, *clk;
+   struct devm_clk_state *state;
+   struct clk *clk;
+   int ret;
 
-   ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-   if (!ptr)
+   state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
+   if (!state)
return ERR_PTR(-ENOMEM);
 
-   clk = clk_get(dev, id);
-   if (!IS_ERR(clk)) {
-   *ptr = clk;
-   devres_add(dev, ptr);
-   } else {
-   devres_free(ptr);
+   clk = get(dev, id);
+   if (IS_ERR(clk)) {
+   ret = PTR_ERR(clk);
+   goto err_clk_get;
}
 
+   if (init) {
+   ret = init(clk);
+   if (ret)
+   goto err_clk_init;
+   }
+
+   state->clk = clk;
+   state->exit = exit;
+
+   devres_add(dev, state);
+
return clk;
+
+err_clk_init:
+
+   clk_put(clk);
+err_clk_get:
+
+   devres_free(state);
+   return ERR_PTR(ret);
 }
-EXPORT_SYMBOL(devm_clk_get);
 
-struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-   struct clk *clk = devm_clk_get(dev, id);
+   return __devm_clk_get(dev, id, clk_get, NULL, NULL);
 
-   if (clk == ERR_PTR(-ENOENT))
-   return NULL;
+}
+EXPORT_SYMBOL(devm_clk_get);
 
-   return clk;
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+   return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
-- 
2.35.1

[PATCH v8 02/16] clk: Provide new devm_clk helpers for prepared and enabled clocks

[Uwe Kleine-König]

When a driver keeps a clock prepared (or enabled) during the whole
lifetime of the driver, these helpers allow to simplify the drivers.

Reviewed-by: Jonathan Cameron 
Reviewed-by: Alexandru Ardelean 
Signed-off-by: Uwe Kleine-König 
---
 drivers/clk/clk-devres.c | 31 ++
 include/linux/clk.h  | 90 +++-
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index fb7761888b30..4707fe718f0b 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -67,12 +67,43 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id)
+{
+   return __devm_clk_get(dev, id, clk_get, clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_prepared);
+
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id)
+{
+   return __devm_clk_get(dev, id, clk_get,
+ clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_enabled);
+
 struct clk *devm_clk_get_optional(struct device *dev, const char *id)
 {
return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
+struct clk *devm_clk_get_optional_prepared(struct device *dev, const char *id)
+{
+   return __devm_clk_get(dev, id, clk_get_optional,
+ clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_prepared);
+
+struct clk *devm_clk_get_optional_enabled(struct device *dev, const char *id)
+{
+   return __devm_clk_get(dev, id, clk_get_optional,
+ clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_enabled);
+
 struct clk_bulk_devres {
struct clk_bulk_data *clks;
int num_clks;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..b011dbba7109 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -449,7 +449,7 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  * the clock producer.  (IOW, @id may be identical strings, but
  * clk_get may return different clock producers depending on @dev.)
  *
- * Drivers must assume that the clock source is not enabled.
+ * Drivers must assume that the clock source is neither prepared nor enabled.
  *
  * devm_clk_get should not be called from within interrupt context.
  *
@@ -458,6 +458,47 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_prepared - devm_clk_get() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno.  The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer.  (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared. Drivers must however assume that 
the
+ * clock is not enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id);
+
+/**
+ * devm_clk_get_enabled - devm_clk_get() + clk_prepare_enable()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or valid IS_ERR()
+ * condition containing errno.  The implementation uses @dev and @id to
+ * determine the clock consumer, and thereby the clock producer.  (IOW, @id may
+ * be identical strings, but clk_get may return different clock producers
+ * depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared and enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be disabled, unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id);
+
 /**
  * devm_clk_get_optional - lookup and obtain a managed reference to an optional
  *clock producer.
@@ -469,6 +510,29 @@ struct clk *devm_clk_get(struct device *dev, const char 
*id);
  */
 struct clk *devm_clk_get_optional(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_optional_prepared - devm_clk_get_optional() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get_prepared() except where there is no clock
+ * producer.  In this case, instead of returning -ENOENT, the function returns
+ * NULL.
+ */
+struct clk *devm_clk_get_optional_prep

[PATCH v8 00/16] clk: provide new devm helpers for prepared and enabled clocks

[Uwe Kleine-König]

Hello,

this is another try to convince the relevant people that
devm_clk_get_enabled() is a nice idea. Compared to v7 (back in May 2021) this
series is rebased to v5.17-rc8 and converts quite some drivers that open code
devm_clk_get_enabled() up to now (patches #3 - #11).

A concern about devm_clk_get_enabled() in v7 was that it helps people to be
lazy and I agree that in some situations when devm_clk_get_enabled() is used it
would be more efficient and sensible to care to only enable the clk when really
needed.

On the other hand, the function is right for some users, e.g. the watchdog
drivers. For the others it's not so simple to judge. Given that there are a
lot of drivers that are lazy even if doing so is some effort (i.e. calling
clk_prepare_enable() and devm_add_action()) convinces me, that introducing the
function family is sensible. (And if you want to work on these drivers,
grepping for devm_clk_get_enabled gives you a few candidates once the
series is in :-)

Otherwise looking at the diffstat of this series:

 48 files changed, 257 insertions(+), 851 deletions(-)

is quite convincing. Just the first two patches (which introduce the new
functions) account for

 2 files changed, 169 insertions(+), 17 deletions(-)

. A rough third of the added lines is documentation. The rest is driver
updates which then has:

 46 files changed, 88 insertions(+), 834 deletions(-)

which makes a really nice cleanup.

The series is build-tested on arm64, m68k, powerpc, riscv, s390, sparc64
and x86_64 using an allmodconfig.

Best regards
Uwe

Uwe Kleine-König (16):
  clk: generalize devm_clk_get() a bit
  clk: Provide new devm_clk helpers for prepared and enabled clocks
  hwmon: Make use of devm_clk_get_enabled()
  iio: Make use of devm_clk_get_enabled()
  hwrng: meson - Don't open-code devm_clk_get_optional_enabled()
  bus: bt1: Don't open code devm_clk_get_enabled()
  gpio: vf610: Simplify error handling in probe
  drm/meson: dw-hdmi: Don't open code devm_clk_get_enabled()
  rtc: ingenic: Simplify using devm_clk_get_enabled()
  clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()
  watchdog: Make use of devm_clk_get_enabled()
  pwm: atmel: Simplify using devm_clk_get_prepared()
  rtc: at91sam9: Simplify using devm_clk_get_enabled()
  i2c: imx: Simplify using devm_clk_get_enabled()
  spi: davinci: Simplify using devm_clk_get_enabled()
  dmaengine: lgm: Fix error handling

 drivers/bus/bt1-apb.c | 23 +--
 drivers/bus/bt1-axi.c | 23 +--
 drivers/char/hw_random/meson-rng.c| 20 +-
 drivers/clk/clk-devres.c  | 96 ++-
 drivers/clk/meson/axg-audio.c | 36 ++
 drivers/dma/lgm/lgm-dma.c |  8 +--
 drivers/gpio/gpio-vf610.c | 45 +++--
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 48 +-
 drivers/hwmon/axi-fan-control.c   | 15 +
 drivers/hwmon/ltc2947-core.c  | 17 +
 drivers/hwmon/mr75203.c   | 26 +---
 drivers/hwmon/sparx5-temp.c   | 19 +-
 drivers/i2c/busses/i2c-imx.c  | 12 +---
 drivers/iio/adc/ad7124.c  | 15 +
 drivers/iio/adc/ad7768-1.c| 17 +
 drivers/iio/adc/ad9467.c  | 17 +
 drivers/iio/adc/ingenic-adc.c | 15 +
 drivers/iio/adc/lpc18xx_adc.c | 18 +
 drivers/iio/adc/rockchip_saradc.c | 44 +---
 drivers/iio/adc/ti-ads131e08.c| 19 +-
 drivers/iio/adc/xilinx-ams.c  | 15 +
 drivers/iio/adc/xilinx-xadc-core.c| 18 +
 drivers/iio/frequency/adf4371.c   | 17 +
 drivers/iio/frequency/admv1013.c  | 15 +
 drivers/iio/frequency/adrf6780.c  | 16 +
 drivers/iio/imu/adis16475.c   | 15 +
 drivers/pwm/pwm-atmel.c   | 16 +
 drivers/rtc/rtc-at91sam9.c| 22 ++
 drivers/rtc/rtc-jz4740.c  | 21 +-
 drivers/spi/spi-davinci.c | 11 +--
 drivers/watchdog/cadence_wdt.c| 17 +
 drivers/watchdog/davinci_wdt.c| 18 +
 drivers/watchdog/imgpdc_wdt.c | 31 +
 drivers/watchdog/imx2_wdt.c   | 15 +
 drivers/watchdog/imx7ulp_wdt.c| 15 +
 drivers/watchdog/loongson1_wdt.c  | 17 +
 drivers/watchdog/lpc18xx_wdt.c| 30 +
 drivers/watchdog/meson_gxbb_wdt.c | 16 +
 drivers/watchdog/of_xilinx_wdt.c  | 16 +
 drivers/watchdog/pic32-dmt.c  | 15 +
 drivers/watchdog/pic32-wdt.c  | 17 +
 drivers/watchdog/pnx4008_wdt.c| 15 +
 drivers/watchdog/qcom-wdt.c   | 16 +
 drivers/watchdog/rtd119x_wdt.c| 16 +
 drivers/watchdog/st_lpc_wdt.c | 16 +
 drivers/watchdog/stm32_iwdg.c | 31 +
 drivers/watchdog/visconti_wdt.c   | 18 +
 include/linux/clk.h   | 90 -
 48 files changed, 257 insertions(+), 851 d

Re: [PATCH v8 02/16] clk: Provide new devm_clk helpers for prepared and enabled clocks

[Uwe Kleine-König]

On Sat, Mar 19, 2022 at 06:29:36PM +, Jonathan Cameron wrote:
> On Mon, 14 Mar 2022 15:16:29 +0100
> Uwe Kleine-König  wrote:
> 
> > When a driver keeps a clock prepared (or enabled) during the whole
> > lifetime of the driver, these helpers allow to simplify the drivers.
> > 
> > Reviewed-by: Jonathan Cameron 
> > Reviewed-by: Alexandru Ardelean 
> > Signed-off-by: Uwe Kleine-König 
> 
> One trivial thing below.
> 
> > ---
> >  drivers/clk/clk-devres.c | 31 ++
> >  include/linux/clk.h  | 90 +++-
> >  2 files changed, 120 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> > index fb7761888b30..4707fe718f0b 100644
> > --- a/drivers/clk/clk-devres.c
> > +++ b/drivers/clk/clk-devres.c
> > @@ -67,12 +67,43 @@ struct clk *devm_clk_get(struct device *dev, const char 
> > *id)
> >  }
> >  EXPORT_SYMBOL(devm_clk_get);
> >  
> > +struct clk *devm_clk_get_prepared(struct device *dev, const char *id)
> > +{
> > +   return __devm_clk_get(dev, id, clk_get, clk_prepare, clk_unprepare);
> 
> Nitpick but this spacing before } in functions is rather unusual and not
> in keeping with the existing code in this file.
> 
> > +
> > +}

ack, I fixed that in my tree, so this will be part of an v9. I won't
send it just for this change, though. I fixed three further functions
that had a similar empty line, too.

Thanks for looking
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 1/2] dt-bindings: display: imx: Add fsl,imx21-lcdc docs

[Uwe Kleine-König]

Hi Rob,

On Mon, Feb 21, 2022 at 02:55:36PM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 10, 2022 at 06:54:13PM +0100, Lucas Stach wrote:
> > Am Dienstag, dem 01.02.2022 um 11:35 -0600 schrieb Rob Herring:
> > > On Fri, Jan 28, 2022 at 06:58:29PM +0100, Uwe Kleine-König wrote:
> > > > On Fri, Jan 28, 2022 at 07:04:10AM -0600, Rob Herring wrote:
> > > > > On Fri, Jan 28, 2022 at 4:59 AM Uwe Kleine-König
> > > > >  wrote:
> > > > > > 
> > > > > > From: Marian Cichy 
> > > > > > 
> > > > > > This files documents the device tree for the new imx21-lcdc DRM 
> > > > > > driver.
> > > > > 
> > > > > No, bindings document h/w and the h/w has not changed. We already have
> > > > > a binding for the LCDC.
> > > > 
> > > > Just to be sure we're talking about the same thing: You're refering to
> > > > Documentation/devicetree/bindings/display/imx/fsl,imx-fb.txt, right?
> > > 
> > > Looks right...
> > > 
> > > > I'm unsure what to do now. Should the two different bindings just be
> > > > described in the same file? Should I stick to fsl,imx21-fb even for the
> > > > new binding? (The hardware unit is named LCDC, so the name chosen here
> > > > is the better one.) Please advise.
> > > 
> > > Yes, the name is unfortunate, but it should be 1 binding, 1 file, and 
> > > unchanged (unless you want to add new optional properties). 
> >
> > The old FB driver binding is pretty insane. Except the reg and
> > interrupt properties it is very confused about things. It exposes
> > internal implementation details (like specifying verbatim register
> > settings in the DT) and other properties are just misplaced, like the
> > lcd-supply property that controls the panel power supply.
> > 
> > I really don't think that trying to stay backwards compatible here is a
> > win for anyone. Anyone willing to switch their systems running on a 15
> > year old SoC to the new DRM driver probably doesn't mind if they have
> > to modify the DTS to make it work. Can we please let the FB driver die
> > in peace and have a clean slate driver/binding for the DRM driver?
> 
> Does this feedback change anything on your side? My expectation is that
> the graphics people will be happy about every fb driver being replaced
> by a DRM driver and there will be hardly any incentive to add a layer
> over the DRM driver to make it honor the old fb binding.
> 
> Assume I convert the old binding to yaml and then add the newly
> supported binding to that using a big oneOf, would that be an acceptable
> compromise?

I'd like to get forward with this driver. What would be a good way to
continue here? 

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v16 21/40] pwm: tegra: Add runtime PM and OPP support

[Uwe Kleine-König]

+ pm_runtime_put(&pdev->dev);
> +
>   return 0;
> +put_pm:
> + pm_runtime_put_sync_suspend(&pdev->dev);
> + pm_runtime_force_suspend(&pdev->dev);
> + return ret;
>  }
>  
>  static int tegra_pwm_remove(struct platform_device *pdev)
> @@ -305,20 +324,44 @@ static int tegra_pwm_remove(struct platform_device 
> *pdev)
>  
>   reset_control_assert(pc->rst);
>  
> + pm_runtime_force_suspend(&pdev->dev);
> +
>   return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int tegra_pwm_suspend(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
>  {
> - return pinctrl_pm_select_sleep_state(dev);
> + struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> + int err;
> +
> + clk_disable_unprepare(pc->clk);
> +
> + err = pinctrl_pm_select_sleep_state(dev);
> + if (err) {
> + clk_prepare_enable(pc->clk);
> + return err;
> + }
> +
> + return 0;
>  }
>  
> -static int tegra_pwm_resume(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_resume(struct device *dev)
>  {
> - return pinctrl_pm_select_default_state(dev);
> + struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> + int err;
> +
> + err = pinctrl_pm_select_default_state(dev);
> + if (err)
> + return err;
> +
> + err = clk_prepare_enable(pc->clk);
> + if (err) {
> + pinctrl_pm_select_sleep_state(dev);
> + return err;
> + }
> +
> + return 0;
>  }
> -#endif
>  
>  static const struct tegra_pwm_soc tegra20_pwm_soc = {
>   .num_channels = 4,
> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>  
>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> + SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
> +NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
>  };
>  
>  static struct platform_driver tegra_pwm_driver = {

I admit to not completely understand the effects of this patch, but I
don't see a problem either. So for me this patch is OK:

Acked-by: Uwe Kleine-König 

I spot a problem, it's not introduced by this patch however: If the
consumer of the PWM didn't stop the hardware, the suspend should IMHO be
prevented.

I wonder if the patches in this series go in in one go via an ARM or
Tegra tree, or each patch via its respective maintainer tree.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] drm: Only select I2C_ALGOBIT for drivers that actually need it

[Uwe Kleine-König]

Hello,

On Fri, May 14, 2021 at 12:01:42PM +0200, Uwe Kleine-König wrote:
> While working on a drm driver that doesn't need the i2c algobit stuff I
> noticed that DRM selects this code even tough only 8 drivers actually use
> it. While also only some drivers use i2c, keep the select for I2C for the
> next cleanup patch. Still prepare this already by also selecting I2C for
> the individual drivers.
> 
> Signed-off-by: Uwe Kleine-König 

This patch is already old but the issue is still valid and the patch
even still applies to today's Linus' master branch.

I didn't receive any human feedback. If you consider this patch
worthwile I can recheck if it's still correct as is or needs adaption.

Best regards
Uwe

> ---
>  drivers/gpu/drm/Kconfig | 5 -
>  drivers/gpu/drm/ast/Kconfig | 2 ++
>  drivers/gpu/drm/gma500/Kconfig  | 2 ++
>  drivers/gpu/drm/hisilicon/hibmc/Kconfig | 2 ++
>  drivers/gpu/drm/i915/Kconfig| 2 ++
>  drivers/gpu/drm/mgag200/Kconfig | 2 ++
>  drivers/gpu/drm/nouveau/Kconfig | 2 ++
>  7 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 3c16bd1afd87..351ea617c498 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,7 +12,6 @@ menuconfig DRM
>   select HDMI
>   select FB_CMDLINE
>   select I2C
> - select I2C_ALGOBIT
>   select DMA_SHARED_BUFFER
>   select SYNC_FILE
>  # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
> @@ -233,6 +232,8 @@ config DRM_RADEON
>  select DRM_KMS_HELPER
>  select DRM_TTM
>   select DRM_TTM_HELPER
> + select I2C
> + select I2C_ALGOBIT
>   select POWER_SUPPLY
>   select HWMON
>   select BACKLIGHT_CLASS_DEVICE
> @@ -254,6 +255,8 @@ config DRM_AMDGPU
>   select DRM_SCHED
>   select DRM_TTM
>   select DRM_TTM_HELPER
> + select I2C
> + select I2C_ALGOBIT
>   select POWER_SUPPLY
>   select HWMON
>   select BACKLIGHT_CLASS_DEVICE
> diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
> index fbcf2f45cef5..bcc25decd485 100644
> --- a/drivers/gpu/drm/ast/Kconfig
> +++ b/drivers/gpu/drm/ast/Kconfig
> @@ -6,6 +6,8 @@ config DRM_AST
>   select DRM_VRAM_HELPER
>   select DRM_TTM
>   select DRM_TTM_HELPER
> + select I2C
> + select I2C_ALGOBIT
>   help
>Say yes for experimental AST GPU driver. Do not enable
>this driver without having a working -modesetting,
> diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
> index 0cff20265f97..e26c3a24955d 100644
> --- a/drivers/gpu/drm/gma500/Kconfig
> +++ b/drivers/gpu/drm/gma500/Kconfig
> @@ -3,6 +3,8 @@ config DRM_GMA500
>   tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
>   depends on DRM && PCI && X86 && MMU
>   select DRM_KMS_HELPER
> + select I2C
> + select I2C_ALGOBIT
>   # GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
>   select ACPI_VIDEO if ACPI
>   select BACKLIGHT_CLASS_DEVICE if ACPI
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig 
> b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
> index 43943e980203..ac8c42dc79f6 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
> @@ -6,6 +6,8 @@ config DRM_HISI_HIBMC
>   select DRM_VRAM_HELPER
>   select DRM_TTM
>   select DRM_TTM_HELPER
> + select I2C
> + select I2C_ALGOBIT
>   help
> Choose this option if you have a Hisilicon Hibmc soc chipset.
> If M is selected the module will be called hibmc-drm.
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 69f57ca9c68d..b3bb6f7cfbbc 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -13,6 +13,8 @@ config DRM_I915
>   select DRM_PANEL
>   select DRM_MIPI_DSI
>   select RELAY
> + select I2C
> + select I2C_ALGOBIT
>   select IRQ_WORK
>   # i915 depends on ACPI_VIDEO when ACPI is enabled
>   # but for select to work, need to select ACPI_VIDEO's dependencies, ick
> diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
> index eec59658a938..b28c5e4828f4 100644
> --- a/drivers/gpu/drm/mgag200/Kconfig
> +++ b/drivers/gpu/drm/mgag200/Kconfig
> @@ -4,6 +4,8 @@ config DRM_MGAG200
>   depends on DRM && PCI && MMU
>   select DRM_GEM_SHMEM_HELPER
>   select DRM_KMS_HELPER
> + select I2C
> + select I2C_ALG

Re: [PATCH v16 21/40] pwm: tegra: Add runtime PM and OPP support

[Uwe Kleine-König]

Hello,

On Mon, Feb 21, 2022 at 12:53:58PM +0300, Dmitry Osipenko wrote:
> 21.02.2022 11:17, Uwe Kleine-König пишет:
> >> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] 
> >> = {
> >>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
> >>  
> >>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
> >> -  SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> >> +  SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
> >> + NULL)
> >> +  SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> +  pm_runtime_force_resume)
> >>  };
> >>  
> >>  static struct platform_driver tegra_pwm_driver = {
> > I admit to not completely understand the effects of this patch, but I
> > don't see a problem either. So for me this patch is OK:
> > 
> > Acked-by: Uwe Kleine-König 
> > 
> > I spot a problem, it's not introduced by this patch however: If the
> > consumer of the PWM didn't stop the hardware, the suspend should IMHO be
> > prevented.
> 
> Why? The PWM driver itself will stop the h/w on suspend.

Stopping the PWM might be bad. Only the consumer can know if it's ok to
stop the PWM on suspend. If so the consumer should stop the PWM in their
suspend callback and the PWM should prevent suspend if it wasn't
stopped.

> > I wonder if the patches in this series go in in one go via an ARM or
> > Tegra tree, or each patch via its respective maintainer tree.
> 
> This series, including this patch, was already applied to 5.17 via the
> tegra/soc tree. No action is needed anymore.

Ah, I missed that, thanks.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 1/2] dt-bindings: display: imx: Add fsl,imx21-lcdc docs

[Uwe Kleine-König]

Hi Rob,

On Thu, Feb 10, 2022 at 06:54:13PM +0100, Lucas Stach wrote:
> Am Dienstag, dem 01.02.2022 um 11:35 -0600 schrieb Rob Herring:
> > On Fri, Jan 28, 2022 at 06:58:29PM +0100, Uwe Kleine-König wrote:
> > > On Fri, Jan 28, 2022 at 07:04:10AM -0600, Rob Herring wrote:
> > > > On Fri, Jan 28, 2022 at 4:59 AM Uwe Kleine-König
> > > >  wrote:
> > > > > 
> > > > > From: Marian Cichy 
> > > > > 
> > > > > This files documents the device tree for the new imx21-lcdc DRM 
> > > > > driver.
> > > > 
> > > > No, bindings document h/w and the h/w has not changed. We already have
> > > > a binding for the LCDC.
> > > 
> > > Just to be sure we're talking about the same thing: You're refering to
> > > Documentation/devicetree/bindings/display/imx/fsl,imx-fb.txt, right?
> > 
> > Looks right...
> > 
> > > I'm unsure what to do now. Should the two different bindings just be
> > > described in the same file? Should I stick to fsl,imx21-fb even for the
> > > new binding? (The hardware unit is named LCDC, so the name chosen here
> > > is the better one.) Please advise.
> > 
> > Yes, the name is unfortunate, but it should be 1 binding, 1 file, and 
> > unchanged (unless you want to add new optional properties). 
>
> The old FB driver binding is pretty insane. Except the reg and
> interrupt properties it is very confused about things. It exposes
> internal implementation details (like specifying verbatim register
> settings in the DT) and other properties are just misplaced, like the
> lcd-supply property that controls the panel power supply.
> 
> I really don't think that trying to stay backwards compatible here is a
> win for anyone. Anyone willing to switch their systems running on a 15
> year old SoC to the new DRM driver probably doesn't mind if they have
> to modify the DTS to make it work. Can we please let the FB driver die
> in peace and have a clean slate driver/binding for the DRM driver?

Does this feedback change anything on your side? My expectation is that
the graphics people will be happy about every fb driver being replaced
by a DRM driver and there will be hardly any incentive to add a layer
over the DRM driver to make it honor the old fb binding.

Assume I convert the old binding to yaml and then add the newly
supported binding to that using a big oneOf, would that be an acceptable
compromise?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH 12/13] staging: fbtft: Make fbtft_remove_common() return void

[Uwe Kleine-König]

fbtft_remove_common() is only called with a non-NULL fb_info. (All
callers are in remove callbacks and the matching probe callbacks set
driver data accordingly.) So fbtft_remove_common() always returns zero.
Make it return void instead which makes it easier to see in the callers
that there is no error to handle.

Also the return value of platform and spi remove callbacks is ignored
anyway and not freeing resources in .remove() is a bad idea.

Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft-core.c | 8 +---
 drivers/staging/fbtft/fbtft.h  | 6 --
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c 
b/drivers/staging/fbtft/fbtft-core.c
index ed992ca605eb..9c9eab1182a6 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1318,23 +1318,17 @@ EXPORT_SYMBOL(fbtft_probe_common);
  * @info: Framebuffer
  *
  * Unregisters and releases the framebuffer
- *
- * Return: 0 if successful, negative if error
  */
-int fbtft_remove_common(struct device *dev, struct fb_info *info)
+void fbtft_remove_common(struct device *dev, struct fb_info *info)
 {
struct fbtft_par *par;
 
-   if (!info)
-   return -EINVAL;
par = info->par;
if (par)
fbtft_par_dbg(DEBUG_DRIVER_INIT_FUNCTIONS, par,
  "%s()\n", __func__);
fbtft_unregister_framebuffer(info);
fbtft_framebuffer_release(info);
-
-   return 0;
 }
 EXPORT_SYMBOL(fbtft_remove_common);
 
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 76f8c090a837..68eba6c71b0f 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -283,7 +283,8 @@ static int fbtft_driver_remove_spi(struct spi_device *spi)  
   \
 {  \
struct fb_info *info = spi_get_drvdata(spi);   \
   \
-   return fbtft_remove_common(&spi->dev, info);   \
+   fbtft_remove_common(&spi->dev, info);  \
+   return 0;  \
 }  \
   \
 static int fbtft_driver_probe_pdev(struct platform_device *pdev)   \
@@ -295,7 +296,8 @@ static int fbtft_driver_remove_pdev(struct platform_device 
*pdev)  \
 {  \
struct fb_info *info = platform_get_drvdata(pdev); \
   \
-   return fbtft_remove_common(&pdev->dev, info);  \
+   fbtft_remove_common(&pdev->dev, info); \
+   return 0;  \
 }  \
   \
 static const struct of_device_id dt_ids[] = {  \
-- 
2.30.2

[PATCH 01/13] drm/panel: s6e63m0: Make s6e63m0_remove() return void

[Uwe Kleine-König]

Up to now s6e63m0_remove() returns zero unconditionally. Make it return
void instead which makes it easier to see in the callers that there is
no error to handle.

Also the return value of spi remove callbacks is ignored anyway.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c | 3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
index e0b1a7e354f3..e0f773678168 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
@@ -116,7 +116,8 @@ static int s6e63m0_dsi_probe(struct mipi_dsi_device *dsi)
 static int s6e63m0_dsi_remove(struct mipi_dsi_device *dsi)
 {
mipi_dsi_detach(dsi);
-   return s6e63m0_remove(&dsi->dev);
+   s6e63m0_remove(&dsi->dev);
+   return 0;
 }
 
 static const struct of_device_id s6e63m0_dsi_of_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
index 3669cc3719ce..c178d962b0d5 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
@@ -64,7 +64,8 @@ static int s6e63m0_spi_probe(struct spi_device *spi)
 
 static int s6e63m0_spi_remove(struct spi_device *spi)
 {
-   return s6e63m0_remove(&spi->dev);
+   s6e63m0_remove(&spi->dev);
+   return 0;
 }
 
 static const struct of_device_id s6e63m0_spi_of_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
index 35d72ac663d6..b34fa4d5de07 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -749,13 +749,11 @@ int s6e63m0_probe(struct device *dev, void *trsp,
 }
 EXPORT_SYMBOL_GPL(s6e63m0_probe);
 
-int s6e63m0_remove(struct device *dev)
+void s6e63m0_remove(struct device *dev)
 {
struct s6e63m0 *ctx = dev_get_drvdata(dev);
 
drm_panel_remove(&ctx->panel);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(s6e63m0_remove);
 
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
index 306605ed1117..c926eca1c817 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
@@ -35,6 +35,6 @@ int s6e63m0_probe(struct device *dev, void *trsp,
   const u8 *data,
   size_t len),
  bool dsi_mode);
-int s6e63m0_remove(struct device *dev);
+void s6e63m0_remove(struct device *dev);
 
 #endif /* _PANEL_SAMSUNG_S6E63M0_H */
-- 
2.30.2

[PATCH 00/13] Make some spi device drivers return zero in .remove()

[Uwe Kleine-König]

Hello,

this series is part of my new quest to make spi remove callbacks return
void. Today they return an int, but the only result of returning a
non-zero value is a warning message. So it's a bad idea to return an
error code in the expectation that not freeing some resources is ok
then. The same holds true for i2c and platform devices which benefit en
passant for a few drivers.

The patches in this series address some of the spi drivers that might
return non-zero and adapt them accordingly to return zero instead. For
most drivers it's just about not hiding the fact that they already
return zero.

Given that there are quite some more patches of this type to create
before I can change the spi remove callback, I suggest the respecive
subsystem maintainers pick up these patches. There are no
interdependencies in this series.

Uwe Kleine-König (13):
  drm/panel: s6e63m0: Make s6e63m0_remove() return void
  hwmon: adt7x10: Make adt7x10_remove() return void
  hwmon: max31722: Warn about failure to put device in stand-by in
.remove()
  input: adxl34xx: Make adxl34x_remove() return void
  input: touchscreen: tsc200x: Make tsc200x_remove() return void
  media: cxd2880: Eliminate dead code
  mfd: mc13xxx: Make mc13xxx_common_exit() return void
  mfd: stmpe: Make stmpe_remove() return void
  mfd: tps65912: Make tps65912_device_exit() return void
  serial: max310x: Make max310x_remove() return void
  serial: sc16is7xx: Make sc16is7xx_remove() return void
  staging: fbtft: Make fbtft_remove_common() return void
  tpm: st33zp24: Make st33zp24_remove() return void

 drivers/char/tpm/st33zp24/i2c.c   |  5 +
 drivers/char/tpm/st33zp24/spi.c   |  5 +
 drivers/char/tpm/st33zp24/st33zp24.c  |  3 +--
 drivers/char/tpm/st33zp24/st33zp24.h  |  2 +-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c |  3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c |  3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.h |  2 +-
 drivers/hwmon/adt7310.c   |  3 ++-
 drivers/hwmon/adt7410.c   |  3 ++-
 drivers/hwmon/adt7x10.c   |  3 +--
 drivers/hwmon/adt7x10.h   |  2 +-
 drivers/hwmon/max31722.c  |  8 +++-
 drivers/input/misc/adxl34x-i2c.c  |  4 +++-
 drivers/input/misc/adxl34x-spi.c  |  4 +++-
 drivers/input/misc/adxl34x.c  |  4 +---
 drivers/input/misc/adxl34x.h  |  2 +-
 drivers/input/touchscreen/tsc2004.c   |  4 +++-
 drivers/input/touchscreen/tsc2005.c   |  4 +++-
 drivers/input/touchscreen/tsc200x-core.c  |  4 +---
 drivers/input/touchscreen/tsc200x-core.h  |  2 +-
 drivers/media/spi/cxd2880-spi.c   | 13 +
 drivers/mfd/mc13xxx-core.c|  4 +---
 drivers/mfd/mc13xxx-i2c.c |  3 ++-
 drivers/mfd/mc13xxx-spi.c |  3 ++-
 drivers/mfd/mc13xxx.h |  2 +-
 drivers/mfd/stmpe-i2c.c   |  4 +++-
 drivers/mfd/stmpe-spi.c   |  4 +++-
 drivers/mfd/stmpe.c   |  4 +---
 drivers/mfd/stmpe.h   |  2 +-
 drivers/mfd/tps65912-core.c   |  4 +---
 drivers/mfd/tps65912-i2c.c|  4 +++-
 drivers/mfd/tps65912-spi.c|  4 +++-
 drivers/staging/fbtft/fbtft-core.c|  8 +---
 drivers/staging/fbtft/fbtft.h |  6 --
 drivers/tty/serial/max310x.c  |  7 +++
 drivers/tty/serial/sc16is7xx.c| 10 +++---
 include/linux/mfd/tps65912.h  |  2 +-
 38 files changed, 77 insertions(+), 81 deletions(-)


base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
-- 
2.30.2

Re: [PATCH 00/13] Make some spi device drivers return zero in .remove()

[Uwe Kleine-König]

Hello,

On Mon, Oct 11, 2021 at 03:27:41PM +0200, Uwe Kleine-König wrote:
> this series is part of my new quest to make spi remove callbacks return
> void. Today they return an int, but the only result of returning a
> non-zero value is a warning message. So it's a bad idea to return an
> error code in the expectation that not freeing some resources is ok
> then. The same holds true for i2c and platform devices which benefit en
> passant for a few drivers.
> 
> The patches in this series address some of the spi drivers that might
> return non-zero and adapt them accordingly to return zero instead. For
> most drivers it's just about not hiding the fact that they already
> return zero.
> 
> Given that there are quite some more patches of this type to create
> before I can change the spi remove callback, I suggest the respecive
> subsystem maintainers pick up these patches. There are no
> interdependencies in this series.
> 
> Uwe Kleine-König (13):
>   drm/panel: s6e63m0: Make s6e63m0_remove() return void
>   hwmon: adt7x10: Make adt7x10_remove() return void
>   hwmon: max31722: Warn about failure to put device in stand-by in
> .remove()
>   input: adxl34xx: Make adxl34x_remove() return void
>   input: touchscreen: tsc200x: Make tsc200x_remove() return void
>   media: cxd2880: Eliminate dead code
>   mfd: mc13xxx: Make mc13xxx_common_exit() return void
>   mfd: stmpe: Make stmpe_remove() return void
>   mfd: tps65912: Make tps65912_device_exit() return void
>   serial: max310x: Make max310x_remove() return void
>   serial: sc16is7xx: Make sc16is7xx_remove() return void
>   staging: fbtft: Make fbtft_remove_common() return void
>   tpm: st33zp24: Make st33zp24_remove() return void

I thought I would be a good enough programmer to not need build tests.
Obviously I was wrong and introduced build problems with the following
patches:

input: touchscreen: tsc200x: Make tsc200x_remove() return void
mfd: mc13xxx: Make mc13xxx_common_exit() return void
serial: max310x: Make max310x_remove() return void
serial: sc16is7xx: Make sc16is7xx_remove() return void

Please don't apply these (unless you also fix the trivial problems in
them). I will prepare a v2 soon.

Best regards and sorry for the inconvenience,
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH v2 01/20] drm/panel: s6e63m0: Make s6e63m0_remove() return void

[Uwe Kleine-König]

Up to now s6e63m0_remove() returns zero unconditionally. Make it return
void instead which makes it easier to see in the callers that there is
no error to handle.

Also the return value of spi remove callbacks is ignored anyway.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c | 3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
index e0b1a7e354f3..e0f773678168 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
@@ -116,7 +116,8 @@ static int s6e63m0_dsi_probe(struct mipi_dsi_device *dsi)
 static int s6e63m0_dsi_remove(struct mipi_dsi_device *dsi)
 {
mipi_dsi_detach(dsi);
-   return s6e63m0_remove(&dsi->dev);
+   s6e63m0_remove(&dsi->dev);
+   return 0;
 }
 
 static const struct of_device_id s6e63m0_dsi_of_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
index 3669cc3719ce..c178d962b0d5 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
@@ -64,7 +64,8 @@ static int s6e63m0_spi_probe(struct spi_device *spi)
 
 static int s6e63m0_spi_remove(struct spi_device *spi)
 {
-   return s6e63m0_remove(&spi->dev);
+   s6e63m0_remove(&spi->dev);
+   return 0;
 }
 
 static const struct of_device_id s6e63m0_spi_of_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
index 35d72ac663d6..b34fa4d5de07 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -749,13 +749,11 @@ int s6e63m0_probe(struct device *dev, void *trsp,
 }
 EXPORT_SYMBOL_GPL(s6e63m0_probe);
 
-int s6e63m0_remove(struct device *dev)
+void s6e63m0_remove(struct device *dev)
 {
struct s6e63m0 *ctx = dev_get_drvdata(dev);
 
drm_panel_remove(&ctx->panel);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(s6e63m0_remove);
 
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h 
b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
index 306605ed1117..c926eca1c817 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
@@ -35,6 +35,6 @@ int s6e63m0_probe(struct device *dev, void *trsp,
   const u8 *data,
   size_t len),
  bool dsi_mode);
-int s6e63m0_remove(struct device *dev);
+void s6e63m0_remove(struct device *dev);
 
 #endif /* _PANEL_SAMSUNG_S6E63M0_H */
-- 
2.30.2

[PATCH v2 19/20] staging: fbtft: Make fbtft_remove_common() return void

[Uwe Kleine-König]

fbtft_remove_common() is only called with a non-NULL fb_info. (All
callers are in remove callbacks and the matching probe callbacks set
driver data accordingly.) So fbtft_remove_common() always returns zero.
Make it return void instead which makes it easier to see in the callers
that there is no error to handle.

Also the return value of platform and spi remove callbacks is ignored
anyway and not freeing resources in .remove() is a bad idea.

Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft-core.c | 8 +---
 drivers/staging/fbtft/fbtft.h  | 8 +---
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c 
b/drivers/staging/fbtft/fbtft-core.c
index ed992ca605eb..9c9eab1182a6 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1318,23 +1318,17 @@ EXPORT_SYMBOL(fbtft_probe_common);
  * @info: Framebuffer
  *
  * Unregisters and releases the framebuffer
- *
- * Return: 0 if successful, negative if error
  */
-int fbtft_remove_common(struct device *dev, struct fb_info *info)
+void fbtft_remove_common(struct device *dev, struct fb_info *info)
 {
struct fbtft_par *par;
 
-   if (!info)
-   return -EINVAL;
par = info->par;
if (par)
fbtft_par_dbg(DEBUG_DRIVER_INIT_FUNCTIONS, par,
  "%s()\n", __func__);
fbtft_unregister_framebuffer(info);
fbtft_framebuffer_release(info);
-
-   return 0;
 }
 EXPORT_SYMBOL(fbtft_remove_common);
 
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 76f8c090a837..6869f3603b0e 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -252,7 +252,7 @@ void fbtft_unregister_backlight(struct fbtft_par *par);
 int fbtft_init_display(struct fbtft_par *par);
 int fbtft_probe_common(struct fbtft_display *display, struct spi_device *sdev,
   struct platform_device *pdev);
-int fbtft_remove_common(struct device *dev, struct fb_info *info);
+void fbtft_remove_common(struct device *dev, struct fb_info *info);
 
 /* fbtft-io.c */
 int fbtft_write_spi(struct fbtft_par *par, void *buf, size_t len);
@@ -283,7 +283,8 @@ static int fbtft_driver_remove_spi(struct spi_device *spi)  
   \
 {  \
struct fb_info *info = spi_get_drvdata(spi);   \
   \
-   return fbtft_remove_common(&spi->dev, info);   \
+   fbtft_remove_common(&spi->dev, info);  \
+   return 0;  \
 }  \
   \
 static int fbtft_driver_probe_pdev(struct platform_device *pdev)   \
@@ -295,7 +296,8 @@ static int fbtft_driver_remove_pdev(struct platform_device 
*pdev)  \
 {  \
struct fb_info *info = platform_get_drvdata(pdev); \
   \
-   return fbtft_remove_common(&pdev->dev, info);  \
+   fbtft_remove_common(&pdev->dev, info); \
+   return 0;  \
 }  \
   \
 static const struct of_device_id dt_ids[] = {  \
-- 
2.30.2

[PATCH v2 00/20] Make some spi device drivers return zero in .remove()

[Uwe Kleine-König]

Hello,

this is v2 of my quest to make spi remove callbacks return void. Today
they return an int, but the only result of returning a non-zero value is
a warning message. So it's a bad idea to return an error code in the
expectation that not freeing some resources is ok then. The same holds
true for i2c and platform devices which benefit en passant for a few
drivers.

The patches in this series address some of the spi drivers that might
return non-zero and adapt them accordingly to return zero instead. For
most drivers it's just about not hiding the fact that they already
return zero.

Given that there are quite some more patches of this type to create
before I can change the spi remove callback, I suggest the respective
subsystem maintainers pick up these patches. There are no
interdependencies in this series.

Compared to (implicit) v1

 - I fixed a few compiler issues (this series it build tested with an
   allmoddefconfig on arm64, m68k, powerpc, riscv, s390, sparc64 and
   x86_64).
 - A few new patches (2x gpio, 2x misc, 4x mtd)
 - One patch already landed in next, this one I dropped. The drm/panel
   patch as claimed to applied, too, but not yet in next. It's included
   here, but I assume I was just too impatient and this one should be
   ignored.

Full range-diff below.

Best regards
Uwe

Uwe Kleine-König (20):
  drm/panel: s6e63m0: Make s6e63m0_remove() return void
  gpio: max730x: Make __max730x_remove() return void
  gpio: mc33880: Drop if with an always false condition
  hwmon: max31722: Warn about failure to put device in stand-by in
.remove()
  input: adxl34xx: Make adxl34x_remove() return void
  input: touchscreen: tsc200x: Make tsc200x_remove() return void
  media: cxd2880: Eliminate dead code
  mfd: mc13xxx: Make mc13xxx_common_exit() return void
  mfd: stmpe: Make stmpe_remove() return void
  mfd: tps65912: Make tps65912_device_exit() return void
  misc: ad525x_dpot: Make ad_dpot_remove() return void
  misc: lis3lv02d: Make lis3lv02d_remove_fs() return void
  mtd: dataflash: Warn about failure to unregister mtd device
  mtd: mchp23k256: Warn about failure to unregister mtd device
  mtd: mchp48l640: Warn about failure to unregister mtd device
  mtd: sst25l: Warn about failure to unregister mtd device
  serial: max310x: Make max310x_remove() return void
  serial: sc16is7xx: Make sc16is7xx_remove() return void
  staging: fbtft: Make fbtft_remove_common() return void
  tpm: st33zp24: Make st33zp24_remove() return void

 drivers/char/tpm/st33zp24/i2c.c   |  5 +
 drivers/char/tpm/st33zp24/spi.c   |  5 +
 drivers/char/tpm/st33zp24/st33zp24.c  |  3 +--
 drivers/char/tpm/st33zp24/st33zp24.h  |  2 +-
 drivers/gpio/gpio-max7300.c   |  4 +++-
 drivers/gpio/gpio-max7301.c   |  4 +++-
 drivers/gpio/gpio-max730x.c   |  6 +-
 drivers/gpio/gpio-mc33880.c   |  2 --
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c |  3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c |  3 ++-
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.h |  2 +-
 drivers/hwmon/max31722.c  |  8 +++-
 drivers/input/misc/adxl34x-i2c.c  |  4 +++-
 drivers/input/misc/adxl34x-spi.c  |  4 +++-
 drivers/input/misc/adxl34x.c  |  4 +---
 drivers/input/misc/adxl34x.h  |  2 +-
 drivers/input/touchscreen/tsc2004.c   |  4 +++-
 drivers/input/touchscreen/tsc2005.c   |  4 +++-
 drivers/input/touchscreen/tsc200x-core.c  |  4 +---
 drivers/input/touchscreen/tsc200x-core.h  |  2 +-
 drivers/media/spi/cxd2880-spi.c   | 13 +
 drivers/mfd/mc13xxx-core.c|  4 +---
 drivers/mfd/mc13xxx-i2c.c |  3 ++-
 drivers/mfd/mc13xxx-spi.c |  3 ++-
 drivers/mfd/mc13xxx.h |  2 +-
 drivers/mfd/stmpe-i2c.c   |  4 +++-
 drivers/mfd/stmpe-spi.c   |  4 +++-
 drivers/mfd/stmpe.c   |  4 +---
 drivers/mfd/stmpe.h   |  2 +-
 drivers/mfd/tps65912-core.c   |  4 +---
 drivers/mfd/tps65912-i2c.c|  4 +++-
 drivers/mfd/tps65912-spi.c|  4 +++-
 drivers/misc/ad525x_dpot-i2c.c|  3 ++-
 drivers/misc/ad525x_dpot-spi.c|  3 ++-
 drivers/misc/ad525x_dpot.c|  4 +---
 drivers/misc/ad525x_dpot.h|  2 +-
 drivers/misc/lis3lv02d/lis3lv02d.c|  3 +--
 drivers/misc/lis3lv02d/lis3lv02d.h|  2 +-
 drivers/misc/lis3lv02d/lis3lv02d_spi.c|  4 +++-
 drivers/mtd/devices/mchp23k256.c  |  9 -
 driv

Re: [PATCH v6 3/3] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

T_SCALE + 1)

For my understanding: T_pwm = period length = 1 / PWM_FREQ, right? Maybe
it's a good idea to state this more explicitly?

> +  * In order to keep BACKLIGHT_SCALE within its 16 bits,
> +  * PWM_PRE_DIV must be:
> +  *
> +  * T_pwm * REFCLK_FREQ
> +  *   PWM_PRE_DIV >= -
> +  *   BACKLIGHT_SCALE_MAX + 1
> +  *
> +  * To simplify the search and to favour higher resolution of
> +  * the duty cycle over accuracy of the period, the lowest
> +  * possible PWM_PRE_DIV is used. Finally the scale is
> +  * calculated as:
> +  *
> +  *  T_pwm * REFCLK_FREQ
> +  *   BACKLIGHT_SCALE = -- - 1
> +  *  PWM_PRE_DIV
> +  *
> +  * Here T_pwm is represented in seconds, so appropriate scaling
> +  * to nanoseconds is necessary.
> +  */
> +
> + /* Minimum T_pwm is 1 / REFCLK_FREQ */
> + if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /*
> +  * Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ
> +  * Limit period to this to avoid overflows
> +  */
> + period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1),
> +  pdata->pwm_refclk_freq);
> + if (period > period_max)

period is uninitialized here. This must be

if (state->period > period_max)

. Alternatively to the if you could use

period = min(state->period, period_max);


Apart from this I'm happy with your patch set now.

> + period = period_max;
> + else
> + period = state->period;
> +
> + pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
> +  (u64)NSEC_PER_SEC * 
> (BACKLIGHT_SCALE_MAX + 1));
> + scale = div64_u64(period * pdata->pwm_refclk_freq, 
> (u64)NSEC_PER_SEC * pre_div) - 1;

After thinking a while about this---I think I stumbled about this
calculation already in earlier revisions of this patch set---I think I
now understood it. I never saw something like this before because other
drivers with similar HW conditions would pick:

pre_div = div64_u64(period * pdata->pwm_refclk_freq,
(u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));

and then scale = BACKLIGHT_SCALE_MAX. This latter approach weights high
resolution of duty_cycle still higher over period exactness than your
approach. For me both approaches are fine.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] pwm: Introduce single-PWM of_xlate function

[Uwe Kleine-König]

On Wed, Sep 29, 2021 at 10:05:55PM -0500, Bjorn Andersson wrote:
> The existing pxa driver and the upcoming addition of PWM support in the
> TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
> thereby a need for a of_xlate function with the period as its single
> argument.
> 
> Introduce a common helper function in the core that can be used as
> of_xlate by such drivers and migrate the pxa driver to use this.
> 
> Signed-off-by: Bjorn Andersson 

I'm OK with this patch, in the long run I'd like to share more code with
of_pwm_xlate_with_flags, but this shouldn't be a stopper here.

Acked-by: Uwe Kleine-König 

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v5 1/2] pwm: Introduce single-PWM of_xlate function

[Uwe Kleine-König]

On Thu, Sep 23, 2021 at 09:12:24PM -0500, Bjorn Andersson wrote:
> The existing pxa driver and the upcoming addition of PWM support in the
> TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
> thereby a need for a of_xlate function with the period as its single
> argument.
> 
> Introduce a common helper function in the core that can be used as
> of_xlate by such drivers and migrate the pxa driver to use this.
> 
> Signed-off-by: Bjorn Andersson 
> ---
> 
> Changes since v4:
> - None
> 
>  drivers/pwm/core.c| 26 ++
>  drivers/pwm/pwm-pxa.c | 16 +---
>  include/linux/pwm.h   |  2 ++
>  3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4527f09a5c50..2c6b155002a2 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const 
> struct of_phandle_args *args)
>  }
>  EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
>  
> +struct pwm_device *
> +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> +{
> + struct pwm_device *pwm;
> +
> + if (pc->of_pwm_n_cells < 1)
> + return ERR_PTR(-EINVAL);
> +
> + /* validate that one cell is specified, optionally with flags */
> + if (args->args_count != 1 && args->args_count != 2)
> + return ERR_PTR(-EINVAL);
> +
> + pwm = pwm_request_from_chip(pc, 0, NULL);
> + if (IS_ERR(pwm))
> + return pwm;
> +
> + pwm->args.period = args->args[0];
> + pwm->args.polarity = PWM_POLARITY_NORMAL;
> +
> + if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)
> + pwm->args.polarity = PWM_POLARITY_INVERSED;

of_pwm_xlate_with_flags is a bit more complicated. Translating
accordingly this would yield:

if (pc->of_pwm_n_cells >= 2) {
if (args->args_count > 1 && args->args[1] & 
PWM_POLARITY_INVERTED)
pwm->args.polarity = PWM_POLARITY_INVERSED;
}

Given that pc->of_pwm_n_cells isn't used when a phandle is parsed (in
of_pwm_get()) I think your variant is fine.

So I think technically the patch is good, for me the question is if we
want to make new drivers of_pwm_xlate_with_flags for consistency even
though this would mean that the first argument has to be 0 for all
phandles. Thierry? Lee?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v5 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

> + dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> + goto out;
> + }
> +
> + pdata->pwm_enabled = state->enabled;
> +out:
> +
> + if (!pdata->pwm_enabled)
> + pm_runtime_put_sync(pdata->dev);
> +
> + return ret;
> +}
> +
> +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device 
> *pwm,
> + struct pwm_state *state)
> +{
> + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> + unsigned int pwm_en_inv;
> + unsigned int pre_div;
> + u16 backlight;
> + u16 scale;
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> + if (ret)
> + return;
> +
> + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> + if (ret)
> + return;
> +
> + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> + if (ret)
> + return;
> +
> + ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> + if (ret)
> + return;
> +
> + state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> + if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 
> 1),
> +  pdata->pwm_refclk_freq);
> + state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * 
> backlight,
> +  pdata->pwm_refclk_freq);

What happens if scale + 1 < backlight?

> +}

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v5 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

535 + 1) / REFCLK_FREQ
> > > +  * Limit period to this to avoid overflows
> > > +  */
> > > + period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), 
> > > pdata->pwm_refclk_freq);
> > > + if (period > period_max)
> > > + period = period_max;
> > > + else
> > > + period = state->period;
> > > +
> > > + pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
> > > +  (u64)NSEC_PER_SEC * 
> > > (BACKLIGHT_SCALE_MAX + 1));
> > > + scale = div64_u64(period * pdata->pwm_refclk_freq, 
> > > (u64)NSEC_PER_SEC * pre_div) - 1;
> > 
> > You're loosing precision here as you divide by the result of a division.
> 
> As described above, I'm trying to find suitable values for PRE_DIV and
> BACKLIGHT_SCALE in:
> 
>   T_pwm * refclk_freq
>  - = PRE_DIV * (BACKLIGHT_SCALE + 1)
>  NSEC_PER_SEC
> 
> BACKLIGHT_SCALE must fit in 16 bits and in order to be useful for
> driving the backlight control be large enough that the resolution
> (BACKLIGTH = [0..BACKLIGHT_SCALE]) covers whatever resolution
> pwm-backlight might expose.
> 
> For the intended use case this seems pretty good, but I presume you
> could pick better values to get closer to the requested period.
> 
> Do you have a better suggestion for how to pick PRE_DIV and
> BACKLIGHT_SCALE?

My motivation is limited to spend brain cycles here if we're not sure
we're using the right formula. Maybe my concern is wrong given that
pre_div isn't only an interim result but an actual register value. I
will have to think about that when I'm not tired.

> > > +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device 
> > > *pwm,
> > > +     struct pwm_state *state)
> > > +{
> > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > + unsigned int pwm_en_inv;
> > > + unsigned int pre_div;
> > > + u16 backlight;
> > > + u16 scale;
> > > + int ret;
> > > +
> > > + ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> > > + if (ret)
> > > + return;
> > > +
> > > + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> > > + if (ret)
> > > + return;
> > > +
> > > + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> > > + if (ret)
> > > + return;
> > > +
> > > + ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> > > + if (ret)
> > > + return;
> > > +
> > > + state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> > > + if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 
> > > 1),
> > > +  pdata->pwm_refclk_freq);
> > > + state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * 
> > > backlight,
> > > +  pdata->pwm_refclk_freq);
> > 
> > What happens if scale + 1 < backlight?
> 
> When we write the backlight register above, we cap it at scale, so for
> the typical case that shouldn't happen.

.get_state() is called before the first .apply(), to the values to
interpret here originate from the bootloader which might write strange
settings that the linux driver would never do. So being prudent here is
IMHO a good idea.

It's a shame that .get_state() cannot return an error.

> So I guess the one case when this could happen is if we get_state()
> before pwm_apply(), when someone else has written broken values

ack.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH 1/2] staging: fbtft: Fix error path in fbtft_driver_module_init()

[Uwe Kleine-König]

If registering the platform driver fails, the function must not return
without undoing the spi driver registration first.

Fixes: c296d5f9957c ("staging: fbtft: core support")
Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 4cdec34e23d2..55677efc0138 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -334,7 +334,10 @@ static int __init fbtft_driver_module_init(void)   
\
ret = spi_register_driver(&fbtft_driver_spi_driver);   \
if (ret < 0)   \
return ret;\
-   return platform_driver_register(&fbtft_driver_platform_driver);\
+   ret = platform_driver_register(&fbtft_driver_platform_driver); \
+   if (ret < 0)   \
+   spi_unregister_driver(&fbtft_driver_spi_driver);   \
+   return ret;\
 }  \
   \
 static void __exit fbtft_driver_module_exit(void)  \

base-commit: 0c947b893d69231a9add855939da7c66237ab44f
-- 
2.34.1

[PATCH 2/2] staging: fbtft: Deduplicate driver registration macros

[Uwe Kleine-König]

The two macros FBTFT_REGISTER_DRIVER and FBTFT_REGISTER_SPI_DRIVER
contain quite some duplication: Both define an spi driver and an of device
table and the differences are quite subtle.

So create two new macros and use both twice.

Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft.h | 93 ++-
 1 file changed, 36 insertions(+), 57 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 55677efc0138..6a7545b5bcd2 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -272,21 +272,40 @@ void fbtft_write_reg8_bus9(struct fbtft_par *par, int 
len, ...);
 void fbtft_write_reg16_bus8(struct fbtft_par *par, int len, ...);
 void fbtft_write_reg16_bus16(struct fbtft_par *par, int len, ...);
 
+#define FBTFT_DT_TABLE(_compatible)
\
+static const struct of_device_id dt_ids[] = {  
\
+   { .compatible = _compatible },  
\
+   {}, 
\
+}; 
\
+MODULE_DEVICE_TABLE(of, dt_ids);
+
+#define FBTFT_SPI_DRIVER(_name, _compatible, _display, _spi_ids)   
\
+   
\
+static int fbtft_driver_probe_spi(struct spi_device *spi)  
\
+{  
\
+   return fbtft_probe_common(_display, spi, NULL); 
\
+}  
\
+   
\
+static int fbtft_driver_remove_spi(struct spi_device *spi) 
\
+{  
\
+   struct fb_info *info = spi_get_drvdata(spi);
\
+   
\
+   fbtft_remove_common(&spi->dev, info);   
\
+   return 0;   
\
+}  
\
+   
\
+static struct spi_driver fbtft_driver_spi_driver = {   
\
+   .driver = { 
\
+   .name = _name,  
\
+   .of_match_table = dt_ids,   
\
+   },  
\
+   .id_table = _spi_ids,   
\
+   .probe = fbtft_driver_probe_spi,
\
+   .remove = fbtft_driver_remove_spi,  
\
+};
+
 #define FBTFT_REGISTER_DRIVER(_name, _compatible, _display)\
   \
-static int fbtft_driver_probe_spi(struct spi_device *spi)  \
-{  \
-   return fbtft_probe_common(_display, spi, NULL);\
-}  \
-  \
-static int fbtft_driver_remove_spi(struct spi_device *spi) \
-{  \
-   struct fb_info *info = spi_get_drvdata(spi);   \
-  \
-   fbtft_remove_common(&spi->dev, info);  \
-   return 0;  \
-}  \
-  \
 static int fbtft_driver_probe_pdev(struct platform_device *pdev)   \
 {  \
return fbtft_probe_common(_display, NULL, pdev);   \
@@ -300,22 +319,9 @@ static int fbtft_driver_remove_pdev(struct platform_device 
*pdev)  \
return 0;  \
 }  \
   \
-static const struct of_device_id dt_ids[] = {  \
-  

[PATCH 1/5] staging: fbtft: Fix error path in fbtft_driver_module_init()

[Uwe Kleine-König]

If registering the platform driver fails, the function must not return
without undoing the spi driver registration first.

Fixes: c296d5f9957c ("staging: fbtft: core support")
Link: 
https://lore.kernel.org/r/20220118181338.207943-1-u.kleine-koe...@pengutronix.de
Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 4cdec34e23d2..55677efc0138 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -334,7 +334,10 @@ static int __init fbtft_driver_module_init(void)   
\
ret = spi_register_driver(&fbtft_driver_spi_driver);   \
if (ret < 0)   \
return ret;\
-   return platform_driver_register(&fbtft_driver_platform_driver);\
+   ret = platform_driver_register(&fbtft_driver_platform_driver); \
+   if (ret < 0)   \
+   spi_unregister_driver(&fbtft_driver_spi_driver);   \
+   return ret;\
 }  \
   \
 static void __exit fbtft_driver_module_exit(void)  \
-- 
2.34.1

[PATCH 2/5] staging: fbtft: Deduplicate driver registration macros

[Uwe Kleine-König]

The two macros FBTFT_REGISTER_DRIVER and FBTFT_REGISTER_SPI_DRIVER
contain quite some duplication: Both define an spi driver and an of device
table and the differences are quite subtle.

So create two new macros and use both twice.

Link: 
https://lore.kernel.org/r/20220118181338.207943-2-u.kleine-koe...@pengutronix.de
Signed-off-by: Uwe Kleine-König 
---
 drivers/staging/fbtft/fbtft.h | 93 ++-
 1 file changed, 36 insertions(+), 57 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 55677efc0138..6a7545b5bcd2 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -272,21 +272,40 @@ void fbtft_write_reg8_bus9(struct fbtft_par *par, int 
len, ...);
 void fbtft_write_reg16_bus8(struct fbtft_par *par, int len, ...);
 void fbtft_write_reg16_bus16(struct fbtft_par *par, int len, ...);
 
+#define FBTFT_DT_TABLE(_compatible)
\
+static const struct of_device_id dt_ids[] = {  
\
+   { .compatible = _compatible },  
\
+   {}, 
\
+}; 
\
+MODULE_DEVICE_TABLE(of, dt_ids);
+
+#define FBTFT_SPI_DRIVER(_name, _compatible, _display, _spi_ids)   
\
+   
\
+static int fbtft_driver_probe_spi(struct spi_device *spi)  
\
+{  
\
+   return fbtft_probe_common(_display, spi, NULL); 
\
+}  
\
+   
\
+static int fbtft_driver_remove_spi(struct spi_device *spi) 
\
+{  
\
+   struct fb_info *info = spi_get_drvdata(spi);
\
+   
\
+   fbtft_remove_common(&spi->dev, info);   
\
+   return 0;   
\
+}  
\
+   
\
+static struct spi_driver fbtft_driver_spi_driver = {   
\
+   .driver = { 
\
+   .name = _name,  
\
+   .of_match_table = dt_ids,   
\
+   },  
\
+   .id_table = _spi_ids,   
\
+   .probe = fbtft_driver_probe_spi,
\
+   .remove = fbtft_driver_remove_spi,  
\
+};
+
 #define FBTFT_REGISTER_DRIVER(_name, _compatible, _display)\
   \
-static int fbtft_driver_probe_spi(struct spi_device *spi)  \
-{  \
-   return fbtft_probe_common(_display, spi, NULL);\
-}  \
-  \
-static int fbtft_driver_remove_spi(struct spi_device *spi) \
-{  \
-   struct fb_info *info = spi_get_drvdata(spi);   \
-  \
-   fbtft_remove_common(&spi->dev, info);  \
-   return 0;  \
-}  \
-  \
 static int fbtft_driver_probe_pdev(struct platform_device *pdev)   \
 {  \
return fbtft_probe_common(_display, NULL, pdev);   \
@@ -300,22 +319,9 @@ static int fbtft_driver_remove_pdev(struct platform_device 
*pdev)  \
return 0;  \
 }  \
  

[PATCH 0/5] spi: make remove callback a void function

[Uwe Kleine-König]

Hello,

this series goal is to change the spi remove callback's return value to void.
After numerous patches nearly all drivers already return 0 unconditionally.
The four first patches in this series convert the remaining three drivers to
return 0, the final patch changes the remove prototype and converts all
implementers.

The driver core doesn't support error handling on remove, the spi core issues
only a very generic warning when a remove callback returns an error. If there
is really the need for a function call that can fail, the driver can issue a
more helpful error message. I didn't find a single driver where returning
an error code and error handling in the spi core would have been helpful.

So change the prototype of the remove function to make it obvious for driver
authors that there is no error handling in the spi core.

The four preparatory patches were already send out, but not yet taken into
next.

Assuming Mark is fine with this change I'd like to have this go in during the
next merge window. I guess we need a tag that can be pulled into trees that add
a new driver in the next cycle. I can provide such a tag, but I'm open to
alternatives.

The patch set survived an allmodconfig build on various archs (arm64 m68k
powerpc riscv s390 sparc64 x86_64) after the following two commits from
next-20220121 were added to fix an unrelated build problem:

be973481daaa ("pinctrl: thunderbay: rework loops looking for groups 
names")
8687999e47d4 ("pinctrl: thunderbay: comment process of building 
functions a bit")

Best regards
Uwe

Uwe Kleine-König (5):
  staging: fbtft: Fix error path in fbtft_driver_module_init()
  staging: fbtft: Deduplicate driver registration macros
  tpm: st33zp24: Make st33zp24_remove() a void function
  platform/chrome: cros_ec: Make cros_ec_unregister() return void
  spi: make remove callback a void function

 drivers/bus/moxtet.c  |  4 +-
 drivers/char/tpm/st33zp24/i2c.c   |  5 +-
 drivers/char/tpm/st33zp24/spi.c   |  9 +-
 drivers/char/tpm/st33zp24/st33zp24.c  |  3 +-
 drivers/char/tpm/st33zp24/st33zp24.h  |  2 +-
 drivers/char/tpm/tpm_tis_spi_main.c   |  3 +-
 drivers/clk/clk-lmk04832.c|  4 +-
 drivers/gpio/gpio-74x164.c|  4 +-
 drivers/gpio/gpio-max3191x.c  |  4 +-
 drivers/gpio/gpio-max7301.c   |  4 +-
 drivers/gpio/gpio-mc33880.c   |  4 +-
 drivers/gpio/gpio-pisosr.c|  4 +-
 drivers/gpu/drm/panel/panel-abt-y030xx067a.c  |  4 +-
 drivers/gpu/drm/panel/panel-ilitek-ili9322.c  |  4 +-
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c  |  3 +-
 drivers/gpu/drm/panel/panel-innolux-ej030na.c |  4 +-
 drivers/gpu/drm/panel/panel-lg-lb035q02.c |  4 +-
 drivers/gpu/drm/panel/panel-lg-lg4573.c   |  4 +-
 drivers/gpu/drm/panel/panel-nec-nl8048hl11.c  |  4 +-
 drivers/gpu/drm/panel/panel-novatek-nt39016.c |  4 +-
 drivers/gpu/drm/panel/panel-samsung-db7430.c  |  3 +-
 drivers/gpu/drm/panel/panel-samsung-ld9040.c  |  4 +-
 drivers/gpu/drm/panel/panel-samsung-s6d27a1.c |  3 +-
 .../gpu/drm/panel/panel-samsung-s6e63m0-spi.c |  3 +-
 .../gpu/drm/panel/panel-sitronix-st7789v.c|  4 +-
 drivers/gpu/drm/panel/panel-sony-acx565akm.c  |  4 +-
 drivers/gpu/drm/panel/panel-tpo-td028ttec1.c  |  4 +-
 drivers/gpu/drm/panel/panel-tpo-td043mtea1.c  |  4 +-
 drivers/gpu/drm/panel/panel-tpo-tpg110.c  |  3 +-
 .../gpu/drm/panel/panel-widechips-ws2401.c|  3 +-
 drivers/gpu/drm/tiny/hx8357d.c|  4 +-
 drivers/gpu/drm/tiny/ili9163.c|  4 +-
 drivers/gpu/drm/tiny/ili9225.c|  4 +-
 drivers/gpu/drm/tiny/ili9341.c|  4 +-
 drivers/gpu/drm/tiny/ili9486.c|  4 +-
 drivers/gpu/drm/tiny/mi0283qt.c   |  4 +-
 drivers/gpu/drm/tiny/repaper.c|  4 +-
 drivers/gpu/drm/tiny/st7586.c |  4 +-
 drivers/gpu/drm/tiny/st7735r.c|  4 +-
 drivers/hwmon/adcxx.c |  4 +-
 drivers/hwmon/adt7310.c   |  3 +-
 drivers/hwmon/max.c   |  3 +-
 drivers/hwmon/max31722.c  |  4 +-
 drivers/iio/accel/bma400_spi.c|  4 +-
 drivers/iio/accel/bmc150-accel-spi.c  |  4 +-
 drivers/iio/accel/bmi088-accel-spi.c  |  4 +-
 drivers/iio/accel/kxsd9-spi.c |  4 +-
 drivers/iio/accel/mma7455_spi.c   |  4 +-
 drivers/iio/accel/sca3000.c   |  4 +-
 drivers/iio/adc/ad7266.c  |  4 +-
 drivers/iio/adc/ltc2496.c |  4 +-
 drivers/iio/adc/mcp320x.c |  4 +-
 drivers/iio/adc/mcp3911.c |  4 +-
 drivers/iio/adc/ti-adc12138.c |  4 +-
 drivers/iio/adc/ti-ads7950.c  |  4 +-
 drivers/iio/adc/ti-ads8688.c  

[PATCH 5/5] spi: make remove callback a void function

[Uwe Kleine-König]

The value returned by an spi driver's remove function is mostly ignored.
(Only an error message is printed if the value is non-zero that the
error is ignored.)

So change the prototype of the remove function to return no value. This
way driver authors are not tempted to assume that passing an error to
the upper layer is a good idea. All drivers are adapted accordingly.
There is no intended change of behaviour, all callbacks were prepared to
return 0 before.

Signed-off-by: Uwe Kleine-König 
---
 drivers/bus/moxtet.c  |  4 +---
 drivers/char/tpm/st33zp24/spi.c   |  4 +---
 drivers/char/tpm/tpm_tis_spi_main.c   |  3 +--
 drivers/clk/clk-lmk04832.c|  4 +---
 drivers/gpio/gpio-74x164.c|  4 +---
 drivers/gpio/gpio-max3191x.c  |  4 +---
 drivers/gpio/gpio-max7301.c   |  4 +---
 drivers/gpio/gpio-mc33880.c   |  4 +---
 drivers/gpio/gpio-pisosr.c|  4 +---
 drivers/gpu/drm/panel/panel-abt-y030xx067a.c  |  4 +---
 drivers/gpu/drm/panel/panel-ilitek-ili9322.c  |  4 +---
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c  |  3 +--
 drivers/gpu/drm/panel/panel-innolux-ej030na.c |  4 +---
 drivers/gpu/drm/panel/panel-lg-lb035q02.c |  4 +---
 drivers/gpu/drm/panel/panel-lg-lg4573.c   |  4 +---
 drivers/gpu/drm/panel/panel-nec-nl8048hl11.c  |  4 +---
 drivers/gpu/drm/panel/panel-novatek-nt39016.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-db7430.c  |  3 +--
 drivers/gpu/drm/panel/panel-samsung-ld9040.c  |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6d27a1.c |  3 +--
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c |  3 +--
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c|  4 +---
 drivers/gpu/drm/panel/panel-sony-acx565akm.c  |  4 +---
 drivers/gpu/drm/panel/panel-tpo-td028ttec1.c  |  4 +---
 drivers/gpu/drm/panel/panel-tpo-td043mtea1.c  |  4 +---
 drivers/gpu/drm/panel/panel-tpo-tpg110.c  |  3 +--
 drivers/gpu/drm/panel/panel-widechips-ws2401.c|  3 +--
 drivers/gpu/drm/tiny/hx8357d.c|  4 +---
 drivers/gpu/drm/tiny/ili9163.c|  4 +---
 drivers/gpu/drm/tiny/ili9225.c|  4 +---
 drivers/gpu/drm/tiny/ili9341.c|  4 +---
 drivers/gpu/drm/tiny/ili9486.c|  4 +---
 drivers/gpu/drm/tiny/mi0283qt.c   |  4 +---
 drivers/gpu/drm/tiny/repaper.c|  4 +---
 drivers/gpu/drm/tiny/st7586.c |  4 +---
 drivers/gpu/drm/tiny/st7735r.c|  4 +---
 drivers/hwmon/adcxx.c |  4 +---
 drivers/hwmon/adt7310.c   |  3 +--
 drivers/hwmon/max.c   |  3 +--
 drivers/hwmon/max31722.c  |  4 +---
 drivers/iio/accel/bma400_spi.c|  4 +---
 drivers/iio/accel/bmc150-accel-spi.c  |  4 +---
 drivers/iio/accel/bmi088-accel-spi.c  |  4 +---
 drivers/iio/accel/kxsd9-spi.c |  4 +---
 drivers/iio/accel/mma7455_spi.c   |  4 +---
 drivers/iio/accel/sca3000.c   |  4 +---
 drivers/iio/adc/ad7266.c  |  4 +---
 drivers/iio/adc/ltc2496.c |  4 +---
 drivers/iio/adc/mcp320x.c |  4 +---
 drivers/iio/adc/mcp3911.c |  4 +---
 drivers/iio/adc/ti-adc12138.c |  4 +---
 drivers/iio/adc/ti-ads7950.c  |  4 +---
 drivers/iio/adc/ti-ads8688.c  |  4 +---
 drivers/iio/adc/ti-tlc4541.c  |  4 +---
 drivers/iio/amplifiers/ad8366.c   |  4 +---
 drivers/iio/common/ssp_sensors/ssp_dev.c  |  4 +---
 drivers/iio/dac/ad5360.c  |  4 +---
 drivers/iio/dac/ad5380.c  |  4 +---
 drivers/iio/dac/ad5446.c  |  4 +---
 drivers/iio/dac/ad5449.c  |  4 +---
 drivers/iio/dac/ad5504.c  |  4 +---
 drivers/iio/dac/ad5592r.c |  4 +---
 drivers/iio/dac/ad5624r_spi.c |  4 +---
 drivers/iio/dac/ad5686-spi.c  |  4 +---
 drivers/iio/dac/ad5761.c  |  4 +---
 drivers/iio/dac/ad5764.c  |  4 +---
 drivers/iio/dac/ad5791.c  |  4 +---
 drivers/iio/dac/ad8801.c  |  4 +---
 drivers/iio/dac/ltc1660.c |  4 +---
 drivers/ii

Re: [PATCH 5/5] spi: make remove callback a void function

[Uwe Kleine-König]

[Dropped a few people from Cc that are not reachable (Harry Morris,
Charles-Antoine Couret, Marco Felsch)]

On Tue, Jan 25, 2022 at 09:47:59AM +, Jonathan Cameron wrote:
> On Sun, 23 Jan 2022 18:52:01 +0100
> Uwe Kleine-König  wrote:
> 
> > The value returned by an spi driver's remove function is mostly ignored.
> > (Only an error message is printed if the value is non-zero that the
> > error is ignored.)
> > 
> > So change the prototype of the remove function to return no value. This
> > way driver authors are not tempted to assume that passing an error to
> > the upper layer is a good idea. All drivers are adapted accordingly.
> > There is no intended change of behaviour, all callbacks were prepared to
> > return 0 before.
> > 
> > Signed-off-by: Uwe Kleine-König 
> 
> For iio drivers.
> 
> Acked-by: Jonathan Cameron 
> 
> As you mention in the cover letter we'll be wanting an immutable
> branch somewhere to pull into subsystem trees.
> 
> Soon is good if possible as otherwise we'll end up with a bunch of merge
> conflicts getting resolved in next.

Yes, I considered creating a tag to pull already when sending out this
series, but I guessed delaying that a little bit to give people the
opportunity to ack would be a good idea.

@broonie: Do you think this change is a good idea? Would you require
some more acks for the preparatory patches? I had hoped to get Acks from
the corresponding maintainers, maybe they are busy and missed this
series as I put them on Cc: only. I promoted them to To: in this mail.

Or is it too ambitious to get this in during the next merge window?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 2/5] staging: fbtft: Deduplicate driver registration macros

[Uwe Kleine-König]

Hello Greg,

On Sun, Jan 23, 2022 at 06:51:58PM +0100, Uwe Kleine-König wrote:
> The two macros FBTFT_REGISTER_DRIVER and FBTFT_REGISTER_SPI_DRIVER
> contain quite some duplication: Both define an spi driver and an of device
> table and the differences are quite subtle.
> 
> So create two new macros and use both twice.
> 
> Link: 
> https://lore.kernel.org/r/20220118181338.207943-2-u.kleine-koe...@pengutronix.de
> Signed-off-by: Uwe Kleine-König 

You picked this patch into your staging-next branch, I guess from the
original submission. Not sure how Mark wants to continue with the series
from this thread, but at least my plan was that he will create an
immutable branch on top of 5.17-rc2 (assuming 5.17-rc2 will contain
"staging: fbtft: Fix error path in fbtft_driver_module_init()") with the
remaining 4 patches in this series.

In a private mail you agreed to this procedure, but this didn't stop you
taking this patch?! What is your plan here? The obvious (to me) options
are:

 - Delay this series until after the next merge window.
 - You back out this patch from staging-next and ack here for Mark to
   apply it to an immutable branch.
 - You keep this patch in staging-next and still ack here for Mark to
   apply it to an immutable branch. Then the patch would be included
   twice.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH 2/2] drm/imx/lcdc: Implement DRM driver for imx21

[Uwe Kleine-König]

From: Marian Cichy 

Add support for the LCD Controller found on i.MX21, i.MX25 and i.MX27

Note there is already a fb driver for this hardware in the tree that is
supposed to be superseded by this one.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/imx/Kconfig |   9 +
 drivers/gpu/drm/imx/Makefile|   2 +
 drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c | 631 
 3 files changed, 642 insertions(+)
 create mode 100644 drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index bb9738c7c825..62b24ea1bbe0 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -42,3 +42,12 @@ config DRM_IMX_HDMI
  Choose this if you want to use HDMI on i.MX6.
 
 source "drivers/gpu/drm/imx/dcss/Kconfig"
+
+config DRM_IMX21_LCDC
+   tristate "Freescale i.MX LCDC displays"
+   depends on DRM && (ARCH_MXC || ARCH_MULTIPLATFORM || COMPILE_TEST)
+   select DRM_GEM_CMA_HELPER
+   select DRM_KMS_HELPER
+   help
+ This driver is for the Liquid Crystal Display Controller (LCDC) found
+ on Freescale i.MX21, i.MX25 and i.MX27.
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index b644deffe948..25685230f4f7 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
 
 obj-$(CONFIG_DRM_IMX_HDMI) += dw_hdmi-imx.o
 obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
+
+obj-$(CONFIG_DRM_IMX21_LCDC) += imx21-lcdc/
diff --git a/drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c 
b/drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c
new file mode 100644
index ..37e75f728293
--- /dev/null
+++ b/drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c
@@ -0,0 +1,631 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2020 Marian Cichy 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* LCDC Screen Start Address Register */
+#define IMX21LCDC_LSSAR0x
+
+/* LCDC Size Register */
+#define IMX21LCDC_LSR  0x0004
+#define   IMX21LCDC_LSR_XMAX   GENMASK(25, 20) /* Screen width (in 
pixels) divided by 16. */
+#define   IMX21LCDC_LSR_YMAX   GENMASK(9, 0)
+
+/* LCDC Virtual Page Width Register */
+#define IMX21LCDC_LVPWR0x0008
+
+/* LCDC Cursor Position Register */
+#define IMX21LCDC_LCPR 0x000c
+#define   IMX21LCDC_LCPR_CCGENMASK(31, 30) /* Cursor Control */
+
+/* LCDC Cursor Width Height and Blink Register*/
+#define IMX21LCDC_LCWHB0x0010
+
+/* LCDC Color Cursor Mapping Register */
+#define IMX21LCDC_LCCMR0x0014
+
+/* LCDC Panel Configuration Register */
+#define IMX21LCDC_LPCR 0x0018
+#define   IMX21LCDC_LPCR_PCD   GENMASK(5, 0)
+#define   IMX21LCDC_LPCR_SHARP BIT(6)
+#define   IMX21LCDC_LPCR_SCLKSEL   BIT(7)
+#define   IMX21LCDC_LPCR_ACD   GENMASK(14, 8)
+#define   IMX21LCDC_LPCR_ACDSELBIT(15)
+#define   IMX21LCDC_LPCR_REV_VSBIT(16)
+#define   IMX21LCDC_LPCR_SWAP_SEL  BIT(17)
+#define   IMX21LCDC_LPCR_END_SEL   BIT(18)
+#define   IMX21LCDC_LPCR_SCLKIDLE  BIT(19)
+#define   IMX21LCDC_LPCR_OEPOL BIT(20)
+#define   IMX21LCDC_LPCR_CLKPOLBIT(21)
+#define   IMX21LCDC_LPCR_LPPOL BIT(22)
+#define   IMX21LCDC_LPCR_FLMPOLBIT(23)
+#define   IMX21LCDC_LPCR_PIXPOLBIT(24)
+#define   IMX21LCDC_LPCR_BPIX  GENMASK(27, 25)
+#define   IMX21LCDC_LPCR_PBSIZ GENMASK(29, 28)
+#define   IMX21LCDC_LPCR_COLOR BIT(30)
+#define   IMX21LCDC_LPCR_TFT   BIT(31)
+
+/* LCDC Horizontal Configuration Register */
+#define IMX21LCDC_LHCR 0x001c
+#define   IMX21LCDC_LHCR_H_WIDTH   GENMASK(31, 26)
+#define   IMX21LCDC_LHCR_H_BPORCH  GENMASK(7, 0)
+#define   IMX21LCDC_LHCR_H_FPORCH  GENMASK(15, 8)
+
+/* LCDC Vertical Configuration Register */
+#define IMX21LCDC_LVCR 0x0020
+#define   IMX21LCDC_LVCR_V_WIDTH   GENMASK(31, 26)
+#define   IMX21LCDC_LVCR_V_BPORCH  GENMASK(7, 0)
+#define   IMX21LCDC_LVCR_V_FPORCH  GENMASK(15, 8)
+
+/* LCDC Panning Offset Register */
+#define IMX21LCDC_LPOR 0x0024
+
+/* LCDC Sharp Configuration Register */
+#define IMX21LCDC_LSCR 0x0028
+
+/* LCDC PWM Contrast Control Register */
+#define IMX21LCDC_LPCCR0x002c
+
+/* LCDC DMA Control Register */
+#define IMX21LCDC_LDCR 0x0030
+
+/* LCDC Refresh Mode Control Register */
+#define IMX21LCDC_LRMCR0x0034
+
+/* LCDC Interrupt Configuration Register */
+#define IMX21LCDC_LICR 0x0038
+
+/* LCDC Interrupt Enable Register */
+#define IMX21LCDC_LIER 0x003c
+#define   IMX21L

[PATCH 1/2] dt-bindings: display: imx: Add fsl,imx21-lcdc docs

[Uwe Kleine-König]

From: Marian Cichy 

This files documents the device tree for the new imx21-lcdc DRM driver.

Signed-off-by: Marian Cichy 
Signed-off-by: Uwe Kleine-König 
---
 .../bindings/display/imx/fsl,imx21-lcdc.yaml  | 79 +++
 1 file changed, 79 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/imx/fsl,imx21-lcdc.yaml

diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx21-lcdc.yaml 
b/Documentation/devicetree/bindings/display/imx/fsl,imx21-lcdc.yaml
new file mode 100644
index ..edf71cfac81c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx21-lcdc.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,imx21-lcdc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX21 LCD Controller
+
+maintainers:
+  - Philipp Zabel 
+
+properties:
+  compatible:
+oneOf:
+  - const: fsl,imx21-lcdc
+  - items:
+  - enum:
+  - fsl,imx25-lcdc
+  - fsl,imx27-lcdc
+  - const: fsl,imx21-lcdc
+
+  reg:
+maxItems: 1
+
+  clocks:
+maxItems: 3
+
+  clock-names:
+items:
+  - const: ipg
+  - const: per
+  - const: ahb
+
+  resets:
+maxItems: 1
+
+  port:
+type: object
+description:
+  "Video port for DPI RGB output."
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+lcdc: lcdc@53fbc000 {
+compatible = "fsl,imx25-lcdc", "fsl,imx21-lcdc";
+reg = <0x53fbc000 0x4000>;
+interrupts = <39>;
+clocks = <&clks 103>, <&clks 66>, <&clks 49>;
+clock-names = "ipg", "ahb", "per";
+
+port {
+ parallel_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ };
+};
+
+};
+
+panel: panel {
+compatible = "edt,etm0700g0dh6";
+power-supply = <&lcd_supply>;
+backlight = <&bl>;
+
+port {
+panel_in: endpoint {
+remote-endpoint = <¶llel_out>;
+};
+};
+};
-- 
2.34.1

[PATCH 0/2] drm/imx/lcdc: drm driver for imx21/25/27

[Uwe Kleine-König]

Hello,

this patchset was created mostly by Marian Cichy, who in the meantime
left Pengutronix. I still kept his name and email address as author, but
note that the email address doesn't reach Marian any more.

There is already a maintainer entry for imx DRM drivers that matches
good enough.

This was tested on an i.MX25 based customer machine.

Best regards
Uwe

Marian Cichy (2):
  dt-bindings: display: imx: Add fsl,imx21-lcdc docs
  drm/imx/lcdc: Implement DRM driver for imx21

 .../bindings/display/imx/fsl,imx21-lcdc.yaml  |  79 +++
 drivers/gpu/drm/imx/Kconfig   |   9 +
 drivers/gpu/drm/imx/Makefile  |   2 +
 drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c   | 631 ++
 4 files changed, 721 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/imx/fsl,imx21-lcdc.yaml
 create mode 100644 drivers/gpu/drm/imx/imx21-lcdc/imx21-lcdc.c

-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: display: imx: Add fsl,imx21-lcdc docs

[Uwe Kleine-König]

Hello Rob,

On Fri, Jan 28, 2022 at 07:04:10AM -0600, Rob Herring wrote:
> On Fri, Jan 28, 2022 at 4:59 AM Uwe Kleine-König
>  wrote:
> >
> > From: Marian Cichy 
> >
> > This files documents the device tree for the new imx21-lcdc DRM driver.
> 
> No, bindings document h/w and the h/w has not changed. We already have
> a binding for the LCDC.

Just to be sure we're talking about the same thing: You're refering to
Documentation/devicetree/bindings/display/imx/fsl,imx-fb.txt, right?

I'm unsure what to do now. Should the two different bindings just be
described in the same file? Should I stick to fsl,imx21-fb even for the
new binding? (The hardware unit is named LCDC, so the name chosen here
is the better one.) Please advise.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 2/2] drm/imx/lcdc: Implement DRM driver for imx21

[Uwe Kleine-König]

On Sat, Jan 29, 2022 at 06:25:53AM +0800, kernel test robot wrote:
> Hi "Uwe,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on shawnguo/for-next]
> [also build test ERROR on robh/for-next pza/reset/next v5.17-rc1 
> next-20220128]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:
> https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/drm-imx-lcdc-drm-driver-for-imx21-25-27/20220128-190002
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git 
> for-next
> config: arc-randconfig-r012-20220128 
> (https://download.01.org/0day-ci/archive/20220129/202201290646.48sngwm1-...@intel.com/config)
> compiler: arc-elf-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # 
> https://github.com/0day-ci/linux/commit/cba99931972f752a7b3105a3697b0cda88fe54d4
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review 
> Uwe-Kleine-K-nig/drm-imx-lcdc-drm-driver-for-imx21-25-27/20220128-190002
> git checkout cba99931972f752a7b3105a3697b0cda88fe54d4
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross 
> O=build_dir ARCH=arc distclean
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
> 
> All errors (new ones prefixed by >>):
> 
> >> scripts/Makefile.clean:15: drivers/gpu/drm/imx/imx21-lcdc/Makefile: No 
> >> such file or directory
> >> make[6]: *** No rule to make target 
> >> 'drivers/gpu/drm/imx/imx21-lcdc/Makefile'.

Argh, yes, I forgot to commit that one. I'll add it in the next round,
but wait a bit for more feedback (mainly by Rob). The Makefile just
contains:

obj-$(CONFIG_DRM_IMX21_LCDC) += imx21-lcdc.o

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v2] backlight: lp855x: Switch to atomic PWM API

[Uwe Kleine-König]

On Wed, Oct 27, 2021 at 03:45:40PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
> 
> Signed-off-by: Maíra Canal 
> Reported-by: kernel test robot 
> ---
> V1 -> V2: Initializing variable and simplyfing conditional loop
> ---
>  drivers/video/backlight/lp855x_bl.c | 23 +--
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/video/backlight/lp855x_bl.c 
> b/drivers/video/backlight/lp855x_bl.c
> index e94932c69f54..a895a8ca6d26 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
>  
>  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  {
> - unsigned int period = lp->pdata->period_ns;
> - unsigned int duty = br * period / max_br;
> - struct pwm_device *pwm;
> + struct pwm_device *pwm = NULL;
> + struct pwm_state state;
>  
>   /* request pwm device with the consumer name */
>   if (!lp->pwm) {
> @@ -244,19 +243,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   return;
>  
>   lp->pwm = pwm;
> -
> - /*
> -  * FIXME: pwm_apply_args() should be removed when switching to
> -  * the atomic PWM API.
> -  */
> - pwm_apply_args(pwm);
>   }
>  
> - pwm_config(lp->pwm, duty, period);
> - if (duty)
> - pwm_enable(lp->pwm);
> - else
> - pwm_disable(lp->pwm);
> + pwm_init_state(pwm, &state);

This is broken. If lp->pwm is already set at function entry, pwm is
NULL.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v5] backlight: lp855x: Switch to atomic PWM API

[Uwe Kleine-König]

On Wed, Nov 03, 2021 at 02:38:05PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
> 
> Signed-off-by: Maíra Canal 
> ---
> V1 -> V2: Initialize variable and simplify conditional loop
> V2 -> V3: Fix assignment of NULL variable
> V3 -> V4: Replace division for pwm_set_relative_duty_cycle
> V4 -> V5: Fix overwrite of state.period
> ---
>  drivers/video/backlight/lp855x_bl.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/backlight/lp855x_bl.c 
> b/drivers/video/backlight/lp855x_bl.c
> index e94932c69f54..e70a7b72dcf3 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
>  
>  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  {
> - unsigned int period = lp->pdata->period_ns;
> - unsigned int duty = br * period / max_br;
>   struct pwm_device *pwm;
> + struct pwm_state state;
>  
>   /* request pwm device with the consumer name */
>   if (!lp->pwm) {
> @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>  
>   lp->pwm = pwm;
>  
> - /*
> -  * FIXME: pwm_apply_args() should be removed when switching to
> -  * the atomic PWM API.
> -  */
> - pwm_apply_args(pwm);
> + pwm_init_state(lp->pwm, &state);
> + state.period = lp->pdata->period_ns;
> + } else {
> + pwm_get_state(lp->pwm, &state);
>   }
>  
> - pwm_config(lp->pwm, duty, period);
> - if (duty)
> - pwm_enable(lp->pwm);
> - else
> - pwm_disable(lp->pwm);
> + pwm_set_relative_duty_cycle(&state, br, max_br);
> + state.enabled = state.duty_cycle;
> +
> + pwm_apply_state(lp->pwm, &state);

Looks mostly right, but only on a deeper look into the driver. The
reason is that in the unmodified code there is always explicitly
period=lp->pdata->period_ns; while after your change the period is unset
(and so the previously set period is used).

So either mention in the change log that this driver doesn't modify the
pwm settings in other places or preferably pick an equivalent conversion
(plus maybe an optimisation in a separate patch).

If you go the "equivalent conversion" path, please note that
pwm_set_relative_duty_cycle() isn't equivalent to br * period / max_br,
as it implements a different rounding.

Best regards
Uwe


-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v6] backlight: lp855x: Switch to atomic PWM API

[Uwe Kleine-König]

Hello,

On Thu, Nov 04, 2021 at 02:58:38PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
> 
> Signed-off-by: Maíra Canal 

LGTM,

Reviewed-by: Uwe Kleine-König 

Thanks
Uwe

-- 
Pengutronix e.K.       | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] drm/bridge: ti-sn65dsi86: rename GPIO register bits

[Uwe Kleine-König]

On Mon, Jan 11, 2021 at 02:25:37PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 2:16 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Thu, Dec 10, 2020 at 12:19 AM Shawn Guo  wrote:
> > >
> > > From: Shawn Guo 
> > >
> > > It renames GPIO register bits to drop 'SN_' prefix, so that they are
> > > consistent to other definitions - prefixing register name with 'SN_' but
> > > not for bit fields.
> > >
> > > Signed-off-by: Shawn Guo 
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 26 +-
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > Sorry for taking so long to get back to this.  I think it came into my
> > inbox at the same time as a pile of other things and then got lost.
> > Your change looks good to me.  Sorry for being inconsistent when
> > adding those defines and thanks for fixing them!
> >
> > Reviewed-by: Douglas Anderson 
> 
> Hrm, I just caught up on more email and found that in:
> 
> https://lore.kernel.org/r/20201210174338.kecryijwptzc2...@pengutronix.de
> 
> ...that Uwe would prefer to keep these bits what I have and change all
> the others.  ;-)  I don't have a strong opinion either way, but I
> definitely agree that it'd be better for all the defines to be
> consistent.  If I had to arbitrarily make the decision one way or the
> other I'd probably land Shawn's patch but I certainly wouldn't object
> if we went Uwe's way either.  :-P

For the the relevant argument for prefixes is that tools like ctags and
cscope work more reliable. Take for example the name TX_TIMEOUT. There
are around 60 symbols with that name in the kernel tree. This is quite
annoying if you want to jump from a certain use to its definition.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] video: fbdev: imxfb: add COMPILE_TEST support

[Uwe Kleine-König]

Hello,

On Fri, Apr 12, 2019 at 12:59:53PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Add COMPILE_TEST support to imxfb driver for better compile
> testing coverage.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz 
> ---
>  drivers/video/fbdev/Kconfig |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/drivers/video/fbdev/Kconfig
> ===
> --- a/drivers/video/fbdev/Kconfig 2019-04-12 12:35:07.322174869 +0200
> +++ b/drivers/video/fbdev/Kconfig 2019-04-12 12:35:07.306174869 +0200
> @@ -334,7 +334,7 @@ config FB_SA1100
>  
>  config FB_IMX
>   tristate "Freescale i.MX1/21/25/27 LCD support"
> - depends on FB && ARCH_MXC
> + depends on FB && (ARCH_MXC || COMPILE_TEST)
>   select BACKLIGHT_LCD_SUPPORT
>   select LCD_CLASS_DEVICE
>   select FB_CFB_FILLRECT

I think you need something like this instead:

depends FB && HAVE_CLK && HAS_IOMEM
    depends ARCH_MXC || COMPILE_TEST

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/etnaviv: don't restrict to certain architectures

[Uwe Kleine-König]

On Wed, Jan 16, 2019 at 04:27:58PM +0100, Lucas Stach wrote:
> The Vivante GPU cores are found in many different SoCs and the driver
> does not depend on anything architecture specific, so just drop the
> architecture restriction.

With my Debian kernel team member hat on I don't like changes like this.
I don't know in which machines the etnaviv IP is available. Should the
Debian kernel enable it on x86? powerpc? riscv? If MXC + DOVE isn't
enough, it's your opportunity to not waste time of people who don't know
etnaviv by heart and only expand the dependency carefully.

Having said that I see the benefit of being able to enable the driver on
a wide variety of machines. It's flexible and everyone who doesn't want
that driver can still just disable it.

But when making this change, please consider also all the people who
will see

ETNAVIV (DRM support for Vivante GPU IP cores) (DRM_ETNAVIV) [N/y/m/?] 
(NEW)

in their next run of make oldconfig and need to decide if this driver is
useful for them. Also note they only see

DRM driver for Vivante GPUs.

when pressing '?'.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2] pwm: Add MediaTek MT8183 display PWM driver support

[Uwe Kleine-König]

On Tue, Jan 22, 2019 at 05:02:43PM +0800, Jitao Shi wrote:
> Use the mtk_pwm_data struction to define different registers
> and add MT8183 specific register operations, such as MT8183
> doesn't have commit register, needs to disable double buffer
> before writing register, and needs to select commit mode
> and use PWM_PERIOD/PWM_HIGH_WIDTH.
> 
> Signed-off-by: Jitao Shi 

There is no difference compared to (implicit) v1 sent a few minutes
earlier, right? There was another patch sent with the same Subject last
week, so I assume the mail from today without "v2" in the Subject was a
mistake?

> ---
Adding a paragraph below the tripple dash that points out what was
changed compared to the previous submission is a good idea to help
reviewers to more easily see what was changed. I guess you only adapted
the commit log as a reaction to Matthias Burgger's review?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] pwm: Add MediaTek MT8183 display PWM driver support

[Uwe Kleine-König]

On Wed, Jan 16, 2019 at 03:52:52PM +0800, Jitao Shi wrote:
> Use the mtk_pwm_data struction to define different registers
> and add MT8183 specific register operations, such as MT8183
> have commit register, needs to enable double buffer
> before writing register, and needs to select commit mode
> and use PWM_PERIOD/PWM_HIGH_WIDTH.

You forgot to add linux-pwm to the recipents.

Uwe

-- 
Pengutronix e.K.           | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Uwe Kleine-König]

On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> When the driver probes, the PWM pin is automatically configured to its
> default state, which should be the "pwm" function. However, at this
> point we don't know the actual level of the pin, which may be active or
> inactive. As a result, if the driver probes without enabling the
> backlight, the PWM pin might be active, and the backlight would be
> lit way before being officially enabled.

I'm not sure I understand the problem completely here. Let me try to
summarize the problem you solve here:

The backlight device's default pinctrl contains the PWM function of the
PWM pin. As the PWM is (or at least might be) in an undefined state the
default pinctrl should only be switched to when it's clear if the
backlight should be on or off.

So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
state and by switching to "sleep" you prevent "default" getting active.

Did I get this right? If not, please correct me.

What is the PWM pin configured to in "init" in your case? Is the pinctrl
just empty? Or is it a gpio-mode (together with a gpio-hog)?

My thoughts to this is are:

 a) This is a general problem that applies (I think) to most if not all
PWM consumers. If the PWM drives a motor, or makes your mobile
vibrate, or drives an LED, or a clk, the PWM shouldn't start
to do something before its consumer is ready.

 b) Thierry made it quite clear[1] that the PWM pin should be configured
in a pinctrl of the pwm device, not the backlight (or more general:
the consumer) device.

While I don't entirely agree with b) I think that even a) alone
justifies to think a bit more about the problem and preferably come up
with a solution that helps other consumers, too. Ideally if the
bootloader sets up the PWM to do something sensible, probing the
lowlevel PWM driver and the consumer driver should not interfere with
the bootloader's intention until the situation reaches a controlled
state. (I.e. if the backlight was left on by the bootloader to show a
nice logo, it should not flicker until a userspace program takes over
the display device.)

A PWM is special in contrast to other devices as its intended behaviour
is only fixed once a consumer is present. Without a consumer it is
unknown if the PWM is inverted or not. And so the common approach that
pinctrl is setup by the device core only doesn't work without drawbacks
for PWMs.

So if a PWM driver is probing and the PWM hardware already runs at say
constant one, some instance must define if the pin is supposed to be
configured in its "default" or "sleep" pinctrl. IMHO this isn't possible
in general without knowing the polarity of the PWM. (And even if it were
known that the polarity is inversed, it might be hard to say if your
PWM's hardware doesn't implement a disabled state and has to simulate
that using a 0% duty cycle.)

Another thing that complicates the matter is that at least pwm-imx27 has
the annoying property that disabling it (in hardware) drives the pin low
irrespective of the configured polarity. So if you want this type of
device to behave properly on disable, it must first drive a 0% duty
cycle, then switch the pinctrl state and only then disable the hardware.
This rules out that the lowlevel driver is unaware of the pinctrl stuff
which would be nice (or an inverted PWM won't be disabled in hardware or
you need an ugly sequence of callbacks to disable glitch-free). Also if
there is no sleep state, you better don't disable an inversed pwm-imx27
at all (in hardware)? (Alternatively we could drop the (undocumented)
guarantee that a disabled PWM results in the pin staying in its idle
level.)

What are the ways out? I think that if we go and apply your patch, we
should at least write some documentation with the details to provide some
"standard" way to solve similar problems.

Also it might be a good idea to let a PWM know if it is inverted or not
such that even without the presence of a consumer it can determine if
the hardware is active or not at probe time (in most cases at least).

Best regards
Uwe

[1] https://www.spinics.net/lists/linux-pwm/msg08246.html

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Uwe Kleine-König]

On Tue, Jun 25, 2019 at 11:58:21AM +0200, Thierry Reding wrote:
> On Tue, Jun 25, 2019 at 09:42:20AM +0200, Uwe Kleine-König wrote:
> > On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> > > When the driver probes, the PWM pin is automatically configured to its
> > > default state, which should be the "pwm" function. However, at this
> > > point we don't know the actual level of the pin, which may be active or
> > > inactive. As a result, if the driver probes without enabling the
> > > backlight, the PWM pin might be active, and the backlight would be
> > > lit way before being officially enabled.
> > 
> > I'm not sure I understand the problem completely here. Let me try to
> > summarize the problem you solve here:
> > 
> > The backlight device's default pinctrl contains the PWM function of the
> > PWM pin. As the PWM is (or at least might be) in an undefined state the
> > default pinctrl should only be switched to when it's clear if the
> > backlight should be on or off.
> > 
> > So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
> > state and by switching to "sleep" you prevent "default" getting active.
> > 
> > Did I get this right? If not, please correct me.
> > 
> > What is the PWM pin configured to in "init" in your case? Is the pinctrl
> > just empty? Or is it a gpio-mode (together with a gpio-hog)?
> > 
> > My thoughts to this is are:
> > 
> >  a) This is a general problem that applies (I think) to most if not all
> > PWM consumers. If the PWM drives a motor, or makes your mobile
> > vibrate, or drives an LED, or a clk, the PWM shouldn't start
> > to do something before its consumer is ready.
> 
> Yes, it shouldn't start before it is explicitly told to do so by the
> consumer. One exception is if the PWM was already set up by firmware
> to run. So I think in general terms we always want the PWM to remain
> in the current state upon probe.

In the end this means that also pinmuxing should not be touched when the
PWM device probes.

> The atomic PWM API was designed with that in mind. The original use-
> case was to allow seamlessly taking over from a PWM regulator. In order
> to do so, the driver needs to be able to read back the hardware state
> and *not* initialize the PWM to some default state.
> 
> I think that same approach can be extended to backlights. The driver's
> probe needs to determine what the current state of the backlight is and
> preferable not touch it. And that basically propagates all the way to
> the display driver, which ultimately needs to determine whether or not
> the display configuration (including the backlight) is enabled.

Are you ambitious enough to handle cases like: PWM is running (maybe
because it cannot be disabled), but the pin is muxed to an "unconnected"
configuration such that the pin doesn't oscillate? In this case you'd
need an inspection function for pinmuxing.

> >  b) Thierry made it quite clear[1] that the PWM pin should be configured
> > in a pinctrl of the pwm device, not the backlight (or more general:
> > the consumer) device.
> > 
> > While I don't entirely agree with b) I think that even a) alone
> > justifies to think a bit more about the problem and preferably come up
> > with a solution that helps other consumers, too. Ideally if the
> > bootloader sets up the PWM to do something sensible, probing the
> > lowlevel PWM driver and the consumer driver should not interfere with
> > the bootloader's intention until the situation reaches a controlled
> > state. (I.e. if the backlight was left on by the bootloader to show a
> > nice logo, it should not flicker until a userspace program takes over
> > the display device.)
> 
> Yes, exactly that.
> 
> > A PWM is special in contrast to other devices as its intended behaviour
> > is only fixed once a consumer is present. Without a consumer it is
> > unknown if the PWM is inverted or not. And so the common approach that
> > pinctrl is setup by the device core only doesn't work without drawbacks
> > for PWMs.
> 
> Actually I don't think PWMs are special in this regard. A GPIO, for
> example, can also be active-low or active-high, and without a consumer
> there's not enough context to determine which one it should be.

Right, PWMs are more similar to GPIOs than to (say) backlight devices.
With your request to configure the pinmux for a PWM pin with the PWM
device instead of its consumer you're making some things more difficult.
For GPIOs it's quite common that they are muxed from their consumer
because there the same problems are present.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Uwe Kleine-König]

On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > [...] although given pwm-backlight is essentially a wrapper driver
> > round a PWM I wondered why the pinctrl was on the backlight node
> > (rather than the PWM node).
> 
> I agree with this. We're defining the pin control state for the PWM pin,
> so in my opinion it should be the PWM driver that controls it.
> 
> One reason why I think this is important is if we ever end up with a
> device that requires pins from two different controllers to be
> configured at runtime, then how would we model that? Since pin control
> states cannot be aggregated, so you'd have to have multiple "default"
> states, each for the pins that they control.

I thought you can do:

pinctrl-names = "default";
pinctrl-0 = <&pinctrl_in_first_pincontroller>, 
<&pinctrl_in_another_controller>;

if two (or more) controllers are involved.
 
> On the other hand if we associate the pin control states with each of
> the resources that need those states, then when those resources are
> controlled, they will automatically know how to deal with the states.
> The top-level device (i.e. backlight) doesn't need to concern itself
> with those details.

So the options are:

 a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
related to the involved PWM pins in the consumer

 b) put the PWM pin config in the consumer's "default" pinctrl (and
maybe leave it out int "init" if you want smooth taking over).

(Or maybe use "enabled" and "disabled" in a) to match the pwm_states
.enabled?)

The advantages I see in b) over a) are:

 - "default" and "init" are a known pinctrl concept that most people
   should have understood.

 - You have all pinctrl config for the backlight in a single place.

 - none of the involved driver must explicitly handle pinctrl stuff

You presume that b) being commonly done is a sign of "our device trees
and kernel subsystems still maturing". But maybe it's only that the
capabilities provided by pinctrl subsystem without extra effort is good
enough?

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 11:58:44AM +0200, Thierry Reding wrote:
> On Wed, Jun 26, 2019 at 10:58:27AM +0200, Uwe Kleine-König wrote:
> > On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> > > On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > > > [...] although given pwm-backlight is essentially a wrapper driver
> > > > round a PWM I wondered why the pinctrl was on the backlight node
> > > > (rather than the PWM node).
> > > 
> > > I agree with this. We're defining the pin control state for the PWM pin,
> > > so in my opinion it should be the PWM driver that controls it.
> > > 
> > > One reason why I think this is important is if we ever end up with a
> > > device that requires pins from two different controllers to be
> > > configured at runtime, then how would we model that? Since pin control
> > > states cannot be aggregated, so you'd have to have multiple "default"
> > > states, each for the pins that they control.
> > 
> > I thought you can do:
> > 
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_in_first_pincontroller>, 
> > <&pinctrl_in_another_controller>;
> > 
> > if two (or more) controllers are involved.
> 
> You're right. Both the bindings say that this can be done and the code
> is also there to parse multiple states per pinctrl-* entry.
> 
> > > On the other hand if we associate the pin control states with each of
> > > the resources that need those states, then when those resources are
> > > controlled, they will automatically know how to deal with the states.
> > > The top-level device (i.e. backlight) doesn't need to concern itself
> > > with those details.
> > 
> > So the options are:
> > 
> >  a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
> > related to the involved PWM pins in the consumer
> > 
> >  b) put the PWM pin config in the consumer's "default" pinctrl (and
> > maybe leave it out int "init" if you want smooth taking over).
> 
> You can't put it into the "default" state because that state is applied
> before the consumer driver's ->probe().

If you do:

mybacklight {
pinctrl-names = "init", "default";
pinctrl-0 = <&pinctrl_without_pwm>
pinctrl-1 = <&pinctrl_with_pwm>;
...
};

Then nothing is done before probing of the backlight and only when the
probing is done and the pwm is taken over, the PWM-pinctrl is applied.

The only ugly thing here I can identify is that probe() might exit with
the PWM running and then enabling the pinmux for the PWM pin results in
an incomplete period at the beginning. But this happens only in some
corner cases that might not matter. (i.e. if the bootloader enabled the
PWM but didn't setup the pinmux; and if .probe enabled the PWM which we
agreed it probably shouldn't on it's own.)
 
> > (Or maybe use "enabled" and "disabled" in a) to match the pwm_states
> > .enabled?)
> 
> Yeah, I think this is what we'll need to do in order to implement the
> explicit behaviour that we need here.
> 
> > The advantages I see in b) over a) are:
> > 
> >  - "default" and "init" are a known pinctrl concept that most people
> >should have understood.
> 
> The problem is that they won't work in this case. The "init" state will
> be applied before the consumer driver's ->probe() if it exists. If it
> doesn't then "default" will be applied instead. Both cases are not
> something that we want if we want to take over the existing
> configuration.
> 
> >  - You have all pinctrl config for the backlight in a single place.
> 
> Depending on your point of view this could be considered a disadvantage.

Yeah, right, this is subjective.

> [...]
> Like I pointed out above, I don't think that's the case. But I don't
> want to overcomplicate things, so if you can prove that it can be done
> with the existing pinctrl helpers, I'd be happy to be proven wrong.

I tried, see above :-)

Best regards
Uwe


-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ARM: imx_v6_v7_defconfig: Enable CONFIG_PWM_IMX27

[Uwe Kleine-König]

Hello,

On Wed, Mar 20, 2019 at 01:01:26PM +, Leonard Crestez wrote:
> Commit d80f8206905c ("pwm: imx: Split into two drivers") also adds a new
> CONFIG_PWM_IMX27 for the PWM block on recent IMX chips and we should
> enable it by default for imx.
> 
> Restoring the PWM driver fixes an infinite probe loop in 5.1-rc1 on
> various imx6qdl-sabresd boards.
> 
> Signed-off-by: Leonard Crestez 
> Reported-by: Abel Vesa 

This is prior art:

https://patchwork.ozlabs.org/project/linux-pwm/list/?series=85452&state=%2A&archive=both

unfortunately this didn't get an Ack by Shawn and Thierry (added to
recipients) marked it as "Not Applicable".

Given that the driver change is in Linus Torvald's tree now (since
v5.1-rc1~38) it doesn't matter much via which tree this goes in and
Shawn's tree is the easier now.

@Shawn: Would you please apply my patches? If you don't have them any
more, I can bounce them to you. See

git show v5.1-rc1~38^2~17 -- drivers/pwm/Kconfig

for their justification.

> ---
>  arch/arm/configs/imx_v6_v7_defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> Probe loop repeats following lines:
> 
> [3.625031] pwm-backlight backlight-lvds: backlight-lvds supply power not 
> found, using dummy regulator
> [3.635868] panel-simple panel: panel supply power not found, using dummy 
> regulator
> [3.644844] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [3.651478] [drm] No driver support for vblank timestamp query.
> [3.657660] imx-drm display-subsystem: bound imx-ipuv3-crtc.2 (ops 
> ipu_crtc_ops)
> [3.665240] imx-drm display-subsystem: bound imx-ipuv3-crtc.3 (ops 
> ipu_crtc_ops)
> [3.672819] imx-drm display-subsystem: bound imx-ipuv3-crtc.6 (ops 
> ipu_crtc_ops)
> [3.680393] imx-drm display-subsystem: bound imx-ipuv3-crtc.7 (ops 
> ipu_crtc_ops)
> [3.688312] dwhdmi-imx 12.hdmi: Detected HDMI TX controller v1.30a 
> with HDCP (DWC HDMI 3D TX PHY)
> [3.699743] imx-drm display-subsystem: bound 12.hdmi (ops 
> dw_hdmi_imx_ops)
> [3.707343] imx-drm display-subsystem: failed to bind ldb (ops 
> imx_ldb_ops): -517
> [3.716921] imx-drm display-subsystem: master bind failed: -517
>
> Maybe it's an imx-drm bug which got exposed by accident?

IMHO this should be debugged independent of this patch.
 
> diff --git a/arch/arm/configs/imx_v6_v7_defconfig 
> b/arch/arm/configs/imx_v6_v7_defconfig
> index 5586a5074a96..2fa5074f5244 100644
> --- a/arch/arm/configs/imx_v6_v7_defconfig
> +++ b/arch/arm/configs/imx_v6_v7_defconfig
> @@ -397,10 +397,11 @@ CONFIG_SENSORS_ISL29018=y
>  CONFIG_MAG3110=y
>  CONFIG_MPL3115=y
>  CONFIG_PWM=y
>  CONFIG_PWM_FSL_FTM=y
>  CONFIG_PWM_IMX=y
> +CONFIG_PWM_IMX27=y

PWM_IMX is gone, so this can be dropped (but see my patch referenced
above).

>  CONFIG_NVMEM_IMX_OCOTP=y
>  CONFIG_NVMEM_VF610_OCOTP=y
>  CONFIG_TEE=y
>  CONFIG_OPTEE=y
>  CONFIG_MUX_MMIO=y

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ARM: imx_v6_v7_defconfig: Enable CONFIG_PWM_IMX27

[Uwe Kleine-König]

On Fri, Mar 22, 2019 at 10:47:00AM +0100, Thierry Reding wrote:
> On Thu, Mar 21, 2019 at 10:49:03AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Wed, Mar 20, 2019 at 01:01:26PM +, Leonard Crestez wrote:
> > > Commit d80f8206905c ("pwm: imx: Split into two drivers") also adds a new
> > > CONFIG_PWM_IMX27 for the PWM block on recent IMX chips and we should
> > > enable it by default for imx.
> > > 
> > > Restoring the PWM driver fixes an infinite probe loop in 5.1-rc1 on
> > > various imx6qdl-sabresd boards.
> > > 
> > > Signed-off-by: Leonard Crestez 
> > > Reported-by: Abel Vesa 
> > 
> > This is prior art:
> > 
> > https://patchwork.ozlabs.org/project/linux-pwm/list/?series=85452&state=%2A&archive=both
> > 
> > unfortunately this didn't get an Ack by Shawn and Thierry (added to
> > recipients) marked it as "Not Applicable".
> 
> If I mark patches as "not applicable" it generally means that I don't
> intend to apply them to the PWM tree.

I understood that. In this case however it would in my eyes have made
sense to take these patches together with the patch that does the driver
split via the pwm tree to make it less likely that people using the
respective defconfigs don't get the PWM driver enabled. That's the
situation now on 5.1-rc1 and people actuall hit this problem. :-| That's
why I wrote:

I think the easiest handling would be to let them go via the pwm
tree with Shawn's Ack to get the update near to the actual split
into the mainline.

in the cover letter.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v3 1/4] dt-bindngs: display: panel: Add BOE tv101wum-n16 panel bindings

[Uwe Kleine-König]

$Subject ~= s/bindngs/bindings/

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i2c/sil164: Drop no-op remove function

[Uwe Kleine-König]

Hello,

On Thu, May 26, 2022 at 10:25:38PM +0200, Uwe Kleine-König wrote:
> A remove callback that just returns 0 is equivalent to no callback at all
> as can be seen in i2c_device_remove(). So simplify accordingly.

I intend to change the prototype of i2c remove callbacks to return void
after the next merge window. This patch is a preparation for that quest.

So I ask you to either take this patch to sil164_drv.c before (my
preferred option), or accept that I send it as part of a bigger series
that will probably be merged via the i2c tree.

See
https://lore.kernel.org/linux-i2c/20220609091018.q52fhowlsdbdk...@pengutronix.de
for some more details.

Best regards
Uwe

-- 
Pengutronix e.K.       | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] drm: bridge: Add TI DLPC3433 DSI to DMD bridge

[Uwe Kleine-König]

Hello,

On Fri, Jun 03, 2022 at 04:37:51PM +0200, Robert Foss wrote:
> On Fri, 3 Jun 2022 at 16:04, Jagan Teki  wrote:
> > +static int dlpc3433_remove(struct i2c_client *client)
> > +{
> > +   struct dlpc *dlpc = i2c_get_clientdata(client);
> > +
> > +   drm_bridge_remove(&dlpc->bridge);
> > +   of_node_put(dlpc->host_node);
> > +
> > +   return 0;
> > +}
> > + [...]
> > +static struct i2c_driver dlpc3433_driver = {
> > +   .probe_new = dlpc3433_probe,
> > +   .remove = dlpc3433_remove,
> > +   .id_table = dlpc3433_id,
> > +   .driver = {
> > +   .name = "ti-dlpc3433",
> > +   .of_match_table = dlpc3433_match_table,
> > +   },
> > +};
> > +module_i2c_driver(dlpc3433_driver);
> 
> Applied to drm-misc-next.

just a quick note that there is an easy conflict between this patch and
my effort to make i2c remove callbacks return void. I intend to post my
series on top of v5.20-rc1, so if this patch goes in before, everything
is fine.

See
https://lore.kernel.org/linux-i2c/20220609091018.q52fhowlsdbdk...@pengutronix.de/
for some more details.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v8 01/16] clk: generalize devm_clk_get() a bit

[Uwe Kleine-König]

bus_for_each_dev from bus_add_driver+0x168/0x1f4
> [2.097749]  bus_add_driver from driver_register+0x7c/0x118
> [2.097766]  driver_register from do_one_initcall+0x44/0x1ec
> [2.097784]  do_one_initcall from kernel_init_freeable+0x1d4/0x224
> [2.097803]  kernel_init_freeable from kernel_init+0x18/0x12c
> [2.097820]  kernel_init from ret_from_fork+0x14/0x2c
> [2.097831] Exception stack(0xf080dfb0 to 0xf080dff8)
> [2.097839] dfa0:   
>  
> [2.097847] dfc0:       
>  
> [2.097854] dfe0:     0013 
> [2.097862] Code: c2288680    (c2288680)
> [2.097872] ---[ end trace  ]---
> 
> 
> Let me know if you have any thoughts.

Yeah, sorry, there is already a fix at
https://lore.kernel.org/linux-clk/20220620171815.114212-1-u.kleine-koe...@pengutronix.de

(Pro tipp: The commit in next has a Link: footer. If you follow the
link, you find the thread that was actually applied (i.e. v9) and where
the fix is also contained.)

@Stephen: It would be a great favour to our testers if you could apply
the fix ...

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] backlight: pwm_bl: Avoid open coded arithmetic in memory allocation

[Uwe Kleine-König]

On Sat, Feb 05, 2022 at 08:40:48AM +0100, Christophe JAILLET wrote:
> kmalloc_array()/kcalloc() should be used to avoid potential overflow when
> a multiplication is needed to compute the size of the requested memory.
> 
> So turn a kzalloc()+explicit size computation into an equivalent kcalloc().
> 
> Signed-off-by: Christophe JAILLET 

LGTM

Acked-by: Christophe JAILLET 

Thanks
Uwe

-- 
Pengutronix e.K.           | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] backlight: pwm_bl: Avoid open coded arithmetic in memory allocation

[Uwe Kleine-König]

On Mon, Feb 07, 2022 at 09:31:00AM +, Lee Jones wrote:
> On Mon, 07 Feb 2022, Uwe Kleine-König wrote:
> 
> > On Sat, Feb 05, 2022 at 08:40:48AM +0100, Christophe JAILLET wrote:
> > > kmalloc_array()/kcalloc() should be used to avoid potential overflow when
> > > a multiplication is needed to compute the size of the requested memory.
> > > 
> > > So turn a kzalloc()+explicit size computation into an equivalent 
> > > kcalloc().
> > > 
> > > Signed-off-by: Christophe JAILLET 
> > 
> > LGTM
> > 
> > Acked-by: Christophe JAILLET 
> > 
> > Thanks
> > Uwe
> 
> I am totally confused!

An rightfully so. Copy-paste-fail, this was supposed to be

Acked-by: Uwe Kleine-König 

Sorry!

Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] Staging: fbtft: Fix style problem in header

[Uwe Kleine-König]

On Tue, Apr 19, 2022 at 03:21:28PM -0400, Ian Cowan wrote:
> Removed an unnecessary semicolon at the end of a macro call
> 
> Signed-off-by: Ian Cowan 
> ---
>  drivers/staging/fbtft/fbtft.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index 2c2b5f1c1df3..aa66760e1a9c 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -277,7 +277,7 @@ static const struct of_device_id dt_ids[] = { 
> \
>   { .compatible = _compatible },  
> \
>   {}, 
> \
>  };   
> \
> -MODULE_DEVICE_TABLE(of, dt_ids);
> +MODULE_DEVICE_TABLE(of, dt_ids)

In fact the ; after MODULE_DEVICE_TABLE is necessary. There is only a
single instance in the kernel without a semicolon[1]. That's in
drivers/pci/controller/pcie-microchip-host.c and this only works because
this driver cannot be compiled as a module and so MODULE_DEVICE_TABLE
evaluates to nothing. Will send a patch for that one.

Best regards
Uwe

[1] git grep -E '^[[:space:]]*MODULE_DEVICE_TABLE\([^;]*$'

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH] Staging: fbtft: Fix style problem in header

[Uwe Kleine-König]

Hello Ian,

On Wed, Apr 20, 2022 at 09:57:27AM -0400, Ian Cowan wrote:
> On Wed, Apr 20, 2022 at 08:47:11AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 19, 2022 at 03:21:28PM -0400, Ian Cowan wrote:
> > > Removed an unnecessary semicolon at the end of a macro call
> > > 
> > > Signed-off-by: Ian Cowan 
> > > ---
> > >  drivers/staging/fbtft/fbtft.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> > > index 2c2b5f1c1df3..aa66760e1a9c 100644
> > > --- a/drivers/staging/fbtft/fbtft.h
> > > +++ b/drivers/staging/fbtft/fbtft.h
> > > @@ -277,7 +277,7 @@ static const struct of_device_id dt_ids[] = { 
> > > \
> > >   { .compatible = _compatible },  
> > > \
> > >   {}, 
> > > \
> > >  };   
> > > \
> > > -MODULE_DEVICE_TABLE(of, dt_ids);
> > > +MODULE_DEVICE_TABLE(of, dt_ids)
> > 
> > In fact the ; after MODULE_DEVICE_TABLE is necessary. There is only a
> > single instance in the kernel without a semicolon[1]. That's in
> > drivers/pci/controller/pcie-microchip-host.c and this only works because
> > this driver cannot be compiled as a module and so MODULE_DEVICE_TABLE
> > evaluates to nothing. Will send a patch for that one.

FTR: Patch was sent: 
https://lore.kernel.org/linux-pci/20220420065832.14173-1-u.kleine-koe...@pengutronix.de

> When I built this, it appeared to succeed. I used the command "make
> M=/drivers/staging/fbtft modules". Is this incorrect? For reference this
> is my first patch so it's highly likely I did this incorrectly.

I don't know for sure, but I'd have said that the M= stuff is for
out-of-tree modules only.

I'd recommend:

make allmodconfig
make drivers/staging/fbtft/

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH] drm/ssd130x: Make ssd130x_remove() return void

[Uwe Kleine-König]

This function returns zero unconditionally, so there isn't any benefit
of returning a value. Make it return void to be able to see at a glance
that the return value of ssd130x_i2c_remove() is always zero.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/solomon/ssd130x-i2c.c | 4 +++-
 drivers/gpu/drm/solomon/ssd130x.c | 4 +---
 drivers/gpu/drm/solomon/ssd130x.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c 
b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index 3126aeda4ced..54c8e2e4d9dd 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -43,7 +43,9 @@ static int ssd130x_i2c_remove(struct i2c_client *client)
 {
struct ssd130x_device *ssd130x = i2c_get_clientdata(client);
 
-   return ssd130x_remove(ssd130x);
+   ssd130x_remove(ssd130x);
+
+   return 0;
 }
 
 static void ssd130x_i2c_shutdown(struct i2c_client *client)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index ce4dc20412e0..e6feb2a166a6 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -824,11 +824,9 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, 
struct regmap *regmap)
 }
 EXPORT_SYMBOL_GPL(ssd130x_probe);
 
-int ssd130x_remove(struct ssd130x_device *ssd130x)
+void ssd130x_remove(struct ssd130x_device *ssd130x)
 {
drm_dev_unplug(&ssd130x->drm);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(ssd130x_remove);
 
diff --git a/drivers/gpu/drm/solomon/ssd130x.h 
b/drivers/gpu/drm/solomon/ssd130x.h
index cd21cdccb566..f633bac84477 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -70,7 +70,7 @@ struct ssd130x_device {
 };
 
 struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap 
*regmap);
-int ssd130x_remove(struct ssd130x_device *ssd130x);
+void ssd130x_remove(struct ssd130x_device *ssd130x);
 void ssd130x_shutdown(struct ssd130x_device *ssd130x);
 
 #endif /* __SSD1307X_H__ */

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH] drm/bridge: tfp410: Make tfp410_fini() return void

[Uwe Kleine-König]

tfp410_fini() always returns zero. Make it return no value which makes it
easier to see in the callers that there is no error to handle.

Also the return value of i2c and platform driver remove callbacks is
ignored anyway. This prepares making i2c and platform remove callbacks
return void, too.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c 
b/drivers/gpu/drm/bridge/ti-tfp410.c
index ba3fa2a9b8a4..756b3e6e776b 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -341,13 +341,11 @@ static int tfp410_init(struct device *dev, bool i2c)
return 0;
 }
 
-static int tfp410_fini(struct device *dev)
+static void tfp410_fini(struct device *dev)
 {
struct tfp410 *dvi = dev_get_drvdata(dev);
 
drm_bridge_remove(&dvi->bridge);
-
-   return 0;
 }
 
 static int tfp410_probe(struct platform_device *pdev)
@@ -357,7 +355,9 @@ static int tfp410_probe(struct platform_device *pdev)
 
 static int tfp410_remove(struct platform_device *pdev)
 {
-   return tfp410_fini(&pdev->dev);
+   tfp410_fini(&pdev->dev);
+
+   return 0;
 }
 
 static const struct of_device_id tfp410_match[] = {
@@ -394,7 +394,9 @@ static int tfp410_i2c_probe(struct i2c_client *client,
 
 static int tfp410_i2c_remove(struct i2c_client *client)
 {
-   return tfp410_fini(&client->dev);
+   tfp410_fini(&client->dev);
+
+   return 0;
 }
 
 static const struct i2c_device_id tfp410_i2c_ids[] = {

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH v2 1/6] drm/i2c/sil164: Drop no-op remove function

[Uwe Kleine-König]

A remove callback that just returns 0 is equivalent to no callback at all
as can be seen in i2c_device_remove(). So simplify accordingly.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/i2c/sil164_drv.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i2c/sil164_drv.c b/drivers/gpu/drm/i2c/sil164_drv.c
index 741886b54419..1bc0b5de4499 100644
--- a/drivers/gpu/drm/i2c/sil164_drv.c
+++ b/drivers/gpu/drm/i2c/sil164_drv.c
@@ -370,12 +370,6 @@ sil164_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
return 0;
 }
 
-static int
-sil164_remove(struct i2c_client *client)
-{
-   return 0;
-}
-
 static struct i2c_client *
 sil164_detect_slave(struct i2c_client *client)
 {
@@ -427,7 +421,6 @@ MODULE_DEVICE_TABLE(i2c, sil164_ids);
 static struct drm_i2c_encoder_driver sil164_driver = {
.i2c_driver = {
.probe = sil164_probe,
-   .remove = sil164_remove,
.driver = {
.name = "sil164",
},
-- 
2.36.1

[PATCH v2 0/6] i2c: Make remove callback return void

[Uwe Kleine-König]

Hello,

as promised there comes the series that does

-   int (*remove)(struct i2c_client *client);
+   void (*remove)(struct i2c_client *client);

in struct i2c_driver. Returning an error code has no (relevant) effect,
as in the Linux device model removal cannot be stopped. So the
possibility to return a value is prone to trouble because driver authors
might assume it does stop the process and .remove is called again at a
later point again. A few errors like these were fixed preparing this
change. See commit bbc126ae381c ("usb: typec: tcpci: Don't skip cleanup
in .remove() on error") for an example.

To prevent such errors in the future, .remove() is changed to return
void which leaves no doubt about its semantics.

This series is build-tested on arm64, m68k, powerpc, riscv, s390, sparc64 and
x86_64 using allmodconfig.

As some conflicts are expected I sent this early after -rc1 such that it
can be included early into next and be put on an immutable branch for
subsystems to resolve merge conflicts.

The first 5 patches in this series are preparatory work with the goal
that the final patch just drops "return 0" (or changes them to a plain
"return"). The three led patches got an Ack by Pavel (= led maintainer),
the other two didn't get final maintainer feedback. See


https://lore.kernel.org/dri-devel/20220526202538.1723142-1-u.kleine-koe...@pengutronix.de

https://lore.kernel.org/linux-gpio/20220502170555.51183-1-u.kleine-koe...@pengutronix.de

for the "discussions" about these two.

The changes since the first submission were already summarized in
https://lore.kernel.org/r/20220811124029.usxahk5nvfgqs...@pengutronix.de
in detail, the only change since then is that the changes to
drivers/input/keyboard/adp5588-keys.c were merged, so additionally to
the changes described before, the change to that driver was adapted
accordingly. To summarize it's just:

 - The preparing patches (#1 - #5) are unchanged

 - Remove a comment form drivers/net/mctp/mctp-i2c.c that doesn't match
   any more in #6. (Noticed by Matt Johnston and independently by Jeremy Kerr)

 - Removed a stray change to lib/Kconfig.kasan that I needed for build
   testing but that wasn't ment to be included; also in patch #6.
   (Noticed by Andrey Ryabinin).

 - Rebase v5.19-rc4 -> 6.0-rc1 and adapt for dropped and new drivers.

The patches are also available at

https://git.pengutronix.de/git/ukl/linux i2c-remove-void

Thanks
Uwe

Uwe Kleine-König (6):
  drm/i2c/sil164: Drop no-op remove function
  leds: lm3697: Remove duplicated error reporting in .remove()
  leds: lm3601x: Don't use mutex after it was destroyed
  leds: lm3601x: Improve error reporting for problems during .remove()
  gpio: pca953x: Make platform teardown callback return void
  i2c: Make remove callback return void

 Documentation/i2c/writing-clients.rst   |  2 +-
 arch/arm/mach-davinci/board-da850-evm.c | 12 
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c  |  3 +--
 drivers/auxdisplay/ht16k33.c|  4 +---
 drivers/auxdisplay/lcd2s.c  |  3 +--
 drivers/char/ipmi/ipmb_dev_int.c|  4 +---
 drivers/char/ipmi/ipmi_ipmb.c   |  4 +---
 drivers/char/ipmi/ipmi_ssif.c   |  6 ++
 drivers/char/tpm/st33zp24/i2c.c |  4 +---
 drivers/char/tpm/tpm_i2c_atmel.c|  3 +--
 drivers/char/tpm/tpm_i2c_infineon.c |  4 +---
 drivers/char/tpm/tpm_i2c_nuvoton.c  |  3 +--
 drivers/char/tpm/tpm_tis_i2c.c  |  3 +--
 drivers/char/tpm/tpm_tis_i2c_cr50.c |  6 ++
 drivers/clk/clk-cdce706.c   |  3 +--
 drivers/clk/clk-cs2000-cp.c |  4 +---
 drivers/clk/clk-si514.c |  3 +--
 drivers/clk/clk-si5341.c|  4 +---
 drivers/clk/clk-si5351.c|  4 +---
 drivers/clk/clk-si570.c |  3 +--
 drivers/clk/clk-versaclock5.c   |  4 +---
 drivers/crypto/atmel-ecc.c  |  6 ++
 drivers/crypto/atmel-sha204a.c  |  6 ++
 drivers/extcon/extcon-rt8973a.c |  4 +---
 drivers/gpio/gpio-adp5588.c |  4 +---
 drivers/gpio/gpio-max7300.c |  4 +---
 drivers/gpio/gpio-pca953x.c | 13 +++--
 drivers/gpio/gpio-pcf857x.c |  4 +---
 drivers/gpio/gpio-tpic2810.c|  4 +---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c|  4 +---
 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c  |  4 +---
 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c  |  4 +---
 drivers/gpu/drm/bridge/analogix/anx7625.c   |  4 +---
 drive

[PATCH] drm/i2c/sil164: Drop no-op remove function

[Uwe Kleine-König]

A remove callback that just returns 0 is equivalent to no callback at all
as can be seen in i2c_device_remove(). So simplify accordingly.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/i2c/sil164_drv.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i2c/sil164_drv.c b/drivers/gpu/drm/i2c/sil164_drv.c
index 741886b54419..1bc0b5de4499 100644
--- a/drivers/gpu/drm/i2c/sil164_drv.c
+++ b/drivers/gpu/drm/i2c/sil164_drv.c
@@ -370,12 +370,6 @@ sil164_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
return 0;
 }
 
-static int
-sil164_remove(struct i2c_client *client)
-{
-   return 0;
-}
-
 static struct i2c_client *
 sil164_detect_slave(struct i2c_client *client)
 {
@@ -427,7 +421,6 @@ MODULE_DEVICE_TABLE(i2c, sil164_ids);
 static struct drm_i2c_encoder_driver sil164_driver = {
.i2c_driver = {
.probe = sil164_probe,
-   .remove = sil164_remove,
.driver = {
.name = "sil164",
},

base-commit: 4b0986a3613c92f4ec1bdc7f60ec66fea135991f
-- 
2.36.1

[PATCH 1/6] drm/i2c/sil164: Drop no-op remove function

[Uwe Kleine-König]

A remove callback that just returns 0 is equivalent to no callback at all
as can be seen in i2c_device_remove(). So simplify accordingly.

Signed-off-by: Uwe Kleine-König 
Forwarded: id:20220526202538.1723142-1-u.kleine-koe...@pengutronix.de
---
 drivers/gpu/drm/i2c/sil164_drv.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i2c/sil164_drv.c b/drivers/gpu/drm/i2c/sil164_drv.c
index 741886b54419..1bc0b5de4499 100644
--- a/drivers/gpu/drm/i2c/sil164_drv.c
+++ b/drivers/gpu/drm/i2c/sil164_drv.c
@@ -370,12 +370,6 @@ sil164_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
return 0;
 }
 
-static int
-sil164_remove(struct i2c_client *client)
-{
-   return 0;
-}
-
 static struct i2c_client *
 sil164_detect_slave(struct i2c_client *client)
 {
@@ -427,7 +421,6 @@ MODULE_DEVICE_TABLE(i2c, sil164_ids);
 static struct drm_i2c_encoder_driver sil164_driver = {
.i2c_driver = {
.probe = sil164_probe,
-   .remove = sil164_remove,
.driver = {
.name = "sil164",
},
-- 
2.36.1

Re: [PATCH 6/6] i2c: Make remove callback return void

[Uwe Kleine-König]

Hello,

[I dropped nearly all individuals from the Cc: list because various
bounces reported to be unhappy about the long (logical) line.]

On Wed, Jun 29, 2022 at 03:03:54PM +0800, Jeremy Kerr wrote:
> Looks good - just one minor change for the mctp-i2c driver, but only
> worthwhile if you end up re-rolling this series for other reasons:
> 
> > -static int mctp_i2c_remove(struct i2c_client *client)
> > +static void mctp_i2c_remove(struct i2c_client *client)
> >  {
> > struct mctp_i2c_client *mcli = i2c_get_clientdata(client);
> > struct mctp_i2c_dev *midev = NULL, *tmp = NULL;
> > @@ -1000,7 +1000,6 @@ static int mctp_i2c_remove(struct i2c_client *client)
> > mctp_i2c_free_client(mcli);
> > mutex_unlock(&driver_clients_lock);
> > /* Callers ignore return code */
> > -   return 0;
> >  }
> 
> The comment there no longer makes much sense, I'd suggest removing that
> too.

Yeah, that was already pointed out to me in a private reply. It's
already fixed in

https://git.pengutronix.de/cgit/ukl/linux/log/?h=i2c-remove-void

> Either way:
> 
> Reviewed-by: Jeremy Kerr 

Added to my tree, too.

Thanks
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 6/6] i2c: Make remove callback return void

[Uwe Kleine-König]

[Dropped most people from Cc, keeping only lists]

On Wed, Jun 29, 2022 at 04:11:26PM +0300, Andrey Ryabinin wrote:
> On 6/28/22 17:03, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König 
> >
> > The value returned by an i2c driver's remove function is mostly ignored.
> > (Only an error message is printed if the value is non-zero that the
> > error is ignored.)
> >
> > So change the prototype of the remove function to return no value. This
> > way driver authors are not tempted to assume that passing an error to
> > the upper layer is a good idea. All drivers are adapted accordingly.
> > There is no intended change of behaviour, all callbacks were prepared to
> > return 0 before.
> >
> > Signed-off-by: Uwe Kleine-König 
> > ---
> | 2 +-
> >  lib/Kconfig.kasan | 1 +
> 
> > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> > index f0973da583e0..366e61639cb2 100644
> > --- a/lib/Kconfig.kasan
> > +++ b/lib/Kconfig.kasan
> > @@ -149,6 +149,7 @@ config KASAN_STACK
> >   depends on KASAN_GENERIC || KASAN_SW_TAGS
> >   depends on !ARCH_DISABLE_KASAN_INLINE
> >   default y if CC_IS_GCC
> > + depends on !ARM
> >   help
> > Disables stack instrumentation and thus KASAN's ability to detect
> > out-of-bounds bugs in stack variables.
> 
> 
> What is this doing here?

Huh, that is wrong. I needed that for build testing, but it shouldn't
have been added to the patch. I'm dropping that for the final
submission.

Thanks for spotting.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 6/6] i2c: Make remove callback return void

[Uwe Kleine-König]

On Tue, Jul 05, 2022 at 12:08:52PM +0200, Jean Delvare wrote:
> On Tue, 28 Jun 2022 16:03:12 +0200, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König 
> > 
> > The value returned by an i2c driver's remove function is mostly ignored.
> > (Only an error message is printed if the value is non-zero that the
> > error is ignored.)
> > 
> > So change the prototype of the remove function to return no value. This
> > way driver authors are not tempted to assume that passing an error to
> > the upper layer is a good idea. All drivers are adapted accordingly.
> > There is no intended change of behaviour, all callbacks were prepared to
> > return 0 before.
> > 
> > Signed-off-by: Uwe Kleine-König 
> > ---
> 
> That's a huge change for a relatively small benefit, but if this is
> approved by the I2C core maintainer then fine with me. For:

Agreed, it's huge. The benefit isn't really measureable, the motivation
is to improve the situation for driver authors who with the change
cannot make wrong assumptions about what to return in .remove(). During
the preparation this uncovered a few bugs. See for example
bbc126ae381cf0a27822c1f822d0aeed74cc40d9.

> >  drivers/hwmon/adc128d818.c| 4 +---
> >  drivers/hwmon/adt7470.c   | 3 +--
> >  drivers/hwmon/asb100.c| 6 ++
> >  drivers/hwmon/asc7621.c   | 4 +---
> >  drivers/hwmon/dme1737.c   | 4 +---
> >  drivers/hwmon/f75375s.c   | 5 ++---
> >  drivers/hwmon/fschmd.c| 6 ++
> >  drivers/hwmon/ftsteutates.c   | 3 +--
> >  drivers/hwmon/ina209.c| 4 +---
> >  drivers/hwmon/ina3221.c   | 4 +---
> >  drivers/hwmon/jc42.c  | 3 +--
> >  drivers/hwmon/mcp3021.c   | 4 +---
> >  drivers/hwmon/occ/p8_i2c.c| 4 +---
> >  drivers/hwmon/pcf8591.c   | 3 +--
> >  drivers/hwmon/smm665.c| 3 +--
> >  drivers/hwmon/tps23861.c  | 4 +---
> >  drivers/hwmon/w83781d.c   | 4 +---
> >  drivers/hwmon/w83791d.c   | 6 ++
> >  drivers/hwmon/w83792d.c   | 6 ++
> >  drivers/hwmon/w83793.c| 6 ++
> >  drivers/hwmon/w83795.c| 4 +---
> >  drivers/hwmon/w83l785ts.c | 6 ++
> >  drivers/i2c/i2c-core-base.c   | 6 +-
> >  drivers/i2c/i2c-slave-eeprom.c| 4 +---
> >  drivers/i2c/i2c-slave-testunit.c  | 3 +--
> >  drivers/i2c/i2c-smbus.c   | 3 +--
> >  drivers/i2c/muxes/i2c-mux-ltc4306.c   | 4 +---
> >  drivers/i2c/muxes/i2c-mux-pca9541.c   | 3 +--
> >  drivers/i2c/muxes/i2c-mux-pca954x.c   | 3 +--
> 
> Reviewed-by: Jean Delvare 

Thanks
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH 6/6] i2c: Make remove callback return void

[Uwe Kleine-König]

On Wed, Jul 06, 2022 at 12:13:15PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 28, 2022 at 04:03:12PM +0200, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König 
> > 
> > The value returned by an i2c driver's remove function is mostly ignored.
> > (Only an error message is printed if the value is non-zero that the
> > error is ignored.)
> > 
> > So change the prototype of the remove function to return no value. This
> > way driver authors are not tempted to assume that passing an error to
> > the upper layer is a good idea. All drivers are adapted accordingly.
> > There is no intended change of behaviour, all callbacks were prepared to
> > return 0 before.
> > 
> > Signed-off-by: Uwe Kleine-König 
> > ---
> 
> Assuming you remove the spurious kasan change:

It's already gone in my tree, see
https://git.pengutronix.de/cgit/ukl/linux/commit/?h=i2c-remove-void

> Reviewed-by: Vladimir Oltean 

Thanks
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH 3/3] drm/mipi-dsi: Make remove callback return void

[Uwe Kleine-König]

All implementations return 0 and the return value of mipi_dsi_drv_remove()
is ignored anyhow.

So change the prototype of the remove function to return no value. This
way driver authors are not tempted to assume that passing an error to
the upper layer is a good idea. All drivers are adapted accordingly.
There is no intended change of behaviour.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 4 +---
 drivers/gpu/drm/bridge/tc358762.c| 4 +---
 drivers/gpu/drm/bridge/tc358764.c| 4 +---
 drivers/gpu/drm/drm_mipi_dsi.c   | 4 +++-
 drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4 +---
 drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c   | 4 +---
 drivers/gpu/drm/panel/panel-boe-himax8279d.c | 4 +---
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c   | 4 +---
 drivers/gpu/drm/panel/panel-dsi-cm.c | 4 +---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 4 +---
 drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c| 4 +---
 drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c| 4 +---
 drivers/gpu/drm/panel/panel-ilitek-ili9881c.c| 4 +---
 drivers/gpu/drm/panel/panel-innolux-p079zca.c| 4 +---
 drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 4 +---
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c   | 4 +---
 drivers/gpu/drm/panel/panel-khadas-ts050.c   | 4 +---
 drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c   | 4 +---
 drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c   | 4 +---
 drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c   | 4 +---
 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c | 4 +---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c| 4 +---
 drivers/gpu/drm/panel/panel-novatek-nt35560.c| 4 +---
 drivers/gpu/drm/panel/panel-novatek-nt35950.c| 4 +---
 drivers/gpu/drm/panel/panel-novatek-nt36672a.c   | 4 +---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 4 +---
 drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c   | 4 +---
 drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 4 +---
 drivers/gpu/drm/panel/panel-raydium-rm67191.c| 4 +---
 drivers/gpu/drm/panel/panel-raydium-rm68200.c| 4 +---
 drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6d16d0.c| 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c| 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c| 3 +--
 drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c | 4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c| 4 +---
 drivers/gpu/drm/panel/panel-samsung-sofef00.c| 4 +---
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c  | 6 ++
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c  | 4 +---
 drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c  | 4 +---
 drivers/gpu/drm/panel/panel-simple.c | 4 +---
 drivers/gpu/drm/panel/panel-sitronix-st7701.c| 4 +---
 drivers/gpu/drm/panel/panel-sitronix-st7703.c| 4 +---
 drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c   | 4 +---
 drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 4 +---
 drivers/gpu/drm/panel/panel-truly-nt35597.c  | 3 +--
 drivers/gpu/drm/panel/panel-visionox-rm69299.c   | 3 +--
 drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 4 +---
 include/drm/drm_mipi_dsi.h   | 2 +-
 50 files changed, 53 insertions(+), 144 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c 
b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 47dea657a752..c57f149ed61a 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -725,14 +725,12 @@ static int chipone_i2c_probe(struct i2c_client *client,
return chipone_dsi_host_attach(icn);
 }
 
-static int chipone_dsi_remove(struct mipi_dsi_device *dsi)
+static void chipone_dsi_remove(struct mipi_dsi_device *dsi)
 {
struct chipone *icn = mipi_dsi_get_drvdata(dsi);
 
mipi_dsi_detach(dsi);
drm_bridge_remove(&icn->bridge);
-
-   return 0;
 }
 
 static const struct of_device_id chipone_of_match[] = {
diff --git a/drivers/gpu/drm/bridge/tc358762.c 
b/drivers/gpu/drm/bridge/tc358762.c
index 40439da4db49..7f4fce1aa998 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -241,14 +241,12 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
return ret;
 }
 
-static int tc358762_remove(struct mipi_dsi_device *dsi)
+static void tc358762_remove(struct mipi_dsi_device *dsi)
 {
struct tc358762 *ctx = mipi_dsi_get_drvdata(dsi);
 
mipi_dsi_det

[PATCH 1/3] drm/panel: simple: Make panel_simple_remove() return void

[Uwe Kleine-König]

panel_simple_remove() returns zero unconditionally. Make it return no value
instead making more obvious what happens in the callers.

This is a preparation for making platform and mipi-dsi remove callbacks
return void.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/panel/panel-simple.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 4a2e580a2f7b..0a07e2849b7b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -680,7 +680,7 @@ static int panel_simple_probe(struct device *dev, const 
struct panel_desc *desc)
return err;
 }
 
-static int panel_simple_remove(struct device *dev)
+static void panel_simple_remove(struct device *dev)
 {
struct panel_simple *panel = dev_get_drvdata(dev);
 
@@ -692,8 +692,6 @@ static int panel_simple_remove(struct device *dev)
pm_runtime_disable(dev);
if (panel->ddc)
put_device(&panel->ddc->dev);
-
-   return 0;
 }
 
 static void panel_simple_shutdown(struct device *dev)
@@ -4140,7 +4138,9 @@ static int panel_simple_platform_probe(struct 
platform_device *pdev)
 
 static int panel_simple_platform_remove(struct platform_device *pdev)
 {
-   return panel_simple_remove(&pdev->dev);
+   panel_simple_remove(&pdev->dev);
+
+   return 0;
 }
 
 static void panel_simple_platform_shutdown(struct platform_device *pdev)
@@ -4441,7 +4441,9 @@ static int panel_simple_dsi_remove(struct mipi_dsi_device 
*dsi)
if (err < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
 
-   return panel_simple_remove(&dsi->dev);
+   panel_simple_remove(&dsi->dev);
+
+   return 0;
 }
 
 static void panel_simple_dsi_shutdown(struct mipi_dsi_device *dsi)
-- 
2.36.1

[PATCH 0/3] drm/mipi-dsi: Make remove callback return void

[Uwe Kleine-König]

Hello,

the intention of this patch series is to make it obvious to mipi-dsi
driver authors that they cannot rely on the mipi-dsi (or driver) core to
handle errors that occur at remove time. This is done by changing the
return type of the remove callback from int to void.

The first two patches make two drivers (obviously) always return zero.
Then in the final patch only return 0 are dropped and so it's easier to
see that there are no side effects.

A positive side effect of the last patch is that mipi_dsi_drv_remove()
now obviously always returns zero, which in turn prepares making driver
core remove functions return void.

Best regards
Uwe

Uwe Kleine-König (3):
  drm/panel: simple: Make panel_simple_remove() return void
  drm/panel-novatek-nt35510: Emit an error message if power off fails
  drm/mipi-dsi: Make remove callback return void

 drivers/gpu/drm/bridge/chipone-icn6211.c |  4 +---
 drivers/gpu/drm/bridge/tc358762.c|  4 +---
 drivers/gpu/drm/bridge/tc358764.c|  4 +---
 drivers/gpu/drm/drm_mipi_dsi.c   |  4 +++-
 drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c |  4 +---
 drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c   |  4 +---
 drivers/gpu/drm/panel/panel-boe-himax8279d.c |  4 +---
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c   |  4 +---
 drivers/gpu/drm/panel/panel-dsi-cm.c |  4 +---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c |  4 +---
 drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c|  4 +---
 .../gpu/drm/panel/panel-feiyang-fy07024di26a30d.c|  4 +---
 drivers/gpu/drm/panel/panel-ilitek-ili9881c.c|  4 +---
 drivers/gpu/drm/panel/panel-innolux-p079zca.c|  4 +---
 drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c |  4 +---
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c   |  4 +---
 drivers/gpu/drm/panel/panel-khadas-ts050.c   |  4 +---
 drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c   |  4 +---
 drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c   |  4 +---
 drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c   |  4 +---
 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c |  4 +---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c|  7 ---
 drivers/gpu/drm/panel/panel-novatek-nt35560.c|  4 +---
 drivers/gpu/drm/panel/panel-novatek-nt35950.c|  4 +---
 drivers/gpu/drm/panel/panel-novatek-nt36672a.c   |  4 +---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c |  4 +---
 drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c   |  4 +---
 drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c |  4 +---
 drivers/gpu/drm/panel/panel-raydium-rm67191.c|  4 +---
 drivers/gpu/drm/panel/panel-raydium-rm68200.c|  4 +---
 drivers/gpu/drm/panel/panel-ronbo-rb070d30.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6d16d0.c|  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c|  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c|  3 +--
 .../gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c |  4 +---
 drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c|  4 +---
 drivers/gpu/drm/panel/panel-samsung-sofef00.c|  4 +---
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c  |  6 ++
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c  |  4 +---
 drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c  |  4 +---
 drivers/gpu/drm/panel/panel-simple.c | 12 ++--
 drivers/gpu/drm/panel/panel-sitronix-st7701.c|  4 +---
 drivers/gpu/drm/panel/panel-sitronix-st7703.c|  4 +---
 .../gpu/drm/panel/panel-sony-tulip-truly-nt35521.c   |  4 +---
 drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c |  4 +---
 drivers/gpu/drm/panel/panel-truly-nt35597.c  |  3 +--
 drivers/gpu/drm/panel/panel-visionox-rm69299.c   |  3 +--
 drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c |  4 +---
 include/drm/drm_mipi_dsi.h   |  2 +-
 50 files changed, 61 insertions(+), 147 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

[PATCH 2/3] drm/panel-novatek-nt35510: Emit an error message if power off fails

[Uwe Kleine-König]

Returning an error code from a mipi_dsi remove callback fails, this is
silently ignored. (mipi_dsi_drv_remove() propagates the return value to
device_remove() which ignores it.) So emit an error code in the driver
remove function and return 0.

Signed-off-by: Uwe Kleine-König 
---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c 
b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
index 873cbd38e6d3..672e49ced240 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
@@ -974,9 +974,12 @@ static int nt35510_remove(struct mipi_dsi_device *dsi)
mipi_dsi_detach(dsi);
/* Power off */
ret = nt35510_power_off(nt);
+   if (ret)
+   dev_err(&dsi->dev, "Failed to power off\n");
+
drm_panel_remove(&nt->panel);
 
-   return ret;
+   return 0;
 }
 
 /*
-- 
2.36.1

Re: [PATCH 3/3] drm/mipi-dsi: Make remove callback return void

[Uwe Kleine-König]

Hello Sam,

On Sat, Jul 09, 2022 at 10:50:55AM +0200, Sam Ravnborg wrote:
> On Fri, Jul 08, 2022 at 11:49:22AM +0200, Uwe Kleine-König wrote:
> > All implementations return 0 and the return value of mipi_dsi_drv_remove()
> > is ignored anyhow.
> > 
> > So change the prototype of the remove function to return no value. This
> > way driver authors are not tempted to assume that passing an error to
> > the upper layer is a good idea. All drivers are adapted accordingly.
> > There is no intended change of behaviour.
> > 
> > Signed-off-by: Uwe Kleine-König 
> 
> Reviewed and applied to drm-misc (drm-misc-next).
> While applying updating panel-ebbg-ft8719

Good catch, at one time during creation of this series I intended to
check next and hint about such requirements. Great you noticed that on
your own, thanks!

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH v2] drm: Only select I2C_ALGOBIT for drivers that actually need it

[Uwe Kleine-König]

While working on a drm driver that doesn't need the i2c algobit stuff I
noticed that DRM selects this code even tough only 8 drivers actually use
it. While also only some drivers use i2c, keep the select for I2C for the
next cleanup patch. Still prepare this already by also selecting I2C for
the individual drivers.

Signed-off-by: Uwe Kleine-König 
---
Changes since v1
(20210514100142.1182997-1-u.kleine-koe...@pengutronix.de) from
2021-05-14:

 - rebased to next-20220909
   was something around v5.13-rc2 before, required to fix context
   changes in the nouveau Kconfig file. git am -3 handled it just fine.

I reverified that no new drivers were added that need a corresponding
select.

Best regards
Uwe

 drivers/gpu/drm/Kconfig | 5 -
 drivers/gpu/drm/ast/Kconfig | 2 ++
 drivers/gpu/drm/gma500/Kconfig  | 2 ++
 drivers/gpu/drm/hisilicon/hibmc/Kconfig | 2 ++
 drivers/gpu/drm/i915/Kconfig| 2 ++
 drivers/gpu/drm/mgag200/Kconfig | 2 ++
 drivers/gpu/drm/nouveau/Kconfig | 2 ++
 7 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 198ba846d34b..593d7335b10a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -13,7 +13,6 @@ menuconfig DRM
select HDMI
select FB_CMDLINE
select I2C
-   select I2C_ALGOBIT
select DMA_SHARED_BUFFER
select SYNC_FILE
 # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
@@ -231,6 +230,8 @@ config DRM_RADEON
 select DRM_KMS_HELPER
 select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
select POWER_SUPPLY
select HWMON
select BACKLIGHT_CLASS_DEVICE
@@ -262,6 +263,8 @@ config DRM_AMDGPU
select DRM_SCHED
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
select POWER_SUPPLY
select HWMON
select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
index fbcf2f45cef5..bcc25decd485 100644
--- a/drivers/gpu/drm/ast/Kconfig
+++ b/drivers/gpu/drm/ast/Kconfig
@@ -6,6 +6,8 @@ config DRM_AST
select DRM_VRAM_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
 Say yes for experimental AST GPU driver. Do not enable
 this driver without having a working -modesetting,
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index 807b989e3c77..2efc0eb41c64 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -3,6 +3,8 @@ config DRM_GMA500
tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
depends on DRM && PCI && X86 && MMU
select DRM_KMS_HELPER
+   select I2C
+   select I2C_ALGOBIT
# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
select ACPI_VIDEO if ACPI
select BACKLIGHT_CLASS_DEVICE if ACPI
diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig 
b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
index 073adfe438dd..90b9e6cce49c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
+++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
@@ -6,6 +6,8 @@ config DRM_HISI_HIBMC
select DRM_VRAM_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
  Choose this option if you have a Hisilicon Hibmc soc chipset.
  If M is selected the module will be called hibmc-drm.
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 3efce05d7b57..c6e3792622f2 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -18,6 +18,8 @@ config DRM_I915
select DRM_PANEL
select DRM_MIPI_DSI
select RELAY
+   select I2C
+   select I2C_ALGOBIT
select IRQ_WORK
# i915 depends on ACPI_VIDEO when ACPI is enabled
# but for select to work, need to select ACPI_VIDEO's dependencies, ick
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index eec59658a938..b28c5e4828f4 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -4,6 +4,8 @@ config DRM_MGAG200
depends on DRM && PCI && MMU
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
+   select I2C
+   select I2C_ALGOBIT
help
 This is a KMS driver for Matrox G200 chips. It supports the original
 MGA G200 desktop chips and the server variants. It requires 0.3.0
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 03d12caf9e26..a0bb3987bf63 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -10,6 +10,8 @@ config DRM_NOUVEAU
select DRM_KMS_HELPER
sel

[PATCH 3/3] pwm: Handle .get_state() failures

[Uwe Kleine-König]

This suppresses diagnosis for PWM_DEBUG routines and makes sure that
pwm->state isn't modified in pwm_device_request() if .get_state() fails.

Signed-off-by: Uwe Kleine-König 
---
 drivers/pwm/core.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 381db04cfa00..421573590613 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -108,9 +108,14 @@ static int pwm_device_request(struct pwm_device *pwm, 
const char *label)
}
 
if (pwm->chip->ops->get_state) {
-   err = pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
+   struct pwm_state state;
+
+   err = pwm->chip->ops->get_state(pwm->chip, pwm, &state);
trace_pwm_get(pwm, &pwm->state, err);
 
+   if (!err)
+   pwm->state = state;
+
if (IS_ENABLED(CONFIG_PWM_DEBUG))
pwm->last = pwm->state;
}
@@ -459,6 +464,9 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 
err = chip->ops->get_state(chip, pwm, &s1);
trace_pwm_get(pwm, &s1, err);
+   if (err)
+   /* If that failed there isn't much to debug */
+   return;
 
/*
 * The lowlevel driver either ignored .polarity (which is a bug) or as
@@ -523,6 +531,8 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 
err = chip->ops->get_state(chip, pwm, last);
trace_pwm_get(pwm, last, err);
+   if (err)
+   return;
 
/* reapplication of the current state should give an exact match */
if (s1.enabled != last->enabled ||
-- 
2.37.2

[PATCH 2/3] pwm/tracing: Also record trace events for failed apply calls

[Uwe Kleine-König]

Record and report an error code for the events. This allows to report
about failed calls without ambiguity and so gives a more complete
picture.

Signed-off-by: Uwe Kleine-König 
---
 drivers/pwm/core.c | 18 --
 include/trace/events/pwm.h | 20 ++--
 2 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 0e042410f6b9..381db04cfa00 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -108,8 +108,8 @@ static int pwm_device_request(struct pwm_device *pwm, const 
char *label)
}
 
if (pwm->chip->ops->get_state) {
-   pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
-   trace_pwm_get(pwm, &pwm->state);
+   err = pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
+   trace_pwm_get(pwm, &pwm->state, err);
 
if (IS_ENABLED(CONFIG_PWM_DEBUG))
pwm->last = pwm->state;
@@ -457,8 +457,8 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 * checks.
 */
 
-   chip->ops->get_state(chip, pwm, &s1);
-   trace_pwm_get(pwm, &s1);
+   err = chip->ops->get_state(chip, pwm, &s1);
+   trace_pwm_get(pwm, &s1, err);
 
/*
 * The lowlevel driver either ignored .polarity (which is a bug) or as
@@ -514,16 +514,15 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 
/* reapply the state that the driver reported being configured. */
err = chip->ops->apply(chip, pwm, &s1);
+   trace_pwm_apply(pwm, &s1, err);
if (err) {
*last = s1;
dev_err(chip->dev, "failed to reapply current setting\n");
return;
}
 
-   trace_pwm_apply(pwm, &s1);
-
-   chip->ops->get_state(chip, pwm, last);
-   trace_pwm_get(pwm, last);
+   err = chip->ops->get_state(chip, pwm, last);
+   trace_pwm_get(pwm, last, err);
 
/* reapplication of the current state should give an exact match */
if (s1.enabled != last->enabled ||
@@ -571,11 +570,10 @@ int pwm_apply_state(struct pwm_device *pwm, const struct 
pwm_state *state)
return 0;
 
err = chip->ops->apply(chip, pwm, state);
+   trace_pwm_apply(pwm, state, err);
if (err)
return err;
 
-   trace_pwm_apply(pwm, state);
-
pwm->state = *state;
 
/*
diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
index cf243de41cc8..12b35e4ff917 100644
--- a/include/trace/events/pwm.h
+++ b/include/trace/events/pwm.h
@@ -10,9 +10,9 @@
 
 DECLARE_EVENT_CLASS(pwm,
 
-   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state),
+   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int 
err),
 
-   TP_ARGS(pwm, state),
+   TP_ARGS(pwm, state, err),
 
TP_STRUCT__entry(
__field(struct pwm_device *, pwm)
@@ -20,6 +20,7 @@ DECLARE_EVENT_CLASS(pwm,
__field(u64, duty_cycle)
__field(enum pwm_polarity, polarity)
__field(bool, enabled)
+   __field(int, err)
),
 
TP_fast_assign(
@@ -28,28 +29,27 @@ DECLARE_EVENT_CLASS(pwm,
__entry->duty_cycle = state->duty_cycle;
__entry->polarity = state->polarity;
__entry->enabled = state->enabled;
+   __entry->err = err;
),
 
-   TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d",
+   TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d 
err=%d",
  __entry->pwm, __entry->period, __entry->duty_cycle,
- __entry->polarity, __entry->enabled)
+ __entry->polarity, __entry->enabled, __entry->err)
 
 );
 
 DEFINE_EVENT(pwm, pwm_apply,
 
-   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state),
-
-   TP_ARGS(pwm, state)
+   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int 
err),
 
+   TP_ARGS(pwm, state, err)
 );
 
 DEFINE_EVENT(pwm, pwm_get,
 
-   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state),
-
-   TP_ARGS(pwm, state)
+   TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int 
err),
 
+   TP_ARGS(pwm, state, err)
 );
 
 #endif /* _TRACE_PWM_H */
-- 
2.37.2