Re: [PATCH v3 1/4] dt-bindings: soundwire: add slave bindings

[Rob Herring]

On Thu, Aug 22, 2019 at 7:56 AM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
>
>
>
> On 22/08/2019 13:36, Rob Herring wrote:
> >>>> +soundwire@c2d0000 {
> >>>> +    compatible = "qcom,soundwire-v1.5.0"
> >>>> +    reg = <0x0c2d0000 0x2000>;
> >>>> +
> >>>> +    spkr_left:wsa8810-left{
> >>>> +            compatible = "sdw0110217201000";
> >>>> +            ...
> >>>> +    };
> >>>> +
> >>>> +    spkr_right:wsa8810-right{
> >>>> +            compatible = "sdw0120217201000";
> >>> The normal way to distinguish instances is with 'reg'. So I think you
> >>> need 'reg' with Instance ID moved there at least. Just guessing, but
> >>> perhaps Link ID, too? And for 2 different classes of device is that
> >>> enough?
> >> In previous bindings (https://lists.gt.net/linux/kernel/3403276  ) we
> >> did have instance-id as different property, however Pierre had some good
> >> suggestion to make it align with _ADR encoding as per MIPI DisCo spec.
> >>
> >> Do you still think that we should split the instance id to reg property?
> > Assuming you could have more than 1 of the same device on the bus,
> > then you need some way to distinguish them and the way that's done for
> > DT is unit-address/reg. And compatible strings should be constant for
> > each instance.
> That is a good point!
> Okay that makes more sense keep compatible string constant.
> Class ID would be constant for given functionality that the driver will
> provide.
>
> So we will end up with some thing like this:
>
> soundwire@c2d0000 {
>         compatible = "qcom,soundwire-v1.5.0"
>         reg = <0x0c2d0000 0x2000>;
>          #address-cells = <1>;
>          #size-cells = <0>;
>
>         spkr_left:skpr@1{
>                 compatible = "sdw10217201000";
>                 reg = <0x1>
>                 sdw-link-id = <0>;

Not really sure what Link ID is, but maybe it should be part of reg
too if it is part of how you address a device.

>                 ...
>         };
>
>         spkr_right:spkr@2{
>                 compatible = "sdw10217201000";
>                 reg = <0x2>
>                 sdw-link-id = <0>;
>         };
> };
>
> I will spin this in next version!
>
> Thanks,
> srini
>
> >
> > Rob