| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0d2df826-1503-42f7-8f60-1025cc641e2c@baylibre.com>
Date: Fri, 4 Oct 2024 17:10:21 +0200
From: Guillaume Stols <gstols@...libre.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Uwe Kleine-König <ukleinek@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Jonathan Corbet <corbet@....net>,
Michal Marek <mmarek@...e.com>, linux-pwm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, aardelean@...libre.com, dlechner@...libre.com,
jstephan@...libre.com
Subject: Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes
On 10/4/24 16:25, Jonathan Cameron wrote:
> On Wed, 2 Oct 2024 02:12:28 +0200
> Guillaume Stols <gstols@...libre.com> wrote:
>
>> On 9/29/24 14:44, Jonathan Cameron wrote:
>>> On Fri, 20 Sep 2024 17:33:27 +0000
>>> Guillaume Stols <gstols@...libre.com> wrote:
>>>
>>>> On the parallel version, the current implementation is only compatible
>>>> with id tables and won't work with fw_nodes, this commit intends to fix
>>>> it.
>>>>
>>>> Also, chip info is moved in the .h file so to be accessible to all the
>>> chip info is not moved (I was going to say no to that) but an
>>> extern is used to make it available. So say that rather than moved here.
>>>
>>>> driver files that can set a pointer to the corresponding chip as the
>>>> driver data.
>>>>
>>>>
>>>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>>>> index c13dda444526..18c87fe9a41a 100644
>>>> --- a/drivers/iio/adc/ad7606.h
>>>> +++ b/drivers/iio/adc/ad7606.h
>>>> @@ -38,8 +38,19 @@
>>>> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>>> 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>>>
>>>> +enum ad7606_supported_device_ids {
>>>> + ID_AD7605_4,
>>>> + ID_AD7606_8,
>>>> + ID_AD7606_6,
>>>> + ID_AD7606_4,
>>>> + ID_AD7606B,
>>>> + ID_AD7616,
>>>> +};
>>>> +
>>>> /**
>>>> * struct ad7606_chip_info - chip specific information
>>>> + * @name device name
>>>> + * @id device id
>>> ID in chip info normally indicates something bad in the design. In that somewhere
>>> we have code that is ID dependent rather than all such code / data being
>>> found directly in this structure (or callbacks found from here).
>>> Can we avoid it here?
>> Hi Jonathan,
>>
>> chip_info has to describe the chip hardwarewise, but there are different
>> bops depending on the wiring (interface used, and backend/no backend).
> Normal solution to this is multiple chip specific structures so they
> become specific to a chip + some wiring option. Then you just
> pick between static const structures.
>
> Does that work here?
>
> You will need them exposed (extern) from your header but that's
> not too bad.
>
> Aim is to pick just one structure that describes all the 'specific'
> stuff for the driver. That brings all that stuff into one place and
> provides an easy way to extend to new combinations of options for
> other devices.
>
> Jonathan
I finally wrote a structure containing the couple chip_info/bops
/**
* struct ad7606_bus_info - agregate ad7606_chip_info and ad7606_bus_ops
* @chip_info entry in the table of chips that describes this device
* @bops bus operations (SPI or parallel)
*/
struct ad7606_bus_info {
const struct ad7606_chip_info *chip_info;
const struct ad7606_bus_ops *bops;
};
and declared the following way
const struct ad7606_bus_info ad7606b_bus_info = {
.chip_info = &ad7606b_info,
.bops = &ad7606b_spi_bops,
};
const struct ad7606_bus_info ad7606c_16_bus_info = {
.chip_info = &ad7606c_16_info,
.bops = &ad7606b_spi_bops,
etc...
Will send the series later today, just doing some last checks.
>
>
>> The easiest way I found was to use the ID in a switch/case to
>> determinate which bops I should take (well it was only needed in the spi
>> version since it is the one supporting almost all the chips while the
>> other ones still support only one). For instance, the ad7606B will use
>> ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.
>>
>> If I can't use the ID, the only way I see is creating 3 fields in
>> chip_info (spi_ops, par_ops, backend_ops) and to initialize every
>> chip_info structure with its associated op(s) for the associated
>> interface. This would also lead to declare the different instances of
>> ad7606_bus_ops directly in ad7606.h (I dont like it very much but see
>> no other option).
>>
>> Do you think it's better that way ? Or do you have any other idea ?
>>
>> Regards,
>>
>> Guillaume
>>
>>>
>>>> * @channels: channel specification
>>>> * @num_channels: number of channels
>>>> * @oversampling_avail pointer to the array which stores the available
>>>> @@ -50,6 +61,8 @@
>>> ...
>>>
>>>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>>>> index d651639c45eb..7bac39033955 100644
>>>> --- a/drivers/iio/adc/ad7606_par.c
>>>> +++ b/drivers/iio/adc/ad7606_par.c
>>>> @@ -11,6 +11,7 @@
>>>> #include <linux/mod_devicetable.h>
>>>> #include <linux/module.h>
>>>> #include <linux/platform_device.h>
>>>> +#include <linux/property.h>
>>>> #include <linux/types.h>
>>>>
>>>> #include <linux/iio/iio.h>
>>>> @@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
>>>>
>>>> static int ad7606_par_probe(struct platform_device *pdev)
>>>> {
>>>> - const struct platform_device_id *id = platform_get_device_id(pdev);
>>>> + const struct ad7606_chip_info *chip_info;
>>>> + const struct platform_device_id *id;
>>>> struct resource *res;
>>>> void __iomem *addr;
>>>> resource_size_t remap_size;
>>>> int irq;
>>>>
>>>> + if (dev_fwnode(&pdev->dev)) {
>>>> + chip_info = device_get_match_data(&pdev->dev);
>>>> + } else {
>>>> + id = platform_get_device_id(pdev);
>>>> + chip_info = (const struct ad7606_chip_info *)id->driver_data;
>>>> + }
>>>> +
>>>> irq = platform_get_irq(pdev, 0);
>>>> if (irq < 0)
>>>> return irq;
>>>> @@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
>>>> remap_size = resource_size(res);
>>>>
>>>> return ad7606_probe(&pdev->dev, irq, addr,
>>>> - id->name, id->driver_data,
>>> Rewrap to move chip_info up a line perhaps.
>>>
>>>> + chip_info,
>>>> remap_size > 1 ? &ad7606_par16_bops :
>>>> &ad7606_par8_bops);
Powered by blists - more mailing lists