On 01/10/2017 08:06 PM, Geert Uytterhoeven wrote:
> Hi Marek,
>
> On Tue, Jan 10, 2017 at 7:52 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
>> On 01/10/2017 07:41 PM, Geert Uytterhoeven wrote:
>>> On Tue, Jan 10, 2017 at 7:35 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
>>>>> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
>>>>>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>>>>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>>>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>>>>>> [...]
>>>>>>>>>>>> +- renesas,gyroadc-mode: GyroADC mode of operation, must be either of:
>>>>>>>>>>>> + 1 - MB88101A mode, 12bit sampling, 4 channels
>>>>>>>>>>>> + 2 - ADCS7476 mode, 15bit sampling, 8 channels
>>>>>>>>>>>> + 3 - MAX1162 mode, 16bit sampling, 8 channels
>>>>>>>>>>>
>>>>>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>>>>>> interfaces to an external ADC?
>>>>>>>>>>
>>>>>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>>>>>
>>>>>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>>>>>
>>>>>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>>>>>> setting rather than a description of the hardware, at least it is not very
>>>>>>>>> DT-ish.
>>>>>>>>>
>>>>>>>>> I'd go with something like
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> &adc {
>>>>>>>>> compatible = "renesas,r8a7791-gyroadc";
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> adc@0 {
>>>>>>>>> reg = <0>;
>>>>>>>>> compatible = "maxim,max1162";
>>>>>>>>
>>>>>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>>>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>>>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>>>>>> actually OK ?
>>>>>>>
>>>>>>> Only if you call of_register_spi_devices().
>>>>>>>
>>>>>>> We for example have devices that can either work in SPI or I2C mode and both
>>>>>>> use the same compatible string. Depending on whether the device is a subnode
>>>>>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>>>>>
>>>>>>> This situation is not that different.
>>>>>>
>>>>>> So in this case, the gyroadc driver would instead iterate over the
>>>>>> subnodes, check the compatible string and configure the channel mode
>>>>>> according to the subnode compatible string?
>>>>>
>>>>> That's correct.
>>>>>
>>>>>> But then, the gyroadc only has one configuration register which applies
>>>>>> to all channels, which selects how many bits to read from the connected
>>>>>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>>>>>> The driver would then have to check if all the compatible properties in
>>>>>> the ADC subnodes select the same ADC or at least ADC which has the same
>>>>>
>>>>> Correct again.
>>>>>
>>>>>> sample width. This looks like a duplication to me. Thoughts ?
>>>>>
>>>>> While the protocols have to be the same for each channel, you can connect
>>>>> different devices. If you have e.g. a thermometer or barimeter that speaks
>>>>> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
>>>>> bytes of data), you can mix and match that with max1162 devices, once
>>>>> you have added support for the thermometer or barimeter to your driver.
>>>>
>>>> Which driver, the gyroadc driver ? That's only an ADC driver, it
>>>
>>> The gyroadc driver.
>>>
>>>> shouldn't be aware of any temperature/pressure sampling or any such
>>>> stuff IMO. What you suggest would result in overbloating the gyroadc
>>>> driver with support for all sorts of ADCs connected to it's channels
>>>> and providing all sorts of information, which I think would violate
>>>> the functional separation in the driver model.
>>>
>>> If the need ever arises, it can be turned into a "gyroadc" framework,
>>> and the actual sensor drivers can be enhanced to support the gyroadc
>>> driver, in addition to regular SPI master drivers?
>>
>> So basically we'd have a gyroadc bus type etc ?
>
> Yep.
>
>>> Something for the far future, of course ;-)
>>> For now we can limit it to the 3 types of devices officially supported by
>>> the GyroADC hardware.
>>
>> I'm fine with this, but this doesn't answer my question about
>> duplication. The gyroadc driver would have to check whether all
>> channels are configured to the same device type then ? Doesn't
>> seem quite right.
>
> Yes, describing the real topology in DT means the driver has to verify that
> all existing child nodes contain the same sensor type.
Now that you put it this way, it does make sense. Thanks.
V4 is out btw.
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
