Message-ID: <d7bd6663-516a-a1f9-506c-5853eaa948e4@linaro.org>
Date:   Tue, 14 Mar 2023 09:54:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
From:   Neil Armstrong <neil.armstrong@linaro.org>
Reply-To: neil.armstrong@linaro.org
Subject: Re: [PATCH v3 01/12] usb: typec: ucsi: add PMIC Glink UCSI driver
Content-Language: en-US
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20230130-topic-sm8450-upstream-pmic-glink-v3-0-4c860d265d28@linaro.org>
 <20230130-topic-sm8450-upstream-pmic-glink-v3-1-4c860d265d28@linaro.org>
 <ZA7wPtttsWlQRpAR@kuha.fi.intel.com>
Organization: Linaro Developer Services
In-Reply-To: <ZA7wPtttsWlQRpAR@kuha.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 13/03/2023 10:43, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Mar 09, 2023 at 02:27:52PM +0100, Neil Armstrong wrote:
>> +static void pmic_glink_ucsi_register(struct work_struct *work)
>> +{
>> +	struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work);
>> +
>> +	ucsi_register(ucsi->ucsi);
>> +}
>> +
>> +static void pmic_glink_ucsi_callback(const void *data, size_t len, void *priv)
>> +{
>> +	struct pmic_glink_ucsi *ucsi = priv;
>> +	const struct pmic_glink_hdr *hdr = data;
>> +
>> +	switch (hdr->opcode) {
>> +	case UC_UCSI_READ_BUF_REQ:
>> +		pmic_glink_ucsi_read_ack(ucsi, data, len);
>> +		break;
>> +	case UC_UCSI_WRITE_BUF_REQ:
>> +		pmic_glink_ucsi_write_ack(ucsi, data, len);
>> +		break;
>> +	case UC_UCSI_USBC_NOTIFY_IND:
>> +		schedule_work(&ucsi->notify_work);
>> +		break;
>> +	};
>> +}
>> +
>> +static void pmic_glink_ucsi_pdr_notify(void *priv, int state)
>> +{
>> +	struct pmic_glink_ucsi *ucsi = priv;
>> +
>> +	if (state == SERVREG_SERVICE_STATE_UP)
>> +		schedule_work(&ucsi->register_work);
>> +	else if (state == SERVREG_SERVICE_STATE_DOWN)
>> +		ucsi_unregister(ucsi->ucsi);
>> +}
>> +
>> +static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
>> +				 const struct auxiliary_device_id *id)
>> +{
>> +	struct pmic_glink_ucsi *ucsi;
>> +	struct device *dev = &adev->dev;
>> +
>> +	ucsi = devm_kzalloc(dev, sizeof(*ucsi), GFP_KERNEL);
>> +	if (!ucsi)
>> +		return -ENOMEM;
>> +
>> +	ucsi->dev = dev;
>> +	dev_set_drvdata(dev, ucsi);
>> +
>> +	INIT_WORK(&ucsi->notify_work, pmic_glink_ucsi_notify);
>> +	INIT_WORK(&ucsi->register_work, pmic_glink_ucsi_register);
>> +	init_completion(&ucsi->read_ack);
>> +	init_completion(&ucsi->write_ack);
>> +	init_completion(&ucsi->sync_ack);
>> +	mutex_init(&ucsi->lock);
>> +
>> +	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
>> +	if (IS_ERR(ucsi->ucsi))
>> +		return PTR_ERR(ucsi->ucsi);
>> +
>> +	ucsi_set_drvdata(ucsi->ucsi, ucsi);
>> +
>> +	ucsi->client = devm_pmic_glink_register_client(dev,
>> +						       PMIC_GLINK_OWNER_USBC,
>> +						       pmic_glink_ucsi_callback,
>> +						       pmic_glink_ucsi_pdr_notify,
>> +						       ucsi);
>> +	return PTR_ERR_OR_ZERO(ucsi->client);
>> +}
>> +
>> +static const struct auxiliary_device_id pmic_glink_ucsi_id_table[] = {
>> +	{ .name = "pmic_glink.ucsi", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(auxiliary, pmic_glink_ucsi_id_table);
>> +
>> +static struct auxiliary_driver pmic_glink_ucsi_driver = {
>> +	.name = "pmic_glink_ucsi",
>> +	.probe = pmic_glink_ucsi_probe,
>> +	.id_table = pmic_glink_ucsi_id_table,
>> +};
> 
> What happens if you remove the module - I think you need to implement
> the remove callback, no?

You're right, I thought devm_pmic_glink_register_client would call
pmic_glink_ucsi_pdr_notify which would unregister ucsi, but no so will add.

Thanks,
Neil

> 
> thanks,
>