Re: [PATCH v2 3/3] mailbox: add Microchip IPC support

[<Valentina.FernandezAlanis@microchip.com>]

On 03/11/2024 00:23, Jassi Brar wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
> <valentina.fernandezalanis@microchip.com> wrote:
> 
> ....
>> +
>> +enum ipc_irq_type {
>> +       IPC_OPS_NOT_SUPPORTED   = 1,
>> +       IPC_MP_IRQ              = 2,
>> +       IPC_MC_IRQ              = 4,
>> +};
> totally unused.
> 
>> +
>> +/**
>> + * struct mchp_ipc_probe - IPC probe message format
>> + *
>> + * @hw_type:           IPC implementation available in the hardware
>> + * @num_channels:      number of IPC channels available in the hardware
>> + *
>> + * Used to retrieve information on the IPC implementation
>> + * using the SBI_EXT_IPC_PROBE SBI function id.
>> + */
>> +struct mchp_ipc_probe {
>   same as the driver.probe(), so maybe call this microchip_mbox_info
> 
> ......
> 
>> +struct mchp_ipc_cluster_cfg {
>> +       void *buf_base;
>> +       unsigned long buf_base_addr;
>> +       int irq;
>> +};
>> +
>> +struct ipc_chan_info {
>   I suggest s/ipc_chan_info/microchip_sbi_chan and hooking it to
> mbox_chan.con_priv
> 
> ....
> 
>> +       unsigned long buf_base_tx_addr;
>> +       unsigned long buf_base_rx_addr;
>> +       unsigned long msg_buf_tx_addr;
>> +       unsigned long msg_buf_rx_addr;
> If these are __pa(), then phys_addr_t please.
> 
>> +       int chan_aggregated_irq;
>> +       int mp_irq;
>> +       int mc_irq;
>> +       u32 id;
>> +       u32 max_msg_size;
>> +};
>> +
>> +struct microchip_ipc {
>   Maybe s/microchip_ipc/microchip_sbi_mbox ?
> 
> 
>> +       struct device *dev;
>> +       struct mbox_chan *chans;
>> +       struct mchp_ipc_cluster_cfg *cluster_cfg;
>> +       struct ipc_chan_info *priv;
>    replace this with 'struct mbox_chan *chan' and hook
>       chan[i].con_priv = priv[i]
>    this will help avoid having to EXPORT mchp_ipc_get_chan_id

Thanks for the feedback, I'll implement the suggestions in v3.

Regarding the EXPORT function, I understand that it’s also possible to 
retrieve con_priv from mbox_chan in the client. However, I felt it would 
be cleaner to export the function to obtain the channel ID directly, 
rather than declaring the struct ipc_chan_info in a header file to make 
it accessible to the client.

If necessary, I can remove the function and instead expose struct 
ipc_chan_info in linux/mailbox/mchp-ipc.h.
> 
> 
>> +       void *buf_base;
>> +       unsigned long buf_base_addr;
> phys_addr_t buf_base_addr ?
> 
>> +       struct mbox_controller controller;
>> +       u8 num_channels;
> this could be dropped by directly using 'controller.num_chans'
> 
> ......
> 
>> +static int mchp_ipc_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
>> +       const struct mchp_ipc_msg *msg = data;
>> +       struct mchp_ipc_sbi_msg sbi_payload;
>> +
>> +       memcpy(chan_info->msg_buf_tx, msg->buf, msg->size);
>> +       sbi_payload.buf_addr = chan_info->msg_buf_tx_addr;
>> +       sbi_payload.size = msg->size;
>> +       memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
> How does this work? sizeof(sbi_payload) is more than
> sizeof(*chan_info->buf_base_tx)
> I think buf_base_tx needs to be u8 array of max{sizeof(struct
> mchp_ipc_init), sizeof(struct mchp_ipc_sbi_msg)}, if there are
> alignment requirements then maybe kmalloc that size.
> Similarly for buf_base_rx.
> 
> ...
> 
>> +static struct platform_driver mchp_ipc_driver = {
>> +       .driver = {
>> +               .name = "microchip_ipc",
>> +               .of_match_table = mchp_ipc_of_match,
>> +       },
>> +       .probe = mchp_ipc_probe,
> The driver could be built as a module, so please provide .remove()
> even if you never intend to unload it.
In this particular case, there is nothing specific to implement in the 
.remove() function because all resources allocated in the .probe() 
function are managed using devm_*
> 
> cheers.