lkml.org 
[lkml]   [2023]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 01/19] bus/cdx: add the cdx bus driver
    On Tue, Jan 17, 2023 at 07:11:33PM +0530, Nipun Gupta wrote:
    > Introduce AMD CDX bus, which provides a mechanism for scanning
    > and probing CDX devices. These devices are memory mapped on
    > system bus for Application Processors(APUs).
    >
    > CDX devices can be changed dynamically in the Fabric and CDX
    > bus interacts with CDX controller to rescan the bus and
    > rediscover the devices.
    >
    > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
    > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com>

    First off, very nice job, I didn't find any obvious issues with this
    integration into the driver core.

    That being said, why do you want this in drivers/bus/? Why not
    drivers/cdx/ ?

    One minor comment to make the code smaller:

    > +static int get_free_index(void)
    > +{
    > + unsigned long id_map;
    > + unsigned long mask;
    > + int index = 0;
    > +
    > + mask = (1UL << MAX_CDX_CONTROLLERS) - 1;
    > +retry:
    > + id_map = cdx_controller_id_map[0];
    > + if ((id_map & mask) == mask)
    > + return -ENOSPC;
    > +
    > + index = ffz(id_map);
    > + if (index >= MAX_CDX_CONTROLLERS)
    > + return -ENOSPC;
    > +
    > + if (test_and_set_bit(index, &cdx_controller_id_map[0]))
    > + goto retry;
    > +
    > + return index;
    > +}

    Why not just use the idr/ida structure instead? That will handle all of
    that logic for you and get rid of your bit twiddling.

    > +/**
    > + * struct cdx_dev_params - CDX device parameters
    > + * @cdx: CDX controller associated with the device
    > + * @parent: Associated CDX controller
    > + * @vendor: Vendor ID for CDX device
    > + * @device: Device ID for CDX device
    > + * @bus_num: Bus number for this CDX device
    > + * @dev_num: Device number for this device
    > + * @res: array of MMIO region entries
    > + * @res_count: number of valid MMIO regions
    > + * @req_id: Requestor ID associated with CDX device
    > + */
    > +struct cdx_dev_params {
    > + struct cdx_controller *cdx;
    > + u16 vendor;
    > + u16 device;

    Are these in little endian format in memory? Or native? Or something
    else?

    thanks,

    greg k-h

    \
     
     \ /
      Last update: 2023-03-26 23:44    [W:2.303 / U:23.504 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site