[v3,10/25] bus: mhi: ep: Add support for creating and destroying MHI EP devices

This commit adds support for creating and destroying MHI endpoint devices.
The MHI endpoint devices binds to the MHI endpoint channels and are used
to transfer data between MHI host and endpoint device.

There is a single MHI EP device for each channel pair. The devices will be
created when the corresponding channels has been started by the host and
will be destroyed during MHI EP power down and reset.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/ep/main.c | 77 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
> This commit adds support for creating and destroying MHI endpoint devices.
> The MHI endpoint devices binds to the MHI endpoint channels and are used
> to transfer data between MHI host and endpoint device.
> 
> There is a single MHI EP device for each channel pair. The devices will be
> created when the corresponding channels has been started by the host and
> will be destroyed during MHI EP power down and reset.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

A few comments again, nothing major.

					-Alex

> ---
>   drivers/bus/mhi/ep/main.c | 77 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index f66404181972..fcaacf9ddbd1 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -67,6 +67,83 @@ static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl,
>   	return mhi_dev;
>   }
>   
> +/*
> + * MHI channels are always defined in pairs with UL as the even numbered
> + * channel and DL as odd numbered one.
> + */

Awesome comment.  And it seems that the channel ID passed
here is even, and that there *must* be a second mhi_chan[]
entry after the one specified.  And UL is also called the
"primary" channel.

> +static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id)
> +{
> +	struct mhi_ep_chan *mhi_chan = &mhi_cntrl->mhi_chan[ch_id];
> +	struct mhi_ep_device *mhi_dev;
> +	int ret;
> +
> +	/* Check if the channel name is same for both UL and DL */
> +	if (strcmp(mhi_chan->name, mhi_chan[1].name))
> +		return -EINVAL;

Maybe log an error to say what's wrong with it?

> +
> +	mhi_dev = mhi_ep_alloc_device(mhi_cntrl, MHI_DEVICE_XFER);
> +	if (IS_ERR(mhi_dev))
> +		return PTR_ERR(mhi_dev);

It looks like the only possible error is no memory, so you could
just have mhi_ep_alloc_device() return NULL.

> +
> +	/* Configure primary channel */
> +	mhi_dev->ul_chan = mhi_chan;
> +	get_device(&mhi_dev->dev);
> +	mhi_chan->mhi_dev = mhi_dev;
> +
> +	/* Configure secondary channel as well */
> +	mhi_chan++;
> +	mhi_dev->dl_chan = mhi_chan;
> +	get_device(&mhi_dev->dev);
> +	mhi_chan->mhi_dev = mhi_dev;
> +
> +	/* Channel name is same for both UL and DL */
> +	mhi_dev->name = mhi_chan->name;
> +	dev_set_name(&mhi_dev->dev, "%s_%s",
> +		     dev_name(&mhi_cntrl->mhi_dev->dev),
> +		     mhi_dev->name);
> +
> +	ret = device_add(&mhi_dev->dev);
> +	if (ret)
> +		put_device(&mhi_dev->dev);
> +
> +	return ret;
> +}
> +
> +static int mhi_ep_destroy_device(struct device *dev, void *data)
> +{
> +	struct mhi_ep_device *mhi_dev;
> +	struct mhi_ep_cntrl *mhi_cntrl;
> +	struct mhi_ep_chan *ul_chan, *dl_chan;
> +
> +	if (dev->bus != &mhi_ep_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_ep_device(dev);
> +	mhi_cntrl = mhi_dev->mhi_cntrl;
> +
> +	/* Only destroy devices created for channels */
> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	ul_chan = mhi_dev->ul_chan;
> +	dl_chan = mhi_dev->dl_chan;

Aren't they required to supply *both* channels?  Or maybe
it's just required that there are transfer callback functions
for both channels.  Anyway, no need to check for null, because
the creation function guarantees they're both non-null I think.

> +	if (ul_chan)
> +		put_device(&ul_chan->mhi_dev->dev);
> +
> +	if (dl_chan)
> +		put_device(&dl_chan->mhi_dev->dev);
> +
> +	dev_dbg(&mhi_cntrl->mhi_dev->dev, "Destroying device for chan:%s\n",
> +		 mhi_dev->name);
> +
> +	/* Notify the client and remove the device from MHI bus */
> +	device_del(dev);
> +	put_device(dev);
> +
> +	return 0;
> +}
> +
>   static int parse_ch_cfg(struct mhi_ep_cntrl *mhi_cntrl,
>   			const struct mhi_ep_cntrl_config *config)
>   {

On Tue, Feb 15, 2022 at 02:02:57PM -0600, Alex Elder wrote:

[...]

> > +
> > +	mhi_dev = mhi_ep_alloc_device(mhi_cntrl, MHI_DEVICE_XFER);
> > +	if (IS_ERR(mhi_dev))
> > +		return PTR_ERR(mhi_dev);
> 
> It looks like the only possible error is no memory, so you could
> just have mhi_ep_alloc_device() return NULL.
> 

I think returning the actual error is more safe as we may end up adding more
stuff into this function in the future.

> > +
> > +	/* Configure primary channel */
> > +	mhi_dev->ul_chan = mhi_chan;
> > +	get_device(&mhi_dev->dev);
> > +	mhi_chan->mhi_dev = mhi_dev;
> > +
> > +	/* Configure secondary channel as well */
> > +	mhi_chan++;
> > +	mhi_dev->dl_chan = mhi_chan;
> > +	get_device(&mhi_dev->dev);
> > +	mhi_chan->mhi_dev = mhi_dev;
> > +
> > +	/* Channel name is same for both UL and DL */
> > +	mhi_dev->name = mhi_chan->name;
> > +	dev_set_name(&mhi_dev->dev, "%s_%s",
> > +		     dev_name(&mhi_cntrl->mhi_dev->dev),
> > +		     mhi_dev->name);
> > +
> > +	ret = device_add(&mhi_dev->dev);
> > +	if (ret)
> > +		put_device(&mhi_dev->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mhi_ep_destroy_device(struct device *dev, void *data)
> > +{
> > +	struct mhi_ep_device *mhi_dev;
> > +	struct mhi_ep_cntrl *mhi_cntrl;
> > +	struct mhi_ep_chan *ul_chan, *dl_chan;
> > +
> > +	if (dev->bus != &mhi_ep_bus_type)
> > +		return 0;
> > +
> > +	mhi_dev = to_mhi_ep_device(dev);
> > +	mhi_cntrl = mhi_dev->mhi_cntrl;
> > +
> > +	/* Only destroy devices created for channels */
> > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > +		return 0;
> > +
> > +	ul_chan = mhi_dev->ul_chan;
> > +	dl_chan = mhi_dev->dl_chan;
> 
> Aren't they required to supply *both* channels?  Or maybe
> it's just required that there are transfer callback functions
> for both channels.  Anyway, no need to check for null, because
> the creation function guarantees they're both non-null I think.
> 

mhi_ep_destroy_device() will be called for each device separately. So we
must check for NULL.

Thanks,
Mani