Re: [PATCH 3/4] thermal: qcom: add qmi-cooling driver

From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Caleb Connolly <caleb.connolly@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Bhupesh Sharma <bhupesh.linux@gmail.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Amit Kucheria <amitk@kernel.org>, Zhang Rui <rui.zhang@intel.com>,
	Sibi Sankar <quic_sibis@quicinc.com>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Thara Gopinath <thara.gopinath@gmail.com>
Cc: linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH 3/4] thermal: qcom: add qmi-cooling driver
Date: Fri, 29 Sep 2023 18:28:49 +0200	[thread overview]
Message-ID: <30aa40c9-b63a-093c-954d-86e4bb232431@linaro.org> (raw)
In-Reply-To: <20230905-caleb-qmi_cooling-v1-3-5aa39d4164a7@linaro.org>


On 9/29/23 18:16, Caleb Connolly wrote:
> The Thermal Mitigation Device (TMD) service exposes various platform
> specific thermal mitigations available on remote subsystems (ie the
> modem, adsp, cdsp, and sdsp). The service is exposed via the QMI messaging
> interface, usually over the QRTR transport.
> 
> These controls affect things like the power limits of the modem power
> amplifier and cpu core, skin temperature mitigations, as well as rail
> voltage restrictions under cold conditions.
> 
> Each remote subsystem has its own TMD instance, where each child node
> represents a single thermal control.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
[...]

> +/* Notify the remote subsystem of the requested cooling state */
> +static int qmi_tmd_send_state_request(struct qmi_tmd *tmd)
> +{
> +	struct tmd_set_mitigation_level_resp_msg_v01 tmd_resp;
> +	struct tmd_set_mitigation_level_req_msg_v01 req;
> +	struct qmi_tmd_client *client;
> +	struct qmi_txn txn;
> +	int ret = 0;
> +
> +	client = tmd->client;
You can assign it at declaration time
> +
> +	if (!client->connection_active)
> +		return 0;
Is that an expected scenario?

> +
> +	memset(&req, 0, sizeof(req));
> +	memset(&tmd_resp, 0, sizeof(tmd_resp));
Since you're declaring them above, you can do = { 0 }; instead, which
will be faster

> +
> +	strscpy(req.mitigation_dev_id.mitigation_dev_id, tmd->qmi_name,
> +		QMI_TMD_MITIGATION_DEV_ID_LENGTH_MAX_V01 + 1);
> +	req.mitigation_level = tmd->cur_state;
> +
> +	mutex_lock(&client->mutex);
Look into fancy scoped mutexes https://lwn.net/Articles/934679/

[...]

> +static int qmi_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +	struct qmi_tmd *tmd = cdev->devdata;
> +
> +	if (!tmd)
> +		return -EINVAL;
> +
> +	if (state > tmd->max_state)
> +		return -EINVAL;Would it be useful if this threw an error in dmesg?


> + * When the QMI service starts up on a remote subsystem this function will fetch
> + * the list of TMDs on the subsystem, match it to the TMDs specified in devicetree
> + * and call qmi_tmd_init_control() for each
> + */
> +static void qmi_tmd_svc_arrive(struct work_struct *work)
> +{
> +	struct qmi_tmd_client *client =
> +		container_of(work, struct qmi_tmd_client, svc_arrive_work);
> +
> +	struct tmd_get_mitigation_device_list_req_msg_v01 req;
> +	struct tmd_get_mitigation_device_list_resp_msg_v01 *resp;
> +	int ret = 0, i;
> +	struct qmi_txn txn;
> +
> +	memset(&req, 0, sizeof(req));
= { 0 }

[...]

> +
> +	pr_info("QMI TMD service reset %s\n", client->name);
Is it useful to the user? pr_debug?

> +
> +	list_for_each_entry(tmd, &client->cdev_list, node) {
> +		qmi_tmd_send_state_request(tmd);
> +	}
> +}
> +
> +static void thermal_qmi_del_server(struct qmi_handle *qmi, struct qmi_service *service)
> +{
> +	struct qmi_tmd_client *client = container_of(qmi, struct qmi_tmd_client, handle);
> +
> +	pr_info("QMI TMD service stop %s\n", client->name);
Ditto

> +
> +	client->connection_active = false;
> +}
> +
> +static int thermal_qmi_new_server(struct qmi_handle *qmi, struct qmi_service *service)
> +{
> +	struct qmi_tmd_client *client = container_of(qmi, struct qmi_tmd_client, handle);
> +	struct sockaddr_qrtr sq = { AF_QIPCRTR, service->node, service->port };
> +
> +	pr_info("QMI TMD service start %s\n", client->name);
Ditto

> +		tmd->type = devm_kasprintf(client->dev, GFP_KERNEL, "%s:%s",
> +						client->name, subnode->name);
> +		if (!tmd->type)
> +			return dev_err_probe(dev, -ENOMEM, "Cooling device name\n");
Cooling device name-what? :D

> +
> +		if (of_property_read_string(subnode, "label", &name)) {
> +			return dev_err_probe(client->dev, -EINVAL,
> +					     "Fail to parse dev name for %s\n",
Failed

[...]

> +static int qmi_tmd_client_probe(struct platform_device *pdev)
> +{
> +	struct device *dev;
> +	struct qmi_tmd_client *client;
> +	const struct qmi_instance_id *match;
> +	int ret;
> +
> +	dev = &pdev->dev;
Initialize at declaration

> +
> +	client = devm_kzalloc(dev, sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->dev = dev;
> +
> +	match = of_device_get_match_data(dev);
> +	if (!match)
> +		return dev_err_probe(dev, -EINVAL, "No match data\n");
> +
> +	client->id = match->id;
> +	client->name = match->name;
> +
> +	mutex_init(&client->mutex);
> +	INIT_LIST_HEAD(&client->cdev_list);
> +	INIT_WORK(&client->svc_arrive_work, qmi_tmd_svc_arrive);
> +
> +	ret = qmi_tmd_alloc_cdevs(client);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, client);
> +
> +	ret = qmi_handle_init(&client->handle,
> +			      TMD_GET_MITIGATION_DEVICE_LIST_RESP_MSG_V01_MAX_MSG_LEN,
> +			      &thermal_qmi_event_ops, NULL);
> +	if (ret < 0)
> +		return dev_err_probe(client->dev, ret, "QMI handle init failed for client %#x\n",
> +			      client->id);
> +
> +	ret = qmi_add_lookup(&client->handle, TMD_SERVICE_ID_V01, TMD_SERVICE_VERS_V01,
> +			     client->id);
> +	if (ret < 0) {
> +		qmi_handle_release(&client->handle);
> +		return dev_err_probe(client->dev, ret, "QMI register failed for client 0x%x\n",
> +			      client->id);
> +	}
> +
> +	return 0;
> +}
> +
> +static int qmi_tmd_client_remove(struct platform_device *pdev)
void + .remove_new

Konrad