On Wed, Feb 6, 2019 at 10:44 PM Pankaj Bansal <[email protected]> wrote: > > > > > -----Original Message----- > > From: Li Yang [mailto:[email protected]] > > Sent: Thursday, 7 February, 2019 05:09 AM > > To: Andrew Lunn <[email protected]> > > Cc: Pankaj Bansal <[email protected]>; Shawn Guo > > <[email protected]>; Florian Fainelli <[email protected]>; > > [email protected]; [email protected]; Rob Herring > > <[email protected]> > > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > > > On Wed, Feb 6, 2019 at 3:46 PM Andrew Lunn <[email protected]> wrote: > > > > > > > > > > &i2c0 { > > > > > > > status = "okay"; > > > > > > > > > > > > > > + fpga@66 { > > > > > > > + compatible = "fsl,lx2160aqds-fpga", > > > > > > > "fsl,fpga-qixis-i2c"; > > > > > > > + reg = <0x66>; > > > > > > > + #address-cells = <1>; > > > > > > > + #size-cells = <0>; > > > > > > > + > > > > > > > + mdio-mux-1@54 { > > > > > > > > > > > > Still no compatible string defined for the node. Probably > > > > > > should be > > > > > > "mdio-mux- mmioreg", "mdio-mux" > > > > > > > > > > it is not a specific device. MDIO mux is meant to be controlled by > > > > > some registers of parent device (FPGA). > > > > > Therefore, IMO this should not be a device and there should not be > > > > > any "compatible" property for it. > > > > > > > If it is not a device why we are defining a device node for it? It > > > > is probably not a physical device per se, but it can be considered a > > > > virtual device provided by FPGA. > > > > > > It is a physical device. But it happens to be embedded inside another > > > device. And that embedded is not performed as a bus with devices on > > > it, so the device tree concepts don't fit directly. > > > > Whether or not it is populated as a bus(which probably should as the FPGA > > does > > contain many different functions and these functions like the mdio-mux we > > are > > discussing about could have separate drivers), the node should have a new > > binding documentation similar to the mdio_mux_mmioreg binding or even > > covers the mmioreg too. And the best way to match the node with the binding > > is through compatible strings IMO. This is why I'm asking the node to have > > a > > compatible string. > > The mdio_mux is NOT a device. FPGA is a device that provides the mdio mux > functionality > (among other functions). The mdio mux is controlled via some bits In one of > the FPGA register.
With modern chips, it is likely to have multiple functions in a single physical device that are covered by multiple subsystems. We shouldn't limit the concept of device to only physical devices. > > In my previous approach, I also used a compatible field for mdio_mux node in > FPGA. > https://www.mail-archive.com/[email protected]/msg252744.html > The FPGA driver would create as many platform devices for each subnode, and > those devices > Would attach to mdio_mux_regmap driver based on compatible field. > > BUT, this platform device creation is the problem. Since mdio_mux is not an > actual device, how can > We create a platform device for it? Like I said platform device doesn't have to be a physically separate device. In this specific case, I think a multi-function device(MFD) will be the best fit for this FPGA device. The framework will help to create sub-devices and help to share the regmap to all the sub-function drivers. Please check existing MFD drivers for more details. > > Which is why I removed the compatible field from mdio_mux nodes. The FPGA > driver detects the mdio_mux > Using their name i.e. " mdio-mux-1@54". Like this: > for_each_child_of_node(dev->of_node, child) { > if (!of_node_name_prefix(child, "mdio-mux")) > > Refer : https://patchwork.kernel.org/patch/10797227/ > > > > > > > > > > This also bring up another question that why this device cannot > > > > reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? > > > > > > Because it is on an i2c bus, not an mmio bus. > > > > Oops, I missed that. > > > > > > > > > If we think regmap is a better solution, shall we replace the > > > > mmioreg driver with the regmap driver? > > > > > > regmap can be used with mmio. But for a single MMIO register it is a > > > huge framework. So it makes sense to keep mdio-mux-mmioreg simple. > > > > > > If however the device is already using regmap, adding one more > > > register is very little overhead. And it might be possible to use this > > > new mux with an mmio regmap, or an spi regmap, etc. So we seem to be > > > covering the best of both worlds. > > > > Ya. It would be ideal if the new driver can cover the legacy > > mdio-mux-mmioreg > > case too. > > Refer : https://patchwork.kernel.org/patch/10797227/ > The mdio_mux_regmap can be used by any FPGA be it i2c connected or MMIO based. > > > > > Regards, > > Leo
