Re: [PATCH v2 04/14] net: wwan: t7xx: Add port proxy infrastructure

["Martinez, Ricardo" <ricardo.martinez@linux.intel.com>]

On 11/6/2021 11:06 AM, Sergey Ryazanov wrote:
> On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
> <ricardo.martinez@linux.intel.com> wrote:
>> Port-proxy provides a common interface to interact with different types
>> of ports. Ports export their configuration via `struct t7xx_port` and
>> operate as defined by `struct port_ops`.
[skipped]
>
>> +       enum ccci_ch            tx_ch;
>> +       enum ccci_ch            rx_ch;
>> +       unsigned char           txq_index;
>> +       unsigned char           rxq_index;
>> +       unsigned char           txq_exp_index;
>> +       unsigned char           rxq_exp_index;
>> +       enum cldma_id           path_id;
>> +       unsigned int            flags;
>> +       struct port_ops         *ops;
>> +       unsigned int            minor;
>> +       char                    *name;
> Why did you need these two fields with the port name and minor number?
> The WWAN subsystem will care about these data for you. It is its
> purpose.

This port proxy structure can abstract different types of ports. 'minor' 
field is used for

tty and char ports but it will be removed since the next iteration will 
contain only

WWAN ports and one control port.

'name' is currently used when printing error logs,  in the future it can 
be used

to define the name in the file system for test and debug ports.


[skipped]

>> +static int proxy_register_char_dev(void)
>> +{
>> +       dev_t dev = 0;
>> +       int ret;
>> +
>> +       if (port_prox->major) {
>> +               dev = MKDEV(port_prox->major, port_prox->minor_base);
>> +               ret = register_chrdev_region(dev, TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
>> +       } else {
>> +               ret = alloc_chrdev_region(&dev, port_prox->minor_base,
>> +                                         TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
>> +               if (ret)
>> +                       dev_err(port_prox->dev, "failed to alloc chrdev region, ret=%d\n", ret);
>> +
>> +               port_prox->major = MAJOR(dev);
>> +       }
> For what do you need these character devices? The WWAN subsystem
> already handle all these tasks.
This infrastructure is not going to be included in the next iteration, 
it is used for debug and test ports.
>> +       return ret;
>> +}
>> ...
>> +static int proxy_alloc(struct mtk_modem *md)
>> +{
>> +       int ret;
>> +
>> +       port_prox = devm_kzalloc(&md->mtk_dev->pdev->dev, sizeof(*port_prox), GFP_KERNEL);
>> +       if (!port_prox)
>> +               return -ENOMEM;
>
[skipped]


Ricardo