Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333470 > unrolled thread

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware  SPI driver Jiang Qiu <qiujiang@huawei.com> - 2016-02-14 10:40 +0100
    Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for  Designware SPI driver Mark Brown <broonie@kernel.org> - 2016-02-15 18:50 +0100

#1333470 — Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

Hi Mark,

Many thanks for your review, I'm so sorry for late reply because The 
Chinese
new year holiday. See my replies below.

Best Regards
Jiang

在 2016/2/5 19:09, Mark Brown 写道:
> On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:
>
>> This patch added ACPI support for DesignWare SPI mmio driver. It
>> was based the corresponding DT driver and compatible for this two
>> way. This patch has been tested on Hisilicon D02 board. It relies
>> on the GPIO patchset.
> Intel are heavy users of this driver on their systems which also use
> ACPI.  Have you discussed this binding with them?  I've copied Andy and
> Jarkko who've worked on the driver recently.
  I'm going to ask Andy to get some ideas that how to use this spi-dw-mmio
  driver by ACPI binding.
>
> Please use subject lines matching the style for the subsystem.  This
> makes it easier for people to identify relevant patches.
Thanks for the reminder, I will fix it in the next version.
>
>> +	char propname[32];
> That's a magic number, where did it come from and why is it a magic
> nummber?
I'm sorry for here without any comments. This number define is come from
gpiolib.c. It means the max size of gpio property name. The reference code
located in line 1815 of gpiolib.c.
>> +	if (ACPI_COMPANION(&pdev->dev)) {
>> +		for (i = 0; i < dws->num_cs; i++) {
>> +			snprintf(propname, sizeof(propname), "cs%d", i);
>> +			gpiod = devm_gpiod_get(&pdev->dev,
>> +				propname, GPIOD_ASIS);
>> +			if (IS_ERR(gpiod)) {
>> +				dev_err(&pdev->dev, "Get gpio desc failed!\n");
>> +				return PTR_ERR(gpiod);
>> +			}
>> +		}
>> +	}
> I'm not seeing anywhere where we store the GPIO in this loop.  It is
> therefore unclear to me how the chip select is going to work?
In DT binding, of_get_named_gpio and devm_gpio_request were used to 
parse gpio
pins defined in DTs and then request these pins. Similarly, for ACPI, 
devm_gpiod_get
can do that two operation in a single function. It is a unified 
interface to ACPI and DT
binding.

If the gpiod is valid, the corresponding gpio pins has been requested. 
We do not need
to save this gpiod any more.

which gpio pin was used is defined in spi_device, named cs_gpio, the 
configuration to the
gpio pins will be done in the setup callback routine of each device. 
What the spi master
should do is just request these pins to the gpio subsystem.
>> +static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
>> +		{"HISI0171", 0},
>> +		{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);
> I really do wish ACPI had some more sensible system for allocating
> device IDs so the tables were a little more legible. :(
This is really a question, I will do this feedback to ACPI maintainers.

[toc] | [next] | [standalone]

#1334663 — Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Multipart message — attachments visible in raw view] — view raw

On Sun, Feb 14, 2016 at 05:31:14PM +0800, Jiang Qiu wrote:
> 在 2016/2/5 19:09, Mark Brown 写道:
> >On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.  Your mail has lines wrapped alternately at
normal length and half length which is difficult to read, I've reflowed
this time.

> >>+	char propname[32];

> >That's a magic number, where did it come from and why is it a magic
> >nummber?

> I'm sorry for here without any comments. This number define is come from
> gpiolib.c. It means the max size of gpio property name. The reference code
> located in line 1815 of gpiolib.c.

That really isn't helping here.  The problem is that you've got a random
magic number thrown in here with no documentation.

> >I'm not seeing anywhere where we store the GPIO in this loop.  It is
> >therefore unclear to me how the chip select is going to work?

> In DT binding, of_get_named_gpio and devm_gpio_request were used to
> parse gpio pins defined in DTs and then request these pins. Similarly,
> for ACPI, devm_gpiod_get can do that two operation in a single
> function. It is a unified interface to ACPI and DT binding.

> If the gpiod is valid, the corresponding gpio pins has been requested.
> We do not need to save this gpiod any more.

No, that makes no sense at all.  If we're requesting a GPIO to use as
chip select we can't just then ignore the GPIO, we need to control it at
some point and...

> which gpio pin was used is defined in spi_device, named cs_gpio, the
> configuration to the gpio pins will be done in the setup callback
> routine of each device. What the spi master should do is just request
> these pins to the gpio subsystem.

...this would be a complete failure of abstraction - you appear to be
suggesting that the client devices should also separately request the
GPIOs and set them up.  That's at best going to be redundant and is
likely to introduce bugs.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web