Date:   Tue, 23 Nov 2021 17:59:33 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc:     Jernej Skrabec <jernej.skrabec@gmail.com>,
        linux-media@vger.kernel.org, ezequiel@vanguardiasur.com.ar,
        nicolas.dufresne@collabora.com, mchehab@kernel.org,
        robh+dt@kernel.org, mripard@kernel.org, wens@csie.org,
        p.zabel@pengutronix.de, gregkh@linuxfoundation.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [PATCH 1/7] media: hantro: add support for reset lines
Message-ID: <20211123145933.GD6514@kadam>
References: <20211122184702.768341-1-jernej.skrabec@gmail.com>
 <20211122184702.768341-2-jernej.skrabec@gmail.com>
 <08e80e9e-f646-72e4-e4d4-f8e6310228f1@collabora.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <08e80e9e-f646-72e4-e4d4-f8e6310228f1@collabora.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mZh9udMV8Me03n7nWB5Hcj5AhGMALehe5RojLZot34qQsueZyknnNo30vEMr?=
 =?us-ascii?Q?qV6tyW+HfMgWi4NlyS5076WgTGvKUclAQovqwSCGLPuo/XuBehRN+exD5H8I?=
 =?us-ascii?Q?ybDkheiw4wmaILASDrqyx+wrH8uMDZ/70jrVeoSoIG9a8ypEcuQlg9MIcWSv?=
 =?us-ascii?Q?T5ELWLB2kt1/pYSFTURBV2M4ijrmwnUSi8NGPbbYRd40t7yfFjPvo91uI2ZK?=
 =?us-ascii?Q?x6qalq4vH5zFUv1z82pjmL8djUxvSg0O3elOvcgxxJHZuMKR1PAMJMT45XnE?=
 =?us-ascii?Q?eeta47a82dNMwxUfsGG74IMHLZ2Ukwbn3kDilTI044giV+558F3XJIRSIFYg?=
 =?us-ascii?Q?76XHU5lIpBYWueSIOBIVYfydsFh23dslnn9F0EILqiUOfoP3xPBL4gwzlNn4?=
 =?us-ascii?Q?Re6WmbwgEErT3LrT1jrDFklq3iNilXMIMULkOvLOwpa3K1/dsQGCAFJzuA4/?=
 =?us-ascii?Q?EXBzzEKdbhmFTcvlLRnXQBD2OTksgcVm+tU9eNfiPbh68IK8F8niEbw9Kpul?=
 =?us-ascii?Q?pd78KkkZbDmfWxbmU58fe5tnDGzPoCBKMzNDnyOwhgRHj2J1WNnrOEwZpKTd?=
 =?us-ascii?Q?nXQQvioE6yo3a19a31q7TXp1b6wM8lzU9Sbvz/DvnaDlG7OqLQz2K8oicaYV?=
 =?us-ascii?Q?S9xFMfdndQDZo26A7MHcADBOpGdqr9vn42FkYeoaopvFoKrNC8+vpJ2dXw4S?=
 =?us-ascii?Q?P46RzNP9G6JUSYGrd5jhbwshr7keg0ZJaKzjmpAhdf4IVYGVazOn4D7kcA9B?=
 =?us-ascii?Q?L/jpD38HvnAihuNm0etk+N3lAlJcEX0JxTj/Aibqqm8Oq01ncttDOFRGRa/c?=
 =?us-ascii?Q?qJW0WF8sPllBPv1H+AeCXpfhc5JNqcgz4B/7luQEoMOBGot+wNamWm+yWAyA?=
 =?us-ascii?Q?hLLdOwDeubpOBmK4pPNkdRH+rU0czndXuxhttAcjq+byZtpeW+hSFI0+xDJ8?=
 =?us-ascii?Q?7Qi7aCNHrmaZvdSB2nyvOK4CgbqfYjIWCWkgbGLdjIN6lnj/r7uS2HyTIWn8?=
 =?us-ascii?Q?IWjrwT3GbybyEotF48HGtrVPrByl70axNP+pIk6xHOCSqSq6jka9hkUoCUJ6?=
 =?us-ascii?Q?mFM1/2jvFN6BWPFTq7EFDBVkUSF/80LvMC/3T0i8OIExUTRH7dwGjQgGI1lu?=
 =?us-ascii?Q?0I6WXBKlI3mEL0BtkewFMz4X7RnW/ROwOnz8DIixgmzrRczbADBPzoV7oV83?=
 =?us-ascii?Q?zMKfJJvB+Oba1Nyjw42lWLgK8OhelTq+dzsJOABRMqD6vZNX7iEBTKv+rxlz?=
 =?us-ascii?Q?+PYDeFCGoj+Bx0lyU4ZNj348x226P8koizJ1DkU3bJc1eN6lt5WVbs/rpL4e?=
 =?us-ascii?Q?kydONluUQxf8A+IVa5Dx3zmFvFI/lYzI2gLhl6Jn2FxLXe4y9gftKxeq0wxv?=
 =?us-ascii?Q?SK/ObX+eQpHZlSnmOH9+kiiVzVPckQee3cEA7Keonm4jk18xTl+8egeYk91z?=
 =?us-ascii?Q?vSl1bXO+MUdRf0gaDwAcUywSnnzDgNfHAxR0Yh4OoPS/ZGA9M0TT97R0LLXf?=
 =?us-ascii?Q?23+fFm3QELQ1UN4lGI+Mu8VtLLGigxWgox2tzjwp+Lklzej9SwgybsvrlPLC?=
 =?us-ascii?Q?KZYBXuWzjYicSN+42Oi9DUPaYTtq6UOmHTBw32Gg/M/7Nl3gnWtU7YiLnZ4T?=
 =?us-ascii?Q?BH9I0tUYtu7nuNeDjo3nfWTjTwQsrOhPVZlMHbxPAniiuODWIZzSEXfV2/Dl?=
 =?us-ascii?Q?SeRQjL43ZLy0PrbtlgYYxksamrc=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85dcb8f1-377c-412d-c40d-08d9ae91ec42
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Nov 2021 15:00:01.4640
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +cuwNouXJ49TQX7Anq/gO5ZCR/u848IvoyJsBU8SX7sbTiDZznQIz5VDS0rKBIybWwHz7BZoC5qBHlHOgGQeiMNLcAijKA5pnbzQd8dBVRk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1001MB2397
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10176 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 malwarescore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 suspectscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111230078
X-Proofpoint-GUID: 9bPVSsYJkYhwnaDOnHqJcxZIRzIPqd7C
X-Proofpoint-ORIG-GUID: 9bPVSsYJkYhwnaDOnHqJcxZIRzIPqd7C
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index ab2467998d29..8c3de31f51b3 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
> >   			return PTR_ERR(vpu->clocks[0].clk);
> >   	}
> > +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
> > +	if (IS_ERR(vpu->resets))
> > +		return PTR_ERR(vpu->resets);
> > +
> >   	num_bases = vpu->variant->num_regs ?: 1;
> >   	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
> >   				      sizeof(*vpu->reg_bases), GFP_KERNEL);
> > @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
> >   	pm_runtime_use_autosuspend(vpu->dev);
> >   	pm_runtime_enable(vpu->dev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
It looks like this is the pm stuff that we have to unwind on error

> > +	ret = reset_control_deassert(vpu->resets);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> > +		return ret;
                ^^^^^^^^^^
So this return should be a goto undo_pm_stuff


> > +	}
> > +
> >   	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> > -		return ret;

And this return should also have been a goto so it's a bug in the
original code.

> > +		goto err_rst_assert;
> 
> Before your patch is applied if clk_bulk_prepare() fails, we
> simply return on the spot. After the patch is applied not only
> do you...
> 
> >   	}
> >   	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> > @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
> >   	v4l2_device_unregister(&vpu->v4l2_dev);
> >   err_clk_unprepare:
> >   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > +err_rst_assert:
> > +	reset_control_assert(vpu->resets);
> 
> ...revert the effect of reset_control_deassert(), you also...
> 
> >   	pm_runtime_dont_use_autosuspend(vpu->dev);
> >   	pm_runtime_disable(vpu->dev);
> 
> ... do pm_*() stuff. Is there any reason why this is needed?

So, yes, it's needed, but you're correct to spot that it's not
consistent.

regards,
dan carpenter