| Message ID | 1472729956-17475-1-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote: > Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a > ("i2c: let I2C masters ignore their children for PM") > the power.ignore_children flag is set when registering an I2C > adapter. Since I2C transfers are not managed by the fimc-isp-i2c > driver its clients use pm_runtime_* calls directly to communicate > required power state of the bus controller. > However when the power.ignore_children flag is set that doesn't > work, so clear that flag back after registering the adapter. > While at it drop pm_runtime_enable() call on the i2c_adapter > as it is already done by the I2C subsystem when registering > I2C adapter. > > Cc: <stable@vger.kernel.org> # 4.7+ > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> CCing the authors of the "offending" commit as well as linux-pm for more PM expertise. > --- > drivers/media/platform/exynos4-is/fimc-is-i2c.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c b/drivers/media/platform/exynos4-is/fimc-is-i2c.c > index 7521aa5..03b4246 100644 > --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c > +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c > @@ -55,26 +55,37 @@ static int fimc_is_i2c_probe(struct platform_device *pdev) > i2c_adap->algo = &fimc_is_i2c_algorithm; > i2c_adap->class = I2C_CLASS_SPD; > > + platform_set_drvdata(pdev, isp_i2c); > + pm_runtime_enable(&pdev->dev); > + > ret = i2c_add_adapter(i2c_adap); > if (ret < 0) { > dev_err(&pdev->dev, "failed to add I2C bus %s\n", > node->full_name); > - return ret; > + goto err_pm_dis; > } > > - platform_set_drvdata(pdev, isp_i2c); > - > - pm_runtime_enable(&pdev->dev); > - pm_runtime_enable(&i2c_adap->dev); > - > + /* > + * Client drivers of this adapter don't do any I2C transfers as that > + * is handled by the ISP firmware. But we rely on the runtime PM > + * state propagation from the clients up to the adapter driver so > + * clear the ignore_children flags here. PM rutnime calls are not > + * used in probe() handler of clients of this adapter so there is > + * no issues with clearing the flag right after registering the I2C > + * adapter. > + */ > + pm_suspend_ignore_children(&i2c_adap->dev, false); > return 0; > + > +err_pm_dis: > + pm_runtime_disable(&pdev->dev); > + return ret; > } > > static int fimc_is_i2c_remove(struct platform_device *pdev) > { > struct fimc_is_i2c *isp_i2c = platform_get_drvdata(pdev); > > - pm_runtime_disable(&isp_i2c->adapter.dev); > pm_runtime_disable(&pdev->dev); > i2c_del_adapter(&isp_i2c->adapter); > > -- > 1.9.1 >
Hi, On Thursday, September 01, 2016 01:39:16 PM Sylwester Nawrocki wrote: > Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a > ("i2c: let I2C masters ignore their children for PM") > the power.ignore_children flag is set when registering an I2C > adapter. Since I2C transfers are not managed by the fimc-isp-i2c > driver its clients use pm_runtime_* calls directly to communicate > required power state of the bus controller. > However when the power.ignore_children flag is set that doesn't > work, so clear that flag back after registering the adapter. > While at it drop pm_runtime_enable() call on the i2c_adapter > as it is already done by the I2C subsystem when registering > I2C adapter. > > Cc: <stable@vger.kernel.org> # 4.7+ You may also use "Fixes:" tag to mark the original commit that this one corrects. > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/media/platform/exynos4-is/fimc-is-i2c.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c b/drivers/media/platform/exynos4-is/fimc-is-i2c.c > index 7521aa5..03b4246 100644 > --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c > +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c > @@ -55,26 +55,37 @@ static int fimc_is_i2c_probe(struct platform_device *pdev) > i2c_adap->algo = &fimc_is_i2c_algorithm; > i2c_adap->class = I2C_CLASS_SPD; > > + platform_set_drvdata(pdev, isp_i2c); > + pm_runtime_enable(&pdev->dev); > + > ret = i2c_add_adapter(i2c_adap); > if (ret < 0) { > dev_err(&pdev->dev, "failed to add I2C bus %s\n", > node->full_name); > - return ret; > + goto err_pm_dis; > } > > - platform_set_drvdata(pdev, isp_i2c); > - > - pm_runtime_enable(&pdev->dev); > - pm_runtime_enable(&i2c_adap->dev); > - > + /* > + * Client drivers of this adapter don't do any I2C transfers as that > + * is handled by the ISP firmware. But we rely on the runtime PM > + * state propagation from the clients up to the adapter driver so > + * clear the ignore_children flags here. PM rutnime calls are not Minor nit: "rutnime" typo Otherwise it looks all fine to me. Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 1, 2016 at 1:47 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote: >> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a >> ("i2c: let I2C masters ignore their children for PM") >> the power.ignore_children flag is set when registering an I2C >> adapter. Since I2C transfers are not managed by the fimc-isp-i2c >> driver its clients use pm_runtime_* calls directly to communicate >> required power state of the bus controller. >> However when the power.ignore_children flag is set that doesn't >> work, so clear that flag back after registering the adapter. >> While at it drop pm_runtime_enable() call on the i2c_adapter >> as it is already done by the I2C subsystem when registering >> I2C adapter. >> >> Cc: <stable@vger.kernel.org> # 4.7+ >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> I understand what the patch is doing but not this commit message. What does it mean when you say "Since I2C transfers are not managed by the fimc-isp-i2c driver its clients use pm_runtime_* calls directly to communicate required power state of the bus controller."? I find it very hard to understand. The intent of the commit is to decouple I2C slave devices' runtime PM state from their parents, so say a gyroscope on an I2C bus does not have to bring up it's host controller to be active, for example it usually has an IRQ line to wake up the driver and that will talk using I2C and the I2C traffic will wake up the I2C master. When I look at the driver it appears it is not even used for I2C traffic, just to take the clocks up and down and make it possible to manage a clock using runtime PM and interface with the DT logic... so I guess since it's likely and odd one-off and the driver is sufficiently weird anyways, it's fine to merge this patch making it even weirder. Maybe a sort of mock adapter type should actually be created in the I2C core for these things so it can be handled there but who am I to say. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/07/2016 10:38 PM, Linus Walleij wrote: > On Thu, Sep 1, 2016 at 1:47 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote: > >>> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a >>> ("i2c: let I2C masters ignore their children for PM") >>> the power.ignore_children flag is set when registering an I2C >>> adapter. Since I2C transfers are not managed by the fimc-isp-i2c >>> driver its clients use pm_runtime_* calls directly to communicate >>> required power state of the bus controller. >>> However when the power.ignore_children flag is set that doesn't >>> work, so clear that flag back after registering the adapter. >>> While at it drop pm_runtime_enable() call on the i2c_adapter >>> as it is already done by the I2C subsystem when registering >>> I2C adapter. >>> >>> Cc: <stable@vger.kernel.org> # 4.7+ >>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > I understand what the patch is doing but not this commit message. > > What does it mean when you say "Since I2C transfers are not > managed by the fimc-isp-i2c driver its clients use pm_runtime_* > calls directly to communicate required power state of the bus > controller."? > > I find it very hard to understand. When a video device node is opened the I2C client (v4l2 subdevice) in its s_power() handler makes pm_runtime_get/pm_runtime_put calls on its corresponding i2c_client. It's true we use an imitation of I2C bus adapter just to make things work with standard DT bindings and except of device instantiation this adapter's task is just to control the clock. I admit that it's all an odd use case, it comes from the fact that the image sensor devices are handled in the ISP's firmware, where normally we have an I2C client driver for each sensor in V4L2. Now, I don't like those pm_runtime_* calls in the sensor drivers as that's a non standard thing. The other option to fix the regression which I have been considering is to remove the whole PM runtime dependency and make the driver of the I2C controller's parent device [1] handle the clocks, as it's done with clocks of some other IP blocks in that whole camera subsystem. Probably it's the lesser of two evils. > The intent of the commit is to decouple I2C slave devices' > runtime PM state from their parents, so say a gyroscope on an > I2C bus does not have to bring up it's host controller to be > active, for example it usually has an IRQ line to wake up > the driver and that will talk using I2C and the I2C traffic will > wake up the I2C master. > > When I look at the driver it appears it is not even used for > I2C traffic, just to take the clocks up and down and make it > possible to manage a clock using runtime PM and interface > with the DT logic... so I guess since it's likely and odd one-off > and the driver is sufficiently weird anyways, it's fine to merge > this patch making it even weirder. Yeah, it's an odd one-off, there is no other drivers in mainline with such a "non standard" split of hardware control responsibility between main CPU and the ISP's MCU. I'd merge the $subject patch as a regression fix for 4.7 and 4.8 and then think about a little larger patch moving the clock handling responsibility the the ISP_I2C controller's parent driver. > Maybe a sort of mock adapter type should actually be > created in the I2C core for these things so it can be handled > there but who am I to say. Not sure if we need it right now, probably if there is more such use cases we could think about something like this. [1] http://lxr.linux.no/#linux+v4.7.3/arch/arm/boot/dts/exynos4x12.dtsi#L148
diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c b/drivers/media/platform/exynos4-is/fimc-is-i2c.c index 7521aa5..03b4246 100644 --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c @@ -55,26 +55,37 @@ static int fimc_is_i2c_probe(struct platform_device *pdev) i2c_adap->algo = &fimc_is_i2c_algorithm; i2c_adap->class = I2C_CLASS_SPD; + platform_set_drvdata(pdev, isp_i2c); + pm_runtime_enable(&pdev->dev); + ret = i2c_add_adapter(i2c_adap); if (ret < 0) { dev_err(&pdev->dev, "failed to add I2C bus %s\n", node->full_name); - return ret; + goto err_pm_dis; } - platform_set_drvdata(pdev, isp_i2c); - - pm_runtime_enable(&pdev->dev); - pm_runtime_enable(&i2c_adap->dev); - + /* + * Client drivers of this adapter don't do any I2C transfers as that + * is handled by the ISP firmware. But we rely on the runtime PM + * state propagation from the clients up to the adapter driver so + * clear the ignore_children flags here. PM rutnime calls are not + * used in probe() handler of clients of this adapter so there is + * no issues with clearing the flag right after registering the I2C + * adapter. + */ + pm_suspend_ignore_children(&i2c_adap->dev, false); return 0; + +err_pm_dis: + pm_runtime_disable(&pdev->dev); + return ret; } static int fimc_is_i2c_remove(struct platform_device *pdev) { struct fimc_is_i2c *isp_i2c = platform_get_drvdata(pdev); - pm_runtime_disable(&isp_i2c->adapter.dev); pm_runtime_disable(&pdev->dev); i2c_del_adapter(&isp_i2c->adapter);
Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a ("i2c: let I2C masters ignore their children for PM") the power.ignore_children flag is set when registering an I2C adapter. Since I2C transfers are not managed by the fimc-isp-i2c driver its clients use pm_runtime_* calls directly to communicate required power state of the bus controller. However when the power.ignore_children flag is set that doesn't work, so clear that flag back after registering the adapter. While at it drop pm_runtime_enable() call on the i2c_adapter as it is already done by the I2C subsystem when registering I2C adapter. Cc: <stable@vger.kernel.org> # 4.7+ Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- drivers/media/platform/exynos4-is/fimc-is-i2c.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)