Thread (25 messages) 25 messages, 5 authors, 2025-09-24

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

From: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Date: 2025-09-23 09:07:11
Also in: linux-devicetree, linux-mediatek, lkml

On Tuesday, 23 September 2025 04:35:59 Central European Summer Time Jjian Zhou (周建) wrote:
On Mon, 2025-09-22 at 15:10 +0200, Nicolas Frattaroli wrote:
quoted
External email : Please do not click links or open attachments until
you have verified the sender or the content.


On Monday, 22 September 2025 09:17:27 Central European Summer Time
Jjian Zhou (周建) wrote:
quoted
On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
quoted
External email : Please do not click links or open attachments
until
you have verified the sender or the content.


On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
[off-list ref] wrote:
quoted
On Friday, 19 September 2025 18:32:12 Central European Summer
Time
Jassi Brar wrote:
quoted
On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <
wenst@chromium.org>
wrote:
quoted
On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
jassisinghbrar@gmail.com> wrote:
quoted
On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
jjian.zhou@mediatek.com> wrote:

.....
quoted
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct mtk_vcp_mbox_priv {
Maybe 'mtk_vcp_mbox' is a more appropriate name ?
quoted
+       void __iomem *base;
+       struct device *dev;
+       struct mbox_controller mbox;
+       const struct mtk_vcp_mbox_cfg *cfg;
+       struct mtk_ipi_info ipi_recv;
Maybe also have "struct mbox_chan chan[1]; " so that you
don't have to
allocate one during the probe.
Also if you have  "struct mbox_controller mbox;" as the
first
member,
you could simply typecast that to get this structure.
Something like "struct mpfs_mbox" in mailbox-mpfs.c
I read somewhere that this way of subclassing is not
recommended.
Instead the base class should explicitly not be the first
member.
And then container_of() should be used.

I don't remember where I read this though. But I think the
explicit
container_of() is easier for understanding the intent.
And how does container_of() work ? :)
typcasting the first member to its parent is the simplest
form of
container_of.

-j
Which is why it's completely equivalent and since code is
supposed
to communicate meaning to humans, container_of should be used.
Nobody is suggesting typecasting cfg, dev or anything else.
Typecasting between mailbox controllers is fine and arguably
easier
on
the eyes than using a container_of.

-j
OK. How about:
struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
quoted
con_priv;
Thanks.
An explicit cast would be worse, as at that point you're telling
C to completely ignore any semblance of a type system it has.

     struct mtk_vcp_mbox *priv; 
     priv->dev = dev;
     priv->chans[0].con_priv = priv;
The type of con_priv is "void *". 
Would the conversion mentioned above also have the issue you mentioned?

Thanks.
No, in that case the cast is implicit. While void pointers do
subvert the type system, they are needed in this case because
the con_priv member needs to point at structs of any type.

The problem is that when you do something like

  struct apple *a = something;
  struct orange *o = (struct orange *)a;

then if the two structs (apple and orange) are incompatible,
the compiler won't even yell at you, because you're explicitly
casting.

With an implicit cast:

  struct apple *a = something;
  struct orange *o = a;

the compiler will tell you if you're doing something wrong.
Here's a userspace code example to illustrate the point:

    #include <stdio.h>

    struct apple {
            const char *name;
            unsigned int weight;
    };

    struct orange {
            int x;
            int y;
            int z;
    };

    int main(int argc, char** argv)
    {
            struct apple a = {"Granny Smith", 200};
            // won't compile, good!
            /* struct orange *o = &a; */
            // will compile, bad!
            struct orange *o = (struct orange *)&a;

            printf("%d\n", o->x);

            return 0;
    }

If you comment out the second struct orange line and uncomment the
first, then you'll get a compilation error, which is what we want
because the two structs are incompatible and we don't want the
assignment to work in this case, as that would be a bug.

The second struct orange line always compiles, even though the two
structs are incompatible, and will cause nonsense to be printed.

I hope this illustrates the point I was trying to make, which is
that explicit casts make it harder to find issues because they
force the language to simply accept the cast rather than give us
a compilation error when something nonsensical is being done.

Kind regards,
Nicolas Frattaroli


Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help