Re: [PATCH 2/2] i3c: i3c-hub: Add Renesas RG3MxxB12A1 I3C HUB driver

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Steven,

> Apologize for the late reply. I am out of work for a long time
> because of my personal emergency. I am back at work now.

No problem at all; Sorry to hear it, I hope things are okay now. Thanks
for getting back to me though!

> We are working on the following items after the first submit of the
> patch:
> 1. Modify the binding file to make it focused on the device
> description instead of driver description.
> 2. Fix the codes which doesn't following the kernel development
> rules.
> 3. Re-implement the SMBus Agent function with IBI mechanism instead
> of polling. 
>    SMBus Agent function requires the driver to check the status
> register of the I3C hub. Polling the status register over I3C bus
> exhausts too much bus resources. I3C Hub supports reporting status
> changes with IBI. This shall be supported in the driver.
> 4. Remove the i2c slave-mqueue module. 
>    The slave-mqueue module is used to provide a user interface for
> SMBus Agent slave function. But it has not been included in upstream.
> 
> The first two items have been finished. And we are working on the #3
> and #4. 

OK, sounds good. I have a few structural suggestions; some of these have
been covered by others' reviews too, but to summarise:

Specificity: it looks like you're proposing this as a generic i3c hub
driver, but I don't think that's the case; as far as I can tell, it's a
Renesas-specific design (and IO interface), so you want to reflect that
in the naming & config options.

which leads to:

Binding: the driver cannot match on the I3C hub class BCR, as that
doesn't specify any sort of register/behavioural interface. If another
hardware hub was implemented, using the same class BCR, this driver
would conflict. You'll need to match on vendor/device IDs instead.

Existing infrastructure: you're re-implementing a few common bits of
kernel infrastructure with this change; for example the LDO settings can
use the regulator devices instead. Is this included in your (2) point
above?

Configuration: as you've noted, the run-time configuration of the device
doesn't belong in the device tree binding in most cases. In fixing
those, there are a couple of approaches:

 * when moving to existing infrastructure (for example, the regulator 
   and/or pinctrl devices), the consumers of those devices can request
   the correct configuration anyway, so you don't need to worry about
   the specific configuration chosen

 * for other runtime things, such as the downstream port configuration
   (in i3c mode vs. SMBus Agent mode) may be better exposed via configfs

 * for things that do belong as new properties in the device tree, you
   want numeric values (with an appropriately named property) rather
   than strings.

 * there may be other configuration settings that don't sit well in
   above categories, best to discuss those here.

SMBus Agent i2c interface: the controller side looks fine, but the
device side should be implemented as i2c_slave_event() events on the
same controllers rather than the i2c-mqueue device (which isn't
upstream, and doesn't look likely to be).

Happy to provide specific feedback as you go, but it's the maintainers
(Alexandre and Krzysztof) that will have the final say in design and
style decisions.

Cheers,


Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c