[U-Boot] [PATCH 03/10] dm: spi: add BCM63xx SPI driver

[Simon Glass]

Hi,

On 12 June 2017 at 00:02, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> On Thu, Jun 8, 2017 at 12:05 AM, Álvaro Fernández Rojas
> <noltari at gmail.com> wrote:
>> Hi Jagan,
>>
>> El 07/06/2017 a las 19:29, Jagan Teki escribió:
>>> On Wed, Jun 7, 2017 at 9:05 PM, Álvaro Fernández Rojas
>>> <noltari at gmail.com> wrote:
>>>> Hi Jagan,
>>>>
>>>> El 7/6/17 a las 9:35, Jagan Teki escribió:
>>>>> On Fri, May 19, 2017 at 12:59 AM, Álvaro Fernández Rojas
>>>>> <noltari at gmail.com> wrote:
>>>>>> This driver is a simplified version of linux/drivers/spi/spi-bcm63xx.c
>>>>>> Instead of supporting both HW revisions of the controller in a single build,
>>>>>> support has been split by the selected config to save space.
>>>>>>
>>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari at gmail.com>
>>>>>> ---
>>>>>>  drivers/spi/Kconfig       |  23 +++
>>>>>>  drivers/spi/Makefile      |   1 +
>>>>>>  drivers/spi/bcm63xx_spi.c | 404 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 428 insertions(+)
>>>>>>  create mode 100644 drivers/spi/bcm63xx_spi.c
>>>>>>
[...]

>>>>>> +static const struct dm_spi_ops bcm63xx_spi_ops = {
>>>>>> +       .cs_info = bcm63xx_spi_cs_info,
>>>>>> +       .set_mode = bcm63xx_spi_set_mode,
>>>>>> +       .set_speed = bcm63xx_spi_set_speed,
>>>>>> +       .xfer = bcm63xx_spi_xfer,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct udevice_id bcm63xx_spi_ids[] = {
>>>>>> +       { .compatible = SPI_DT_ID, },
>>>>>> +       { /* sentinel */ }
>>>>>
>>>>> Try to add .data with reg_space of respective controller, this will
>>>>> eventually reduce to driver configs and make it CONFIG_BCM63XX and
>>>>> reduce unneeded#ifdef.
>>>> This implies adding considerable bloat to the driver...
>>>> Is it imperative for the driver to be accepted?
>>>
>>> Yes, as per as recent U-boot developement we're trying to reduce
>>> defconfig code as possible and even this method can improve code
>>> quality.
>> I don't think that method applies here, since u-boot is usally built for each SoC and supporting two cores in the same build makes no sense to me.
>> These should be done in two different drivers but I merged them into a single one to improve maintainability.
>
> I don't like the approch of adding ifdef with two controller
> reg_space, we have .data that has been used in many dm-driver, let's
> have a try.

I do agree that #ifdefs in the driver are not desirable and it is
better to use the device tree .data field to select the correct
feature. Hopefully this involves not too much refactoring. As an
example of this see ich.c which handles a few different Intel SPI
controllers. It's a complicated driver because of the way the hardware
works but it does show how to do this sort of thing at run-time.

>
>> BTW, two u-boot developers already reviewed this patch and didn't complain about it...
>
> Wrong statement, it is community project every-body comment need to be
> considrable.
>
> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com
> U-Boot, Linux | Upstream Maintainer
> Hyderabad, India.

Regards,
Simon