Message-ID: <a066f0bc-7b27-771c-544d-cacd15aa2374@redhat.com>
Date:   Tue, 11 Jan 2022 10:20:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
Content-Language: en-US
To:     Stefan Binding <sbinding@opensource.cirrus.com>,
        'Mark Brown' <broonie@kernel.org>,
        "'Rafael J . Wysocki'" <rafael@kernel.org>,
        'Len Brown' <lenb@kernel.org>,
        'Mark Gross' <markgross@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
        linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        patches@opensource.cirrus.com
References: <20211210154050.3713-1-sbinding@opensource.cirrus.com>
 <20211210154050.3713-7-sbinding@opensource.cirrus.com>
 <c311642f-38ab-4914-cf92-852e6a20cfc9@redhat.com>
 <00af01d8062f$75aed010$610c7030$@opensource.cirrus.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <00af01d8062f$75aed010$610c7030$@opensource.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 1/10/22 15:36, Stefan Binding wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: 21 December 2021 18:32
>> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
>> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
>> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
>> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
>> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> patches@opensource.cirrus.com
>> Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
> 
>>> +	ret = bmi_spi_count_resources(adev);
>>> +	if (ret <= 0)
>>> +		return ret;
>>> +	count = ret;
>>
>> Ok, so why not do the following here instead (and drop a whole bunch of
>> functions above):
>>
>> 	ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
>> 	if (ret < 0)
>> 		return ret;
>>
>> 	if (count <= 0) {
>> 		acpi_dev_free_resource_list(&r);
>> 		return count;
>> 	}
>>
>> 	/* Note we are not freeing the resource list yet here !!! */
>>
>>> +
>>> +	bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),
>> GFP_KERNEL);
>>> +	if (!bmi->spi_devs)
>>> +		return -ENOMEM;
>>> +
>>> +	acpi_data = bmi_spi_get_resources(dev, adev, count);
>>> +	if (!acpi_data)
>>> +		return -ENOMEM;
>>
>> Remove the bmi_spi_get_resources() call here.
>>
>>> +
>>> +	for (i = 0; i < count && inst_array[i].type; i++) {
>>
>> Write a new:
>>
>> int bmi_get_spi_resource_by_index(list_head *resource_list, struct
>> acpi_resource_spi_serialbus *sb_ret, int index)
>> {}
>>
>> Helper which walks the list and fills in *sb_ret with the Nth (matching index)
>> SpiSerialBus resource found in the
>> list.
>>
>> And then do:
>>
>> 		ret = bmi_get_spi_resource_by_index(&r, &sb, i);
>> 		if (ret)
>> 			return ret;
>>
>> 		ctrl =
>> bmi_find_spi_controller(sb.resource_source.string_ptr);
>>
>>
>>> +		ctlr = bmi_find_spi_controller(acpi_data-
>>> acpi_data[i].resource_source);
>>> +		if (!ctlr) {
>>> +			ret = -EPROBE_DEFER;
>>> +			goto error;
>>> +		}
>>> +
>>> +		spi_dev = spi_alloc_device(ctlr);
>>> +		if (!spi_dev) {
>>> +			dev_err(&ctlr->dev, "failed to allocate SPI device for
>> %s\n",
>>> +				dev_name(&adev->dev));
>>> +			ret = -ENOMEM;
>>> +			goto error;
>>> +		}
>>> +
>>> +		strscpy(spi_dev->modalias, inst_array[i].type,
>> sizeof(spi_dev->modalias));
>>> +
>>
>> And replace all the "acpi_data->acpi_data[i].sb." reference below with
>> simple "sb.".
>>
>>
>>> +		if (ctlr->fw_translate_cs) {
>>> +			ret = ctlr->fw_translate_cs(ctlr,
>>> +						    acpi_data-
>>> acpi_data[i].sb.device_selection);
>>> +			if (ret < 0) {
>>> +				spi_dev_put(spi_dev);
>>> +				goto error;
>>> +			}
>>> +			spi_dev->chip_select = ret;
>>> +		} else {
>>> +			spi_dev->chip_select = acpi_data-
>>> acpi_data[i].sb.device_selection;
>>> +		}
>>> +
>>> +		spi_dev->max_speed_hz = acpi_data-
>>> acpi_data[i].sb.connection_speed;
>>> +		spi_dev->bits_per_word = acpi_data-
>>> acpi_data[i].sb.data_bit_length;
>>> +
>>> +		if (acpi_data->acpi_data[i].sb.clock_phase ==
>> ACPI_SPI_SECOND_PHASE)
>>> +			spi_dev->mode |= SPI_CPHA;
>>> +		if (acpi_data->acpi_data[i].sb.clock_polarity ==
>> ACPI_SPI_START_HIGH)
>>> +			spi_dev->mode |= SPI_CPOL;
>>> +		if (acpi_data->acpi_data[i].sb.device_polarity ==
>> ACPI_SPI_ACTIVE_HIGH)
>>> +			spi_dev->mode |= SPI_CS_HIGH;
>>> +
>>> +		ret = bmi_get_irq(pdev, adev, &inst_array[i]);
>>> +		if (ret < 0) {
>>> +			spi_dev_put(spi_dev);
>>> +			goto error;
>>> +		}
>>> +		spi_dev->irq = ret;
>>> +
>>> +		snprintf(name, sizeof(name), "%s-%s-%s.%d",
>> dev_name(&ctlr->dev), dev_name(dev),
>>> +			 inst_array[i].type, i);
>>> +		spi_dev->dev.init_name = name;
>>> +
>>> +		ret = spi_add_device(spi_dev);
>>> +		if (ret) {
>>> +			dev_err(&ctlr->dev, "failed to add SPI device %s from
>> ACPI: %d\n",
>>> +				dev_name(&adev->dev), ret);
>>> +			spi_dev_put(spi_dev);
>>> +			goto error;
>>> +		}
>>> +
>>> +		dev_dbg(dev, "SPI device %s using chip select %u", name,
>> spi_dev->chip_select);
>>> +
>>> +		bmi->spi_devs[i] = spi_dev;
>>> +		bmi->spi_num++;
>>> +	}
>>> +
>>> +	if (bmi->spi_num < count) {
>>> +		dev_err(dev, "Error finding driver, idx %d\n", i);
>>> +		ret = -ENODEV;
>>> +		goto error;
>>> +	}
>>> +
>>> +	dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
>>
>> And here replace the bmi_spi_res_free(acpi_data); call in both exit paths
>> with:
>> acpi_dev_free_resource_list(&r); .
>>
>> To me this way, simply using the already allocated resources from the list,
>> rather then making a temp copy of them and throwing that away seems like
>> a simpler solution ?
>>
>> If you go this route, please also remove the struct bmi_spi_acpi and
>> struct bmi_spi_sb_acpi data types which you now no longer need.
>>
> 
> I tried to implement this idea, and reuse the resource list, but I hit an issue. 
> The resources saved in the list are not "struct acpi_resource", but instead the 
> generic "struct resource".
> We need the acpi_resource structure to pull the parameters from to be able to
> create the spi devices.
> As far as I know there is no way to convert the "struct resource" into a
> "struct acpi_resource". Is there another way to do this?

Ugh, you're right. Sorry about that. I still don't realy like the code
from your original v2 patch for this.

So maybe this comment from my second reply on this patch can help
clean things up:

"So thinking a bit more about this, then looking up the nth SpiSerialBus
resource, and then turning that into a spi_client is something which
the SPI core ACPI code should already be doing for index==0. So I think
that you should be able to modify the SPI core ACPI code to take index
as a parameter and then have it export a helper for this which you
can use rather then duplicate the SPI core ACPI code  ? Note this is
also what the I2C code is already doing.

And if you go that route you may also want to consider to add the SPI
equivalent of the i2c_acpi_client_count() helper."

Maybe that is a possible route to go to clean this up?

Note there are also 2 other small remarks pending:

1. My comment about adding the _t_ at the end of detect
2. + Mark's remark about patch 3/6 missing your Signed-off-by.

Regards,

Hans