Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Mar 24, 2021 at 06:07:17PM +0800, Yicong Yang wrote:
> On 2021/3/23 0:59, Andy Shevchenko wrote:
> > On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote:

...

> > Missed mod_devicetable.h.
> > Probably missed property.h, but not sure.>
> 
> i think this has been included implicitly as the module compilation worked well.

The rule of thumb is to include headers you are direct user of. The implicit
inclusion is allowed if and only if there is a guarantee by explicit inclusion
that an implicit one will be included no matter what. I don't remember we have
such guarantee for mod_devicetable.h.

On top of that, it's performance wise to explicitly include, otherwise it's an
additional burden for compiler and thus on electrical plant and hence not
environment friendly. And all this for peanuts.

...

> >> +#define HISI_I2C_STD_SPEED_MODE		0x0
> >> +#define HISI_I2C_FAST_SPEED_MODE	0x1
> >> +#define HISI_I2C_HIGH_SPEED_MODE	0x2
> > 
> > Why not plain decimal numbers?
> 
> it's something will be written to the register, so i make them
> hexadecimal to better corresponding to the register fields.

Are they bits? No. Why hex numbers? Please, give a better justification.

...

> >> +	ctlr->bus_freq_hz = t.bus_freq_hz;
> > 
> >> +	ctlr->scl_fall_time = t.scl_fall_ns;
> >> +	ctlr->scl_rise_time = t.scl_rise_ns;
> >> +	ctlr->sda_hold_time = t.sda_hold_ns;
> > 
> > Why not simply to have the timings structure embedded into hisi_i2c_controller
> > one?
> > 
> 
> not all the fields of the timing structures are needed to be stored, only three
> of them are necessary.

Four as far as I see. But for the sake of standardization I would keep a whole
structure. It will be easier to grep and find users of it (looking into data
structures only).

...

> >> +	ctlr->dev = dev;
> > 
> > Would it make sense to assign aster getting IRQ resource...
> > 
> >> +	ctlr->iobase = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(ctlr->iobase))
> >> +		return PTR_ERR(ctlr->iobase);
> >> +
> >> +	ctlr->irq = platform_get_irq(pdev, 0);
> >> +	if (ctlr->irq < 0)
> >> +		return ctlr->irq;
> > 
> > ...somewhere here?
> 
> i cannot see any differences and some other drivers also assign the 'dev' before getting IRQ
> resources.
> 
> are there any considerations on this?

Of course. The rule of thumb: be lazy. Why do you assign earlier if
a) there are no users;
b) in between it may bail out with error
?

Give a better justification why it should be there.

...

> >> +	ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, 1000);
> > 
> > HZ_PER_KHZ ?
> 
> the macro is not public. seems it's redundant to have this macro which
> will only be used once.

It will be easier to see how many users of it we have. It's not needed to make
it public right now, but keeping something like

#define HZ_PER_KHZ  1000L

in this file will be helpful.

Give a better justification why it should not be there.

-- 
With Best Regards,
Andy Shevchenko