| Message ID | 20220411210406.21404-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB | expand |
On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > RTL8367RB-VB was not mentioned in the compatible table, nor in the > Kconfig help text. > > The driver still detects the variant by itself and ignores which > compatible string was used to select it. So, any compatible string will > work for any compatible model. Meaning the compatible string is pointless, and cannot be trusted. So yes, you can add it, but don't actually try to use it for anything, like quirks. Andrew
> On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > Kconfig help text. > > > > The driver still detects the variant by itself and ignores which > > compatible string was used to select it. So, any compatible string will > > work for any compatible model. > > Meaning the compatible string is pointless, and cannot be trusted. So > yes, you can add it, but don't actually try to use it for anything, > like quirks. Thanks, Andrew. Those compatible strings are indeed useless for now. The driver probes the chip variant. Maybe in the future, if required, we could provide a way to either force a model or let it autodetect as it does today. There is no "family name" for those devices. The best we had was rtl8367c (with "c" probably meaning 3rd family). I suggested renaming the driver to rtl8367c but, in the end, we kept it as the first supported device name. My plan is, at least, to allow the user to specify the correct model without knowing which model it is equivalent to. Regards, Luiz
On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > RTL8367RB-VB was not mentioned in the compatible table, nor in the > Kconfig help text. > > The driver still detects the variant by itself and ignores which > compatible string was used to select it. So, any compatible string will > work for any compatible model. This is not quite true: a compatible string of realtek,rtl8366rb will select the other subdriver, assuming it is available. Besides that small inaccuracy, I think your description is missing one crucial bit of information, which is that the chip ID/version of the '67RB is already included in the driver. Otherwise it reads as though the '67RB has the same ID as one of the already-supported chips ('65MB or '67S). With the above clarifications: Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Kind regards, Alvin > > Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/Kconfig | 3 ++- > drivers/net/dsa/realtek/realtek-mdio.c | 1 + > drivers/net/dsa/realtek/realtek-smi.c | 4 ++++ > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig > index b7427a8292b2..8eb5148bcc00 100644 > --- a/drivers/net/dsa/realtek/Kconfig > +++ b/drivers/net/dsa/realtek/Kconfig > @@ -29,7 +29,8 @@ config NET_DSA_REALTEK_RTL8365MB > depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO > select NET_DSA_TAG_RTL8_4 > help > - Select to enable support for Realtek RTL8365MB-VC and RTL8367S. > + Select to enable support for Realtek RTL8365MB-VC, RTL8367RB-VB > + and RTL8367S. > > config NET_DSA_REALTEK_RTL8366RB > tristate "Realtek RTL8366RB switch subdriver" > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 31e1f100e48e..a36b0d8f17ff 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -267,6 +267,7 @@ static const struct of_device_id realtek_mdio_of_match[] = { > #endif > #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) > { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, > + { .compatible = "realtek,rtl8367rb", .data = &rtl8365mb_variant, }, > { .compatible = "realtek,rtl8367s", .data = &rtl8365mb_variant, }, > #endif > { /* sentinel */ }, > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 2243d3da55b2..c2200bd23448 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -556,6 +556,10 @@ static const struct of_device_id realtek_smi_of_match[] = { > .compatible = "realtek,rtl8365mb", > .data = &rtl8365mb_variant, > }, > + { > + .compatible = "realtek,rtl8367rb", > + .data = &rtl8365mb_variant, > + }, > { > .compatible = "realtek,rtl8367s", > .data = &rtl8365mb_variant, > -- > 2.35.1 >
On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote: > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > > Kconfig help text. > > > > > > The driver still detects the variant by itself and ignores which > > > compatible string was used to select it. So, any compatible string will > > > work for any compatible model. > > > > Meaning the compatible string is pointless, and cannot be trusted. So > > yes, you can add it, but don't actually try to use it for anything, > > like quirks. > > > Thanks, Andrew. Those compatible strings are indeed useless for now. > The driver probes the chip variant. Maybe in the future, if required, > we could provide a way to either force a model or let it autodetect as > it does today. The problem is, you have to assume some percentage of shipped DT blobs have the wrong compatible string, but work because it is not actually used in a meaningful way. This is why the couple of dozen Marvell switches have just 3 compatible strings, which is enough to find the ID registers to identify the actual switch. The three compatibles are the name of the lowest chip in the family which introduced to location of the ID register. > There is no "family name" for those devices. The best we had was > rtl8367c (with "c" probably meaning 3rd family). I suggested renaming > the driver to rtl8367c but, in the end, we kept it as the first > supported device name. My plan is, at least, to allow the user to > specify the correct model without knowing which model it is equivalent > to. In order words, you are quite happy to allow the DT author to get is wrong, and do not care it is wrong. So the percentage of DT blobs with the wrong compatible will go up, making it even more useless. It is also something you cannot retrospectively make useful, because of all those broken DT blobs. Andrew
On Tue, Apr 12, 2022 at 02:43:06PM +0200, Andrew Lunn wrote: > On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote: > > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > > > Kconfig help text. > > > > > > > > The driver still detects the variant by itself and ignores which > > > > compatible string was used to select it. So, any compatible string will > > > > work for any compatible model. > > > > > > Meaning the compatible string is pointless, and cannot be trusted. So > > > yes, you can add it, but don't actually try to use it for anything, > > > like quirks. > > > > > > Thanks, Andrew. Those compatible strings are indeed useless for now. > > The driver probes the chip variant. Maybe in the future, if required, > > we could provide a way to either force a model or let it autodetect as > > it does today. > > The problem is, you have to assume some percentage of shipped DT blobs > have the wrong compatible string, but work because it is not actually > used in a meaningful way. This is why the couple of dozen Marvell > switches have just 3 compatible strings, which is enough to find the > ID registers to identify the actual switch. The three compatibles are > the name of the lowest chip in the family which introduced to location > of the ID register. Right, this was basically the original behaviour: - realtek,rtl8265mb -> use rtl8365mb.c subdriver - realtek,rtl8366rb -> use rtl8366rb.c subdriver (different family with different register layout) We then check a chip ID/version register and store that in the driver-private data, in case of quirks or different behaviours between chips in the same family. I think Andrew has a point that adding more compatible strings is not really going to add any tangible benefit, due to the above bahviour. People can equally well just put one of the above two compatible strings. > > > There is no "family name" for those devices. The best we had was > > rtl8367c (with "c" probably meaning 3rd family). I suggested renaming > > the driver to rtl8367c but, in the end, we kept it as the first > > supported device name. My plan is, at least, to allow the user to > > specify the correct model without knowing which model it is equivalent > > to. > > In order words, you are quite happy to allow the DT author to get is > wrong, and do not care it is wrong. So the percentage of DT blobs with > the wrong compatible will go up, making it even more useless. > > It is also something you cannot retrospectively make useful, because > of all those broken DT blobs. I think Luiz is saying he wants to allow device tree authors to write "realtek,rtl8367rb" if their hardware really does have an RTL8367RB switch and not an RTL8365MB, rather than writing "realtek,rtl8365mb". But an enterprising device tree author could just as well write: compatible = "realtek,rtl8367rb", "realtek,rtl8365mb"; ... which would work without us having to continually add more (arguably useless) compatible strings to the driver, including this one. Kind regards, Alvin
> On Tue, Apr 12, 2022 at 02:43:06PM +0200, Andrew Lunn wrote: > > On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote: > > > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > > > > Kconfig help text. > > > > > > > > > > The driver still detects the variant by itself and ignores which > > > > > compatible string was used to select it. So, any compatible string will > > > > > work for any compatible model. > > > > > > > > Meaning the compatible string is pointless, and cannot be trusted. So > > > > yes, you can add it, but don't actually try to use it for anything, > > > > like quirks. > > > > > > > > > Thanks, Andrew. Those compatible strings are indeed useless for now. > > > The driver probes the chip variant. Maybe in the future, if required, > > > we could provide a way to either force a model or let it autodetect as > > > it does today. > > > > The problem is, you have to assume some percentage of shipped DT blobs > > have the wrong compatible string, but work because it is not actually > > used in a meaningful way. This is why the couple of dozen Marvell > > switches have just 3 compatible strings, which is enough to find the > > ID registers to identify the actual switch. The three compatibles are > > the name of the lowest chip in the family which introduced to location > > of the ID register. > > Right, this was basically the original behaviour: > > - realtek,rtl8265mb -> use rtl8365mb.c subdriver > - realtek,rtl8366rb -> use rtl8366rb.c subdriver (different family with different register layout) > > We then check a chip ID/version register and store that in the driver-private > data, in case of quirks or different behaviours between chips in the same > family. > > I think Andrew has a point that adding more compatible strings is not really > going to add any tangible benefit, due to the above bahviour. People can equally > well just put one of the above two compatible strings. The Realtek driver (rtl8367c) does provide a way to skip the probe and force a specific model detection. Maybe that is a requirement for some kind of device we might see in the future. If needed, we could add a new property ("autodetect-{model,variant} = false") to force the model based only on the compatible string. It would also allow a compatible device to use the driver even if its ids are not known by the driver. > > > There is no "family name" for those devices. The best we had was > > > rtl8367c (with "c" probably meaning 3rd family). I suggested renaming > > > the driver to rtl8367c but, in the end, we kept it as the first > > > supported device name. My plan is, at least, to allow the user to > > > specify the correct model without knowing which model it is equivalent > > > to. > > > > In order words, you are quite happy to allow the DT author to get is > > wrong, and do not care it is wrong. So the percentage of DT blobs with > > the wrong compatible will go up, making it even more useless. I wouldn't say it is wrong but not as specific as possible. "compatible" seems to indicate that a driver from modelA can be used for modelB. We could start to warn the user when the compatible string does not match the model (at least from what we already know). Today, the driver only uses the model to tell the CPU and user ports (and chip version is enough for that usage). However, the vendor driver does an independent probe when it is setting the external port and it does check additional registers. Today the Linux driver only supports RGMII without actually checking if the model does support it. When we expand that support to other port modes, we might need to revisit the chip probe. > > It is also something you cannot retrospectively make useful, because > > of all those broken DT blobs. We cannot tell if there are other models that share the same chip version (from reg 0x1302). We can only tell from the devices we have access to. Yes, there might already be a device using a different compatible string from the real device and I don't intend to break it. > I think Luiz is saying he wants to allow device tree authors to write > "realtek,rtl8367rb" if their hardware really does have an RTL8367RB switch and > not an RTL8365MB, rather than writing "realtek,rtl8365mb". But an enterprising > device tree author could just as well write: > > compatible = "realtek,rtl8367rb", "realtek,rtl8365mb"; > > ... which would work without us having to continually add more (arguably > useless) compatible strings to the driver, including this one. Yes, Alvin. Thanks. It feels strange to force the user to use "realtek,rtl8365mb" or any other different string that does not match the chip's real name. I would not expect the one writing the DT to know that rtl8367s shares the same family with rtl8365mb and rtl8365mb driver does support rtl8367s. Before writing the rtl8367s driver, I also didn't know the relation between those chips. The common was only to relate rtl8367s (or any other chip model) with the vendor driver rtl8367c. As we don't have a generic family string, I think it is better to add every model variant. Regards, Luiz
> On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > Kconfig help text. > > > > The driver still detects the variant by itself and ignores which > > compatible string was used to select it. So, any compatible string will > > work for any compatible model. > > This is not quite true: a compatible string of realtek,rtl8366rb will select the > other subdriver, assuming it is available. Yes, how about: The string (no matter which one) is currently only used to select the subdriver. Then, the subdriver will ignore which compatible string was used and it will detect the variant by itself using the chip id/version returned by the device. rtl8367rb chip ID/version of the '67RB is already included in the driver and in the dt-bindings. > Besides that small inaccuracy, I think your description is missing one crucial > bit of information, which is that the chip ID/version of the '67RB is already > included in the driver. Otherwise it reads as though the '67RB has the same ID > as one of the already-supported chips ('65MB or '67S). > With the above clarifications: > > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> It feels strange to force the user to use "realtek,rtl8365mb" or any > other different string that does not match the chip's real name. I > would not expect the one writing the DT to know that rtl8367s shares > the same family with rtl8365mb and rtl8365mb driver does support > rtl8367s. Before writing the rtl8367s driver, I also didn't know the > relation between those chips. The common was only to relate rtl8367s > (or any other chip model) with the vendor driver rtl8367c. As we don't > have a generic family string, I think it is better to add every model > variant. I will just quote the Marvell mv88e6xxx binding: The compatibility string is used only to find an identification register, which is at a different MDIO base address in different switch families. - "marvell,mv88e6085" : Switch has base address 0x10. Use with models: 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, 6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321, 6341, 6350, 6351, 6352 - "marvell,mv88e6190" : Switch has base address 0x00. Use with models: 6190, 6190X, 6191, 6290, 6390, 6390X - "marvell,mv88e6250" : Switch has base address 0x08 or 0x18. Use with model: 6220, 6250 This has worked well for that driver. Andrew
On Wed, Apr 13, 2022 at 08:40:54PM +0200, Andrew Lunn wrote: > > It feels strange to force the user to use "realtek,rtl8365mb" or any > > other different string that does not match the chip's real name. I > > would not expect the one writing the DT to know that rtl8367s shares > > the same family with rtl8365mb and rtl8365mb driver does support > > rtl8367s. Before writing the rtl8367s driver, I also didn't know the > > relation between those chips. The common was only to relate rtl8367s > > (or any other chip model) with the vendor driver rtl8367c. As we don't > > have a generic family string, I think it is better to add every model > > variant. > > I will just quote the Marvell mv88e6xxx binding: > > The compatibility string is used only to find an identification register, > which is at a different MDIO base address in different switch families. > - "marvell,mv88e6085" : Switch has base address 0x10. Use with models: > 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, > 6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321, > 6341, 6350, 6351, 6352 > - "marvell,mv88e6190" : Switch has base address 0x00. Use with models: > 6190, 6190X, 6191, 6290, 6390, 6390X > - "marvell,mv88e6250" : Switch has base address 0x08 or 0x18. Use with model: > 6220, 6250 > > This has worked well for that driver. Right, so there's no real reason to add more compatible strings as long as the DT bindings are clear about it. Luiz, if you think the DT bindings are somehow unclear, feel free to update them. But I think we are probably better off not adding any more compatible strings unless there is a real technical need for it. In the same way, I guess the recently added compatible string "realtek,rtl8367s" was actually unnecessary. Kind regards, Alvin
On Wed, Apr 13, 2022 at 03:38:31PM -0300, Luiz Angelo Daros de Luca wrote: > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote: > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the > > > Kconfig help text. > > > > > > The driver still detects the variant by itself and ignores which > > > compatible string was used to select it. So, any compatible string will > > > work for any compatible model. > > > > This is not quite true: a compatible string of realtek,rtl8366rb will select the > > other subdriver, assuming it is available. > > Yes, how about: > > The string (no matter which one) is currently only used to select the > subdriver. Then, the subdriver > will ignore which compatible string was used and it will detect the > variant by itself using the > chip id/version returned by the device. > > rtl8367rb chip ID/version of the '67RB is already included in the > driver and in the dt-bindings. > > > Besides that small inaccuracy, I think your description is missing one crucial > > bit of information, which is that the chip ID/version of the '67RB is already > > included in the driver. Otherwise it reads as though the '67RB has the same ID > > as one of the already-supported chips ('65MB or '67S). > > With the above clarifications: > > > > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> While the code is OK, on second thought I think based on Andrew's points in the other subthread that we are better off without this patch. Kind regards, Alvin
> While the code is OK, on second thought I think based on Andrew's points in the > other subthread that we are better off without this patch. I agree, although the rtl8365mb name will make it harder for a newcomer to find the driver. Is it too late to get rid of all those compatible strings from dt-bindings? And rtl8367s from the driver? We must add all supported devices to the doc as well, similar to mv88e6085. Regards, Luiz
On Wed, Apr 13, 2022 at 11:32:42PM -0300, Luiz Angelo Daros de Luca wrote: > > While the code is OK, on second thought I think based on Andrew's points in the > > other subthread that we are better off without this patch. > > I agree, although the rtl8365mb name will make it harder for a > newcomer to find the driver. If it is made clear in the DT bindings, I think it's fine. If it works for Marvell switches, it will work for Realtek switches too. > > Is it too late to get rid of all those compatible strings from > dt-bindings? And rtl8367s from the driver? > We must add all supported devices to the doc as well, similar to mv88e6085. You can always try! I'm OK with those things in principle, but others might object due to ABI reasons. Don't get hung up on the vestigial "realtek,rtl8367s" compatible string though... while it's probably harmless to remove it, it's also relatively harmless to leave it there. Linux is full of such examples. Kind regards, Alvin
> > Is it too late to get rid of all those compatible strings from > > dt-bindings? And rtl8367s from the driver? > > We must add all supported devices to the doc as well, similar to mv88e6085. > > You can always try! I'm OK with those things in principle, but others might > object due to ABI reasons. Anything which is in a released Linus kernel is ABI and cannot be removed. Anything in net-next, or an -rcX kernel can still be changed. Andrew
> > > Is it too late to get rid of all those compatible strings from > > > dt-bindings? And rtl8367s from the driver? > > > We must add all supported devices to the doc as well, similar to mv88e6085. > > > > You can always try! I'm OK with those things in principle, but others might > > object due to ABI reasons. > > Anything which is in a released Linus kernel is ABI and cannot be > removed. Anything in net-next, or an -rcX kernel can still be changed. rtl8367s was added for 5.18. I'll prepare a patch for net to remove it. Now, about dt-bindings, I don't know what is the best approach. As device-tree should not focus on Linux, it is strange to use a compatible "rtl8365mb" just because it is the Linux subdriver name and that name was just because it was the first device supported. Also, once published, it is not good to break it. Just to illustrate it better, these are the chip versions rtl8365mb.c already supports or could support: - rtl8363nb (not in dt) - rtl8363nb-vb (not in dt) - rtl8363sc (not in dt) - rtl8363sc-vb (not in dt) - rtl8364nb (not in dt) - rtl8364nb-vb (not in dt) - rtl8365mb (the first supported by rtl8365mb.c, maybe it should be realtek,rtl8365mb-vc as rtl8365mb and rtl8365mb-vb seems to exist and they might be incompatible) - rtl8366sc (not in dt) - rtl8367rb-vb (not in dt) - rtl8367s (in dt and referenced in the code. The one I'll remove from code) - rtl8367sb (new) - rtl8370mb (new) - rtl8310sr (new) and these are the ones referenced in rtl8366rb.c code: - rtl8366rb (rtl8366rb.c) - rtl8366s (removed from rtl8366rb.c recently) but I know nothing about unsupported chip versions. These "models" are referenced in the bindings but some of them are not really a chip, like rtl8367 and rtl8367b: - rtl8366 (??) - rtl8367 (??) - rtl8367b (??) - rtl8367rb (??) - rtl8368s (??) - rtl8369 (??) - rtl8370 (??) Anyway, I'm planning on submitting something like this: --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml @@ -31,28 +31,14 @@ properties: compatible: enum: - realtek,rtl8365mb - - realtek,rtl8366 - realtek,rtl8366rb - - realtek,rtl8366s - - realtek,rtl8367 - - realtek,rtl8367b - - realtek,rtl8367rb - - realtek,rtl8367s - - realtek,rtl8368s - - realtek,rtl8369 - - realtek,rtl8370 description: | - realtek,rtl8365mb: 4+1 ports - realtek,rtl8366: 5+1 ports - realtek,rtl8366rb: 5+1 ports - realtek,rtl8366s: 5+1 ports - realtek,rtl8367: - realtek,rtl8367b: - realtek,rtl8367rb: 5+2 ports - realtek,rtl8367s: 5+2 ports - realtek,rtl8368s: 8 ports - realtek,rtl8369: 8+1 ports - realtek,rtl8370: 8+2 ports + realtek,rtl8365mb: + Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB, + RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S, + RTL8367SB, RTL8370MB, RTL8310SR + realtek,rtl8367rb: + Use with models RTL8366RB, RTL8366S mdc-gpios: description: GPIO line for the MDC clock line.
> Now, about dt-bindings, I don't know what is the best approach. As > device-tree should not focus on Linux, it is strange to use a > compatible "rtl8365mb" just because it is the Linux subdriver name and > that name was just because it was the first device supported. What you are trying to express is, how do you access the ID register. There is no obvious One True Compatible string for that. So just picking one switch name for that is O.K. There is nothing Linux specific in that, FreeBSD or whatever can use the label as a clue where to find the ID register. > + realtek,rtl8365mb: > + Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB, > + RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S, > + RTL8367SB, RTL8370MB, RTL8310SR > + realtek,rtl8367rb: > + Use with models RTL8366RB, RTL8366S So to me, this is fine. But i might add a bit more detail, that the compatible is used by the driver to find the ID register, and the driver then uses to ID register to decide how to drive the switch. The problem i had with the mv88e6xxx binding was until i spelt this out in the binding, people kept submitting patches adding new compatible strings, rather than extend the documented list of switches supported by a compatible. Andrew
> So to me, this is fine. But i might add a bit more detail, that the > compatible is used by the driver to find the ID register, and the > driver then uses to ID register to decide how to drive the switch. The > problem i had with the mv88e6xxx binding was until i spelt this out in > the binding, people kept submitting patches adding new compatible > strings, rather than extend the documented list of switches supported > by a compatible. Thanks, Andrew. I just sent two patches to deal with the cleanup. Regards, Luiz
diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig index b7427a8292b2..8eb5148bcc00 100644 --- a/drivers/net/dsa/realtek/Kconfig +++ b/drivers/net/dsa/realtek/Kconfig @@ -29,7 +29,8 @@ config NET_DSA_REALTEK_RTL8365MB depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO select NET_DSA_TAG_RTL8_4 help - Select to enable support for Realtek RTL8365MB-VC and RTL8367S. + Select to enable support for Realtek RTL8365MB-VC, RTL8367RB-VB + and RTL8367S. config NET_DSA_REALTEK_RTL8366RB tristate "Realtek RTL8366RB switch subdriver" diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 31e1f100e48e..a36b0d8f17ff 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -267,6 +267,7 @@ static const struct of_device_id realtek_mdio_of_match[] = { #endif #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, + { .compatible = "realtek,rtl8367rb", .data = &rtl8365mb_variant, }, { .compatible = "realtek,rtl8367s", .data = &rtl8365mb_variant, }, #endif { /* sentinel */ }, diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 2243d3da55b2..c2200bd23448 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -556,6 +556,10 @@ static const struct of_device_id realtek_smi_of_match[] = { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, + { + .compatible = "realtek,rtl8367rb", + .data = &rtl8365mb_variant, + }, { .compatible = "realtek,rtl8367s", .data = &rtl8365mb_variant,
RTL8367RB-VB was not mentioned in the compatible table, nor in the Kconfig help text. The driver still detects the variant by itself and ignores which compatible string was used to select it. So, any compatible string will work for any compatible model. Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/Kconfig | 3 ++- drivers/net/dsa/realtek/realtek-mdio.c | 1 + drivers/net/dsa/realtek/realtek-smi.c | 4 ++++ 3 files changed, 7 insertions(+), 1 deletion(-)