[PATCH v3 1/2] drm/bridge: add Silicon Image SiI9234 driver

[Krzysztof Kozlowski]

On Mon, Sep 11, 2017 at 3:39 PM, Marek Szyprowski
<m.szyprowski at samsung.com> wrote:
> Hi Krzysztof,
>
>
> On 2017-09-11 15:18, Krzysztof Kozlowski wrote:
>>
>> On Mon, Sep 11, 2017 at 1:42 PM, Maciej Purski <m.purski at samsung.com>
>> wrote:
>>>>>
>>>>> +       ctx->client[I2C_MHL] = client;
>>>>> +
>>>>> +       ctx->client[I2C_TPI] = i2c_new_dummy(adapter, I2C_TPI_ADDR);
>>>>> +       if (!ctx->client[I2C_TPI]) {
>>>>> +               dev_err(ctx->dev, "failed to create TPI client\n");
>>>>> +               return -ENODEV;
>>>>> +       }
>>>>> +
>>>>> +       ctx->client[I2C_HDMI] = i2c_new_dummy(adapter, I2C_HDMI_ADDR);
>>>>> +       if (!ctx->client[I2C_HDMI]) {
>>>>> +               dev_err(ctx->dev, "failed to create HDMI RX client\n");
>>>>> +               goto fail_tpi;
>>>>> +       }
>>>>> +
>>>>> +       ctx->client[I2C_CBUS] = i2c_new_dummy(adapter, I2C_CBUS_ADDR);
>>>>> +       if (!ctx->client[I2C_CBUS]) {
>>>>> +               dev_err(ctx->dev, "failed to create CBUS client\n");
>>>>> +               goto fail_hdmi;
>>>>> +       }
>>>>
>>>> You are using i2c_smbus_* calls. Why not converting to regmap_i2c? It is
>>>> preferred interface.
>>>
>>>
>>> According to my search, there are only 5 drivers, which use regmap_i2c
>>> and
>>> none of them in drm. If you think that it is really needed, could you
>>> please
>>> explain
>>> why?
>>
>> There are slightly more than five drivers:
>>
>> $ git grep regmap_init_i2c | wc -l
>> 352
>>
>> ... and even more using regmap interface in generic (not i2c).
>>
>> I think this is really wanted because for free you get:
>> 1. locking,
>> 2. proper locking - with lockdep and nested mutexes :),
>> 3. caching of accesses,
>> 4. handling of endianness,
>> 5. optionally a trivial way of handling interrupts (regmap_irq_chip),
>>
>> Also this brings unified interface for most of the drivers regardless
>> of protocol used beneath (I2C, SPI and even MMIO but for simple cases
>> MMIO might not have much sense). This last argument actually brings
>> the most benefit for me because it abstracts driver from trivial I2C
>> implementation and it could allow even easy code-reuse for e.g. SPI
>> version.
>
>
> Regmap make sense for multi-function chips, which require more than one
> client driver (so proper locking is important). It also makes sense if
> the chip is produced in more than one flavor (like i2c, spi, MMIO, etc).
>
> None of the above takes place in this case... So in case of this driver
> using regmap is IMHO an over-engineering.

Good points and true that setting up regmap requires some more code
around probe. However it will remove half or even more of your
interrupt handler. Less driver specific code, more generic code used.
Also current code around I2C (sii9234_writeb() etc) is not thread
safe... which for now is not a problem as the only entrance point is
the interrupt. Maybe it's fine then to use existing implementation
especially if you do not see any future enhancements to the driver.

Best regards,
Krzysztof