On Tue, Jul 31, 2018 at 09:51:37PM +0530, Jassi Brar wrote:
> Hi Oleksij,
>
> A fast and fine turnaround! Mostly nits and a suggestion follows ...
>
> On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> >
> > This driver was tested using the mailbox-test driver sending messages
> > between the Cortex-A7 and the Cortex-M4.
> >
> Do we need something MU specific here?
> I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
> maybe just the two-line ritual intro to MU?
Till now, every additional word about MU added more disagreement in this
topic. I prefer the reduced variant.
>
> > +enum imx_mu_chan_type {
> > + IMX_MU_TYPE_TX, /* Tx with FIFO */
> > + IMX_MU_TYPE_RX, /* Rx with FIFO */
> >
> nit: its a register, not fifo
ok
>
> > +
> > +static void imx_mu_txdb_work(struct work_struct *work)
> > +{
> > + struct imx_mu_con_priv *cp =
> > + container_of(work, struct imx_mu_con_priv, work);
> > + mbox_chan_txdone(cp->chan, 0);
> > +}
> >
> The api assumes a controller have same type of channels. So we are
> having to do this here.
> I am not sure, but would declaring two mailbox controllers (one with
> doorbells and the other data channels) work?
I was thinking about it and decided that this pain was not worth it.
> If not, at least we could use a tasklet instead of a work queue?
ok. probably it is better to fix the mailbox framework.... may be not right
now.
>
> > +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
> > +{
> > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > + struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > + /* test if transmit register is empty */
> > + return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);
> > +}
> > +
> > +static int imx_mu_send_data(struct mbox_chan *chan, void *data)
> > +{
> > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > + struct imx_mu_con_priv *cp = chan->con_priv;
> > + u32 *arg = data;
> > +
> > + switch (cp->type) {
> > + case IMX_MU_TYPE_TX:
> > + if (!imx_mu_last_tx_done(chan))
> > + return -EBUSY;
> >
> This test should be moved to imx_mu_startup().
> The api won't ever call if last tx was not done, and so it doesn't
> know how to recover from send_data() failure.
ok.
>
> > +
> > +static int imx_mu_startup(struct mbox_chan *chan)
> > +{
> > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > + struct imx_mu_con_priv *cp = chan->con_priv;
> > + int ret;
> > +
> > + if (cp->type == IMX_MU_TYPE_TXDB) {
> > + /* Tx doorbell don't have ACK support */
> > + INIT_WORK(&cp->work, imx_mu_txdb_work);
> > + return 0;
> > + }
> > +
> > + cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
> > + cp->idx);
> > + if (!cp->irq_desc)
> > + return -ENOMEM;
> >
> Probably you won't do but still .... name of irq is but one channel
> property. So you could set it in probe() and avoid creating another
> situation when startup could fail.
You mean, fail on probe with no mem is better then fail on startup? Why?
>
> > + ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> > + chan);
> > + if (ret) {
> > + dev_err(priv->dev,
> > + "Unable to acquire IRQ %d\n", priv->irq);
> > + return ret;
> > + }
> > +
> > + switch (cp->type) {
> > + break;
> >
> Is this intentional?
no. thx.
>
> > +
> > +static void imx_mu_shutdown(struct mbox_chan *chan)
> > +{
> > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > + struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > + if (IMX_MU_TYPE_TXDB == cp->type)
> >
> nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)
why? I do it with simple reason:
this error will be detected by compiler
if (IMX_MU_TYPE_TXDB = cp->type)
this error will silently fail
if (cp->type = IMX_MU_TYPE_TXDB)
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
