Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 6 Oct 2021 11:29:03 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>
Cc:     sboyd@kernel.org, linux-clk@vger.kernel.org,
        gregkh@linuxfoundation.org, devicetree@vger.kernel.org,
        robh+dt@kernel.org, linux-staging@lists.linux.dev, neil@brown.name,
        linux-kernel@vger.kernel.org, john@phrozen.org
Subject: Re: [PATCH 3/4] clk: ralink: make system controller node a reset
 provider
Message-ID: <20211006082903.GZ2048@kadam>
References: <20211006061204.2854-1-sergio.paracuellos@gmail.com>
 <20211006061204.2854-4-sergio.paracuellos@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211006061204.2854-4-sergio.paracuellos@gmail.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?fk+VThJnp1xV70vd1AurqBuclylZKblmuf76lDH6SOM3bk3M6YRF/ir3GZAT?=
 =?us-ascii?Q?wDzI8m+EvWWqzTPH0jdnA79jJmGH0pFSqlj44fi5kTurdFMvGZcFBCsftwmd?=
 =?us-ascii?Q?DEor9di+bXPwAj6YpwcXZfOinxkfpIJDKgracRCiIJQuNu2FtnivrUhxXGMs?=
 =?us-ascii?Q?RpPpDLuxssQ2CsUKpy39797pakcIf2T8f6C9WGtqzzyTnpbFKJdwG4TYrAnc?=
 =?us-ascii?Q?kpXrPu9lNyw2bGk/1O4XTEDdzf1HxUCFYhM95b3VarrSCXTc6Va81GjcsmyO?=
 =?us-ascii?Q?sC/35f08AeCuhOHDv1XRkkkSQhroqNE5NfR/4oJawxGiXWFuQWrKEcxY1zSj?=
 =?us-ascii?Q?WcAZsfCLLPZy4DRHXf2IY8flSGd9maSuR2wvF7Kn/T+vfOZGmHVkCbsVL1EZ?=
 =?us-ascii?Q?RComdMpo6BFA6iZ53zUc/+wtyjnEO24wn7q/SzxeuueVEKoefsQ4dlkYwblI?=
 =?us-ascii?Q?7ilmwhRJ6mhqruAIBaAab5W5EQPkPBOx6COsq+azJaKDiFhLmXagLjJ7tl+S?=
 =?us-ascii?Q?epATPMKn8euY2hzlP+M7AeKQMHb8dfUOHYiyqJDhABO9Zy5iH4zibvfa9ZaL?=
 =?us-ascii?Q?knLaxMbASW7AF45quAFPgbpS3Roxr6/t0ythuejOAQFvPIFE8y05Sm9VMgQx?=
 =?us-ascii?Q?hPKV0vlffk6WwGddFvwA5iAfGveuh8DQZTPB8GDu1rsZ5f3A322Uh3BMsUWw?=
 =?us-ascii?Q?ePGHkgeq+f93ZvmJulJ+OnnVN2ysqVLN0ahPWXTUsbtdwm9M18XAsCQF1zKK?=
 =?us-ascii?Q?hE8ASF56XhBrA7yWY2QX19HZfgvBIp3Dw8gSZZWimplUw1EqckJH97xgyHfp?=
 =?us-ascii?Q?zJWOeHjuzfWTeK0jfGmcR4RsrBYrERIVO+fxiL0l6wUvbs6zqA7o3SxmcH3C?=
 =?us-ascii?Q?VEeTYyH0CpoZQ+4z33oSHNkxnE4ccyx191RbVr1U1lDuDk3GoZZD2J8Uip7o?=
 =?us-ascii?Q?Yv945YWOKnLdNlf0URYviJl9vYRf+rcKYf7iYKoT4HN5G/84KxD4k7gsB2nz?=
 =?us-ascii?Q?0Y2B794kTm3qb42MxZf2ujYQQh7WYObdELBcm2CarkE6vjzGzrVvflejGmns?=
 =?us-ascii?Q?0ClROYs4BsoX/W9MJmcHtF/1r/cwRpl93NFcTKAfMyEgBdjgtfKFk4kEYGXn?=
 =?us-ascii?Q?y/K6Dpj0LEX21uZ4p6KvweXxWdffBjWLi3cwpC4zWmL2ODhc+BDFHXvMTmZw?=
 =?us-ascii?Q?FnRpntEF+2E16b/TuLH1FTMnuIKhzhyGs6w7ATQNP0PD6NfMejcSCp5T+i20?=
 =?us-ascii?Q?c9Lk47etQqYXPOMhZBUlpbeTHADvJWAGQwYK5AR1HyA4t/BIRzbBheSvhAdr?=
 =?us-ascii?Q?0OvIYNqd0lzZxlm1ndWgW+Gt?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4de7eb57-39d1-4b96-713e-08d988a365b8
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Oct 2021 08:29:22.7053
 (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: Sw4iSX+F1cuSAS0IpzhA23qIOdpQmHCaTXslS2a1sx5JLnVI32aX/T6cHeIbUibxKOufbMm9/WUZqt6NE2tbudZXsC2h1Z1j7GXeNSV3Dco=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR10MB5789
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10128 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 adultscore=0
 bulkscore=0 suspectscore=0 malwarescore=0 spamscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2110060052
X-Proofpoint-ORIG-GUID: TWToqgPkw8zbw3ONeb_I--p-t8VB-Fwc
X-Proofpoint-GUID: TWToqgPkw8zbw3ONeb_I--p-t8VB-Fwc
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote:
> @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
>  }
>  CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
>  
> +struct mt7621_rst {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *sysc;
> +};
> +
> +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)

No need to mark this as inline.  The compiler should do it automatically
or it will ignore the inline.

> +{
> +	return container_of(dev, struct mt7621_rst, rcdev);
> +}
> +
> +static int mt7621_assert_device(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	struct mt7621_rst *data = to_mt7621_rst(rcdev);
> +	struct regmap *sysc = data->sysc;
> +
> +	if (id == MT7621_RST_SYS)
> +		return -1;

Please, return proper error codes.

> +
> +	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id));
> +}
> +
> +static int mt7621_deassert_device(struct reset_controller_dev *rcdev,
> +				  unsigned long id)
> +{
> +	struct mt7621_rst *data = to_mt7621_rst(rcdev);
> +	struct regmap *sysc = data->sysc;
> +
> +	if (id == MT7621_RST_SYS)
> +		return -1;

Here too.

> +
> +	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0);
> +}
> +
> +static int mt7621_reset_device(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	int ret;
> +
> +	ret = mt7621_assert_device(rcdev, id);
> +	if (ret < 0)
> +		return ret;
> +
> +	return mt7621_deassert_device(rcdev, id);
> +}
> +
> +static const struct reset_control_ops reset_ops = {
> +	.reset = mt7621_reset_device,
> +	.assert = mt7621_assert_device,
> +	.deassert = mt7621_deassert_device
> +};
> +
> +static int mt7621_reset_init(struct device *dev, struct regmap *sysc)
> +{
> +	struct mt7621_rst *rst_data;
> +
> +	rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL);


Can we use devm_ to allocate this or do we need to clean up if
devm_reset_controller_register() fails?  Also a free in the release
function I suppose.  (Please, use devm_).


> +	if (!rst_data)
> +		return -ENOMEM;
> +
> +	rst_data->sysc = sysc;
> +	rst_data->rcdev.ops = &reset_ops;
> +	rst_data->rcdev.owner = THIS_MODULE;
> +	rst_data->rcdev.nr_resets = 32;
> +	rst_data->rcdev.of_reset_n_cells = 1;
> +	rst_data->rcdev.of_node = dev_of_node(dev);
> +
> +	return devm_reset_controller_register(dev, &rst_data->rcdev);
> +}


regards,
dan carpenter