On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote:
> On Thu, 19 Oct 2017 05:03:18 +0200,
> Vinod Koul wrote:
> > +
> > +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.
hmmm good point. I think would make sense to make SOUNDWIRE as tristate too,
just like SOUND :)
> > + * 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.
thanks for spotting, will fix
> > + * 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.
what do you advise?
if (slave->id.mfg_id == id->mfg_id &&
slave->id.part_id == id->part_id) {
would mean below one is at same indent. Some people use:
if (slave->id.mfg_id == id->mfg_id &&
slave->id.part_id == id->part_id) {
Is it Documented anywhere...
>
> > + return id;
> > + }
>
> Superfluous braces for a single-line.
That bit was intentional. Yes it is not required but given that if condition
was falling to two lines, I wanted to help readability by adding these. I can
remove them..
>
> > + 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.
right...
>
> > --- 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.
sorry not quite clear, do you mind elaborating which ones to align?
>
>
> > --- /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?
That a very good suggestion, it will help in other things too :)
--
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
