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

[Kishon Vijay Abraham I <kishon@ti.com>]

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