Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6cc2948c-04eb-d62b-dfa0-5a94544ea6c9@gmail.com>
Date:   Sat, 29 Oct 2022 09:45:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH] soc: ti: k3-ringacc: Allow the driver to be built as
 module
To:     Nishanth Menon <nm@ti.com>
Cc:     ssantosh@kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, khilman@baylibre.com,
        nfrayer@baylibre.com
References: <20221021080231.1076-1-peter.ujfalusi@gmail.com>
 <20221028005521.gzdo46oz65cxlthd@recent>
Content-Language: en-US
From:   =?UTF-8?Q?P=c3=a9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>
In-Reply-To: <20221028005521.gzdo46oz65cxlthd@recent>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/28/22 03:55, Nishanth Menon wrote:
>> @@ -336,6 +336,11 @@ struct k3_ring *k3_ringacc_request_ring(struct k3_ringacc *ringacc,
>>  
>>  	mutex_lock(&ringacc->req_lock);
>>  
>> +	if (!try_module_get(ringacc->dev->driver->owner)) {
> 
> Does a cascaded error handling make sense instead?
> 
> goto error_mod_fail?

I think it does not make the code nicer, but I can do that way

> 
>> +		mutex_unlock(&ringacc->req_lock);
>> +		return NULL;
>> +	}
>> +
>>  	if (id == K3_RINGACC_RING_ID_ANY) {
>>  		/* Request for any general purpose ring */
>>  		struct ti_sci_resource_desc *gp_rings =
>> @@ -380,6 +385,7 @@ struct k3_ring *k3_ringacc_request_ring(struct k3_ringacc *ringacc,
>>  	return &ringacc->rings[id];
>>  
>>  error:
>> +	module_put(ringacc->dev->driver->owner);
> 
> error_mod_fail:
> 
>>  	mutex_unlock(&ringacc->req_lock);
>>  	return NULL;
>>  }
>> @@ -616,6 +622,8 @@ int k3_ringacc_ring_free(struct k3_ring *ring)
>>  no_init:
>>  	clear_bit(ring->ring_id, ringacc->rings_inuse);
>>  
>> +	module_put(ringacc->dev->driver->owner);
>> +
>>  out:
>>  	mutex_unlock(&ringacc->req_lock);
>>  	return 0;
>> @@ -1450,6 +1458,7 @@ static const struct of_device_id k3_ringacc_of_match[] = {
>>  	{ .compatible = "ti,am654-navss-ringacc", .data = &k3_ringacc_data, },
>>  	{},
>>  };
>> +MODULE_DEVICE_TABLE(of, k3_ringacc_of_match);
>>  
>>  struct k3_ringacc *k3_ringacc_dmarings_init(struct platform_device *pdev,
>>  					    struct k3_ringacc_init_data *data)
>> @@ -1544,12 +1553,26 @@ static int k3_ringacc_probe(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static int k3_ringacc_remove(struct platform_device *pdev)
>> +{
>> +	struct k3_ringacc *ringacc = dev_get_drvdata(&pdev->dev);
>> +
>> +	mutex_lock(&k3_ringacc_list_lock);
>> +	list_del(&ringacc->list);
>> +	mutex_unlock(&k3_ringacc_list_lock);
>> +	return 0;
>> +}
>> +
>>  static struct platform_driver k3_ringacc_driver = {
>>  	.probe		= k3_ringacc_probe,
>> +	.remove		= k3_ringacc_remove,
>>  	.driver		= {
>>  		.name	= "k3-ringacc",
>>  		.of_match_table = k3_ringacc_of_match,
>> -		.suppress_bind_attrs = true,
> 
> Might be good to note sysfs behavior change in commit log?

Since the udma driver kept this I will keep it here as well.

> 
>>  	},
>>  };
>> -builtin_platform_driver(k3_ringacc_driver);
>> +module_platform_driver(k3_ringacc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
> 
> GPL ? checkpatch should have complained?

Yes, it complains if I run it ;)

> 
>> +MODULE_DESCRIPTION("TI Ringacc driver for K3 SOCs");
>> +MODULE_AUTHOR("Grygorii Strashko <grygorii.strashko@ti.com>");
>> -- 
>> 2.38.1
>>
> 

-- 
Péter