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.cI 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. -jWhich 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. -jOK. 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