On 01/10/2017 07:41 PM, Geert Uytterhoeven wrote:
> Hi Marek,
>
> 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 ?
> 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.
--
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
