Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Guillaume Stols <gstols@baylibre.com>]

On 10/4/24 16:25, Jonathan Cameron wrote:
> On Wed, 2 Oct 2024 02:12:28 +0200
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> On 9/29/24 14:44, Jonathan Cameron wrote:
>>> On Fri, 20 Sep 2024 17:33:27 +0000
>>> Guillaume Stols <gstols@baylibre.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);