Re: [PATCH v4 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

Wolfram,

Thanks for your review!

On Mon, Apr 8, 2013 at 3:26 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:

> I'd like to have the bindings more generic. They should allow for n
> possible masters IMO. It doesn't need to be implemented right now, but
> it should be possible to add that later.

Done.  Left code as only handling one other master (as you suggested)
since it has been tested that way and there are no known users with
more than one other master.  Code will give an error if it encounters
a device tree with more than one master.


> I wonder about a more generic name. i2c-arb-gpio-challenge.* maybe?

Done.


> I'd like to drop the specific terms of AP and EC and just talk about
> multiple masters.

Done.


> An array based approach like in the i2c-mux-gpio driver would be more
> generic. Just mention that the driver only supports 2 entries at the
> moment.

Done.  Mentioned that some drivers may only support one other master
(rather than describing the current state of the kernel code) since
bindings should make sense even outside of a single driver, I think.


> Grant, I assume it is okay to introduce these generic bindings?

You are thinking that Grant wouldn't like the names I chose, or that
bindings of this type might not be OK?  The scheme as it stands is
pretty close to what Grant suggested himself here
<https://patchwork.kernel.org/patch/1877311/>.

I did drop the "bus-arbitration-" prefix from the ones he wrote up
since it seemed redundant to have that prefix on all properties of a
node that's only for bus arbitration anyway.  I can add them back in
if you think it's better.  I think Grant only had that prefix in his
proposed bindings because that was the name of the properties in
Naveen's patch (where the bus arbitration was bolted on to the i2c bus
and thus needed differentiation).


>> +config I2C_ARBITRATOR_CROS_EC
>> +     tristate "GPIO-based I2C arbitrator used on exynos5250-snow"
>> +     depends on GENERIC_GPIO && OF
>> +     help
>> +       If you say yes to this option, support will be included for an
>> +       I2C multimaster arbitration scheme using GPIOs that is used in
>> +       the Samsung ARM Chromebook (exynos5250-snow).
>> +
>> +       This driver can also be built as a module.  If so, the module
>> +       will be called i2c-arbitrator-cros-ec.
>> +
>
> This text could be more generic then, too.

Done.


>> +     /* We only support probing from device tree; no platform_data */
>> +     if (WARN_ON(!np))
>> +             return -ENODEV;
>
> Too much WARN_ON for my taste.

Sure.  I've removed them all and put dev_err() messages everywhere as
other mux drivers seem to.  I'd been of the mindset that there was no
need for verbose error messages and extra complexity for uncommon
cases like this, but I can see the benefit of describing exactly what
went wrong in the kernel log.


Please let me know if there's any other fixes needed.

-Doug