lkml.org 
[lkml]   [2017]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 02/14] soundwire: Add SoundWire bus type
    On Thu, 19 Oct 2017 05:03:18 +0200,
    Vinod Koul wrote:
    >
    > --- /dev/null
    > +++ b/drivers/soundwire/Kconfig
    > @@ -0,0 +1,22 @@
    > +#
    > +# SoundWire subsystem configuration
    > +#
    > +
    > +menuconfig SOUNDWIRE
    > + bool "SoundWire support"
    > + ---help---
    > + SoundWire is a 2-Pin interface with data and clock line ratified
    > + by the MIPI Alliance. SoundWire is used for transporting data
    > + typically related to audio functions. SoundWire interface is
    > + optimized to integrate audio devices in mobile or mobile inspired
    > + systems
    > +
    > +if SOUNDWIRE
    > +
    > +comment "SoundWire Devices"
    > +
    > +config SOUNDWIRE_BUS
    > + tristate
    > + default SOUNDWIRE
    > +

    Does it make sense to be tristate?
    Since CONFIG_SOUNDWIRE is a bool, the above would be also only either
    Y or N. If it's Y and others select M, it'll be still Y.


    > --- /dev/null
    > +++ b/drivers/soundwire/bus_type.c
    > +/**
    > + * sdw_get_device_id: find the matching SoundWire device id
    > + *
    > + * @slave: SoundWire Slave device
    > + * @drv: SoundWire Slave Driver

    Inconsistent upper/lower letters in these two lines.

    > + * The match is done by comparing the mfg_id and part_id from the
    > + * struct sdw_device_id. class_id is unused, as it is a placeholder
    > + * in MIPI Spec.
    > + */
    > +static const struct sdw_device_id *
    > +sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
    > +{
    > + const struct sdw_device_id *id = drv->id_table;
    > +
    > + while (id && id->mfg_id) {
    > + if (slave->id.mfg_id == id->mfg_id &&
    > + slave->id.part_id == id->part_id) {

    Please indentation properly.

    > + return id;
    > + }

    Superfluous braces for a single-line.

    > + id++;
    > + }
    > +
    > + return NULL;
    > +}
    > +
    > +static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
    > +{
    > + struct sdw_slave *slave = dev_to_sdw_dev(dev);
    > + struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
    > +
    > + return !!sdw_get_device_id(slave, drv);
    > +}
    > +
    > +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size)

    I'd put const to slave argument, as it won't be modified.

    > --- a/include/linux/mod_devicetable.h
    > +++ b/include/linux/mod_devicetable.h
    > @@ -228,6 +228,13 @@ struct hda_device_id {
    > unsigned long driver_data;
    > };
    >
    > +struct sdw_device_id {
    > + __u16 mfg_id;
    > + __u16 part_id;
    > + __u8 class_id;
    > + kernel_ulong_t driver_data;

    Better to think of alignment.


    > --- /dev/null
    > +++ b/include/linux/soundwire/sdw.h
    ....
    > +/**
    > + * struct sdw_bus: SoundWire bus
    > + *
    > + * @dev: Master linux device
    > + * @link_id: Link id number, can be 0 to N, unique for each Master
    > + * @slaves: list of Slaves on this bus
    > + * @assigned: logical addresses assigned, Index 0 (broadcast) would be unused
    > + * @bus_lock: bus lock
    > + */
    > +struct sdw_bus {
    > + struct device *dev;
    > + unsigned int link_id;
    > + struct list_head slaves;
    > + bool assigned[SDW_MAX_DEVICES + 1];

    Why not a bitmap?


    thanks,

    Takashi

    \
     
     \ /
      Last update: 2017-10-22 17:20    [W:4.053 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site