Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

On Fri, 2016-12-23 at 19:04 +1100, Cyril Bur wrote:
> On Fri, 2016-12-23 at 13:25 +1030, Andrew Jeffery wrote:
> > On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > > +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> > > +	if (!node) {
> > > +		/*
> > > +		 * Should probaby handle this by allocating 4-64k now and
> > > +		 * using that
> > > +		 */
> > > 
> > > +		dev_err(dev, "Didn't find reserved memory\n");
> > 
> > I think this is good enough for the moment?
> > 
> 
> Yep, would you prefer I remove the comment? My userspace daemon
> wouldn't handle a buffer smaller than flash so even if we wrote it we
> couldn't use it.

I think removing the comment is the right way to go.

> 
> > > +static int lpc_ctrl_remove(struct platform_device *pdev)
> > > +{
> > > > +	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> > > 
> > > +
> > > +	misc_deregister(&lpc_ctrl->miscdev);
> > 
> > It feels like there should be a devm_misc_register()...
> > 
> 
> Uh sorry?

Just a passing comment on the misc_* interfaces. Nothing to worry
about.

> 
> > > > > > +	lpc_ctrl = NULL;
> > > +
> > > > +	return 0;
> > > 
> > > +}
> > > +
> > > +static const struct of_device_id lpc_ctrl_match[] = {
> > > +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> > 
> > Another issue I meant to mention on 4/5 was we'll need bindings
> > documentation for these compatible strings.
> > 
> > It's probably also worth adding the AST2500.
> > 
> 
> True,

Mainly because we should be targeting this at mainline, not the OpenBMC
kernel tree[1]:

    Once you are sure a driver needs to be written, you should develop
    and test the driver, before sending it upstream to the relevant
    maintainers. You should feel welcome to cc the OpenBMC list when
    sending upstream, so other kernel developers can provide input where
    appropriate. Be sure to follow the upstream development process.

    In the past patches underwent 'pre-review' on the OpenBMC mailing
    list. While this is useful for developers who are not familiar with
    writing kernel code, it has lead to confusion about the upstreaming
    process, so now we do all of our development in the community. 

[1] https://github.com/openbmc/docs/blob/master/kernel-development.md#developing-a-new-driver

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]