Re: [PATCH 3/5] phy: miphy365x: Provide support for the MiPHY356x Generic PHY

Kishon Vijay Abraham I
2014-07-03T10:07:41Z

Hi,

On Thursday 03 July 2014 01:37 PM, Lee Jones wrote:
> On Wed, 02 Jul 2014, Kishon Vijay Abraham I wrote:
>> On Monday 30 June 2014 06:31 PM, Lee Jones wrote:
>>> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
>>> devices. It has 2 ports which it can use for either; both SATA, both
>>> PCIe or one of each in any configuration.
>>>
>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>
> Removed.
>
>>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>>> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>> drivers/phy/Kconfig | 10 +
>>> drivers/phy/Makefile | 1 +
>>> drivers/phy/phy-miphy365x.c | 630 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 641 insertions(+)
>>> create mode 100644 drivers/phy/phy-miphy365x.c
>
> [...]
>
>>> +struct miphy365x_dev {
>>> + struct device *dev;
>>> + struct mutex miphy_mutex;
>>> + struct miphy365x phys[ARRAY_SIZE(ports)];
>>
>> Avoid using fixed array sizes for ports or channels. Refer [1].
>
> Just addressing this point in this mail. Any other subsequent points
> will either be fixed up or addressed in other correspondence.
>
> I don't agree with this point. I don't believe the number of channels
> should be dictated by the number of DT sub-nodes supplied. Instead,

But that's the way it is. The DT describes your hw and not the driver. However
the driver may not support everything that is in the hw.
> the driver should contain knowledge about what is supported and
> validate the DT data accordingly. If it's omitted we lose the ability

IMO the driver cannot validate DT data, it can just return error if there is
something the _driver_ cannot support.
> to conduct any kind of bounds checking, such like the following:
>
> if (WARN_ON(port >= ARRAY_SIZE(ports)))
> return ERR_PTR(-EINVAL);

Just as I mentioned in the other patch, 'ports' shouldn't be needed at all. If
we directly give phandle to the sub-node, it won't be needed.
> And
> if (child_count != ARRAY_SIZE(ports)) {
> dev_err(&pdev->dev, "%d ports supported, %d supplied\n",
> ARRAY_SIZE(ports), child_count);
> return -EINVAL;
> }
>
> If at a later point, we need to expand the driver to support a new
> chip which supports more channels/ports then we need to expand the
> bounds checking based on match data extracted from the supplied
> compatible string. For instance, if a 4 port controller is being used
> and only 2 channels have been supplied, or vice versa then probe()
> should fail.

I don't think error checking of this sort should be done in driver. The dt
_should_ know what is the controller that is being used.

Cheers
Kishon