Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/16 22:45, Andy Shevchenko wrote:
> On Fri, 2014-05-16 at 21:37 +0800, Li, Aubrey wrote:
>> On 2014/5/16 15:04, Andy Shevchenko wrote:
>>> On Fri, 2014-05-16 at 07:29 +0800, Li, Aubrey wrote:
>>>> On 2014/5/16 0:11, Mika Westerberg wrote:
>>>>> On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
>>>>>> On 2014/5/15 22:53, Andy Shevchenko wrote:
>>>>>>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
>>>>>>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
>>>>>>>>> Changes since v1:
>>>>>>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
>>>>>>>>> - NULL checks for clk_name allocation in acpi_lpss.c
>>>>>>>>>
>>>>>>>>> This combines two patch sets for LPSS that I had already send for
>>>>>>>>> review separately. They conflicted with each other.
>>>>>>>>>
>>>>>>>>> The first two patches will fix a problem were the context of the
>>>>>>>>> private LPSS registers is lost when entering D3. The last two will add
>>>>>>>>> support for the M/N dividers on LPSS by adding a new basic clock type
>>>>>>>>> for fractional dividers. The UART driver needs support for it in order
>>>>>>>>> to get clock rates that suit the requested baud rates.
>>>>>>>>
>>>>>>>> The major issue in my mind is, this proposal makes a couple between I2C
>>>>>>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
>>>>>>>
>>>>>>> acpi_lpss driver creates platform devices for each found and enumerated
>>>>>>> device.
>>>>>>
>>>>>>> If there no acpi_lpss enabled the drivers work as supposed without it.
>>>>>>
>>>>>> This is not true.
>>>>>
>>>>> The drivers work fine on non-LPSS platform. If you make them depend on
>>>>> acpi_lpss, you break that.
>>>>>
>>>>
>>>> People don't know the relationship between LPSS driver and I2C/HSUART,
>>>> there is nowhere to describe that. If LPSS driver is unchecked, they
>>>> will encounter a weird issue which is very hard to figure out what's
>>>> going on.
>>>
>>> Besides this discussion is off the topic, I could say that LPSS drivers
>>> are kinda optional (we won't enforce user to use them) even on some
>>> systems where they are present. Relationship is provided by the proper
>>> kernel configuration.
>>>
>> It is optional previously but definitely not optional after this patch.
> 
> I'm sorry, I didn't clearly see what part in this patchset prevents
> this.

That's exactly what I worry about. It's very hard to figure it out.

> 
>> The user who uses I2C designeware driver without LPSS now, I2C won't
>> work properly on Asus T100.
> 
> In that case you specifically enable this driver in the kernel
> configuration. T100 may require that driver to be fully functional.

Is it better to rephrase the help information to give some hints to the
user?

Thanks,
-Aubrey

> 
>>
>> The proper configuration leads to a question, why a completed I2C
>> controller driver doesn't work properly without another LPSS driver. I
>> worry about this is hard to maintain in future.
>>
>> Do we have a platform configuration to specify LPSS is needed?
>>
>> BTW, do we have a system with I2C and HSUART but without LPSS?
> 
> I believe we have.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/